ceph: Adding support for admission controllers#5045
Conversation
|
There were the following issues with this Pull Request
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! |
umangachapagain
left a comment
There was a problem hiding this comment.
Can you provide context and explanation around this feature?
It is not obvious from the PR.
What is the scope of this PR?
leseb
left a comment
There was a problem hiding this comment.
@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.
68bb6b7 to
d115618
Compare
|
There were the following issues with this Pull Request
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! |
|
@travisn @leseb @umangachapagain @aruniiird Please review ! |
d115618 to
c99c163
Compare
|
There were the following issues with this Pull Request
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! |
c99c163 to
eeefef4
Compare
leseb
left a comment
There was a problem hiding this comment.
Please update the doc as well as the pending release note file.
cluster/examples/kubernetes/ceph/admission-controller/deployment/roles.yaml
Outdated
Show resolved
Hide resolved
cluster/examples/kubernetes/ceph/admission-controller/deployment/webhook.yaml
Outdated
Show resolved
Hide resolved
cluster/examples/kubernetes/ceph/admission-controller/hack/deploy.sh
Outdated
Show resolved
Hide resolved
cluster/examples/kubernetes/ceph/admission-controller/hack/deploy.sh
Outdated
Show resolved
Hide resolved
cluster/examples/kubernetes/ceph/admission-controller/hack/deploy.sh
Outdated
Show resolved
Hide resolved
cluster/examples/kubernetes/ceph/admission-controller/hack/deploy.sh
Outdated
Show resolved
Hide resolved
aruniiird
left a comment
There was a problem hiding this comment.
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?
travisn
left a comment
There was a problem hiding this comment.
Definitely looking forward to the admission controller in rook! We just need to discuss the design first.
cluster/examples/kubernetes/ceph/admission-controller/deployment/deployment.yaml
Outdated
Show resolved
Hide resolved
cluster/examples/kubernetes/ceph/admission-controller/deployment/deployment.yaml
Outdated
Show resolved
Hide resolved
cluster/examples/kubernetes/ceph/admission-controller/deployment/webhook.yaml
Outdated
Show resolved
Hide resolved
cluster/examples/kubernetes/ceph/admission-controller/hack/webhook-create-signed-cert.sh
Show resolved
Hide resolved
eeefef4 to
4a1cfc6
Compare
f4f611b to
2115fe2
Compare
@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. |
PendingReleaseNotes.md
Outdated
| - 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. |
There was a problem hiding this comment.
| - 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" |
There was a problem hiding this comment.
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.
| SERVICE_NAME="rook-admission-controller" | |
| SERVICE_NAME="rook-ceph-admission-controller" |
| return true, nil | ||
| } | ||
|
|
||
| func createWebhookService(context *clusterd.Context) error { |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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
}
There was a problem hiding this comment.
Added integration tests
There was a problem hiding this comment.
As a follow-up PR, please also add unit tests in addition to the integration tests.
cebd552 to
f8485fd
Compare
pkg/operator/admission/webhook.go
Outdated
| }, | ||
| }, | ||
| } | ||
| _, err := context.Clientset.CoreV1().Services(namespace).Create(&webhookService) |
There was a problem hiding this comment.
In case the service spec has changed between releases, the k8sutils.CreateOrUpdateService() method will help here.
8aca28f to
aa89a37
Compare
| if !h.k8shelper.IsPodInExpectedState("rook-ceph-admission-controller", onamespace, "Running") { | ||
| h.k8shelper.GetStatus(onamespace) | ||
| logger.Error("admission controller is not running") | ||
| return false, err |
There was a problem hiding this comment.
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) |
There was a problem hiding this comment.
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.
cluster/examples/kubernetes/ceph/admission-controller/deploy.sh
Outdated
Show resolved
Hide resolved
travisn
left a comment
There was a problem hiding this comment.
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.shthat calls out to the scripts under test/scripts.
cluster/examples/kubernetes/ceph/admission-controller/config-admission-controllers.sh
Outdated
Show resolved
Hide resolved
cluster/examples/kubernetes/ceph/admission-controller/config-admission-controllers.sh
Outdated
Show resolved
Hide resolved
travisn
left a comment
There was a problem hiding this comment.
The integration test is working locally for me now, just a few final suggestions.
|
@BlaineEXE @jmolmo @umangachapagain @aruniiird Any final feedback? If no response we will plan on merging tomorrow. |
cluster/examples/kubernetes/ceph/config-admission-controller.sh
Outdated
Show resolved
Hide resolved
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>
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 allowedMore such validations can be applied for any kind of resource
How to test this PR
kubectl apply -f cluster.yamlOn doing all the above, we could see the request being rejected.
Which issue is resolved by this Pull Request:
#2363
#4819
[test ceph]