Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

🌱 Update dependencies to v1.19.2 #501

Closed
wants to merge 1 commit into from
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
26 changes: 20 additions & 6 deletions .run-controller-gen.sh
Expand Up @@ -4,11 +4,25 @@
# it's the equivalent of `go run sigs.k8s.io/controller-tools/cmd/controller-gen`
# if you could somehow do that without modifying your go.mod.

current_dir=$(pwd)
if ! readlink -f . &>/dev/null; then
echo "you're probably on OSX. Please install gnu readlink -- otherwise you're missing the most useful readlink flag."
exit 1
fi
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
Copy link
Contributor

@estroz estroz Dec 15, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

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 $@
Comment on lines +7 to 28
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

22 changes: 11 additions & 11 deletions go.mod
@@ -1,20 +1,20 @@
module sigs.k8s.io/controller-tools

go 1.13
go 1.15

require (
github.com/fatih/color v1.7.0
github.com/fatih/color v1.9.0
github.com/gobuffalo/flect v0.2.2
github.com/google/go-cmp v0.3.0
github.com/mattn/go-colorable v0.1.2 // indirect
github.com/onsi/ginkgo v1.11.0
github.com/onsi/gomega v1.8.1
github.com/google/go-cmp v0.5.2
github.com/mattn/go-colorable v0.1.8 // indirect
github.com/onsi/ginkgo v1.14.1
github.com/onsi/gomega v1.10.2
github.com/spf13/cobra v1.0.0
github.com/spf13/pflag v1.0.5
golang.org/x/tools v0.0.0-20200616195046-dc31b401abb5
gopkg.in/yaml.v3 v3.0.0-20190905181640-827449938966
k8s.io/api v0.18.2
k8s.io/apiextensions-apiserver v0.18.2
k8s.io/apimachinery v0.18.2
golang.org/x/tools v0.0.0-20201007032633-0806396f153e
gopkg.in/yaml.v3 v3.0.0-20200615113413-eeeca48fe776
k8s.io/api v0.19.2
k8s.io/apiextensions-apiserver v0.19.2
k8s.io/apimachinery v0.19.2
Comment on lines +16 to +18
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@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

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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:

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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

sigs.k8s.io/yaml v1.2.0
)
340 changes: 218 additions & 122 deletions go.sum

Large diffs are not rendered by default.

6 changes: 3 additions & 3 deletions pkg/crd/gen_integration_test.go
Expand Up @@ -22,6 +22,7 @@ import (
"io/ioutil"
"os"

"github.com/google/go-cmp/cmp"
. "github.com/onsi/ginkgo"
. "github.com/onsi/gomega"
"sigs.k8s.io/controller-tools/pkg/crd"
Expand Down Expand Up @@ -72,7 +73,7 @@ var _ = Describe("CRD Generation proper defaulting", func() {
Expect(err).NotTo(HaveOccurred())

By("comparing the two")
Expect(out.buf.Bytes()).To(Equal(expectedFile))
Expect(out.buf.String()).To(Equal(string(expectedFile)), cmp.Diff(out.buf.String(), string(expectedFile)))

})

Expand All @@ -88,8 +89,7 @@ var _ = Describe("CRD Generation proper defaulting", func() {
Expect(err).NotTo(HaveOccurred())

By("comparing the two")
Expect(out.buf.Bytes()).To(Equal(expectedFile))

Expect(out.buf.String()).To(Equal(string(expectedFile)), cmp.Diff(out.buf.String(), string(expectedFile)))
})
})

Expand Down
11 changes: 3 additions & 8 deletions pkg/crd/testdata/gen/foo_crd_v1.yaml
Expand Up @@ -21,23 +21,18 @@ spec:
openAPIV3Schema:
properties:
apiVersion:
description: 'APIVersion defines the versioned schema of this representation
of an object. Servers should convert recognized schemas to the latest
internal value, and may reject unrecognized values. More info: https://git.k8s.io/community/contributors/devel/api-conventions.md#resources'
description: 'APIVersion defines the versioned schema of this representation of an object. Servers should convert recognized schemas to the latest internal value, and may reject unrecognized values. More info: https://git.k8s.io/community/contributors/devel/api-conventions.md#resources'
type: string
kind:
description: 'Kind is a string value representing the REST resource this
object represents. Servers may infer this from the endpoint the client
submits requests to. Cannot be updated. In CamelCase. More info: https://git.k8s.io/community/contributors/devel/api-conventions.md#types-kinds'
description: 'Kind is a string value representing the REST resource this object represents. Servers may infer this from the endpoint the client submits requests to. Cannot be updated. In CamelCase. More info: https://git.k8s.io/community/contributors/devel/api-conventions.md#types-kinds'
type: string
metadata:
type: object
spec:
properties:
defaultedString:
default: fooDefaultString
description: This tests that defaulted fields are stripped for v1beta1,
but not for v1
description: This tests that defaulted fields are stripped for v1beta1, but not for v1
type: string
required:
- defaultedString
Expand Down
11 changes: 3 additions & 8 deletions pkg/crd/testdata/gen/foo_crd_v1beta1.yaml
Expand Up @@ -19,22 +19,17 @@ spec:
openAPIV3Schema:
properties:
apiVersion:
description: 'APIVersion defines the versioned schema of this representation
of an object. Servers should convert recognized schemas to the latest
internal value, and may reject unrecognized values. More info: https://git.k8s.io/community/contributors/devel/api-conventions.md#resources'
description: 'APIVersion defines the versioned schema of this representation of an object. Servers should convert recognized schemas to the latest internal value, and may reject unrecognized values. More info: https://git.k8s.io/community/contributors/devel/api-conventions.md#resources'
type: string
kind:
description: 'Kind is a string value representing the REST resource this
object represents. Servers may infer this from the endpoint the client
submits requests to. Cannot be updated. In CamelCase. More info: https://git.k8s.io/community/contributors/devel/api-conventions.md#types-kinds'
description: 'Kind is a string value representing the REST resource this object represents. Servers may infer this from the endpoint the client submits requests to. Cannot be updated. In CamelCase. More info: https://git.k8s.io/community/contributors/devel/api-conventions.md#types-kinds'
type: string
metadata:
type: object
spec:
properties:
defaultedString:
description: This tests that defaulted fields are stripped for v1beta1,
but not for v1
description: This tests that defaulted fields are stripped for v1beta1, but not for v1
type: string
required:
- defaultedString
Expand Down
60 changes: 28 additions & 32 deletions pkg/schemapatcher/testdata/expected/kubebuilder-example-crd.v1.yaml
Expand Up @@ -11,35 +11,31 @@ spec:
plural: examples
listKind: ExampleList
versions:
- name: v1
schema:
openAPIV3Schema:
description: Example is a kind with schema changes.
type: object
required:
- spec
properties:
apiVersion:
description: 'APIVersion defines the versioned schema of this representation
of an object. Servers should convert recognized schemas to the latest
internal value, and may reject unrecognized values. More info: https://git.k8s.io/community/contributors/devel/sig-architecture/api-conventions.md#resources'
type: string
kind:
description: 'Kind is a string value representing the REST resource this
object represents. Servers may infer this from the endpoint the client
submits requests to. Cannot be updated. In CamelCase. More info: https://git.k8s.io/community/contributors/devel/sig-architecture/api-conventions.md#types-kinds'
type: string
metadata:
type: object
spec:
type: object
required:
- bar
- foo
properties:
bar:
description: foo contains foo.
type: string
foo:
description: foo contains foo.
type: string
- name: v1
schema:
openAPIV3Schema:
description: Example is a kind with schema changes.
type: object
required:
- spec
properties:
apiVersion:
description: 'APIVersion defines the versioned schema of this representation of an object. Servers should convert recognized schemas to the latest internal value, and may reject unrecognized values. More info: https://git.k8s.io/community/contributors/devel/sig-architecture/api-conventions.md#resources'
type: string
kind:
description: 'Kind is a string value representing the REST resource this object represents. Servers may infer this from the endpoint the client submits requests to. Cannot be updated. In CamelCase. More info: https://git.k8s.io/community/contributors/devel/sig-architecture/api-conventions.md#types-kinds'
type: string
metadata:
type: object
spec:
type: object
required:
- bar
- foo
properties:
bar:
description: foo contains foo.
type: string
foo:
description: foo contains foo.
type: string
14 changes: 5 additions & 9 deletions pkg/schemapatcher/testdata/expected/kubebuilder-example-crd.yaml
Expand Up @@ -16,25 +16,21 @@ spec:
description: Example is a kind with schema changes.
type: object
required:
- spec
- spec
properties:
apiVersion:
description: 'APIVersion defines the versioned schema of this representation
of an object. Servers should convert recognized schemas to the latest
internal value, and may reject unrecognized values. More info: https://git.k8s.io/community/contributors/devel/sig-architecture/api-conventions.md#resources'
description: 'APIVersion defines the versioned schema of this representation of an object. Servers should convert recognized schemas to the latest internal value, and may reject unrecognized values. More info: https://git.k8s.io/community/contributors/devel/sig-architecture/api-conventions.md#resources'
type: string
kind:
description: 'Kind is a string value representing the REST resource this
object represents. Servers may infer this from the endpoint the client
submits requests to. Cannot be updated. In CamelCase. More info: https://git.k8s.io/community/contributors/devel/sig-architecture/api-conventions.md#types-kinds'
description: 'Kind is a string value representing the REST resource this object represents. Servers may infer this from the endpoint the client submits requests to. Cannot be updated. In CamelCase. More info: https://git.k8s.io/community/contributors/devel/sig-architecture/api-conventions.md#types-kinds'
type: string
metadata:
type: object
spec:
type: object
required:
- bar
- foo
- bar
- foo
properties:
bar:
description: foo contains foo.
Expand Down
Expand Up @@ -16,25 +16,21 @@ spec:
description: Unchanged is a kind without schema changes.
type: object
required:
- spec
- spec
properties:
apiVersion:
description: 'APIVersion defines the versioned schema of this representation
of an object. Servers should convert recognized schemas to the latest
internal value, and may reject unrecognized values. More info: https://git.k8s.io/community/contributors/devel/sig-architecture/api-conventions.md#resources'
description: 'APIVersion defines the versioned schema of this representation of an object. Servers should convert recognized schemas to the latest internal value, and may reject unrecognized values. More info: https://git.k8s.io/community/contributors/devel/sig-architecture/api-conventions.md#resources'
type: string
kind:
description: 'Kind is a string value representing the REST resource this
object represents. Servers may infer this from the endpoint the client
submits requests to. Cannot be updated. In CamelCase. More info: https://git.k8s.io/community/contributors/devel/sig-architecture/api-conventions.md#types-kinds'
description: 'Kind is a string value representing the REST resource this object represents. Servers may infer this from the endpoint the client submits requests to. Cannot be updated. In CamelCase. More info: https://git.k8s.io/community/contributors/devel/sig-architecture/api-conventions.md#types-kinds'
type: string
metadata:
type: object
spec:
type: object
required:
- bar
- foo
- bar
- foo
properties:
bar:
description: foo contains foo.
Expand Down
60 changes: 28 additions & 32 deletions pkg/schemapatcher/testdata/expected/kubebuilder-unchanged-crd.yaml
Expand Up @@ -11,35 +11,31 @@ spec:
plural: unchangeds
listKind: UnchangedList
versions:
- name: v1
schema:
openAPIV3Schema:
description: Unchanged is a kind without schema changes.
type: object
required:
- spec
properties:
apiVersion:
description: 'APIVersion defines the versioned schema of this representation
of an object. Servers should convert recognized schemas to the latest
internal value, and may reject unrecognized values. More info: https://git.k8s.io/community/contributors/devel/sig-architecture/api-conventions.md#resources'
type: string
kind:
description: 'Kind is a string value representing the REST resource this
object represents. Servers may infer this from the endpoint the client
submits requests to. Cannot be updated. In CamelCase. More info: https://git.k8s.io/community/contributors/devel/sig-architecture/api-conventions.md#types-kinds'
type: string
metadata:
type: object
spec:
type: object
required:
- bar
- foo
properties:
bar:
description: foo contains foo.
type: string
foo:
description: foo contains foo.
type: string
- name: v1
schema:
openAPIV3Schema:
description: Unchanged is a kind without schema changes.
type: object
required:
- spec
properties:
apiVersion:
description: 'APIVersion defines the versioned schema of this representation of an object. Servers should convert recognized schemas to the latest internal value, and may reject unrecognized values. More info: https://git.k8s.io/community/contributors/devel/sig-architecture/api-conventions.md#resources'
type: string
kind:
description: 'Kind is a string value representing the REST resource this object represents. Servers may infer this from the endpoint the client submits requests to. Cannot be updated. In CamelCase. More info: https://git.k8s.io/community/contributors/devel/sig-architecture/api-conventions.md#types-kinds'
type: string
metadata:
type: object
spec:
type: object
required:
- bar
- foo
properties:
bar:
description: foo contains foo.
type: string
foo:
description: foo contains foo.
type: string
14 changes: 5 additions & 9 deletions pkg/schemapatcher/testdata/expected/legacy-example-crd.yaml
Expand Up @@ -20,25 +20,21 @@ spec:
description: Example is a kind with schema changes.
type: object
required:
- spec
- spec
properties:
apiVersion:
description: 'APIVersion defines the versioned schema of this representation
of an object. Servers should convert recognized schemas to the latest
internal value, and may reject unrecognized values. More info: https://git.k8s.io/community/contributors/devel/sig-architecture/api-conventions.md#resources'
description: 'APIVersion defines the versioned schema of this representation of an object. Servers should convert recognized schemas to the latest internal value, and may reject unrecognized values. More info: https://git.k8s.io/community/contributors/devel/sig-architecture/api-conventions.md#resources'
type: string
kind:
description: 'Kind is a string value representing the REST resource this
object represents. Servers may infer this from the endpoint the client
submits requests to. Cannot be updated. In CamelCase. More info: https://git.k8s.io/community/contributors/devel/sig-architecture/api-conventions.md#types-kinds'
description: 'Kind is a string value representing the REST resource this object represents. Servers may infer this from the endpoint the client submits requests to. Cannot be updated. In CamelCase. More info: https://git.k8s.io/community/contributors/devel/sig-architecture/api-conventions.md#types-kinds'
type: string
metadata:
type: object
spec:
type: object
required:
- bar
- foo
- bar
- foo
properties:
bar:
description: foo contains foo.
Expand Down
14 changes: 5 additions & 9 deletions pkg/schemapatcher/testdata/expected/legacy-unchanged-crd.yaml
Expand Up @@ -21,25 +21,21 @@ spec:
description: Unchanged is a kind without schema changes.
type: object
required:
- spec
- spec
properties:
apiVersion:
description: 'APIVersion defines the versioned schema of this representation
of an object. Servers should convert recognized schemas to the latest
internal value, and may reject unrecognized values. More info: https://git.k8s.io/community/contributors/devel/sig-architecture/api-conventions.md#resources'
description: 'APIVersion defines the versioned schema of this representation of an object. Servers should convert recognized schemas to the latest internal value, and may reject unrecognized values. More info: https://git.k8s.io/community/contributors/devel/sig-architecture/api-conventions.md#resources'
type: string
kind:
description: 'Kind is a string value representing the REST resource this
object represents. Servers may infer this from the endpoint the client
submits requests to. Cannot be updated. In CamelCase. More info: https://git.k8s.io/community/contributors/devel/sig-architecture/api-conventions.md#types-kinds'
description: 'Kind is a string value representing the REST resource this object represents. Servers may infer this from the endpoint the client submits requests to. Cannot be updated. In CamelCase. More info: https://git.k8s.io/community/contributors/devel/sig-architecture/api-conventions.md#types-kinds'
type: string
metadata:
type: object
spec:
type: object
required:
- bar
- foo
- bar
- foo
properties:
bar:
description: foo contains foo.
Expand Down