Create/Delete HealthCheckProbeCR from DNSRecord Reconciler #271
Conversation
maksymvavilov
force-pushed
the
gh-144
branch
2 times, most recently
from
ec73121 to
526484f
Compare
maksymvavilov
changed the title
| } | ||
| }) | ||
|
|
||
| It("Should create valid probe CRs and remove them on DNSRecord deletion", func() { |
There was a problem hiding this comment.
Creating a trivial integration check. At this moment we can only verify the creation of CR to our liking and a correct removal. Will expand in tests as the work progresses
maksymvavilov
force-pushed
the
gh-144
branch
6 times, most recently
from
15d0b49 to
e9a9109
Compare
| for _, probe := range healthProbes.Items { | ||
| logger.V(1).Info(fmt.Sprintf("Deleting probe: %s", probe.Name)) | ||
| if err := r.Delete(ctx, &probe); err != nil { | ||
| return err |
There was a problem hiding this comment.
should we gather the errors and attempt a delete on each of them to avoid leavng around ones we could have deleted due to an issue with another probe?
There was a problem hiding this comment.
We will immediately requeue as the reconciler will return this error. But I see no harm in it - changed
| } else { | ||
| // we can't deal with it just by shortening one of them | ||
| // default to UID of record | ||
| value = record.GetUIDHash() |
There was a problem hiding this comment.
why don't we just use this all the time?
There was a problem hiding this comment.
actually the pobe will always be in the same namespace as the record. The record name has to be unique per namespace. So the label can just be the name of the DNSRecord it came from right? Don't need namespace here ?
There was a problem hiding this comment.
If i recall correctly it was something to do with this label being human-readable. The probe name will be a combination of hostname and IP/CNAME address of a leaf. This will not help a human to find the owner of the probe (DNS record name of gateway name + listener name)
There was a problem hiding this comment.
actually the pobe will always be in the same namespace as the record. The record name has to be unique per namespace. So the label can just be the name of the DNSRecord it came from right? Don't need namespace here ?
if we have multiple namespaces and the same names of records among them this might case a problem for the controller. Right now I'm restricting it to a namespace but I'd rather have it safe and have those label values be as unique as we can
There was a problem hiding this comment.
however, if that makes no sense I'd rather have this logic as simple as possible (either name of record or uid if the name is too long). I might be just too paranoid 🤔
There was a problem hiding this comment.
I think just go with the name as the simplest option for the controller to use and when you do the list also restrict to namespace (can never get the wrong ones then). So the list of probes is always done in the same namespace as the policy and record. You can do this with listOptions
Signed-off-by: Maskym Vavilov <[email protected]>
Create and delete
DNSHealthProbeCRs during reconciliation of the DNSRecords.This replaced logic in
internal/controller/dnsrecord_healthchecks.goto create new healthchecks but does not remove the code that would generate old healthchecks.