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.

Sign up for GitHub

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

Jump to bottom

SC panics with nil pointer dereference #680

Closed
makkes opened this issue Apr 21, 2022 · 4 comments · Fixed by #683
Closed

SC panics with nil pointer dereference #680

makkes opened this issue Apr 21, 2022 · 4 comments · Fixed by #683
Assignees
@cwyl02
Labels
area/helm Helm related issues and pull requests bug Something isn't working good first issue Good for newcomers

Comments

@makkes
Copy link
Member

makkes commented Apr 21, 2022 

panic: runtime error: invalid memory address or nil pointer dereference
[signal SIGSEGV: segmentation violation code=0x1 addr=0x0 pc=0x1c67451]

goroutine 420 [running]:
github.com/fluxcd/source-controller/controllers.(*HelmRepositoryReconciler).notify(0xc0003e4660, 0xc0008da340, 0xc000c01040, {{0xc000618c30, 0x26}, {0x28875e0, 0xc0006f6780}, {0xc0002dbc68, 0x3, 0x3}, ...}, ...)
        github.com/fluxcd/source-controller/controllers/helmrepository_controller.go:258 +0x1b1

This is coming from https://github.com/fluxcd/source-controller/blob/v0.24.0/controllers/helmrepository_controller.go#L258 where SC in fact doesn't check if newObj.Status.Artifact.Size is nil, dereferencing it in any case.

I'm not sure in which situation this field would be nil but I don't see it set in older, working instances of SC, too.
@makkes makkes added the good first issue Good for newcomers label Apr 21, 2022
@hiddeco hiddeco added bug Something isn't working area/helm Helm related issues and pull requests labels Apr 21, 2022
@cwyl02
Copy link
Contributor

cwyl02 commented Apr 21, 2022

I would like to work on this issue @makkes @hiddeco

@makkes
Copy link
Member Author

makkes commented Apr 21, 2022

Thank you @cwyl02.

makkes pushed a commit that referenced this issue Apr 22, 2022
fix panic when HelmRepository's artifact size is nil
b907001 
This fixes the immediate issue of the nil pointer dereference but we
still haven't isolated the actual cause of the size being nil to begin
with. This is ongoing work and as soon as we have boiled that down to
the simplest case we will provide a regression test for that case.

closes #680

Signed-off-by: Max Jonas Werner <[email protected]>
@makkes makkes mentioned this issue Apr 22, 2022
Merged
makkes pushed a commit that referenced this issue Apr 22, 2022
@hiddeco
fix panic when HelmRepository's artifact size is nil
c3d5dac 
This fixes the immediate issue of the nil pointer dereference but we
still haven't isolated the actual cause of the size being nil to begin
with. This is ongoing work and as soon as we have boiled that down to
the simplest case we will provide a regression test for that case.

closes #680

Signed-off-by: Max Jonas Werner <[email protected]>
Co-authored-by: Hidde Beydals <[email protected]>
@hiddeco hiddeco mentioned this issue Apr 22, 2022
Closed
1 task
@cwyl02
Copy link
Contributor

cwyl02 commented Apr 25, 2022

I think the root cause is that HelmRepository created with v1beta1 version comes with Artifact that doesn't include the Size field at all.

Then, when SC(v0.23.0+) reconciles an HelmRepository, it panics at that notify and cannot move on reconciling the object(including the API version). This is fixed in @makkes' PR.

Thus, HelmRepository's artifact size can be nil and can be very common when the user is upgrading from a legacy HelmRepository object.

I don't think additional changes are required.

@makkes
Copy link
Member Author

makkes commented Apr 26, 2022

Great catch @cwyl02. Thanks for your persistence.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@cwyl02 cwyl02

Labels
area/helm Helm related issues and pull requests bug Something isn't working good first issue Good for newcomers
Projects
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

fix panic when HelmRepository's artifact size is nil fluxcd/source-controller
3 participants
@makkes @cwyl02 @hiddeco