Vap for updates

[chris-kaiser-7]

What type of PR is this?

/kind feature

What this PR does / why we need it: This is required for the release changes https://docs.google.com/document/d/1iG6RKVJFZUxG1mZ4NbUL5Tm09n_zDck-LukYmUXoQ7c/edit?tab=t.0

Which issue(s) this PR fixes:

Fixes # VAP for Upgrades #4162

Does this PR introduce a user-facing change?:

Adds a VAP that prohibits the following:
- Installation of experimental CRDs on top of standard channel CRDs (within the same API group)
- Installation of monthly releases
- Installation of older releases

[k8s-ci-robot]

Hi @chris-kaiser-7. Thanks for your PR.

I'm waiting for a kubernetes-sigs member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository.

[kflynn]

/approve

Looks good to me with a couple of minor comments, leaving the final LGTM for someone else -- thanks, @chris-kaiser-7! 🙂

[k8s-ci-robot]

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: chris-kaiser-7, kflynn Once this PR has been reviewed and has the lgtm label, please assign aojea for approval. For more information see the Code Review Process.

The full list of commands accepted by this bot can be found here.

Needs approval from an approver in each of these files:

• OWNERS

Approvers can indicate their approval by writing /approve in a comment Approvers can cancel approval by writing /approve cancel in a comment

[kflynn]

/ok-to-test

[rikatz]

/cc /assign

[chris-kaiser-7]

Thanks @chris-kaiser-7, this is a really great change! Any chance that you could include some basic tests for this VAP?

I'm thinking that we'd want them to validate the following behavior when the VAP is installed:

1. Experimental CRDs that are part of x-k8s.io can be installed
2. Experimental CRDs that are part of k8s.io can NOT be installed
3. Standard CRDs with an old version can NOT be installed
4. Standard CRDs with the current version can be installed

It might actually be possible in https://github.com/kubernetes-sigs/gateway-api/blob/150554d3a55b731d3261525cf265bc930d2d5d85/pkg/test/crd/crd_test.go, but I'll defer to @rikatz on the best home for this.

Thanks for the review. I just started working on the tests.

[chris-kaiser-7]

Thanks @chris-kaiser-7, this is a really great change! Any chance that you could include some basic tests for this VAP?

I'm thinking that we'd want them to validate the following behavior when the VAP is installed:

1. Experimental CRDs that are part of x-k8s.io can be installed
2. Experimental CRDs that are part of k8s.io can NOT be installed
3. Standard CRDs with an old version can NOT be installed
4. Standard CRDs with the current version can be installed

It might actually be possible in https://github.com/kubernetes-sigs/gateway-api/blob/150554d3a55b731d3261525cf265bc930d2d5d85/pkg/test/crd/crd_test.go, but I'll defer to @rikatz on the best home for this.

I pushed some tests in CRD_test. I can move them to a separate test file if that is better. I also made a few changes to that. I pulled some of the init tests up a level so you can run isolated tests like. "make test.crds-validation GO_TEST_FLAGS="-run TestCRDValidation/safeupgrades_VAP_should_validate_correctly"".

[chris-kaiser-7]

I noticed a race condition and spent a while trying to fix it without time.sleep. I'm going to look at this some more but figured I should push what I got.

[chris-kaiser-7]

@rikatz @robscott See anything else that needs fixing?

[rikatz]

overall lgtm, thanks.

I just want to be sure that:

• We have a comment and an action item to use the semver library on vap once we can safely rely on it (eg.: Kubernetes 1.34 added it, once 1.37 is released we could rely on it as we would drop support for 1.33).
• The CRD generation can be much much smaller as Rob pointed out. or even use the existing one, but do some "inplace replacement" just of the annotation (like some strings.Replace)

[chris-kaiser-7]

overall lgtm, thanks.

I just want to be sure that:

• We have a comment and an action item to use the semver library on vap once we can safely rely on it (eg.: Kubernetes 1.34 added it, once 1.37 is released we could rely on it as we would drop support for 1.33).
• The CRD generation can be much much smaller as Rob pointed out. or even use the existing one, but do some "inplace replacement" just of the annotation (like some strings.Replace)

@rikatz @robscott Thanks for the feedback. I was considering using logic like strings.Replace but I was a bit worried about the integrity of the test if it has logical dependencies like that but that might be a little to nit. Though if you're both ok with that I can definitely do that.

[rikatz]

yeah, I am ok, I think it would make sense to test from what we have in real life :)

will wait for Rob for some more comment

[chris-kaiser-7]

@rikatz I pushed a string substitution method for old versions, let me know what you think.

[rikatz]

@chris-kaiser-7 thanks! I think we are almost good to go! I will try to run some local tests here with your changes.

Btw the string replacement also lgtm, much better to review the PR :)

[chris-kaiser-7]

Hey @rikatz @robscott Wanted to check in. Is there anything this PR needs from me? Thanks.

[rikatz]

Hi @chris-kaiser-7

I was meaning to do it this week and forgot, my bad :man_facepalming:

Leaving this open to be my first thing tomorrow morning! Thanks!

[chris-kaiser-7]

@chris-kaiser-7 some changes that needs to be made.

* The relative path for the manifests changed after we moved the pkg/test to a better place

* when running `make test.crds-validation` you may see that the test is failing. This is mostly probably because testenv already installs the CRDs, but then you delete them as part of one of the cleanups of the VAP test, so the examples tests fail.

I think you should split the tests from TestCRDValidation and add a new TestVAPValidation to keep them isolated. The concerns here are different on this suite: one should be testing if the CRDs and examples are fine, the other if VAP is properly working

On TestVAPValidation you can create the testenv without passing the CRDInstallOptions, so you can properly test the CRDs being installed/removed (including the VAP).

Maybe instead of touching the crd_test.go you can just create a new vap_test.go and do just your test suite over there :)

Thanks!

@rikatz Thanks for the feed back! I'll get started on these. I think separating crd_test and vap_test is a good idea. It seemed to make since at first to have the vap tests in crd_test, but I kept adding things and makes it sense to pull it out now.

[rikatz]

Thanks! if you are done on it and I miss the notification, please ping me on Slack, I am willing to check it still today.

[rikatz]

@chris-kaiser-7 just as a suggestion,as you are going to create a separate file and test, let's not touch crd_test.go and focus on vap_test.go.

I guess (IIRC) that each test will provision its own envtest and we should be safe with that :D