policy: Handle failure due to invalid semver range #172

darkowlzz · 2021-09-20T13:00:59Z

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 branch
after 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.

stefanprodan

LGTM

Thanks @darkowlzz 🥇

squaremo · 2021-09-20T13:14:43Z

controllers/imagepolicy_controller.go

@@ -148,6 +148,20 @@ func (r *ImagePolicyReconciler) Reconcile(ctx context.Context, req ctrl.Request)

 	var err error


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 ~~loop~~block a few lines below.

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?

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.

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.

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.

Yeah, adding that. Thanks for digging into it.

squaremo · 2021-09-20T13:50:51Z

controllers/imagepolicy_controller.go

@@ -148,6 +148,20 @@ func (r *ImagePolicyReconciler) Reconcile(ctx context.Context, req ctrl.Request)

 	var err error


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.

darkowlzz · 2021-09-20T14:07:26Z

Adding a test for the PolicyFromSpec() fix as well, please don't merge it.

darkowlzz · 2021-09-20T14:15:12Z

Added a test for PolicyFromSpec() in the factory tests to ensure that it returns a nil checkable Policer. This is ready now 🙂

squaremo · 2021-09-20T14:45:26Z

controllers/imagepolicy_controller.go

@@ -148,6 +148,20 @@ func (r *ImagePolicyReconciler) Reconcile(ctx context.Context, req ctrl.Request)

 	var err error


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.

I'll just move this error before the block that fetches the latest tag with a different name.

Changed my mind, just removing the first declaration 😅

squaremo

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>

darkowlzz · 2021-09-20T15:13:47Z

This is ready from my side.

stefanprodan approved these changes Sep 20, 2021

View reviewed changes

squaremo reviewed Sep 20, 2021

View reviewed changes

squaremo self-requested a review September 20, 2021 13:47

squaremo requested changes Sep 20, 2021

View reviewed changes

darkowlzz force-pushed the main-invalid-semvar-policy branch from 17c2981 to d03c7f8 Compare September 20, 2021 14:13

darkowlzz force-pushed the main-invalid-semvar-policy branch 2 times, most recently from ceb2abf to 6e8bafb Compare September 20, 2021 14:17

squaremo reviewed Sep 20, 2021

View reviewed changes

squaremo approved these changes Sep 20, 2021

View reviewed changes

darkowlzz added 2 commits September 20, 2021 20:41

PolicyFromSpec: return nil on error

ae1f427

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>

darkowlzz force-pushed the main-invalid-semvar-policy branch from 6e8bafb to ae1f427 Compare September 20, 2021 15:13

squaremo merged commit 327a6ea into fluxcd:main Sep 21, 2021

darkowlzz deleted the main-invalid-semvar-policy branch September 21, 2021 14:00

darkowlzz mentioned this pull request Oct 5, 2021

policy: Handle failure due to invalid semver range #176

Merged

darkowlzz mentioned this pull request Oct 20, 2021

REQUEST: New membership for @darkowlzz fluxcd/community#128

Closed

5 tasks

pjbgf mentioned this pull request Mar 19, 2022

image reflector crashlooping #168

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

policy: Handle failure due to invalid semver range #172

policy: Handle failure due to invalid semver range #172

darkowlzz commented Sep 20, 2021

stefanprodan left a comment

squaremo Sep 20, 2021 •

edited

Loading

squaremo Sep 20, 2021

squaremo Sep 20, 2021

darkowlzz Sep 20, 2021

squaremo Sep 20, 2021

darkowlzz Sep 20, 2021

squaremo Sep 20, 2021

darkowlzz commented Sep 20, 2021 •

edited

Loading

darkowlzz commented Sep 20, 2021 •

edited

Loading

squaremo Sep 20, 2021

darkowlzz Sep 20, 2021

darkowlzz Sep 20, 2021

squaremo left a comment

darkowlzz commented Sep 20, 2021

		@@ -148,6 +148,20 @@ func (r *ImagePolicyReconciler) Reconcile(ctx context.Context, req ctrl.Request)

		var err error

policy: Handle failure due to invalid semver range #172

policy: Handle failure due to invalid semver range #172

Conversation

darkowlzz commented Sep 20, 2021

stefanprodan left a comment

Choose a reason for hiding this comment

squaremo Sep 20, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

darkowlzz commented Sep 20, 2021 • edited Loading

darkowlzz commented Sep 20, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

squaremo left a comment

Choose a reason for hiding this comment

darkowlzz commented Sep 20, 2021

squaremo Sep 20, 2021 •

edited

Loading

darkowlzz commented Sep 20, 2021 •

edited

Loading

darkowlzz commented Sep 20, 2021 •

edited

Loading