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

operator: Clean up webhook when disabled #11432

Merged
merged 1 commit into from
Dec 15, 2022

Conversation

subhamkrai
Copy link
Contributor

@subhamkrai subhamkrai commented Dec 13, 2022

disable webhook in the downstream cluster.

Signed-off-by: subhamkrai srai@redhat.com

Description of your changes:

Which issue is resolved by this Pull Request:
Resolves #

Checklist:

  • Commit Message Formatting: Commit titles and messages follow guidelines in the developer guide).
  • Skip Tests for Docs: If this is only a documentation change, add the label skip-ci on the PR.
  • Reviewed the developer guide on Submitting a Pull Request
  • Pending release notes updated with breaking and/or notable changes for the next minor release.
  • Documentation has been updated, if necessary.
  • Unit tests have been added, if necessary.
  • Integration tests have been added, if necessary.

@subhamkrai
Copy link
Contributor Author

testing result before and after webhook enable and disabled

srai@192 ~ (disable-webhook-openshift) $ kc get issuers.cert-manager.io 
NAME                READY   AGE
selfsigned-issuer   True    3m33s
~/go/src/github.com/rook
srai@192 ~ (disable-webhook-openshift) $ kc get certificates
NAME                             READY   SECRET                           AGE
rook-admission-controller-cert   True    rook-ceph-admission-controller   3m33s
~/go/src/github.com/rook
srai@192 ~ (disable-webhook-openshift) $ kc get validatingwebhookconfigurations.admissionregistration.k8s.io 
NAME                   WEBHOOKS   AGE
cert-manager-webhook   1          12m
rook-ceph-webhook      5          3m35s
~/go/src/github.com/rook
srai@192 ~ (disable-webhook-openshift) $ kc get svc
NAME                             TYPE        CLUSTER-IP       EXTERNAL-IP   PORT(S)             AGE
rook-ceph-admission-controller   ClusterIP   10.101.113.45    <none>        443/TCP             3m39s
rook-ceph-mgr                    ClusterIP   10.109.205.137   <none>        9283/TCP            5m34s
rook-ceph-mgr-dashboard          ClusterIP   10.103.84.30     <none>        7000/TCP            5m34s
rook-ceph-mon-a                  ClusterIP   10.104.13.92     <none>        6789/TCP,3300/TCP   6m22s
~/go/src/github.com/rook
srai@192 ~ (disable-webhook-openshift) $ kc get issuers.cert-manager.io 
No resources found in rook-ceph namespace.
~/go/src/github.com/rook
srai@192 ~ (disable-webhook-openshift) $ kc get certificates
No resources found in rook-ceph namespace.
~/go/src/github.com/rook
srai@192 ~ (disable-webhook-openshift) $ kc get validatingwebhookconfigurations.admissionregistration.k8s.io 
NAME                   WEBHOOKS   AGE
cert-manager-webhook   1          14m
~/go/src/github.com/rook
srai@192 ~ (disable-webhook-openshift) $ kc get svc
NAME                      TYPE        CLUSTER-IP       EXTERNAL-IP   PORT(S)             AGE
rook-ceph-mgr             ClusterIP   10.109.205.137   <none>        9283/TCP            8m
rook-ceph-mgr-dashboard   ClusterIP   10.103.84.30     <none>        7000/TCP            8m
rook-ceph-mon-a           ClusterIP   10.104.13.92     <none>        6789/TCP,3300/TCP   8m48s
rook-ceph-rgw-my-store    ClusterIP   10.111.243.80    <none>        80/TCP              28s

@subhamkrai subhamkrai marked this pull request as ready for review December 14, 2022 02:51
@subhamkrai
Copy link
Contributor Author

btw @travisn anything in the below check needs to be updated

func (c *CephCluster) ValidateCreate() error {
logger.Infof("validate create cephcluster %q", c.ObjectMeta.Name)
//If external mode enabled, then check if other fields are empty
if c.Spec.External.Enable {
if c.Spec.Mon != (MonSpec{}) || c.Spec.Dashboard != (DashboardSpec{}) || !reflect.DeepEqual(c.Spec.Monitoring, (MonitoringSpec{})) || c.Spec.DisruptionManagement != (DisruptionManagementSpec{}) || len(c.Spec.Mgr.Modules) > 0 || len(c.Spec.Network.Provider) > 0 || len(c.Spec.Network.Selectors) > 0 {
return errors.New("invalid create : external mode enabled cannot have mon,dashboard,monitoring,network,disruptionManagement,storage fields in CR")
}
}
return nil
}

since original error is coming from above only

Error while reconciling: admission webhook "cephcluster-wh-rook-ceph-admission-controller-openshift-storage.rook.io" denied the request: invalid create : external mode enabled cannot have mon,dashboard,monitoring,network,disruptionManagement,storage fields in CR

deploy/examples/operator-openshift.yaml Outdated Show resolved Hide resolved
deploy/examples/operator-openshift.yaml Outdated Show resolved Hide resolved
@@ -213,6 +213,8 @@ function generate_package() {
function apply_rook_op_img() {
"${YQ_CMD_WRITE[@]}" "$CSV_FILE_NAME" metadata.annotations.containerImage "$ROOK_OP_VERSION"
"${YQ_CMD_WRITE[@]}" "$CSV_FILE_NAME" spec.install.spec.deployments[0].spec.template.spec.containers[0].image "$ROOK_OP_VERSION"
"${YQ_CMD_WRITE[@]}" "$CSV_FILE_NAME" spec.install.spec.deployments[0].spec.template.spec.containers[0].env[6].value "true"
Copy link
Member

Choose a reason for hiding this comment

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

The env[6] could be brittle if another env var is added or removed. Instead of replacing the value like this, I wonder if it would be simpler to leave the env var out of the operator-openshift.yaml, and append the env var here with yq.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

this is something I tried most of the time today to make more generalize and I was able to do it is possible wit yq 4.x but currently in rook csv generation its hard requirements 3.x

deploy/examples/operator-openshift.yaml Outdated Show resolved Hide resolved
pkg/operator/ceph/webhook.go Outdated Show resolved Hide resolved
disable webhook in downstream cluster.

Signed-off-by: subhamkrai <srai@redhat.com>
}
return nil
logger.Info("deleting webhook cert manager Issuer %s", issuerName)
_ = certMgrClient.Issuers(namespace).Delete(ctx, issuerName, metav1.DeleteOptions{})
Copy link
Member

Choose a reason for hiding this comment

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

@travisn should we log this error message for debugging in case of any failures?

Copy link
Member

Choose a reason for hiding this comment

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

oh, just missed this message. Yes we should at least log the failures even if we don't fail on them.
@subhamkrai How about a follow up PR?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

here #11448

@travisn travisn merged commit fa35c06 into rook:master Dec 15, 2022
mergify bot added a commit that referenced this pull request Dec 15, 2022
operator: disable webhook by default (backport #11432)
@subhamkrai subhamkrai deleted the disable-webhook-openshift branch December 16, 2022 03:08
@travisn travisn changed the title operator: disable webhook by default operator: Clean up webhook when disabled Dec 21, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants