2.6 RC1 - Progressive Rollout does not respect SyncWindows

[thober35]

Checklist:

• I've searched in the docs and FAQ for my answer: https://bit.ly/argocd-faq.
• I've included steps to reproduce the bug.
• I've pasted the output of argocd version.

Describe the bug
Using the new Progressive Rollout Feature for ApplicationSets
If I have an active SyncWindow on my application with an explicit deny at all times and make a change in the source (i.e. helm chart)
The syncWindow is not respected and the sync starts right away

To Reproduce

kubectl create namespace argocd
kubectl apply -n argocd -f https://raw.githubusercontent.com/argoproj/argo-cd/v2.6.0-rc1/manifests/install.yaml
# edit deployment argocd-applicationset-controller and add --enable-progressive-rollouts to  spec.containers.command

# add example project with syncWindow and example ApplicationSet with progressiveRollout

apiVersion: argoproj.io/v1alpha1
kind: AppProject
metadata:
  name: my-project
  namespace: argocd
spec:
  description: Example Project
  sourceRepos:
    - '*'
  destinations:
    - namespace: '*'
      server: '*'
  clusterResourceWhitelist:
    - group: '*'
      kind: '*'
  syncWindows:
    - kind: deny
      schedule: '* * * * *'
      duration: 1h
      applications:
        - 'argocd-dev01*'
---
apiVersion: argoproj.io/v1alpha1
kind: ApplicationSet
metadata:
  name: hello-world-nginx
spec:
  generators:
    - list:
        elements:
          - name: "1"
            rolloutWave: "1"
          - name: "2"
            rolloutWave: "2"
          - name: "3"
            rolloutWave: "3"
          - name: "4"
            rolloutWave: "4"
          - name: "5"
            rolloutWave: "5"

  syncPolicy:
    preserveResourcesOnDeletion: true
  strategy:
    type: RollingSync
    rollingSync:
      steps:
        - matchExpressions:
            - key: rolloutWave
              operator: In
              values:
                - "1"
          #maxUpdate: 0 # if undefined or 0, all applications matched are updated together
        - matchExpressions:
            - key: rolloutWave
              operator: In
              values:
                - "2"
        - matchExpressions:
            - key: rolloutWave
              operator: In
              values:
                - "3"
        - matchExpressions:
            - key: rolloutWave
              operator: In
              values:
                - "4"
        - matchExpressions:
            - key: rolloutWave
              operator: In
              values:
                - "5"
          #maxUpdate: 1 # maxUpdate supports both integer and percentage string values
  template:
    metadata:
      name: 'argocd-dev01-hello-world-nginx-{{name}}'
      labels:
        rolloutWave: '{{rolloutWave}}'
    spec:
      project: my-project
      source:
        repoURL: https://github.com/thober35/hello-kubernetes.git
        path: deploy/helm/hello-kubernetes
        targetRevision: main
        helm:
          releaseName: 'hello-kubernetes-{{name}}'
          values: |-
            message: "3"
      destination:
        server: https://kubernetes.default.svc
        namespace: 'demo'
      syncPolicy:
        automated:
          prune: true
          selfHeal: true
        syncOptions:
          - ApplyOutOfSyncOnly=true
          - CreateNamespace=true

Issue seems to appear when directly editing the source repo i.e helm chart change. In this example I change message2 to a hardcoded value.

Expected behavior
Applications are not synced as sync window with explicit deny is active. As soon as sync window is removed, the sync should start.

Screenshots

Version

argocd: v2.6.0-rc1+81e40d5
  BuildDate: 2022-12-19T16:48:52Z
  GitCommit: 81e40d53fe8eee50b00ab38c4b07b34b3dcd6d25
  GitTreeState: clean
  GoVersion: go1.18.9
  Compiler: gc
  Platform: linux/amd64

Logs

Paste any relevant application logs here.

[thober35]

Hey @crenshaw-dev! Do you know if this will make the 2.6 cut? Thanks!

[crenshaw-dev]

@thober35 tbh I doubt it. Too many other things to knock out I'm afraid. :-(

[thober35]

@wmgroot sorry for tagging. do you think this may be covered by your pr #12103. Thanks!

[wmgroot]

Is there a good way I can get notifications about progressive rollout issues?

I'm a little surprised that the sync would be allowed, but would need to understand how SyncWindows gate manual sync actions to understand why it's still working. I agree that it's unlikely this can be resolved before the 2.6 release.

[crenshaw-dev]

@wmgroot I can start adding that label to issues.

My guess is that the application controller doesn't actually enforce sync windows, and that happens at the API server level. But I could be wrong.

[mtougeron]

Hey all, out of curiosity, what's the status of this issue?

[crenshaw-dev]

@mtougeron in need of a PR author.

[mtougeron]

thanks for the update, I appreciate it.

[wmgroot]

Just want to chime in with some more details on the state of this issue. The right way to solve this is likely on the application controller side rather than the applicationset controller side.

I think this may be an oversight with the current implementation of sync windows. SyncWindows are defined on AppProject resources, meaning they should not be able to be bypassed by users if they're set up to not allow manual overrides. We allow users to freely manipulate their own Argo Applications, but restrict access to AppProject resources that require central approval from our Argo maintainers.

As it stands now, a user with access to modify their Argo Application can freely bypass any configured sync window by modifying the status.operationState field to tell the application controller to perform a sync operation.

[crenshaw-dev]

Yep, 100% agree.