-
Notifications
You must be signed in to change notification settings - Fork 2.6k
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
Source IPv6 node IPs as AAAA endpoints #1887
Conversation
Thanks for your pull request. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA). 📝 Please follow instructions at https://git.k8s.io/community/CLA.md#the-contributor-license-agreement to sign the CLA. It may take a couple minutes for the CLA signature to be fully registered; after that, please reply here with a new comment and we'll verify. Thanks.
Instructions 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/test-infra repository. I understand the commands that are listed here. |
Welcome @danopia! |
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: danopia The full list of commands accepted by this bot can be found here.
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
It's there any update on the progress? Seems like there weren't any comments from maintainers. This PR seems to mitigate problem with IPv6. Currently external-dns creates A records with IPv6 which fails on cloud provider validation |
Correct assessment @hrvolapeter. IPv6 support in general is pretty much lacking until further work is done. Due to this (and other) painpoints, and especially after #1923 I've invested in my own from-scratch analogue of external-dns: https://github.com/danopia/kubernetes-dns-sync & no longer run external-dns myself. However I still would like to see good IPv6 support across the ecosystem so I'm keeping this PR open to see where it goes. |
Includes DNSEndpoint support, but read the comments in the template.yml for gotchas. TL;DR only A & CNAME records are supported. AAAA is most missed. re kubernetes-sigs/external-dns#1923 re kubernetes-sigs/external-dns#1887 Signed-off-by: Gerhard Lazu <gerhard@lazu.co.uk>
/kind feature |
Rebased; there was a conflict from this new line https://github.com/kubernetes-sigs/external-dns/blame/979c2d611bb7a626dfaaade5fe36f7693949a733/source/node.go#L154 which I moved down into the endpoints loop since this changeset can introduce up to two endpoint records per node. |
@danopia: PR needs rebase. Instructions 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/test-infra repository. |
The Kubernetes project currently lacks enough contributors to adequately respond to all issues and PRs. This bot triages issues and PRs according to the following rules:
You can:
Please send feedback to sig-contributor-experience at kubernetes/community. /lifecycle stale |
/remove-lifecycle stale |
The Kubernetes project currently lacks enough contributors to adequately respond to all issues and PRs. This bot triages issues and PRs according to the following rules:
You can:
Please send feedback to sig-contributor-experience at kubernetes/community. /lifecycle stale |
I'll finally accept the staleness of this; as of 2022 I have a pretty low appetite for rebasing on this codebase. Hopefully improved AAAA support will eventually merge from other contributions. |
Description
In an IPv6-enabled cluster, it's possible (and legitiment) for a node's
.status.addresses
list to contain IPv6 addresses. Usually alongside an IPv4 address for dual-stack clusters. Here's a partial example: https://kubernetes.io/docs/tasks/network/validate-dual-stack/#validate-node-addressingMultiple external-dns sources assume that every IP address is IPv4 and thus has
A
hardcoded as the record type. This results in invalid DNS endpoints being generated. With the node source, this means the sync loop is broken while such a node is still present.This PR replaces some of the
node.go
logic to support creating either an A endpoint, or an AAAA endpoint, or both at the same time, for each node. Some of the logic might be questionable and I welcome the review.Fixes #1875 but does not actually result in AAAA records being created at this time because external-dns itself does not support AAAA records yet. The planner needs some work to support dual-stack records properly and I'm considering it out of scope of upgrading each source to emit proper endpoints.
So, this PR fixes invalid A endpoints being generated, and replaces them with correct AAAA endpoints that simply aren't actioned on at this point in time.
Checklist