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

Handle nil obj when processing custom actions #1700

Merged
merged 1 commit into from
Jun 6, 2019

Conversation

dthomson25
Copy link
Member

@dthomson25 dthomson25 commented Jun 5, 2019

If an application has a missing resource, running a custom action CLI command will cause a panic on the CLI. The CLI will make a call to the ManageResource endpoint, and the server will return a nil object that the CLI will try to operate on. This PR addresses the panic by adding a nil check on the CLI. Before the fix, the CLI will produce a stacktrace like the following:

[signal SIGSEGV: segmentation violation code=0x1 addr=0x0 pc=0x1b7f09a]

goroutine 1 [running]:
github.com/argoproj/argo-cd/vendor/k8s.io/apimachinery/pkg/apis/meta/v1/unstructured.(*Unstructured).GetAPIVersion(0x0, 0xc0007e5ad4, 0x9)
    /Users/dthomson/go/src/github.com/argoproj/argo-cd/vendor/k8s.io/apimachinery/pkg/apis/meta/v1/unstructured/unstructured.go:200 +0x3a
github.com/argoproj/argo-cd/vendor/k8s.io/apimachinery/pkg/apis/meta/v1/unstructured.(*Unstructured).GroupVersionKind(0x0, 0x0, 0x0, 0x0, 0x0, 0x0, 0x0)
    /Users/dthomson/go/src/github.com/argoproj/argo-cd/vendor/k8s.io/apimachinery/pkg/apis/meta/v1/unstructured/unstructured.go:396 +0x5b
github.com/argoproj/argo-cd/cmd/argocd/commands.filterResources(0xc000448000, 0xc000368700, 0x18, 0x20, 0x7ffeefbff728, 0xb, 0x7ffeefbff718, 0x7, 0x0, 0x0, ...)
    /Users/dthomson/go/src/github.com/argoproj/argo-cd/cmd/argocd/commands/app.go:1794 +0x107
github.com/argoproj/argo-cd/cmd/argocd/commands.NewApplicationResourceActionsListCommand.func1(0xc000448000, 0xc00038a5a0, 0x1, 0x5)
    /Users/dthomson/go/src/github.com/argoproj/argo-cd/cmd/argocd/commands/app_actions.go:56 +0x2d8
github.com/argoproj/argo-cd/vendor/github.com/spf13/cobra.(*Command).execute(0xc000448000, 0xc00038a550, 0x5, 0x5, 0xc000448000, 0xc00038a550)
    /Users/dthomson/go/src/github.com/argoproj/argo-cd/vendor/github.com/spf13/cobra/command.go:702 +0x2d3
github.com/argoproj/argo-cd/vendor/github.com/spf13/cobra.(*Command).ExecuteC(0xc0004aa480, 0xc0004aad80, 0xc0000d5d10, 0xc0001d5ab0)
    /Users/dthomson/go/src/github.com/argoproj/argo-cd/vendor/github.com/spf13/cobra/command.go:783 +0x2dc
github.com/argoproj/argo-cd/vendor/github.com/spf13/cobra.(*Command).Execute(0xc0004aa480, 0xc0004c1f88, 0xc00009e058)
    /Users/dthomson/go/src/github.com/argoproj/argo-cd/vendor/github.com/spf13/cobra/command.go:736 +0x2b
main.main()
    /Users/dthomson/go/src/github.com/argoproj/argo-cd/cmd/argocd/main.go:14 +0x27```

@dthomson25
Copy link
Member Author

dthomson25 commented Jun 5, 2019

Just thought of another way to address this bug. We can remove any nil objects from the array on the server-side. thoughts @jessesuen?

@codecov
Copy link

codecov bot commented Jun 5, 2019

Codecov Report

Merging #1700 into master will decrease coverage by <.01%.
The diff coverage is 0%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #1700      +/-   ##
==========================================
- Coverage   34.12%   34.12%   -0.01%     
==========================================
  Files          75       75              
  Lines       11331    11333       +2     
==========================================
  Hits         3867     3867              
- Misses       6926     6928       +2     
  Partials      538      538
Impacted Files Coverage Δ
cmd/argocd/commands/app.go 0.77% <0%> (-0.01%) ⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 23ad098...0a9c8dd. Read the comment docs.

1 similar comment
@codecov
Copy link

codecov bot commented Jun 5, 2019

Codecov Report

Merging #1700 into master will decrease coverage by <.01%.
The diff coverage is 0%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #1700      +/-   ##
==========================================
- Coverage   34.12%   34.12%   -0.01%     
==========================================
  Files          75       75              
  Lines       11331    11333       +2     
==========================================
  Hits         3867     3867              
- Misses       6926     6928       +2     
  Partials      538      538
Impacted Files Coverage Δ
cmd/argocd/commands/app.go 0.77% <0%> (-0.01%) ⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 23ad098...0a9c8dd. Read the comment docs.

@jessesuen
Copy link
Member

Just thought of another way to address this bug. We can remove any nil objects from the array on the server-side. thoughts @jessesuen?

We can't, because the nil live objects has meaning. It indicates the resource is missing from the cluster.

@dthomson25 dthomson25 merged commit b16c485 into argoproj:master Jun 6, 2019
@dthomson25 dthomson25 deleted the add-nil-check branch June 6, 2019 01:04
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