From 34bba70be6f3994f2e38ff0a92e9cb27a74ccf32 Mon Sep 17 00:00:00 2001 From: Nathan Coleman Date: Tue, 29 Nov 2022 11:04:31 -0500 Subject: [PATCH 1/3] Treat ResourceVersion as opaque string instead of integer The Kubernetes API explicitly disallows parsing the ResourceVersion as an integer https://kubernetes.io/docs/reference/using-api/api-concepts/#resource-versions --- internal/k8s/reconciler/gatewayclass.go | 5 ++--- internal/k8s/reconciler/manager.go | 4 ++-- internal/k8s/utils/versions.go | 17 ----------------- 3 files changed, 4 insertions(+), 22 deletions(-) delete mode 100644 internal/k8s/utils/versions.go diff --git a/internal/k8s/reconciler/gatewayclass.go b/internal/k8s/reconciler/gatewayclass.go index b3d4b37ac..41c1194af 100644 --- a/internal/k8s/reconciler/gatewayclass.go +++ b/internal/k8s/reconciler/gatewayclass.go @@ -11,7 +11,6 @@ import ( "github.com/hashicorp/consul-api-gateway/internal/k8s/gatewayclient" rstatus "github.com/hashicorp/consul-api-gateway/internal/k8s/reconciler/status" - "github.com/hashicorp/consul-api-gateway/internal/k8s/utils" apigwv1alpha1 "github.com/hashicorp/consul-api-gateway/pkg/apis/v1alpha1" ) @@ -51,8 +50,8 @@ func (g *K8sGatewayClasses) Upsert(ctx context.Context, class *K8sGatewayClass) g.mutex.Lock() defer g.mutex.Unlock() - if current, ok := g.gatewayClasses[class.class.Name]; ok { - if utils.ResourceVersionGreater(current.class.ResourceVersion, class.class.ResourceVersion) { + if cachedClass, ok := g.gatewayClasses[class.class.Name]; ok { + if cachedClass.class.ResourceVersion == class.class.ResourceVersion { // we have an old gatewayclass update ignore return nil } diff --git a/internal/k8s/reconciler/manager.go b/internal/k8s/reconciler/manager.go index 51defc338..023ae72b6 100644 --- a/internal/k8s/reconciler/manager.go +++ b/internal/k8s/reconciler/manager.go @@ -197,7 +197,7 @@ func (m *GatewayReconcileManager) UpsertGateway(ctx context.Context, g *gwv1beta if current == nil { return true } - return !utils.ResourceVersionGreater(current.(*K8sGateway).ResourceVersion, gateway.ResourceVersion) + return current.(*K8sGateway).ResourceVersion != gateway.ResourceVersion }) } @@ -235,7 +235,7 @@ func (m *GatewayReconcileManager) upsertRoute(ctx context.Context, r Route, pare if current == nil { return true } - return !utils.ResourceVersionGreater(current.(*K8sRoute).GetResourceVersion(), route.GetResourceVersion()) + return current.(*K8sRoute).GetResourceVersion() != route.GetResourceVersion() }) } diff --git a/internal/k8s/utils/versions.go b/internal/k8s/utils/versions.go deleted file mode 100644 index 561c20fd1..000000000 --- a/internal/k8s/utils/versions.go +++ /dev/null @@ -1,17 +0,0 @@ -package utils - -import "strconv" - -func ResourceVersionGreater(a, b string) bool { - aVal, err := strconv.Atoi(a) - if err != nil { - // a isn't numeric, return that b is greater - return false - } - bVal, err := strconv.Atoi(b) - if err != nil { - // b isn't numeric, return that a is greater - return true - } - return aVal > bVal -} From 35c453aff77c73b609d1d8e110143d6276ba2c91 Mon Sep 17 00:00:00 2001 From: Nathan Coleman Date: Tue, 29 Nov 2022 11:04:50 -0500 Subject: [PATCH 2/3] Update unit tests for ReconcileManager --- internal/k8s/reconciler/manager_test.go | 45 ++++++++++++++++--------- 1 file changed, 29 insertions(+), 16 deletions(-) diff --git a/internal/k8s/reconciler/manager_test.go b/internal/k8s/reconciler/manager_test.go index 0b221e6b4..29dbe2f0c 100644 --- a/internal/k8s/reconciler/manager_test.go +++ b/internal/k8s/reconciler/manager_test.go @@ -6,7 +6,10 @@ import ( "testing" "github.com/golang/mock/gomock" + "github.com/google/uuid" + "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" + v1 "k8s.io/apimachinery/pkg/apis/meta/v1" "k8s.io/apimachinery/pkg/types" gwv1alpha2 "sigs.k8s.io/gateway-api/apis/v1alpha2" gwv1beta1 "sigs.k8s.io/gateway-api/apis/v1beta1" @@ -34,24 +37,34 @@ func TestUpsertGatewayClass(t *testing.T) { Logger: hclog.NewNullLogger(), }) - inner := &gwv1beta1.GatewayClass{} - expected := errors.New("expected") - client.EXPECT().UpdateStatus(gomock.Any(), inner).Return(expected) - require.Equal(t, expected, manager.UpsertGatewayClass(context.Background(), inner)) + t.Run("error validating GatewayClassConfig", func(t *testing.T) { + inner := &gwv1beta1.GatewayClass{ + Spec: gwv1beta1.GatewayClassSpec{ + ParametersRef: &gwv1beta1.ParametersReference{ + Group: apigwv1alpha1.Group, + Kind: apigwv1alpha1.GatewayClassConfigKind, + }, + }, + } + expected := errors.New("expected") + client.EXPECT().GetGatewayClassConfig(gomock.Any(), gomock.Any()).Return(nil, expected) - client.EXPECT().UpdateStatus(gomock.Any(), inner) - require.NoError(t, manager.UpsertGatewayClass(context.Background(), inner)) + err := manager.UpsertGatewayClass(context.Background(), inner) + assert.EqualError(t, err, expected.Error()) + }) - // validation - client.EXPECT().GetGatewayClassConfig(gomock.Any(), gomock.Any()).Return(nil, expected) - require.Equal(t, expected, manager.UpsertGatewayClass(context.Background(), &gwv1beta1.GatewayClass{ - Spec: gwv1beta1.GatewayClassSpec{ - ParametersRef: &gwv1beta1.ParametersReference{ - Group: apigwv1alpha1.Group, - Kind: apigwv1alpha1.GatewayClassConfigKind, - }, - }, - })) + t.Run("error updating status", func(t *testing.T) { + inner := &gwv1beta1.GatewayClass{ObjectMeta: v1.ObjectMeta{ResourceVersion: uuid.NewString()}} + expected := errors.New("expected") + client.EXPECT().UpdateStatus(gomock.Any(), inner).Return(expected) + assert.EqualError(t, manager.UpsertGatewayClass(context.Background(), inner), expected.Error()) + }) + + t.Run("successful update", func(t *testing.T) { + inner := &gwv1beta1.GatewayClass{ObjectMeta: v1.ObjectMeta{ResourceVersion: uuid.NewString()}} + client.EXPECT().UpdateStatus(gomock.Any(), inner) + assert.NoError(t, manager.UpsertGatewayClass(context.Background(), inner)) + }) } func TestUpsertGateway(t *testing.T) { From be3c62443a434d10dbce8c46028f88f8314066b3 Mon Sep 17 00:00:00 2001 From: Nathan Coleman Date: Tue, 29 Nov 2022 11:41:08 -0500 Subject: [PATCH 3/3] Update unit tests for K8sGatewayClasses --- internal/k8s/reconciler/gatewayclass_test.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/internal/k8s/reconciler/gatewayclass_test.go b/internal/k8s/reconciler/gatewayclass_test.go index dd2135a8b..0d0b3cae9 100644 --- a/internal/k8s/reconciler/gatewayclass_test.go +++ b/internal/k8s/reconciler/gatewayclass_test.go @@ -119,7 +119,7 @@ func TestGatewayClasses(t *testing.T) { gatewayClass = NewK8sGatewayClass(&gwv1beta1.GatewayClass{ ObjectMeta: meta.ObjectMeta{ - ResourceVersion: "0", + ResourceVersion: "1", }, Spec: gwv1beta1.GatewayClassSpec{ ParametersRef: &gwv1beta1.ParametersReference{