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

nil pointer dereference when user provides invalid cron schedule #19605

Closed
3 tasks done
drigz opened this issue Aug 20, 2024 · 4 comments · Fixed by #20043
Closed
3 tasks done

nil pointer dereference when user provides invalid cron schedule #19605

drigz opened this issue Aug 20, 2024 · 4 comments · Fixed by #20043
Labels
bug Something isn't working component:application

Comments

@drigz
Copy link

drigz commented Aug 20, 2024

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. (caveat below)

Describe the bug

The SyncWindow code has no error checking and fails with a nilpointer deref when the user provides an invalid schedule such as * * * * 2-7. The invalid code is repeated multiple times in application/v1alpha1/types.go:

			schedule, _ := specParser.Parse(w.Schedule)  // returns nil on error
			[...]
			nextWindow := schedule.Next(currentTime.Add(timeZoneOffsetDuration - duration)) // crashes if schedule is nil

In one instance they tried to handle the error but don't check sErr until after the nil deref:

				schedule, sErr := specParser.Parse(w.Schedule)  // returns nil on error
				[...]
				nextWindow := schedule.Next(currentTime.Add(timeZoneOffsetDuration - duration)) // crashes if schedule is nil

				if !nextWindow.Before(currentTime.Add(timeZoneOffsetDuration)) && sErr == nil && dErr == nil { // never reached if sErr != nil
					inactive = append(inactive, w)
				}

You can work around this by copy-pasting all your schedules into the Go Playground to check the format: https://go.dev/play/p/Vfg1RJtIGdA

To Reproduce

Create an AppProject with an invalid schedule:

apiVersion: argoproj.io/v1alpha1
kind: AppProject
metadata:
  name: default
  namespace: argocd
spec:
  clusterResourceWhitelist:
    - group: '*'
      kind: '*'
  destinations:
    - namespace: '*'
      server: '*'
  sourceRepos:
    - '*'
  syncWindows:
    - kind: allow
      schedule: '0 3 * * 1-5'
      duration: 2h
      timeZone: UTC
      applications:
        - '*'
      manualSync: true
    - kind: deny
      schedule: '* * * * 2-7'
      applications:
      - '*'
      clusters:
      - '*prod*'   # Apply to clusters with "prod" in their name

Then try to Sync in the UI.

Expected behavior

User sees an error message explaining why their schedule is invalid, eg end of range (7) above maximum (6): 2-7.

Version

$ argocd version --port-forward-namespace argocd
FATA[0005] Failed to establish connection to 127.0.0.1:39267: EOF
$ kubectl port-forward svc/argo-cd-argocd-server -n argocd 8080:443 &
Forwarding from 127.0.0.1:8080 -> 8080
Forwarding from [::1]:8080 -> 8080
$ argocd login https://localhost:8080
[0000] dial tcp: address https://localhost:8080: too many colons in address

I gave up at this point. I'm using ArgoCD v2.11.5.

Logs

Recovered from panic: runtime error: invalid memory address or nil pointer dereference
goroutine 213 [running]:
runtime/debug.Stack()n  /usr/local/go/src/runtime/debug/stack.go:24 +0x5e
github.com/argoproj/argo-cd/v2/controller.(*ApplicationController).processAppRefreshQueueItem.func1()
        /go/src/github.com/argoproj/argo-cd/controller/appcontroller.go:1475 +0x54
panic({0x36a5fc0?, 0x7204b20?})
        /usr/local/go/src/runtime/panic.go:920 +0x270
github.com/argoproj/argo-cd/v2/pkg/apis/application/v1alpha1.(*SyncWindows).active(0xc002e3fef0, {0xc003ae5d60?, 0x1e?, 0x744d9e0?})
        /go/src/github.com/argoproj/argo-cd/pkg/apis/application/v1alpha1/types.go:2267 +0x1ce
github.com/argoproj/argo-cd/v2/pkg/apis/application/v1alpha1.(*SyncWindows).Active(0x0?)
        /go/src/github.com/argoproj/argo-cd/pkg/apis/application/v1alpha1/types.go:2249 +0x2b
github.com/argoproj/argo-cd/v2/pkg/apis/application/v1alpha1.(*SyncWindows).CanSync(0xc002e3fef0, 0x0)
        /go/src/github.com/argoproj/argo-cd/pkg/apis/application/v1alpha1/types.go:2423 +0x2c
github.com/argoproj/argo-cd/v2/controller.(*ApplicationController).processAppRefreshQueueItem(0xc000d00e00)
        /go/src/github.com/argoproj/argo-cd/controller/appcontroller.go:1608 +0x1605
github.com/argoproj/argo-cd/v2/controller.(*ApplicationController).Run.func3()
        /go/src/github.com/argoproj/argo-cd/controller/appcontroller.go:824 +0x25
k8s.io/apimachinery/pkg/util/wait.BackoffUntil.func1(0x30?)
        /go/pkg/mod/k8s.io/apimachinery@v0.26.11/pkg/util/wait/wait.go:157 +0x33
k8s.io/apimachinery/pkg/util/wait.BackoffUntil(0x0?, {0x50cb3c0, 0xc000f9ff20}, 0x1, 0xc0000cbd40)
        /go/pkg/mod/k8s.io/apimachinery@v0.26.11/pkg/util/wait/wait.go:158 +0xaf
k8s.io/apimachinery/pkg/util/wait.JitterUntil(0x0?, 0x3b9aca00, 0x0, 0x0?, 0x0?)
        /go/pkg/mod/k8s.io/apimachinery@v0.26.11/pkg/util/wait/wait.go:135 +0x7f
k8s.io/apimachinery/pkg/util/wait.Until(0x0?, 0x0?, 0x0?)
        /go/pkg/mod/k8s.io/apimachinery@v0.26.11/pkg/util/wait/wait.go:92 +0x1e
created by github.com/argoproj/argo-cd/v2/controller.(*ApplicationController).Run in goroutine 75
        /go/src/github.com/argoproj/argo-cd/controller/appcontroller.go:823 +0x89b

See also previous report: #16701

@Tchoupinax
Copy link
Contributor

We did meet the same. Changing '* * * * 5-7 to '* * * * 5 fixed the issue. Unfortunately, we need to declare three rules instead of one.

@drigz
Copy link
Author

drigz commented Oct 11, 2024

Changing '* * * * 5-7 to '* * * * 5 fixed the issue

@Tchoupinax I think 0,5-6 might work here as well but best to check with the playground link above.

@crenshaw-dev
Copy link
Member

Fix will be in 2.13.0-rc4 (not sure yet whether we'll backport to earlier versions).

@Tchoupinax
Copy link
Contributor

Tchoupinax commented Oct 14, 2024

@drigz I tested and it works well, thanks!
Well, once you understand that Sunday is 0 and Monday is 1, it is easy.

Maybe we could specify this tip on this page: https://argo-cd.readthedocs.io/en/stable/user-guide/sync_windows
What do you think @crenshaw-dev?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working component:application
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants