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

enhancement(link_local_dns.go): avoid occur runtime panic when the nameservers are empty. #393

Merged
merged 1 commit into from
May 20, 2024

Conversation

orange-guo
Copy link
Contributor

@orange-guo orange-guo commented May 17, 2024

Summary

Use return to instead of runtime panic if there no nameservers are found in /etc/resolve.conf

In the following case, the nameservers are empty

The container uses the host network but the nameserver is empty in the host file

Use Cases

If nameservers are empty, no runtime panic

Checklist

  • I have viewed, signed, and submitted the Contributor License Agreement.
  • I have linked issue(s) that this PR should close using keywords or the Github UI (See docs)
  • I have added an integration test, if necessary.
  • I have reviewed the styleguide for guidance on my code quality.
  • I'm happy with the commit history on this PR (I have rebased/squashed as needed).

@orange-guo orange-guo requested a review from a team as a code owner May 17, 2024 07:22
Copy link

linux-foundation-easycla bot commented May 17, 2024

CLA Signed


The committers listed above are authorized under a signed CLA.

@dmikusa dmikusa added type:bug A general bug semver:patch A change requiring a patch version bump labels May 17, 2024
@dmikusa
Copy link
Contributor

dmikusa commented May 17, 2024

This looks reasonable to me.

When this case occurs, and you have no DNS servers (i.e. /etc/resolv.conf is empty), what happens in your app with regards to DNS? Do requests fails (I'm guessing)? Or is your app just not using DNS?

I'm mostly curious because this helper will set the following two config settings, which disable DNS caching in the JVM, and I'm wondering if it might still be a good idea to do this if that file is empty. Even if you have no DNS servers, if your app tried to resolve a DNS that would presumably fail and the JVM might cache that result (not 100% sure it caches failures, or just successes, I didn't look). Probably not a big deal either way, but thought I'd ask.

networkaddress.cache.ttl=0
networkaddress.cache.negative.ttl=0

Thanks!

@orange-guo
Copy link
Contributor Author

orange-guo commented May 18, 2024

The application is deployed into the on-prem environment in my case, so the application does not use DNS.

DNS is one of the ways to find a host

In Linux, /etc/nsswitch.conf is used to configure how the system finds a host,
for example:

hosts: dns nis files

If the host is not found from the aforementioned sources, Java may throw an error similar to "Temporary failure in name resolution."

JVM parameters.

networkaddress.cache.ttl
networkaddress.cache.negative.ttl

These parameters are looks like the host caching parameters

In cases no DNS, the decision to disable caching may depend on the frequency of content changes in other host sources, such as (files, nis) in /etc/nsswitch.conf.

However, it is generally difficult to make assumptions about the frequency of these changes.

Therefore, I think that explicitly configuring to disable the cache might be a better approach.

This approach is ensure that the system does not rely on potentially outdated or incorrect information from other host sources.

Copy link
Contributor

@dmikusa dmikusa left a comment

Choose a reason for hiding this comment

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

Ok, that sounds reasonable to me. Thanks for following up with those additional details & thanks for the PR!

@dmikusa dmikusa merged commit 62a9f74 into paketo-buildpacks:main May 20, 2024
6 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
semver:patch A change requiring a patch version bump type:bug A general bug
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants