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.

Sign up for GitHub

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

Jump to bottom

Update k8s tests and libraries to v1.27.0 #24837

Merged
merged 2 commits into from
Apr 19, 2023
Merged

Update k8s tests and libraries to v1.27.0 #24837

merged 2 commits into from
Apr 19, 2023

Conversation

tklauser
Copy link
Member

@tklauser tklauser commented Apr 12, 2023 •
edited

Updating k8s libs to v1.27.0 requires updating sigs.k8s.io/controller-runtime to an unreleased version with kubernetes-sigs/controller-runtime#2223 in place. This in turn requires some additional adjustments to changed APIs in the controller-runtime, mainly in operator/pkg/gateway-api.

The service.kubernetes.io/topology-aware-hints annotation got deprecated with k8s 1.27 in favor of service.kubernetes.io/topology-mode (see kubernetes/kubernetes#116522). This change keeps support for the deprecated annotation. Support for the new annotation will be added in a successive commit.

Supersedes #24553

Add support for Kubernetes v1.27

@tklauser tklauser added sig/k8s Impacts the kubernetes API, or kubernetes -> cilium internals translation layers. release-note/major This PR introduces major new functionality to Cilium. labels Apr 12, 2023
@tklauser tklauser mentioned this pull request Apr 12, 2023
Closed
@tklauser tklauser self-assigned this Apr 12, 2023
@tklauser tklauser added this to the 1.14 milestone Apr 12, 2023
@tklauser tklauser force-pushed the pr/tklauser/k8s-update-1.27.0 branch 3 times, most recently from 598a3d4 to 6897b4b Compare April 12, 2023 13:26
@nbusseneau
Copy link
Member

/test-1.27-net-next

@tklauser tklauser force-pushed the pr/tklauser/k8s-update-1.27.0 branch from 6897b4b to 37bfc6e Compare April 12, 2023 15:31
@tklauser
Copy link
Member Author

/test-1.27-net-next

@tklauser
Copy link
Member Author

/test-upstream-k8s

@tklauser tklauser mentioned this pull request Apr 13, 2023
Merged
@tklauser
Copy link
Member Author

k8s-1.27-kernel-net-next and k8s-upstream both passed. Marking as ready to review and running remaining tests.

@tklauser tklauser marked this pull request as ready for review April 13, 2023 16:01
@tklauser tklauser requested review from a team as code owners April 13, 2023 16:01
@tklauser
Copy link
Member Author

/test

aanm
aanm requested changes Apr 17, 2023
View reviewed changes
Copy link
Member

@aanm aanm left a comment

Choose a reason for hiding this comment

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

Discussed offline

zacharysarah
zacharysarah requested changes Apr 18, 2023
View reviewed changes
Copy link
Contributor

@zacharysarah zacharysarah left a comment

Choose a reason for hiding this comment

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

@tklauser 👋🏻 Some edits for clarity, otherwise LGTM from a docs perspective

vendor/github.com/golang-jwt/jwt/v4/README.md Show resolved Hide resolved
vendor/github.com/golang-jwt/jwt/v4/README.md Show resolved Hide resolved
vendor/github.com/golang-jwt/jwt/v4/README.md Show resolved Hide resolved
vendor/github.com/golang-jwt/jwt/v4/README.md Show resolved Hide resolved
vendor/github.com/golang-jwt/jwt/v4/SECURITY.md Show resolved Hide resolved
vendor/github.com/golang-jwt/jwt/v4/SECURITY.md Show resolved Hide resolved
@tklauser
Copy link
Member Author

@zacharysarah The changes you requested are all in the 3rd party dependency package github.com/golang-jwt/jwt/v4 which we copy 1:1 to our code base, see https://go.dev/ref/mod#vendoring for how vendoring works in Go. We'd need to make the changes in the upstream package first and then vendor them back in. I think this is out of scope for this PR. What do you think?

@tklauser tklauser force-pushed the pr/tklauser/k8s-update-1.27.0 branch from 37bfc6e to 09d344e Compare April 18, 2023 08:39
@tklauser tklauser requested a review from aanm April 18, 2023 08:39
@tklauser
Copy link
Member Author

/test-1.27-net-next

@tklauser
Copy link
Member Author

/test-upstream-k8s

@tklauser tklauser force-pushed the pr/tklauser/k8s-update-1.27.0 branch 3 times, most recently from 2615e7c to 8f8ca7d Compare April 18, 2023 09:56
@tklauser
Update k8s tests and libraries to v1.27.0
f079558 
Updating k8s libs to v1.27.0 requires updating
sigs.k8s.io/controller-runtime to an unreleased version with
kubernetes-sigs/controller-runtime#2223 in
place. This in turn requires some additional adjustments to changed APIs
in the controller-runtime, mainly in operator/pkg/gateway-api.

The service.kubernetes.io/topology-aware-hints annotation got deprecated
with k8s 1.27 in favor of service.kubernetes.io/topology-mode (see
kubernetes/kubernetes#116522). This change keeps
support for the deprecated annotation. Support for the new annotation
will be added in a successive commit.

Signed-off-by: Chris Tarazi <chris@isovalent.com>
Signed-off-by: Tobias Klauser <tobias@cilium.io>
@tklauser
k8s: add support for service.kubernetes.io/topology-mode
387b223 
k8s 1.27 deprecated the service.kubernetes.io/topology-aware-hints
annotation in favor of the service.kubernetes.io/topology-mode
annotation to allow implementation-specific values for the annotation.
See kubernetes/kubernetes#116522 for more
details.

Add support for service.kubernetes.io/topology-mode, letting the
deprecated service.kubernetes.io/topology-aware-hints take precedence.
This is in correspondence with the upstream implementation.

Signed-off-by: Tobias Klauser <tobias@cilium.io>
@tklauser tklauser force-pushed the pr/tklauser/k8s-update-1.27.0 branch from 8f8ca7d to 387b223 Compare April 18, 2023 10:48
@tklauser
Copy link
Member Author

/test-1.27-net-next

@tklauser
Copy link
Member Author

/test-upstream-k8s

@tklauser
Copy link
Member Author

/test

@tklauser
Copy link
Member Author

tklauser commented Apr 18, 2023 •
edited

/test-upstream-k8s

previous attempt timed out: https://jenkins.cilium.io/job/Cilium-PR-K8s-Upstream/4283/

@zacharysarah
Copy link
Contributor

zacharysarah commented Apr 18, 2023 •
edited

@tklauser 👋🏻

We'd need to make the changes in the upstream package first and then vendor them back in. I think this is out of scope for this PR. What do you think?

I agree that those changes are out of scope for this PR.

For future PRs that import vendor content, is there a way to signal that clearly? I'd like to avoid giving unnecessary reviews.

@tklauser
Copy link
Member Author

@zacharysarah 👋

@tklauser 👋🏻

We'd need to make the changes in the upstream package first and then vendor them back in. I think this is out of scope for this PR. What do you think?

I agree that those changes are out of scope for this PR.

Sorry for taking your time to review these files 😞

For future PRs that import vendor content, is there a way to signal that clearly? I'd like to avoid giving unnecessary reviews.

I think GitHub should hide the files under vendor/ by default in the review view:

image

Unfortunately it also hides other files which still might be of interest, e.g. in case the diff is too large. But at least this should be a good first indicator.

maybe this is something we could point out more explicitly in the contributor guide's pull requests review process docs?

sayboras
sayboras approved these changes Apr 19, 2023
View reviewed changes
Copy link
Member

@sayboras sayboras left a comment

Choose a reason for hiding this comment

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

Thanks and looks good to me for servicemesh related files

@tklauser tklauser removed the request for review from youngnick April 19, 2023 09:23
@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 19, 2023
@christarazi christarazi merged commit 8544406 into main Apr 19, 2023
42 checks passed
@christarazi christarazi deleted the pr/tklauser/k8s-update-1.27.0 branch April 19, 2023 15:07
@pchaigno pchaigno mentioned this pull request Apr 21, 2023
Merged
@tklauser tklauser mentioned this pull request May 22, 2023
Merged
@nbusseneau nbusseneau mentioned this pull request May 23, 2023
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@zacharysarah zacharysarah zacharysarah approved these changes

@christarazi christarazi christarazi approved these changes

@nbusseneau nbusseneau nbusseneau approved these changes

@aanm aanm aanm approved these changes

@sayboras sayboras sayboras approved these changes

Assignees

@tklauser tklauser

Labels
ready-to-merge This PR has passed all tests and received consensus from code owners to merge. release-note/major This PR introduces major new functionality to Cilium. sig/k8s Impacts the kubernetes API, or kubernetes -> cilium internals translation layers.
Projects
None yet
Milestone
1.14
Development

Successfully merging this pull request may close these issues.

None yet
6 participants
@tklauser @nbusseneau @zacharysarah @christarazi @aanm @sayboras