Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Externally Managed CRD Fields #5723

Open
dalton-hill-0 opened this issue May 21, 2024 · 6 comments · May be fixed by #5730
Open

Externally Managed CRD Fields #5723

dalton-hill-0 opened this issue May 21, 2024 · 6 comments · May be fixed by #5730
Labels
composition enhancement New feature or request

Comments

@dalton-hill-0
Copy link
Contributor

dalton-hill-0 commented May 21, 2024

What problem are you facing?

When configuring an XRD with a conversion webhook and CA Bundle injection via CertManager, the resulting CRD reconciliation enters an infinite patch loop between Crossplane and CertManager.

The loop behaves as follows:

  1. Crossplane creates the CRD representing either the XR or Claim. No CA Bundle is present in the spec.
  2. CertManager notices the CRD is marked for CA Bundle injection and patches the CRD with the proper CA Bundle.
  3. Crossplane reconciles the updated CRD and removes the CA Bundle.
  4. Go to step 2.

This results in:

  • An infinite spam of reconciliations for the affected CRD.
  • A conversion webhook which is hard to reach, as you must get lucky and hope your request connects when the CRD has the CA Bundle present.

How could Crossplane help solve your problem?

Two Three approaches come to mind.

Hard-Coded Approach
Add hard-coded logic in Crossplane's Claim CRD and CompositeResource CRD reconcilers that is specific to CertManager. This logic would check for a CA Bundle in the existing CRD and copy it over before applying the spec derived from the XRD.

Generic Approach
Extend the XRD API to allow users to specify paths that are externally managed (e.g., by CertManger). For each path, the CRD reconciler would attempt to copy the value from existing CRD before applying the CRD derived from the XRD.

Using Patch Applicator
We could replace the usage of NewAPIUpdatingApplicator with NewAPIPatchingApplicator for the CRD reconcilers. I tested this and it solves the issue, but may have negative side effects.

@dalton-hill-0 dalton-hill-0 added the enhancement New feature or request label May 21, 2024
@negz
Copy link
Member

negz commented May 21, 2024

@dalton-hill-0 WRT using NewAPIPatchingApplicator, would using server-side apply to apply the CRDs work? I think eventually we'd like to remove our applicators and just use either SSA or get-mutate-update.

@negz
Copy link
Member

negz commented May 21, 2024

Also, breadcrumbs to #4149 which I think was related.

@dalton-hill-0
Copy link
Contributor Author

@dalton-hill-0 WRT using NewAPIPatchingApplicator, would using server-side apply to apply the CRDs work? I think eventually we'd like to remove our applicators and just use either SSA or get-mutate-update.

@negz
I believe this should work. I will throw something together and let you know how it goes.

@dalton-hill-0 dalton-hill-0 linked a pull request May 23, 2024 that will close this issue
6 tasks
@dalton-hill-0
Copy link
Contributor Author

@negz

I opened a draft PR. This currently implements SSA for the XR CRDs. I tested this locally and it fixes the CertManager issue mentioned in this thread. If this is the desired approach, I can implement the same for claims.

Copy link

Crossplane does not currently have enough maintainers to address every issue and pull request. This issue has been automatically marked as stale because it has had no activity in the last 90 days. It will be closed in 14 days if no further activity occurs. Leaving a comment starting with /fresh will mark this issue as not stale.

@github-actions github-actions bot added the stale label Aug 22, 2024
@stevendborrelli
Copy link
Contributor

/fresh

@github-actions github-actions bot removed the stale label Aug 22, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
composition enhancement New feature or request
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants