fix: preserve equals signs in endpoint label values#6443
Conversation
k8s-ci-robot
added
the
cncf-cla: yes
|
Welcome @immanuwell! |
k8s-ci-robot
added
the
needs-ok-to-test
|
Hi @immanuwell. Thanks for your PR. I'm waiting for a kubernetes-sigs member to verify that this patch is reasonable to test. If it is, they should reply with Regular contributors should join the org to skip this step. Once the patch is verified, the new status will be reflected by the I understand the commands that are listed here. DetailsInstructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. |
k8s-ci-robot
added
the
size/S
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: The full list of commands accepted by this bot can be found here. DetailsNeeds approval from an approver in each of these files:Approvers can indicate their approval by writing |
|
Could you share actual reproducible example? Like here #5111 (comment) What is wrong with owner |
u-kai
left a comment
u-kai
left a comment
There was a problem hiding this comment.
My understanding is that values containing "=" could not be set for owner label until now.
|
|
||
| serialized := labels.SerializePlain(false) | ||
| parsed, err := NewLabelsFromStringPlain(serialized) | ||
|
|
There was a problem hiding this comment.
Could you add tests for actual label texts?
| suite.Nil(multipleHeritage, "if error should return nil") | ||
| } | ||
|
|
||
| func (suite *LabelsSuite) TestSerializeDeserializeWithEqualsInValue() { |
There was a problem hiding this comment.
IMO, I would prefer using table-driven tests for this file, but I think that refactor is out of scope for this PR.
There was a problem hiding this comment.
It must be table driven test. As we need to make sure other cases of owner still supported.
Do not use suite. Name could be TestParseLabels or of similar semantic. the current
PR's test name (TestSerializeDeserializeWithEqualsInValue) is misleading precisely because it goes through the full round-trip to avoid writing the raw string by hand, masking that the bug is purely in parsing.
| suite.Nil(multipleHeritage, "if error should return nil") | ||
| } | ||
|
|
||
| func (suite *LabelsSuite) TestSerializeDeserializeWithEqualsInValue() { |
There was a problem hiding this comment.
It must be table driven test. As we need to make sure other cases of owner still supported.
Do not use suite. Name could be TestParseLabels or of similar semantic. the current
PR's test name (TestSerializeDeserializeWithEqualsInValue) is misleading precisely because it goes through the full round-trip to avoid writing the raw string by hand, masking that the bug is purely in parsing.
4 participants
What does it do ?
Preserves
=inside endpoint label values when TXT registry labels are parsed.Before this,
heritage=external-dns,external-dns/owner=team=platformparsed withoutowner. Tiny parser footgun, tbh.Motivation
--txt-owner-idis a plain string flag, so values liketeam=platformcan get written into TXT ownership records. TXT records can contain=, so this is a real input, not some impossible edge-case.Repro before the fix:
go test ./endpoint -run TestLabels/TestSerializeDeserializeWithEqualsInValue -count=1It fails because
ownergets dropped after the round trip.Fixed by splitting labels on the first
=only. Existing malformed tokens without=are still skipped, same as before.Tested:
More