Skip to content

Commit

Permalink
Address comments
Browse files Browse the repository at this point in the history
  • Loading branch information
@sttts
sttts committed Mar 24, 2022
1 parent 43a5bef commit cf71cd0
Show file tree
Hide file tree
Showing 4 changed files with 33 additions and 41 deletions.
4 changes: 2 additions & 2 deletions pkg/reconciler/apibinding/apibinding_controller.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -50,7 +50,7 @@ const (
controllerName = "kcp-apibinding"
indexAPIBindingsByWorkspaceExport = "apiBindingsByWorkspaceExport"
indexAPIExportsByAPIResourceSchema = "apiExportsByAPIResourceSchema"
shadowWorkspaceName = "system:bound-crds"
ShadowWorkspaceName = "system:bound-crds"
)

// NewController returns a new controller for APIBindings.
Expand Down Expand Up @@ -99,7 +99,7 @@ func NewController(
return false
}

return crd.ClusterName == shadowWorkspaceName
return crd.ClusterName == ShadowWorkspaceName
},
Handler: cache.ResourceEventHandlerFuncs{
AddFunc: func(obj interface{}) { c.enqueueCRD(obj) },
Expand Down
21 changes: 8 additions & 13 deletions pkg/reconciler/apibinding/apibinding_reconcile.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -111,9 +111,6 @@ func (r *phaseReconciler) reconcile(_ context.Context, apiBinding *apisv1alpha1.
}

var exportedSchemas []*apisv1alpha1.APIResourceSchema
// name -> uid
exportedSchemaUIDs := map[string]string{}

for _, schemaName := range apiExport.Spec.LatestResourceSchemas {
apiResourceSchema, err := r.getAPIResourceSchema(apiExportClusterName, schemaName)
if err != nil && apierrors.IsNotFound(err) {
Expand All @@ -135,11 +132,10 @@ func (r *phaseReconciler) reconcile(_ context.Context, apiBinding *apisv1alpha1.
}

exportedSchemas = append(exportedSchemas, apiResourceSchema)
exportedSchemaUIDs[schemaName] = string(apiResourceSchema.UID)
}

if apiExportLatestResourceSchemasChanged(apiBinding, exportedSchemas) {
klog.Infof("APIBinding %s|%s needs rebinding because the APIExport's latestResourceSchemas has changed", apiBinding.ClusterName, apiBinding.Name)
klog.V(2).Infof("APIBinding %s|%s needs rebinding because the APIExport's latestResourceSchemas has changed", apiBinding.ClusterName, apiBinding.Name)

apiBinding.Status.Phase = apisv1alpha1.APIBindingPhaseBinding

Expand Down Expand Up @@ -284,16 +280,15 @@ func (r *workspaceAPIExportReferenceReconciler) reconcile(ctx context.Context, a
return reconcileStatusStop, nil // don't retry
}

existingCRD, err := r.getCRD(shadowWorkspaceName, crd.Name)
existingCRD, err := r.getCRD(ShadowWorkspaceName, crd.Name)
if err != nil && !apierrors.IsNotFound(err) {
return reconcileStatusStop, err // temporary error, retry
}

// If the deletedCRDTracker has this CRD in it, it means it was deleted. There is a chance that the cache
// still has the CRD in it, and existingCRD is populated instead of being nil. If that's the case, forcibly
// set existingCRD to nil so we know we need to (re)create the CRD.
// The crd was deleted and needs to be recreated. `existingCRD` might be non-nil if
// the lister is behind, so explicitly set to nil to ensure recreation.
if r.deletedCRDTracker.Has(crd.Name) {
klog.Infof("Bound CRD %s|%s was deleted - need to recreate", shadowWorkspaceName, crd.Name)
klog.Infof("Bound CRD %s|%s was deleted - need to recreate", ShadowWorkspaceName, crd.Name)
existingCRD = nil
}

Expand Down Expand Up @@ -448,7 +443,7 @@ func (c *controller) updateCRD(ctx context.Context, crd *apiextensionsv1.CustomR
func crdFromAPIResourceSchema(schema *apisv1alpha1.APIResourceSchema) (*apiextensionsv1.CustomResourceDefinition, error) {
crd := &apiextensionsv1.CustomResourceDefinition{
ObjectMeta: metav1.ObjectMeta{
ClusterName: shadowWorkspaceName,
ClusterName: ShadowWorkspaceName,
Name: string(schema.UID),
Annotations: map[string]string{
annotationBoundCRDKey: "",
Expand Down Expand Up @@ -500,14 +495,14 @@ func newLockedStringSet(s ...string) *lockedStringSet {

func (l *lockedStringSet) Add(s string) {
l.lock.Lock()
defer l.lock.Unlock()
l.s.Insert(s)
l.lock.Unlock()
}

func (l *lockedStringSet) Remove(s string) {
l.lock.Lock()
defer l.lock.Unlock()
l.s.Delete(s)
l.lock.Unlock()
}

func (l *lockedStringSet) Has(s string) bool {
Expand Down
17 changes: 8 additions & 9 deletions pkg/reconciler/apibinding/apibinding_reconcile_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -180,11 +180,10 @@ func TestPhaseReconciler(t *testing.T) {
}

status, err := r.reconcile(context.Background(), tc.apiBinding)

gotErr := err != nil
require.Equal(t, tc.wantError, gotErr)
if tc.wantError {
return
require.Error(t, err)
} else {
require.NoError(t, err)
}

require.Equal(t, status, tc.wantReconcileStatus)
Expand Down Expand Up @@ -299,7 +298,7 @@ func TestCRDFromAPIResourceSchema(t *testing.T) {
},
want: &apiextensionsv1.CustomResourceDefinition{
ObjectMeta: metav1.ObjectMeta{
ClusterName: shadowWorkspaceName,
ClusterName: ShadowWorkspaceName,
Name: "my-uuid",
Annotations: map[string]string{
annotationBoundCRDKey: "",
Expand Down Expand Up @@ -634,7 +633,7 @@ func TestWorkspaceAPIExportReferenceReconciler(t *testing.T) {
return tc.apiResourceSchemas[name], tc.getAPIResourceSchemaError
},
getCRD: func(clusterName, name string) (*apiextensionsv1.CustomResourceDefinition, error) {
require.Equal(t, shadowWorkspaceName, clusterName)
require.Equal(t, ShadowWorkspaceName, clusterName)
return tc.crds[name], tc.getCRDError
},
createCRD: func(ctx context.Context, crd *apiextensionsv1.CustomResourceDefinition) (*apiextensionsv1.CustomResourceDefinition, error) {
Expand All @@ -650,10 +649,10 @@ func TestWorkspaceAPIExportReferenceReconciler(t *testing.T) {

status, err := r.reconcile(context.Background(), tc.apiBinding)

gotErr := err != nil
require.Equal(t, tc.wantError, gotErr)
if tc.wantError {
return
require.Error(t, err)
} else {
require.NoError(t, err)
}

require.Equal(t, status, tc.wantReconcileStatus)
Expand Down
32 changes: 15 additions & 17 deletions pkg/server/apiextensions.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -45,6 +45,7 @@ import (
kcpclientset "github.com/kcp-dev/kcp/pkg/client/clientset/versioned"
apislisters "github.com/kcp-dev/kcp/pkg/client/listers/apis/v1alpha1"
tenancylisters "github.com/kcp-dev/kcp/pkg/client/listers/tenancy/v1alpha1"
"github.com/kcp-dev/kcp/pkg/reconciler/apibinding"
)

// SystemCRDLogicalCluster is the logical cluster we install system CRDs into for now. These are needed
Expand Down Expand Up @@ -144,7 +145,7 @@ type apiBindingAwareCRDLister struct {

var _ apiextensionslisters.CustomResourceDefinitionLister = (*apiBindingAwareCRDLister)(nil)

// List lists all CustomResourceDefinitions in the underlying store matching selector. This method does not
// List lists those CustomResourceDefinitions in the underlying store matching selector. This method does not
// support scoping to logical clusters or APIBindings.
func (c *apiBindingAwareCRDLister) List(selector labels.Selector) ([]*apiextensionsv1.CustomResourceDefinition, error) {
return c.crdLister.ListWithContext(context.Background(), selector)
Expand All @@ -158,7 +159,6 @@ func (c *apiBindingAwareCRDLister) ListWithContext(ctx context.Context, selector
return nil, err
}

var ret []*apiextensionsv1.CustomResourceDefinition
crdName := func(crd *apiextensionsv1.CustomResourceDefinition) string {
return crd.Spec.Names.Plural + "." + crd.Spec.Group
}
Expand All @@ -177,7 +177,7 @@ func (c *apiBindingAwareCRDLister) ListWithContext(ctx context.Context, selector
}

// Priority 1: add system CRDs. These take priority over CRDs from APIBindings and CRDs from the local workspace.
ret = kcpSystemCRDs
var ret = kcpSystemCRDs
for i := range kcpSystemCRDs {
seen.Insert(crdName(kcpSystemCRDs[i]))
}
Expand All @@ -187,6 +187,7 @@ func (c *apiBindingAwareCRDLister) ListWithContext(ctx context.Context, selector
return nil, err
}

// TODO(sttts): optimize this looping by using an informer index
for _, apiBinding := range apiBindings {
if apiBinding.ClusterName != cluster.Name {
continue
Expand All @@ -198,7 +199,7 @@ func (c *apiBindingAwareCRDLister) ListWithContext(ctx context.Context, selector
}

for _, boundResource := range apiBinding.Status.BoundResources {
crdKey := clusters.ToClusterAwareKey("system:bound-crds", boundResource.Schema.UID)
crdKey := clusters.ToClusterAwareKey(apibinding.ShadowWorkspaceName, boundResource.Schema.UID)
crd, err := c.crdLister.GetWithContext(ctx, crdKey)
if err != nil {
klog.Errorf("Error getting bound CRD %q: %v", crdKey, err)
Expand Down Expand Up @@ -293,8 +294,8 @@ func (c *apiBindingAwareCRDLister) GetWithContext(ctx context.Context, name stri
return nil, fmt.Errorf("error determining workspace name from cluster name %q: %w", cluster.Name, err)
}

// Priority 1: see in system CRDs
systemCRDKeys := c.systemCRDProvider.Keys(ctx, org, workspace)

systemCRDKeyName := clusters.ToClusterAwareKey(SystemCRDLogicalCluster, name)
if systemCRDKeys.Has(systemCRDKeyName) {
crd, err = c.crdLister.Get(systemCRDKeyName)
Expand All @@ -310,7 +311,7 @@ func (c *apiBindingAwareCRDLister) GetWithContext(ctx context.Context, name stri
return nil, apierrors.NewNotFound(schema.GroupResource{Group: apiextensionsv1.SchemeGroupVersion.Group, Resource: "customresourcedefinitions"}, name)
}

// Priority 1: see if it comes from any APIBindings
// Priority 2: see if it comes from any APIBindings
parts := strings.SplitN(name, ".", 2)
resource, group := parts[0], parts[1]

Expand All @@ -336,25 +337,22 @@ func (c *apiBindingAwareCRDLister) GetWithContext(ctx context.Context, name stri

for _, boundResource := range apiBinding.Status.BoundResources {
if boundResource.Group == group && boundResource.Resource == resource {
crdKey := clusters.ToClusterAwareKey("system:bound-crds", boundResource.Schema.UID)
crdKey := clusters.ToClusterAwareKey(apibinding.ShadowWorkspaceName, boundResource.Schema.UID)
crd, err = c.crdLister.Get(crdKey)
if err != nil && !apierrors.IsNotFound(err) {
if err != nil && apierrors.IsNotFound(err) {
// If we got here, it means there is supposed to be a CRD coming from an APIBinding, but
// the CRD doesn't exist for some reason.
return nil, apierrors.NewServiceUnavailable(fmt.Sprintf("%s is currently unavailable", name))
} else if err != nil {
// something went wrong w/the lister - could only happen if meta.Accessor() fails on an item in the store.
return nil, err
}

if crd != nil {
return crd, nil
}

// If we got here, it means there is supposed to be a CRD coming from an APIBinding, but
// the CRD doesn't exist for some reason.
return nil, apierrors.NewServiceUnavailable(fmt.Sprintf("%s is currently unavailable", name))
return crd, nil
}
}
}

// Priority 2: see if it exists in the current logical cluster
// Priority 3: see if it exists in the current logical cluster
crdKey := clusters.ToClusterAwareKey(cluster.Name, name)
crd, err = c.crdLister.Get(crdKey)
if err != nil && !apierrors.IsNotFound(err) {
Expand Down

0 comments on commit cf71cd0

Please sign in to comment.