GH-551 check traversability of generated endpoints #613
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #613 +/- ##
==========================================
+ Coverage 80.20% 83.38% +3.17%
==========================================
Files 64 72 +8
Lines 4492 5704 +1212
==========================================
+ Hits 3603 4756 +1153
- Misses 600 628 +28
- Partials 289 320 +31
Flags with carried forward coverage won't be shown. Click here to find out more.
|
maksymvavilov
changed the title
controllers/helper_test.go
Outdated
| @@ -538,6 +540,52 @@ func testBuildManagedZone(name, ns, domainName string) *kuadrantdnsv1alpha1.Mana | |||
| } | |||
| } | |||
|
|
|||
| // testEndpointsTraversable consumes an array of endpoints and returns a boolean | |||
| // indicating presence of that path from host to all IPs | |||
| func testEndpointsTraversable(endpoints []*endpoint.Endpoint, host string, ips []string) bool { | |||
There was a problem hiding this comment.
| func testEndpointsTraversable(endpoints []*endpoint.Endpoint, host string, ips []string) bool { | |
| func testEndpointsTraversable(endpoints []*endpoint.Endpoint, host string, targets []string) bool { |
There was a problem hiding this comment.
those are always an array of IP addresses 🤔 why rename?
There was a problem hiding this comment.
Why does it have to be limited to checking for ip targets?
There was a problem hiding this comment.
right now it is "I am looking for IPs. Havent found them but still have where to look / found them so succeed / haven't found them and novhere to look so fail". If I will allow it to look for targets there will be two paths: "I looking for a CNAME. Found it so look for it's target / didn't found it - fail" and "I'm looking for an IPs" one. The difference would be an extra if statement that will decide if to say it is traversable or use the current target as a new host (the later is done now by just recurring). And this will, actually, lead to more calls (more expecive to run) if I'm not mistaken.
controllers/helper_test.go
Outdated
| } | ||
| } | ||
| // at least one endpoint that matches host targeted IP | ||
| return allTargetsFound |
There was a problem hiding this comment.
Does this not return on the first found target?
There was a problem hiding this comment.
No, it returns "at least one emdpoint has IP" after we went though the list of all endpoints
There was a problem hiding this comment.
@mikenairn are you ok with the changes made?
|
This doesn't really do what was asked for in #551, and you have gone a different route of checking the eventual target IP(s) are reachable. This may or may not be useful, but we probably want to consider why #551 was created in the first place. There were a number of PRs being opened that were changing the tests to suit the results they were seeing, in particular when it came to generated hashes in the dns names, and even though our current tests do check all endpoints in most cases, the verbosity of those tests makes it very difficult to be sure that things weren't changing that shouldn't be unless you really looked for it in reviews. To make this less likely to happen we created #551 to add validation to make sure one dns name could always be reached from another i.e. test.example.com -> clusterHash-gwHash.klb.test.example.com, and it was to be a generic enough function that would allow us to use it wherever it was needed, so could be used to check the geo dns names were still connected for example i.e. test.example.com -> geo1.klb.test.example.com. This would allow the addition of explicit steps to assert the structure of the endpoints where it is important to do so. The solution given here is to make sure that all the IPs given are reachable from a given host, but it doesn't really check how they come to be accessible, you could make that test pass and still have an endpoints list that we don't really desire. We already check the endpoint target values explicitly, what advantage are we gaining by doing this? It doesn't really tell me that my geo target is still accessible from the root host, just that the IP is a target somewhere somehow. This also highlights that we have no tests checking for CNAME targets. It is the case that an endpoint list can be created that has no IPs, but rather it creates a CNAME record pointing to a loadbalancer dns name. The current solution of checking the ip's only wouldn't allow for a test that is checking for a known CNAME target, if we want to check targets it should at least allow this. |
|
The last comment was discussed in the call. It was decided to keep this test checking for targets on the endpoint but not restrain it to A records only. Now it can consume an array of targets (destinations) to reach |
| // testEndpointsTraversable consumes an array of endpoints and returns a boolean | ||
| // indicating presence of that path from host to all destinations | ||
| // this function DOES NOT report a presence of an endpoint with one of destinations DNSNames | ||
| func testEndpointsTraversable(endpoints []*endpoint.Endpoint, host string, destinations []string) bool { |
There was a problem hiding this comment.
You really don't want to call this targets 🤷♂️
Since you want this to take a host and a list of expected endpoint "target" values it should be able to reach, using a completely different name for the concept seems odd.
There was a problem hiding this comment.
it will create confusion since we have endpoint.targets and a list of targets we are trying to reach. They were called targets and endpointTargets for a while but this does not help. But yes, i really want to avoid calling them targets as much as I can: the function is awkward to read as is and creating similar names won't help.
Adding a new helper function that checks if endpoints on the DNSRecord are traversable or not. And using that function in existing integration suie