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

fix!: change the default cluster issuer, remove the namespace variable, hardcode the Helm release name, add workload identity support #65

Merged
merged 8 commits into from
Jan 18, 2024

Conversation

lentidas
Copy link
Contributor

@lentidas lentidas commented Dec 21, 2023

Description of the changes

The main changes of this PR are the following:

  • fix!: hardcode the release name to remove the destination cluster

    I found out that Argo CD passes the name of the application as a value to set the Helm chart. This means that all the templating that used { $.Release.Name } would resolve to the name given to Argo CD application.

    In a multicluster deployment, using a single Argo CD, the names of the applications must be different. We solved that by appending the cluster name to the default application name when deploying on different clusters than in-cluster. However, this resulted in multiple problems for deployments that depended on the name of the application being static, so this solves that.

    This is a breaking change because sometimes this requires an application to be deleted and recreated.

  • fix!: remove the namespace variable

    We found out that on multiple modules we were referencing resources from other modules using their default namespaces and this was hardcoded. In case someone decided to change the namespace of deployment of a certain application, this could break the way modules work with each other.

    We've decided that in order to avoid undefined behaviors caused by overloading this variable, it would be best to remove it entirely.

  • fix!: remove the ArgoCD namespace variable

    Since we are hardcoding the namespace variable on all modules, the variable to set the ArgoCD namespace will no longer be needed as well.

  • feat(aks): add support to use workload identity for storage auth

    This requires a chart version bump to at least v12.16.2. This will be done with a different PR.

  • Change the default cluster issuer to the one that is in fact always created by the cert-manager module.

⚠️ Do a Rebase and merge

Breaking change

  • Yes (in the module itself): because of the removal of the namespace and argocd_namespace variables and the fact that overloading the release name can force a recreation of the application.

Tests executed on which distribution(s)

  • AKS (Azure)
  • EKS (AWS)
  • SKS (Exoscale)

I found out that Argo CD passes the name of the application as a value to set the Helm chart. This means that all the templating that used `{ $.Release.Name }` would resolve to the name given to Argo CD application.

In a multicluster deployment, using a single Argo CD, the names of the applications must be different. We solved that by appending the cluster name to the default application name when deploying on different clusters than `in-cluster`. However, this resulted in multiple problems for deployments that depended on the name of the application being static, so this solves that.

This is a breaking change because sometimes this requires an application to be deleted and recreated.
We found out that on multiple modules we were referencing resources from other modules using their default namespaces and this was hardcoded. In case someone decided to change the namespace of deployment of a certain application, this could break the way modules work with each other.

We've decided that in order to avoid undefined behaviors caused by overloading this variable, it would be best to remove it entirely.
@lentidas lentidas self-assigned this Dec 21, 2023
@lentidas lentidas requested a review from a team as a code owner December 21, 2023 19:15
@lentidas lentidas force-pushed the fix/release_name_namespace branch from 967f6e5 to d2bcfed Compare December 21, 2023 19:15
@lentidas lentidas marked this pull request as draft December 21, 2023 19:38
@lentidas lentidas force-pushed the fix/release_name_namespace branch from 5cca64e to c3cb586 Compare January 12, 2024 08:47
@lentidas lentidas marked this pull request as ready for review January 12, 2024 08:49
Since we are hardcoding the namespace variable on all modules, the variable to set the ArgoCD namespace will no longer be needed as well.
@lentidas lentidas force-pushed the fix/release_name_namespace branch from a28ee15 to b93b8d2 Compare January 12, 2024 12:51
@lentidas lentidas changed the title fix!: change the default cluster issuer, remove the namespace variable and hardcode the Helm release name fix!: change the default cluster issuer, remove the namespace variable, hardcode the Helm release name, add workload identity support Jan 18, 2024
…ation

This requires a chart version bump to at least v12.16.2. This will be done with a different PR.
jbarascut
jbarascut previously approved these changes Jan 18, 2024
@lentidas lentidas merged commit 28f2129 into main Jan 18, 2024
@lentidas lentidas deleted the fix/release_name_namespace branch January 18, 2024 15:54
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.

2 participants