-
Notifications
You must be signed in to change notification settings - Fork 67
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
policy: Handle failure due to invalid semver range #172
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
Thanks @darkowlzz 🥇
@@ -148,6 +148,20 @@ func (r *ImagePolicyReconciler) Reconcile(ctx context.Context, req ctrl.Request) | |||
|
|||
var err error |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this shadowed right away? If I'm reading this right, probably worth just removing this declaration as part of this change, for clarity. And using a differently-named var to assign to in the loopblock a few lines below.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
OK I think I see what's going on. The expectation is that the value returned from PolicerFromSpec
either has non-nil policer
or non-nil err
; so if there's an error, the block which accesses policer won't run, and the conditional following that will catch errors from parsing the policy or from applying the policy. All else being correct, this would work fine.
However, policer
is typed as Policer
, whereas the return value is *SemVer
; this type checks, but nil(Policer) != nil(*SemVer)
, so the block conditional on policer != nil
runs after all. Am I right?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
However, policer is typed as Policer, whereas the return value is *SemVer; this type checks, but nil(Policer) != nil(*SemVer), so the block conditional on policer != nil runs after all. Am I right?
Yes I think I am. If I remove the early exit you added, and put fmt.Printf("%s\n", policer)
inside the block conditional on policer != nil
, I can see
[DEBUG] %!s(*policy.SemVer=<nil>)
printed in the output of
go test -v ./controllers/...
(and a rightly failing test). Though the fix here already avoids it crashing, I think it'd be better to fix that underlying problem as well.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, that's seems to be what was happening. And later on when policer.Latest()
is called, it panicked. An interface holding a nil value not equal to nil. The policer nil check will only protect against the situation where the choice is something other than the known choices, but I don't see that'll really ever happen. We should be able to remove the policer check.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it would pay to make PolicerFromSpec
return Policer(nil) values when it gets an error, otherwise this will remain as a trap for future changes.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, adding that. Thanks for digging into it.
@@ -148,6 +148,20 @@ func (r *ImagePolicyReconciler) Reconcile(ctx context.Context, req ctrl.Request) | |||
|
|||
var err error |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
However, policer is typed as Policer, whereas the return value is *SemVer; this type checks, but nil(Policer) != nil(*SemVer), so the block conditional on policer != nil runs after all. Am I right?
Yes I think I am. If I remove the early exit you added, and put fmt.Printf("%s\n", policer)
inside the block conditional on policer != nil
, I can see
[DEBUG] %!s(*policy.SemVer=<nil>)
printed in the output of
go test -v ./controllers/...
(and a rightly failing test). Though the fix here already avoids it crashing, I think it'd be better to fix that underlying problem as well.
Adding a test for the PolicyFromSpec() fix as well, please don't merge it. |
17c2981
to
d03c7f8
Compare
Added a test for |
ceb2abf
to
6e8bafb
Compare
@@ -148,6 +148,20 @@ func (r *ImagePolicyReconciler) Reconcile(ctx context.Context, req ctrl.Request) | |||
|
|||
var err error |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This declaration could still do with removing, it's redundant. I think the shadowing only escapes the linter because policer is a new variable.
I'd also like to avoid the reuse of err
afterward. It made some sense before (it might be the error returned by PolicerFromSpec
), but now it's confusing.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'll just move this error before the block that fetches the latest tag with a different name.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Changed my mind, just removing the first declaration 😅
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I leave my last suggestion at your disposal. Thanks Sunny!
This adds error check after creating a Policer. For alphabetical and numerical policies, the k8s API validates the input data. But for semver policy, there aren't predefined valid values. Adds a test to verify the fix. Signed-off-by: Sunny <darkowlzz@protonmail.com>
Returning nil wrapped in Policy interface along with error returns an interface with a nil value. This can't be used to perform a nil check. To make it safe, return nil value on error. Signed-off-by: Sunny <darkowlzz@protonmail.com>
6e8bafb
to
ae1f427
Compare
This is ready from my side. |
This adds error check after creating a Policer.
For alphabetical and numerical policies, the k8s API validates the
input data. But for semver policy, there aren't predefined valid
values.
Adds a test to verify the fix.
Fixes #171
Reference to the same fix and tests against
reconcilers-dev
branchafter testenv test refactor : https://github.com/darkowlzz/image-reflector-controller/compare/testenv-test-refactor...darkowlzz:invalid-policy-semver-range?expand=1, table test with new test case.