NS Deletion hardening #156
Conversation
|
Changes here look fine, but does this cover everything that was being asked for from #106? The change to setting an |
hmm, yeah, that seems out of scope for the title of that issue, but it is mentioned in the body alright and I'd overlooked it. I'll make some changes to cover the extra areas. |
philbrookes
force-pushed
the
gh-106
branch
3 times, most recently
from
58d2e7f
to
41ec695
Compare
philbrookes
force-pushed
the
gh-106
branch
2 times, most recently
from
e896ab7
to
5c62c6f
Compare
| } | ||
| err = r.Client.Update(ctx, dnsRecord) | ||
| return ctrl.Result{}, err | ||
| } |
There was a problem hiding this comment.
Arguably not gaining anything by setting this relationship up since it didn't work in the way that we/i hoped (Doesn't trigger reconciles of children when the owner changes).
| }, | ||
| } | ||
| err = r.Get(ctx, client.ObjectKeyFromObject(managedZone), managedZone, &client.GetOptions{}) | ||
| if err != nil { |
There was a problem hiding this comment.
what if the managed zone is not found when the dns record is being deleted? The finalizer on the dnsrecord will never be removed
There was a problem hiding this comment.
Yes, this is the intention, that means that DNS Records have been created in the DNS Provider and have not been cleaned up.
There was a problem hiding this comment.
Well, you might be right. I know I do not know. But that is causing an issue anyway. Some tests are failing because the dns record is never gone after having deleted it.
There was a problem hiding this comment.
@philbrookes Is this the intention if a user deletes a ManagedZone first before deleting the DNSRecords ? 🤔 This is how the integration tests are in Kuadrant Operator. In the tests, the ManagedZone has completed their deletion, but DNSRecords remained, and prevented the test NS deletion.
Seems like this introduces a required order of deletion for the user. So maybe the ManagedZone deletion should wait until the assoicated DNSRecords are deleted ? 🤔
If this is the intended behaviour anyway, I've a fix to update the Kuadrant Operator DNSPolicy tests as part of a PR Kuadrant/kuadrant-operator@c70b5e1. Just want to check this is the expected flow being merging
There was a problem hiding this comment.
Yes, this is the intention, if the DNS Record is deleted after the managed zone, the DNS Operator cannot remove the records from the DNS Provider, in order to highlight this to the user/admin the DNS Record never has it's finalizer removed.
See the original issue here: #106
There was a problem hiding this comment.
Cool, thanks, I'll merge the PR containing the changes to the DNS Policy tests to account for this 👍
…one + timeouts Since Kuadrant/dns-operator#156 was merged, dnsrecords with finizalers are left behind as ManagedZone was deleted causing the test namespaces to never be fully deleted. Additionally use SpecContext and add timeouts for context so problematic tests terminate earlier
…one + timeouts Since Kuadrant/dns-operator#156 was merged, dnsrecords with finizalers are left behind as ManagedZone was deleted causing the test namespaces to never be fully deleted. Additionally use SpecContext and add timeouts for context so problematic tests terminate earlier
* make: concurrent tests flags * refactor: concurrent bare k8s tests * refactor: concurrent gateway api k8s tests * refactor: concurrent istio tests * refactor: test packages for base concurrent integration tests * refactor: do not wait kuadrant to be ready for integration tests * fix: sort policy by kind, creation, condition for target status * refactor: separate out serial tests that can affect other tests * harden: RLP integration test updates & sort by PolicyByTargetRefKindAndAcceptedStatus tests * refactor: rename ratelimit test package to ratelimitpolicy * fix: wait for dnsrecords to finish deleting before deleting managed zone + timeouts Since Kuadrant/dns-operator#156 was merged, dnsrecords with finizalers are left behind as ManagedZone was deleted causing the test namespaces to never be fully deleted. Additionally use SpecContext and add timeouts for context so problematic tests terminate earlier
* make: concurrent tests flags * refactor: concurrent bare k8s tests * refactor: concurrent gateway api k8s tests * refactor: concurrent istio tests * refactor: test packages for base concurrent integration tests * refactor: do not wait kuadrant to be ready for integration tests * fix: sort policy by kind, creation, condition for target status * refactor: separate out serial tests that can affect other tests * harden: RLP integration test updates & sort by PolicyByTargetRefKindAndAcceptedStatus tests * refactor: rename ratelimit test package to ratelimitpolicy * fix: wait for dnsrecords to finish deleting before deleting managed zone + timeouts Since Kuadrant/dns-operator#156 was merged, dnsrecords with finizalers are left behind as ManagedZone was deleted causing the test namespaces to never be fully deleted. Additionally use SpecContext and add timeouts for context so problematic tests terminate earlier
closes: #106
Moved some logic around as we were pulling the managed zone from the API in a few places, instead I've just pulled it in the main reconcile and passed it around. This also meant that I could verify that the MZ and it's secret existed before moving on to setting finalizers and owner references.
In other words, if a DNS Record is created and it's managed zone (or that managed zones secret) has never existed, it will not get the cleanup finalizer, this makes more sense to me, as this record will never have sent anything to a DNS Provider.
I had to modify a test to reflect this more accurately.
New watches:
New metric: