-
Notifications
You must be signed in to change notification settings - Fork 336
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
feat(kuma-cp) domain name support in dataplane.networking.address #965
Conversation
Signed-off-by: Ilya Lobkov <[email protected]>
Signed-off-by: Ilya Lobkov <[email protected]>
Signed-off-by: Ilya Lobkov <[email protected]>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good with a couple of nits from me.
pkg/core/alias.go
Outdated
@@ -15,6 +18,7 @@ var ( | |||
NewLogger = kuma_log.NewLogger | |||
SetLogger = kube_log.SetLogger | |||
Now = time.Now | |||
LookupIP = dns.MakeCaching(net.LookupIP, 10*time.Second) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
please parametrize this 10 seconds. I have a feeling this will be useful value to change while debugging stuff
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
do you mean to put this value into Config? Then I probably have to move LookupIP initialization into RuntimeBuilder or something like that, is it okay?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Added new parameter - Config.GeneralConfig.DNSCacheTTL
, not sure about place for this parameter in the config
if len(ips) == 0 { | ||
return errors.Errorf("can't resolve address %v", dataplane.Spec.Networking.Address) | ||
} | ||
dataplane.Spec.Networking.Address = ips[0].String() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
please double-check out all usages of Address. I see GetIP()
method on Dataplane and we use in Kuma Prometheus SD. What will happen there if we suddenly return hostname, and not IP?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I resolve the address as soon as dataplanes are fetched. Only one place has an unresolved host - MADS, but according to the docs https://prometheus.io/docs/prometheus/latest/configuration/configuration/#relabel_config, it can accept host:port
format
Signed-off-by: Ilya Lobkov <[email protected]>
Signed-off-by: Ilya Lobkov <[email protected]>
Signed-off-by: Ilya Lobkov <[email protected]>
Signed-off-by: Ilya Lobkov <[email protected]>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry, one more important thing about resolving. Other than this it looks pretty legit 👍
Signed-off-by: Ilya Lobkov <[email protected]>
Signed-off-by: Ilya Lobkov <[email protected]>
Signed-off-by: Ilya Lobkov <[email protected]>
Signed-off-by: Ilya Lobkov <[email protected]>
Do we need to update the docs as a result of this PR? |
Yes, right, I will update it |
Summary
On Universal deployment, a user has to manually set
networking.address
for Dataplane resource. It might be inconvenient since the address could be changed eventually. This PR gives an ability to set a domain name as anetworking.address
. The name will be resolved every time before xds-resource generation. By default names are cached with TTL equals to 10s.Full changelog
dataplane.networking.address
Issues resolved
Fix #755 #971
Documentation