Use Webhook API to v1

[anishj0shi]

Proposed Changes

• use v1 apis for ValidatingWebhookConfiguration and MutatingWebhookConfiguration

Also see:
knative/pkg#1444,
knative/serving#8461

Release Note

Eventing Webhooks now use apiextensions.k8s.io/v1 APIs. to ensure smooth upgradability user has to upgrade eventing  from eventing version 0.15.2. Or else, Upgrade might have to be attempted twice, which might have some ramifications such as webhook downtime.

[lberk]

/lgtm

[anishj0shi]

/test pull-knative-eventing-upgrade-tests

[vaikas]

/lgtm
/approve

[matzew]

this looks good!

/lgtm
/approve

But, I guess the tests are failing, since the upgrade is IMO wrong.

[knative-prow-robot]

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: anishj0shi, matzew, vaikas

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 [matzew,vaikas]

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

[anishj0shi]

/test pull-knative-eventing-upgrade-tests

[anishj0shi]

@matzew any clue on how can I fix these upgrade tests?

[matzew]

I've not check the logs, sorry

[matzew]

or: "STDIN": Internal error occurred: failed calling webhook "config.webhook.eventing.knative.dev": expected webhook response of admission.k8s.io/v1, Kind=AdmissionReview, got /, Kind=
Error from server (InternalError): error when applying patch:
{"metadata":{"annotations":{"kubectl.kubernetes.io/last-applied-configuration":"{\"apiVersion\":\"v1\",\"data\":{\"loglevel.controller\":\"info\",\"loglevel.webhook\":\"info\",\"zap-logger-config\":\"{\\n  \\\"level\\\": \\\"info\\\",\\n  \\\"development\\\": false,\\n  \\\"outputPaths\\\": [\\\"stdout\\\"],\\n  \\\"errorOutputPaths\\\": [\\\"stderr\\\"],\\n  \\\"encoding\\\": \\\"json\\\",\\n  \\\"encoderConfig\\\": {\\n    \\\"timeKey\\\": \\\"ts\\\",\\n    \\\"levelKey\\\": \\\"level\\\",\\n    \\\"nameKey\\\": \\\"logger\\\",\\n    \\\"callerKey\\\": \\\"caller\\\",\\n    \\\"messageKey\\\": \\\"msg\\\",\\n    \\\"stacktraceKey\\\": \\\"stacktrace\\\",\\n    \\\"lineEnding\\\": \\\"\\\",\\n    \\\"levelEncoder\\\": \\\"\\\",\\n    \\\"timeEncoder\\\": \\\"iso8601\\\",\\n    \\\"durationEncoder\\\": \\\"\\\",\\n    \\\"callerEncoder\\\": \\\"\\\"\\n  }\\n}\\n\"},\"kind\":\"ConfigMap\",\"metadata\":{\"annotations\":{},\"labels\":{\"eventing.knative.dev/release\":\"devel\",\"knative.dev/config-category\":\"eventing\",\"knative.dev/config-propagation\":\"original\"},\"name\":\"config-logging\",\"namespace\":\"knative-eventing\"}}\n"},"labels":{"eventing.knative.dev/release":"devel"}}}
to:

I guess, something wrong, with applying this in the upgrade phase ? Perhaps worth to run some of that locally for further debugging ?

[dprotaso]

pull in the latest knative.dev/pkg

[dprotaso]

Hm.. it's actually there

eventing/vendor/knative.dev/pkg/webhook/admission.go

Lines 92 to 95 in baaaeff

	// Use the same type meta as the request - this is required by the K8s API
	// note: v1beta1 & v1 AdmissionReview shapes are identical so even though
	// we're using v1 types we still support v1beta1 admission requests
	TypeMeta: review.TypeMeta,

[dprotaso]

I wonder if there's a race where the new webhook k8s deployment should be deployed prior to the webhook configurations

[dprotaso]

/retest

[anishj0shi]

I wonder if there's a race where the new webhook k8s deployment should be deployed prior to the webhook configurations

we can check this, by changing the prefix numbers of CRDs in config/ dir, right?

[dprotaso]

I'm just surprised knative/serving didn't encounter this

[dprotaso]

serving has the webhook.yaml at the bottom of that directory - In my mind that should trigger what you're seeing here.

[dprotaso]

/hold

Going forward

1. Need to figure out why serving's upgrade test didn't catch this
2. Pull the webhook fix in knative.dev/pkg to older releases branches and cut a few point releases next Tuesday.

#2 will unblock this PR but it will require users to upgrade to that point release prior to upgrading

Another option is to delay merging this PR until the release after next (ie. skip 0.16 and ship in 0.17). But then I would need to rollback the serving change as well.

[matzew]

Another option is to delay merging this PR until the release after next

(ie. skip 0.16 and ship in 0.17). But then I would need to rollback the serving change as well. shipping in 0.17 seems like a good thing to me

On Thu 25. Jun 2020 at 19:00, Dave Protasowski ***@***.***> wrote: /hold Going forward 1. Need to figure out why serving's upgrade test didn't catch this 2. Pull the webhook fix in knative.dev/pkg to older releases branches and cut a few point releases next Tuesday. #2 <#2> will unblock this PR but it will require users to upgrade to that point release prior to upgrading Another option is to delay merging this PR until the release after next (ie. skip 0.16 and ship in 0.17). But then I would need to rollback the serving change as well. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#3409 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AABGPTUSGLGR2I5ADUJK773RYN7CLANCNFSM4OHKWWWQ> .

-- Sent from Gmail Mobile

[dprotaso]

Technically conversion web-hooks are also affected - though not all requests need to be converted.

I cherry-picked the right changes here: #3416

So as long as end-users upgrade to the point release that comes out on Tuesday they should be able to smoothly update to 0.16

[grantr]

Thanks @dprotaso for explaining the situation to me on slack. Here's my current understanding:

#3416 will be part of 0.15.2 which will be compatible with v1 webhook api and enable a smooth upgrade.

Anyone upgrading from a pre-0.15.2 version may need to attempt the upgrade twice, and there may be some webhook downtime during the upgrade. But that can be avoided by upgrading to 0.15.2 first. That seems fine to me.

@anishj0shi can you add a release note to this PR explaining the upgrade situation?

[dprotaso]

Note: we'll have to wait for the point release for the pull-knative-eventing-upgrade-tests to pass :)

[dprotaso]

New point release is out
/test pull-knative-eventing-upgrade-tests

[dprotaso]

weird it didn't pick up the point release

Installing Knative Eventing from: https://github.com/knative/eventing/releases/download/v0.15.0/eventing.yaml

[dprotaso]

This seems wrong

eventing/test/e2e-common.sh

Lines 54 to 56 in e1e67cf

	# Latest release. If user does not supply this as a flag, the latest
	# tagged release on the current branch will be used.
	readonly LATEST_RELEASE_VERSION=$(git describe --match "v[0-9]*" --abbrev=0)

with it's use here:

eventing/test/e2e-common.sh

Line 106 in e1e67cf

local url="https://github.com/knative/eventing/releases/download/${LATEST_RELEASE_VERSION}"

[knative-test-reporter-robot]

The following jobs failed:

Test name	Triggers	Retries
pull-knative-eventing-upgrade-tests		0/3

Failed non-flaky tests preventing automatic retry of pull-knative-eventing-upgrade-tests:

test/upgrade.TestContinuousEventsPropagationWithProber

[vaikas]

/test pull-knative-eventing-upgrade-tests

[dprotaso]

/hold cancel

[dprotaso]

/lgtm

[dprotaso]

/hold

I forgot we should update the release notes

[anishj0shi]

@dprotaso @grantr
Added Release Notes to the PR :)

[matzew]

/lgtm

[anishj0shi]

/unhold

[dprotaso]

Awesome thanks for your patience @anishj0shi while we sorted out the kinks