fix: workload rollout spec is invalid template is not empty

[huikang]

Signed-off-by: Hui Kang [email protected]

close #1222

Checklist:

• Either (a) I've created an enhancement proposal and discussed it with the community, (b) this is a bug fix, or (c) this is a chore.
• The title of the PR is (a) conventional, (b) states what changed, and (c) suffixes the related issues number. E.g. "fix(controller): Updates such and such. Fixes #1234".
• I've signed my commits with DCO
• I have written unit and/or e2e tests for my change. PRs without these are unlikely to be merged.
• My builds are green. Try syncing with master if they are not.
• My organization is added to USERS.md.

[huikang]

@jessesuen , after some investigation, I found that

argo-rollouts/rollout/controller.go

Line 462 in ff65b39

func (c *rolloutContext) getRolloutValidationErrors() error {

does not return any error in this scenario.

The check could be placed during resolving the workload ref for the first time. Is there any corner case the fix does not cover?

[codecov]

Codecov Report

Merging #1224 (5427383) into master (ff65b39) will increase coverage by 0.00%.
The diff coverage is 100.00%.

@@           Coverage Diff           @@
##           master    #1224   +/-   ##
=======================================
  Coverage   81.29%   81.30%           
=======================================
  Files         106      106           
  Lines        9522     9527    +5     
=======================================
+ Hits         7741     7746    +5     
  Misses       1256     1256           
  Partials      525      525           

Impacted Files	Coverage Δ
pkg/apis/rollouts/validation/validation.go	90.44% <100.00%> (+0.18%)	⬆️
rollout/temlateref.go	88.18% <100.00%> (+0.18%)	⬆️

---

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 ff65b39...5427383. Read the comment docs.

[jessesuen]

@huikang - the original problem was that it was possible for a user to make a mistake in the spec (using my example) without that error surfacing into a condition (there was no update to status). And so the user did not know why the rollout was stuck. Will this fix solve that?

[huikang]

@huikang - the original problem was that it was possible for a user to make a mistake in the spec (using my example) without that error surfacing into a condition (there was no update to status). And so the user did not know why the rollout was stuck. Will this fix solve that?

I am not sure if I understand your question.

With the PR, if the uses submits a rollout like your example, the get rollout will show the following error

Name:            rollouts-demo-canary
Namespace:       default
Status:          ✖ Degraded
Message:         InvalidSpec: The Rollout "rollouts-demo-canary" is invalid: template must be empty for workload reference rollout

Since the change occurs to Resolve(), it surfaces into a condition.

To prevent the user makes a mistake, do you mean change is suppose to throw an error when user submit the manifest (or even using kubectl-argo-rollouts lint)?

[jessesuen]

Instead of in here, can we put this in validation library so that the linter can catch the problem?

[huikang]

Updated. The method is moved to validation package. However, lint still can not catch the problem as it reports the following error.

Error: spec.template.spec.containers: Required value

Please see the test case I added to validation_test.go. During reconciliation time, since the template.spec.containers is filled by clientset.Get(), validation method will only report this error -template must be empty for workload reference rollout.

[huikang]

Hi, @jessesuen , the validation are placed in both Resolve and ValidateRolloutSpec method. In ValidateRolloutSpec, an additional condition (!rollout.Spec.TemplateResolvedFromRef) is added because during reconciliation template is filled prior to validation; otherwise it results in false alarm.

An e2e test is added.

[jessesuen]

I am not sure if I understand your question.
With the PR, if the uses submits a rollout like your example, the get rollout will show the following error

Yes, this answers my question! With the original problem, the rollout status field was never populated by the controller.

[sonarcloud]

Kudos, SonarCloud Quality Gate passed!

0 Bugs
0 Vulnerabilities
0 Security Hotspots
2 Code Smells

No Coverage information
0.0% Duplication