add on-cluster healtcheck test and prevent enforced false on not healhy probes #977
Conversation
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #977 +/- ##
==========================================
+ Coverage 76.15% 76.91% +0.76%
==========================================
Files 111 111
Lines 8986 8944 -42
==========================================
+ Hits 6843 6879 +36
+ Misses 1852 1786 -66
+ Partials 291 279 -12
Flags with carried forward coverage won't be shown. Click here to find out more.
|
api/v1alpha1/dnspolicy_types.go
Outdated
| @@ -289,9 +289,9 @@ func (p *DNSPolicy) WithTargetGatewayListener(gwName string, lName string) *DNSP | |||
| func (p *DNSPolicy) WithHealthCheckFor(endpoint string, port int, protocol string, failureThreshold int) *DNSPolicy { | |||
| return p.WithHealthCheck(dnsv1alpha1.HealthCheckSpec{ | |||
| Path: endpoint, | |||
| Port: &port, | |||
| Port: port, | |||
There was a problem hiding this comment.
This needs to land in v1 now
There was a problem hiding this comment.
rebased. It is v1 now
There was a problem hiding this comment.
no as in it needs go into the types in the v1 folde
There was a problem hiding this comment.
I have a bad habit of commenting before the push. It is, I promise 😆
| @@ -78,3 +81,27 @@ func EnforcedCondition(policy Policy, err PolicyError, fully bool) *metav1.Condi | |||
|
|
|||
| return cond | |||
| } | |||
|
|
|||
| func HealthyCondition(policy Policy, err PolicyError) *metav1.Condition { | |||
There was a problem hiding this comment.
not 100% sure if this should be here or not @mikenairn ?
There was a problem hiding this comment.
Are you expecting this to ever be used by anything other than DNSPolicy? Seems unlikely, I'd probably move it all into the dns_workflow
There was a problem hiding this comment.
It is strange to have a condition defined in the workflow and not alongside other conditions 🤔 don't have a strong opinion on this, however - moved
There was a problem hiding this comment.
🤷 Put it wherever you like, it's not a gateway api status like "Enforce/Accepted", and is dns policy specific is all I'm saying.
| "Status": Equal(metav1.ConditionFalse), | ||
| "Reason": Equal(string(kuadrant.PolicyReasonUnknown)), | ||
| "Message": And( | ||
| ContainSubstring("DNSPolicy has encountered some issues: not all sub-resources of policy are healthy. Not healthy DNSRecords are:"), |
There was a problem hiding this comment.
should this message be
not all sub-resources of policy are passing the policy defined health check? ? might be a bit more obvious then?
There was a problem hiding this comment.
changed the message
There was a problem hiding this comment.
We need these changes against v1 now. Also not sure if we want the condition stuff under library will wait for @mikenairn on that one. Otherwise it looks good
maksymvavilov
force-pushed
the
gh-970
branch
2 times, most recently
from
bc9493b
to
1103ab0
Compare
Signed-off-by: Maskym Vavilov <[email protected]>
| ContainSubstring("Not healthy addresses"), | ||
| ContainSubstring(tests.IPAddressOne), | ||
| ContainSubstring(tests.IPAddressTwo)), | ||
| }), |
There was a problem hiding this comment.
duplicating checks from the dns-operator. This is technically redundant as it will influence enforced down the line, but I want to be sure that we are not ready for a correct reason
| Not(ContainElement( | ||
| MatchFields(IgnoreExtras, Fields{ | ||
| "Type": Equal(string(kuadrantdnsv1alpha1.ConditionTypeHealthy)), | ||
| }), |
There was a problem hiding this comment.
This one is redundant as well. If we are ready then we either don't have healthy cond or it is true. For this suite we never have healthy - true but I still would like to have a check for this
| ContainSubstring("DNSPolicy has encountered some issues: not all sub-resources of policy are passing the policy defined health check. Not healthy DNSRecords are:"), | ||
| ContainSubstring(recordName), | ||
| Not(ContainSubstring(wildcardRecordName))), // explicitly make sure that we have no probes for the wildcard record | ||
| })), |
There was a problem hiding this comment.
Checking for the absence of a wildcard here is redundant as well. If we are partially enforced and have a normal record - wildcard is ready -> wildcard is not affected by healthchecks. But still would like to have it checked
| "Message": And( | ||
| ContainSubstring("DNSPolicy has been partially enforced. Not ready DNSRecords are:"), | ||
| ContainSubstring(recordName), | ||
| Not(ContainSubstring(wildcardRecordName))), |
There was a problem hiding this comment.
If we are partially enforced with only two records it is sufficient to only check for the presence of one of the records. Leaving it here to be extra cautious.
Signed-off-by: Maskym Vavilov <[email protected]>
No description provided.