source/pod: skip internal-hostname pod when PodIP is empty#6382
Conversation
k8s-ci-robot
added
the
do-not-merge/invalid-commit-message
|
[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 |
|
Welcome @SAY-5! |
k8s-ci-robot
added
the
needs-ok-to-test
|
Hi @SAY-5. 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
size/S
| // Pods in Pending / Scheduled state without a PodIP must | ||
| // not emit an endpoint: endpoint.SuitableType("") returns | ||
| // CNAME, which would create a provider record whose target | ||
| // is the empty string. Once the pod eventually gets an IP | ||
| // external-dns then tries to create an A record at the | ||
| // same name and the provider rejects it because A and | ||
| // CNAME cannot coexist (#6375, #6199 on Azure / RFC2136). |
There was a problem hiding this comment.
I don’t think this comment is necessary, as it’s already clear why we filter out pods without an IP. Endpoint shouldn't have an empty address.
vflaux
left a comment
vflaux
left a comment
There was a problem hiding this comment.
pod.Status.PodIP is also used in addKopsDNSControllerEndpoints() and addPodSourceDomainEndpoints(). I think we need to filter there too.
Could you also add tests for those cases?
vflaux
left a comment
vflaux
left a comment
There was a problem hiding this comment.
I'm surprised that an endpoint with an empty target is accepted by your provider, I would expect it to fail.
|
Good catch. Added the same empty-PodIP guard to addKopsDNSControllerEndpoints and addPodSourceDomainEndpoints, and dropped the now-redundant comment block in addInternalHostnameAnnotationEndpoints. Also added two table-test cases covering the kops and pod-source-domain paths with a pod that has no PodIP. |
k8s-ci-robot
added
needs-rebase
addInternalHostnameAnnotationEndpoints called endpoint.SuitableType(pod.Status.PodIP) and addToEndpointMap without first checking whether PodIP was empty. Pending or Scheduled pods that carry external-dns.alpha.kubernetes.io/internal-hostname ended up producing a CNAME record whose target was the empty string, because SuitableType on an empty string falls through to CNAME. Once the pod got a PodIP and external-dns tried to write an A record at the same name, the provider rejected the update (A and CNAME cannot coexist), leaving the zone permanently stuck on the empty-target CNAME. Skip the pod entirely when PodIP is empty. The pod will be reconciled again once the kubelet assigns an address. Ref: kubernetes-sigs#6375 Signed-off-by: SAY-5 <SAY-5@users.noreply.github.com>
…mpty Apply the same empty-PodIP guard to addKopsDNSControllerEndpoints and addPodSourceDomainEndpoints so kops-dns-controller and pod-source-domain pods without an assigned address do not emit empty-target CNAME records. Signed-off-by: SAY-5 <SAY-5@users.noreply.github.com>
SAY-5
force-pushed
the
fix/pod-source-skip-empty-podip-6375
branch
from
bad3cf5 to
3115d27
Compare
k8s-ci-robot
removed
the
do-not-merge/invalid-commit-message
|
Rebased onto master to drop the merge commit and reworded the first commit to reference the issue by URL instead of a closing keyword, which clears the invalid-commit-message check. Also folded the kops and pod-source-domain guards into a second commit. Tests pass locally. |
3 participants
Description
addInternalHostnameAnnotationEndpointscalledendpoint.SuitableType(pod.Status.PodIP)andaddToEndpointMapwithout first checking whether PodIP was empty. Pending / Scheduled pods carryingexternal-dns.alpha.kubernetes.io/internal-hostnameemit a CNAME record whose target is the empty string (becauseSuitableType("")falls through toCNAME). Once the pod gets a PodIP and external-dns tries to write an A record at the same name, the provider rejects the update (A and CNAME cannot coexist), leaving the zone permanently stuck on the empty-target CNAME, the exact scenario the reporter hit on Azure Private DNS.Fix
Skip the pod entirely when
PodIPis empty. The pod will be reconciled again once the kubelet assigns an address.Fixes
Fixes #6375
Checklist
go test ./source/...).go build ./source/...clean.gofmtclean.Signed-off-by: SAY-5 SAY-5@users.noreply.github.com