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

Copy correct resolv.conf for Bottlerocket in debug mode. #4186

Open
wants to merge 6 commits into
base: dev
Choose a base branch
from

Conversation

hoshank10
Copy link
Contributor

@hoshank10 hoshank10 commented May 22, 2024

Summary

In EC2 debug mode, while setting up the dns config we copy the `/etc/resolv.conf` to `/etc/netns/(NetNSName)/`. This works fine on AL2 hosts, although on Bottlerocket hosts the `/etc/resolv.conf` is symlinked to a dummy stub `resolv.conf`. Instead the correct file to use for Bottlerocket is at `/run/netdog/resolv.conf`. This change copies the correct file to task network namespace dns config for debug mode on bottlerocket.

Implementation details

Check if the file `/run/netdog/resolv.conf` exists on the host and copy it to task network namsepace.

Testing

New tests cover the changes: NA

Description for the changelog

Copy correct resolv.conf for Bottlerocket to launch tasks in debug mode.

Does this PR include breaking model changes? If so, Have you added transformation functions? No

Licensing

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.

@hoshank10 hoshank10 requested a review from a team as a code owner May 22, 2024 00:56
@hoshank10 hoshank10 requested a review from samjkon May 22, 2024 16:25
err = c.copyFile(filepath.Join(netNSDir, ResolveConfFileName), "/etc/resolv.conf", taskDNSConfigFileMode)
// Check if the /run/netdog/resolv.conf file exists on the host. For Bottlerocket, instead of /etc/resolv.conf,
// this is the file we copy to the task network namespace dns config.
if _, err = c.os.Stat(srcBRResolv); err == nil {
Copy link
Contributor

Choose a reason for hiding this comment

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

Is there a better way to detect if the host is using a bottle rocket AMI?

Choose a reason for hiding this comment

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

I think we should create a bottle-rocket specific platform in Network builder. Maybe not in this PR, but as a todo long-term. I can take that AI.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Agreed. Thanks Jose

Copy link
Contributor

Choose a reason for hiding this comment

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

Is /run/netdog/resolv.conf guaranteed to exist on Bottlerocket only and not on any other supported distros? I second @samjkon's comment.

Copy link
Contributor Author

@hoshank10 hoshank10 May 23, 2024

Choose a reason for hiding this comment

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

I checked with a Bottlerocket Engineer and he confirmed the /run/netdog/resolv.conf will only exist in Bottlerocket. Although we could add additional checks in the if condition to confirm its a BR AMI but I don't feel its necessary since checking for /run/netdog/resolv.conf is sufficient

Choose a reason for hiding this comment

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

The risk with this approach is that you're coupling the ECS Agent with special behavior which is OS/distro-specific. Should Bottlerocket change this behavior, or if other OSes introduce different behavior, then this pattern suggests that we should update the ECS Agent to accommodate that behavior. Instead is it possible to introduce a configuration value to override the default resolv.conf location? By default the option can point to /etc/resolv.conf; should you need to override it, it can instead be configured with whatever distro the ECS Agent is included on.

)

var srcResolv = "/etc/resolv.conf"
Copy link
Contributor

Choose a reason for hiding this comment

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

Instead of making this a var, I would suggest keeping this a const. You can use a local var in the function instead of creating a global var if needed.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ack, will make that change

@hoshank10 hoshank10 changed the base branch from master to dev May 23, 2024 16:59
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.

None yet

7 participants