search

cybozu-go / accurate

Kubernetes controller for multi-tenancy. It propagates resources between namespaces accurately and allows tenant users to create/delete sub-namespaces.
https://cybozu-go.github.io/accurate/
Apache License 2.0
34 stars 5 forks source link

chore: change v2alpha1 SubNamespace status to kstatus-compatible subresource #96

Closed erikgb closed 8 months ago

erikgb commented 9 months ago

A new and currently unserved API version was added with https://github.com/cybozu-go/accurate/pull/101. This PR makes status in the new version a subresource compatible with kstatus.

I have also added conversion code with unit tests, so you can get an idea of how I plan to move forward after this is (hopefully) merged. The next steps will probably be:

  1. Add conversion webhook using the conversion logic established in this PR.
  2. Make v2alpha1 served, migrate controller to reconcile v2alpha1 and make v2alpha1 the storage version.

I would prefer using SSA when updating status from controller in the latter step. Please let me know what you think! This is probably best done by generating apply-configurations in a separate pre-PR.

Relates to https://github.com/cybozu-go/accurate/issues/85

NOTE: The proposed fuzz conversion tests added in this PR are highly inspired by the equivalent test setup in https://github.com/kubernetes-sigs/cluster-api. Example: https://github.com/kubernetes-sigs/cluster-api/blob/main/api/v1alpha4/conversion_test.go

erikgb commented 9 months ago

If you want, I can split this PR:

  1. Establish the new API version as a copy of v1
  2. Make the new version compatible with kstatus
erikgb commented 9 months ago

I have now created a dedicated PR for (just) introducing the new API version: https://github.com/cybozu-go/accurate/pull/101

erikgb commented 8 months ago

@zoetrope @ymmt2005 I think this PR is ready for a round of review now. Please let me know what you think!

erikgb commented 8 months ago

Thank you for the excellent pull request! I believe the PR looks great.

I wasn't familiar with the "kstatus" library before, but it looks interesting. In situations like this, I often use apimachinery's Condition.

Thanks for the nice feedback, @zoetrope! We are also heavy users of apimachinery's Condition in our operators not yet migrated to SSA. I think the utilities in this package are mostly useful with CSA. But we will get to that in the follow-up PRs. 😉

erikgb commented 8 months ago

Is there anything more needed to get this merged?