argoproj / argo-cd

Declarative Continuous Deployment for Kubernetes
https://argo-cd.readthedocs.io
Apache License 2.0
16.89k stars 5.13k forks source link

Allow explicit disable of autosync for applications #10070

Open maartengo opened 2 years ago

maartengo commented 2 years ago

What is the current behavior

Configuring an application to have no automated property within the syncPolicy property will turn autosync off once. This will not however enforce that autosync is off. You can manually turn autosync on through the ArgoCD website and it will not be automatically disabled again.

Expected behavior

There should be a way to explicitly turn autosync off. If a user then turns it on through the website, it should (depending on the autosync policy of the parent application) be automatically turned off again.

Perhaps an enabled flag would make sense here, instead of just not configuring automated which is the current way.

  syncPolicy:
    automated:
      enabled: false

Other info

Version: v2.4.7+81630e6 Helm: v3.8.1+g5cb9af4

RomainBelorgey commented 1 year ago

Would love also to have that to help configure syncPolicy when using applicationSet and have a way to remove the syncing of a specific app.

jkurek1 commented 1 year ago

+1

fabioaraujopt commented 1 year ago

This is really needed.

crenshaw-dev commented 1 year ago

What templating tool(s) need this? I believe Helm is powerful enough to handle the current disable mechanism. And the ApplicationSet controller isn't currently powerful enough to handle templating non-string fields (i.e. booleans like this one). So which templating tools are we solving this problem for?

crenshaw-dev commented 1 year ago

@alexec have the Workflows folks been happy with the decision to use StringOrBoolean fields to support templating? We could consider doing that to make this proposal work for ApplicationSets.

maartengo commented 1 year ago

Helm can indeed handle the part of don't configure the property if you want it off. But this isn't a templating problem. The problem is that there is a one-time disable mechanism; not setting the property. But that does not prevent users from enabling the property through the website. The disable isn't enforceable at this moment because Argo doesn't detect it as out-of-sync. So long as I can use a mechanism to automatically disable autosync, and keep it disabled, then I'm content.

crenshaw-dev commented 1 year ago

Ahh. Basically you need the parent App to control the auto-sync on/off mechanism (whatever it might be).

Is auto-syncing a parent app sufficient for your use case? Seems like there will be a moment of time where the child app's configuration is applied before the parent app's preference overrides it. Is that gap okay?

richardjennings-fc commented 1 year ago

This allows for an ApplicationSet generator to specify if AutoSync should be enabled on e.g. a Cluster basis. We are not able to control the structure of the yaml, e.g. autosync: {} using the templating available in an ApplicationSet. To disable it using configuration from a generator we need an explicit property such as disabled: {{ autosyncDisabled }}

crenshaw-dev commented 1 year ago

This allows for an ApplicationSet generator to specify if AutoSync should be enabled

As it's written, I'm not sure the proposal (or PR) would accomplish this, because the ApplicationSet controller is currently incapable of writing boolean values into a template. We could theoretically make this possible by either 1) modifying the ApplicationSet controller to write booleans or 2) making the disabled field a string instead of a boolean.

But either way, I'm not sure we're properly solving the problem. If the problem is that we want the ApplicationSet controller to ensure that the disabled flag always has the desired value, I don't think it can fulfill that role. If, for example, the ApplicationSet controller is slow or broken or encounters an error while generating parameters, then the user-overridden disabled value will "stick" indefinitely. The only sure way to keep the desired disabled value is to prevent the user from modifying the Application in the first place (by controlling both the UI [via RBAC] and [if managed via gitops] the git/helm source of truth for the ApplicationSet).

crenshaw-dev commented 1 year ago

Ah! I see that your use case is different from @maartengo. You're using ApplicationSets to set different sync options for different apps, not as an enforcement mechanism. I think the current PR almost gets you that. But we still gotta get the ApplicationSet controller to handle that field.

mrsimo commented 1 year ago

We have a similar use case to @richardjennings-fc. We'd like to be able to conditionally disable auto sync on generated Applications from an ApplicationSet. In our case we have a race condition when removing one of the Appliaction created by the ApplicationSet, and the best solution we can imagine involves first disabling auto sync and then doing the proper deletion.

We're going to try the skip-reconcile annotation, because it seems like it'll work in this use case, but it has so many warnings and caveats about being an alpha feature that it's a bit discouraging and imagine it breaking in a future update.

Thank you!!

debu99 commented 1 year ago

@mrsimo does skip-reconcile work in your side?

mrsimo commented 1 year ago

@debu99 we ended up not using it. We needed to prevent syncs while doing some cleaning up before destroying a whole environment (we saw edge cases when syncs got triggered while cleanup was happening), but with the skip-reconcile annotation, the Application never gets deleted as it appears disabled, so we would have needed 4 steps: disable app, do the cleanup, enable app (danger of syncs happening), delete app.

We switched to deleting the Application (through the ApplicationSet) without cascade deletion, waiting until it's gone, and then doing everything in the cleaning script we were running anyway, including deleting the Namespace so all the objects would get deleted.

todaywasawesome commented 1 year ago

Another easy way to do this is just add a syncwindow to disallow specific applications.

debu99 commented 1 year ago

Another easy way to do this is just add a syncwindow to disallow specific applications.

Thanks for the idea, but unfortunately we want to use it under applicationSet to disable one application without impact others

wofr commented 10 months ago

Ah! I see that your use case is different from @maartengo. You're using ApplicationSets to set different sync options for different apps, not as an enforcement mechanism. I think the current PR almost gets you that. But we still gotta get the ApplicationSet controller to handle that field.

Is there any update on this. I do use ApplicatonSet with an list-generator and I would like to disable/enable the automated sync via templating. I saw there are features like https://argo-cd.readthedocs.io/en/stable/operator-manual/applicationset/Controlling-Resource-Modification/ but this not totally addresses the issue.

didlawowo commented 3 months ago

any news about this ?

rouke-broersma commented 3 months ago

@crenshaw-dev The issue is that there is no way to enforce autosync off through gitops. If someone can enable autosync through the CLI/webui or through kubernetes there is nothing to turn off autosync again through gitops.

This isn't strictly true, we used to enforce it by setting automated: null and this seemed to work well enough. However I am experimenting with server side diff and because of the type of automated being checked in server side diff, this no longer works (unmanaged fields are ignored as far as I understand, so the server-side value is taken and merged in, instead of the omission of the value in the request).

Error:

Failed to compare desired state to live state: failed to calculate diff: error calculating server side diff: serverSideDiff error: error running server side apply in dryrun mode for resource Application/argocd: Application.argoproj.io "argocd" is invalid: spec.syncPolicy.automated: Invalid value: "null": spec.syncPolicy.automated in body must be of type object: "null"

This means with server side diff it is no longer possible to control the disabled value of auto-sync.

Example:

Setting automated to null used to work, this value would be discarded after apply of course, but would overwrite the manually set automated:

apiVersion: argoproj.io/v1alpha1
kind: Application
metadata:
  annotations:
    argocd.argoproj.io/tracking-id: 'gitops:argoproj.io/Application:argocd/argocd'
  finalizers:
    - resources-finalizer.argocd.argoproj.io
  name: argocd
  namespace: argocd
spec:
  destination:
    name: in-cluster
    namespace: argocd
  project: default
  revisionHistoryLimit: 0
  source:
    path: apps/argocd
    repoURL: 'https://github.com/broersma-forslund/homelab.git'
    targetRevision: HEAD
  syncPolicy:
    automated: null
    syncOptions:
      - FailOnSharedResource=true

With server side diff I now have to generate my Apps like this:

apiVersion: argoproj.io/v1alpha1
kind: Application
metadata:
  annotations:
    argocd.argoproj.io/tracking-id: 'gitops:argoproj.io/Application:argocd/argocd'
  finalizers:
    - resources-finalizer.argocd.argoproj.io
  name: argocd
  namespace: argocd
spec:
  destination:
    name: in-cluster
    namespace: argocd
  project: default
  revisionHistoryLimit: 0
  source:
    path: apps/argocd
    repoURL: 'https://github.com/broersma-forslund/homelab.git'
    targetRevision: HEAD
  syncPolicy:
    syncOptions:
      - FailOnSharedResource=true

After enabling auto-sync and self-heal through the webui the live manifest is:

apiVersion: argoproj.io/v1alpha1
kind: Application
metadata:
  annotations:
    argocd.argoproj.io/tracking-id: 'gitops:argoproj.io/Application:argocd/argocd'
    kubectl.kubernetes.io/last-applied-configuration: >
      {"apiVersion":"argoproj.io/v1alpha1","kind":"Application","metadata":{"annotations":{"argocd.argoproj.io/tracking-id":"gitops:argoproj.io/Application:argocd/argocd"},"finalizers":["resources-finalizer.argocd.argoproj.io"],"name":"argocd","namespace":"argocd"},"spec":{"destination":{"name":"in-cluster","namespace":"argocd"},"project":"default","revisionHistoryLimit":0,"source":{"path":"apps/argocd","repoURL":"https://github.com/broersma-forslund/homelab.git","targetRevision":"HEAD"},"syncPolicy":{"automated":null,"syncOptions":["FailOnSharedResource=true"]}}}
  creationTimestamp: '2023-10-29T16:22:55Z'
  finalizers:
    - resources-finalizer.argocd.argoproj.io
  generation: 89678
  name: argocd
  namespace: argocd
  resourceVersion: '103306926'
  uid: 37986920-7741-4349-be6c-42bc97306d7b
spec:
  destination:
    name: in-cluster
    namespace: argocd
  project: default
  revisionHistoryLimit: 0
  source:
    path: apps/argocd
    repoURL: 'https://github.com/broersma-forslund/homelab.git'
    targetRevision: HEAD
  syncPolicy:
    automated:
      selfHeal: true
    syncOptions:
      - FailOnSharedResource=true

And the diff after a hard refresh is empty, so auto-sync and auto-heal are not disabled.

PhamQuang-512 commented 1 week ago

any updated?

maartengo commented 1 week ago

One workaround is to use syncPolicy: {}, but this only works if you have no policies except the automated one.

maartengo commented 1 week ago

https://github.com/argoproj/argo-cd/issues/10070#issuecomment-1268454363 Ahh. Basically you need the parent App to control the auto-sync on/off mechanism (whatever it might be).

Is auto-syncing a parent app sufficient for your use case? Seems like there will be a moment of time where the child app's configuration is applied before the parent app's preference overrides it. Is that gap okay?

I apparently never replied to this, so to answer your question @crenshaw-dev; yes that would fix my use case. So long as we have a way to re-sync the disabled state of the automated policy.

A short 'interruption' to the desired state is OK for me, because the user would have decided its OK to sync by enabling the automated option through the UI.

PhamQuang-512 commented 1 week ago

One workaround is to use syncPolicy: {}, but this only works if you have no policies except the automated one.

in my case, i use applicationset to generate applications, so i want to set some condition that only some applications (maybe 50% of the applications generated by applicationset) can be automatically synced.

Adam262 commented 1 week ago

Would also find conditional sync policy by cluster super useful in an ApplicationSet! In the meantime, progressive sync seems like a possible workaround. It did do exactly what I wanted - applicationset.spec.strategy.rollingSync.steps.maxUpdate values allowed conditiional autosync / manual sync / dependent sync by cluster based on the labels I assigned.

Big caveat, I have only tested it with a dummy app, as it is an alpha feature, and just enabling it on the applicationset controller seemed to raise the cluster CPU load in Grafana by about 15x! But will at least track its development - would be a nice production feature.

This is the salient part of the test ApplicationSet spec

apiVersion: argoproj.io/v1alpha1
kind: ApplicationSet
metadata:
  name: hello-world
  namespace: argocd
spec:
  syncPolicy:
    applicationsSync: create-update
    preserveResourcesOnDeletion: true
  generators:
    - clusters:
        selector:
          matchExpressions:
            - key: clusterID
              operator: In
              values:
                - dev
                - staging
                - prod
  strategy:
    type: RollingSync
    rollingSync:
      steps:
        # Sync immediately when an Application is out of sync. 
        # This is effectively an auto-sync (even though auto-sync is disabled under spec.template.spec.syncPolicy)
        # In fact, auto-sync is not allowed when strategy is set to RollingSync
        - maxUpdate: 100% 
          matchExpressions:
            - key: cluster
              operator: In
              values:
                - dev
        # This is equivalent to a manual sync
        - maxUpdate: 0 
          matchExpressions:
            - key: cluster
              operator: In
              values:
                - staging
        # Once the prior step succeeds, 50% of matching Applications will be synced at a time      
        - maxUpdate: 50% 
          matchExpressions:
            - key: cluster
              operator: In
              values:
                - prod
   template:
     metadata:
       name: '{{name}}-hello-world'
       labels:
         cluster: '{{metadata.labels.cluster}}'