feat: Support DNS resolution settings #3974
Conversation
Signed-off-by: Alexander Volchok <[email protected]>
Signed-off-by: Alexander Volchok <[email protected]>
Signed-off-by: Alexander Volchok <[email protected]>
Signed-off-by: Alexander Volchok <[email protected]>
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #3974 +/- ##
==========================================
+ Coverage 67.56% 67.59% +0.02%
==========================================
Files 183 183
Lines 22406 22444 +38
==========================================
+ Hits 15139 15171 +32
- Misses 6188 6193 +5
- Partials 1079 1080 +1 ☔ View full report in Codecov by Sentry. |
Signed-off-by: Alexander Volchok <[email protected]>
Signed-off-by: Alexander Volchok <[email protected]>
|
/retest |
| // DNSSettings includes dns resolution settings. | ||
| // | ||
| // +optional | ||
| DNSSettings *DNSSettings `json:"dnsSettings,omitempty"` |
| @@ -772,6 +772,10 @@ func processXdsCluster(tCtx *types.ResourceVersionTable, httpRoute *ir.HTTPRoute | |||
| clusterArgs.backendConnection = bt.BackendConnection | |||
| } | |||
|
|
|||
| if httpRoute.DNS != nil { | |||
There was a problem hiding this comment.
is something similar needed for grpc, tcp, udp, tls ?
There was a problem hiding this comment.
Its probably can be still applicable If the route specifies a DNS hostname, it should function for all protocols within the context of the Envoy gateway cluster.
There was a problem hiding this comment.
@arkodg - do you want to create an issue following DNS setting implementation across routes and non-routes? I think that we can start out with limited support for HTTP routes.
There was a problem hiding this comment.
yeah for xRoutes is fine for now, what I was trying to get at - the code is not uniform, for e.g. for TCP Route translation we have another code flow
gateway/internal/xds/translator/translator.go
Line 576 in cbabdc6
There was a problem hiding this comment.
Indeed, thanks for catching it.
I have just added that both to TCP and UDP routes.
There was a problem hiding this comment.
thanks @alexwo can you help with the tech debt here , is it possible to reuse processXdsCluster for other route types, so we dont miss something like this again
There was a problem hiding this comment.
Sure, I have modified processXdsCluster to be compatible with any type that can return cluster arguments, currently used for TCP, UDP, and HTTP routes.
Signed-off-by: Alexander Volchok <[email protected]>
|
/retest |
internal/xds/translator/cluster.go
Outdated
| if args.dns.DNSRefreshRate.Duration > 0 { | ||
| cluster.DnsRefreshRate = durationpb.New(args.dns.DNSRefreshRate.Duration) | ||
| } | ||
| if args.dns.RespectDNSTTL != nil { |
There was a problem hiding this comment.
should this live outside the the args.dns.DNSRefreshRate nil check, or does envoy require DNSRefreshRate to be set for RespectDNSTTL to be set?
| @@ -772,6 +772,10 @@ func processXdsCluster(tCtx *types.ResourceVersionTable, httpRoute *ir.HTTPRoute | |||
| clusterArgs.backendConnection = bt.BackendConnection | |||
| } | |||
|
|
|||
| if httpRoute.DNS != nil { | |||
There was a problem hiding this comment.
@arkodg - do you want to create an issue following DNS setting implementation across routes and non-routes? I think that we can start out with limited support for HTTP routes.
Signed-off-by: Alexander Volchok <[email protected]>
Signed-off-by: Alexander Volchok <[email protected]>
Signed-off-by: Alexander Volchok <[email protected]>
|
/retest |
Signed-off-by: Alexander Volchok <[email protected]>
Signed-off-by: Alexander Volchok <[email protected]>
|
/retest |
Signed-off-by: Alexander Volchok <[email protected]>
|
/retest |
DNS resolution settings
The purpose is to allow control over DNS resolution settings for non static cluster endpoints.
With this PR, it will be possible to define custom DNS resolution settings through a BTP policy.
Fixes # #3690