From 958a9d6cf6efa20633aba6e4a443ff992133764e Mon Sep 17 00:00:00 2001 From: Mike Morris Date: Wed, 4 Jan 2023 15:06:42 -0500 Subject: [PATCH 01/10] k8s/controller: always map "default" Consul namespace to empty string --- internal/k8s/controller.go | 14 ++++++++++++-- 1 file changed, 12 insertions(+), 2 deletions(-) diff --git a/internal/k8s/controller.go b/internal/k8s/controller.go index 48c92a2d7..855bbceb4 100644 --- a/internal/k8s/controller.go +++ b/internal/k8s/controller.go @@ -56,10 +56,20 @@ type ConsulNamespaceConfig struct { } func (c ConsulNamespaceConfig) Namespace(namespace string) string { + var consulNamespace string + if c.MirrorKubernetesNamespaces { - return c.MirrorKubernetesNamespacePrefix + namespace + consulNamespace = c.MirrorKubernetesNamespacePrefix + namespace + } else { + consulNamespace = c.ConsulDestinationNamespace + } + + // Always map the default namespace to "" for compatibility with Consul OSS + if consulNamespace == "default" { + consulNamespace = "" } - return c.ConsulDestinationNamespace + + return consulNamespace } type Kubernetes struct { From 0a7bc443e0deca90e45cb0f5a18824fefd130cf6 Mon Sep 17 00:00:00 2001 From: Mike Morris Date: Thu, 5 Jan 2023 16:58:54 -0500 Subject: [PATCH 02/10] e2e: add test for Consul desintation namespace without k8s namespace mirroring --- internal/commands/server/k8s_e2e_test.go | 59 ++++++++++++++++++++++++ internal/testing/e2e/consul.go | 50 +++++++++++--------- internal/testing/e2e/environment.go | 13 ++++++ internal/testing/e2e/gateway.go | 2 +- internal/testing/e2e/stack.go | 3 +- 5 files changed, 103 insertions(+), 24 deletions(-) diff --git a/internal/commands/server/k8s_e2e_test.go b/internal/commands/server/k8s_e2e_test.go index a7d73d7a5..fd3438301 100644 --- a/internal/commands/server/k8s_e2e_test.go +++ b/internal/commands/server/k8s_e2e_test.go @@ -116,6 +116,65 @@ func TestGatewayWithClassConfigChange(t *testing.T) { testenv.Test(t, feature.Feature()) } +func TestGatewayWithoutNamespaceMirroring(t *testing.T) { + feature := features.New("gateway admission"). + Assess("gateway sync without namespace mirroring", func(ctx context.Context, t *testing.T, cfg *envconf.Config) context.Context { + namespace := e2e.Namespace(ctx) + resources := cfg.Client().Resources(namespace) + + // Disable namespace mirroring + ctx, err := e2e.SetNamespaceMirroring(false)(ctx, nil) + require.NoError(t, err) + + useHostPorts := false + gcc, gc := createGatewayClassWithParams(ctx, t, resources, GatewayClassConfigParams{ + UseHostPorts: &useHostPorts, + }) + require.Eventually(t, gatewayClassStatusCheck(ctx, resources, gc.Name, namespace, conditionAccepted), checkTimeout, checkInterval, "gatewayclass not accepted in the allotted time") + + // Create an HTTPS Gateway Listener to pass when creating gateways + httpsListener := createHTTPSListener(ctx, t, 443) + + // Create a Gateway and wait for it to be ready + // This will attempt to sync to a randomly generated Consul desintation namespace + firstGatewayName := envconf.RandomName("gw", 16) + firstGateway := createGateway(ctx, t, resources, firstGatewayName, namespace, gc, []gwv1beta1.Listener{httpsListener}) + require.Eventually(t, gatewayStatusCheck(ctx, resources, firstGatewayName, namespace, conditionReady), checkTimeout, checkInterval, "no gateway found in the allotted time") + checkGatewayConfigAnnotation(ctx, t, resources, firstGatewayName, namespace, gcc) + + // Set a different Consul destination namespace + defaultNamespace := "" + ctx, err = e2e.SetConsulNamespace(&defaultNamespace)(ctx, nil) + require.NoError(t, err) + + // Create a second Gateway and wait for it to be ready + secondGatewayName := envconf.RandomName("gw", 16) + secondGateway := createGateway(ctx, t, resources, secondGatewayName, namespace, gc, []gwv1beta1.Listener{httpsListener}) + require.Eventually(t, gatewayStatusCheck(ctx, resources, secondGatewayName, namespace, conditionReady), checkTimeout, checkInterval, "no gateway found in the allotted time") + checkGatewayConfigAnnotation(ctx, t, resources, secondGatewayName, namespace, gcc) + + // Set a different Consul destination namespace + defaultEnterpriseNamespace := "default" + ctx, err = e2e.SetConsulNamespace(&defaultEnterpriseNamespace)(ctx, nil) + require.NoError(t, err) + + // Create a third Gateway and wait for it to be ready + thirdGatewayName := envconf.RandomName("gw", 16) + thirdGateway := createGateway(ctx, t, resources, thirdGatewayName, namespace, gc, []gwv1beta1.Listener{httpsListener}) + require.Eventually(t, gatewayStatusCheck(ctx, resources, thirdGatewayName, namespace, conditionReady), checkTimeout, checkInterval, "no gateway found in the allotted time") + checkGatewayConfigAnnotation(ctx, t, resources, thirdGatewayName, namespace, gcc) + + // Cleanup + assert.NoError(t, resources.Delete(ctx, firstGateway)) + assert.NoError(t, resources.Delete(ctx, secondGateway)) + assert.NoError(t, resources.Delete(ctx, thirdGateway)) + + return ctx + }) + + testenv.Test(t, feature.Feature()) +} + func TestGatewayWithReplicas(t *testing.T) { feature := features.New("gateway class config configure instances"). Assess("gateway is created with appropriate number of replicas set", func(ctx context.Context, t *testing.T, cfg *envconf.Config) context.Context { diff --git a/internal/testing/e2e/consul.go b/internal/testing/e2e/consul.go index 4de4bb230..497e9feb9 100644 --- a/internal/testing/e2e/consul.go +++ b/internal/testing/e2e/consul.go @@ -510,14 +510,15 @@ func ConsulHTTPPort(ctx context.Context) int { return mustGetTestEnvironment(ctx).httpPort } -func isConsulNamespaceMirroringOn() bool { - return IsEnterprise() +func isConsulNamespaceMirroringOn(ctx context.Context) bool { + return IsEnterprise() && NamespaceMirroring(ctx) } func ConsulNamespace(ctx context.Context) string { - if isConsulNamespaceMirroringOn() { - //assume mirroring is on + if isConsulNamespaceMirroringOn(ctx) { return Namespace(ctx) } + + // Return Consul namespace return mustGetTestEnvironment(ctx).namespace } @@ -588,27 +589,32 @@ func IsEnterprise() bool { return strings.HasSuffix(consulImage, "ent") } -func CreateConsulNamespace(ctx context.Context, cfg *envconf.Config) (context.Context, error) { - if IsEnterprise() { - log.Print("Creating Consul Namespace") - namespace := envconf.RandomName("test", 16) +func SetConsulNamespace(namespace *string) env.Func { + return func(ctx context.Context, _ *envconf.Config) (context.Context, error) { + if IsEnterprise() { + log.Print("Creating Consul Namespace") + if namespace == nil { + *namespace = envconf.RandomName("consul", 16) + } - consulEnvironment := ctx.Value(consulTestContextKey) - if consulEnvironment == nil { - return ctx, nil - } - env := consulEnvironment.(*consulTestEnvironment) - _, _, err := env.consulClient.Namespaces().Create(&api.Namespace{ - Name: namespace, - }, &api.WriteOptions{ - Token: env.token, - }) - if err != nil { - return nil, err + consulEnvironment := ctx.Value(consulTestContextKey) + if consulEnvironment == nil { + return ctx, nil + } + env := consulEnvironment.(*consulTestEnvironment) + _, _, err := env.consulClient.Namespaces().Create(&api.Namespace{ + Name: *namespace, + }, &api.WriteOptions{ + Token: env.token, + }) + // TODO: ignore error if namespace already exists + if err != nil { + return nil, err + } + env.namespace = *namespace } - env.namespace = namespace + return ctx, nil } - return ctx, nil } func gatewayConsulAuthMethod(name, token string, k8sConfig *rest.Config) *api.ACLAuthMethod { diff --git a/internal/testing/e2e/environment.go b/internal/testing/e2e/environment.go index 1c0c8e078..1e62fecf6 100644 --- a/internal/testing/e2e/environment.go +++ b/internal/testing/e2e/environment.go @@ -11,9 +11,11 @@ import ( ) type namespaceContext struct{} +type namespaceMirroringContext struct{} type clusterNameContext struct{} var namespaceContextKey = namespaceContext{} +var namespaceMirroringContextKey = namespaceMirroringContext{} var clusterNameContextKey = clusterNameContext{} func SetNamespace(namespace string) env.Func { @@ -22,6 +24,12 @@ func SetNamespace(namespace string) env.Func { } } +func SetNamespaceMirroring(namespaceMirroring bool) env.Func { + return func(ctx context.Context, cfg *envconf.Config) (context.Context, error) { + return context.WithValue(ctx, namespaceMirroringContextKey, namespaceMirroring), nil + } +} + func Namespace(ctx context.Context) string { namespace := ctx.Value(namespaceContextKey) if namespace == nil { @@ -30,6 +38,11 @@ func Namespace(ctx context.Context) string { return namespace.(string) } +func NamespaceMirroring(ctx context.Context) bool { + namespaceMirroring := ctx.Value(namespaceMirroringContextKey) + return namespaceMirroring.(bool) +} + func SetClusterName(name string) env.Func { return func(ctx context.Context, cfg *envconf.Config) (context.Context, error) { return context.WithValue(ctx, clusterNameContextKey, name), nil diff --git a/internal/testing/e2e/gateway.go b/internal/testing/e2e/gateway.go index c0c074417..46f4a5c4b 100644 --- a/internal/testing/e2e/gateway.go +++ b/internal/testing/e2e/gateway.go @@ -64,7 +64,7 @@ func (p *gatewayTestEnvironment) run(ctx context.Context, namespace string, cfg CACert: ConsulCA(ctx), ConsulNamespaceConfig: k8s.ConsulNamespaceConfig{ ConsulDestinationNamespace: ConsulNamespace(ctx), - MirrorKubernetesNamespaces: isConsulNamespaceMirroringOn(), + MirrorKubernetesNamespaces: isConsulNamespaceMirroringOn(ctx), }, } diff --git a/internal/testing/e2e/stack.go b/internal/testing/e2e/stack.go index 8daaf56f3..8eb27507e 100644 --- a/internal/testing/e2e/stack.go +++ b/internal/testing/e2e/stack.go @@ -28,6 +28,7 @@ func SetUpStack(hostRoute string) env.Func { for _, f := range []env.Func{ SetClusterName(kindClusterName), SetNamespace(namespace), + SetNamespaceMirroring(true), CrossCompileProject, BuildDockerImage, CreateKindCluster(kindClusterName), @@ -38,7 +39,7 @@ func SetUpStack(hostRoute string) env.Func { CreateTestConsulContainer(kindClusterName, namespace), CreateConsulACLPolicy, CreateConsulAuthMethod(), - CreateConsulNamespace, + SetConsulNamespace(nil), CreateTestGatewayServer(namespace), } { ctx, err = f(ctx, cfg) From b7e77c05cd641841b191afd764432080e2e84e0a Mon Sep 17 00:00:00 2001 From: Mike Morris Date: Thu, 5 Jan 2023 17:36:10 -0500 Subject: [PATCH 03/10] add changelog entry --- .changelog/483.txt | 3 +++ 1 file changed, 3 insertions(+) create mode 100644 .changelog/483.txt diff --git a/.changelog/483.txt b/.changelog/483.txt new file mode 100644 index 000000000..7b9c6cb0e --- /dev/null +++ b/.changelog/483.txt @@ -0,0 +1,3 @@ +```release-note:bug +consul: fix Consul Enterprise gateway sync issue with Kubernetes namespace mirroring disabled and the Consul destination namespace set to "default" +``` From f52be4658c190536142fe9e6dfa26440dab5e7d3 Mon Sep 17 00:00:00 2001 From: Mike Morris Date: Thu, 5 Jan 2023 17:41:19 -0500 Subject: [PATCH 04/10] fixup nil pointer deref --- internal/testing/e2e/consul.go | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/internal/testing/e2e/consul.go b/internal/testing/e2e/consul.go index 497e9feb9..22e218807 100644 --- a/internal/testing/e2e/consul.go +++ b/internal/testing/e2e/consul.go @@ -594,7 +594,8 @@ func SetConsulNamespace(namespace *string) env.Func { if IsEnterprise() { log.Print("Creating Consul Namespace") if namespace == nil { - *namespace = envconf.RandomName("consul", 16) + name := envconf.RandomName("consul", 16) + namespace = &name } consulEnvironment := ctx.Value(consulTestContextKey) From 4a0263bce70298d123ab211c6d11a0a374836b4f Mon Sep 17 00:00:00 2001 From: Mike Morris Date: Fri, 6 Jan 2023 10:57:07 -0500 Subject: [PATCH 05/10] fixup: skip trying to create the empty string namespace --- internal/testing/e2e/consul.go | 22 +++++++++++++--------- 1 file changed, 13 insertions(+), 9 deletions(-) diff --git a/internal/testing/e2e/consul.go b/internal/testing/e2e/consul.go index 22e218807..4ae044caf 100644 --- a/internal/testing/e2e/consul.go +++ b/internal/testing/e2e/consul.go @@ -602,15 +602,19 @@ func SetConsulNamespace(namespace *string) env.Func { if consulEnvironment == nil { return ctx, nil } - env := consulEnvironment.(*consulTestEnvironment) - _, _, err := env.consulClient.Namespaces().Create(&api.Namespace{ - Name: *namespace, - }, &api.WriteOptions{ - Token: env.token, - }) - // TODO: ignore error if namespace already exists - if err != nil { - return nil, err + // Passing an empty string for namespace name returns a Consul API error, + // and the default namespace should always exist + if *namespace != "" { + env := consulEnvironment.(*consulTestEnvironment) + _, _, err := env.consulClient.Namespaces().Create(&api.Namespace{ + Name: *namespace, + }, &api.WriteOptions{ + Token: env.token, + }) + // TODO: ignore error if namespace already exists + if err != nil { + return nil, err + } } env.namespace = *namespace } From ccf66469ee9ab9f858662aa9dc930436692a0678 Mon Sep 17 00:00:00 2001 From: Mike Morris Date: Fri, 6 Jan 2023 10:58:07 -0500 Subject: [PATCH 06/10] fixup: env scoping --- internal/testing/e2e/consul.go | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/internal/testing/e2e/consul.go b/internal/testing/e2e/consul.go index 4ae044caf..f415c0fff 100644 --- a/internal/testing/e2e/consul.go +++ b/internal/testing/e2e/consul.go @@ -602,10 +602,11 @@ func SetConsulNamespace(namespace *string) env.Func { if consulEnvironment == nil { return ctx, nil } + env := consulEnvironment.(*consulTestEnvironment) + // Passing an empty string for namespace name returns a Consul API error, // and the default namespace should always exist if *namespace != "" { - env := consulEnvironment.(*consulTestEnvironment) _, _, err := env.consulClient.Namespaces().Create(&api.Namespace{ Name: *namespace, }, &api.WriteOptions{ @@ -616,6 +617,7 @@ func SetConsulNamespace(namespace *string) env.Func { return nil, err } } + env.namespace = *namespace } return ctx, nil From 3dce084b0fcc1f32e962ade1a5391fd50ae301c2 Mon Sep 17 00:00:00 2001 From: Mike Morris Date: Fri, 6 Jan 2023 12:35:05 -0500 Subject: [PATCH 07/10] re-enable namespace mirroring in cleanup --- internal/commands/server/k8s_e2e_test.go | 8 ++++++++ internal/testing/e2e/consul.go | 1 - internal/testing/e2e/stack.go | 2 ++ 3 files changed, 10 insertions(+), 1 deletion(-) diff --git a/internal/commands/server/k8s_e2e_test.go b/internal/commands/server/k8s_e2e_test.go index fd3438301..f7609ee25 100644 --- a/internal/commands/server/k8s_e2e_test.go +++ b/internal/commands/server/k8s_e2e_test.go @@ -169,6 +169,10 @@ func TestGatewayWithoutNamespaceMirroring(t *testing.T) { assert.NoError(t, resources.Delete(ctx, secondGateway)) assert.NoError(t, resources.Delete(ctx, thirdGateway)) + // Re-enable namespace mirroring + ctx, err := e2e.SetNamespaceMirroring(true)(ctx, nil) + require.NoError(t, err) + return ctx }) @@ -344,6 +348,10 @@ func TestGatewayBasic(t *testing.T) { // check for the service being registered client := e2e.ConsulClient(ctx) + t.Log("k8s namespace:", e2e.Namespace(ctx) + t.Log("consul namespace:", e2e.ConsulNamespace(ctx) + t.Log("mirroring:", e2e.NamespaceMirroring(ctx) + require.Eventually(t, func() bool { services, _, err := client.Catalog().Service(gatewayName, "", &api.QueryOptions{ Namespace: e2e.ConsulNamespace(ctx), diff --git a/internal/testing/e2e/consul.go b/internal/testing/e2e/consul.go index f415c0fff..3705473bc 100644 --- a/internal/testing/e2e/consul.go +++ b/internal/testing/e2e/consul.go @@ -612,7 +612,6 @@ func SetConsulNamespace(namespace *string) env.Func { }, &api.WriteOptions{ Token: env.token, }) - // TODO: ignore error if namespace already exists if err != nil { return nil, err } diff --git a/internal/testing/e2e/stack.go b/internal/testing/e2e/stack.go index 8eb27507e..a7d9a4164 100644 --- a/internal/testing/e2e/stack.go +++ b/internal/testing/e2e/stack.go @@ -23,6 +23,8 @@ func SetUpStack(hostRoute string) env.Func { kindClusterName := envconf.RandomName("consul-api-gateway-test", 30) namespace := envconf.RandomName("test", 16) + // TODO: should this instead create a new context? + ctx = SetHostRoute(ctx, hostRoute) for _, f := range []env.Func{ From 6ee5e09734fc9a4b49f78e205b8676a9ea3fb8c7 Mon Sep 17 00:00:00 2001 From: Mike Morris Date: Fri, 6 Jan 2023 12:36:17 -0500 Subject: [PATCH 08/10] fixup: closing parens --- internal/commands/server/k8s_e2e_test.go | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/internal/commands/server/k8s_e2e_test.go b/internal/commands/server/k8s_e2e_test.go index f7609ee25..755459d58 100644 --- a/internal/commands/server/k8s_e2e_test.go +++ b/internal/commands/server/k8s_e2e_test.go @@ -348,9 +348,9 @@ func TestGatewayBasic(t *testing.T) { // check for the service being registered client := e2e.ConsulClient(ctx) - t.Log("k8s namespace:", e2e.Namespace(ctx) - t.Log("consul namespace:", e2e.ConsulNamespace(ctx) - t.Log("mirroring:", e2e.NamespaceMirroring(ctx) + t.Log("k8s namespace:", e2e.Namespace(ctx)) + t.Log("consul namespace:", e2e.ConsulNamespace(ctx)) + t.Log("mirroring:", e2e.NamespaceMirroring(ctx)) require.Eventually(t, func() bool { services, _, err := client.Catalog().Service(gatewayName, "", &api.QueryOptions{ From 7bb5ffb1295e60569732937e23ea7ea83fb97b7d Mon Sep 17 00:00:00 2001 From: Mike Morris Date: Fri, 6 Jan 2023 12:37:50 -0500 Subject: [PATCH 09/10] remove unnecessary init in assignment --- internal/commands/server/k8s_e2e_test.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/internal/commands/server/k8s_e2e_test.go b/internal/commands/server/k8s_e2e_test.go index 755459d58..cfb9c78d7 100644 --- a/internal/commands/server/k8s_e2e_test.go +++ b/internal/commands/server/k8s_e2e_test.go @@ -170,7 +170,7 @@ func TestGatewayWithoutNamespaceMirroring(t *testing.T) { assert.NoError(t, resources.Delete(ctx, thirdGateway)) // Re-enable namespace mirroring - ctx, err := e2e.SetNamespaceMirroring(true)(ctx, nil) + ctx, err = e2e.SetNamespaceMirroring(true)(ctx, nil) require.NoError(t, err) return ctx From ee4ac2c157bcfe178e772126ea5d5b95d1fc6618 Mon Sep 17 00:00:00 2001 From: Mike Morris Date: Tue, 10 Jan 2023 17:35:46 -0500 Subject: [PATCH 10/10] Update internal/k8s/controller.go Co-authored-by: Andrew Stucki --- internal/k8s/controller.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/internal/k8s/controller.go b/internal/k8s/controller.go index 855bbceb4..da2c71729 100644 --- a/internal/k8s/controller.go +++ b/internal/k8s/controller.go @@ -66,7 +66,7 @@ func (c ConsulNamespaceConfig) Namespace(namespace string) string { // Always map the default namespace to "" for compatibility with Consul OSS if consulNamespace == "default" { - consulNamespace = "" + return "" } return consulNamespace