Merge pull request #955 from vincepri/backport-fixes

k8s-ci-robot · web-flow · commit 4715a5e0b11c · 2020-05-20T15:22:19.000-07:00
Backports for v0.5.3
diff --git a/pkg/client/config/config.go b/pkg/client/config/config.go
@@ -133,11 +133,8 @@ func loadConfig(context string) (*rest.Config, error) {
 		}
 		loadingRules.Precedence = append(loadingRules.Precedence, path.Join(u.HomeDir, clientcmd.RecommendedHomeDir, clientcmd.RecommendedFileName))
 	}
-	if c, err := loadConfigWithContext(apiServerURL, loadingRules, context); err == nil {
-		return c, nil
-	}
 
-	return nil, fmt.Errorf("could not locate a kubeconfig")
+	return loadConfigWithContext(apiServerURL, loadingRules, context)
 }
 
 func loadConfigWithContext(apiServerURL string, loader clientcmd.ClientConfigLoader, context string) (*rest.Config, error) {
diff --git a/pkg/client/options.go b/pkg/client/options.go
@@ -381,7 +381,7 @@ type MatchingLabels map[string]string
 
 func (m MatchingLabels) ApplyToList(opts *ListOptions) {
 	// TODO(directxman12): can we avoid reserializing this over and over?
-	sel := labels.SelectorFromSet(map[string]string(m))
+	sel := labels.SelectorFromValidatedSet(map[string]string(m))
 	opts.LabelSelector = sel
 }
 
diff --git a/pkg/client/options_test.go b/pkg/client/options_test.go
@@ -215,3 +215,17 @@ var _ = Describe("DeleteAllOfOptions", func() {
 		Expect(newDeleteAllOfOpts).To(Equal(o))
 	})
 })
+
+var _ = Describe("MatchingLabels", func() {
+	It("Should produce an invalid selector when given invalid input", func() {
+		matchingLabels := client.MatchingLabels(map[string]string{"k": "axahm2EJ8Phiephe2eixohbee9eGeiyees1thuozi1xoh0GiuH3diewi8iem7Nui"})
+		listOpts := &client.ListOptions{}
+		matchingLabels.ApplyToList(listOpts)
+
+		r, _ := listOpts.LabelSelector.Requirements()
+		_, err := labels.NewRequirement(r[0].Key(), r[0].Operator(), r[0].Values().List())
+		Expect(err).ToNot(BeNil())
+		expectedErrMsg := `invalid label value: "axahm2EJ8Phiephe2eixohbee9eGeiyees1thuozi1xoh0GiuH3diewi8iem7Nui": at key: "k": must be no more than 63 characters`
+		Expect(err.Error()).To(Equal(expectedErrMsg))
+	})
+})
diff --git a/pkg/controller/controllerutil/controllerutil.go b/pkg/controller/controllerutil/controllerutil.go
@@ -262,9 +262,10 @@ func AddFinalizerWithError(o runtime.Object, finalizer string) error {
 // RemoveFinalizer accepts a metav1 object and removes the provided finalizer if present.
 func RemoveFinalizer(o metav1.Object, finalizer string) {
 	f := o.GetFinalizers()
-	for i, e := range f {
-		if e == finalizer {
+	for i := 0; i < len(f); i++ {
+		if f[i] == finalizer {
 			f = append(f[:i], f[i+1:]...)
+			i--
 		}
 	}
 	o.SetFinalizers(f)
@@ -280,3 +281,10 @@ func RemoveFinalizerWithError(o runtime.Object, finalizer string) error {
 	RemoveFinalizer(m, finalizer)
 	return nil
 }
+
+// Object allows functions to work indistinctly with any resource that
+// implements both Object interfaces.
+type Object interface {
+	metav1.Object
+	runtime.Object
+}
diff --git a/pkg/controller/controllerutil/controllerutil_test.go b/pkg/controller/controllerutil/controllerutil_test.go
@@ -445,6 +445,12 @@ var _ = Describe("Controllerutil", func() {
 				controllerutil.RemoveFinalizer(deploy, testFinalizer)
 				Expect(deploy.ObjectMeta.GetFinalizers()).To(Equal([]string{}))
 			})
+
+			It("should remove all equal finalizers if present", func() {
+				deploy.SetFinalizers(append(deploy.Finalizers, testFinalizer, testFinalizer))
+				controllerutil.RemoveFinalizer(deploy, testFinalizer)
+				Expect(deploy.ObjectMeta.GetFinalizers()).To(Equal([]string{}))
+			})
 		})
 	})
 })
diff --git a/pkg/envtest/crd.go b/pkg/envtest/crd.go
@@ -278,10 +278,16 @@ func renderCRDs(options *CRDInstallOptions) ([]runtime.Object, error) {
 	var (
 		err   error
 		info  os.FileInfo
-		crds  []*unstructured.Unstructured
 		files []os.FileInfo
 	)
 
+	type GVKN struct {
+		GVK  schema.GroupVersionKind
+		Name string
+	}
+
+	crds := map[GVKN]*unstructured.Unstructured{}
+
 	for _, path := range options.Paths {
 		var filePath = path
 
@@ -294,7 +300,7 @@ func renderCRDs(options *CRDInstallOptions) ([]runtime.Object, error) {
 		}
 
 		if !info.IsDir() {
-			filePath, files = filepath.Dir(path), append(files, info)
+			filePath, files = filepath.Dir(path), []os.FileInfo{info}
 		} else {
 			if files, err = ioutil.ReadDir(path); err != nil {
 				return nil, err
@@ -307,14 +313,23 @@ func renderCRDs(options *CRDInstallOptions) ([]runtime.Object, error) {
 			return nil, err
 		}
 
-		// If CRD already in the list, skip it.
-		if existsUnstructured(crds, crdList) {
-			continue
+		for i, crd := range crdList {
+			gvkn := GVKN{GVK: crd.GroupVersionKind(), Name: crd.GetName()}
+			if _, found := crds[gvkn]; found {
+				// Currently, we only print a log when there are duplicates. We may want to error out if that makes more sense.
+				log.Info("there are more than one CRD definitions with the same <Group, Version, Kind, Name>", "GVKN", gvkn)
+			}
+			// We always use the CRD definition that we found last.
+			crds[gvkn] = crdList[i]
 		}
-		crds = append(crds, crdList...)
 	}
 
-	return unstructuredCRDListToRuntime(crds), nil
+	// Converting map to a list to return
+	var res []runtime.Object
+	for _, obj := range crds {
+		res = append(res, obj)
+	}
+	return res, nil
 }
 
 // readCRDs reads the CRDs from files and Unmarshals them into structs
diff --git a/pkg/envtest/envtest_test.go b/pkg/envtest/envtest_test.go
@@ -213,6 +213,19 @@ var _ = Describe("Test", func() {
 			close(done)
 		}, 10)
 
+		It("should be able to install CRDs using multiple files", func(done Done) {
+			crds, err = InstallCRDs(env.Config, CRDInstallOptions{
+				Paths: []string{
+					filepath.Join(".", "testdata", "examplecrd.yaml"),
+					filepath.Join(".", "testdata", "examplecrd_v1.yaml"),
+				},
+			})
+			Expect(err).NotTo(HaveOccurred())
+			Expect(crds).To(HaveLen(2))
+
+			close(done)
+		}, 10)
+
 		It("should filter out already existent CRD", func(done Done) {
 			crds, err = InstallCRDs(env.Config, CRDInstallOptions{
 				Paths: []string{
diff --git a/pkg/envtest/helper.go b/pkg/envtest/helper.go
@@ -1,8 +1,6 @@
 package envtest
 
 import (
-	"reflect"
-
 	apiextensionsv1 "k8s.io/apiextensions-apiserver/pkg/apis/apiextensions/v1"
 	apiextensionsv1beta1 "k8s.io/apiextensions-apiserver/pkg/apis/apiextensions/v1beta1"
 	"k8s.io/apimachinery/pkg/apis/meta/v1/unstructured"
@@ -57,18 +55,6 @@ func mergeCRDs(s1, s2 []runtime.Object) []runtime.Object {
 	return merged
 }
 
-// existsUnstructured verify if a any item is common between two lists.
-func existsUnstructured(s1, s2 []*unstructured.Unstructured) bool {
-	for _, s1obj := range s1 {
-		for _, s2obj := range s2 {
-			if reflect.DeepEqual(s1obj, s2obj) {
-				return true
-			}
-		}
-	}
-	return false
-}
-
 func runtimeCRDListToUnstructured(l []runtime.Object) []*unstructured.Unstructured {
 	res := []*unstructured.Unstructured{}
 	for _, obj := range l {
@@ -81,11 +67,3 @@ func runtimeCRDListToUnstructured(l []runtime.Object) []*unstructured.Unstructur
 	}
 	return res
 }
-
-func unstructuredCRDListToRuntime(l []*unstructured.Unstructured) []runtime.Object {
-	res := []runtime.Object{}
-	for _, obj := range l {
-		res = append(res, obj.DeepCopy())
-	}
-	return res
-}
diff --git a/pkg/envtest/testdata/webhooks/manifests.yaml b/pkg/envtest/testdata/webhooks/manifests.yaml
@@ -0,0 +1,101 @@
+---
+apiVersion: admissionregistration.k8s.io/v1beta1
+kind: MutatingWebhookConfiguration
+metadata:
+  creationTimestamp: null
+  name: mutating-webhook-configuration
+webhooks:
+- clientConfig:
+    caBundle: Cg==
+    service:
+      name: webhook-service
+      namespace: system
+      path: /mutate-v1beta1
+  failurePolicy: Fail
+  name: mpods.kb.io
+  rules:
+  - apiGroups:
+    - ""
+    apiVersions:
+    - v1
+    operations:
+    - CREATE
+    - UPDATE
+    resources:
+    - pods
+---
+apiVersion: admissionregistration.k8s.io/v1
+kind: MutatingWebhookConfiguration
+metadata:
+  creationTimestamp: null
+  name: mutating-webhook-configuration2
+webhooks:
+- clientConfig:
+    caBundle: Cg==
+    service:
+      name: webhook-service
+      namespace: system
+      path: /mutate-v1
+  failurePolicy: Fail
+  name: mpods2.kb.io
+  rules:
+  - apiGroups:
+    - ""
+    apiVersions:
+    - v1
+    operations:
+    - CREATE
+    - UPDATE
+    resources:
+    - pods
+---
+apiVersion: admissionregistration.k8s.io/v1beta1
+kind: ValidatingWebhookConfiguration
+metadata:
+  creationTimestamp: null
+  name: validating-webhook-configuration
+webhooks:
+- clientConfig:
+    caBundle: Cg==
+    service:
+      name: webhook-service
+      namespace: system
+      path: /validate-v1beta1
+  failurePolicy: Fail
+  name: vpods.kb.io
+  rules:
+  - apiGroups:
+    - ""
+    apiVersions:
+    - v1
+    operations:
+    - CREATE
+    - UPDATE
+    resources:
+    - pods
+---
+apiVersion: admissionregistration.k8s.io/v1
+kind: ValidatingWebhookConfiguration
+metadata:
+  creationTimestamp: null
+  name: validating-webhook-configuration2
+webhooks:
+- clientConfig:
+    caBundle: Cg==
+    service:
+      name: webhook-service
+      namespace: system
+      path: /validate-v1
+  failurePolicy: Fail
+  name: vpods2.kb.io
+  rules:
+  - apiGroups:
+    - ""
+    apiVersions:
+    - v1
+    operations:
+    - CREATE
+    - UPDATE
+    resources:
+    - pods
+
diff --git a/pkg/envtest/webhook.go b/pkg/envtest/webhook.go
@@ -375,9 +375,13 @@ func readWebhooks(path string) ([]runtime.Object, []runtime.Object, error) {
 				return nil, nil, err
 			}
 
+			const (
+				admissionregv1      = "admissionregistration.k8s.io/v1"
+				admissionregv1beta1 = "admissionregistration.k8s.io/v1beta1"
+			)
 			switch {
 			case generic.Kind == "MutatingWebhookConfiguration":
-				if generic.APIVersion != "v1beta1" && generic.APIVersion != "v1" {
+				if generic.APIVersion != admissionregv1beta1 && generic.APIVersion != admissionregv1 {
 					return nil, nil, fmt.Errorf("only v1beta1 and v1 are supported right now for MutatingWebhookConfiguration (name: %s)", generic.Name)
 				}
 				hook := &unstructured.Unstructured{}
@@ -386,7 +390,7 @@ func readWebhooks(path string) ([]runtime.Object, []runtime.Object, error) {
 				}
 				mutHooks = append(mutHooks, hook)
 			case generic.Kind == "ValidatingWebhookConfiguration":
-				if generic.APIVersion != "v1beta1" && generic.APIVersion != "v1" {
+				if generic.APIVersion != admissionregv1beta1 && generic.APIVersion != admissionregv1 {
 					return nil, nil, fmt.Errorf("only v1beta1 and v1 are supported right now for ValidatingWebhookConfiguration (name: %s)", generic.Name)
 				}
 				hook := &unstructured.Unstructured{}
diff --git a/pkg/envtest/webhook_test.go b/pkg/envtest/webhook_test.go
@@ -3,6 +3,7 @@ package envtest
 import (
 	"context"
 	"fmt"
+	"path/filepath"
 	"time"
 
 	. "github.com/onsi/ginkgo"
@@ -73,6 +74,16 @@ var _ = Describe("Test", func() {
 			close(stopCh)
 			close(done)
 		})
+
+		It("should load webhooks from files", func() {
+			installOptions := WebhookInstallOptions{
+				DirectoryPaths: []string{filepath.Join("testdata", "webhooks")},
+			}
+			err := parseWebhookDirs(&installOptions)
+			Expect(err).NotTo(HaveOccurred())
+			Expect(len(installOptions.MutatingWebhooks)).To(Equal(2))
+			Expect(len(installOptions.ValidatingWebhooks)).To(Equal(2))
+		})
 	})
 })
 
diff --git a/pkg/log/zap/kube_helpers.go b/pkg/log/zap/kube_helpers.go
@@ -105,7 +105,9 @@ func (k *KubeAwareEncoder) EncodeEntry(entry zapcore.Entry, fields []zapcore.Fie
 		// intercept stringer fields that happen to be Kubernetes runtime.Object or
 		// types.NamespacedName values (Kubernetes runtime.Objects commonly
 		// implement String, apparently).
-		if field.Type == zapcore.StringerType {
+		// *unstructured.Unstructured does NOT implement fmt.Striger interface.
+		// We have handle it specially.
+		if field.Type == zapcore.StringerType || field.Type == zapcore.ReflectType {
 			switch val := field.Interface.(type) {
 			case runtime.Object:
 				fields[i] = zapcore.Field{
diff --git a/pkg/log/zap/zap_test.go b/pkg/log/zap/zap_test.go
@@ -25,6 +25,7 @@ import (
 	. "github.com/onsi/ginkgo"
 	. "github.com/onsi/gomega"
 	kapi "k8s.io/api/core/v1"
+	"k8s.io/apimachinery/pkg/apis/meta/v1/unstructured"
 	"k8s.io/apimachinery/pkg/types"
 )
 
@@ -238,6 +239,27 @@ var _ = Describe("Zap logger setup", func() {
 				}))
 			})
 
+			It("should log an unstructured Kubernetes object", func() {
+				pod := &unstructured.Unstructured{
+					Object: map[string]interface{}{
+						"metadata": map[string]interface{}{
+							"name":      "some-pod",
+							"namespace": "some-ns",
+						},
+					},
+				}
+				logger.Info("here's a kubernetes object", "thing", pod)
+
+				outRaw := logOut.Bytes()
+				res := map[string]interface{}{}
+				Expect(json.Unmarshal(outRaw, &res)).To(Succeed())
+
+				Expect(res).To(HaveKeyWithValue("thing", map[string]interface{}{
+					"name":      "some-pod",
+					"namespace": "some-ns",
+				}))
+			})
+
 			It("should log a standard namespaced NamespacedName name and namespace", func() {
 				name := types.NamespacedName{Name: "some-pod", Namespace: "some-ns"}
 				logger.Info("here's a kubernetes object", "thing", name)

Original file line number	Diff line number	Diff line change
`@@ -133,11 +133,8 @@ func loadConfig(context string) (*rest.Config, error) {`
`133`	`133`	`}`
`134`	`134`	`loadingRules.Precedence = append(loadingRules.Precedence, path.Join(u.HomeDir, clientcmd.RecommendedHomeDir, clientcmd.RecommendedFileName))`
`135`	`135`	`}`
`136`		`- if c, err := loadConfigWithContext(apiServerURL, loadingRules, context); err == nil {`
`137`		`- return c, nil`
`138`		`- }`
`139`	`136`
`140`		`- return nil, fmt.Errorf("could not locate a kubeconfig")`
	`137`	`+ return loadConfigWithContext(apiServerURL, loadingRules, context)`
`141`	`138`	`}`
`142`	`139`
`143`	`140`	`func loadConfigWithContext(apiServerURL string, loader clientcmd.ClientConfigLoader, context string) (*rest.Config, error) {`
Original file line number	Diff line number	Diff line change
`@@ -381,7 +381,7 @@ type MatchingLabels map[string]string`
`381`	`381`
`382`	`382`	`func (m MatchingLabels) ApplyToList(opts *ListOptions) {`
`383`	`383`	`// TODO(directxman12): can we avoid reserializing this over and over?`
`384`		`- sel := labels.SelectorFromSet(map[string]string(m))`
	`384`	`+ sel := labels.SelectorFromValidatedSet(map[string]string(m))`
`385`	`385`	`opts.LabelSelector = sel`
`386`	`386`	`}`
`387`	`387`
Original file line number	Diff line number	Diff line change
`@@ -375,9 +375,13 @@ func readWebhooks(path string) ([]runtime.Object, []runtime.Object, error) {`
`375`	`375`	`return nil, nil, err`
`376`	`376`	`}`
`377`	`377`
	`378`	`+ const (`
	`379`	`+ admissionregv1 = "admissionregistration.k8s.io/v1"`
	`380`	`+ admissionregv1beta1 = "admissionregistration.k8s.io/v1beta1"`
	`381`	`+ )`
`378`	`382`	`switch {`
`379`	`383`	`case generic.Kind == "MutatingWebhookConfiguration":`
`380`		`- if generic.APIVersion != "v1beta1" && generic.APIVersion != "v1" {`
	`384`	`+ if generic.APIVersion != admissionregv1beta1 && generic.APIVersion != admissionregv1 {`
`381`	`385`	`return nil, nil, fmt.Errorf("only v1beta1 and v1 are supported right now for MutatingWebhookConfiguration (name: %s)", generic.Name)`
`382`	`386`	`}`
`383`	`387`	`hook := &unstructured.Unstructured{}`
`@@ -386,7 +390,7 @@ func readWebhooks(path string) ([]runtime.Object, []runtime.Object, error) {`
`386`	`390`	`}`
`387`	`391`	`mutHooks = append(mutHooks, hook)`
`388`	`392`	`case generic.Kind == "ValidatingWebhookConfiguration":`
`389`		`- if generic.APIVersion != "v1beta1" && generic.APIVersion != "v1" {`
	`393`	`+ if generic.APIVersion != admissionregv1beta1 && generic.APIVersion != admissionregv1 {`
`390`	`394`	`return nil, nil, fmt.Errorf("only v1beta1 and v1 are supported right now for ValidatingWebhookConfiguration (name: %s)", generic.Name)`
`391`	`395`	`}`
`392`	`396`	`hook := &unstructured.Unstructured{}`