Allow Externally Managed CRD Fields #5730
Conversation
| @@ -267,8 +282,9 @@ func NewReconciler(mgr manager.Manager, opts ...ReconcilerOption) *Reconciler { | |||
|
|
|||
| // A Reconciler reconciles CompositeResourceDefinitions. | |||
| type Reconciler struct { | |||
| client resource.ClientApplicator | |||
| mgr manager.Manager | |||
| client resource.ClientApplicator | |||
There was a problem hiding this comment.
Does this still need to be a ClientApplicator? We don't call the Apply method anymore, right?
| // WithClientset specifies how the Reconciler should interact with the | ||
| // Kubernetes API. |
There was a problem hiding this comment.
It always bothers me a bit when we have to use both a controller-runtime client and a client-go clientset. I'm guessing its unavoidable in this case right, because the controller-runtime client can't SSA typed objects (only *Unstructured)?
| @@ -278,13 +294,16 @@ type Reconciler struct { | |||
| xrInformers composedResourceInformers | |||
|
|
|||
| options apiextensionscontroller.Options | |||
|
|
|||
| name string | |||
internal/xcrd/crd.go
Outdated
| @@ -97,6 +101,117 @@ func ForCompositeResource(xrd *v1.CompositeResourceDefinition) (*extv1.CustomRes | |||
| return crd, nil | |||
| } | |||
|
|
|||
| func ModifyApplyConfigurationForCompositeResource(crd *appcfgextv1.CustomResourceDefinitionApplyConfiguration, xrd *v1.CompositeResourceDefinition) error { //nolint:gocognit | |||
There was a problem hiding this comment.
Needing to do all of this feels a little like it defeats the point of SSA (i.e. just form a simple intent and send it). I wonder if it would be simpler to just do a get-mutate-update of the CRD? I think we could assume exclusive ownership of the CRD spec (i.e. just overwrite that each time).
|
@negz |
|
@dalton-hill-0 This looks great, and is definitely ideally how SSA would work. Unfortunately I think there are bugs when you SSA a 'real' type (i.e. a CRD) using the controller-runtime client today. See kubernetes-sigs/controller-runtime#347 (comment). This approach works in other Crossplane controllers because we're using |
That is indeed unfortunate.
I agree, and it introduces a lot of new code that could introduce bugs. However, CertManager needs to own the field at A couple of ideas:
|
This sounds good to me - if it looks like we're not actually affected in practice I'm happy to stick with the implementation you have here and maybe add a comment mentioning that we're aware of the issue but don't seem to be affected. |
|
I added comments and updated tests. |
|
@dalton-hill-0 I noticed you closed this - did you find that this approach wouldn't work? |
No that was on accident. I accidentally synced the wrong fork branch and I think it closed this as a result. |
| // We are aware that using controller-runtime server-side apply will result in | ||
| // some zero-value fields, however, this should not affect the way CRDs are | ||
| // implemented in this controller. For a discussion on the runtime issue, see | ||
| // https://github.com/kubernetes-sigs/controller-runtime/issues/347. |
There was a problem hiding this comment.
@turkenh Could you do a quick sanity check on this. I know you went pretty deep on SSA issues recently for provider-kubernetes.
I feel okay switching to SSA here as long as we don't see any issues applying CRDs in practice (which appears to be the case in @dalton-hill-0's testing.)
There was a problem hiding this comment.
Just a friendly reminder 🙂
@turkenh
There was a problem hiding this comment.
@dalton-hill-0 It looks like @turkenh is on vacation. He'll be back in a week.
There was a problem hiding this comment.
This sounds good to me.
To see whether SSA causing any immediate problem, a quick sanity test could be leaving a watch request for a CRD of an XRD open for a while, e.g. kubectl get crds <crd-name> -w, and checking whether it gets periodic updates or not (i.e. resourceVersion keeps increasing). You may restart Crossplane or add some labels to XRDs to trigger new reconciliations.
There was a problem hiding this comment.
I performed the sanity checks above and all behavior was expected.
- no increase to resource version on crossplane restart
- no increase to resource version when adding annotations to xrds (they do not get copied over from meta)
- resource version increased when adding label to xrd (copies over to crds)
- no infinite patch loop between crossplane and cert manager
Signed-off-by: dalton hill <[email protected]>
|
Rebased from master and re-tested. |
|
From what I can tell, the e2e tests that are failing are expected with the change Edit: There might be some requirement that unspecified fields get wiped after upgrade? If so, this could be problematic. |
Description of your changes
Use server-side apply for reconciling CRDs for composite resources and claims. This will allow for other processes (e.g., CertManager) to control fields on the CRD that are not owned by the reconciler.
Note: The current implementation only fixes composite resource CRDs. If this approach is desired, I will implement for Claims as well.
Fixes #5723
I have:
make reviewableto ensure this PR is ready for review.Added or updated e2e tests.Linked a PR or a [docs tracking issue] to [document this change].Addedbackport release-x.ylabels to auto-backport this PR.