Webhooks setup and server #9

sergioifg94 · 2023-01-18T14:47:40Z

Description

Add the resources and logic to set up a webhook server running alongside the controller, as well as the reconciliation of the webhook configuration

Verification steps

In order to verify these changes, the controller must run in-cluster and be exposed via an Ingress.

Run the local-setup
```
make local-setup
```

Build the controller image

# Build the image
make docker-build
# Load it into the cluster
kind load docker-image controller:latest --name mctc-control-plane  --nodes mctc-control-plane-control-plane

Deploy the controller. Prior to that, ensure that the manager includes the required environment variables to run
```
make deploy
```
Deploy the secret with the kubeconfig for the control cluster

Verify the controller Ingress is created

kubectl get ingress mctc-ingress-mctc -n multi-cluster-traffic-controller-system

kind: Ingress
apiVersion: networking.k8s.io/v1
metadata:
  name: mctc-ingress-mctc
  namespace: multi-cluster-traffic-controller-system
  annotations:   
    mctc-component: webhook 
spec:
  ingressClassName: nginx
  tls:
    - hosts:
        - cwxkxzmah79fqyecvplwzf.sf.hcpapps.net
      secretName: cwxkxzmah79fqyecvplwzf.sf.hcpapps.net
  rules:
    - host: host.whatever.com
      http:
        paths:
          - path: /
            pathType: Prefix
            backend:
              service:
                name: mctc-mctc-webhooks
                port:
                  number: 8082
    - host: cwxkxzmah79fqyecvplwzf.sf.hcpapps.net
      http:
        paths:
          - path: /
            pathType: Prefix
            backend:
              service:
                name: mctc-mctc-webhooks
                port:
                  number: 8082
status:
  loadBalancer:
    ingress:
      - ip: 172.21.0.2

Verify that the webhook configuration is reconciled

 kubectl get mutatingwebhookconfiguration mctc -o yaml

Ensure it references the webhook server and the caClient field is set

// ...
clientConfig:
  caBundle: ...
  url: https://cwxkxzmah79fqyecvplwzf.sf.hcpapps.net/ingress
// ...

Verify that the webhook handler is reached successfully by making a change in the webhook ingress. For example, adding an annotation
```
kubectl edit ingress mctc-ingress-mctc -n multi-cluster-traffic-controller-system
```
The Pod logs should append level=info msg="WEBHOOK HANDLER", and the update should be performed correctly
In order to verify independently that the webhook creates successfully the new DNSRecord and Certificate, disable the traffic controller
1. Comment out the code that starts up the controller at main.go
2. Follow step 2 to build the image
3. Delete the controller Pod so that a new Pod with the new image is created
Create the sample service so that the creation of the Ingress triggers the webhook
```
kubectl apply -f samples/echo-service/echo.yaml -n default
```

Verify that the DNSRecord and the Certificate are created

kubectl get dnsrecords s6yxqyfx3awf53iasbk7a6.sf.hcpapps.net -n argocd
NAME                                    AGE
s6yxqyfx3awf53iasbk7a6.sf.hcpapps.net   117m

kubectl get certificate s6yxqyfx3awf53iasbk7a6.sf.hcpapps.net -n argocd
NAME                                    READY   SECRET                                  AGE
s6yxqyfx3awf53iasbk7a6.sf.hcpapps.net   True    s6yxqyfx3awf53iasbk7a6.sf.hcpapps.net   118m

Verify that the Ingress is mutated

kubectl get ingress echo -n default -o yaml

apiVersion: networking.k8s.io/v1
kind: Ingress
metadata:
  annotations:
    kuadrant.io/managed-hosts: s6yxqyfx3awf53iasbk7a6.sf.hcpapps.net
  creationTimestamp: "2023-01-25T10:07:15Z"
  generation: 1
  name: echo
  namespace: default
  resourceVersion: "8576"
  uid: caee7c05-cefe-43d6-bd67-6cd56a3033cd
spec:
  ingressClassName: nginx
  rules:
  - http:
      paths:
      - backend:
          service:
            name: echo
            port:
              number: 80
        path: /
        pathType: Prefix
  - host: s6yxqyfx3awf53iasbk7a6.sf.hcpapps.net
    http:
      paths:
      - backend:
          service:
            name: echo
            port:
              number: 80
        path: /
        pathType: Prefix
  tls:
  - hosts:
    - s6yxqyfx3awf53iasbk7a6.sf.hcpapps.net
    secretName: s6yxqyfx3awf53iasbk7a6.sf.hcpapps.net
status:
  loadBalancer:
    ingress:
    - ip: 172.21.0.2

philbrookes · 2023-01-24T09:28:50Z

pkg/admission/ingress/webhook_handler.go

+
+}
+
+func (h *Handler) handle(ctx context.Context, ingress *networkingv1.Ingress) (bool, error) {


This looks to be pulling the status from the Ingress to create entries in the DNS Record? I'm not sure the webhook is the best place for this? I think a lot of the logic in here should be instead run from the pkg/controllers/traffic/traffic_controller.go?

I have done a similar thing by calling out the traffic_controller logic from the syncer, in my syncer PR, see here: https://github.com/Kuadrant/multi-cluster-traffic-controller/pull/11/files#diff-cf3ce1c5bad0247b8bcf2c5d5eb789a53b4f752bc1684449472eca358dda2658R68

The same too for setting the TLS certificate and maybe even the managed host? It all will still ultimately be a response from the webhook, but by moving the logic into the traffic_controller we should get the same logic when reconciling the ingress (i.e. not just for the webhook)

My understanding is that we want to have that logic in both places (the reconciler and the webhook). It is a bit redundant for now but I think the webhook will have more complex logic later on when we implement custom domain verification.

Maybe @maleck13 can confirm that

So domain verification, host / dns record registration can be duplicated, but I would go for having the actual updating of the DNSRecord happen outside of the webhook if you follow?
Webhook can validate the ingress and create the initial DNSRecord and TLS certificte to register a new host and mutate the ingress to add tls section

my suggestion is rather: let's move the "duplicated" code to the traffic_controller, and call that from the webhook, effectively removing the duplication, anything that is webhook-specific should not be moved and instead stay here.

or in other words:

webhook receives ingress -> webhook passes ingress to traffic_controller -> webhook modifies returned ingress with webhook specific logic -> output ingress on webhook response.

traffic controller right now modifiies the ingress directly

yes, I don't think that makes this incompatible? It doesn't send it to the API, just updates it in memory.

I think there's still a few differences between the logic in the reconciler and the logic in the webhook. My understanding is that the webhook:

Only creates (but never updates) the DNSRecords and Certificates for the Ingress

Doesn't copy the TLS secret once it's created by cert-manager. It only sets the name in the TLS section of the Ingress

We could still probably refactor it to reduce the duplicated logic to a minimum but I'd leave that for another time I think

ok, we can address this later.

pkg/admission/ingress/webhook_handler.go

david-martin · 2023-01-26T16:19:16Z

Some cmds I used to get the controller running locally:

# Patch imagepullpolicy
kubectl -n multi-cluster-traffic-controller-system patch deployment mctc-controller-manager -p '{"spec": {"template": {"spec":{"containers":[{"name":"manager","imagePullPolicy":"Never"}]}}}}' -n multi-cluster-traffic-controller-system

# Patch envFrom
kubectl -n multi-cluster-traffic-controller-system patch deployment mctc-controller-manager -p '{"spec": {"template": {"spec": {"containers": [{"name":"manager", "envFrom": [{"configMapRef":{"name":"controller-config"}}, {"secretRef":{"name":"aws-credentials"}}] }] }}}}'

# Create Configmap from file
kubectl -n multi-cluster-traffic-controller-system create configmap controller-config $(for envvar in $(cat controller-config.env); do echo -n "--from-literal=$envvar ";done)

# Create Secret from file
kubectl -n multi-cluster-traffic-controller-system create secret generic aws-credentials $(for envvar in $(cat aws-credentials.env); do echo -n "--from-literal=$envvar ";done)

@sergioifg94 I'm getting an admission error when trying to create an ingress

admission webhook "validate.nginx.ingress.kubernetes.io" denied the request: host "_" and path "/" is already defined in ingress multi-cluster-traffic-controller-system/mctc-ingress-mctc

The ingress yaml:

apiVersion: networking.k8s.io/v1
kind: Ingress
metadata:
  labels:
    argocd.argoproj.io/instance: mctc-control-plane-echo-app
  name: echo
  namespace: echo-app
spec:
  rules:
    - host: ''
      http:
        paths:
          - backend:
              service:
                name: echo
                port:
                  number: 80
            path: /
            pathType: Prefix

The mctc webhook ingress that got created looks like this:

kubectl get ingress mctc-ingress-mctc -n multi-cluster-traffic-controller-system -o yaml
apiVersion: networking.k8s.io/v1
kind: Ingress
metadata:
  annotations:
    kubectl.kubernetes.io/last-applied-configuration: |
      {"apiVersion":"networking.k8s.io/v1","kind":"Ingress","metadata":{"annotations":{"mctc-component":"webhook"},"name":"mctc-ingress-mctc","namespace":"multi-cluster-traffic-controller-system"},"spec":{"rules":[{"host":"","http":{"paths":[{"backend":{"service":{"name":"mctc-webhooks","port":{"number":8082}}},"path":"/","pathType":"Prefix"}]}}]}}
    mctc-component: webhook
  creationTimestamp: "2023-01-26T15:42:08Z"
  generation: 1
  name: mctc-ingress-mctc
  namespace: multi-cluster-traffic-controller-system
  resourceVersion: "2235"
  uid: 7fb786bc-1816-4e3e-b6f6-1bf8cd45bd3e
spec:
  ingressClassName: nginx
  rules:
  - http:
      paths:
      - backend:
          service:
            name: mctc-webhooks
            port:
              number: 8082
        path: /
        pathType: Prefix
status:
  loadBalancer:
    ingress:
    - ip: 172.18.0.2

It looks like nginx is not allowing the echo ingress as it doesn't have a host and sees it as a wildcard?
While the mctc ingress is already set up as a wildcard host.

sergioifg94 · 2023-01-26T16:23:28Z

@david-martin That's right, I forgot to update the verification steps after a recent change. The reason for that is that I initially didn't use the empty host for the webhooks ingress, and the Nginx webhook validates that the empty host can only be used once. I think for now you can use any different host name for the echo ingress

sergioifg94 · 2023-01-27T14:30:11Z

main.go

@@ -107,7 +111,7 @@ func main() {
 		setupLog.Error(err, "unable to create controller", "controller", "DNSRecord")
 		os.Exit(1)
 	}
-	dnsService := dns.NewService(mgr.GetClient(), dns.NewDefaultHostResolver(), defaultCtrlNS)
+	dnsService := dns.NewService(mgr.GetClient(), dns.NewSafeHostResolver(dns.NewDefaultHostResolver()), defaultCtrlNS)


In case anyone wonders. The reason to wrap it with the SafeHostResolver is because it'll be shared both by the Controller and the Webhook server so it avoids concurrency issues

david-martin · 2023-01-30T15:01:30Z

@philbrookes @maleck13 I think we can land this now, if you can ack again.

@sergioifg94 Have you captured all followups?
I'm thinking:

webhook creation for each new cluster
what's the extent of potential problems with ingress admission hooks preventing creation e.g. wildcards not allowed by nginx

sergioifg94 · 2023-01-30T15:39:15Z

@david-martin Thank you, I pushed as a new commit the changes for the webhook creation for each new cluster. I think it all makes more sense now.
I also created that separate issue to investigate the conflict around the wildcards (#25), and I'll create a separate one to investigate potential conflicts with other traffic controllers

david-martin

Latest change verified locally with 2 workload clusters.

The webhook is created on all clusters:

2023-01-31T10:27:56Z	INFO	new cluster added 	{"name": "mctc-workload-1"}
2023-01-31T10:27:56Z	INFO	getting webhook configurations
2023-01-31T10:27:56Z	INFO	create/update validating webhooks
2023-01-31T10:27:56Z	INFO	create/update mutating webhooks
2023-01-31T10:27:56Z	INFO	new cluster added 	{"name": "mctc-workload-2"}
2023-01-31T10:27:56Z	INFO	getting webhook configurations
2023-01-31T10:27:56Z	INFO	create/update validating webhooks
2023-01-31T10:27:56Z	INFO	create/update mutating webhooks
2023-01-31T10:27:56Z	INFO	new cluster added 	{"name": "mctc-control-plane"}
2023-01-31T10:27:56Z	INFO	getting webhook configurations
2023-01-31T10:27:56Z	INFO	create/update validating webhooks
2023-01-31T10:27:56Z	INFO	create/update mutating webhooks

Webhook config reconciliation Co-Authored-By: Name sergioifg94 Co-Authored-By: Name R-Lawton

david-martin

Rebased

philbrookes

LGTM

sergioifg94 changed the title ~~[WIP] Hcg webhook~~ Webhooks setup and server Jan 23, 2023

sergioifg94 marked this pull request as ready for review January 23, 2023 16:24

philbrookes reviewed Jan 24, 2023

View reviewed changes

maleck13 reviewed Jan 24, 2023

View reviewed changes

pkg/admission/ingress/webhook_handler.go Outdated Show resolved Hide resolved

maleck13 reviewed Jan 24, 2023

View reviewed changes

pkg/admission/ingress/webhook_handler.go Outdated Show resolved Hide resolved

sergioifg94 force-pushed the HCG-webhook branch 2 times, most recently from 9e570e4 to 7f0c720 Compare January 26, 2023 16:01

R-Lawton force-pushed the HCG-webhook branch 2 times, most recently from 5a4e979 to c42f39f Compare January 27, 2023 13:19

sergioifg94 commented Jan 27, 2023

View reviewed changes

R-Lawton force-pushed the HCG-webhook branch from f43a683 to 2f4f10f Compare January 27, 2023 15:58

david-martin approved these changes Jan 30, 2023

View reviewed changes

david-martin approved these changes Jan 31, 2023

View reviewed changes

R-Lawton and others added 5 commits January 31, 2023 10:32

Webhook Server

cd479d2

Webhook config reconciliation Co-Authored-By: Name sergioifg94 Co-Authored-By: Name R-Lawton

Updating ingress with dummy host name and adding extra logs

f3ae0ed

handling errors

f741679

Webhook reconciliation in workload clusters

c2704da

Register control plane cluster with ArgoCD

cf4b2db

david-martin force-pushed the HCG-webhook branch from 19871c5 to cf4b2db Compare January 31, 2023 10:33

david-martin approved these changes Jan 31, 2023

View reviewed changes

philbrookes approved these changes Jan 31, 2023

View reviewed changes

sergioifg94 merged commit 86ba524 into Kuadrant:main Jan 31, 2023

This was referenced Feb 1, 2023

Fix webhook reconciliation when controller runs locally #31

Closed

Enable webhooks when running the controller locally #32

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Webhooks setup and server #9

Webhooks setup and server #9

sergioifg94 commented Jan 18, 2023 •

edited

philbrookes Jan 24, 2023

philbrookes Jan 24, 2023

sergioifg94 Jan 24, 2023

maleck13 Jan 24, 2023

philbrookes Jan 24, 2023

philbrookes Jan 24, 2023 •

edited

maleck13 Jan 24, 2023

philbrookes Jan 26, 2023

sergioifg94 Jan 26, 2023

philbrookes Jan 26, 2023

david-martin commented Jan 26, 2023 •

edited

sergioifg94 commented Jan 26, 2023

sergioifg94 Jan 27, 2023 •

edited

david-martin commented Jan 30, 2023

sergioifg94 commented Jan 30, 2023

david-martin left a comment

david-martin left a comment

philbrookes left a comment


		}

		func (h Handler) handle(ctx context.Context, ingress networkingv1.Ingress) (bool, error) {

Webhooks setup and server #9

Webhooks setup and server #9

Conversation

sergioifg94 commented Jan 18, 2023 • edited

Description

Verification steps

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

philbrookes Jan 24, 2023 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

david-martin commented Jan 26, 2023 • edited

sergioifg94 commented Jan 26, 2023

sergioifg94 Jan 27, 2023 • edited

Choose a reason for hiding this comment

david-martin commented Jan 30, 2023

sergioifg94 commented Jan 30, 2023

david-martin left a comment

Choose a reason for hiding this comment

david-martin left a comment

Choose a reason for hiding this comment

philbrookes left a comment

Choose a reason for hiding this comment

sergioifg94 commented Jan 18, 2023 •

edited

philbrookes Jan 24, 2023 •

edited

david-martin commented Jan 26, 2023 •

edited

sergioifg94 Jan 27, 2023 •

edited