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

test: Extend coredns clusterrole with additional resource permissions #18104

Merged

Conversation

aditighag
Copy link
Member

@aditighag aditighag commented Dec 2, 2021

Commit didn't add permissions for endpointslices resource to the
coredns cluterrole on k8s < 1.20. As a result, core-dns deployments
failed on the these versions with the error -

2021-11-30T14:09:43.349414540Z E1130 14:09:43.349292 1 reflector.go:138] pkg/mod/k8s.io/client-go@v0.20.2/tools/cache/reflector.go:167: Failed to watch *v1beta1.EndpointSlice: failed to list *v1beta1.EndpointSlice: endpointslices.discovery.k8s.io is forbidden: User "system:serviceaccount:kube-system:coredns" cannot list resource "endpointslices" in API group "discovery.k8s.io" at the cluster scope

Note : Hold off onto enabling the feature gate on older versions. -

CONTROLLER_FEATURE_GATES="EndpointSlice=true"
.

Fixes: 398d55c
Fixes: #18086

Signed-off-by: Aditi Ghag aditi@cilium.io

@aditighag aditighag requested a review from a team as a code owner December 2, 2021 16:30
@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 Dec 2, 2021
@aditighag
Copy link
Member Author

aditighag commented Dec 2, 2021

test-only --focus="K8sServicesTest.*" --k8s_version=1.19 --kernel_version="49"

Edit : Passed - https://jenkins.cilium.io/job/Cilium-PR-Tests-Kernel-Focus/374/console

@aditighag aditighag added the release-note/ci This PR makes changes to the CI. label Dec 2, 2021
@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 Dec 2, 2021
@aditighag aditighag marked this pull request as draft December 2, 2021 16:33
@aditighag
Copy link
Member Author

aditighag commented Dec 2, 2021

I've triggered --focus="K8sServicesTest.*" with k8s 1.16, kernel 4.19 and k8s 1.19, kernel 4.19 manually - https://jenkins.cilium.io/view/PR/job/Cilium-PR-Tests-Kernel-Focus/380/
https://jenkins.cilium.io/view/PR/job/Cilium-PR-Tests-Kernel-Focus/381/ (Hit #18072)

@aditighag aditighag marked this pull request as ready for review December 2, 2021 17:48
@aditighag
Copy link
Member Author

test-only --focus="K8sServicesTest.*" --k8s_version=1.19 --kernel_version="49"

Edit : Passed - https://jenkins.cilium.io/job/Cilium-PR-Tests-Kernel-Focus/374/console

I've triggered --focus="K8sServicesTest.*" with k8s 1.16, kernel 4.19 and k8s 1.19, kernel 4.19 manually - https://jenkins.cilium.io/view/PR/job/Cilium-PR-Tests-Kernel-Focus/380/ https://jenkins.cilium.io/view/PR/job/Cilium-PR-Tests-Kernel-Focus/381/

All focused tests have passed.

@maintainer-s-little-helper maintainer-s-little-helper bot added this to Needs backport from master in 1.11.0 Dec 2, 2021
@maintainer-s-little-helper maintainer-s-little-helper bot added this to Needs backport from master in 1.10.6 Dec 2, 2021
@maintainer-s-little-helper maintainer-s-little-helper bot added this to Needs backport from master in 1.9.12 Dec 2, 2021
Copy link
Member

@nbusseneau nbusseneau left a comment

Choose a reason for hiding this comment

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

Ah, I think the commit description might be malformed as its missing a link to the commit it fixes after Fixes:?

Commit 398d55c didn't add permissions for `endpointslices` resource to the
coredns `cluterrole` on k8s < 1.20. As a result, core-dns deployments
failed on the these versions with the error -

`2021-11-30T14:09:43.349414540Z E1130 14:09:43.349292 1 reflector.go:138] pkg/mod/k8s.io/client-go@v0.20.2/tools/cache/reflector.go:167: Failed to watch *v1beta1.EndpointSlice: failed to list *v1beta1.EndpointSlice: endpointslices.discovery.k8s.io is forbidden: User "system:serviceaccount:kube-system:coredns" cannot list resource "endpointslices" in API group "discovery.k8s.io" at the cluster scope`

Fixes: 398d55c
Signed-off-by: Aditi Ghag <aditi@cilium.io>
@aditighag aditighag force-pushed the pr/aditighag/extend-coredns-bump branch from 04cacdc to 80f872f Compare December 2, 2021 17:54
@aditighag
Copy link
Member Author

aditighag commented Dec 2, 2021

Ah, I think the commit description might be malformed as its missing a link to the commit it fixes after Fixes:?

Fixed.

Only updated the commit description in the latest push, no need to re-run the tests.

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.

FYI for generating the "fixes:" tag for commits, typically we will use this format:

git log -1 --pretty="%h (\"%s\")"

This way, if the sha is ambiguous (eg because it's too short and the first N characters are identical between two different git commit shas, which can happen over time as the number of commits grows), then we can clearly disambiguate which underlying commit is being referenced here.

@joestringer
Copy link
Member

joestringer commented Dec 2, 2021

^^ Given that the bug is only on master and we're not likely to track this down in future, I don't think it's necessary to fix this up in git history. Good to merge.

@aditighag
Copy link
Member Author

FYI for generating the "fixes:" tag for commits, typically we will use this format:

git log -1 --pretty="%h (\"%s\")"

This way, if the sha is ambiguous (eg because it's too short and the first N characters are identical between two different git commit shas, which can happen over time as the number of commits grows), then we can clearly disambiguate which underlying commit is being referenced here.

Noted. I simply copied it from the IDE git window so it ended up copying just the first N characters.

@maintainer-s-little-helper maintainer-s-little-helper bot moved this from Needs backport from master to Backport pending to v1.10 in 1.11.0 Dec 2, 2021
@joestringer joestringer added backport-done/1.11 The backport for Cilium 1.11.x for this PR is done. and removed backport-pending/1.11 labels Dec 3, 2021
@maintainer-s-little-helper maintainer-s-little-helper bot moved this from Backport pending to v1.10 to Backport done to v1.11 in 1.11.0 Dec 3, 2021
@maintainer-s-little-helper maintainer-s-little-helper bot moved this from Needs backport from master to Backport pending to v1.10 in 1.10.6 Dec 6, 2021
@maintainer-s-little-helper maintainer-s-little-helper bot moved this from Needs backport from master to Backport pending to v1.9 in 1.9.12 Dec 6, 2021
@maintainer-s-little-helper maintainer-s-little-helper bot moved this from Backport pending to v1.10 to Backport done to v1.10 in 1.10.6 Dec 10, 2021
@maintainer-s-little-helper maintainer-s-little-helper bot moved this from Backport pending to v1.9 to Backport done to v1.9 in 1.9.12 Dec 15, 2021
@maintainer-s-little-helper maintainer-s-little-helper bot moved this from Backport pending to v1.9 to Backport done to v1.9 in 1.9.12 Dec 15, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backport-done/1.11 The backport for Cilium 1.11.x for this PR is done. release-note/ci This PR makes changes to the CI.
Projects
No open projects
1.10.6
Backport done to v1.10
1.11.0
Backport done to v1.11
1.9.12
Backport done to v1.9
Development

Successfully merging this pull request may close these issues.

CI: k8s v1.19 or older: Kubernetes DNS did not become ready in time (all tests)
5 participants