From 69452414de4db0df64b3bf45d3204d67b0050ad5 Mon Sep 17 00:00:00 2001 From: Tomasz Mielech Date: Wed, 30 Oct 2019 14:47:42 +0100 Subject: [PATCH] Discard alpha and beta phrases from annotations --- pkg/util/k8sutil/constants.go | 5 +- pkg/util/k8sutil/services.go | 11 +- pkg/util/k8sutil/storageclass.go | 25 ++-- pkg/util/k8sutil/storageclass_test.go | 193 +++++++++++++++++++------- 4 files changed, 154 insertions(+), 80 deletions(-) diff --git a/pkg/util/k8sutil/constants.go b/pkg/util/k8sutil/constants.go index 0ae48e1dd..807d26184 100644 --- a/pkg/util/k8sutil/constants.go +++ b/pkg/util/k8sutil/constants.go @@ -30,9 +30,8 @@ const ( ArangoExporterPort = 9101 // K8s constants - ClusterIPNone = "None" - TolerateUnreadyEndpointsAnnotation = "service.alpha.kubernetes.io/tolerate-unready-endpoints" - TopologyKeyHostname = "kubernetes.io/hostname" + ClusterIPNone = "None" + TopologyKeyHostname = "kubernetes.io/hostname" // Internal constants ImageIDAndVersionRole = "id" // Role use by identification pods diff --git a/pkg/util/k8sutil/services.go b/pkg/util/k8sutil/services.go index d5eb00e33..e250fa900 100644 --- a/pkg/util/k8sutil/services.go +++ b/pkg/util/k8sutil/services.go @@ -188,14 +188,9 @@ func createService(svcs ServiceInterface, svcName, deploymentName, ns, clusterIP labels := LabelsForDeployment(deploymentName, role) svc := &v1.Service{ ObjectMeta: metav1.ObjectMeta{ - Name: svcName, - Labels: labels, - Annotations: map[string]string{ - // This annotation is deprecated, PublishNotReadyAddresses is - // used instead. We leave the annotation in for a while. - // See https://github.com/kubernetes/kubernetes/pull/49061 - TolerateUnreadyEndpointsAnnotation: strconv.FormatBool(publishNotReadyAddresses), - }, + Name: svcName, + Labels: labels, + Annotations: map[string]string{}, }, Spec: v1.ServiceSpec{ Type: serviceType, diff --git a/pkg/util/k8sutil/storageclass.go b/pkg/util/k8sutil/storageclass.go index e0e212052..5b59b86e8 100644 --- a/pkg/util/k8sutil/storageclass.go +++ b/pkg/util/k8sutil/storageclass.go @@ -26,29 +26,22 @@ import ( "strconv" "time" - "k8s.io/api/storage/v1" + "github.com/arangodb/kube-arangodb/pkg/util/retry" + v1 "k8s.io/api/storage/v1" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" storagev1 "k8s.io/client-go/kubernetes/typed/storage/v1" - - "github.com/arangodb/kube-arangodb/pkg/util/retry" ) var ( - annStorageClassIsDefault = []string{ - // Make sure first entry is the one we'll put in - "storageclass.kubernetes.io/is-default-class", - "storageclass.beta.kubernetes.io/is-default-class", - } + annStorageClassIsDefault = "storageclass.kubernetes.io/is-default-class" ) // StorageClassIsDefault returns true if the given storage class is marked default, // false otherwise. func StorageClassIsDefault(sc *v1.StorageClass) bool { - for _, key := range annStorageClassIsDefault { - if value, found := sc.GetObjectMeta().GetAnnotations()[key]; found { - if boolValue, err := strconv.ParseBool(value); err == nil && boolValue { - return true - } + if value, found := sc.GetObjectMeta().GetAnnotations()[annStorageClassIsDefault]; found { + if boolValue, err := strconv.ParseBool(value); err == nil && boolValue { + return true } } return false @@ -70,11 +63,9 @@ func PatchStorageClassIsDefault(cli storagev1.StorageV1Interface, name string, i if ann == nil { ann = make(map[string]string) } - for _, key := range annStorageClassIsDefault { - delete(ann, key) - } - ann[annStorageClassIsDefault[0]] = strconv.FormatBool(isDefault) + ann[annStorageClassIsDefault] = strconv.FormatBool(isDefault) current.SetAnnotations(ann) + // Save StorageClass if _, err := stcs.Update(current); IsConflict(err) { // StorageClass has been modified since we read it diff --git a/pkg/util/k8sutil/storageclass_test.go b/pkg/util/k8sutil/storageclass_test.go index 7f503f7e9..3db55ef09 100644 --- a/pkg/util/k8sutil/storageclass_test.go +++ b/pkg/util/k8sutil/storageclass_test.go @@ -23,78 +23,167 @@ package k8sutil import ( + "errors" "testing" - "k8s.io/api/storage/v1" + "github.com/arangodb/kube-arangodb/pkg/util/retry" + "github.com/stretchr/testify/require" + "k8s.io/apimachinery/pkg/runtime" + + "github.com/stretchr/testify/assert" + v1 "k8s.io/api/storage/v1" + er "k8s.io/apimachinery/pkg/api/errors" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" + "k8s.io/client-go/kubernetes/fake" + k8stesting "k8s.io/client-go/testing" ) -// StorageClassIsDefault returns true if the given storage class is marked default, -// false otherwise. func TestStorageClassIsDefault(t *testing.T) { - tests := []struct { + testCases := []struct { + Name string StorageClass v1.StorageClass IsDefault bool }{ - // final annotation - {v1.StorageClass{ - ObjectMeta: metav1.ObjectMeta{ - Annotations: map[string]string{}, + { + Name: "Storage class without annotations", + StorageClass: v1.StorageClass{ + ObjectMeta: metav1.ObjectMeta{}, }, - }, false}, - {v1.StorageClass{ - ObjectMeta: metav1.ObjectMeta{ - Annotations: map[string]string{ - annStorageClassIsDefault[0]: "false", + IsDefault: false, + }, + { + Name: "Storage class with empty annotations", + StorageClass: v1.StorageClass{ + ObjectMeta: metav1.ObjectMeta{ + Annotations: map[string]string{}, }, }, - }, false}, - {v1.StorageClass{ - ObjectMeta: metav1.ObjectMeta{ - Annotations: map[string]string{ - annStorageClassIsDefault[0]: "foo", + IsDefault: false, + }, + { + Name: "Storage class without default", + StorageClass: v1.StorageClass{ + ObjectMeta: metav1.ObjectMeta{ + Annotations: map[string]string{ + annStorageClassIsDefault: "false", + }, }, }, - }, false}, - {v1.StorageClass{ - ObjectMeta: metav1.ObjectMeta{ - Annotations: map[string]string{ - annStorageClassIsDefault[0]: "true", + IsDefault: false, + }, + { + Name: "Storage class with invalid value in annotation", + StorageClass: v1.StorageClass{ + ObjectMeta: metav1.ObjectMeta{ + Annotations: map[string]string{ + annStorageClassIsDefault: "foo", + }, }, }, - }, true}, - // beta annotation - {v1.StorageClass{ - ObjectMeta: metav1.ObjectMeta{ - Annotations: map[string]string{}, - }, - }, false}, - {v1.StorageClass{ - ObjectMeta: metav1.ObjectMeta{ - Annotations: map[string]string{ - annStorageClassIsDefault[1]: "false", + IsDefault: false, + }, + { + Name: "Default storage class exits", + StorageClass: v1.StorageClass{ + ObjectMeta: metav1.ObjectMeta{ + Annotations: map[string]string{ + annStorageClassIsDefault: "true", + }, }, }, - }, false}, - {v1.StorageClass{ - ObjectMeta: metav1.ObjectMeta{ - Annotations: map[string]string{ - annStorageClassIsDefault[1]: "foo", - }, + IsDefault: true, + }, + } + + for _, testCase := range testCases { + t.Run(testCase.Name, func(t *testing.T) { + result := StorageClassIsDefault(&testCase.StorageClass) + assert.Equal(t, testCase.IsDefault, result, "StorageClassIsDefault failed. Expected %v, got %v for %#v", + testCase.IsDefault, result, testCase.StorageClass) + }) + } +} + +func TestPatchStorageClassIsDefault(t *testing.T) { + // Arrange + resourceName := "storageclasses" + testCases := []struct { + Name string + StorageClassName string + ExpectedErr error + Reactor func(action k8stesting.Action) (handled bool, ret runtime.Object, err error) + ReactorActionVerb string + }{ + { + Name: "Set storage class is set to default", + StorageClassName: "test", + }, + { + Name: "Storage class does not exist", + StorageClassName: "invalid", + ExpectedErr: er.NewNotFound(v1.Resource(resourceName), "invalid"), + }, + { + Name: "Can not get storage class from kubernetes", + StorageClassName: "test", + Reactor: func(action k8stesting.Action) (handled bool, ret runtime.Object, err error) { + return true, nil, retry.Permanent(errors.New("test")) }, - }, false}, - {v1.StorageClass{ - ObjectMeta: metav1.ObjectMeta{ - Annotations: map[string]string{ - annStorageClassIsDefault[1]: "true", - }, + ReactorActionVerb: "get", + ExpectedErr: errors.New("test"), + }, + { + Name: "Can not update storage class", + StorageClassName: "test", + Reactor: func(action k8stesting.Action) (handled bool, ret runtime.Object, err error) { + return true, nil, errors.New("test") }, - }, true}, + ReactorActionVerb: "update", + ExpectedErr: errors.New("test"), + }, + { + Name: "Can not update Storage class due to permanent conflict", + StorageClassName: "test", + Reactor: func(action k8stesting.Action) (handled bool, ret runtime.Object, err error) { + return true, nil, + retry.Permanent(er.NewConflict(v1.Resource(resourceName), "test", nil)) + }, + ReactorActionVerb: "update", + ExpectedErr: er.NewConflict(v1.Resource(resourceName), "test", nil), + }, } - for _, test := range tests { - result := StorageClassIsDefault(&test.StorageClass) - if result != test.IsDefault { - t.Errorf("StorageClassIsDefault failed. Expected %v, got %v for %#v", test.IsDefault, result, test.StorageClass) - } + + for _, testCase := range testCases { + //nolint:scopelint + t.Run(testCase.Name, func(t *testing.T) { + // Arrange + var err error + + clientSet := fake.NewSimpleClientset() + storageSet := clientSet.StorageV1() + _, err = storageSet.StorageClasses().Create(&v1.StorageClass{ + TypeMeta: metav1.TypeMeta{}, + ObjectMeta: metav1.ObjectMeta{ + Name: "test", + }, + }) + require.NoError(t, err) + + if testCase.Reactor != nil { + clientSet.PrependReactor(testCase.ReactorActionVerb, resourceName, testCase.Reactor) + } + + // Act + err = PatchStorageClassIsDefault(storageSet, testCase.StorageClassName, true) + + // Assert + if testCase.ExpectedErr != nil { + require.EqualError(t, err, testCase.ExpectedErr.Error()) + return + } + + assert.NoError(t, err) + }) } + }