fix(pkg/render/apiserver.go): Correctly add validatingadmissionpolicy rules for kubernetes 1.30+

[philroche]

The expectation was that the "validatingadmissionpolicies" and "validatingadmissionpolicybindings" rules would
be added when using kubernetes 1.30+ but this is not happening due to reversed < > logic.

This was highlighted using calico-apiserver 3.29.2 where this validation is performed by default but was unable
to when using the tigera operator due to missing rules.

This resolves issue #3780

[CLAassistant]

All committers have signed the CLA.

[rene-dekker]

Hi Phil, thank you for your contribution. I did add a small suggestion, other than that it looks good.

[rene-dekker]

/merge-when-ready

[marvin-tigera]

OK, I will merge the pull request when it's ready, leave the commits as is when I merge it, and leave the branch after I've merged it.

[caseydavenport]

Can we make sure we have unit tests that verify the correct behavior for relevant versions?

e.g., 1.29, 1.30, 1.31

[philroche]

I am not familiar with the codebase yet and was hoping i'd learn how to run the tests via CI check logs.

I'll spend some time understanding the tests.

[rene-dekker]

On second glance I missed the !. I think the original code was correct. That said, it is an overly hard-to-read statement.

I think it can be simplified to if c.cfg.KubernetesVersion == nil || (c.cfg.KubernetesVersion.Major == 1 && c.cfg.KubernetesVersion.Minor >= 30) {

[philroche]

On second glance I missed the !. I think the original code was correct. That said, it is an overly hard-to-read statement.

I think it can be simplified to if c.cfg.KubernetesVersion == nil || (c.cfg.KubernetesVersion.Major == 1 && c.cfg.KubernetesVersion.Minor >= 30) {

On further review you are right yes, but in my tests the rules are not being added when testing with Kubernetes 1.32

[philroche]

@rene-dekker Thanks for pointing out my error. Revisiting the original if statement

if c.cfg.KubernetesVersion == nil || !(c.cfg.KubernetesVersion != nil && c.cfg.KubernetesVersion.Major < 2 && c.cfg.KubernetesVersion.Minor < 30) {

And given a test kubernetes version of 1.32

# if c.cfg.KubernetesVersion == nil || !(c.cfg.KubernetesVersion != nil && c.cfg.KubernetesVersion.Major < 2 && c.cfg.KubernetesVersion.Minor < 30)

c.cfg.KubernetesVersion = 1.32

c.cfg.KubernetesVersion.Major = 1

c.cfg.KubernetesVersion.Minor = 32

# !(c.cfg.KubernetesVersion != nil && c.cfg.KubernetesVersion.Major < 2 && c.cfg.KubernetesVersion.Minor < 30)

(c.cfg.KubernetesVersion != nil) = true

(c.cfg.KubernetesVersion.Major < 2) = true

(c.cfg.KubernetesVersion.Minor < 30) = false 

(true && true && false) = false 

!(false) = true 

# c.cfg.KubernetesVersion == nil || !(c.cfg.KubernetesVersion != nil && c.cfg.KubernetesVersion.Major < 2 && c.cfg.KubernetesVersion.Minor < 30)

(c.cfg.KubernetesVersion == nil) = false

(false || true) = true

So evaluating to true should be adding the rules. I didn't see this in my tests. I did see the other rules being added though.

I think it can be simplified to if c.cfg.KubernetesVersion == nil || (c.cfg.KubernetesVersion.Major == 1 && c.cfg.KubernetesVersion.Minor >= 30) {

Yes, much more readable

[caseydavenport]

@philroche FYI we'll need you to sign the CLA in order to make the bot happy before merging

[philroche]

@philroche FYI we'll need you to sign the CLA in order to make the bot happy before merging

Yup I have escalated to Chainguard legal team to make sure that's OK for me using my Chainguard email.

[philroche]

Further debug output added to original issue @ #3780 (comment)

[philroche]

@caseydavenport CLA signed

[github-actions]

This PR is stale because it has been open for 60 days with no activity.

[caseydavenport]

/sem-approve

[caseydavenport]

Sorry this got delayed.. I am not sure if this is still needed, but it seems like there are some test failures.