Skip to content

Move api-int record from coredns to /etc/hosts #2236

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.

Sign up for GitHub

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

Closed
cybertron wants to merge 1 commit into from
Closed

Move api-int record from coredns to /etc/hosts #2236

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
9 changes: 0 additions & 9 deletions templates/common/on-prem/files/coredns-corefile.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -27,13 +27,4 @@ contents:
match api.{{ .DNS.Spec.BaseDomain }}
fallthrough
}
template IN {{`{{ .Cluster.APIVIPRecordType }}`}} {{ .DNS.Spec.BaseDomain }} {
match api-int.{{ .DNS.Spec.BaseDomain }}
answer "{{`{{"{{ .Name }}"}}`}} 60 in {{`{{"{{ .Type }}"}}`}} {{ onPremPlatformAPIServerInternalIP . }}"
fallthrough
}
template IN {{`{{ .Cluster.APIVIPEmptyType }}`}} {{ .DNS.Spec.BaseDomain }} {
match api-int.{{ .DNS.Spec.BaseDomain }}
fallthrough
}
}
7 changes: 7 additions & 0 deletions templates/common/on-prem/files/etc-hosts.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,7 @@
mode: 0644
path: "/etc/hosts"
Copy link
Member

@cgwalters cgwalters Nov 17, 2020

Choose a reason for hiding this comment

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

People may have legitimately added custom content in here before, I don't think we can unilaterally own this file - particularly on upgrades.

I approve the general idea (this also seems related to #2190 right?).

It's too bad there's not a simple /etc/hosts.d but I guess everyone who wants this ends up using a local resolver.
So that's one option...we could switch to on-host dnsmasq or resolved.

Or we could try to hack in some sort of Ansible-like "lineinfile" support to the MCO just for this.

Copy link
Member Author

@cybertron cybertron Nov 17, 2020

Choose a reason for hiding this comment

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

This is primarily targeted at enabling openshift/enhancements#524

The problem with using dnsmasq for this is that we already run a local DNS server for the other records. It's possible we could make it work, but it gets tricky and makes the DNS flow even more complicated. Right now we configure a local server that forwards to the previously configured servers in resolv.conf. This would add another dnsmasq somewhere in that flow. I guess if there's no other way to do it then we'll have to eat the complexity, but it's not ideal either.

I also did look at using the append option in ignition and had that working. It's a weird fit for MCO though and I couldn't get it to work so I switched to just templating the whole file. It's another thing I could investigate further though.

Copy link
Member

@cgwalters cgwalters Nov 19, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

OK I see. Hum...do we have any offhand guesses around whether my concern about customers injecting /etc/hosts via Ignition is a real concern? It may not happen. If it does though, what would we tell them for configuring static resolution?

Perhaps we can implement our own "merge /etc/hosts" logic by having code in the MCO that on startup takes the current content, appends this data to it into e.g. /run/hosts and then does mount --bind /run/hosts /etc/hosts or so.

That said it feels like what we really want to achieve here is switch over the host's resolver to coredns but only if coredns is active and working? Basically if coredns is up, update /etc/resolv.conf to use 127.0.0.1:53. It's a bit like where we ended up with #2011 - we have a systemd unit which monitors a pod, and if the pod is down takes action.

Copy link

@Miciah Miciah Nov 20, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

People may have legitimately added custom content in here before, I don't think we can unilaterally own this file - particularly on upgrades.

The DNS operator adds an entry for the cluster image registry to /etc/hosts so that the container runtime can pull from the registry: https://github.com/openshift/cluster-dns-operator/blob/0d46f0303675473ce5d022904ddaff23132bac97/assets/dns/daemonset.yaml#L90-L146

Would MCO overwrite /etc/hosts only on upgrades, every time the DNS operator modified it (the DNS operator checks minutely for the entry that it adds to /etc/hosts and adds it if it is missing or outdated), or what?

It's too bad there's not a simple /etc/hosts.d but I guess everyone who wants this ends up using a local resolver.

Yeah, something similar to /etc/NetworkManager/dnsmasq.d/. A /etc/hosts.d/ or similar would be useful to the DNS operator.

That said it feels like what we really want to achieve here is switch over the host's resolver to coredns but only if coredns is active and working? Basically if coredns is up, update /etc/resolv.conf to use 127.0.0.1:53. It's a bit like where we ended up with #2011 - we have a systemd unit which monitors a pod, and if the pod is down takes action.

That would be terrific— it would obviate the need for the DNS operator to update /etc/hosts and would solve several related longstanding problems.

Copy link
Member Author

@cybertron cybertron Nov 25, 2020

Choose a reason for hiding this comment

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

Oops, I missed these comments. We did discuss the possibility of dynamically switching between dnsmasq and coredns for local resolution. It's still a bit complicated, but certainly an option.

I just pushed an alternate approach that uses a simple service to do the appending. It's fairly primitive, but it does what we need and shouldn't mess with any other customizations to /etc/hosts: #2258

contents:
inline: |
127.0.0.1 localhost localhost.localdomain localhost4 localhost4.localdomain4
Copy link
Contributor

@yboaron yboaron Nov 18, 2020

Choose a reason for hiding this comment

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

I guess we should use $cluster_name and $domain_name instead of 'ostest.test.metalkube.org' , right?

Copy link
Member Author

@cybertron cybertron Nov 18, 2020

Choose a reason for hiding this comment

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

Whoops, yeah.

::1 localhost localhost.localdomain localhost6 localhost6.localdomain6
{{ onPremPlatformAPIServerInternalIP . }} api-int api-int.ostest.test.metalkube.org