Skip to content

protokube: support writing AAAA records to /etc/hosts#15931

Merged
k8s-ci-robot merged 1 commit into
kubernetes:masterfrom
justinsb:support_ipv6_in_protokube
Sep 21, 2023
Merged

protokube: support writing AAAA records to /etc/hosts#15931
k8s-ci-robot merged 1 commit into
kubernetes:masterfrom
justinsb:support_ipv6_in_protokube

Conversation

@justinsb
Copy link
Copy Markdown
Member

Though we likely want to stop using this code altogether, it is still
used on GCE (for now). dns-controller will "gossip" AAAA records, and
they are valid (and work) in /etc/hosts.

@k8s-ci-robot k8s-ci-robot added the cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. label Sep 19, 2023
@k8s-ci-robot k8s-ci-robot added the size/S Denotes a PR that changes 10-29 lines, ignoring generated files. label Sep 19, 2023
@justinsb
Copy link
Copy Markdown
Member Author

This exercises the node -> apiserver ipv6 code path, rather than relying on ipv4. Suggested by @aojea , because then we can get one step closer to a pure-IPv6 world.

klog.Warningf("ignoring unexpected lines in /etc/hosts: %v", badLines)
}

hostsToAddr := make(map[string][]string)
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

if you make a map then one hostname can only have one IP family, if that is your preference I suggest you to default always to the same one, usually ipv4. Otherwise you can create one mapping per ip familiy, this will give you a cleaner separation in the code between families

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So this is a map from hosts to ip addresses, the addresses can be a mix of ipv4 and ipv6 (i.e. we build this map by appending to the existing map entries).

I actually introduced this map because previously the code did replace the addresses, so ipv4 would overwrite the ipv6 address.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

(It's confusing because this map is "backwards" compared to /etc/hosts; I find the mapping in /etc/hosts unintuitive considering how we are using it, so we try to keep the address -> names mapping isolated in hosts.HostMap)

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

osh, so record.Name is an IP address?

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

are not Rrdatasthe ip addresses?

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

oh, nevermind, I got it now

Comment thread protokube/pkg/gossip/dns/hosts.go Outdated
@aojea
Copy link
Copy Markdown
Member

aojea commented Sep 21, 2023

LGTM

Though we likely want to stop using this code altogether, it is still
used on GCE (for now).  dns-controller will "gossip" AAAA records, and
they are valid (and work) in /etc/hosts.

Co-authored-by: John Gardiner Myers <jgmyers@proofpoint.com>
@justinsb justinsb force-pushed the support_ipv6_in_protokube branch from 38a8e4a to d9d9134 Compare September 21, 2023 09:23
@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Sep 21, 2023
@k8s-ci-robot
Copy link
Copy Markdown
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: johngmyers

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Details Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@k8s-ci-robot k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Sep 21, 2023
@k8s-ci-robot k8s-ci-robot merged commit ca0e5a2 into kubernetes:master Sep 21, 2023
@k8s-ci-robot k8s-ci-robot added this to the v1.29 milestone Sep 21, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

approved Indicates a PR has been approved by an approver from all required OWNERS files. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. lgtm "Looks good to me", indicates that a PR is ready to be merged. size/S Denotes a PR that changes 10-29 lines, ignoring generated files.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants