Improve Capabilities object

Improve the new Capabilities struct to better encapsulate cluster
capabilities.

- add NewCapabilties function to return the filled Capabilities struct
- made both IsOnOpenshift and HasMachineAPI private under
  cluster.capabilities file
- Access to the respective values is only allowed via the
  Capabilities struct's fields

Signed-off-by: Carlo Lobrano <[email protected]>

Author: clobrano

controllers/cluster/capabilities.go

@@ -6,18 +6,56 @@ import (
 	"github.com/pkg/errors"
 
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
+	"k8s.io/apimachinery/pkg/runtime/schema"
+	"k8s.io/client-go/discovery"
 	"k8s.io/client-go/rest"
 
 	v1 "github.com/openshift/api/config/v1"
 	configv1 "github.com/openshift/client-go/config/clientset/versioned/typed/config/v1"
 )
 
 type Capabilities struct {
-	HasEtcdPDBQuorum, HasMachineAPI bool
+	IsOnOpenshift, HasMachineAPI bool
+}
+
+func NewCapabilities(config *rest.Config) (Capabilities, error) {
+	var err error
+	var onOpenshift, machineAPI bool
+
+	if onOpenshift, err = isOnOpenshift(config); err != nil {
+		return Capabilities{}, errors.Wrap(err, "failed to check if cluster is on OpenShift")
+	} else if onOpenshift {
+		if machineAPI, err = hasMachineAPICapability(config); err != nil {
+			return Capabilities{}, errors.Wrap(err, "failed to check machine API capability")
+		}
+	}
+
+	return Capabilities{
+		IsOnOpenshift: onOpenshift,
+		HasMachineAPI: machineAPI,
+	}, nil
+}
+
+// IsOnOpenshift returns true if the cluster has the openshift config group
+func isOnOpenshift(config *rest.Config) (bool, error) {
+	dc, err := discovery.NewDiscoveryClientForConfig(config)
+	if err != nil {
+		return false, err
+	}
+	apiGroups, err := dc.ServerGroups()
+	kind := schema.GroupVersionKind{Group: "config.openshift.io", Version: "v1", Kind: "ClusterVersion"}
+	for _, apiGroup := range apiGroups.Groups {
+		for _, supportedVersion := range apiGroup.Versions {
+			if supportedVersion.GroupVersion == kind.GroupVersion().String() {
+				return true, nil
+			}
+		}
+	}
+	return false, nil
 }
 
 // HasMachineAPICapability returns true if the cluster has the MachineAPI Enabled
-func HasMachineAPICapability(config *rest.Config) (bool, error) {
+func hasMachineAPICapability(config *rest.Config) (bool, error) {
 	configV1Client, err := configv1.NewForConfig(config)
 	if err != nil {
 		return false, errors.Wrapf(err, "failed to create cluster version client")

controllers/cluster/upgrade_checker.go

@@ -13,8 +13,6 @@ import (
 
 	v1 "github.com/openshift/api/config/v1"
 	clusterversion "github.com/openshift/client-go/config/clientset/versioned/typed/config/v1"
-
-	"github.com/medik8s/node-healthcheck-operator/controllers/utils"
 )
 
 var unsupportedUpgradeCheckerErr = errors.New(
@@ -66,12 +64,8 @@ func (n *noopClusterUpgradeStatusChecker) Check() (bool, error) {
 
 // NewClusterUpgradeStatusChecker will return some implementation of a checker or err in case it can't
 // reliably detect which implementation to use.
-func NewClusterUpgradeStatusChecker(mgr manager.Manager) (UpgradeChecker, error) {
-	openshift, err := utils.IsOnOpenshift(mgr.GetConfig())
-	if err != nil {
-		return nil, err
-	}
-	if !openshift {
+func NewClusterUpgradeStatusChecker(mgr manager.Manager, caps Capabilities) (UpgradeChecker, error) {
+	if !caps.IsOnOpenshift {
 		return &noopClusterUpgradeStatusChecker{}, nil
 	}
 	checker, err := newOpenshiftClusterUpgradeChecker(mgr)

controllers/console/plugin.go

@@ -29,7 +29,7 @@ import (
 
 	"github.com/openshift/api/console/v1alpha1"
 
-	"github.com/medik8s/node-healthcheck-operator/controllers/utils"
+	"github.com/medik8s/node-healthcheck-operator/controllers/cluster"
 )
 
 const (
@@ -50,9 +50,11 @@ const (
 func CreateOrUpdatePlugin(ctx context.Context, cl client.Client, config *rest.Config, namespace string, log logr.Logger) error {
 
 	// check if we are on OCP
-	if isOpenshift, err := utils.IsOnOpenshift(config); err != nil {
-		return errors.Wrap(err, "failed to check if we are on Openshift")
-	} else if !isOpenshift {
+	clusterCapabilities, err := cluster.NewCapabilities(config)
+	if err != nil {
+		return errors.Wrap(err, "failed to check cluster capabilities")
+	}
+	if !clusterCapabilities.IsOnOpenshift {
 		log.Info("we are not on Openshift, skipping console plugin activation")
 		return nil
 	}

controllers/machinehealthcheck_controller_test.go

@@ -33,6 +33,7 @@ import (
 
 	machinev1 "github.com/openshift/api/machine/v1beta1"
 
+	"github.com/medik8s/node-healthcheck-operator/controllers/cluster"
 	"github.com/medik8s/node-healthcheck-operator/controllers/featuregates"
 	"github.com/medik8s/node-healthcheck-operator/controllers/mhc"
 	"github.com/medik8s/node-healthcheck-operator/controllers/resources"
@@ -2386,7 +2387,8 @@ func newFakeReconcilerWithCustomRecorder(recorder record.EventRecorder, initObje
 		WithRuntimeObjects(initObjects...).
 		WithStatusSubresource(&machinev1.MachineHealthCheck{}).
 		Build()
-	mhcChecker, _ := mhc.NewMHCChecker(k8sManager, false, nil)
+	caps := cluster.Capabilities{IsOnOpenshift: false, HasMachineAPI: false}
+	mhcChecker, _ := mhc.NewMHCChecker(k8sManager, caps, nil)
 	return &MachineHealthCheckReconciler{
 		Client:                         fakeClient,
 		Recorder:                       recorder,

controllers/mhc/checker.go

@@ -11,6 +11,8 @@ import (
 	"sigs.k8s.io/controller-runtime/pkg/manager"
 
 	"github.com/openshift/api/machine/v1beta1"
+
+	"github.com/medik8s/node-healthcheck-operator/controllers/cluster"
 )
 
 // NodeConditionTerminating is the node condition type used by the termination handler MHC
@@ -25,9 +27,9 @@ type Checker interface {
 }
 
 // NewMHCChecker creates a new Checker
-func NewMHCChecker(mgr manager.Manager, hasMachineAPI bool, mhcEvents chan<- event.GenericEvent) (Checker, error) {
+func NewMHCChecker(mgr manager.Manager, caps cluster.Capabilities, mhcEvents chan<- event.GenericEvent) (Checker, error) {
 
-	if !hasMachineAPI {
+	if !caps.HasMachineAPI {
 		return DummyChecker{}, nil
 	}
 

controllers/nodehealthcheck_controller.go

@@ -683,7 +683,8 @@ func (r *NodeHealthCheckReconciler) isControlPlaneRemediationAllowed(ctx context
 	}
 
 	// no ongoing control plane remediation, check etcd quorum
-	if !r.Capabilities.HasEtcdPDBQuorum {
+	if !r.Capabilities.IsOnOpenshift {
+		// etcd quorum PDB is only installed in OpenShift
 		return true, nil
 	}
 	var allowed bool

controllers/suite_test.go

@@ -169,7 +169,9 @@ var _ = BeforeSuite(func() {
 		Upgrading: false,
 	}
 
-	mhcChecker, err := mhc.NewMHCChecker(k8sManager, false, nil)
+	caps := cluster.Capabilities{IsOnOpenshift: true, HasMachineAPI: true}
+
+	mhcChecker, err := mhc.NewMHCChecker(k8sManager, caps, nil)
 	Expect(err).NotTo(HaveOccurred())
 
 	os.Setenv("DEPLOYMENT_NAMESPACE", DeploymentNamespace)
@@ -197,7 +199,7 @@ var _ = BeforeSuite(func() {
 		ClusterUpgradeStatusChecker: upgradeChecker,
 		MHCChecker:                  mhcChecker,
 		MHCEvents:                   mhcEvents,
-		Capabilities:                cluster.Capabilities{HasEtcdPDBQuorum: true, HasMachineAPI: true},
+		Capabilities:                caps,
 	}).SetupWithManager(k8sManager)
 	Expect(err).NotTo(HaveOccurred())
 

controllers/utils/utils.go

@@ -11,9 +11,6 @@ import (
 
 	v1 "k8s.io/api/core/v1"
 	"k8s.io/apimachinery/pkg/apis/meta/v1/unstructured"
-	"k8s.io/apimachinery/pkg/runtime/schema"
-	"k8s.io/client-go/discovery"
-	"k8s.io/client-go/rest"
 	"k8s.io/client-go/tools/cache"
 	"sigs.k8s.io/controller-runtime/pkg/client"
 
@@ -45,24 +42,6 @@ func GetDeploymentNamespace() (string, error) {
 	return ns, nil
 }
 
-// IsOnOpenshift returns true if the cluster has the openshift config group
-func IsOnOpenshift(config *rest.Config) (bool, error) {
-	dc, err := discovery.NewDiscoveryClientForConfig(config)
-	if err != nil {
-		return false, err
-	}
-	apiGroups, err := dc.ServerGroups()
-	kind := schema.GroupVersionKind{Group: "config.openshift.io", Version: "v1", Kind: "ClusterVersion"}
-	for _, apiGroup := range apiGroups.Groups {
-		for _, supportedVersion := range apiGroup.Versions {
-			if supportedVersion.GroupVersion == kind.GroupVersion().String() {
-				return true, nil
-			}
-		}
-	}
-	return false, nil
-}
-
 // GetLogWithNHC return a logger with NHC namespace and name
 func GetLogWithNHC(log logr.Logger, nhc *v1alpha1.NodeHealthCheck) logr.Logger {
 	return log.WithValues("NodeHealthCheck name", nhc.Name)

main.go

@@ -52,7 +52,6 @@ import (
 	"github.com/medik8s/node-healthcheck-operator/controllers/featuregates"
 	"github.com/medik8s/node-healthcheck-operator/controllers/initializer"
 	"github.com/medik8s/node-healthcheck-operator/controllers/mhc"
-	"github.com/medik8s/node-healthcheck-operator/controllers/utils"
 	"github.com/medik8s/node-healthcheck-operator/metrics"
 	"github.com/medik8s/node-healthcheck-operator/version"
 )
@@ -131,34 +130,22 @@ func main() {
 		os.Exit(1)
 	}
 
-	upgradeChecker, err := cluster.NewClusterUpgradeStatusChecker(mgr)
+	caps, err := cluster.NewCapabilities(mgr.GetConfig())
 	if err != nil {
-		setupLog.Error(err, "unable initialize cluster upgrade checker")
+		setupLog.Error(err, "unable to determine cluster capabilities")
 		os.Exit(1)
 	}
 
-	onOpenshift, err := utils.IsOnOpenshift(mgr.GetConfig())
+	setupLog.Info("Cluster capabilities", "IsOnOpenshift", caps.IsOnOpenshift, "HasMachineAPI", caps.HasMachineAPI)
+
+	upgradeChecker, err := cluster.NewClusterUpgradeStatusChecker(mgr, caps)
 	if err != nil {
-		setupLog.Error(err, "failed to check if we run on Openshift")
+		setupLog.Error(err, "unable initialize cluster upgrade checker")
 		os.Exit(1)
 	}
 
-	var hasMachineAPI bool
-	if onOpenshift {
-		hasMachineAPI, err := cluster.HasMachineAPICapability(mgr.GetConfig())
-		if err != nil {
-			setupLog.Error(err, "failed to check if MachineAPI is enabled")
-			os.Exit(1)
-		}
-		if hasMachineAPI {
-			setupLog.Info("MachineAPI is enabled")
-		} else {
-			setupLog.Info("MachineAPI is not enabled")
-		}
-	}
-
 	mhcEvents := make(chan event.GenericEvent)
-	mhcChecker, err := mhc.NewMHCChecker(mgr, hasMachineAPI, mhcEvents)
+	mhcChecker, err := mhc.NewMHCChecker(mgr, caps, mhcEvents)
 	if err != nil {
 		setupLog.Error(err, "unable initialize MHC checker")
 		os.Exit(1)
@@ -168,26 +155,20 @@ func main() {
 		os.Exit(1)
 	}
 
-	clusterCapabilities := cluster.Capabilities{
-		// etcd quorum PDB is only installed in OpenShift
-		HasEtcdPDBQuorum: onOpenshift,
-		HasMachineAPI:    hasMachineAPI,
-	}
-
 	if err := (&controllers.NodeHealthCheckReconciler{
 		Client:                      mgr.GetClient(),
 		Log:                         ctrl.Log.WithName("controllers").WithName("NodeHealthCheck"),
 		Recorder:                    mgr.GetEventRecorderFor("NodeHealthCheck"),
 		ClusterUpgradeStatusChecker: upgradeChecker,
 		MHCChecker:                  mhcChecker,
-		Capabilities:                clusterCapabilities,
+		Capabilities:                caps,
 		MHCEvents:                   mhcEvents,
 	}).SetupWithManager(mgr); err != nil {
 		setupLog.Error(err, "unable to create controller", "controller", "NodeHealthCheck")
 		os.Exit(1)
 	}
 
-	if hasMachineAPI {
+	if caps.HasMachineAPI {
 		featureGateMHCControllerDisabledEvents := make(chan event.GenericEvent)
 		featureGateAccessor := featuregates.NewAccessor(mgr.GetConfig(), featureGateMHCControllerDisabledEvents)
 		if err = mgr.Add(featureGateAccessor); err != nil {