Skip to content

ceph: Adding support for admission controllers#5045

Merged
travisn merged 1 commit intorook:masterfrom
vbnrh:rook-admission-controller
Jun 25, 2020
Merged

ceph: Adding support for admission controllers#5045
travisn merged 1 commit intorook:masterfrom
vbnrh:rook-admission-controller

Conversation

@vbnrh
Copy link
Contributor

@vbnrh vbnrh commented Mar 18, 2020

Description of your changes:
Adds various scripts and code changes to deploy admission controllers alongside rook.

What are admission controllers
Admission controllers help in validating the CRDs at native kubernetes level, accepting or rejecting an api request based on our custom criteria

For example, The PR here attempts to provide solution for one of the issues mentioned in #4819

Cannot change the dataDirHostPath after initial creation

Our webhooks have the logic defined in them to to continuously check if an api request is an update for an existing ceph cluster and validates if the dataDirHostPath has been changed or not.

If it has been changed from initial creation, it will reject the request and throw the below error

for: "cluster.yaml": admission webhook "webhook-server.rook-ceph.svc" denied the request: Invalid Update : DataDirHostPath change from /var/lib/rook to /var/rook is not allowed

More such validations can be applied for any kind of resource

How to test this PR

  1. Clone
  2. Create any cluster
  3. Run the following commands
kubectl create -f common.yaml
./config-admission-controllers.sh
  1. Now that the secrets have been deployed, we can deploy the operator and create a cluster using the following commands
kubectl create -f operator.yaml
kubectl create -f cluster.yaml
  1. The operator on detecting the secrets will deploy the admission controllers and start listening for all validations
  2. Make changes to cluster.yaml in spec.datadirhostpath
    kubectl apply -f cluster.yaml

On doing all the above, we could see the request being rejected.

Which issue is resolved by this Pull Request:
#2363
#4819

[test ceph]

@ghost
Copy link

ghost commented Mar 18, 2020

There were the following issues with this Pull Request

  • Commit: 68bb6b7
    • ✖ message may not be empty
    • ✖ type may not be empty

You may need to change the commit messages to comply with the repository contributing guidelines.


🤖 This comment was generated by commitlint[bot]. Please report issues here.

Happy coding!

@vbnrh vbnrh changed the title [WIP] Admission controllers on Rook [WIP] Rook: Admission controllers on Rook Mar 18, 2020
Copy link
Member

@umangachapagain umangachapagain left a comment

Choose a reason for hiding this comment

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

Can you provide context and explanation around this feature?
It is not obvious from the PR.

What is the scope of this PR?

Copy link
Member

@leseb leseb left a comment

Choose a reason for hiding this comment

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

@vbnrh Rook 1.3 is freezing soon next week so if you want to see this as part of 1.3 I'd suggest to quickly address @umangachapagain initial review. Thanks.

@vbnrh vbnrh force-pushed the rook-admission-controller branch from 68bb6b7 to d115618 Compare March 23, 2020 14:30
@ghost
Copy link

ghost commented Mar 23, 2020

There were the following issues with this Pull Request

  • Commit: d115618
    • ✖ message may not be empty
    • ✖ type may not be empty

You may need to change the commit messages to comply with the repository contributing guidelines.


🤖 This comment was generated by commitlint[bot]. Please report issues here.

Happy coding!

@vbnrh
Copy link
Contributor Author

vbnrh commented Mar 23, 2020

@travisn @leseb @umangachapagain @aruniiird Please review !

@vbnrh vbnrh force-pushed the rook-admission-controller branch from d115618 to c99c163 Compare March 23, 2020 15:31
@ghost
Copy link

ghost commented Mar 23, 2020

There were the following issues with this Pull Request

  • Commit: c99c163
    • ✖ message may not be empty
    • ✖ type may not be empty

You may need to change the commit messages to comply with the repository contributing guidelines.


🤖 This comment was generated by commitlint[bot]. Please report issues here.

Happy coding!

@vbnrh vbnrh changed the title [WIP] Rook: Admission controllers on Rook Rook: Adding support for admission controllers Mar 24, 2020
@vbnrh vbnrh force-pushed the rook-admission-controller branch from c99c163 to eeefef4 Compare March 24, 2020 04:05
Copy link
Member

@leseb leseb left a comment

Choose a reason for hiding this comment

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

Please update the doc as well as the pending release note file.

Copy link
Contributor

@aruniiird aruniiird left a comment

Choose a reason for hiding this comment

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

More of a general/design question, will there be scenario where user wants to add a new validation? In that case, how is that possible through this setup?

Copy link
Member

@travisn travisn left a comment

Choose a reason for hiding this comment

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

Definitely looking forward to the admission controller in rook! We just need to discuss the design first.

@vbnrh vbnrh force-pushed the rook-admission-controller branch from eeefef4 to 4a1cfc6 Compare March 25, 2020 18:25
@vbnrh vbnrh force-pushed the rook-admission-controller branch 2 times, most recently from f4f611b to 2115fe2 Compare March 25, 2020 18:38
@travisn
Copy link
Member

travisn commented Mar 25, 2020

@travisn , I initially thought the webhook to be a separate pluggable (optional) component to our rook operator so people could add their own custom validations if needed be.On the optimization/RBAC side, it is definitely a plus but If we add the the container as a side-car; Would'nt that make it a mandatory component alonside rook. Please let me know your thoughts on this....

@vbnrh How do you see someone adding their own custom validations? They would have to modify the rook repo to add new validations, right? This means it would still be tied to Rook. Or are you proposing a plugin mechanism? I don't see that. Let's schedule a discussion for the design.

- Added a [toolbox job](Documentation/ceph-toolbox.md#toolbox-job) for running a script with Ceph commands, similar to running commands in the Rook toolbox.
- Ceph RBD Mirror daemon has been extracted to its own CRD, it has been removed from the `CephCluster` CRD, see the [rbd-mirror crd](Documentation/ceph-rbd-mirror-crd.html).
- CephCluster CRD has been converted to use the controller-runtime framework.
- Added [admission controller](Documentation/admission-controller-usage.md) support in Rook for CRD validations.
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
- Added [admission controller](Documentation/admission-controller-usage.md) support in Rook for CRD validations.
- Added [admission controller](Documentation/admission-controller-usage.md) support for Ceph CRD validations.

# Set the variables for webhook. Any change here for namespace and service name should be done in pkg/operator/admission/init.go also
NAMESPACE="rook-ceph"
WEBHOOK_CONFIG_NAME="rook-ceph-webhook"
SERVICE_NAME="rook-admission-controller"
Copy link
Member

Choose a reason for hiding this comment

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

Each rook operator will need to deploy its own admission controller. in this PR only the admission controller for Ceph is being implemented. Wherever there is something specific to the ceph admission controller we should add it to the name.

Suggested change
SERVICE_NAME="rook-admission-controller"
SERVICE_NAME="rook-ceph-admission-controller"

return true, nil
}

func createWebhookService(context *clusterd.Context) error {
Copy link
Member

Choose a reason for hiding this comment

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

We really need some unit tests for the new admission package. These functions that create service, deployment, or pod specs are generally simple to test. You can see other operator unit tests for examples where services or other resources are created.

Copy link
Member

Choose a reason for hiding this comment

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

Integration tests will also confirm the admission controller can start and run successfully. I believe we could keep this fairly simple and get a lot of validation with these steps:

  • In the CreateCephOperator() method:
    • Create the RBAC. The easiest thing might be to add the yaml to GetRookOperator() in the ceph_manifests.go.
    • Call out to the script that generates the self-signed cert
  • After the tests start the operator around this line, verify that the admission controller is in running state
	if !h.k8shelper.IsPodInExpectedState("rook-ceph-admission-controller", onamespace, "Running") {
		logger.Error("admission controller is not running")
		return false, err
	}

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Added integration tests

Copy link
Member

Choose a reason for hiding this comment

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

As a follow-up PR, please also add unit tests in addition to the integration tests.

@vbnrh vbnrh force-pushed the rook-admission-controller branch 3 times, most recently from cebd552 to f8485fd Compare June 4, 2020 13:36
},
},
}
_, err := context.Clientset.CoreV1().Services(namespace).Create(&webhookService)
Copy link
Member

Choose a reason for hiding this comment

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

In case the service spec has changed between releases, the k8sutils.CreateOrUpdateService() method will help here.

@vbnrh vbnrh force-pushed the rook-admission-controller branch 10 times, most recently from 8aca28f to aa89a37 Compare June 5, 2020 14:23
if !h.k8shelper.IsPodInExpectedState("rook-ceph-admission-controller", onamespace, "Running") {
h.k8shelper.GetStatus(onamespace)
logger.Error("admission controller is not running")
return false, err
Copy link
Member

Choose a reason for hiding this comment

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

Instead of returning false, I'd suggest and assert.Fail() and then allow the test to continue. This will raise an error to fail the test, but it won't prevent the test from running.

return err
}

err = h.startAdmissionControllers(namespace)
Copy link
Member

Choose a reason for hiding this comment

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

You're configuring the admission controller after the helm chart is installed, which means the operator will already be running. You need to configure the admission controller before launching the helm chart.

Copy link
Member

@travisn travisn left a comment

Choose a reason for hiding this comment

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

To follow up from our discussion on the symlinks...

  • We need to avoid duplicating the scripts
  • The scripts are needed in tests/scripts for the integration tests
  • Instead of duplicating the scripts in cluster/examples/kubernetes/ceph/admission-controller, add a single script such as cluster/examples/kubernetes/ceph/config-admission-controller.sh that calls out to the scripts under test/scripts.

Copy link
Member

@travisn travisn left a comment

Choose a reason for hiding this comment

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

The integration test is working locally for me now, just a few final suggestions.

@travisn
Copy link
Member

travisn commented Jun 23, 2020

@BlaineEXE @jmolmo @umangachapagain @aruniiird Any final feedback? If no response we will plan on merging tomorrow.

adds deploy.sh script to deploy validatingwebhookconfiguration and create secrets.
adds new command ceph admission-controller to start webhook servers.
adds validation for various rook custom resources

Signed-off-by: Vineet Badrinath <vbadrina@redhat.com>
Copy link
Contributor

@aruniiird aruniiird left a comment

Choose a reason for hiding this comment

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

Seems good to me.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

ceph main ceph tag feature

Projects

None yet

Development

Successfully merging this pull request may close these issues.

9 participants