Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

k8s: api: clean up CRD versioning #24671

Merged
merged 3 commits into from
Apr 4, 2023

Conversation

julianwiedmann
Copy link
Member

@julianwiedmann julianwiedmann commented Mar 31, 2023

Clean up some confusion in the v2 / v2alpha1 versioning for CRDs. Turns out the CustomResourceDefinitionSchemaVersion for v2alpha1 isn't actually used anymore.

@maintainer-s-little-helper maintainer-s-little-helper bot added the dont-merge/needs-release-note-label The author needs to describe the release impact of these changes. label Mar 31, 2023
@julianwiedmann julianwiedmann force-pushed the 1.14-crd-version branch 2 times, most recently from 035799c to f7f95f6 Compare March 31, 2023 13:02
@julianwiedmann julianwiedmann changed the title WIP CRD versioning k8s: api: clean up CRD versioning Mar 31, 2023
@julianwiedmann julianwiedmann added sig/k8s Impacts the kubernetes API, or kubernetes -> cilium internals translation layers. area/operator Impacts the cilium-operator component release-note/misc This PR makes changes that have no direct user impact. labels Mar 31, 2023
@maintainer-s-little-helper maintainer-s-little-helper bot removed the dont-merge/needs-release-note-label The author needs to describe the release impact of these changes. label Mar 31, 2023
@julianwiedmann
Copy link
Member Author

/test

@julianwiedmann julianwiedmann marked this pull request as ready for review March 31, 2023 15:18
@julianwiedmann julianwiedmann requested review from a team as code owners March 31, 2023 15:18
pkg/k8s/apis/cilium.io/register.go Outdated Show resolved Hide resolved
Copy link
Member

@joestringer joestringer left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM except for the version which should follow Andre's comment above.

@julianwiedmann
Copy link
Member Author

julianwiedmann commented Apr 2, 2023

/test

Job 'Cilium-PR-K8s-1.24-kernel-5.4' failed:

Click to show.

Test Name

K8sDatapathConfig Transparent encryption DirectRouting Check connectivity with transparent encryption and direct routing with bpf_host

Failure Output

FAIL: Timed out after 240.001s.

If it is a flake and a GitHub issue doesn't already exist to track it, comment /mlh new-flake Cilium-PR-K8s-1.24-kernel-5.4 so I can create one.

Job 'Cilium-PR-K8s-1.26-kernel-net-next' failed:

Click to show.

Test Name

K8sUpdates Tests upgrade and downgrade from a Cilium stable image to master

Failure Output

FAIL: Timed out after 246.338s.

If it is a flake and a GitHub issue doesn't already exist to track it, comment /mlh new-flake Cilium-PR-K8s-1.26-kernel-net-next so I can create one.

@julianwiedmann julianwiedmann requested a review from aanm April 2, 2023 16:32
The `v2` and `v2alpha1` variants of CustomResourceDefinitionGroup are just
indirections of the same constant.

The operator's CRD code needs to handle both v2 and v2alpha1 CRDs. Make
it clear that the used CustomResourceDefinitionGroup isn't tied to v2.

Signed-off-by: Julian Wiedmann <jwi@isovalent.com>
Both v2 and v2alpha1 use the same label string for the CRD Schema Version.
Add a shared constant to clarify that the relevant operator code isn't
actually tied to v2.

Signed-off-by: Julian Wiedmann <jwi@isovalent.com>
We've been maintaining distinct CRD Schema versions for v2 and v2alpha1.
But the operator actually only cared about the v2 version.

So make it clear that this is a shared version across both v2 and v2alpha1.

Signed-off-by: Julian Wiedmann <jwi@isovalent.com>
@julianwiedmann
Copy link
Member Author

The update test failed in two runs (https://jenkins.cilium.io/job/Cilium-PR-K8s-1.16-kernel-4.19/749/, https://jenkins.cilium.io/job/Cilium-PR-K8s-1.26-kernel-net-next/1602/). Maybe because the branch didn't have #24676, or maybe because it's not actually resolved yet (#24687). Let's rebase and find out.

@julianwiedmann
Copy link
Member Author

/test

@maintainer-s-little-helper maintainer-s-little-helper bot added the ready-to-merge This PR has passed all tests and received consensus from code owners to merge. label Apr 3, 2023
@julianwiedmann julianwiedmann merged commit c75e4a1 into cilium:master Apr 4, 2023
42 checks passed
@julianwiedmann julianwiedmann deleted the 1.14-crd-version branch May 10, 2023 13:47
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/operator Impacts the cilium-operator component ready-to-merge This PR has passed all tests and received consensus from code owners to merge. release-note/misc This PR makes changes that have no direct user impact. sig/k8s Impacts the kubernetes API, or kubernetes -> cilium internals translation layers.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants