GH-551 check traversability of generated endpoints

[maksymvavilov]

Adding a new helper function that checks if endpoints on the DNSRecord are traversable or not. And using that function in existing integration suie

[codecov[bot]]

Codecov Report

All modified and coverable lines are covered by tests :white_check_mark:

Project coverage is 83.38%. Comparing base (ece13e8) to head (bd7353f). Report is 110 commits behind head on main.

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     

Flag	Coverage Δ
bare-k8s-integration	4.45% <ø> (?)
controllers-integration	72.83% <ø> (?)
gatewayapi-integration	11.20% <ø> (?)
integration	?
istio-integration	56.45% <ø> (?)
unit	32.76% <ø> (+2.73%)	:arrow_up:

Flags with carried forward coverage won't be shown. Click here to find out more.

Components	Coverage Δ
api/v1beta1 (u)	71.42% <ø> (ø)
api/v1beta2 (u)	93.58% <100.00%> (+2.16%)	:arrow_up:
pkg/common (u)	88.13% <ø> (-0.70%)	:arrow_down:
pkg/istio (u)	75.09% <ø> (+1.17%)	:arrow_up:
pkg/log (u)	94.73% <ø> (ø)
pkg/reconcilers (u)	∅ <ø> (∅)
pkg/rlptools (u)	79.84% <ø> (+0.39%)	:arrow_up:
controllers (i)	82.56% <79.96%> (+5.76%)	:arrow_up:

see 32 files with indirect coverage changes

[mikenairn]

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.

[maksymvavilov]

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 Note that it cannot guarantee the presence of the endpoint but only the presence of the target on the endpoint