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

Introduce DNS_NAME_PATTERNS_TO_RESOLVE_UPSTREAM config #9692

Merged
merged 3 commits into from Dec 6, 2023

Conversation

simonrw
Copy link
Contributor

@simonrw simonrw commented Nov 20, 2023

Motivation

Sometimes we may wish to skip resolving a DNS name with the LocalStack DNS server. For example, when using transparent endpoint injection some AWS URLs need to resolve to addresses on the public internet in order to e.g. fetch something from an S3 bucket.

We currently support this feature with the configuration variable DNS_LOCAL_NAME_PATTERNS, but this name sounds like it does the opposite -> "local name patterns" sounds like it will either:

  • add a name to be resolved locally, or
  • resolve a specific name to the local address.

This is not the case.

Changes

We cannot remove this configuration variable since it is a breaking change, and we have not introduced a deprecation phase. This PR starts the deprecation phase of DNS_LOCAL_NAME_PATTERNS, and instead lets the user configure DNS_NAME_PATTERNS_TO_RESOLVE_UPSTREAM. Currently during this deprecation phase, we accept both variables, however in the future DNS_LOCAL_NAME_PATTERNS will not be used to configure DNS resolution.

TODO

@simonrw simonrw added the semver: minor Non-breaking changes which can be included in minor releases, but not in patch releases label Nov 20, 2023
@simonrw simonrw self-assigned this Nov 20, 2023
@simonrw simonrw requested a review from dfangl November 20, 2023 15:33
@localstack-bot
Copy link
Collaborator

Currently, only patch changes are allowed on master. Your PR labels (semver: minor) indicate that it cannot be merged into the master at this time.

@coveralls
Copy link

coveralls commented Nov 20, 2023

Coverage Status

coverage: 84.266% (+0.003%) from 84.263%
when pulling e611a0f on dns/rename-dns-local-name-patterns
into 0595d87 on master.

Copy link

github-actions bot commented Nov 20, 2023

LocalStack Community integration with Pro

       2 files         2 suites   1h 12m 7s ⏱️
2 397 tests 2 169 ✔️ 228 💤 0
2 398 runs  2 169 ✔️ 229 💤 0

Results for commit e611a0f.

♻️ This comment has been updated with latest results.

Copy link
Member

@dfangl dfangl left a comment

Choose a reason for hiding this comment

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

I think this is a good idea, and the new name is clearer. Not entirely sure about it, but I cannot name a better alternative for now :)

Copy link

github-actions bot commented Dec 5, 2023

S3 Image Test Results (AMD64 / ARM64)

    2 files  ±0      2 suites  ±0   3m 18s ⏱️ ±0s
377 tests ±0  327 ✔️ ±0    50 💤 ±0  0 ±0 
754 runs  ±0  654 ✔️ ±0  100 💤 ±0  0 ±0 

Results for commit e611a0f. ± Comparison against base commit 0595d87.

♻️ This comment has been updated with latest results.

@simonrw simonrw changed the title Introduce DNS_SKIP_LOCAL_RESOLUTION config Introduce DNS_NAME_PATTERNS_TO_RESOLVE_UPSTREAM config Dec 5, 2023
@simonrw simonrw force-pushed the dns/rename-dns-local-name-patterns branch from ae1ee60 to e611a0f Compare December 6, 2023 15:30
@simonrw simonrw merged commit 50c062a into master Dec 6, 2023
32 checks passed
@simonrw simonrw deleted the dns/rename-dns-local-name-patterns branch December 6, 2023 16:26
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
semver: minor Non-breaking changes which can be included in minor releases, but not in patch releases
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants