🌱 Update dependencies to v1.19.2 #501

vincepri · 2020-10-07T20:45:06Z

Signed-off-by: Vince Prignano vincepri@vmware.com

k8s-ci-robot · 2020-10-07T20:45:19Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: vincepri

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

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

vincepri · 2020-10-07T21:10:53Z

.run-controller-gen.sh

+set -o errexit
+set -o nounset
+set -o pipefail
+
+readlink=$(command -v readlink)

-cd $(dirname $(readlink -f ${BASH_SOURCE[0]}))
+check_readlink() {
+    if ! ${readlink} -f &>/dev/null; then
+        if [[ "${OSTYPE}" == "darwin"* ]]; then
+            if command -v greadlink; then
+                readlink=$(command -v greadlink)
+                return
+            fi
+        fi
+        echo "you're probably on OSX.  Please install gnu readlink -- otherwise you're missing the most useful readlink flag."
+        exit 1
+    fi
+}
+current_dir=$(pwd)
+check_readlink
+cd $(dirname $(${readlink} -f ${BASH_SOURCE[0]}))
 go run -v -exec "./.run-in.sh ${current_dir} " ./cmd/controller-gen $@


@DirectXMan12 Made some changes to support gnu readlink on OSX when it's not linked (usually installed with brew install coreutils)

vincepri · 2020-10-07T21:15:37Z

go.mod

+	k8s.io/api v0.19.2
+	k8s.io/apiextensions-apiserver v0.19.2
+	k8s.io/apimachinery v0.19.2


@DirectXMan12 It seems that a test is now failing after updating to v0.19.2

Expected success, but got an error: <*errors.errorString | 0xc0000b8ed0>: { s: "converting (v1.MutatingWebhookConfiguration) to (v1beta1.MutatingWebhookConfiguration): unknown conversion", } converting (v1.MutatingWebhookConfiguration) to (v1beta1.MutatingWebhookConfiguration): unknown conversion

From 1.19 changelog:

K8s.io/apimachinery - scheme.Convert() now uses only explicitly registered conversions - default reflection based conversion is no longer available. +k8s:conversion-gen tags can be used with the k8s.io/code-generator component to generate conversions.

Change came in: kubernetes/kubernetes#90018

It seems we're relying on scheme.Convert to convert webhooks, and I can't seem to find where/how to register the conversion functions in the admission APIs 🤔, I might be missing something here, although just registering the types with AddToScheme in https://github.com/kubernetes-sigs/controller-tools/blob/master/pkg/webhook/conv.go#L27-L33 doesn't seem enough for the conversions to work.

cc @wojtek-t @liggitt

Conversions from v1 directly to v1beta1 AdmissionReview objects are not defined, even internally in kube-apiserver.

The API server would convert from one to the other by way of the internal (unversioned) AdmissionReview type:

v1beta1 -> internal

internal -> v1

This conversions and the internal AdmissionReview type are registered in https://github.com/kubernetes/kubernetes/blob/master/pkg/apis/admission/install/install.go but are not exported for use outside the kube-apiserver

The reflect-based conversion controller-tools was depending on previously was closer to decoding v1beta1 JSON into the v1 struct and hoping all the field names stayed the same.

The reflect-based conversion controller-tools was depending on previously was closer to decoding v1beta1 JSON into the v1 struct and hoping all the field names stayed the same.

It looks like this happens to be the case:

diff staging/src/k8s.io/api/admission/{v1,v1beta1}/types.go

2c2 < Copyright 2019 The Kubernetes Authors. --- > Copyright 2017 The Kubernetes Authors. 17c17 < package v1 --- > package v1beta1 26a27,31 > // +k8s:prerelease-lifecycle-gen:introduced=1.9 > // +k8s:prerelease-lifecycle-gen:deprecated=1.19 > // This API is never server served. It is used for outbound requests from apiservers. This will ensure it never gets served accidentally > // and having the generator against this group will protect future APIs which may be served. > // +k8s:prerelease-lifecycle-gen:replacement=admission.k8s.io,v1,AdmissionReview 118c123 < // This must be copied over from the corresponding AdmissionRequest. --- > // This should be copied over from the corresponding AdmissionRequest.

but the fact that the structs happened to be identical between versions was luck, and doesn't mean this is a correct way to convert between them

Thanks Jordan, it seems the removal of the default reflection-based conversion was definitely a good step, things might have failed silently. For this codebase, I think the best way is to add support for both types, or deprecate one, or write our own conversion functions. @DirectXMan12 @alvaroaleman what do you all think?

Why do we have a need to convert? APIServers should still serve both or not?

AdmissionReview is not a served API, it is the API sent by the API server to admission webhooks and expected in response.

I'm not sure why this component is converting... is it trying to unify v1 and v1beta1 AdmissionReview handling to a single type?

IIRC, we have a single interface that represents "webhook" that can interact with whatever the hook sends, and we autoconvert from whatever the server sends to the version the interface uses.

Part of it was simplifying the writing of the webhook while still giving folks access to the underlying information. We should be able to paper over it, I think, but I haven't looked at this bit in a while.

estroz · 2020-12-15T09:48:30Z

@vincepri this can be updated now.

vincepri · 2020-12-15T16:02:22Z

@estroz Rebased on top of the latest changes, although we still need to apply the same patch we did in Controller Runtime here

Webhook Generation From Parsing to CustomResourceDefinition
/Users/vince/go/src/sigs.k8s.io/controller-tools/pkg/webhook/parser_integration_test.go:38
  should properly generate the webhook definition [It]
  /Users/vince/go/src/sigs.k8s.io/controller-tools/pkg/webhook/parser_integration_test.go:43

  Expected success, but got an error:
      <*errors.errorString | 0xc00031adb0>: {
          s: "converting (v1.MutatingWebhookConfiguration) to (v1beta1.MutatingWebhookConfiguration): unknown conversion",
      }
      converting (v1.MutatingWebhookConfiguration) to (v1beta1.MutatingWebhookConfiguration): unknown conversion

  /Users/vince/go/src/sigs.k8s.io/controller-tools/pkg/webhook/parser_integration_test.go:70

Signed-off-by: Vince Prignano <vincepri@vmware.com>

estroz · 2020-12-15T19:28:25Z

.run-controller-gen.sh


-cd $(dirname $(readlink -f ${BASH_SOURCE[0]}))
+check_readlink() {
+    if ! ${readlink} -f &>/dev/null; then


This will always fail with no file argument.

$ uname -o GNU/Linux $ readlink -f readlink: missing operand Try 'readlink --help' for more information.

To fix:

Suggested change

if ! ${readlink} -f &>/dev/null; then

local test_file="$(mktemp)"

trap "rm -f $test_file" EXIT

if ! ${readlink} -f "$test_file" &>/dev/null; then

tamalsaha · 2020-12-26T18:41:45Z

Could this be upgraded to 1.120 instead since controller-runtime is already using 1.20 ? kubernetes-sigs/controller-runtime#1268

vincepri · 2021-01-04T06:50:31Z

We should probably release two versions, one for 1.19 and one for 1.20

vincepri · 2021-01-04T18:26:31Z

@estroz Do you have some time to finish up these changes?

k8s-ci-robot added the cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. label Oct 7, 2020

k8s-ci-robot requested review from mengqiy and pwittrock October 7, 2020 20:45

k8s-ci-robot added approved Indicates a PR has been approved by an approver from all required OWNERS files. size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Oct 7, 2020

vincepri force-pushed the update-deps branch from 98d499e to d01e243 Compare October 7, 2020 21:09

k8s-ci-robot added size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. and removed size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Oct 7, 2020

vincepri commented Oct 7, 2020

View reviewed changes

k8s-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Nov 10, 2020

vincepri mentioned this pull request Nov 16, 2020

Cut new release #453

Closed

Update dependencies to v1.19.2

3e2063f

Signed-off-by: Vince Prignano <vincepri@vmware.com>

vincepri force-pushed the update-deps branch from d01e243 to 3e2063f Compare December 15, 2020 16:02

k8s-ci-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Dec 15, 2020

estroz reviewed Dec 15, 2020

View reviewed changes

estroz mentioned this pull request Jan 5, 2021

⚠️ update dependencies to v1.19.2 #534

Merged

k8s-ci-robot closed this in #534 Jan 5, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

🌱 Update dependencies to v1.19.2 #501

🌱 Update dependencies to v1.19.2 #501

vincepri commented Oct 7, 2020

k8s-ci-robot commented Oct 7, 2020

vincepri Oct 7, 2020

vincepri Oct 7, 2020

liggitt Oct 8, 2020

liggitt Oct 8, 2020

vincepri Oct 8, 2020

alvaroaleman Oct 8, 2020

liggitt Oct 8, 2020

DirectXMan12 Oct 13, 2020

estroz commented Dec 15, 2020

vincepri commented Dec 15, 2020

estroz Dec 15, 2020 •

edited

tamalsaha commented Dec 26, 2020 •

edited

vincepri commented Jan 4, 2021

vincepri commented Jan 4, 2021

-    if ! ${readlink} -f &>/dev/null; then
+    local test_file="$(mktemp)"
+    trap "rm -f $test_file" EXIT
+    if ! ${readlink} -f "$test_file" &>/dev/null; then

🌱 Update dependencies to v1.19.2 #501

🌱 Update dependencies to v1.19.2 #501

Conversation

vincepri commented Oct 7, 2020

k8s-ci-robot commented Oct 7, 2020

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

estroz commented Dec 15, 2020

vincepri commented Dec 15, 2020

estroz Dec 15, 2020 • edited

Choose a reason for hiding this comment

tamalsaha commented Dec 26, 2020 • edited

vincepri commented Jan 4, 2021

vincepri commented Jan 4, 2021

estroz Dec 15, 2020 •

edited

tamalsaha commented Dec 26, 2020 •

edited