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
Handle .status.conditions
on Service
s using in accordance with KEP-1623
#27399
Conversation
60c39e4
to
366c66f
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
docs structure looks good
/test |
Thanks for the PR! CI was passing before. Next step is getting @cilium/sig-k8s review, then once all reviewers are addressed and green we can do a rebase + re-run CI to confirm there's no breakage on tip of main branch. It'll also be worth evaluating whether we should backport this fix to v1.14 assuming it satisfies the backport criteria. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the PR!
Not quite sure if copy-pasting the required code for github.com/cilium/cilium/pkg/k8s/slim/k8s/apimachinery/pkg/api/meta from the apimachinery master branch and modifying the header is the correct way of doing things. Is that supposed to follow some specific version or be tracked somehow?
Please follow https://github.com/cilium/cilium/blob/main/pkg/k8s/slim/README.md and update it if the files you need in this PR aren't inside this script.
366c66f
to
59e8197
Compare
Updated the slim README with another |
…ties This changes the behaviour such that any one type of condition will only appear once in the .status.conditions list. Fixes: cilium#24945 Signed-off-by: Andreas Mårtensson <andreas@addem.se>
Signed-off-by: Andreas Mårtensson <andreas@addem.se>
59e8197
to
5a1058a
Compare
LGTM. CCing @aanm to double check the process for slim-related changes. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
slim-related changes LGTM
/test Edit: ci-ginkgo suite failed on one case due to some cancellation of a step, re-running |
Currently the lbipam controller creates multiple
.status.conditions
pertype
of condition on theService
s it updates. This conflicts with what's expected, as theServiceStatus.Conditions
is defined as a// +listType=map
and// +listMapKey=type
.While the Kubernetes API server seems to not notice, tooling that utilizes strategic merge patches on the in-cluster resources will encounter errors such as
error: failed to create manager for existing fields: failed to convert new object (network-cluster/nginx-ingress-controller; /v1, Kind=Service) to smd typed: .status.conditions: duplicate entries for key [type="io.cilium/lb-ipam-request-satisfied"]
.Not quite sure if copy-pasting the required code for
github.com/cilium/cilium/pkg/k8s/slim/k8s/apimachinery/pkg/api/meta
from the apimachinery master branch and modifying the header is the correct way of doing things. Is that supposed to follow some specific version or be tracked somehow?Haven't dug into building an image locally for testing, but at least
go test ./operator/pkg/lbipam
passes.Fixes: #24945