ceph: implement controller-runtime for admission controllers by vbnrh · Pull Request #5761 · rook/rook

vbnrh · 2020-07-06T05:14:36Z

moves the admission-controller server to use controller-runtime library for better support, maintainibility of code.

Signed-off-by: Vineet Badrinath vbadrina@redhat.com

Description of your changes:
This is a follow-up PR for #5045
It moves the server logic to use controller-runtime library for webhooks.

This help in better support, maintenance and performance of the code.

travisn · 2020-07-06T17:44:01Z

pkg/daemon/admission/server.go

 	tlsCertFile       = "tls.crt"
 	tlsKeyFile        = "tls.key"
 	serverPort  int32 = 8079
+	port              = 8079


What is the difference between serverPort and port? Can't we just re-use serverPort?

Removed ! Moved the dir and port to cmd/ceph/admission.go

travisn · 2020-07-06T17:46:38Z

pkg/daemon/admission/server.go

 	logger.Infof("starting the webhook for backend %q", a.providerName)
-	certPath := filepath.Join(tlsDir, tlsCertFile)
-	keyPath := filepath.Join(tlsDir, tlsKeyFile)
-	keyPair, err := NewTlsKeypairReloader(certPath, keyPath)


Are tlsutil.go and controller.go used anymore? Can we remove them? If so, then perhaps this package is so simple we can delete the whole package and move the remaining code under the ceph provider. For example, similar to what was done for the nfs operator in pkg/operator/nfs/webhook.go.

This is a draft PR. There are still some issues i am working on. I'll let you know once the updates are done

I've removed the tlsutil.go and controller.go

@travisn the code from the package (server.go) is meant to be a generic server code that can work for all providers. I think it would make sense to move it under pkg/operator/admission/server.go. May add another file later in this package for the unit tests. WDYT ?

The generic code is so short now that I wonder if it makes sense to have any common code. The controller runtime packages already have the common code.

Ok, i've moved the server code under pkg/operator/ceph/server.go

travisn · 2020-07-07T18:50:15Z

pkg/daemon/admission/server.go

 	logger.Infof("starting the webhook for backend %q", a.providerName)
-	certPath := filepath.Join(tlsDir, tlsCertFile)
-	keyPath := filepath.Join(tlsDir, tlsKeyFile)
-	keyPair, err := NewTlsKeypairReloader(certPath, keyPath)


The generic code is so short now that I wonder if it makes sense to have any common code. The controller runtime packages already have the common code.

travisn · 2020-07-07T18:52:13Z

cmd/rook/ceph/admission.go

+	if err != nil {
+		logger.Errorf("failed to add to scheme %v. ", err)
+	}
+	a := admission.New("ceph", port, certDir, &cephv1.CephCluster{}, scheme)


If we want to implement the admission webhook for multiple CRDs, we would need to start them all here (in separate goroutines, right? I guess they would just each listen on a separate port.

Though i havent tested it yet but i think that may not be the right approach as adding new ports means adding more services + validatingwebhookconfigs+ adding each port on the deployment for each of the CRDs to expose the port.

I have updated the code to add CephBlockPool validation. I've tried to make the code as generic and easy to change to add more CRDs. Here are the steps i did for cephblockpool which can be used for other ceph CRDs.

Implement webhook.Validator interface in pkg/apis/ceph.rook.io/v1/webhook.go for the CRD which needs to be validated

Add a ValidatingWebhookConfiguration (copy-paste work) in webhook-config.yaml changing the relevant fields ( name, path, resources) and make sure to note down the path given here.

In cmd/ceph/admission.go, Add the above path and map it to the given CRD in pathResourceMap.

Done ! Test it out.

Similiar approach can be followed for other providers also.

travisn · 2020-07-08T21:40:34Z

cmd/rook/ceph/admission.go

 	}
+	scheme          = runtime.NewScheme()
+	pathResourceMap = map[string]webhook.Validator{
+		"/validate-cephclusters":   &cephv1.CephCluster{},


Let's define this pathResourceMap inside the pkg/operator/ceph/server.go file. I don't see a reason to have this in the cmd package.

Yes, i'll move it here as it is part of ceph package. We may need to move it out once other providers implement their validations.

travisn · 2020-07-08T21:29:42Z

cmd/rook/ceph/admission.go

-	a := admission.New(context, "ceph", operator.ValidateCephResource)
+	err := cephv1.AddToScheme(scheme)
+	if err != nil {
+		logger.Errorf("failed to add to scheme %v. ", err)


If there is a failure we need to exit since the admission controller would fail to start later anyway. See other places where we call

rook.TerminateFatal(err)

travisn · 2020-07-08T21:31:20Z

pkg/apis/ceph.rook.io/v1/webhook.go

@@ -0,0 +1,89 @@
+package v1


How about separate source files for different CRDs? For example, validate_cluster.go and validate_pool.go

travisn · 2020-07-08T21:32:42Z

pkg/apis/ceph.rook.io/v1/webhook.go

+// will be registered for the validating webhook.
+var _ webhook.Validator = &CephCluster{}
+
+func (in *CephCluster) ValidateCreate() error {


Instead of in, it's more common to use the first letter of the type.

Suggested change

func (in *CephCluster) ValidateCreate() error {

func (c *CephCluster) ValidateCreate() error {

travisn · 2020-07-08T21:32:57Z

pkg/apis/ceph.rook.io/v1/webhook.go

+
+var _ webhook.Validator = &CephBlockPool{}
+
+func (in *CephBlockPool) ValidateCreate() error {


p for pool

Suggested change

func (in *CephBlockPool) ValidateCreate() error {

func (p *CephBlockPool) ValidateCreate() error {

travisn · 2020-07-08T21:36:31Z

pkg/apis/ceph.rook.io/v1/webhook.go

+		if in.Spec.Replicated.Size > 0 || in.Spec.Replicated.TargetSizeRatio > 0 {
+			return errors.New("invalid create: both erasurecoded and replicated fields cannot be set at the same time")
+		}
+	}


A couple other checks:

If either codingChunks or dataChunks are set, then dataChunks must be at least 2 and codingChunks must be at least 1.

If all of these values are 0, it's an error. You must set either replicated or erasure coded settings.

travisn · 2020-07-08T21:46:42Z

pkg/operator/ceph/server.go

+}
+
+// StartServer will start the server
+func (a *AdmissionController) StartServer() {


StartServer() should return an err so that the cmd package can call TerminateFatal() if there is some issue starting the webhook.

Suggested change

func (a *AdmissionController) StartServer() {

func (a *AdmissionController) StartServer() error {

travisn · 2020-07-08T21:46:57Z

pkg/operator/ceph/server.go

+	}
+	mgr, err := ctrl.NewManager(ctrl.GetConfigOrDie(), opts)
+	if err != nil {
+		logger.Errorf("failed to create manager %v. ", err)


return the err

travisn · 2020-07-08T21:47:01Z

pkg/operator/ceph/server.go

+	logger.Info("starting webhook server")
+	err = mgr.Start(ctrl.SetupSignalHandler())
+	if err != nil {
+		logger.Errorf("failed to start server %v. ", err)


return the err

travisn · 2020-07-08T21:56:55Z

tests/scripts/webhook-config.yaml

+      - apiGroups:   ["ceph.rook.io"]
+        apiVersions: ["v1"]
+        operations:  ["CREATE","UPDATE","DELETE"]
+        resources:   ["cephblockpools"]


Defining a separate ValidatingWebhookConfiguration for every CRD is really painful. Can you point to other examples of where this is done in other projects? I would really like to confirm if we can have one definition and watch all CRDs in the apiGroup ceph.rook.io

I've updated the code. We can now make do with just one ValidatingWebhookConfiguration but we have to add a webhook in the array for each CRD and make sure that the path is written with the same template

Looks good since it is in the same webhook config and we can use a single endpoint.

travisn · 2020-07-09T16:23:06Z

tests/scripts/webhook-config.yaml

+      - apiGroups:   ["ceph.rook.io"]
+        apiVersions: ["v1"]
+        operations:  ["CREATE","UPDATE","DELETE"]
+        resources:   ["cephblockpools"]


Looks good since it is in the same webhook config and we can use a single endpoint.

travisn · 2020-07-09T16:24:46Z

cmd/rook/ceph/admission.go

+	if err != nil {
+		rook.TerminateFatal(err)
+	}
+	a := operator.NewAdmissionController("ceph", port, certDir, scheme)


Do we even need to pass any of these parameters? What if we just define them directly inside StartServer()? It looks like they aren't configurable, so we don't need to expose them here.

Moved to server.go

travisn · 2020-07-09T16:25:22Z

pkg/operator/ceph/server.go

+func (a *AdmissionController) StartServer() error {
+	logger.Infof("starting the webhook for backend %q", a.providerName)
+
+	opts := ctrl.Options{


For example, related to my other comment, can we just initialize the scheme, port, and certDir here?

I've moved the inits to server.go

travisn · 2020-07-09T16:25:39Z

pkg/operator/ceph/server.go

+)
+
+type AdmissionController struct {
+	providerName string


This code is in the ceph namespace now, so no need to make it generic.

travisn · 2020-07-09T16:49:45Z

cmd/rook/ceph/admission.go

-
-	a := admission.New(context, "ceph", operator.ValidateCephResource)
-	a.StartServer()
+	err := operator.StartServer()


Since we don't have an admission controller struct anymore, let's rename the method.

Suggested change

err := operator.StartServer()

err := operator.StartAdmissionController()

travisn · 2020-07-09T17:28:52Z

pkg/apis/ceph.rook.io/v1/validate_pools.go

+
+func ValidatePoolSpecs(ps PoolSpec) error {
+	// Checks if either ErasureCoded or Replicated fields are set
+	if ps.ErasureCoded.CodingChunks <= 0 && ps.ErasureCoded.DataChunks <= 0 && ps.Replicated.TargetSizeRatio <= 0 && ps.Replicated.Size <= 0 {


We really should have unit tests for all of these validations. It should be fairly simple to create them, so would be great to have them with this PR.

Working on it

Updated with unit tests. please review

travisn · 2020-07-13T17:50:19Z