Disallow legacy hostNetwork together with non-default provider

[obnoxxx]

Description of changes:

Since the introduction of the "host" network provider, the legacy
"hostNetwork" setting is intended to be used only in combination with
the default network provider (""), but originally, the code did not enforce this.

This change adds the required validation checks to throw errors
in invalid constellations.

Issue resolved by this Pull Request:
Resolves #13692

Checklist:

• Commit Message Formatting: Commit titles and messages follow guidelines in the developer guide.
• Reviewed the developer guide on Submitting a Pull Request
• Pending release notes updated with breaking and/or notable changes for the next minor release.
• Documentation has been updated, if necessary.
• Unit tests have been added, if necessary.
• Integration tests have been added, if necessary.

[mergify]

⚠️ The sha of the head commit of this PR conflicts with #13651. Mergify cannot evaluate rules on this PR. ⚠️

[BlaineEXE]

This looks good. Let's also add a validating admission policy to reflect this in types.go. That will probably happen in between two of the lines here. And there is an example of how to use the VAPs with // +kubebuilder:validation:XValidation.

With that set, it should mean that k8s won't let you set hostNetwork: true at the same time as provider != "" in the CRD when creating/updating it.

[obnoxxx]

@BlaineEXE wrote:

This looks good. Let's also add a validating admission policy to reflect this in types.go. That will probably happen in between two of the lines here. And there is an example of how to use the VAPs with // +kubebuilder:validation:XValidation.

With that set, it should mean that k8s won't let you set hostNetwork: true at the same time as provider != "" in the CRD when creating/updating it.

Thanks for this hint! I found the place in the code.
It looks a bit as if one should essentially repeat the logic of the Validate function here.

I will soon update the PR accordingly

[obnoxxx]

@BlaineEXE wrote:

This looks good. Let's also add a validating admission policy to reflect this in types.go. That will probably happen in between two of the lines here. And there is an example of how to use the VAPs with // +kubebuilder:validation:XValidation.

With that set, it should mean that k8s won't let you set hostNetwork: true at the same time as provider != "" in the CRD when creating/updating it.

I think I have now added what you suggested. And added the updated crds resulting from that.

I guess now it "just" need some testing ...

[BlaineEXE]

I had to look up CEL documentation to learn that we do need has() statements when JSON omitempty is used.

The expression here is failing because it does not allow the empty-omitted case to succeed. The expression must be modified to allow !has(self.hostNetwork) to be interpreted the same as self.hostNetwork == false.

Ditto for !has(self.provider) being interpreted the same as self.provider == "".

Please also use self.hostNetwork == false instead of self.hostNetwork != true. == is always easier to read than !=.

The parens surrounding the statements between || are also unnecessary and don't improve legibility. Let's remove those before final merge.

[BlaineEXE]

Also, I certainly find that sometimes the required logic needed to explain to VAPs when to pass validation can be hard to grok. I imagine this may be the case for you as well.

I have been finding it easier to mentally write out the failure case and then use DeMorgan's Laws to get the inverse expression. I learned this in school, but I'm not sure how common it is in other curriculums.

For example, the failure case is has(self.hostNetwork) && self.hostNetwork == true && has(self.provider) && self.provider != "". The inverse of this is !(<the whole previous expression>), and then DeMorgan's Laws allow simplifying the expression into || statements.

FWIW, this is also often how I go about reading/interpreting the existing validation statements. I find it easier to mentally grasp the failure cases.

[obnoxxx]

done

[obnoxxx]

@BlaineEXE thanks fore the detailed explanation above! I agree with everything you wrote. fwiw, application of Boolean algebra and de Morgan's law is how I originally arrived at the rule I initially had in the code (involving || instead of &&.

[obnoxxx]

After these latest updates, the CI looks a lot better. One failing check looks unrelated.

Locally everything builds and deploys fine. got cluster-test.yaml to deploy to status HEALTH_OK ! 🎉

[BlaineEXE]

The inverse of the "failure case" expression I mentioned would be this:

!has(self.hostNetwork) || self.hostNetwork == false || !has(self.provider) || self.provider == ""

Note that I am using == false as the inverse of == true and == "" as the inverse of != "" to reduce the statement's layers to their simplest boolean algebra forms.

[parth-gr]

rule="!has(self.hostNetwork) || !has(self.provider)
-> rule="has(self.hostNetwork) && !has(self.provider)" might be this

[obnoxxx]

@parth-gr wrote:

rule="!has(self.hostNetwork) || !has(self.provider) -> rule="has(self.hostNetwork) && !has(self.provider)" might be this

Actually no: We want to prevent hostNetwork from being set together with a non-empty provider, so the rule has to be this:

!(has(self.hostNetwork) && has(self.provider))

which Boolean algebra simplifies to:

!has(self.hostNetwork) || !has(self.provider)

[obnoxxx]

It is actually even more complex since we do need to check the values in addition to using has() I'll update shortly.

[BlaineEXE]

Looks good, but please squash the commits into one. It's fine to leave the first (ci: add "network" to the allowed commit message prefixes) commit separate if you like.

[obnoxxx]

@BlaineEXE wrote:

Looks good, but please squash the commits into one. It's fine to leave the first (ci: add "network" to the allowed commit message prefixes) commit separate if you like.

Thanks!

I'll do the squashing tomorrow

[obnoxxx]

@BlaineEXE wrote:

Looks good, but please squash the commits into one. It's fine to leave the first (ci: add "network" to the allowed commit message prefixes) commit separate if you like.

Thanks!

I'll do the squashing tomorrow

I actually already did it now 😬