Merge pull request #955 from vincepri/backport-fixes

Backports for v0.5.3

Author: k8s-ci-robot

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) {

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
 }
 

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))
+	})
+})

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
+}

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{}))
+			})
 		})
 	})
 })

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

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{

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
-}

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
+

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{}

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))
+		})
 	})
 })
 

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{

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)