Skip to content

Commit

Permalink
Pass IsOpenshiftSupported to Webhook instead of global var
Browse files Browse the repository at this point in the history
Global variable should be avoided and encapsulating the value in a better way was needed by using an initalizaion function, NewOpenshiftValidator, and modify SetupWebhookWithManager input
  • Loading branch information
razo7 committed Jan 21, 2024
1 parent 138830d commit 472dae3
Show file tree
Hide file tree
Showing 5 changed files with 48 additions and 28 deletions.
12 changes: 6 additions & 6 deletions api/v1beta1/nodemaintenance_webhook.go
Original file line number Diff line number Diff line change
Expand Up @@ -32,8 +32,6 @@ import (
logf "sigs.k8s.io/controller-runtime/pkg/log"
"sigs.k8s.io/controller-runtime/pkg/webhook"
"sigs.k8s.io/controller-runtime/pkg/webhook/admission"

"github.com/medik8s/node-maintenance-operator/pkg/utils"
)

const (
Expand All @@ -57,15 +55,17 @@ var nodemaintenancelog = logf.Log.WithName("nodemaintenance-resource")
// NodeMaintenanceValidator validates NodeMaintenance resources. Needed because we need a client for validation
// +k8s:deepcopy-gen=false
type NodeMaintenanceValidator struct {
client client.Client
client client.Client
isOpenShift bool
}

var validator *NodeMaintenanceValidator

func (r *NodeMaintenance) SetupWebhookWithManager(mgr ctrl.Manager) error {
func (r *NodeMaintenance) SetupWebhookWithManager(isOpenShift bool, mgr ctrl.Manager) error {
// init the validator!
validator = &NodeMaintenanceValidator{
client: mgr.GetClient(),
client: mgr.GetClient(),
isOpenShift: isOpenShift,
}

return ctrl.NewWebhookManagedBy(mgr).
Expand Down Expand Up @@ -165,7 +165,7 @@ func (v *NodeMaintenanceValidator) validateNoNodeMaintenanceExists(nodeName stri
}

func (v *NodeMaintenanceValidator) validateControlPlaneQuorum(nodeName string) error {
if !utils.IsOpenshiftSupported {
if !v.isOpenShift {
// etcd quorum PDB is only installed in OpenShift
nodemaintenancelog.Info("Cluster does not have etcd quorum PDB, thus we can't asses control-plane quorum violation")
return nil
Expand Down
7 changes: 0 additions & 7 deletions api/v1beta1/nodemaintenance_webhook_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -11,8 +11,6 @@ import (
policyv1 "k8s.io/api/policy/v1"
metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
"sigs.k8s.io/controller-runtime/pkg/client"

"github.com/medik8s/node-maintenance-operator/pkg/utils"
)

var _ = Describe("NodeMaintenance Validation", func() {
Expand Down Expand Up @@ -93,10 +91,6 @@ var _ = Describe("NodeMaintenance Validation", func() {
node = getTestNode(existingNodeName, true)
err := k8sClient.Create(context.Background(), node)
Expect(err).ToNot(HaveOccurred())
// mock IsOpenshiftSupported value and set it true for the quorum violation check
orgValue := utils.IsOpenshiftSupported
utils.IsOpenshiftSupported = true
DeferCleanup(func() { utils.IsOpenshiftSupported = orgValue })
})

AfterEach(func() {
Expand Down Expand Up @@ -160,7 +154,6 @@ var _ = Describe("NodeMaintenance Validation", func() {
nm := getTestNMO(existingNodeName)
Expect(nm.ValidateCreate()).Error().NotTo(HaveOccurred())
})

})

Context("without etcd quorum guard PDB", func() {
Expand Down
20 changes: 14 additions & 6 deletions api/v1beta1/webhook_suite_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -42,16 +42,20 @@ import (
"sigs.k8s.io/controller-runtime/pkg/log/zap"
metricsServer "sigs.k8s.io/controller-runtime/pkg/metrics/server"
"sigs.k8s.io/controller-runtime/pkg/webhook"

"github.com/medik8s/node-maintenance-operator/pkg/utils"
)

// These tests use Ginkgo (BDD-style Go testing framework). Refer to
// http://onsi.github.io/ginkgo/ to learn more about Ginkgo.

var cfg *rest.Config
var k8sClient client.Client
var testEnv *envtest.Environment
var ctx context.Context
var cancel context.CancelFunc
var (
cfg *rest.Config
k8sClient client.Client
testEnv *envtest.Environment
ctx context.Context
cancel context.CancelFunc
)

func TestAPIs(t *testing.T) {
RegisterFailHandler(Fail)
Expand Down Expand Up @@ -97,6 +101,10 @@ var _ = BeforeSuite(func() {
Expect(err).NotTo(HaveOccurred())
Expect(k8sClient).NotTo(BeNil())

openshiftCheck, err := utils.NewOpenshiftValidator(cfg)
Expect(err).NotTo(HaveOccurred(), "failed to check if we run on Openshift")
Expect(openshiftCheck.IsOpenshiftSupported()).To(BeFalse())

// start webhook server using Manager
webhookInstallOptions := &testEnv.WebhookInstallOptions
mgr, err := ctrl.NewManager(cfg, ctrl.Options{
Expand All @@ -110,7 +118,7 @@ var _ = BeforeSuite(func() {
})
Expect(err).NotTo(HaveOccurred())

err = (&NodeMaintenance{}).SetupWebhookWithManager(mgr)
err = (&NodeMaintenance{}).SetupWebhookWithManager(true, mgr)
Expect(err).NotTo(HaveOccurred())

//+kubebuilder:scaffold:webhook
Expand Down
10 changes: 7 additions & 3 deletions main.go
Original file line number Diff line number Diff line change
Expand Up @@ -112,11 +112,15 @@ func main() {
os.Exit(1)
}

if err := utils.ValidateIsOpenshift(mgr.GetConfig()); err != nil {
openshiftCheck,err := utils.NewOpenshiftValidator(mgr.GetConfig())
if err != nil {
setupLog.Error(err, "failed to check if we run on Openshift")
os.Exit(1)
}
setupLog.Info("NMO was installed on Openshift cluster")
isOpenShift := openshiftCheck.IsOpenshiftSupported()
if isOpenShift{
setupLog.Info("NMO was installed on Openshift cluster")
}


if err = (&controllers.NodeMaintenanceReconciler{
Expand All @@ -127,7 +131,7 @@ func main() {
setupLog.Error(err, "unable to create controller", "controller", "NodeMaintenance")
os.Exit(1)
}
if err = (&nodemaintenancev1beta1.NodeMaintenance{}).SetupWebhookWithManager(mgr); err != nil {
if err = (&nodemaintenancev1beta1.NodeMaintenance{}).SetupWebhookWithManager(isOpenShift, mgr); err != nil {
setupLog.Error(err, "unable to create webhook", "webhook", "NodeMaintenance")
os.Exit(1)
}
Expand Down
27 changes: 21 additions & 6 deletions pkg/utils/validation.go
Original file line number Diff line number Diff line change
Expand Up @@ -6,11 +6,26 @@ import (
"k8s.io/client-go/rest"
)

// IsOpenshiftSupported will be set to true in case the operator was installed on OpenShift cluster
var IsOpenshiftSupported bool
type OpenshiftValidator struct {
// isOpenshiftSupported will be set to true in case the operator was installed on OpenShift cluster
isOpenshiftSupported bool
}

// ValidateIsOpenshift returns true if the cluster has the openshift config group
func ValidateIsOpenshift(config *rest.Config) error {
// NewOpenshiftValidator initialization function for OpenshiftValidator
func NewOpenshiftValidator(config *rest.Config) (*OpenshiftValidator, error) {
v := &OpenshiftValidator{}
if err := v.validateIsOpenshift(config); err != nil {
return nil, err
}
return v, nil
}

// IsOpenshiftSupported returns whether the cluster supports OpenShift
func (v *OpenshiftValidator) IsOpenshiftSupported() bool {
return v.isOpenshiftSupported
}
// validateIsOpenshift returns true if the cluster has the openshift config group
func (v *OpenshiftValidator) validateIsOpenshift(config *rest.Config) error {
dc, err := discovery.NewDiscoveryClientForConfig(config)
if err != nil {
return err
Expand All @@ -19,12 +34,12 @@ func ValidateIsOpenshift(config *rest.Config) error {
if err != nil {
return err
}

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() {
IsOpenshiftSupported = true
v.isOpenshiftSupported = true
return nil
}
}
Expand Down

0 comments on commit 472dae3

Please sign in to comment.