✨ Add webhook framework #194

jichenjc · 2021-07-22T07:59:25Z

What this PR does / why we need it:

enabled a set of manifests and webhook configuration, so the webhook sevice
will run on 9443 of capn-controller-manager , in case we edit the nestedcluster ,e.g

kubectl patch --type=merge nestedcluster nestedcluster-sample -p '{"spec":{"controlPlaneEndpoint":{"port":6444}}}'

we will have following error

....
rolPlaneEndpoint:v1alpha4.APIEndpoint{Host:"localhost", Port:6444}}, Status:v1alpha4.NestedClusterStatus{Ready:false}}: Nestedclusters spec field is immutable. Please create a new resource instead. Ref doc: https://cluster-api.sigs.k8s.io/tasks/change-machine-template.html

Which issue(s) this PR fixes (optional, in fixes #<issue number>(, fixes #<issue_number>, ...) format, will close the issue(s) when PR gets merged):
part of #184

gyliu513 · 2021-07-23T12:40:52Z

@jichenjc I was using dev build and found the patch can be successful, am I missing anything?

root@xanthene1:~/go/src/github.com/kubernetes-sigs/cluster-api-provider-nested# kubectl patch --type=merge nestedcluster nestedcluster-sample -p '{"spec":{"controlPlaneEndpoint":{"port":6444}}}'
nestedcluster.infrastructure.cluster.x-k8s.io/nestedcluster-sample patched

root@xanthene1:~/go/src/github.com/kubernetes-sigs/cluster-api-provider-nested# kubectl  get nestedclusters.infrastructure.cluster.x-k8s.io -A -oyaml
apiVersion: v1
items:
- apiVersion: infrastructure.cluster.x-k8s.io/v1alpha4
  kind: NestedCluster
  metadata:
    annotations:
      kubectl.kubernetes.io/last-applied-configuration: |
        {"apiVersion":"infrastructure.cluster.x-k8s.io/v1alpha4","kind":"NestedCluster","metadata":{"annotations":{},"name":"nestedcluster-sample","namespace":"default"},"spec":{"controlPlaneEndpoint":{"host":"localhost","port":6443}}}
    creationTimestamp: "2021-07-23T03:52:17Z"
    generation: 2
    labels:
      cluster.x-k8s.io/cluster-name: cluster-sample
    managedFields:
    - apiVersion: infrastructure.cluster.x-k8s.io/v1alpha4
      fieldsType: FieldsV1
      fieldsV1:
        f:metadata:
          f:annotations:
            .: {}
            f:kubectl.kubernetes.io/last-applied-configuration: {}
        f:spec:
          .: {}
          f:controlPlaneEndpoint:
            .: {}
            f:host: {}
            f:port: {}
      manager: kubectl
      operation: Update
      time: "2021-07-23T03:52:17Z"
    - apiVersion: infrastructure.cluster.x-k8s.io/v1alpha4
      fieldsType: FieldsV1
      fieldsV1:
        f:metadata:
          f:labels:
            .: {}
            f:cluster.x-k8s.io/cluster-name: {}
          f:ownerReferences:
            .: {}
            k:{"uid":"ece6ff0b-be59-456d-ac1d-b5387c0dba64"}:
              .: {}
              f:apiVersion: {}
              f:blockOwnerDeletion: {}
              f:controller: {}
              f:kind: {}
              f:name: {}
              f:uid: {}
        f:status:
          .: {}
          f:ready: {}
      manager: manager
      operation: Update
      time: "2021-07-23T03:55:44Z"
    name: nestedcluster-sample
    namespace: default
    ownerReferences:
    - apiVersion: cluster.x-k8s.io/v1alpha4
      blockOwnerDeletion: true
      controller: true
      kind: Cluster
      name: cluster-sample
      uid: ece6ff0b-be59-456d-ac1d-b5387c0dba64
    resourceVersion: "167373"
    uid: f5db2a16-be0d-42a6-9b43-7c42e592d83b
  spec:
    controlPlaneEndpoint:
      host: localhost
      port: 6444
  status:
    ready: true
kind: List
metadata:
  resourceVersion: ""
  selfLink: ""

root@xanthene1:~/go/src/github.com/kubernetes-sigs/cluster-api-provider-nested# kubectl patch --type=merge nestedcluster nestedcluster-sample -p '{"spec":{"controlPlaneEndpoint":{"port":6443}}}'
nestedcluster.infrastructure.cluster.x-k8s.io/nestedcluster-sample patched

root@xanthene1:~/go/src/github.com/kubernetes-sigs/cluster-api-provider-nested# kubectl  get nestedclusters.infrastructure.cluster.x-k8s.io -A -oyaml
apiVersion: v1
items:
- apiVersion: infrastructure.cluster.x-k8s.io/v1alpha4
  kind: NestedCluster
  metadata:
    annotations:
      kubectl.kubernetes.io/last-applied-configuration: |
        {"apiVersion":"infrastructure.cluster.x-k8s.io/v1alpha4","kind":"NestedCluster","metadata":{"annotations":{},"name":"nestedcluster-sample","namespace":"default"},"spec":{"controlPlaneEndpoint":{"host":"localhost","port":6443}}}
    creationTimestamp: "2021-07-23T03:52:17Z"
    generation: 3
    labels:
      cluster.x-k8s.io/cluster-name: cluster-sample
    managedFields:
    - apiVersion: infrastructure.cluster.x-k8s.io/v1alpha4
      fieldsType: FieldsV1
      fieldsV1:
        f:metadata:
          f:annotations:
            .: {}
            f:kubectl.kubernetes.io/last-applied-configuration: {}
        f:spec:
          .: {}
          f:controlPlaneEndpoint:
            .: {}
            f:host: {}
            f:port: {}
      manager: kubectl
      operation: Update
      time: "2021-07-23T03:52:17Z"
    - apiVersion: infrastructure.cluster.x-k8s.io/v1alpha4
      fieldsType: FieldsV1
      fieldsV1:
        f:metadata:
          f:labels:
            .: {}
            f:cluster.x-k8s.io/cluster-name: {}
          f:ownerReferences:
            .: {}
            k:{"uid":"ece6ff0b-be59-456d-ac1d-b5387c0dba64"}:
              .: {}
              f:apiVersion: {}
              f:blockOwnerDeletion: {}
              f:controller: {}
              f:kind: {}
              f:name: {}
              f:uid: {}
        f:status:
          .: {}
          f:ready: {}
      manager: manager
      operation: Update
      time: "2021-07-23T03:55:44Z"
    name: nestedcluster-sample
    namespace: default
    ownerReferences:
    - apiVersion: cluster.x-k8s.io/v1alpha4
      blockOwnerDeletion: true
      controller: true
      kind: Cluster
      name: cluster-sample
      uid: ece6ff0b-be59-456d-ac1d-b5387c0dba64
    resourceVersion: "167479"
    uid: f5db2a16-be0d-42a6-9b43-7c42e592d83b
  spec:
    controlPlaneEndpoint:
      host: localhost
      port: 6443
  status:
    ready: true
kind: List
metadata:
  resourceVersion: ""
  selfLink: ""

jichenjc · 2021-07-26T02:25:54Z

@gyliu513 thanks！I forgot to add this new file config/webhook/manifests.yaml into git
can you apply this file and try again on your env ? I used a new env to re-verifiy and it should be good now...

gyliu513 · 2021-07-26T03:02:52Z

@jichenjc my question here is: Even without your fix, I did not get any error when kubectl patch, do you know why? :)

jichenjc · 2021-07-26T03:30:02Z

@gyliu513 because my fix added a ValidatingWebhookConfiguration
this is important because without this CR, the validation will not validate anything
now wiith this, whenever we update the nested cluster, it will post API to the URL here

https://github.com/kubernetes-sigs/cluster-api-provider-nested/pull/194/files#diff-369b61dd1f2f1f60722927fda70e5a51e130cea68336e6d536086b522ffef0c6R15

and do the validation ..

christopherhein

Looking really good so far. Are we able to write any automated tests for this? Alson one nit about the main.go changes.

christopherhein · 2021-07-26T17:48:29Z

main.go

+func setupWebhooks(mgr ctrl.Manager) {
+	if err := (&infrastructurev1.NestedCluster{}).SetupWebhookWithManager(mgr); err != nil {
+		setupLog.Error(err, "unable to create webhook", "webhook", "NestedCluster")
+		os.Exit(1)
+	}
+}


Can we have this if block defined above in main() where setupWebhooks(mgr) is, the go tag below // +kubebuilder:scaffold:builder should actually auto inject this for you if you use kubebuilder new ... so to keep the configuration the same place always then when anyone makes changes they always get the same exp.

not sure I fully understand the request, anyway, I updated per my understanding

Just to reduce turn around cycles on this, all I'm thinking if instead of calling/adding a new func for this put the if block back in the main() func where you are calling the func.

The reasoning is the go comment tag is what kubebuilder uses to inject code and one of the piece of code it usually injects is this same exact if block.

ok, I think I understand the right thing, I updated with additional test, please help check again, thanks !~
@christopherhein

christopherhein · 2021-07-27T02:58:45Z

main.go

+	if err := (&infrastructurev1.NestedCluster{}).SetupWebhookWithManager(mgr); err != nil {
+		setupLog.Error(err, "unable to create webhook", "webhook", "NestedCluster")
+		os.Exit(1)
+	}


Sorry to be pedantic about this but can you move it above // +kubebuilder:scaffold:builder

Other than this, this PR looks and works great. Once this is done we can lgtm and approve it.

thanks for comment, just updated , appreciate the review~

christopherhein

/lgtm
/approve

🎉

k8s-ci-robot · 2021-07-27T05:04:22Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: christopherhein, jichenjc

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:

~~OWNERS~~ [christopherhein]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

k8s-ci-robot added do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. labels Jul 22, 2021

k8s-ci-robot requested review from CecileRobertMichon and davidewatson July 22, 2021 07:59

k8s-ci-robot added the size/L Denotes a PR that changes 100-499 lines, ignoring generated files. label Jul 22, 2021

jichenjc force-pushed the add_webhook branch 2 times, most recently from 13cdd9f to f8a37d1 Compare July 23, 2021 09:00

jichenjc changed the title ~~WIP: Add webhook framework~~ Add webhook framework Jul 23, 2021

k8s-ci-robot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Jul 23, 2021

jichenjc force-pushed the add_webhook branch from f8a37d1 to 02b5630 Compare July 23, 2021 09:30

jichenjc force-pushed the add_webhook branch from 02b5630 to a2af494 Compare July 26, 2021 02:23

christopherhein suggested changes Jul 26, 2021

View reviewed changes

jichenjc force-pushed the add_webhook branch 2 times, most recently from 64ae4b9 to 3565983 Compare July 27, 2021 01:17

christopherhein changed the title ~~Add webhook framework~~ ✨ Add webhook framework Jul 27, 2021

christopherhein reviewed Jul 27, 2021

View reviewed changes

Add webhook framework

c1d92df

jichenjc force-pushed the add_webhook branch from 3565983 to c1d92df Compare July 27, 2021 04:02

christopherhein approved these changes Jul 27, 2021

View reviewed changes

k8s-ci-robot assigned christopherhein Jul 27, 2021

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Jul 27, 2021

k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Jul 27, 2021

k8s-ci-robot merged commit 058f132 into kubernetes-retired:main Jul 27, 2021

christopherhein mentioned this pull request Sep 22, 2021

🐛 Fix webhook issue when there are multiple replication of controller #217

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

✨ Add webhook framework #194

✨ Add webhook framework #194

jichenjc commented Jul 22, 2021 •

edited

Loading

gyliu513 commented Jul 23, 2021 •

edited

Loading

jichenjc commented Jul 26, 2021

gyliu513 commented Jul 26, 2021

jichenjc commented Jul 26, 2021

christopherhein left a comment

christopherhein Jul 26, 2021

jichenjc Jul 27, 2021

christopherhein Jul 27, 2021

jichenjc Jul 27, 2021

christopherhein Jul 27, 2021

christopherhein Jul 27, 2021

jichenjc Jul 27, 2021

christopherhein left a comment

k8s-ci-robot commented Jul 27, 2021

✨ Add webhook framework #194

✨ Add webhook framework #194

Conversation

jichenjc commented Jul 22, 2021 • edited Loading

gyliu513 commented Jul 23, 2021 • edited Loading

jichenjc commented Jul 26, 2021

gyliu513 commented Jul 26, 2021

jichenjc commented Jul 26, 2021

christopherhein left a comment

Choose a reason for hiding this comment

christopherhein Jul 26, 2021

Choose a reason for hiding this comment

jichenjc Jul 27, 2021

Choose a reason for hiding this comment

christopherhein Jul 27, 2021

Choose a reason for hiding this comment

jichenjc Jul 27, 2021

Choose a reason for hiding this comment

christopherhein Jul 27, 2021

Choose a reason for hiding this comment

christopherhein Jul 27, 2021

Choose a reason for hiding this comment

jichenjc Jul 27, 2021

Choose a reason for hiding this comment

christopherhein left a comment

Choose a reason for hiding this comment

k8s-ci-robot commented Jul 27, 2021

jichenjc commented Jul 22, 2021 •

edited

Loading

gyliu513 commented Jul 23, 2021 •

edited

Loading