s/edgeDNSServer/edgeDNSServers/g #605

jkremser · 2021-09-07T13:44:47Z

As described in the Issue #154 this adds the ability to fall-back to another edge dns server from the list. These are passed to the operator as env variable EDGE_DNS_SERVERS (was singular before) and the value is a comma-separted list of ips/hostnames followed optionally with port number. example: 1.1.1.1:11, 2.2.2.2,hostname (spaces between entries are supported).

Then function Dig in dns.go has been modified to work with this list (variadic function) and also Exchange function in the same file was tweaked to support it.

This change doesn't break the backward compatibility, because both EDGE_DNS_{SERVER,PORT} still works, but user is notified in the operator logs that these two were deprecated.

Some old tests needed to be modified in order to pass and I've added also these new tests:
(in controllers/depresolver/depresolver_test.go)

TestResolveConfigWithEmptyEdgeDNSServersKey
TestResolveConfigWithMalformedEdgeDNSServersKey
TestResolveConfigWithTwoEdgeDnsServers
TestResolveConfigWithMultipleEdgeDnsServers1
TestResolveConfigWithMultipleEdgeDnsServers2
TestResolveConfigWithMultipleEdgeDnsServersMalformed1
TestResolveConfigWithMultipleEdgeDnsServersMalformed2
TestResolveConfigWithInvalidEdgeDnsServersValue

(in controllers/gslb_controller_test.go)

TestReturnsExternalRecordsUsingFailoverStrategyAndFallbackDNSserver

(in controllers/internal/utils/dns_test.go)

TestOneValidEdgeDNSInTheList
TestNoValidEdgeDNSInTheList
TestEmptyEdgeDNSInTheList
TestMultipleValidEdgeDNSInTheList
TestEmptyDNSList
TestValidDigFQDNWithDot

ytsarev · 2021-09-07T14:01:16Z

Thanks for the early exposure, I've converted it to Draft

kuritka

thx for the PR. We shouldn't keep in master untested code or have possibility to start app with invalid inputs.
Please add tests and validations.

kuritka · 2021-09-07T14:11:05Z

@jkremser, I don't see all checks including linter, testrun, terratest etc. Could you make PR from feature branch?

ytsarev · 2021-09-07T14:42:41Z

@kuritka that's kinda of weird - workflows should be executed for all PRs even from the remote forks aren't they?

jkremser · 2021-09-07T14:48:06Z

thx for the PR. We shouldn't keep in master untested code or have possibility to start app with invalid inputs.
Please add tests and validations.

yes, that's still on my plate, some tests are still failing, hence the "wip" :)

it's a pr from my fork. I like this approach more, because that's how the open-source is done, nobody should assume that people will have write access to this repo, right? I can try to create a feature branch on this repo if it makes any difference

kuritka · 2021-09-07T14:49:32Z

@ytsarev , I think @jkremser must allow actions in his fork, where workflows are running

jkremser · 2021-09-07T14:54:16Z

@ytsarev , I think @jkremser must allow actions in his fork, where workflows are running

I've enabled the actions on my fork, let me try to close and open the pr

jkremser · 2021-09-08T07:46:27Z

@kuritka
Enabling the gh actions on my fork is not related to the fact that the actions are not run against the pull request from a different fork. This should be fixed on our (k8gb-io/k8gb) repo. I will create another PR for this.

kuritka

@jkremser, please squash commits into one , align PR message with squashed commit.
thx

kuritka

looks, ok.
Dig needs to be refactored and must be tested for corner cases. PR affects several parts of k8gb and I didn't have time to go through everything. I will continue with the review tomorrow.

controllers/internal/utils/dns.go

controllers/internal/utils/dns_test.go

controllers/internal/utils/dns.go

controllers/internal/utils/dns_test.go

controllers/internal/utils/dns.go

controllers/depresolver/depresolver_config.go

kuritka · 2021-09-15T12:41:42Z

controllers/depresolver/depresolver_config.go

@@ -67,8 +86,7 @@ func (dr *DependencyResolver) ResolveOperatorConfig() (*Config, error) {
 		dr.config.route53Enabled = env.GetEnvAsBoolOrFallback(Route53EnabledKey, false)
 		dr.config.ns1Enabled = env.GetEnvAsBoolOrFallback(NS1EnabledKey, false)
 		dr.config.CoreDNSExposed = env.GetEnvAsBoolOrFallback(CoreDNSExposedKey, false)
-		dr.config.EdgeDNSServer = env.GetEnvAsStringOrFallback(EdgeDNSServerKey, "")
-		dr.config.EdgeDNSServerPort, _ = env.GetEnvAsIntOrFallback(EdgeDNSServerPortKey, 53)
+		dr.config.EdgeDNSServers = parseEdgeDNSServers(env.GetEnvAsStringOrFallback(EdgeDNSServersKey, ""))


We can keep old format and transform it to EdgeDNSServers here (or for clarity, transform before starting the validations)

dr.config.EdgeDNSServers = parseEdgeDNSServers(env.GetEnvAsStringOrFallback(EdgeDNSServersKey, "")) dr.config.EdgeDNSServer = env.GetEnvAsStringOrFallback(EdgeDNSServerKey, "") dr.config.EdgeDNSServerPort, _ = env.GetEnvAsIntOrFallback(EdgeDNSServerPortKey, 53) // Make a transofrmation here (borrowing your comment): old contract, let's transform it to the new api -> will be also validated

^^ consider EdgeDNSServer, EdgeDNSServerPort private until deprecations will be removed (edgeDNSServer, edgeDNSServerPort)

This is actually my initial approach :) that's exactly what I have, but then I was thinking about the log messages for the user and the logger is not being initialized before the config (chicken egg) so I ended up setting the value in the findDeprecations, but I think you are right and we can use the findDeprecations (or GetDeprecations) merely for getting the messages and calling it on demand... instead of returning the 3-tuple 👍

actually, not exactly, this is what I have there before:

dr.config.EdgeDNSServers = parseEdgeDNSServers(env.GetEnvAsStringOrFallback(EdgeDNSServersKey, fmt.Sprintf("%s:%v", env.GetEnvAsStringOrFallback(EdgeDNSServerKey, ""), env.GetEnvAsStringOrFallback(EdgeDNSServerPortKey, "53"))))

^^ consider EdgeDNSServer, EdgeDNSServerPort private until deprecations will be removed (edgeDNSServer, edgeDNSServerPort)

nah, too messy. The api would be pretty confusing having a plural form, singular form... list of values in one thing, single value in the other... what should be used etc. definitely not going this route, it should be unified to the new version during the validation phase to make our lives easier

controllers/depresolver/depresolver_config.go

kuritka · 2021-09-15T13:21:37Z

controllers/depresolver/depresolver_config.go

 func (c *Config) GetExternalClusterNSNames() (m map[string]string) {
 	m = make(map[string]string, len(c.ExtClustersGeoTags))
 	for _, tag := range c.ExtClustersGeoTags {
-		m[tag] = getNsName(tag, c.DNSZone, c.EdgeDNSZone, c.EdgeDNSServer)
+		m[tag] = getNsName(tag, c.DNSZone, c.EdgeDNSZone, c.EdgeDNSServers[0].Host)
 	}
 	return
 }


@k0da, @ytsarev , I'm checking this, but please have a look on it as well (one EdgeDNSServer goes to multiple).
What should be GetExternalClusterNames ? (c.EdgeDNSServers[0].Host ???)

EdgeDNS server has no influence on NS name (see this), maybe it would be enough to add validation that would look for EDGE_DNS_SERVERS can be only 127.0.0.1 or localhost, or server list (except 127.0.0.1, localhost)

It would simply pick the first one and never fallback

For now, we should probably not allow the combination {127.0.0.1, host1, host2,....}, we should only allow {127.0.0.1}, or {localhost}; or {host1, host2, host....} . The locahost host should always remain as single element. @jkremser, could you provide validation rule and tests?

edgedns server has a direct influence on NS name resolution. Sticking with only one defeats the idea of multiple edgeDNS Servers to rely on unless I miss something

@kuritka @ytsarev hmm, what about the other loopback addresses (127.x.x.x)? Should I hardcode only the 127.0.0.1+localhost there or should it be more something like startsWith("127.") rule?

@jkremser actually i recalled the getNSName logic, it's fine to evaluate the first one for local scenario. Not sure how it is obvious for external guys though

I've added the check that would fail if localhost or 127.0.0.1 appears on some other position in the list + test called TestResolveConfigWithMultipleEdgeDnsServersLocalhostNotFirst

This may sound a bit nitpicky, but won't there be a difference between what the customer expects and what we will do if he insert the following value ?

EDGE_DNS_SERVERS = "localhost:5053,a.cloud.example.com, b.cloud.example.com"

That's just the reason to stick localhost with single element, or is value above supported?

right, the above example will pass the validation (as I've done it) and I am not sure if it should or should not be supported as a valid use-case. Either way I am not sure how the getNSName should behave in this case. Perhaps instead of passing the c.EdgeDNSServers[0].Host to that method, we can pass a boolean called isLocal or something similar with the intention of not doing the string concat logic in that method, but returning localhost instead... well and that boolean can be calculated as true iff the list contains only 1 element and the element.Host == localhost / 127.0.0.1

wdyt? that way we will still have the fallback mechanism for other methods (Dig, Exchange)

controllers/depresolver/depresolver_config.go

kuritka · 2021-09-15T20:47:10Z

@jkremser , is PR name s/edgeDNSServer/edgeDNSServers/g relevant ?

kuritka

please check my comments,

depresolver implementation progress well
consider more EdgeDNSServersKey corner cases and cover by tests.

kuritka · 2021-09-21T09:19:51Z

controllers/depresolver/depresolver_config.go

+type oldVar = string
+type newVar struct {
+	Name string
+	Msg  string
+}
+
+var deprecated = map[oldVar]newVar{
+	EdgeDNSServerKey: newVar{
+		Name: EdgeDNSServersKey,
+		Msg:  "Pass the hostname or IP address as comma-separated list",
+	},
+	EdgeDNSServerPortKey: newVar{
+		Name: EdgeDNSServersKey,
+		Msg:  "Port is an optional in the comma-separated list of dns edge servers, in following form: dns1:53,dns2",
+	},
+}


Consider optimization:

var deprecated = map[string]string{ EdgeDNSServerKey: "Pass the hostname or IP address as comma-separated list", EdgeDNSServerPortKey: "Port is an optional in the comma-separated list of dns edge servers, in following form: dns1:53,dns2", }

Maybe deprecated list doesn't need to be at package level when it is used by only function.

I didn't want the function to do it over and over again, but I guess that would be my cognitive baggage from my previous job on graalvm compiler (premature optimisation, yada yada..) I've learnt that package scoped vars are evil so I will move that to that function as u suggested because in our case, the function is called only once and even if it's called bazzilion times, I assume the golang compiler can do inlining and escape analysis these days.

As for the suggested new form: I need to also keep track of the "old form" -> "new form" so that user is informed about what is the remedy for the deprecation (check depresolver_config.go:L264).

kuritka · 2021-09-21T09:21:36Z

controllers/depresolver/depresolver_config.go

+		dr.config.EdgeDNSServers = parseEdgeDNSServers(env.GetEnvAsStringOrFallback(EdgeDNSServersKey,
+			fmt.Sprintf("%s:%v", env.GetEnvAsStringOrFallback(EdgeDNSServerKey, ""),
+				env.GetEnvAsStringOrFallback(EdgeDNSServerPortKey, "53"))))


Nit: possibly both can be filled EdgeDNSServersKey and <EdgeDNSServerKey; EdgeDNSServerPortKey>, but for now looks good.

right, in this case the EdgeDNSServersKey takes the precedence which is imho reasonable behavior

kuritka · 2021-09-21T09:43:15Z

controllers/depresolver/depresolver_config.go

 func (c *Config) GetExternalClusterNSNames() (m map[string]string) {
 	m = make(map[string]string, len(c.ExtClustersGeoTags))
 	for _, tag := range c.ExtClustersGeoTags {
-		m[tag] = getNsName(tag, c.DNSZone, c.EdgeDNSZone, c.EdgeDNSServer)
+		m[tag] = getNsName(tag, c.DNSZone, c.EdgeDNSZone, c.EdgeDNSServers[0].Host)
 	}
 	return
 }


For now, we should probably not allow the combination {127.0.0.1, host1, host2,....}, we should only allow {127.0.0.1}, or {localhost}; or {host1, host2, host....} . The locahost host should always remain as single element. @jkremser, could you provide validation rule and tests?

controllers/depresolver/depresolver_config.go

kuritka · 2021-09-29T14:23:02Z

👍 good job @jkremser

ytsarev · 2021-09-29T15:13:42Z

controllers/depresolver/depresolver_config.go

@@ -257,13 +350,13 @@ func parseLogOutputFormat(value string) LogFormat {
 func (c *Config) GetExternalClusterNSNames() (m map[string]string) {
 	m = make(map[string]string, len(c.ExtClustersGeoTags))
 	for _, tag := range c.ExtClustersGeoTags {
-		m[tag] = getNsName(tag, c.DNSZone, c.EdgeDNSZone, c.EdgeDNSServer)
+		m[tag] = getNsName(tag, c.DNSZone, c.EdgeDNSZone, c.EdgeDNSServers[0].Host)


What will happen if first ([0]) edge dns server is not accessible?

inside the getNsName() the dns server is used only in case it's equal to localhost or 127.0.0.1 (then it's returned -> dummy for tests) otherwise the dns server is ignored and the return value is constructed using EDGE_DNS_ZONE, geotag and what not.

After the validation it's guaranteed that there will be at least 1 element in that array so c.EdgeDNSServers[0].Host can't fail.

..is not accessible..

In other words we don't use the dns server here for resolving, instead only the string representation is used in that function. Might be probably done by some mocking + iface for tests but I am not a golang expert :D

@ytsarev , @jkremser - here is the discussion about the topic: #605 (comment)

ytsarev · 2021-09-29T15:48:05Z

controllers/depresolver/depresolver_config.go

+		},
+		EdgeDNSServerPortKey: newVar{
+			Name: EdgeDNSServersKey,
+			Msg:  "Port is an optional in the comma-separated list of dns edge servers, in following form: dns1:53,dns2",


does it default to 53 in case of dns2? if yes, then the message is not totally clear

added better message here and also to values.yaml

ytsarev · 2021-09-29T15:56:29Z

@jkremser one last thing, could you please highlight the places in code where we actually increasing reliability with the multiple edgedns servers? https://github.com/k8gb-io/k8gb/pull/605/files#diff-b93d6284f641cd9c665d0167b418250059f2ac5438acf1fe775001c13973a368R295 here and https://github.com/k8gb-io/k8gb/pull/605/files#diff-b93d6284f641cd9c665d0167b418250059f2ac5438acf1fe775001c13973a368R313 here? anywhere else?

As described in the Issue k8gb-io#154 this adds the ability to fall-back to another edge dns server from the list. These are passed to the operator as env variable `EDGE_DNS_SERVERS` (was singular before) and the value is a comma-separted list of ips/hostnames followed optionally with port number. example: `1.1.1.1:11, 2.2.2.2,hostname` (spaces between entries are supported). Then function `Dig` in `dns.go` has been modified to work with this list (variadic function) and also `Exchange` function in the same file was tweaked to support it. This change doesn't break the backward compatibility, because both `EDGE_DNS_{SERVER,PORT}` still works, but user is notified in the operator logs that these two were deprecated. Some old tests needed to be modified in order to pass and I've added also these new tests: (`in controllers/depresolver/depresolver_test.go`) - TestResolveConfigWithEmptyEdgeDNSServersKey - TestResolveConfigWithMalformedEdgeDNSServersKey - TestResolveConfigWithTwoEdgeDnsServers - TestResolveConfigWithMultipleEdgeDnsServers1 - TestResolveConfigWithMultipleEdgeDnsServers2 - TestResolveConfigWithMultipleEdgeDnsServersMalformed1 - TestResolveConfigWithMultipleEdgeDnsServersMalformed2 - TestResolveConfigWithInvalidEdgeDnsServersValue - TestResolveConfigWithMultipleEdgeDnsServersLocalhostNotFirst (in `controllers/gslb_controller_test.go`) - TestReturnsExternalRecordsUsingFailoverStrategyAndFallbackDNSserver (in `controllers/internal/utils/dns_test.go`) - TestOneValidEdgeDNSInTheList - TestNoValidEdgeDNSInTheList - TestEmptyEdgeDNSInTheList - TestMultipleValidEdgeDNSInTheList - TestEmptyDNSList - TestValidDigFQDNWithDot Signed-off-by: Jirka Kremser <[email protected]>

jkremser · 2021-09-29T16:44:07Z

@jkremser one last thing, could you please highlight the places in code where we actually increasing reliability with the multiple edgedns servers? https://github.com/k8gb-io/k8gb/pull/605/files#diff-b93d6284f641cd9c665d0167b418250059f2ac5438acf1fe775001c13973a368R295 here and https://github.com/k8gb-io/k8gb/pull/605/files#diff-b93d6284f641cd9c665d0167b418250059f2ac5438acf1fe775001c13973a368R313 here? anywhere else?

also utils.Exchange(..) called here but that's part of the dnsQuery function so impl detail
utils.Dig's signature now uses the list as well. Dig is being called from tests, but also from:
- gslb.go:104
- gslb.go:140

and that should be it

ytsarev

Ok, I guess we can merge this epic PR :)

@jkremser impressive job, thanks!

@kuritka thanks a ton for careful review

@jeffhelps it's all your fault man :D

jkremser requested review from donovanmuller, k0da, kuritka, somaritane and ytsarev as code owners September 7, 2021 13:44

jkremser force-pushed the issue-154 branch 2 times, most recently from 0674e99 to 8886cdd Compare September 7, 2021 13:45

ytsarev marked this pull request as draft September 7, 2021 14:00

kuritka requested changes Sep 7, 2021

View reviewed changes

ytsarev marked this pull request as ready for review September 7, 2021 14:41

ytsarev marked this pull request as draft September 7, 2021 14:41

jkremser force-pushed the issue-154 branch from 3f69008 to 9c77160 Compare September 7, 2021 14:49

jkremser closed this Sep 7, 2021

jkremser reopened this Sep 7, 2021

jkremser force-pushed the issue-154 branch from 7f0994b to 516df74 Compare September 9, 2021 10:32

jkremser changed the title ~~[WIP] Fix #154: s/edgeDNSServer/edgeDNSServers/g~~ Fix #154: s/edgeDNSServer/edgeDNSServers/g Sep 9, 2021

jkremser force-pushed the issue-154 branch from 516df74 to ed03fe3 Compare September 9, 2021 13:03

jkremser changed the title ~~Fix #154: s/edgeDNSServer/edgeDNSServers/g~~ s/edgeDNSServer/edgeDNSServers/g Sep 9, 2021

jkremser marked this pull request as ready for review September 9, 2021 13:14

jkremser requested a review from kuritka September 9, 2021 13:19

kuritka requested changes Sep 9, 2021

View reviewed changes

jkremser force-pushed the issue-154 branch from ed03fe3 to 835f5ae Compare September 10, 2021 14:39

jkremser requested a review from ytsarev September 13, 2021 15:26

kuritka requested changes Sep 13, 2021

View reviewed changes

controllers/internal/utils/dns_test.go Show resolved Hide resolved

controllers/internal/utils/dns.go Outdated Show resolved Hide resolved

controllers/internal/utils/dns_test.go Show resolved Hide resolved

controllers/internal/utils/dns.go Outdated Show resolved Hide resolved

jkremser force-pushed the issue-154 branch 4 times, most recently from b51ae96 to d7a9473 Compare September 14, 2021 09:27

kuritka reviewed Sep 14, 2021

View reviewed changes

controllers/internal/utils/dns.go Show resolved Hide resolved

jkremser force-pushed the issue-154 branch 2 times, most recently from 19b12ee to fd7b0e9 Compare September 15, 2021 16:28

kuritka requested changes Sep 15, 2021

View reviewed changes

jkremser force-pushed the issue-154 branch 3 times, most recently from 1444d14 to 7a2adfe Compare September 20, 2021 09:03

kuritka requested changes Sep 21, 2021

View reviewed changes

jkremser force-pushed the issue-154 branch 2 times, most recently from 6a4c3db to 8f796ad Compare September 29, 2021 12:23

kuritka previously approved these changes Sep 29, 2021

View reviewed changes

ytsarev reviewed Sep 29, 2021

View reviewed changes

jkremser dismissed kuritka’s stale review via 014a05c September 29, 2021 16:20

jkremser force-pushed the issue-154 branch from 8f796ad to 014a05c Compare September 29, 2021 16:20

jkremser force-pushed the issue-154 branch from 014a05c to 567b688 Compare September 29, 2021 16:33

ytsarev approved these changes Sep 29, 2021

View reviewed changes

jkremser merged commit 857edde into k8gb-io:master Sep 29, 2021

jkremser deleted the issue-154 branch September 29, 2021 17:52

s/edgeDNSServer/edgeDNSServers/g #605

s/edgeDNSServer/edgeDNSServers/g #605

Conversation

jkremser commented Sep 7, 2021 • edited Loading

ytsarev commented Sep 7, 2021

kuritka left a comment • edited Loading

Choose a reason for hiding this comment

kuritka commented Sep 7, 2021

ytsarev commented Sep 7, 2021

jkremser commented Sep 7, 2021

kuritka commented Sep 7, 2021

jkremser commented Sep 7, 2021

jkremser commented Sep 8, 2021

kuritka left a comment

Choose a reason for hiding this comment

kuritka left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kuritka commented Sep 15, 2021

kuritka left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kuritka commented Sep 29, 2021 • edited Loading

Choose a reason for hiding this comment

jkremser Sep 29, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ytsarev commented Sep 29, 2021

jkremser commented Sep 29, 2021

ytsarev left a comment

Choose a reason for hiding this comment

jkremser commented Sep 7, 2021 •

edited

Loading

kuritka left a comment •

edited

Loading

kuritka left a comment •

edited

Loading

kuritka commented Sep 29, 2021 •

edited

Loading

jkremser Sep 29, 2021 •

edited

Loading