Skip to content
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

[dns] Add support for static DNS configuration. #14239

Closed
wants to merge 0 commits into from

Conversation

oleksandrivantsiv
Copy link
Collaborator

Why I did it

Add support for static DNS configuration. According to sonic-net/SONiC#1262 HLD.

How I did it

Add a new resolv-config.service that is responsible for transferring configuration from Config DB into /etc/resolv.conf file that is consumed by various subsystems in Linux to resolve domain names into IP addresses.

How to verify it

  • Run the image compilation. Each component related to the static DNS feature is covered with the unit tests.
  • Run sonic-mgmt tests. Static DNS feature will be covered with the system tests.
  • Install the image and run manual tests.

Which release branch to backport (provide reason below if selected)

  • 201811
  • 201911
  • 202006
  • 202012
  • 202106
  • 202111
  • 202205
  • 202211

Description for the changelog

Ensure to add label/tag for the feature raised. example - PR#2174 under sonic-utilities repo. where, Generic Config and Update feature has been labelled as GCU.

Link to config_db schema for YANG module changes

A picture of a cute animal (not mandatory but encouraged)

@ganglyu
Copy link
Contributor

ganglyu commented Apr 3, 2023

Does this feature cover DNS configuration inside container? For example, "docker exec telemetry cat /etc/resolv.conf"

@oleksandrivantsiv
Copy link
Collaborator Author

Does this feature cover DNS configuration inside container? For example, "docker exec telemetry cat /etc/resolv.conf"

Yes. During the container start the docker daemon copies "/etc/resolv.conf" from the host OS rootfs to the docker container rootfs.
But if the DNS configuration is changed in the run time when the container is running this change won't be reflected inside the docker container. In this case config reload will be required.

@qiluo-msft
Copy link
Collaborator

config reload will impact data plane, which is not necessary for DNS config. Could you improve this PR to update DNS config inside each container?


start()
{
redis-dump -d 4 -k "DNS_NAMESERVER*" -y > /tmp/dns.json
Copy link
Collaborator

Choose a reason for hiding this comment

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

-d 4

If your intention is to check if the table is empty, no need to dump.

4 is magic number, you may consider sonic-db-cli with CONFIG_DB.

@judyjoseph to check multi-asic and T2.

@qiluo-msft
Copy link
Collaborator

echo 'KUBELET_EXTRA_ARGS="--resolv-conf=/etc/resolv.conf --cgroup-driver=cgroupfs --node-ip=::"' | sudo tee -a $FILESYSTEM_ROOT/etc/default/kubelet

Wondering if KUBELET_EXTRA_ARGS is impacted by this new feature?

@lixiaoyuner could you help review?


Refers to: files/build_templates/sonic_debian_extension.j2:477 in 4fd0180. [](commit_id = 4fd01802a87f2edfe5ac75651ac5e3c7dacf146e, deletion_comment = False)

@@ -0,0 +1,3 @@
{% for ip in DNS_NAMESERVER|sort %}
nameserver {{ ip }}
Copy link
Collaborator

Choose a reason for hiding this comment

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

nameserver

In some use case, people are using "/etc/resolv.conf" to manually config DNS entries. In this PR, the content will be overwrite by j2 rendering.

Could you still allow old use case?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Those are mutually exclusive approaches. If the user wants to edit /etc/resolv.conf manually it should not configure DNS via config DB and vice-versa. The default behavior is preserved if no DNS entries are specified in the config DB.

@@ -0,0 +1,6 @@
{
"DNS_NAMESERVER": {
Copy link
Collaborator

Choose a reason for hiding this comment

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

DNS_NAMESERVER

Need new yang model for this new ConfigDB table.
Also need yang unit test.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Yang model and tests are added: #13834

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants