Simplify CA Bundle injection for multus admission webhook #761

alexanderConstantinescu · 2020-08-20T10:49:48Z

Preamble:

Coming soon will be a validating admission webhook for ovn-kubernetes. Leading to that: I investigated the different mechanisms used in OpenShift and in our CNO to generate certs/ca-bundles, realizing that nothing generates these in the same way. Multus has one way of doing it, Kuryr another, OVN another...deciding to focus on multus I however realized that multus was severely over-complicating things.

The mechanism to inject the CA bundle into its webhook was:

Use multus-admission-controller/004-configmap.yaml to generate a configmap (openshift-network-operator/openshift-service-ca) with the annotation service.beta.openshift.io/inject-cabundle: "true" which is reconciled by the ServiceCA to inject the proper CA bundle.
Reconcile that reconciled config map ourselves to read the injected value, and inject that value into the webhook.

In fact, the ServiceCA reconciles any webhook with the annotation service.beta.openshift.io/inject-cabundle: "true" and injects the CA-bundle into the webhook directly.

This patch thus removes one operator in the CNO, one config map and consolidates the webhook generation according to a more "standard" procedure for OpenShift.

/assign @dougbtv

Signed-off-by: Alexander Constantinescu <aconstan@redhat.com>

juanluisvaladas · 2020-08-20T15:16:30Z

I've been checking this and I don't see where the openshift-service-ca is used at all, I can't find a single reference to it in the multus admission controller. Do we still need this configmap to be created? I think we may be able to also remove the annotation.

@s1061123 can you PTAL?

alexanderConstantinescu · 2020-08-20T15:29:45Z

I've been checking this and I don't see where the openshift-service-ca is used at all

It's created here:

https://github.com/openshift/cluster-network-operator/blob/master/bindata/network/multus-admission-controller/004-configmap.yaml#L5

And reconciled here:

https://github.com/openshift/cluster-network-operator/blob/master/pkg/controller/operconfig/configmap_controller.go#L71

Do we still need this configmap to be created?

No, which is why I removed it

I think we may be able to also remove the annotation.

I removed the entire config map, which annotation am I missing?

juanluisvaladas · 2020-08-20T16:25:16Z

Ah, you're right, I was refering to the annotation service.beta.openshift.io/inject-cabundle but this is in mutation webhook now, I didn't know this could be used outside a configmap.
/lgtm

danwinship · 2020-08-20T19:41:47Z

I don't think we have any e2e of the multus admission webhook (grumble grumble) so this will need to be manually tested.

I don't really know any of this code so I'm not sure, but it looks like a nice simplification if it does indeed work.

alexanderConstantinescu · 2020-08-20T20:55:50Z

I don't think we have any e2e of the multus admission webhook (grumble grumble) so this will need to be manually tested.

I don't really know any of this code so I'm not sure, but it looks like a nice simplification if it does indeed work.

I tested that the admission control webhook got the proper CA bundle injected by the ServiceCA, earlier today. So I am sure that works.

I will test that it properly works and validates any network-attachment-definitions next week when I am back. Unless @s1061123 / @dougbtv knows if such objects are created during a CI run? Which would sort that out already, given that some CI jobs have already passed.

alexanderConstantinescu · 2020-08-26T08:10:04Z

FYI, using https://github.com/openshift/multus-admission-controller#example-of-the-admission-controller-in-action I just validated that it works:

aconstan@linux ~ $ cat tmp
apiVersion: "k8s.cni.cncf.io/v1"
kind: NetworkAttachmentDefinition
metadata:
  name: bad-conf
spec:
  config: '{
      "cniVersion": "0.3.0"malFormattedJSON
      }
    }'
aconstan@linux ~ $ oc create -f tmp
Error from server: error when creating "tmp": admission webhook "multus-validating-config.k8s.io" denied the request: configuration string is not in JSON format
aconstan@linux ~ $ vi tmp
aconstan@linux ~ $ cat tmp
apiVersion: "k8s.cni.cncf.io/v1"
kind: NetworkAttachmentDefinition
metadata:
  name: macvlan-conf
spec:
  config: '{
      "cniVersion": "0.3.0",
      "type": "macvlan",
      "master": "eth0",
      "mode": "bridge",
      "ipam": {
        "type": "host-local",
        "subnet": "192.168.1.0/24",
        "rangeStart": "192.168.1.200",
        "rangeEnd": "192.168.1.216",
        "routes": [
          { "dst": "0.0.0.0/0" }
        ],
        "gateway": "192.168.1.1"
      }
    }'
aconstan@linux ~ $ oc create -f tmp
networkattachmentdefinition.k8s.cni.cncf.io/macvlan-conf created
aconstan@linux ~ $ oc get clusterversion
NAME      VERSION                                           AVAILABLE   PROGRESSING   SINCE   STATUS
version   4.6.0-0.ci.test-2020-08-26-065839-ci-ln-1yvm47t   True        False         17m     Cluster version is 4.6.0-0.ci.test-2020-08-26-065839-ci-ln-1yvm47t

The message configuration string is not in JSON format comes from here: https://github.com/openshift/multus-admission-controller/blob/8c0ba31c0beebcd72d9d57513c0b47a2b6a545cf/pkg/webhook/webhook.go#L124

danwinship · 2020-08-26T17:57:35Z

/lgtm cancel
/approve
@dougbtv can you /lgtm this if it makes sense to you?

danwinship · 2020-08-26T18:01:41Z

/retest

alexanderConstantinescu · 2020-09-15T08:22:30Z

@dougbtv, could you please take a look at this PR and let us know if what you think?

alexanderConstantinescu · 2020-10-19T16:16:42Z

@dougbtv : PTAL

dougbtv · 2020-10-21T15:09:19Z

/lgtm

openshift-ci-robot · 2020-10-21T15:10:36Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: alexanderConstantinescu, danwinship, dougbtv, juanluisvaladas

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~~ [danwinship]

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

openshift-bot · 2020-10-21T16:25:24Z

/retest

Please review the full test history for this PR and help us cut down flakes.

openshift-bot · 2020-10-21T16:51:27Z

/retest

Please review the full test history for this PR and help us cut down flakes.

openshift-bot · 2020-10-21T18:22:26Z

/retest

Please review the full test history for this PR and help us cut down flakes.

openshift-bot · 2020-10-21T18:48:27Z

/retest

Please review the full test history for this PR and help us cut down flakes.

openshift-bot · 2020-10-21T19:01:27Z

/retest

Please review the full test history for this PR and help us cut down flakes.

openshift-bot · 2020-10-21T20:45:25Z

/retest

Please review the full test history for this PR and help us cut down flakes.

openshift-bot · 2020-10-21T21:11:29Z

/retest

Please review the full test history for this PR and help us cut down flakes.

openshift-bot · 2020-10-26T11:19:59Z

/retest

Please review the full test history for this PR and help us cut down flakes.

danwinship · 2020-10-26T13:28:56Z

/override ci/prow/e2e-aws-sdn-multi

openshift-ci-robot · 2020-10-26T13:29:33Z

@danwinship: Overrode contexts on behalf of danwinship: ci/prow/e2e-aws-sdn-multi

In response to this:

/override ci/prow/e2e-aws-sdn-multi

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.

openshift-bot · 2020-10-26T13:55:52Z

/retest

Please review the full test history for this PR and help us cut down flakes.

openshift-bot · 2020-10-26T23:02:00Z

/retest

Please review the full test history for this PR and help us cut down flakes.

openshift-bot · 2020-10-26T23:14:54Z

/retest

Please review the full test history for this PR and help us cut down flakes.

openshift-bot · 2020-10-27T04:13:54Z

/retest

Please review the full test history for this PR and help us cut down flakes.

openshift-bot · 2020-10-27T09:38:54Z

/retest

Please review the full test history for this PR and help us cut down flakes.

openshift-bot · 2020-10-27T09:51:55Z

/retest

Please review the full test history for this PR and help us cut down flakes.

openshift-bot · 2020-10-27T12:01:55Z

/retest

Please review the full test history for this PR and help us cut down flakes.

openshift-bot · 2020-10-27T12:40:53Z

/retest

Please review the full test history for this PR and help us cut down flakes.

openshift-bot · 2020-10-27T14:50:55Z

/retest

Please review the full test history for this PR and help us cut down flakes.

openshift-bot · 2020-10-27T15:03:54Z

/retest

Please review the full test history for this PR and help us cut down flakes.

openshift-bot · 2020-10-27T18:06:03Z

/retest

Please review the full test history for this PR and help us cut down flakes.

openshift-bot · 2020-10-27T18:18:56Z

/retest

Please review the full test history for this PR and help us cut down flakes.

openshift-bot · 2020-10-27T21:07:28Z

/retest

Please review the full test history for this PR and help us cut down flakes.

openshift-bot · 2020-10-28T00:22:12Z

/retest

Please review the full test history for this PR and help us cut down flakes.

openshift-bot · 2020-10-28T01:01:10Z

/retest

Please review the full test history for this PR and help us cut down flakes.

openshift-bot · 2020-10-28T03:37:15Z

/retest

Please review the full test history for this PR and help us cut down flakes.

openshift-bot · 2020-10-28T03:50:10Z

/retest

Please review the full test history for this PR and help us cut down flakes.

openshift-bot · 2020-10-28T06:00:17Z

/retest

Please review the full test history for this PR and help us cut down flakes.

openshift-bot · 2020-10-28T06:26:11Z

/retest

Please review the full test history for this PR and help us cut down flakes.

openshift-bot · 2020-10-28T09:28:06Z

/retest

Please review the full test history for this PR and help us cut down flakes.

danwinship · 2020-10-28T13:15:38Z

/override ci/prow/e2e-aws-sdn-multi

openshift-ci-robot · 2020-10-28T13:16:21Z

@danwinship: Overrode contexts on behalf of danwinship: ci/prow/e2e-aws-sdn-multi

In response to this:

/override ci/prow/e2e-aws-sdn-multi

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.

openshift-bot · 2020-10-28T14:27:47Z

/retest

Please review the full test history for this PR and help us cut down flakes.

openshift-bot · 2020-10-28T14:39:49Z

/retest

Please review the full test history for this PR and help us cut down flakes.

openshift-bot · 2020-10-28T16:36:52Z

/retest

Please review the full test history for this PR and help us cut down flakes.

openshift-bot · 2020-10-28T19:12:49Z

/retest

Please review the full test history for this PR and help us cut down flakes.

openshift-ci-robot assigned dougbtv Aug 20, 2020

openshift-ci-robot requested review from juanluisvaladas and tssurya August 20, 2020 10:50

Simplify CA Bundle injection for multus admission webhook

3a2d240

Signed-off-by: Alexander Constantinescu <aconstan@redhat.com>

alexanderConstantinescu force-pushed the simplify-ca-bundle-injection branch from b8a875c to 3a2d240 Compare August 20, 2020 10:55

openshift-ci-robot assigned juanluisvaladas Aug 20, 2020

openshift-ci-robot added the lgtm Indicates that a PR is ready to be merged. label Aug 20, 2020

openshift-ci-robot assigned danwinship Aug 26, 2020

openshift-ci-robot added approved Indicates a PR has been approved by an approver from all required OWNERS files. and removed lgtm Indicates that a PR is ready to be merged. labels Aug 26, 2020

openshift-ci-robot added the lgtm Indicates that a PR is ready to be merged. label Oct 21, 2020

openshift-merge-robot merged commit 5483fd3 into openshift:master Oct 28, 2020

Simplify CA Bundle injection for multus admission webhook #761

Simplify CA Bundle injection for multus admission webhook #761

Conversation

alexanderConstantinescu commented Aug 20, 2020 • edited

juanluisvaladas commented Aug 20, 2020

alexanderConstantinescu commented Aug 20, 2020

juanluisvaladas commented Aug 20, 2020

danwinship commented Aug 20, 2020

alexanderConstantinescu commented Aug 20, 2020

alexanderConstantinescu commented Aug 26, 2020

danwinship commented Aug 26, 2020

danwinship commented Aug 26, 2020

alexanderConstantinescu commented Sep 15, 2020

alexanderConstantinescu commented Oct 19, 2020

dougbtv commented Oct 21, 2020

openshift-ci-robot commented Oct 21, 2020

openshift-bot commented Oct 21, 2020

openshift-bot commented Oct 21, 2020

openshift-bot commented Oct 21, 2020

openshift-bot commented Oct 21, 2020

openshift-bot commented Oct 21, 2020

openshift-bot commented Oct 21, 2020

openshift-bot commented Oct 21, 2020

openshift-bot commented Oct 26, 2020

danwinship commented Oct 26, 2020

openshift-ci-robot commented Oct 26, 2020

openshift-bot commented Oct 26, 2020

openshift-bot commented Oct 26, 2020

openshift-bot commented Oct 26, 2020

openshift-bot commented Oct 27, 2020

openshift-bot commented Oct 27, 2020

openshift-bot commented Oct 27, 2020

openshift-bot commented Oct 27, 2020

openshift-bot commented Oct 27, 2020

openshift-bot commented Oct 27, 2020

openshift-bot commented Oct 27, 2020

openshift-bot commented Oct 27, 2020

openshift-bot commented Oct 27, 2020

openshift-bot commented Oct 27, 2020

openshift-bot commented Oct 28, 2020

openshift-bot commented Oct 28, 2020

openshift-bot commented Oct 28, 2020

openshift-bot commented Oct 28, 2020

openshift-bot commented Oct 28, 2020

openshift-bot commented Oct 28, 2020

openshift-bot commented Oct 28, 2020

danwinship commented Oct 28, 2020

openshift-ci-robot commented Oct 28, 2020

openshift-bot commented Oct 28, 2020

openshift-bot commented Oct 28, 2020

openshift-bot commented Oct 28, 2020

openshift-bot commented Oct 28, 2020

alexanderConstantinescu commented Aug 20, 2020 •

edited