Skip to content
This repository has been archived by the owner on Jun 26, 2023. It is now read-only.

Create client for checking CRD deletion #1305

Merged
merged 1 commit into from
Dec 9, 2020
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
2 changes: 1 addition & 1 deletion incubator/hnc/cmd/manager/main.go
Original file line number Diff line number Diff line change
Expand Up @@ -192,7 +192,7 @@ func startControllers(mgr ctrl.Manager, certsCreated chan struct{}) {

// Create all reconciling controllers
setupLog.Info("Creating controllers", "maxReconciles", maxReconciles)
if err := reconcilers.Create(mgr, f, maxReconciles); err != nil {
if err := reconcilers.Create(mgr, f, maxReconciles, false); err != nil {
setupLog.Error(err, "cannot create controllers")
os.Exit(1)
}
Expand Down
2 changes: 1 addition & 1 deletion incubator/hnc/internal/reconcilers/anchor.go
Original file line number Diff line number Diff line change
Expand Up @@ -128,7 +128,7 @@ func (r *AnchorReconciler) onDeleting(ctx context.Context, log logr.Logger, inst
// We handle deletions differently depending on whether _one_ anchor is being deleted (i.e., the
// user wants to delete the namespace) or whether the Anchor CRD is being deleted, which usually
// means HNC is being uninstalled and we shouldn't delete _any_ namespaces.
deletingCRD, err := isDeletingCRD(ctx, r, api.Anchors)
deletingCRD, err := isDeletingCRD(ctx, api.Anchors)
if err != nil {
log.Error(err, "Couldn't determine if CRD is being deleted")
return false, err
Expand Down
2 changes: 1 addition & 1 deletion incubator/hnc/internal/reconcilers/hierarchy_config.go
Original file line number Diff line number Diff line change
Expand Up @@ -625,7 +625,7 @@ func (r *HierarchyConfigReconciler) getSingleton(ctx context.Context, nm string)
deletingCRD := false
if inst.CreationTimestamp.IsZero() || !inst.DeletionTimestamp.IsZero() {
var err error
deletingCRD, err = isDeletingCRD(ctx, r, api.HierarchyConfigurations)
deletingCRD, err = isDeletingCRD(ctx, api.HierarchyConfigurations)
if err != nil {
return nil, false, err
}
Expand Down
2 changes: 1 addition & 1 deletion incubator/hnc/internal/reconcilers/hnc_config.go
Original file line number Diff line number Diff line change
Expand Up @@ -235,7 +235,7 @@ func (r *ConfigReconciler) validateSingleton(inst *api.HNCConfiguration) {
func (r *ConfigReconciler) writeSingleton(ctx context.Context, inst *api.HNCConfiguration) error {
if inst.CreationTimestamp.IsZero() {
// No point creating it if the CRD's being deleted
if isDeleted, err := isDeletingCRD(ctx, r, api.HNCConfigSingletons); isDeleted || err != nil {
if isDeleted, err := isDeletingCRD(ctx, api.HNCConfigSingletons); isDeleted || err != nil {
r.Log.Info("CRD is being deleted (or CRD deletion status couldn't be determined); skip update")
return err
}
Expand Down
38 changes: 35 additions & 3 deletions incubator/hnc/internal/reconcilers/setup.go
Original file line number Diff line number Diff line change
Expand Up @@ -9,25 +9,57 @@ import (
"k8s.io/apimachinery/pkg/runtime"
"k8s.io/apimachinery/pkg/types"
ctrl "sigs.k8s.io/controller-runtime"
"sigs.k8s.io/controller-runtime/pkg/client"
"sigs.k8s.io/controller-runtime/pkg/client/config"
"sigs.k8s.io/controller-runtime/pkg/event"

api "sigs.k8s.io/multi-tenancy/incubator/hnc/api/v1alpha2"
"sigs.k8s.io/multi-tenancy/incubator/hnc/internal/forest"
)

// FakeDeleteCRDClient is a "fake" client used for testing only
type FakeDeleteCRDClient struct{}
GinnyJI marked this conversation as resolved.
Show resolved Hide resolved

// FakeDeleteCRDClient doesn't return any err on Get() because none of the reconciler test performs CRD deletion
func (f FakeDeleteCRDClient) Get(context.Context, types.NamespacedName, runtime.Object) error {
GinnyJI marked this conversation as resolved.
Show resolved Hide resolved
return nil
}

var crds = map[string]bool{
"hncconfigurations.hnc.x-k8s.io": true,
"subnamespaceanchors.hnc.x-k8s.io": true,
"hierarchyconfigurations.hnc.x-k8s.io": true,
}

// deleteCRDClientType could be either a real client or FakeDeleteCRDClient
type deleteCRDClientType interface {
Get(context.Context, types.NamespacedName, runtime.Object) error
}

// deleteCRDClient is an uncached client for checking CRD deletion
var deleteCRDClient deleteCRDClientType

// Create creates all reconcilers.
//
// This function is called both from main.go as well as from the integ tests.
func Create(mgr ctrl.Manager, f *forest.Forest, maxReconciles int) error {
func Create(mgr ctrl.Manager, f *forest.Forest, maxReconciles int, useFakeClient bool) error {
Copy link

Choose a reason for hiding this comment

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

Since the client is created 100% by variables/info available outside this function, wouldn't it make more sense to just pass the client into this function instead of using a special bool to create a fake client or not?

Copy link
Contributor

Choose a reason for hiding this comment

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

It's a good point, although in this context I'm not sure it's worth the added complexity. The interface and global variable would still have to be declared in this file, while the clients that implement it would now be in cmd/manager/main.go and internal/reconcilers/suite_test.go, so the logic would be fairly scattered. The way @GinnyJI has written it, pretty much everything you need to know is in the same place, and I think that simplicity is worthwhile.

If we start adding a bunch more options here, then I'd be open to refactoring it. wdyt?

Copy link

Choose a reason for hiding this comment

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

Yeah, that sounds good.

hcChan := make(chan event.GenericEvent)
anchorChan := make(chan event.GenericEvent)

// Create uncached client for CRD deletion check
if !useFakeClient {
var err error
deleteCRDClient, err = client.New(config.GetConfigOrDie(), client.Options{
Scheme: mgr.GetScheme(),
// I'm not sure if this mapper is needed - @ginnyji Dec2020
Mapper: mgr.GetRESTMapper(),
GinnyJI marked this conversation as resolved.
Show resolved Hide resolved
})
if err != nil {
return fmt.Errorf("cannot create deleteCRDClient: %s", err.Error())
}
} else {
deleteCRDClient = FakeDeleteCRDClient{}
}
// Create AnchorReconciler.
sar := &AnchorReconciler{
Client: mgr.GetClient(),
Expand Down Expand Up @@ -74,10 +106,10 @@ type crdClient interface {

// isDeletingCRD returns true if the specified HNC CRD is being or has been deleted. The argument
// expected is the CRD name minus the HNC suffix, e.g. "hierarchyconfigurations".
func isDeletingCRD(ctx context.Context, c crdClient, nm string) (bool, error) {
func isDeletingCRD(ctx context.Context, nm string) (bool, error) {
crd := &apiextensions.CustomResourceDefinition{}
nsn := types.NamespacedName{Name: nm + "." + api.MetaGroup}
if err := c.Get(ctx, nsn, crd); err != nil {
if err := deleteCRDClient.Get(ctx, nsn, crd); err != nil {
if apierrors.IsNotFound(err) {
return true, nil
}
Expand Down
3 changes: 2 additions & 1 deletion incubator/hnc/internal/reconcilers/suite_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -35,6 +35,7 @@ import (

// +kubebuilder:scaffold:imports

_ "k8s.io/client-go/plugin/pkg/client/auth"
api "sigs.k8s.io/multi-tenancy/incubator/hnc/api/v1alpha2"
"sigs.k8s.io/multi-tenancy/incubator/hnc/internal/forest"
"sigs.k8s.io/multi-tenancy/incubator/hnc/internal/reconcilers"
Expand Down Expand Up @@ -99,7 +100,7 @@ var _ = BeforeSuite(func(done Done) {
Scheme: scheme.Scheme,
})
Expect(err).ToNot(HaveOccurred())
err = reconcilers.Create(k8sManager, forest.NewForest(), 100)
err = reconcilers.Create(k8sManager, forest.NewForest(), 100, true)
Expect(err).ToNot(HaveOccurred())

k8sClient = k8sManager.GetClient()
Expand Down