fix(mapper): prevent index out of bounds in ToEndpointName with multi-dot suffix#6433
Conversation
k8s-ci-robot
added
cncf-cla: yes
|
Hi @carterpewpew. 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 Tip We noticed you've done this a few times! Consider joining the org to skip this step and gain 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
k8s-ci-robot
added
ok-to-test
Coverage Report for CI Build 26103801569Coverage increased (+0.006%) to 80.629%Details
Uncovered ChangesNo uncovered changes found. Coverage RegressionsNo coverage regressions found. Coverage Stats
💛 - Coveralls |
|
Thank you for the PR! Just to confirm my understanding: after this fix, stale TXT records created under a previous Not panicking is clearly better. |
Thanks @u-kai for taking a look at this PR, as I see it there are no real negative consequences here. ("", "") is already what the mapper returns for any record it can't parse (e.g. no affix match), so the callers already handle this path. The empty key just won't match any real endpoint, meaning the stale TXT record is effectively ignored. In the DynamoDB registry it actually gets picked up by the cleanup loop. I am happy to add a debug log for these cases in a follow-up if that'd be useful for operator visibility. |
| if a.isSuffix() { | ||
| dc := strings.Count(a.suffix, ".") | ||
| DNSName := strings.SplitN(lowerDNSName, ".", 2+dc) | ||
| if len(DNSName) <= dc { |
There was a problem hiding this comment.
I knows it was there before, could you change the case to Go convention. Should be dnsName
There was a problem hiding this comment.
Addressed, renamed to parts (took inspiration from your code suggestion 😄).
| wantEndpointName: "foo.example.com", | ||
| wantRecordType: endpoint.RecordTypeCNAME, | ||
| }, | ||
| { |
There was a problem hiding this comment.
There is no direct test case for the full path with trailing labels - e.g., a-example.foo.bar.com with suffix .foo.bar should return ("example.com", "A"). The TestToEndpointNameNewTXT round-trip test for "suffix with multiple dots" covers a 3-dot suffix case indirectly, but adding an explicit assertion for trailing labels with a 2-dot suffix would close the gap and document the expected behavior.
There was a problem hiding this comment.
You're right, that was an oversight on my part. Added an explicit test for a-example.foo.bar.com with suffix .foo.bar expecting ("example.com", "A").
| if a.isSuffix() { | ||
| dc := strings.Count(a.suffix, ".") | ||
| DNSName := strings.SplitN(lowerDNSName, ".", 2+dc) | ||
| if len(DNSName) <= dc { | ||
| // Not enough labels to isolate the suffix segment. | ||
| return a.dropAffixExtractType(lowerDNSName) | ||
| } | ||
| domainWithSuffix := strings.Join(DNSName[:1+dc], ".") | ||
|
|
||
| r, rType := a.dropAffixExtractType(domainWithSuffix) | ||
| if !strings.Contains(lowerDNSName, ".") { | ||
| if len(DNSName) <= 1+dc { | ||
| // Name has no labels after the suffix segment. | ||
| return r, rType | ||
| } |
There was a problem hiding this comment.
I could be wrong here. The early-exit guard calling dropAffixExtractType(lowerDNSName) when there are too few labels is fine, but it's misleading - the name can't possibly match the suffix at that point, so it always returns ("", ""). Being explicit is cleaner. Also, domainWithSuffix can be inlined:
if a.isSuffix() {
dc := strings.Count(a.suffix, ".")
parts := strings.SplitN(lowerDNSName, ".", 2+dc)
if len(parts) <= dc {
log.Debugf("skipping TXT record %q: too few labels for suffix %q", dns, a.suffix)
return "", ""
}
r, rType := a.dropAffixExtractType(strings.Join(parts[:1+dc], "."))
if len(parts) <= 1+dc {
return r, rType
}
return r + "." + parts[1+dc], rType
}
^ Same algorithm, no slightly misleading fallthrough call, no intermediate variable. The parts name is also more conventional than DNSName in Go.
There was a problem hiding this comment.
I have addressed this as well in the latest commit
…-dot suffix
When a multi-dot txt-suffix (e.g. ".foo.bar") is configured,
ToEndpointName panics on DNS names with fewer labels than the
suffix expects:
1. parts[:1+dc] panics when len(parts) <= dc
2. parts[1+dc] panics when len(parts) == 1+dc
Add bounds checks before both slice accesses and return ("", "")
directly for the too-few-labels case, with a debug log for
operator visibility. Return the correct result when the name has
exactly enough labels to match the suffix but no trailing labels.
This generalizes the single-label fix from kubernetes-sigs#4885, which only
guarded the zero-dot case via strings.Contains.
Signed-off-by: Jathavedhan M <jathavedhan.m@ibm.com>
carterpewpew
force-pushed
the
fix/mapper-multi-dot-suffix-panic
branch
from
5b28cc3 to
83f445d
Compare
|
[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 |
k8s-ci-robot
added
size/M
|
Thank you @ivankatliarchuk for the insights and suggestions, I have addressed all of them, PTAL |
5 participants
What does it do ?
Adds bounds checks in
AffixNameMapper.ToEndpointNameto prevent two index-out-of-bounds panics when a multi-dottxt-suffix(e.g..foo.bar) is configured and DNS names have fewer labels than the suffix expects.Motivation
ToEndpointNameis called on every TXT record duringRecords()in both the TXT and DynamoDB registries. When using a multi-dot suffix, DNS names with fewer labels than the suffix cause slice index panics that crash the controller:parts[:1+dc]panics whenlen(parts) <= dcparts[1+dc]panics whenlen(parts) == 1+dcThis generalizes the single-label fix from #4885, which only guarded the zero-dot case via
strings.Contains. The!strings.Containscheck is replaced with proper length checks that cover all short-label cases.More