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

wait on startup to get permissions #1858

Merged
merged 1 commit into from
Nov 17, 2022
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
31 changes: 26 additions & 5 deletions pkg/crd/crd.go
Original file line number Diff line number Diff line change
Expand Up @@ -31,6 +31,10 @@ import (
)

const (
// Sleep interval to check the CRD is present.
checkCRDPresentInterval = time.Second
// Timeout to check the CRD is present.
checkCRDPresentTimeout = 60 * time.Second
// Sleep interval to check the Established condition of CRD.
checkCRDEstablishedInterval = time.Second
// Timeout for checking the Established condition of CRD.
Expand Down Expand Up @@ -78,16 +82,33 @@ func (h *CRDHandler) EnsureCRD(meta *CRDMeta, namespacedScoped bool) (*apiextens
}

func (h *CRDHandler) createOrUpdateCRD(meta *CRDMeta, namespacedScoped bool) (*apiextensionsv1.CustomResourceDefinition, error) {
updateCRD := false
crd := crd(meta, namespacedScoped)
existingCRD, err := h.client.ApiextensionsV1().CustomResourceDefinitions().Get(context.TODO(), crd.Name, metav1.GetOptions{})
if err != nil && !apierrors.IsNotFound(err) {
return nil, fmt.Errorf("failed to verify the existence of %v CRD: %v", meta.kind, err)
if err := wait.PollImmediate(checkCRDPresentInterval, checkCRDPresentTimeout, func() (bool, error) {
existingCRD, err := h.client.ApiextensionsV1().CustomResourceDefinitions().Get(context.TODO(), crd.Name, metav1.GetOptions{})
// Retry until the RBAC permissions are propagated to be able to read the CRD
if apierrors.IsForbidden(err) {
return false, nil
}
// CRD doesn't exist, create it
if apierrors.IsNotFound(err) {
return true, nil
}
// Fail on any other error
if err != nil {
return false, fmt.Errorf("failed to verify the existence of %v CRD: %v", meta.kind, err)
}
// CRD exists, get current resource version and update it
updateCRD = true
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In a followup we should check if we even need to ensure. The case I am thinking is say we graduate an API, but had to rollback. We will fail here because in the rollback we try ensure a version without the graduated API.

crd.ResourceVersion = existingCRD.ResourceVersion
return true, nil
}); err != nil {
return nil, fmt.Errorf("timed out waiting to Get %v CRD: %v", meta.kind, err)
}

// Update CRD if already present.
if err == nil {
if updateCRD {
klog.V(0).Infof("Updating existing %v CRD...", meta.kind)
crd.ResourceVersion = existingCRD.ResourceVersion
return h.client.ApiextensionsV1().CustomResourceDefinitions().Update(context.TODO(), crd, metav1.UpdateOptions{})
}

Expand Down
70 changes: 66 additions & 4 deletions pkg/crd/crd_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -18,13 +18,17 @@ package crd

import (
"context"
"fmt"
"testing"

"github.com/google/go-cmp/cmp"
apiextensionsv1 "k8s.io/apiextensions-apiserver/pkg/apis/apiextensions/v1"
crdclient "k8s.io/apiextensions-apiserver/pkg/client/clientset/clientset"
crdclientfake "k8s.io/apiextensions-apiserver/pkg/client/clientset/clientset/fake"
apierrors "k8s.io/apimachinery/pkg/api/errors"
metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
"k8s.io/apimachinery/pkg/runtime"
k8stesting "k8s.io/client-go/testing"
"k8s.io/kube-openapi/pkg/common"
)

Expand All @@ -48,8 +52,9 @@ func testGetOpenAPIDefinitions(common.ReferenceCallback) map[string]common.OpenA

func TestCreateOrUpdateCRD(t *testing.T) {
testCases := []struct {
desc string
initFunc func(clientset crdclient.Interface, namespaceScoped bool) error
desc string
initFunc func(clientset crdclient.Interface, namespaceScoped bool) error
retryOnForbidden bool
}{
{
desc: "Create CRD when not exist",
Expand Down Expand Up @@ -77,20 +82,62 @@ func TestCreateOrUpdateCRD(t *testing.T) {
return nil
},
},
{
desc: "Create CRD when not exist after receiving a forbidden error the first time",
retryOnForbidden: true,
},
{
desc: "Update CRD when exist with wrongname after receiving a forbidden error the first time",
initFunc: func(clientset crdclient.Interface, namespaceScoped bool) error {
crd := crd(crdMeta, namespaceScoped)
crd.Spec.Names.Kind = "wrongname"
crd.Spec.Names.ListKind = "wrongnameList"
if _, err := clientset.ApiextensionsV1().CustomResourceDefinitions().Create(context.TODO(), crd, metav1.CreateOptions{}); err != nil {
return err
}
return nil
},
retryOnForbidden: true,
},
{
desc: "Update CRD when pruning is not enabled after receiving a forbidden error the first time",
initFunc: func(clientset crdclient.Interface, namespaceScoped bool) error {
crd := crd(crdMeta, namespaceScoped)
crd.Spec.PreserveUnknownFields = trueValue
if _, err := clientset.ApiextensionsV1().CustomResourceDefinitions().Create(context.TODO(), crd, metav1.CreateOptions{}); err != nil {
return err
}
return nil
},
retryOnForbidden: true,
},
}

for _, namespacedScoped := range []bool{true, false} {
expectedCRD := crd(crdMeta, namespacedScoped)
for _, tc := range testCases {
t.Run(tc.desc, func(t *testing.T) {
t.Run(tc.desc+fmt.Sprintf(" namespaced scoped %v", namespacedScoped), func(t *testing.T) {
fakeCRDClient := crdclientfake.NewSimpleClientset()
fakeCRDHandler := NewCRDHandler(fakeCRDClient)
if tc.initFunc != nil {
if err := tc.initFunc(fakeCRDClient, namespacedScoped); err != nil {
t.Errorf("Unexpected error in initFunc(): %v", err)
}
}

// test createOrUpdateCRD retries on http forbidden errors
numRetries := 3
if tc.retryOnForbidden {
counter := 0
fakeCRDClient.PrependReactor("get", "customresourcedefinitions", k8stesting.ReactionFunc(func(a k8stesting.Action) (bool, runtime.Object, error) {
counter++
if counter < numRetries {
return true, &apiextensionsv1.CustomResourceDefinition{}, apierrors.NewForbidden(a.GetResource().GroupResource(), a.(k8stesting.GetAction).GetName(), fmt.Errorf(`User "system:controller:glbc" cannot get resource`))
}

// delegate to the next reactor in the chain
return false, &apiextensionsv1.CustomResourceDefinition{}, nil
}))
}
crd, err := fakeCRDHandler.createOrUpdateCRD(crdMeta, namespacedScoped)
if err != nil {
t.Errorf("Unexpected error in createOrUpdateCRD(): %v", err)
Expand Down Expand Up @@ -123,6 +170,21 @@ func TestCreateOrUpdateCRD(t *testing.T) {
t.Errorf("CRD should be cluster scoped but found %s", crd.Spec.Scope)
}

getActions := 0
for _, a := range fakeCRDClient.Actions() {
if a.GetVerb() == "get" && a.GetResource().Resource == "customresourcedefinitions" {
getActions++
}
}
expectedGetActions := 1
if tc.retryOnForbidden {
expectedGetActions = numRetries
}

if getActions != expectedGetActions {
t.Errorf("Unexpected number of Get requests, got %d expected %d", getActions, expectedGetActions)
}

// Nuke CRD status before comparing.
crd.Status = apiextensionsv1.CustomResourceDefinitionStatus{}
if diff := cmp.Diff(expectedCRD, crd); diff != "" {
Expand Down