-
Notifications
You must be signed in to change notification settings - Fork 43
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Create validation webhook for lvmcluster #255
Conversation
Signed-off-by: Nitin Goyal <[email protected]>
Output of: ./bin/operator-sdk create webhook --group lvm --version v1alpha1 --kind LVMCluster --programmatic-validation Signed-off-by: Nitin Goyal <[email protected]>
af8e126
to
fe7db9e
Compare
@nbalacha @sp98 We have one corner case for the update validation where we should allow the user to delete the wrong path from the LVMCluster but we are not. As it is really difficult to find if the path is really wrong or not from the webhook server. Steps to reproduce:
Workaround:
|
ab698ac
to
fa75cd7
Compare
apiVersion: cert-manager.io/v1 | ||
kind: Certificate | ||
metadata: | ||
name: serving-cert # this name should match the one appeared in kustomizeconfig.yaml |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't see this in kustomizeconfig.yaml
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is not required as we are not using cert-manager in the openshift
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
How are certificates set up in Openshift? What about if the operator is installed on plain K8s?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
They are set up using the https://github.com/openshift/service-ca-operator and OLM.
In the case of k8s user has to make changes to the manifests to get it working in k8s.
82eccb0
to
8853ee1
Compare
Signed-off-by: Nitin Goyal <[email protected]>
Signed-off-by: Nitin Goyal <[email protected]>
Signed-off-by: Nitin Goyal <[email protected]>
8853ee1
to
08188c4
Compare
@iamniting: The following test failed, say
Full PR test history. Your PR dashboard. Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. I understand the commands that are listed here. |
if val, ok := devices[nodeSelector][path]; ok { | ||
var err error | ||
if val != deviceClass.Name { | ||
err = fmt.Errorf("Error: device path %s overlaps in two different deviceClasss %s and %s", path, val, deviceClass.Name) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ideally this is no required seeing that we are now restricting it to one deviceClass.
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: iamniting, nbalacha The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
webhook intercepts the faulty CR and raises an appropriate error to guide the user.
Signed-off-by: Nitin Goyal [email protected]
Ref:
https://olm.operatorframework.io/docs/advanced-tasks/adding-admission-and-conversion-webhooks/
https://github.com/openshift/service-ca-operator