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

conf(*) allow to configure DnsLookupFamily for ads_cluster of XDS server #4264

Conversation

slonka
Copy link
Contributor

@slonka slonka commented May 10, 2022

Summary

Allow users to configure DnsLookupFamily for ads_cluster of XDS server so that there are no unnecessary DNS queries.

Full changelog

  • Implement xdsHostDnsLookupFamily field which allows to set dns lookup family if XDS host is a domain

Second approach: #4275

Issues resolved

Fix #2000

Documentation

Is there any docs on this?

Testing

  • Unit tests
  • E2E tests
  • Manual testing on Universal
  • Manual testing on Kubernetes

Backwards compatibility

  • Update UPGRADE.md with any steps users will need to take when upgrading.
  • Add backport-to-stable label if the code follows our backporting policy

slonka added 3 commits May 10, 2022 16:53
Fix #2000

Signed-off-by: slonka <slonka@users.noreply.github.com>
Signed-off-by: slonka <slonka@users.noreply.github.com>
Signed-off-by: slonka <slonka@users.noreply.github.com>
@slonka slonka force-pushed the fix/allow-to-configure-add-dns-lookup-family-for-ads-cluster-of-xds-host branch from d874b04 to 121734b Compare May 10, 2022 14:53
slonka added 3 commits May 10, 2022 17:05
Signed-off-by: slonka <slonka@users.noreply.github.com>
…uster-of-xds-host' of github.com:kumahq/kuma into fix/allow-to-configure-add-dns-lookup-family-for-ads-cluster-of-xds-host
@codecov-commenter
Copy link

codecov-commenter commented May 10, 2022

Codecov Report

Merging #4264 (7bec648) into master (8545ba2) will increase coverage by 0.01%.
The diff coverage is 95.18%.

@@            Coverage Diff             @@
##           master    #4264      +/-   ##
==========================================
+ Coverage   55.68%   55.69%   +0.01%     
==========================================
  Files         935      935              
  Lines       56401    56421      +20     
==========================================
+ Hits        31405    31424      +19     
- Misses      22515    22517       +2     
+ Partials     2481     2480       -1     
Impacted Files Coverage Δ
pkg/xds/bootstrap/template_v3.go 97.67% <92.72%> (-1.50%) ⬇️
pkg/config/xds/bootstrap/config.go 57.57% <100.00%> (+1.32%) ⬆️
pkg/xds/bootstrap/generator.go 58.68% <100.00%> (+0.19%) ⬆️
...s/authn/api-server/tokens/admin_token_bootstrap.go 82.00% <0.00%> (-4.00%) ⬇️
pkg/core/secrets/manager/global_manager.go 42.42% <0.00%> (-3.04%) ⬇️
pkg/core/tokens/default_signing_key.go 77.77% <0.00%> (-2.78%) ⬇️
pkg/xds/generator/direct_access_proxy_generator.go 89.65% <0.00%> (-1.15%) ⬇️
pkg/plugins/resources/postgres/store.go 76.37% <0.00%> (-0.43%) ⬇️
pkg/core/resources/manager/cache.go 88.31% <0.00%> (+2.59%) ⬆️
pkg/core/runtime/component/component.go 92.98% <0.00%> (+3.50%) ⬆️
... and 3 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 8545ba2...7bec648. Read the comment docs.

@lahabana
Copy link
Contributor

Is there no way to make it do the right thing? Figure out if v6 is enabled and use v4 otherwise fallback to v6.
My problem with this is that it's yet another magical config users will need to set and by default the behaviour isn't great

@slonka slonka marked this pull request as ready for review May 11, 2022 07:13
@slonka slonka requested a review from a team as a code owner May 11, 2022 07:13
@slonka
Copy link
Contributor Author

slonka commented May 11, 2022

Is there no way to make it do the right thing? Figure out if v6 is enabled and use v4 otherwise fallback to v6.

Just so I'm 100% getting this right: you want to resolve the address in kuma-dp, check if ipv6 is available, if not fallback to ipv4, and then set the appropriate setting in Envoy?

@lahabana
Copy link
Contributor

Yes it's something that was done there for a similar problem: #3403

Actually should the bootstrap template use the configurers in pkg/xds/envoy/clusters/configurers? It seems to me that we rebuild the whole envoy conf from scratch when we've spent time at making good helpers in pkg/xds/envoy WDYT @jakubdyszkiewicz ?

@slonka slonka force-pushed the fix/allow-to-configure-add-dns-lookup-family-for-ads-cluster-of-xds-host branch 2 times, most recently from d8500b1 to 7bec648 Compare May 13, 2022 07:35
@slonka
Copy link
Contributor Author

slonka commented May 23, 2022

We picked this #4275 over this approach.

@slonka slonka closed this May 23, 2022
@lahabana lahabana deleted the fix/allow-to-configure-add-dns-lookup-family-for-ads-cluster-of-xds-host branch July 6, 2022 13:02
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.

ads_cluster + STRICT_DNS tries to resolve IPV6 addr
3 participants