From e3bfa5c2af1c304950e077449e3d6d8058015ba1 Mon Sep 17 00:00:00 2001 From: Thomas Eckert Date: Thu, 15 Jun 2023 16:29:58 -0400 Subject: [PATCH] api-gateway: fix cache and service deletion issue (#2377) * Fix cache and service deletion issue * Add comments * add in acceptance test * Fix indentation * Fix unit test for deleting gateway w/ consul services * Remove redundant service deregistration code * Exit loop early once registration is found for service * Fix import blocking * Set status on pods added to test * Apply suggestions from code review * Reduce count of test gateways to 10 from 100 --------- Co-authored-by: Nathan Coleman Co-authored-by: Sarah Alsmiller --- acceptance/go.mod | 3 +- acceptance/go.sum | 2 + .../api_gateway_gatewayclassconfig_test.go | 207 ++++++++++++++++++ control-plane/api-gateway/binding/binder.go | 1 + .../api-gateway/binding/binder_test.go | 25 ++- control-plane/api-gateway/cache/gateway.go | 17 -- 6 files changed, 236 insertions(+), 19 deletions(-) create mode 100644 acceptance/tests/api-gateway/api_gateway_gatewayclassconfig_test.go diff --git a/acceptance/go.mod b/acceptance/go.mod index ddce3e4e5c..59cbbab79f 100644 --- a/acceptance/go.mod +++ b/acceptance/go.mod @@ -16,6 +16,7 @@ require ( k8s.io/api v0.26.3 k8s.io/apimachinery v0.26.3 k8s.io/client-go v0.26.3 + k8s.io/utils v0.0.0-20230209194617-a36077c30491 sigs.k8s.io/controller-runtime v0.14.6 sigs.k8s.io/gateway-api v0.7.0 ) @@ -30,6 +31,7 @@ require ( github.com/cespare/xxhash/v2 v2.1.2 // indirect github.com/cpuguy83/go-md2man/v2 v2.0.0 // indirect github.com/davecgh/go-spew v1.1.2-0.20180830191138-d8f796af33cc // indirect + github.com/deckarep/golang-set v1.7.1 // indirect github.com/emicklei/go-restful/v3 v3.9.0 // indirect github.com/evanphx/json-patch v5.6.0+incompatible // indirect github.com/evanphx/json-patch/v5 v5.6.0 // indirect @@ -120,7 +122,6 @@ require ( k8s.io/component-base v0.26.3 // indirect k8s.io/klog/v2 v2.100.1 // indirect k8s.io/kube-openapi v0.0.0-20230501164219-8b0f38b5fd1f // indirect - k8s.io/utils v0.0.0-20230209194617-a36077c30491 // indirect sigs.k8s.io/json v0.0.0-20221116044647-bc3834ca7abd // indirect sigs.k8s.io/structured-merge-diff/v4 v4.2.3 // indirect sigs.k8s.io/yaml v1.3.0 // indirect diff --git a/acceptance/go.sum b/acceptance/go.sum index bd7421de60..578a95b1dd 100644 --- a/acceptance/go.sum +++ b/acceptance/go.sum @@ -144,6 +144,8 @@ github.com/davecgh/go-spew v1.1.0/go.mod h1:J7Y8YcW2NihsgmVo/mv3lAwl/skON4iLHjSs github.com/davecgh/go-spew v1.1.1/go.mod h1:J7Y8YcW2NihsgmVo/mv3lAwl/skON4iLHjSsI+c5H38= github.com/davecgh/go-spew v1.1.2-0.20180830191138-d8f796af33cc h1:U9qPSI2PIWSS1VwoXQT9A3Wy9MM3WgvqSxFWenqJduM= github.com/davecgh/go-spew v1.1.2-0.20180830191138-d8f796af33cc/go.mod h1:J7Y8YcW2NihsgmVo/mv3lAwl/skON4iLHjSsI+c5H38= +github.com/deckarep/golang-set v1.7.1 h1:SCQV0S6gTtp6itiFrTqI+pfmJ4LN85S1YzhDf9rTHJQ= +github.com/deckarep/golang-set v1.7.1/go.mod h1:93vsz/8Wt4joVM7c2AVqh+YRMiUSc14yDtF28KmMOgQ= github.com/dgrijalva/jwt-go v3.2.0+incompatible/go.mod h1:E3ru+11k8xSBh+hMPgOLZmtrrCbhqsmaPHjLKYnJCaQ= github.com/dimchansky/utfbom v1.1.0/go.mod h1:rO41eb7gLfo8SF1jd9F8HplJm1Fewwi4mQvIirEdv+8= github.com/dnaeon/go-vcr v1.0.1/go.mod h1:aBB1+wY4s93YsC3HHjMBMrwTj2R9FHDzUr9KyGc8n1E= diff --git a/acceptance/tests/api-gateway/api_gateway_gatewayclassconfig_test.go b/acceptance/tests/api-gateway/api_gateway_gatewayclassconfig_test.go new file mode 100644 index 0000000000..add65b89af --- /dev/null +++ b/acceptance/tests/api-gateway/api_gateway_gatewayclassconfig_test.go @@ -0,0 +1,207 @@ +// Copyright (c) HashiCorp, Inc. +// SPDX-License-Identifier: MPL-2.0 + +package apigateway + +import ( + "context" + "testing" + + "github.com/hashicorp/consul-k8s/acceptance/framework/consul" + "github.com/hashicorp/consul-k8s/acceptance/framework/helpers" + "github.com/hashicorp/consul-k8s/acceptance/framework/logger" + "github.com/hashicorp/consul-k8s/control-plane/api-gateway/common" + "github.com/hashicorp/consul-k8s/control-plane/api/v1alpha1" + "github.com/hashicorp/consul/api" + "github.com/hashicorp/consul/sdk/testutil/retry" + "github.com/stretchr/testify/require" + appsv1 "k8s.io/api/apps/v1" + corev1 "k8s.io/api/core/v1" + metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" + "k8s.io/apimachinery/pkg/types" + "k8s.io/utils/pointer" + "sigs.k8s.io/controller-runtime/pkg/client" + gwv1beta1 "sigs.k8s.io/gateway-api/apis/v1beta1" +) + +// GatewayClassConfig tests the creation of a gatewayclassconfig object and makes sure that its configuration +// is properly applied to any child gateway objects, namely that the number of gateway instances match the defined +// minInstances,maxInstances and defaultInstances parameters, and that changing the parent gateway does not affect +// the child gateways. +func TestAPIGateway_GatewayClassConfig(t *testing.T) { + ctx := suite.Environment().DefaultContext(t) + cfg := suite.Config() + helmValues := map[string]string{ + "global.logLevel": "trace", + "connectInject.enabled": "true", + } + releaseName := helpers.RandomName() + consulCluster := consul.NewHelmCluster(t, helmValues, ctx, cfg, releaseName) + consulCluster.Create(t) + // Override the default proxy config settings for this test. + consulClient, _ := consulCluster.SetupConsulClient(t, false) + _, _, err := consulClient.ConfigEntries().Set(&api.ProxyConfigEntry{ + Kind: api.ProxyDefaults, + Name: api.ProxyConfigGlobal, + Config: map[string]interface{}{ + "protocol": "http", + }, + }, nil) + require.NoError(t, err) + + k8sClient := ctx.ControllerRuntimeClient(t) + namespace := "gateway-namespace" + + //create clean namespace + err = k8sClient.Create(context.Background(), &corev1.Namespace{ + ObjectMeta: metav1.ObjectMeta{ + Name: namespace, + }, + }) + require.NoError(t, err) + helpers.Cleanup(t, cfg.NoCleanupOnFailure, func() { + logger.Log(t, "deleting gateway namesapce") + k8sClient.Delete(context.Background(), &corev1.Namespace{ + ObjectMeta: metav1.ObjectMeta{ + Name: namespace, + }, + }) + }) + + defaultInstances := pointer.Int32(2) + maxInstances := pointer.Int32(8) + minInstances := pointer.Int32(1) + // create a GatewayClassConfig with configuration set + gatewayClassConfigName := "gateway-class-config" + gatewayClassConfig := &v1alpha1.GatewayClassConfig{ + ObjectMeta: metav1.ObjectMeta{ + Name: gatewayClassConfigName, + }, + Spec: v1alpha1.GatewayClassConfigSpec{ + DeploymentSpec: v1alpha1.DeploymentSpec{ + DefaultInstances: defaultInstances, + MaxInstances: maxInstances, + MinInstances: minInstances, + }, + }, + } + logger.Log(t, "creating gateway class config") + err = k8sClient.Create(context.Background(), gatewayClassConfig) + require.NoError(t, err) + helpers.Cleanup(t, cfg.NoCleanupOnFailure, func() { + logger.Log(t, "deleting all gateway class configs") + k8sClient.DeleteAllOf(context.Background(), &v1alpha1.GatewayClassConfig{}) + }) + + gatewayParametersRef := &gwv1beta1.ParametersReference{ + Group: gwv1beta1.Group(v1alpha1.ConsulHashicorpGroup), + Kind: gwv1beta1.Kind(v1alpha1.GatewayClassConfigKind), + Name: gatewayClassConfigName, + } + + // create gateway class referencing gateway-class-config + gatewayClassName := "gateway-class" + logger.Log(t, "creating controlled gateway class") + createGatewayClass(t, k8sClient, gatewayClassName, gatewayClassControllerName, gatewayParametersRef) + helpers.Cleanup(t, cfg.NoCleanupOnFailure, func() { + logger.Log(t, "deleting all gateway classes") + k8sClient.DeleteAllOf(context.Background(), &gwv1beta1.GatewayClass{}) + }) + + // Create a certificate to reference in listeners + certificateInfo := generateCertificate(t, nil, "certificate.consul.local") + certificateName := "certificate" + certificate := &corev1.Secret{ + ObjectMeta: metav1.ObjectMeta{ + Name: certificateName, + Namespace: namespace, + Labels: map[string]string{ + "test-certificate": "true", + }, + }, + Type: corev1.SecretTypeTLS, + Data: map[string][]byte{ + corev1.TLSCertKey: certificateInfo.CertPEM, + corev1.TLSPrivateKeyKey: certificateInfo.PrivateKeyPEM, + }, + } + logger.Log(t, "creating certificate") + err = k8sClient.Create(context.Background(), certificate) + require.NoError(t, err) + helpers.Cleanup(t, cfg.NoCleanupOnFailure, func() { + k8sClient.Delete(context.Background(), certificate) + }) + + // Create gateway referencing gateway class config + gatewayName := "gateway" + logger.Log(t, "creating controlled gateway") + gateway := createGateway(t, k8sClient, gatewayName, namespace, gatewayClassName, certificateName) + // make sure it exists + logger.Log(t, "checking that gateway one is synchronized to Consul") + checkConsulExists(t, consulClient, api.APIGateway, gatewayName) + + helpers.Cleanup(t, cfg.NoCleanupOnFailure, func() { + logger.Log(t, "deleting all gateways") + k8sClient.DeleteAllOf(context.Background(), &gwv1beta1.Gateway{}, client.InNamespace(namespace)) + }) + + // Scenario: Gateway deployment should match the default instances defined on the gateway class config + logger.Log(t, "checking that gateway instances match defined gateway class config") + checkNumberOfInstances(t, k8sClient, consulClient, gateway.Name, gateway.Namespace, defaultInstances, gateway) + + //Scenario: Updating the GatewayClassConfig should not affect gateways that have already been created + logger.Log(t, "updating gatewayclassconfig values") + err = k8sClient.Get(context.Background(), types.NamespacedName{Name: gatewayClassConfigName, Namespace: namespace}, gatewayClassConfig) + require.NoError(t, err) + gatewayClassConfig.Spec.DeploymentSpec.DefaultInstances = pointer.Int32(8) + gatewayClassConfig.Spec.DeploymentSpec.MinInstances = pointer.Int32(5) + err = k8sClient.Update(context.Background(), gatewayClassConfig) + require.NoError(t, err) + checkNumberOfInstances(t, k8sClient, consulClient, gateway.Name, gateway.Namespace, defaultInstances, gateway) + + //Scenario: gateways should be able to scale independently and not get overridden by the controller unless it's above the max + scale(t, k8sClient, gateway.Name, gateway.Namespace, maxInstances) + checkNumberOfInstances(t, k8sClient, consulClient, gateway.Name, gateway.Namespace, maxInstances, gateway) + scale(t, k8sClient, gateway.Name, gateway.Namespace, pointer.Int32(10)) + checkNumberOfInstances(t, k8sClient, consulClient, gateway.Name, gateway.Namespace, maxInstances, gateway) + scale(t, k8sClient, gateway.Name, gateway.Namespace, pointer.Int32(0)) + checkNumberOfInstances(t, k8sClient, consulClient, gateway.Name, gateway.Namespace, minInstances, gateway) + +} + +func scale(t *testing.T, client client.Client, name, namespace string, scaleTo *int32) { + t.Helper() + + retryCheck(t, 30, func(r *retry.R) { + var deployment appsv1.Deployment + err := client.Get(context.Background(), types.NamespacedName{Name: name, Namespace: namespace}, &deployment) + require.NoError(r, err) + + deployment.Spec.Replicas = scaleTo + err = client.Update(context.Background(), &deployment) + require.NoError(r, err) + }) +} + +func checkNumberOfInstances(t *testing.T, k8client client.Client, consulClient *api.Client, name, namespace string, wantNumber *int32, gateway *gwv1beta1.Gateway) { + t.Helper() + + retryCheck(t, 30, func(r *retry.R) { + //first check to make sure the number of replicas has been set properly + var deployment appsv1.Deployment + err := k8client.Get(context.Background(), types.NamespacedName{Name: name, Namespace: namespace}, &deployment) + require.NoError(r, err) + require.EqualValues(r, *wantNumber, *deployment.Spec.Replicas) + + //then check to make sure the number of gateway pods matches the replicas generated + podList := corev1.PodList{} + labels := common.LabelsForGateway(gateway) + err = k8client.List(context.Background(), &podList, client.InNamespace(namespace), client.MatchingLabels(labels)) + require.NoError(r, err) + require.EqualValues(r, *wantNumber, len(podList.Items)) + + services, _, err := consulClient.Catalog().Service(name, "", nil) + require.NoError(r, err) + require.EqualValues(r, *wantNumber, len(services)) + }) +} diff --git a/control-plane/api-gateway/binding/binder.go b/control-plane/api-gateway/binding/binder.go index 28a26985a8..7fbf18d412 100644 --- a/control-plane/api-gateway/binding/binder.go +++ b/control-plane/api-gateway/binding/binder.go @@ -197,6 +197,7 @@ func (b *Binder) Snapshot() *Snapshot { for _, registration := range registrations { if service.ServiceID == registration.Service.ID { found = true + break } } if !found { diff --git a/control-plane/api-gateway/binding/binder_test.go b/control-plane/api-gateway/binding/binder_test.go index a3af702dab..bb30b98f52 100644 --- a/control-plane/api-gateway/binding/binder_test.go +++ b/control-plane/api-gateway/binding/binder_test.go @@ -15,6 +15,7 @@ import ( logrtest "github.com/go-logr/logr/testing" "github.com/google/go-cmp/cmp" + "github.com/hashicorp/consul/api" "github.com/stretchr/testify/require" corev1 "k8s.io/api/core/v1" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" @@ -25,7 +26,6 @@ import ( "github.com/hashicorp/consul-k8s/control-plane/api-gateway/common" "github.com/hashicorp/consul-k8s/control-plane/api/v1alpha1" - "github.com/hashicorp/consul/api" ) func init() { @@ -809,6 +809,29 @@ func TestBinder_Registrations(t *testing.T) { {Node: "test", ServiceID: "pod2", Namespace: "namespace1"}, {Node: "test", ServiceID: "pod3", Namespace: "namespace1"}, }, + Pods: []corev1.Pod{ + { + ObjectMeta: metav1.ObjectMeta{Name: "pod1"}, + Status: corev1.PodStatus{ + Phase: corev1.PodRunning, + Conditions: []corev1.PodCondition{{Type: corev1.PodReady, Status: corev1.ConditionTrue}}, + }, + }, + { + ObjectMeta: metav1.ObjectMeta{Name: "pod2"}, + Status: corev1.PodStatus{ + Phase: corev1.PodRunning, + Conditions: []corev1.PodCondition{{Type: corev1.PodReady, Status: corev1.ConditionTrue}}, + }, + }, + { + ObjectMeta: metav1.ObjectMeta{Name: "pod3"}, + Status: corev1.PodStatus{ + Phase: corev1.PodRunning, + Conditions: []corev1.PodCondition{{Type: corev1.PodReady, Status: corev1.ConditionTrue}}, + }, + }, + }, }), expectedDeregistrations: []api.CatalogDeregistration{ {Node: "test", ServiceID: "pod1", Namespace: "namespace1"}, diff --git a/control-plane/api-gateway/cache/gateway.go b/control-plane/api-gateway/cache/gateway.go index d1de8dd7cc..0d79542eec 100644 --- a/control-plane/api-gateway/cache/gateway.go +++ b/control-plane/api-gateway/cache/gateway.go @@ -14,7 +14,6 @@ import ( "github.com/hashicorp/consul-k8s/control-plane/consul" "github.com/hashicorp/consul/api" "k8s.io/apimachinery/pkg/types" - "sigs.k8s.io/controller-runtime/pkg/event" ) type GatewayCache struct { @@ -22,8 +21,6 @@ type GatewayCache struct { serverMgr consul.ServerConnectionManager logger logr.Logger - events chan event.GenericEvent - data map[api.ResourceReference][]api.CatalogService dataMutex sync.RWMutex @@ -38,7 +35,6 @@ func NewGatewayCache(ctx context.Context, config Config) *GatewayCache { config: config, serverMgr: config.ConsulServerConnMgr, logger: config.Logger, - events: make(chan event.GenericEvent), data: make(map[api.ResourceReference][]api.CatalogService), subscribedGateways: make(map[api.ResourceReference]context.CancelFunc), ctx: ctx, @@ -140,18 +136,5 @@ func (r *GatewayCache) subscribeToGateway(ctx context.Context, ref api.ResourceR r.dataMutex.Lock() r.data[common.NormalizeMeta(ref)] = derefed r.dataMutex.Unlock() - - event := event.GenericEvent{ - Object: newConfigEntryObject(resource), - } - - select { - case <-ctx.Done(): - r.dataMutex.Lock() - delete(r.data, ref) - r.dataMutex.Unlock() - return - case r.events <- event: - } } }