encode NamespacedName with lower case in JSON #117238
Conversation
That is how a workaround in controller-runtime has encoded NamespacedName (https://github.com/kubernetes-sigs/controller-runtime/blob/a33d038b84d0e7f615b8d803bdc47f0d1f8484b7/pkg/log/zap/kube_helpers.go#L49-L57) and it is also more consistent with Kubernetes API standards. The namespace can be left out. Returning an anonymous struct in MarshalLog makes it impossible for controller-runtime to adapt the type and thus blocks updating controller-runtime to Kubernetes 1.27 because log content would change.
b66e2ac
to
4438208
Compare
|
LGTM and I've tested this as part of my PR kubernetes-sigs/controller-runtime#2189 |
|
/assign @dims This is a follow-up to #106379 which you also helped to get merged. Sorry that we need to modify the behavior again so soon! But that controller-runtime had a workaround for this problem shows that it was a real issue that bothered people, we just need to make the upstream solution work for them. |
This does not seem like a Kubernetes bug. Log content is not an API we guarantee, and downstreams taking hard dependencies on exact log content seems like a problem / anti-pattern we want to flush out and get rid of, not enable. |
|
To be clear, I don't really object to lowercasing the keys... I object strongly to positioning log content as a shadow API surface area downstreams take hard dependencies on and we treat changes in as bugs. |
|
Okay, so not a bug. I agree with the argument and had the same thought initially, but then came to the conclusion that we should have chosen lower case for internal consistency, too, and therefore should change this also in 1.27.1, i.e. backport the change. Can we do that? |
|
Note that it's not just general API conventions, but also |
|
/lgtm I updated the release note to remove the reference to consistency with controller-runtime / API standards, since neither of those are things that we promise log output will match I'm on the fence about picking to 1.27.1... I could go either way |
|
LGTM label has been added. Git tree hash: e353a4917eb359207bf1541da37598ad9045d83d
|
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: liggitt, pohly The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
|
/remove-kind bug |
|
/remove-kind failing-test |
|
The release note now describes the inconsistency with |
|
Let's backport. There's no need to make the life of our downstream consumers harder than it has to be. We still reserve the right to make log changes when it's necessary, but in this case it wasn't. In a way we should be thankful for the test in component-base because it highlighted a bad (= inconsistent) choice. |
|
=> #117298 |
|
/triage accepted |
…8-origin-release-1.27 Automated cherry pick of #117238: api: encode NamespacedName with lower case in JSON
What type of PR is this?
/kind bug
/kind failing-test
What this PR does / why we need it:
Lower case is how a workaround in controller-runtime has encoded NamespacedName (https://github.com/kubernetes-sigs/controller-runtime/blob/a33d038b84d0e7f615b8d803bdc47f0d1f8484b7/pkg/log/zap/kube_helpers.go#L49-L57) and it is also more consistent with Kubernetes API standards.
Which issue(s) this PR fixes:
Returning an anonymous struct in MarshalLog makes it impossible for controller-runtime to adapt the type and thus blocks updating controller-runtime to Kubernetes 1.27 because log content would change.
Special notes for your reviewer:
Once merged we need to backport.
Does this PR introduce a user-facing change?