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

Can't delete XRD due to missing "DeleteOptions" #2284

Closed
negz opened this issue May 3, 2021 · 11 comments · Fixed by #2285
Closed

Can't delete XRD due to missing "DeleteOptions" #2284

negz opened this issue May 3, 2021 · 11 comments · Fixed by #2285
Labels
bug Something isn't working

Comments

@negz
Copy link
Member

negz commented May 3, 2021

What happened?

When deleting an XRD I'm seeing the following event, and my XRD refuses to delete.

Warning  TerminateComposite  2m37s                   defined/compositeresourcedefinition.apiextensions.crossplane.io  cannot delete defined composite resources: no kind "DeleteOptions" is registered for version "admissionregistration.k8s.io/v1" in scheme "k8s.io/apiextensions-apiserver/pkg/apiserver/apiserver.go:52"

Interestingly I never actually created any XRs using this XRD.

How can we reproduce it?

Here's the XRD I'm using:

apiVersion: apiextensions.crossplane.io/v1
  kind: CompositeResourceDefinition
  metadata:
    creationTimestamp: "2021-05-03T22:41:16Z"
    deletionGracePeriodSeconds: 0
    deletionTimestamp: "2021-05-03T22:41:26Z"
    finalizers:
    - defined.apiextensions.crossplane.io
    generation: 2
    name: clusterconformances.test.crossplane.io
    resourceVersion: "950166"
    uid: f43c8d95-25d0-4b0d-99d2-57961b8ebf19
  spec:
    claimNames:
      kind: Conformance
      plural: conformances
    group: test.crossplane.io
    names:
      kind: ClusterConformance
      plural: clusterconformances
    versions:
    - name: v1alpha1
      referenceable: true
      schema:
        openAPIV3Schema: {}
      served: true
  status:
    conditions:
    - lastTransitionTime: "2021-05-03T22:41:26Z"
      reason: TerminatingCompositeResource
      status: "False"
      type: Established
    - lastTransitionTime: "2021-05-03T22:41:26Z"
      reason: TerminatingCompositeResourceClaim
      status: "False"
      type: Offered
    controllers:
      compositeResourceClaimType:
        apiVersion: test.crossplane.io/v1alpha1
        kind: Conformance
      compositeResourceType:
        apiVersion: test.crossplane.io/v1alpha1
        kind: ClusterConformance

This is the first time I'm testing composition with a Kubernetes cluster newer than 1.16. Perhaps this is due to the newer Kubernetes version I'm running?

What environment did it happen in?

Crossplane version:

$ kubectl version
Client Version: version.Info{Major:"1", Minor:"21", GitVersion:"v1.21.0", GitCommit:"cb303e613a121a29364f75cc67d3d580833a7479", GitTreeState:"clean", BuildDate:"2021-04-08T16:31:21Z", GoVersion:"go1.16.1", Compiler:"gc", Platform:"linux/amd64"}
Server Version: version.Info{Major:"1", Minor:"20", GitVersion:"v1.20.2", GitCommit:"faecb196815e248d3ecfb03c680a4507229c2a56", GitTreeState:"clean", BuildDate:"2021-03-11T06:23:38Z", GoVersion:"go1.15.5", Compiler:"gc", Platform:"linux/amd64"}

$ helm -n crossplane-system list
NAME            NAMESPACE               REVISION        UPDATED                                 STATUS          CHART                   APP VERSION
crossplane      crossplane-system       1               2021-04-30 23:55:47.823185221 +0000 UTC deployed        crossplane-1.2.0        1.2.0   
@negz negz added the bug Something isn't working label May 3, 2021
@negz
Copy link
Member Author

negz commented May 3, 2021

It's worth noting we're using DeleteAllOf from controller-runtime on the offending line, and it's the only place we use DeleteAllOf in our codebase.

if err := r.client.DeleteAllOf(ctx, o); err != nil && !kmeta.IsNoMatchError(err) && !kerrors.IsNotFound(err) {

@negz
Copy link
Member Author

negz commented May 3, 2021

Did a little digging. I can't immediately find any related issues on controller-runtime or the greater Googles. I've determined that this was a regression between Crossplane v1.1.1 and v1.2.0. I can reproduce it on both Kubernetes v1.16.15 and v.1.20.2 using Crossplane v1.2.0.

@negz
Copy link
Member Author

negz commented May 3, 2021

Hrm, I had hoped to blame some underlying controller-runtime or API machinery changes between 1.1.1 and 1.2.0 but as far as I can tell none of that changed. 🤔 release-1.1...release-1.2 (see go.mod).

@negz
Copy link
Member Author

negz commented May 3, 2021

Worth noting that we did go from crossplane-runtime ~v0.12 to v0.13, which bumped its version of k8s.io/apiextensions-apiserver from v0.18.6 to v0.20.1.

@negz
Copy link
Member Author

negz commented May 4, 2021

# v1.2.0
$ go list -m all|grep k8s.io
k8s.io/api v0.20.1
k8s.io/apiextensions-apiserver v0.20.1
k8s.io/apimachinery v0.20.1
k8s.io/apiserver v0.20.1
k8s.io/client-go v0.20.1
k8s.io/cloud-provider v0.19.7
k8s.io/code-generator v0.20.1
k8s.io/component-base v0.20.1
k8s.io/csi-translation-lib v0.19.7
k8s.io/gengo v0.0.0-20201113003025-83324d819ded
k8s.io/klog v1.0.0
k8s.io/klog/v2 v2.5.0
k8s.io/kube-openapi v0.0.0-20210113233702-8566a335510f
k8s.io/legacy-cloud-providers v0.19.7
k8s.io/utils v0.0.0-20201110183641-67b214c5f920
sigs.k8s.io/apiserver-network-proxy/konnectivity-client v0.0.14
sigs.k8s.io/controller-runtime v0.8.0
sigs.k8s.io/controller-tools v0.3.0
sigs.k8s.io/structured-merge-diff v0.0.0-20190817042607-6149e4549fca
sigs.k8s.io/structured-merge-diff/v3 v3.0.0
sigs.k8s.io/structured-merge-diff/v4 v4.0.2
sigs.k8s.io/yaml v1.2.0
# v1.1.1
$ go list -m all|grep k8s.io
k8s.io/api v0.20.1
k8s.io/apiextensions-apiserver v0.20.1
k8s.io/apimachinery v0.20.1
k8s.io/apiserver v0.20.1
k8s.io/client-go v0.20.1
k8s.io/cloud-provider v0.18.8
k8s.io/code-generator v0.20.1
k8s.io/component-base v0.20.1
k8s.io/csi-translation-lib v0.18.8
k8s.io/gengo v0.0.0-20201113003025-83324d819ded
k8s.io/klog v1.0.0
k8s.io/klog/v2 v2.4.0
k8s.io/kube-openapi v0.0.0-20201113171705-d219536bb9fd
k8s.io/legacy-cloud-providers v0.18.8
k8s.io/utils v0.0.0-20201110183641-67b214c5f920
sigs.k8s.io/apiserver-network-proxy/konnectivity-client v0.0.14
sigs.k8s.io/controller-runtime v0.8.0
sigs.k8s.io/controller-tools v0.3.0
sigs.k8s.io/structured-merge-diff v0.0.0-20190817042607-6149e4549fca
sigs.k8s.io/structured-merge-diff/v3 v3.0.0
sigs.k8s.io/structured-merge-diff/v4 v4.0.2
sigs.k8s.io/yaml v1.2.0

I've confirmed there were negligible differences in the Kube dependencies between v1.1.1 and v1.2.0.

@negz
Copy link
Member Author

negz commented May 4, 2021

It's also worth noting that the actual error is really weird

no kind "DeleteOptions" is registered for version "admissionregistration.k8s.io/v1" in scheme "k8s.io/apiextensions-apiserver/pkg/apiserver/apiserver.go:52"

I'm pretty sure DeleteOptions is not something that you'd expect to be registered with a scheme, and never existed as a type in the admissionregistration.k8s.io API group.

This reads almost like some function like Delete(ctx, object, deleteOptions) has its arguments mixed up, but I can't imagine that DeleteOptions exists as a valid runtime.Object anywhere? 🤔

@negz
Copy link
Member Author

negz commented May 4, 2021

It seems like it was not the crossplane-runtime bump. I've tested the commit that made the bump and everything works fine.

@negz
Copy link
Member Author

negz commented May 4, 2021

I went through a bit of a manual git bisect and I believe the issue was introduced in #2160. I can reproduce it after this PR was merged, but not before.

@negz
Copy link
Member Author

negz commented May 4, 2021

The above PR switches us over from adding Crossplane specific types to the default controller-manager scheme (which appears to be https://github.com/kubernetes/client-go/blob/master/kubernetes/scheme/register.go#L72) to the (recommended) approach of creating our own scheme and adding what we need. Notably admissionregistrationv1 is added to the default controller-runtime scheme, but not our new one.

Edit: Nope, I'm wrong. We're adding the same scheme controller-runtime seems to use as its default to our scheme.

@negz
Copy link
Member Author

negz commented May 4, 2021

Well it's definitely something to do with how we're using the scheme. If I make this following changes to 4e8f359 it all works as expected:

--- a/cmd/crossplane/core/core.go
+++ b/cmd/crossplane/core/core.go
@@ -22,11 +22,14 @@ import (
        "github.com/pkg/errors"
        "github.com/spf13/afero"
        "gopkg.in/alecthomas/kingpin.v2"
+       extv1 "k8s.io/apiextensions-apiserver/pkg/apis/apiextensions/v1"
+       extv1beta1 "k8s.io/apiextensions-apiserver/pkg/apis/apiextensions/v1beta1"
        "k8s.io/apimachinery/pkg/runtime"
        ctrl "sigs.k8s.io/controller-runtime"
 
        "github.com/crossplane/crossplane-runtime/pkg/logging"
 
+       "github.com/crossplane/crossplane/apis"
        "github.com/crossplane/crossplane/internal/controller/apiextensions"
        "github.com/crossplane/crossplane/internal/controller/pkg"
        "github.com/crossplane/crossplane/internal/xpkg"
@@ -57,7 +60,7 @@ func FromKingpin(cmd *kingpin.CmdClause) (*Command, *InitCommand) {
 }
 
 // Run core Crossplane controllers.
-func (c *Command) Run(s *runtime.Scheme, log logging.Logger) error {
+func (c *Command) Run(_ *runtime.Scheme, log logging.Logger) error {
        cfg, err := ctrl.GetConfig()
        if err != nil {
                return errors.Wrap(err, "Cannot get config")
@@ -65,7 +68,7 @@ func (c *Command) Run(s *runtime.Scheme, log logging.Logger) error {
        log.Debug("Starting", "sync-period", c.Sync.String())
 
        mgr, err := ctrl.NewManager(cfg, ctrl.Options{
-               Scheme:           s,
+               // Scheme:           s,
                LeaderElection:   c.LeaderElection,
                LeaderElectionID: "crossplane-leader-election-core",
                SyncPeriod:       &c.Sync,
@@ -74,6 +77,10 @@ func (c *Command) Run(s *runtime.Scheme, log logging.Logger) error {
                return errors.Wrap(err, "Cannot create manager")
        }
 
+       kingpin.FatalIfError(extv1.AddToScheme(mgr.GetScheme()), "cannot add client-go extensions v1 scheme")
+       kingpin.FatalIfError(extv1beta1.AddToScheme(mgr.GetScheme()), "cannot add client-go extensions v1beta1 scheme")
+       kingpin.FatalIfError(apis.AddToScheme(mgr.GetScheme()), "cannot add crossplane scheme")
+
        if err := apiextensions.Setup(mgr, log); err != nil {
                return errors.Wrap(err, "Cannot setup API extension controllers")
        }

@negz
Copy link
Member Author

negz commented May 4, 2021

I believe somehow, somewhere, we're missing a call to metav1.AddToGroupVersion, which would register DeleteOptions with the scheme per https://github.com/kubernetes/apimachinery/blob/v0.20.1/pkg/apis/meta/v1/register.go#L66.

That said, it's not clear to me how the apiserver-apiextensions scheme is coming into play here at all.

negz added a commit to negz/crossplane that referenced this issue May 4, 2021
This appears to be required to avoid issues like
crossplane#2284

Signed-off-by: Nic Cope <[email protected]>
@negz negz closed this as completed in #2285 May 4, 2021
negz added a commit to negz/crossplane that referenced this issue May 4, 2021
This appears to be required to avoid issues like
crossplane#2284

Signed-off-by: Nic Cope <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging a pull request may close this issue.

1 participant