Minor corner-case improvements in SubNamespace reconciler

[erikgb]

This PR will hopefully address #168, but it's a bit hard to know for sure, since I was unable to reproduce the issue in tests, even if the issue is 100% reproducible in one of our clusters. 🤔 I suggest keeping #168 open until the issue isn't reproducible in a real cluster.

This PR should be reviewed carefully, but some of the main things I have tried to address are:

• Ensure that the finalizer is always removed from SubNamespace on finalization. I'm personally not a great fan of the goto construct, and it's even more confusing when the goto is named DELETE, which is misleading IMO.
• Error handling: ignoring and not ignoring errors.
• Small, but important change in the namespace event handler: ensure events for all potential owner subnamespaces when a sub-namespace is deleted (deletionTimestamp set).
• Use a JSON Merge Patch to remove the subnamespace finalizer.
• Reviewed and added/tuned logging.

Fixes #168
Closes #196

[erikgb]

@ymmt2005 I would really love your review of this PR if you have time! 🙏

[erikgb]

I believe the failing check is a flake. Is there a way for me to retrigger the job?

[zoetrope]

@erikgb
I’ve added you as an outside collaborator with Write access. Could you try rerunning the CI to see if it works?

[erikgb]

Thanks @zoetrope! I managed to re-run the failing job now.

[zoetrope]

I confirmed that removing this return statement resolved the issue👍

[erikgb]

Nice, then I will update the PR description to close the issue when this PR is merged. How do you explain the non-failing e2e-test? It is testing exactly this.

[zoetrope]

In #168, the conflicting Namespace was created as a SubNamespace, so the Namespace has a parent label.
In contrast, in the test, the Namespace is created directly and therefore doesn’t have a parent label.

As a result, it doesn’t meet the condition at the following line, and the Reconcile process for SubNamespaces is triggered.

accurate/controllers/subnamespace_controller.go

Line 160 in af7d797

if parent != "" {

[zoetrope]

Based on my testing, it appears that even with the previous code, finalizers added by other controllers would not have been removed.

[erikgb]

Yes, that's probably true. But as the finalizer is added by our admission webhook, I think it makes sense to remove it as precisely as possible. And a JSON patch is the most exact we can use AFAIK. So I don't think this harms. Do you want me to revert this change?

[zoetrope]

No problem. This implementation works for me.

[zoetrope]

@erikgb
I think this can be merged as is. If you feel that additional tests are needed, please feel free to add them.

[erikgb]

@erikgb I think this can be merged as is. If you feel that additional tests are needed, please feel free to add them.

Thanks @zoetrope. I would love to write a new end-to-end test to verify that this PR fixes the linked issue, as it cannot be tested with envTest. Thanks for explaining how this case differs from the e2e-test we already have.

[erikgb]

@zoetrope, sorry for the very long delay, but I have now added an e2e-test to verify that this PR fixes the issue. I created a draft PR to verify that the test is effective: #196. I would have preferred to write an integration test (envTest), but envTest doesn't support namespace deletion, as documented in the kubebuilder book.

Please review when you have some time!

[zoetrope]

LGTM!!