fix: Only set ConstraintTemplate's status.created on success #2208
Conversation
Codecov Report
@@ Coverage Diff @@
## master #2208 +/- ##
==========================================
+ Coverage 54.26% 54.41% +0.14%
==========================================
Files 111 111
Lines 9529 9535 +6
==========================================
+ Hits 5171 5188 +17
+ Misses 3957 3949 -8
+ Partials 401 398 -3
Flags with carried forward coverage won't be shown. Click here to find out more.
Continue to review full report at Codecov.
|
stek29
changed the title
|
I'm not sure on how to test the other path (when byPod has a error), the only way I can think of is a different test which only has constraintTemplateStatus controller started, without ConstraintTemplateController running, and adding a fakePod PodStatus – and I'm not quite sure on how to do that properly. |
|
thanks for the PR! Probably the easiest way to hit the other path (all show errors) is to try to apply a constraint template with invalid Rego in it:
|
stek29
force-pushed
the
fix-2053
branch
2 times, most recently
from
bad1f8c
to
151dbe9
Compare
|
@maxsmythe I've already tried that and failed, because I forgot to remove Done, I've tests for |
| templateCpy.Spec.Targets[0].Rego = ` | ||
| package foo | ||
|
|
||
| violtaion[invalid syntax error` |
There was a problem hiding this comment.
this is still a sufficient test case without the typo right? can we remove the typo?
There was a problem hiding this comment.
@ritazh the typo was accidental, didn't notice it :D
| func verifyTStatusCreated(ctx context.Context, c client.Client, expected bool) func() error { | ||
| return func() error { | ||
| ct := &v1beta1.ConstraintTemplate{} | ||
| if err := c.Get(ctx, types.NamespacedName{Name: "denyall"}, ct); err != nil { |
There was a problem hiding this comment.
maybe not hardcode this but instead pass in a var?
There was a problem hiding this comment.
verifyTByPodStatusCount has it hardcoded, that's why I've kept it hardcoded :)
I'd suggest to add a const with a name instead of passing a var or hardcoding, would that be fine?
There was a problem hiding this comment.
Thank you for the PR! LGTM, LMK when it's ready to merge vis-a-vis Rita's comments.
One thought:
There are some race conditions where this mistakenly points to "false" instead of true (e.g. all pods are restarted but the CRD exists from previous pods), but better to be pessimistic than optimistic.
Watching for the CRD resource could also work but would be subject to other errors (e.g. a CRD that corresponds to an old, deleted CT that has yet to be GC'd)
stek29
force-pushed
the
fix-2053
branch
3 times, most recently
from
c17b742
to
17eec55
Compare
Signed-off-by: Viktor Oreshkin <imselfish@stek29.rocks>
Only set ConstraintTemplate status.created to true if at least one ConstraintTemplatePodStatus had no errors Fixes open-policy-agent#2053 Signed-off-by: Viktor Oreshkin <imselfish@stek29.rocks>
Signed-off-by: Viktor Oreshkin <imselfish@stek29.rocks>
Signed-off-by: Viktor Oreshkin <imselfish@stek29.rocks>
|
@maxsmythe @ritazh I've fixed the typo and moved names to constants :) |
…licy-agent#2208) * fix: clean up error consts in constrainttemplate controller Signed-off-by: Viktor Oreshkin <imselfish@stek29.rocks> * fix: only set CT status.created on success Only set ConstraintTemplate status.created to true if at least one ConstraintTemplatePodStatus had no errors Fixes open-policy-agent#2053 Signed-off-by: Viktor Oreshkin <imselfish@stek29.rocks> * fix: add tests for CT status.created Signed-off-by: Viktor Oreshkin <imselfish@stek29.rocks> * style: const for a constraint name in tests Signed-off-by: Viktor Oreshkin <imselfish@stek29.rocks>
What this PR does / why we need it:
Currently ConstraintTemplate's
.status.createdfield is set to true if there's at least one ConstraintTemplatePodStatus for it.However, it doesn't check if ConstraintTemplatePodStatus has actually succeeded (has no errors). This PR fixes that –
.status.createdis only set totrueif there's at least oneConstraintTemplatePodStatusfor this template with no errors.I've also noticed that
Err..Codeconstants are used inconsistently in the constrainttemplate controller, so I've fixed that too. Please tell me if it has to be separated into a different PR.Which issue(s) this PR fixes:
Fixes #2053