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

feat: add ExternalSecret to sync wave #612

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

ckav370
Copy link

@ckav370 ckav370 commented Jul 16, 2024

I want to be able to use a preSync hook to run some database migrations. This has been working fine, until such a time as updating the container secrets which are using the External Secret Operator and CRD to create a k8s secret which the job created by the preSync hook creates mounts.

As per this Github issue #9891, the recommended approach is to use Argo sync waves. However ExternalSecret is not included in one of the predefined kinds and therefore cannot be part of a different "wave" meaning that any secrets defined by ExternalSecrets cannot be synced before the preSync hook as part of the pre Sync wave.

This would allow the ExternalSecret to have an annotation such as the following

argocd.argoproj.io/sync-wave: "-2"

And a preSync hook to have a greater weighted sync wave such as

argocd.argoproj.io/sync-wave: "-1"

And all other resources which are part of the deployment would default to weight of 0 without the need for an annotation

@schlags
Copy link

schlags commented Aug 1, 2024

Isn't this already possible?

apiVersion: external-secrets.io/v1beta1
kind: ExternalSecret
metadata:
  annotations:
    argocd.argoproj.io/hook: PreSync
    argocd.argoproj.io/sync-wave: "-1000"

We already use ExternalSecrets in this fashion. One of our PreSync jobs requires a secret present and this was our strategy to prevent the catch 22 of initial sync not having that secret defined yet.

@ckav370
Copy link
Author

ckav370 commented Aug 2, 2024

Isn't this already possible?

apiVersion: external-secrets.io/v1beta1
kind: ExternalSecret
metadata:
  annotations:
    argocd.argoproj.io/hook: PreSync
    argocd.argoproj.io/sync-wave: "-1000"

We already use ExternalSecrets in this fashion. One of our PreSync jobs requires a secret present and this was our strategy to prevent the catch 22 of initial sync not having that secret defined yet.

Using a PreSync jon in this way means that when the secret is updated, the secret will be recreated. In the case where some PreSync job and the main application uses this secret, this would not be a very robust- especially in the case where the secret refresh failed to pull from the ExternalSecret backend.

Granted this is probably more likely in legacy code bases, but it would not be a solution for my current use case :)

@kostis-codefresh
Copy link
Member

Hello

First of all, looking at the commit on its own I have two comments

  1. The list you modify contains a list of built-in Kubernetes resources. What you are adding is not a built-in resource. Even if that would fix your use case, the next person would then add their own custom CRD and so on. This is not sustainable in the long run (for everybody to add their own resource there)
  2. As the comment says this list is designed on purpose to mimic the default ordering that Helm follows. By editing this list, you break this convention.

However just to take a step backwards. Can you explain what exactly is your use case and why it is not covered by sync waves? And I mean JUST sync waves and not phases/hooks.

However ExternalSecret is not included in one of the predefined kinds and therefore cannot be part of a different "wave" meaning that any secrets defined by ExternalSecrets cannot be synced before the preSync hook as part of the pre Sync wave.

I don't understand this sentence. Could you please attach a minimal example of two resources with just sync waves and explain what Argo CD does versus what you expected to happen?

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