Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add module for Azure PG flex server #152

Merged
merged 7 commits into from
Jun 10, 2024

Conversation

Ferdinanddb
Copy link
Contributor

@Ferdinanddb Ferdinanddb commented May 29, 2024

Add a new module to create an Azure Postgres Flexible Server and modify the AKS and Managed Identities modules. I listed the following in the issue associated to this PR.

Modification of the managed identities module

  • Add a azurerm_user_assigned_identity that will be used by the e2e tests

Modification of the AKS module

  • Add a azurerm_federated_identity_credential to map the newly created user assigned identity to the keda-operator Kubernetes service account.

New module for Azure Postgres Flexible Server

  • Add a azurerm_postgresql_flexible_server,
  • Add a azurerm_postgresql_flexible_server_active_directory_administrator to grant admin role to the newly created user assigned identity,
  • Add a azurerm_postgresql_flexible_server_firewall_rule to allow access to all Azure IPs.
    • I am implementing it that way because the AKS cluster has an Azure IP, so I think it is the way to go, or maybe the range could be more specific? What do you think?
  • Add a azurerm_postgresql_flexible_server_database
    • This database will be used by the e2e tests.

What do you think about this?


EDIT: I decided to add support for basic Postgres authentication, so that the current way of performing actions in the server (creating a table, write to the table, ...) for the e2e tests can persist. But the ScaledObject of the e2e tests will authenticate as the user managed identity added by this PR to interact with the server.

There could be another way to handle this, but it is not as simple:

  • Add a azurerm_postgresql_flexible_server_active_directory_administrator for the service principal used by Terraform,
  • Add a azurerm_postgresql_flexible_server_firewall_rule to add the public IP (or range) of the machines from where the GitHub Actions workflows are executed. This is not necessary if Terraform is being executed from a resource having an Azure IP.
  • Generate SQL queries and run them via null_resource resources + provisioner local-exec to add Azure Managed Identities, see this link for an example of the queries to run. This query must be executed by a user AD admin within Postgres (this is why the resource from the 1st bullet point is needed).
  • With that, it would mean that basic authentication is disabled, and the only way to connect to the server is by using access token, meaning that the following code should be adapted to support authentication using Access Token. This is the code used by the container performing the e2e tests to create a table and perform other actions.

Checklist

  • Commits are signed with Developer Certificate of Origin (DCO)

Related to #151

…aged Identities modules

Signed-off-by: Ferdinand de Baecque <45566171+Ferdinanddb@users.noreply.github.com>
@Ferdinanddb Ferdinanddb requested a review from a team as a code owner May 29, 2024 22:03
Signed-off-by: Ferdinand de Baecque <45566171+Ferdinanddb@users.noreply.github.com>
Signed-off-by: Ferdinand de Baecque <45566171+Ferdinanddb@users.noreply.github.com>
…connect to it and create tables for the e2e test

Signed-off-by: Ferdinand de Baecque <45566171+Ferdinanddb@users.noreply.github.com>
Signed-off-by: Ferdinand de Baecque <45566171+Ferdinanddb@users.noreply.github.com>
terraform/modules/azure/managed_identities/main.tf Outdated Show resolved Hide resolved
terraform/modules/azure/managed_identities/outputs.tf Outdated Show resolved Hide resolved
terraform/main.tf Outdated Show resolved Hide resolved
@JorTurFer
Copy link
Member

Looking really good, some comments inline

Signed-off-by: Ferdinand de Baecque <45566171+Ferdinanddb@users.noreply.github.com>
@Ferdinanddb
Copy link
Contributor Author

@JorTurFer I took your comments in account and adapted the code accordingly :).

Signed-off-by: Ferdinand de Baecque <45566171+Ferdinanddb@users.noreply.github.com>
Copy link
Member

@zroubalik zroubalik left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, great job

@zroubalik zroubalik merged commit 3eaa25e into kedacore:main Jun 10, 2024
4 checks passed
@Ferdinanddb
Copy link
Contributor Author

@zroubalik Thanks for your feedback and merging it!

However, I see that the CICD stage that runs the Terraform apply command failed because of something I did not expect, it seems to be related to the Azure region being used or the quota of the subscription.

In my case, I used the Switzerland North region to test this locally and I only created an AKS cluster and the resources from this module.

Let me know if I can do something regarding this.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants