Skip to content
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 a webhook for volumereplication and volumereplicationclass #90

Closed
wants to merge 8 commits into from

Conversation

iamniting
Copy link
Member

Fixes #88, #89

Signed-off-by: Nitin Goyal [email protected]

operator-sdk alpha config-3alpha-to-3

WARN[0000] Config version 3-alpha has been stabilized as 3, and 3-alpha is no
longer supported. Run `operator-sdk alpha config-3alpha-to-3` to upgrade
your PROJECT config file to version 3

Signed-off-by: Nitin Goyal <[email protected]>
operator-sdk create webhook --group replication --version v1alpha1 \
--kind VolumeReplication --programmatic-validation --defaulting

Signed-off-by: Nitin Goyal <[email protected]>
@iamniting iamniting requested review from sp98 and Madhu-1 June 24, 2021 08:51
@iamniting
Copy link
Member Author

I have not tested this yet. Will give it a shot.

@Madhu-1
Copy link
Member

Madhu-1 commented Jun 24, 2021

Please provide the logs and output once you finish with the testing.

@Madhu-1 Madhu-1 added the DNM Do not merge label Jun 24, 2021
@iamniting
Copy link
Member Author

VRC webhook results when I am trying to change anything in the spec

$ oc edit vrc vrc 
error: volumereplicationclasses.replication.storage.openshift.io "vrc" could not be patched: admission webhook "vvolumereplicationclass.kb.io" denied the request: Spec.Provisioner is immutable
You can run `oc replace -f /tmp/oc-edit-cb67t.yaml` to try this update again.
$ oc edit vrc vrc 
error: volumereplicationclasses.replication.storage.openshift.io "vrc" could not be patched: admission webhook "vvolumereplicationclass.kb.io" denied the request: Spec.Parameters is immutable
You can run `oc replace -f /tmp/oc-edit-0brft.yaml` to try this update again.

operator-sdk create webhook --group replication --version v1alpha1 \
--kind VolumeReplicationClass --programmatic-validation --defaulting

Signed-off-by: Nitin Goyal <[email protected]>
@iamniting
Copy link
Member Author

VR webhook is not working as expected as of now will debug and update

$ oc edit vr vr1
error: volumereplications.replication.storage.openshift.io "vr1" could not be patched: Internal error occurred: failed calling webhook "mvolumereplication.kb.io": webhook returned response.patchType but not response.patch
You can run `oc replace -f /tmp/oc-edit-3uojs.yaml` to try this update again.

@iamniting
Copy link
Member Author

VR webhook is not working as expected as of now will debug and update

$ oc edit vr vr1
error: volumereplications.replication.storage.openshift.io "vr1" could not be patched: Internal error occurred: failed calling webhook "mvolumereplication.kb.io": webhook returned response.patchType but not response.patch
You can run `oc replace -f /tmp/oc-edit-3uojs.yaml` to try this update again.

Let me try this if it works operator-framework/operator-sdk#4602

@iamniting
Copy link
Member Author

Finally VR webhook is also working

$ oc edit vr vr1
error: volumereplications.replication.storage.openshift.io "vr1" could not be patched: admission webhook "vvolumereplication.kb.io" denied the request: Spec.DataSource is immutable
You can run `oc replace -f /tmp/oc-edit-3nzi3.yaml` to try this update again.
$ oc edit vr vr1
error: volumereplications.replication.storage.openshift.io "vr1" could not be patched: admission webhook "vvolumereplication.kb.io" denied the request: Spec.VolumeReplicationClass is immutable
You can run `oc replace -f /tmp/oc-edit-vuh8i.yaml` to try this update again.

Copy link
Collaborator

@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.

Update PROJECT config for 3alpha to 3 commit should be a separate PR as it is not specific to webhooks. This will help us track changes through PRs easily.

VolumeReplication webhook scaffold and implementation should be a single commit. Same for VolumeReplicationClass webhook scaffold and implementation.

Controller runtime update should be a separate PR. Sometimes it has breaking changes which needs explicit communication.

# TODO: remove --skip-files controllers/suite_test.go once logic is implemented
args: -E gosec --timeout=6m --skip-files controllers/suite_test.go
# TODO: remove --skip-files controllers/suite_test.go api/v1alpha1/webhook_suite_test.go once logic is implemented
args: -E gosec --timeout=6m --skip-files controllers/suite_test.go,api/v1alpha1/webhook_suite_test.go
Copy link
Collaborator

Choose a reason for hiding this comment

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

Why are we skipping tests instead of implementing it? At the every least we need to mark it as TODO and create a tracker issue.

Copy link
Member Author

Choose a reason for hiding this comment

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

It already a TODO to remove these skip-files once we implement tests. We can create a issue.

@iamniting
Copy link
Member Author

iamniting commented Jul 5, 2021

Update PROJECT config for 3alpha to 3 commit should be a separate PR as it is not specific to webhooks. This will help us track changes through PRs easily.

This can be done. I faced this issue while creating scafolding for webhook that's why updated in this one itself

VolumeReplication webhook scaffold and implementation should be a single commit. Same for VolumeReplicationClass webhook scaffold and implementation.

I would like to keep them as separate commits. It makes life easy to distinguish b/w generated code and implemented logic.

Controller runtime update should be a separate PR. Sometimes it has breaking changes which needs explicit communication.

I still think that we should send with this PR itself, because new controller runtime has a fix for webhook itself.

@iamniting
Copy link
Member Author

iamniting commented Jul 5, 2021

How to test:
once you have a working setup

change the image to CSI_VOLUME_REPLICATION_IMAGE: quay.io/nigoyal/volumereplication-operator:webhook
kubectl edit configmaps rook-ceph-operator-config

create a cert manager
kubectl apply -f https://github.com/jetstack/cert-manager/releases/download/v1.4.0/cert-manager.yaml

create certies, required service etc
kubectl apply -f https://raw.githubusercontent.com/iamniting/conf/master/k8s/vr/webhook-conf.yaml --validate=false

now you can test the webhook functionality

@mergify
Copy link

mergify bot commented Aug 25, 2021

This pull request now has conflicts with the target branch. Could you please resolve conflicts and force push the corrected changes? 🙏

@iamniting
Copy link
Member Author

Closing this PR as it is not required anymore.

@iamniting iamniting closed this Dec 8, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
DNM Do not merge
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Mark parameter of volume replication class as immutable object
3 participants