Refactor patrially enforced condition for dnspolicy

[maksymvavilov]

closes #586
fixes #685

Changing the enforced logic around DNSPolicy.
Now it works as one would expect. Also updated relevant integration tests.
Added a new test to cover all possible Enforced conditions. It is arguable if we need such a hefty test, that is, more-less, a copy of an already existent one. My reasoning is that unexpected Enforced behaviour caused a few bugs and I'd rather have tests run an extra 20-ish seconds but be confident in the scope of the test.

[codecov]

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 81.49%. Comparing base (ece13e8) to head (72e7bac).
Report is 157 commits behind head on main.

Additional details and impacted files

@@            Coverage Diff             @@
##             main     #722      +/-   ##
==========================================
+ Coverage   80.20%   81.49%   +1.28%     
==========================================
  Files          64       77      +13     
  Lines        4492     6209    +1717     
==========================================
+ Hits         3603     5060    +1457     
- Misses        600      780     +180     
- Partials      289      369      +80     

Flag	Coverage Δ
bare-k8s-integration	4.41% <0.00%> (?)
controllers-integration	71.36% <100.00%> (?)
gatewayapi-integration	10.98% <0.00%> (?)
integration	?
istio-integration	55.26% <0.00%> (?)
unit	31.67% <29.41%> (+1.64%)	⬆️

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)	85.35% <100.00%> (-6.08%)	⬇️
pkg/common (u)	88.13% <ø> (-0.70%)	⬇️
pkg/istio (u)	72.50% <ø> (-1.41%)	⬇️
pkg/log (u)	94.73% <ø> (ø)
pkg/reconcilers (u)	∅ <ø> (∅)
pkg/rlptools (u)	83.64% <ø> (+4.19%)	⬆️
controllers (i)	81.76% <83.75%> (+4.96%)	⬆️

Files	Coverage Δ
controllers/dnspolicy_status.go	82.02% <100.00%> (-4.35%)	⬇️

... and 48 files with indirect coverage changes

[mikenairn]

Couple of comments/questions but i think the states of the enforced condition looks good as i understand it:

Policy + No DNSRecords = Enforced false
Policy + DNSRecords all ready = Enforced true
Policy + DNSRecords none ready = Enforced false
Policy + DNSRecord, some ready = Partially Enforced

It does however look like it's basing these decisions on just listing DNSRecord resources that are owned by the policy, is this enough? Should we not be checking the DNSRecord for a corresponding listener exists and is ready? What would happen in the case where we have two listeners but for some reason a record wasn't being created for one of them? In this case would the Enforced condition be set to true even though there is a record missing?

TLS does something different here, i don't think its fully right either btw as it seems to do it only for one certificate, but it gets the list of expected certificates and then tries to find them, if one is missing it's going to cause it be not enforced.

[maksymvavilov]

Couple of comments/questions but i think the states of the enforced condition looks good as i understand it:

Policy + No DNSRecords = Enforced false Policy + DNSRecords all ready = Enforced true Policy + DNSRecords none ready = Enforced false Policy + DNSRecord, some ready = Partially Enforced

It does however look like it's basing these decisions on just listing DNSRecord resources that are owned by the policy, is this enough? Should we not be checking the DNSRecord for a corresponding listener exists and is ready? What would happen in the case where we have two listeners but for some reason a record wasn't being created for one of them? In this case would the Enforced condition be set to true even though there is a record missing?

TLS does something different here, i don't think its fully right either btw as it seems to do it only for one certificate, but it gets the list of expected certificates and then tries to find them, if one is missing it's going to cause it be not enforced.

It is a good point - what do we consider to be an "Enforced" DNS Policy? When initially making the change the idea was that as soon as DNS Record is published to the provider the policy is enforced. Unhealthy listeners will prevent publishing. E.G. if the listener is conflicted with another listener on the gateway it won't be published as it will conflict (what is in the MZ vs what we want) when we validate the plan or will lead to an "awaiting validation" loop on booth records.
Also one could create a situation when we don't have DNSRecord CR but it was never removed from MZ - de facto policy is enforced since there are records in the provider but the controller will never know about it as it only checks for the presence of the CR.
I'm not sure what is the best way to approach it. Would it be feasible to create an issue to address this problem @mikenairn ?

[mikenairn]

I'm not sure what is the best way to approach it. Would it be feasible to create an issue to address this problem @mikenairn ?

The current approach wasn't started in this PR so yes we can create a follow on issue to look into changing that behaviour. Can you create that follow on issue before closing this issue out?

[maksymvavilov]

I've created #792 as a follow-up @mikenairn