From d32c76fc03381784516c47cb1bf62ef932189afa Mon Sep 17 00:00:00 2001 From: Jan Chaloupka Date: Tue, 17 Sep 2019 17:52:28 +0200 Subject: [PATCH] Require exact match when calling Get method within fake clientset `Get` method within the fake clientset returns an object that would not be normally returned when using the real clientset. Reproducer: ```go package main import ( v1 "k8s.io/api/core/v1" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "k8s.io/client-go/kubernetes/fake" ) func main () { cm := &v1.ConfigMap{ ObjectMeta: metav1.ObjectMeta{Namespace: metav1.NamespaceSystem, Name: "cm"}, } client := fake.NewSimpleClientset(cm) obj, err := client.CoreV1().ConfigMaps("").Get("", metav1.GetOptions{}) if err != nil { panic(err) } fmt.Printf("obj: %#v\n", obj) } ``` stored under `test.go` of `github.com/kubernetes/kubernetes` (master HEAD) root directory and ran: ```sh $ go run test.go obj: &v1.ConfigMap{TypeMeta:v1.TypeMeta{Kind:"", APIVersion:""}, ObjectMeta:v1.ObjectMeta{Name:"cm", GenerateName:"", Namespace:"kube-system", SelfLink:"", UID:"", ResourceVersion:"", Generation:0, CreationTimestamp:v1.Time{Time:time.Time{wall:0x0, ext:0, loc:(*time.Location)(nil)}}, DeletionTimestamp:(*v1.Time)(nil), DeletionGracePeriodSeconds:(*int64)(nil), Labels:map[string]string(nil), Annotations:map[string]string(nil), OwnerReferences:[]v1.OwnerReference(nil), Finalizers:[]string(nil), ClusterName:"", ManagedFields:[]v1.ManagedFieldsEntry(nil)}, Data:map[string]string(nil), BinaryData:map[string][]uint8(nil)} ``` As you can see fake clientset with a "test" configmap is created. When getting the object through the clientset back, I intentionally set the object name to an empty string. I would expect to get an error saying config map "" was not found. However, I get "test" configmap instead. Reason for that is inside implementation of `filterByNamespaceAndName` private function: ```go func filterByNamespaceAndName(objs []runtime.Object, ns, name string) ([]runtime.Object, error) { var res []runtime.Object for _, obj := range objs { acc, err := meta.Accessor(obj) if err != nil { return nil, err } if ns != "" && acc.GetNamespace() != ns { continue } if name != "" && acc.GetName() != name { continue } res = append(res, obj) } return res, nil } ``` When `name` is empty, `name != "" && acc.GetName() != name` condition is false and thus `obj` is consider as a fit. [1] https://github.com/kubernetes/client-go/blob/master/testing/fixture.go#L481-L493 --- .../volumemanager/volume_manager_test.go | 6 ++- pkg/volume/storageos/storageos_test.go | 5 +- staging/src/k8s.io/client-go/testing/BUILD | 1 + .../src/k8s.io/client-go/testing/fixture.go | 27 +++++++---- .../k8s.io/client-go/testing/fixture_test.go | 48 +++++++++++++++++++ .../pkg/describe/versioned/describe_test.go | 2 +- 6 files changed, 74 insertions(+), 15 deletions(-) diff --git a/pkg/kubelet/volumemanager/volume_manager_test.go b/pkg/kubelet/volumemanager/volume_manager_test.go index ed8bdb9acc5..91ad174d6d0 100644 --- a/pkg/kubelet/volumemanager/volume_manager_test.go +++ b/pkg/kubelet/volumemanager/volume_manager_test.go @@ -252,7 +252,8 @@ func TestGetExtraSupplementalGroupsForPod(t *testing.T) { }, }, ClaimRef: &v1.ObjectReference{ - Name: claim.ObjectMeta.Name, + Name: claim.ObjectMeta.Name, + Namespace: claim.ObjectMeta.Namespace, }, VolumeMode: &fs, }, @@ -381,7 +382,8 @@ func createObjects(pvMode, podMode v1.PersistentVolumeMode) (*v1.Node, *v1.Pod, }, }, ClaimRef: &v1.ObjectReference{ - Name: "claimA", + Namespace: "nsA", + Name: "claimA", }, VolumeMode: &pvMode, }, diff --git a/pkg/volume/storageos/storageos_test.go b/pkg/volume/storageos/storageos_test.go index c51dfe25732..9a6dd7540e9 100644 --- a/pkg/volume/storageos/storageos_test.go +++ b/pkg/volume/storageos/storageos_test.go @@ -268,8 +268,9 @@ func TestPlugin(t *testing.T) { // PVName: "test-volume-name", PersistentVolumeReclaimPolicy: v1.PersistentVolumeReclaimDelete, Parameters: map[string]string{ - "VolumeNamespace": "test-volume-namespace", - "adminSecretName": secretName, + "VolumeNamespace": "test-volume-namespace", + "adminSecretName": secretName, + "adminsecretnamespace": "default", }, MountOptions: mountOptions, } diff --git a/staging/src/k8s.io/client-go/testing/BUILD b/staging/src/k8s.io/client-go/testing/BUILD index fc8998248b6..44706ac0d93 100644 --- a/staging/src/k8s.io/client-go/testing/BUILD +++ b/staging/src/k8s.io/client-go/testing/BUILD @@ -40,6 +40,7 @@ go_test( ], embed = [":go_default_library"], deps = [ + "//staging/src/k8s.io/apimachinery/pkg/api/errors:go_default_library", "//staging/src/k8s.io/apimachinery/pkg/api/meta:go_default_library", "//staging/src/k8s.io/apimachinery/pkg/apis/meta/v1/unstructured:go_default_library", "//staging/src/k8s.io/apimachinery/pkg/runtime:go_default_library", diff --git a/staging/src/k8s.io/client-go/testing/fixture.go b/staging/src/k8s.io/client-go/testing/fixture.go index 98f82326730..54f600ad3f7 100644 --- a/staging/src/k8s.io/client-go/testing/fixture.go +++ b/staging/src/k8s.io/client-go/testing/fixture.go @@ -248,7 +248,7 @@ func (t *tracker) List(gvr schema.GroupVersionResource, gvk schema.GroupVersionK return list, nil } - matchingObjs, err := filterByNamespaceAndName(objs, ns, "") + matchingObjs, err := filterByNamespace(objs, ns) if err != nil { return nil, err } @@ -282,9 +282,19 @@ func (t *tracker) Get(gvr schema.GroupVersionResource, ns, name string) (runtime return nil, errNotFound } - matchingObjs, err := filterByNamespaceAndName(objs, ns, name) - if err != nil { - return nil, err + var matchingObjs []runtime.Object + for _, obj := range objs { + acc, err := meta.Accessor(obj) + if err != nil { + return nil, err + } + if acc.GetNamespace() != ns { + continue + } + if acc.GetName() != name { + continue + } + matchingObjs = append(matchingObjs, obj) } if len(matchingObjs) == 0 { return nil, errNotFound @@ -472,10 +482,10 @@ func (t *tracker) Delete(gvr schema.GroupVersionResource, ns, name string) error return errors.NewNotFound(gvr.GroupResource(), name) } -// filterByNamespaceAndName returns all objects in the collection that -// match provided namespace and name. Empty namespace matches +// filterByNamespace returns all objects in the collection that +// match provided namespace. Empty namespace matches // non-namespaced objects. -func filterByNamespaceAndName(objs []runtime.Object, ns, name string) ([]runtime.Object, error) { +func filterByNamespace(objs []runtime.Object, ns string) ([]runtime.Object, error) { var res []runtime.Object for _, obj := range objs { @@ -486,9 +496,6 @@ func filterByNamespaceAndName(objs []runtime.Object, ns, name string) ([]runtime if ns != "" && acc.GetNamespace() != ns { continue } - if name != "" && acc.GetName() != name { - continue - } res = append(res, obj) } diff --git a/staging/src/k8s.io/client-go/testing/fixture_test.go b/staging/src/k8s.io/client-go/testing/fixture_test.go index a0338b707f2..912156d316c 100644 --- a/staging/src/k8s.io/client-go/testing/fixture_test.go +++ b/staging/src/k8s.io/client-go/testing/fixture_test.go @@ -25,6 +25,7 @@ import ( "github.com/stretchr/testify/assert" + "k8s.io/apimachinery/pkg/api/errors" "k8s.io/apimachinery/pkg/api/meta" "k8s.io/apimachinery/pkg/apis/meta/v1/unstructured" runtime "k8s.io/apimachinery/pkg/runtime" @@ -274,3 +275,50 @@ func TestPatchWithMissingObject(t *testing.T) { assert.Nil(t, node) assert.EqualError(t, err, `nodes "node-1" not found`) } + +func TestGetWithExactMatch(t *testing.T) { + scheme := runtime.NewScheme() + codecs := serializer.NewCodecFactory(scheme) + + constructObject := func(s schema.GroupVersionResource, name, namespace string) (*unstructured.Unstructured, schema.GroupVersionResource) { + obj := getArbitraryResource(s, name, namespace) + gvks, _, err := scheme.ObjectKinds(obj) + assert.Nil(t, err) + gvr, _ := meta.UnsafeGuessKindToResource(gvks[0]) + return obj, gvr + } + + var err error + // Object with empty namespace + o := NewObjectTracker(scheme, codecs.UniversalDecoder()) + nodeResource := schema.GroupVersionResource{Group: "", Version: "v1", Resource: "node"} + node, gvr := constructObject(nodeResource, "node", "") + + assert.Nil(t, o.Add(node)) + + // Exact match + _, err = o.Get(gvr, "", "node") + assert.Nil(t, err) + + // Unexpected namespace provided + _, err = o.Get(gvr, "ns", "node") + assert.NotNil(t, err) + errNotFound := errors.NewNotFound(gvr.GroupResource(), "node") + assert.EqualError(t, err, errNotFound.Error()) + + // Object with non-empty namespace + o = NewObjectTracker(scheme, codecs.UniversalDecoder()) + podResource := schema.GroupVersionResource{Group: "", Version: "v1", Resource: "pod"} + pod, gvr := constructObject(podResource, "pod", "default") + assert.Nil(t, o.Add(pod)) + + // Exact match + _, err = o.Get(gvr, "default", "pod") + assert.Nil(t, err) + + // Missing namespace + _, err = o.Get(gvr, "", "pod") + assert.NotNil(t, err) + errNotFound = errors.NewNotFound(gvr.GroupResource(), "pod") + assert.EqualError(t, err, errNotFound.Error()) +} diff --git a/staging/src/k8s.io/kubectl/pkg/describe/versioned/describe_test.go b/staging/src/k8s.io/kubectl/pkg/describe/versioned/describe_test.go index 6204a66f138..f14a7519783 100644 --- a/staging/src/k8s.io/kubectl/pkg/describe/versioned/describe_test.go +++ b/staging/src/k8s.io/kubectl/pkg/describe/versioned/describe_test.go @@ -3133,7 +3133,7 @@ Spec: }, }) d := NetworkPolicyDescriber{versionedFake} - out, err := d.Describe("", "network-policy-1", describe.DescriberSettings{}) + out, err := d.Describe("default", "network-policy-1", describe.DescriberSettings{}) if err != nil { t.Errorf("unexpected error: %s", err) }