Prevent extra-config from being loaded twice (and erroring for segmen…

…t config) on clients and servers (#3337)

* wip: testing with server works when you add segments as extraValues.
Todos:
* make similar changes to clients
* potentially upgrade test?
* consider locality having its own volume, rather than 2 volumes with extra in them

* move extra-config out of /consul/config so it does not get applied twice

* add comments about use of additional config maps

* remove temporary inclusion of values.yaml in root that was used for hand off

* get rid of temporary config.file

* add segments test

* test using 3 servers in a single cluster

* add changelog

* fix linting issues.

* add comment to test. remove extra lines from config map.

* fix bats tests

---------

Co-authored-by: Nitya Dhanushkodi <nitya@hashicorp.com>

acceptance/framework/cli/cli.go

@@ -4,10 +4,10 @@ import (
 	"fmt"
 	"os/exec"
 	"strings"
-	"testing"
 
 	"github.com/gruntwork-io/terratest/modules/k8s"
 	"github.com/hashicorp/consul-k8s/acceptance/framework/logger"
+	"github.com/hashicorp/consul/sdk/testutil"
 )
 
 const (
@@ -25,7 +25,7 @@ func NewCLI() (*CLI, error) {
 }
 
 // Run runs the CLI with the given args.
-func (c *CLI) Run(t *testing.T, options *k8s.KubectlOptions, args ...string) ([]byte, error) {
+func (c *CLI) Run(t testutil.TestingTB, options *k8s.KubectlOptions, args ...string) ([]byte, error) {
 	if !c.initialized {
 		return nil, fmt.Errorf("CLI must be initialized before calling Run, use `cli.NewCLI()` to initialize.")
 	}

acceptance/framework/connhelper/connect_helper.go

@@ -158,7 +158,7 @@ func (c *ConnectHelper) DeployClientAndServer(t *testing.T) {
 		&retry.Timer{Timeout: 30 * time.Second, Wait: 100 * time.Millisecond}, t,
 		func(r *retry.R) {
 			for _, labelSelector := range []string{"app=static-server", "app=static-client"} {
-				podList, err := c.Ctx.KubernetesClient(t).CoreV1().
+				podList, err := c.Ctx.KubernetesClient(r).CoreV1().
 					Pods(opts.Namespace).
 					List(context.Background(), metav1.ListOptions{
 						LabelSelector: labelSelector,

acceptance/framework/consul/cli_cluster.go

@@ -253,11 +253,8 @@ func (c *CLICluster) SetupConsulClient(t *testing.T, secure bool) (*api.Client,
 		c.logger)
 
 	// Retry creating the port forward since it can fail occasionally.
-	retry.RunWith(&retry.Counter{Wait: 1 * time.Second, Count: 3}, t, func(r *retry.R) {
-		// NOTE: It's okay to pass in `t` to ForwardPortE despite being in a retry
-		// because we're using ForwardPortE (not ForwardPort) so the `t` won't
-		// get used to fail the test, just for logging.
-		require.NoError(r, tunnel.ForwardPortE(t))
+	retry.RunWith(&retry.Counter{Wait: 3 * time.Second, Count: 60}, t, func(r *retry.R) {
+		require.NoError(r, tunnel.ForwardPortE(r))
 	})
 
 	t.Cleanup(func() {

acceptance/framework/consul/helm_cluster.go

@@ -152,77 +152,77 @@ func (h *HelmCluster) Destroy(t *testing.T) {
 		// graceful termination takes a long time and since this is an uninstall
 		// we don't care that they're stopped gracefully.
 		pods, err := h.kubernetesClient.CoreV1().Pods(h.helmOptions.KubectlOptions.Namespace).List(context.Background(), metav1.ListOptions{LabelSelector: "release=" + h.releaseName})
-		require.NoError(t, err)
+		require.NoError(r, err)
 		for _, pod := range pods.Items {
 			if strings.Contains(pod.Name, h.releaseName) {
 				var gracePeriod int64 = 0
 				err := h.kubernetesClient.CoreV1().Pods(h.helmOptions.KubectlOptions.Namespace).Delete(context.Background(), pod.Name, metav1.DeleteOptions{GracePeriodSeconds: &gracePeriod})
 				if !errors.IsNotFound(err) {
-					require.NoError(t, err)
+					require.NoError(r, err)
 				}
 			}
 		}
 
 		// Delete PVCs.
 		err = h.kubernetesClient.CoreV1().PersistentVolumeClaims(h.helmOptions.KubectlOptions.Namespace).DeleteCollection(context.Background(), metav1.DeleteOptions{}, metav1.ListOptions{LabelSelector: "release=" + h.releaseName})
-		require.NoError(t, err)
+		require.NoError(r, err)
 
 		// Delete any serviceaccounts that have h.releaseName in their name.
 		sas, err := h.kubernetesClient.CoreV1().ServiceAccounts(h.helmOptions.KubectlOptions.Namespace).List(context.Background(), metav1.ListOptions{LabelSelector: "release=" + h.releaseName})
-		require.NoError(t, err)
+		require.NoError(r, err)
 		for _, sa := range sas.Items {
 			if strings.Contains(sa.Name, h.releaseName) {
 				err := h.kubernetesClient.CoreV1().ServiceAccounts(h.helmOptions.KubectlOptions.Namespace).Delete(context.Background(), sa.Name, metav1.DeleteOptions{})
 				if !errors.IsNotFound(err) {
-					require.NoError(t, err)
+					require.NoError(r, err)
 				}
 			}
 		}
 
 		// Delete any roles that have h.releaseName in their name.
 		roles, err := h.kubernetesClient.RbacV1().Roles(h.helmOptions.KubectlOptions.Namespace).List(context.Background(), metav1.ListOptions{LabelSelector: "release=" + h.releaseName})
-		require.NoError(t, err)
+		require.NoError(r, err)
 		for _, role := range roles.Items {
 			if strings.Contains(role.Name, h.releaseName) {
 				err := h.kubernetesClient.RbacV1().Roles(h.helmOptions.KubectlOptions.Namespace).Delete(context.Background(), role.Name, metav1.DeleteOptions{})
 				if !errors.IsNotFound(err) {
-					require.NoError(t, err)
+					require.NoError(r, err)
 				}
 			}
 		}
 
 		// Delete any rolebindings that have h.releaseName in their name.
 		roleBindings, err := h.kubernetesClient.RbacV1().RoleBindings(h.helmOptions.KubectlOptions.Namespace).List(context.Background(), metav1.ListOptions{LabelSelector: "release=" + h.releaseName})
-		require.NoError(t, err)
+		require.NoError(r, err)
 		for _, roleBinding := range roleBindings.Items {
 			if strings.Contains(roleBinding.Name, h.releaseName) {
 				err := h.kubernetesClient.RbacV1().RoleBindings(h.helmOptions.KubectlOptions.Namespace).Delete(context.Background(), roleBinding.Name, metav1.DeleteOptions{})
 				if !errors.IsNotFound(err) {
-					require.NoError(t, err)
+					require.NoError(r, err)
 				}
 			}
 		}
 
 		// Delete any secrets that have h.releaseName in their name.
 		secrets, err := h.kubernetesClient.CoreV1().Secrets(h.helmOptions.KubectlOptions.Namespace).List(context.Background(), metav1.ListOptions{})
-		require.NoError(t, err)
+		require.NoError(r, err)
 		for _, secret := range secrets.Items {
 			if strings.Contains(secret.Name, h.releaseName) {
 				err := h.kubernetesClient.CoreV1().Secrets(h.helmOptions.KubectlOptions.Namespace).Delete(context.Background(), secret.Name, metav1.DeleteOptions{})
 				if !errors.IsNotFound(err) {
-					require.NoError(t, err)
+					require.NoError(r, err)
 				}
 			}
 		}
 
 		// Delete any jobs that have h.releaseName in their name.
 		jobs, err := h.kubernetesClient.BatchV1().Jobs(h.helmOptions.KubectlOptions.Namespace).List(context.Background(), metav1.ListOptions{LabelSelector: "release=" + h.releaseName})
-		require.NoError(t, err)
+		require.NoError(r, err)
 		for _, job := range jobs.Items {
 			if strings.Contains(job.Name, h.releaseName) {
 				err := h.kubernetesClient.BatchV1().Jobs(h.helmOptions.KubectlOptions.Namespace).Delete(context.Background(), job.Name, metav1.DeleteOptions{})
 				if !errors.IsNotFound(err) {
-					require.NoError(t, err)
+					require.NoError(r, err)
 				}
 			}
 		}

acceptance/framework/consul/helm_cluster_test.go

@@ -6,6 +6,7 @@ import (
 	"github.com/gruntwork-io/terratest/modules/k8s"
 	"github.com/hashicorp/consul-k8s/acceptance/framework/config"
 	"github.com/hashicorp/consul-k8s/acceptance/framework/environment"
+	"github.com/hashicorp/consul/sdk/testutil"
 	"github.com/stretchr/testify/require"
 	"k8s.io/client-go/kubernetes"
 	"k8s.io/client-go/kubernetes/fake"
@@ -73,14 +74,17 @@ func (c *ctx) Name() string {
 	return ""
 }
 
-func (c *ctx) KubectlOptions(_ *testing.T) *k8s.KubectlOptions {
+func (c *ctx) KubectlOptions(_ testutil.TestingTB) *k8s.KubectlOptions {
 	return &k8s.KubectlOptions{}
 }
 func (c *ctx) KubectlOptionsForNamespace(ns string) *k8s.KubectlOptions {
 	return &k8s.KubectlOptions{}
 }
-func (c *ctx) KubernetesClient(_ *testing.T) kubernetes.Interface {
+func (c *ctx) KubernetesClient(_ testutil.TestingTB) kubernetes.Interface {
 	return fake.NewSimpleClientset()
 }
+func (c *ctx) ControllerRuntimeClient(_ testutil.TestingTB) client.Client {
+	return runtimefake.NewClientBuilder().Build()
+}
 
 var _ environment.TestContext = (*ctx)(nil)

acceptance/framework/environment/environment.go

@@ -18,15 +18,14 @@ const (
 // TestEnvironment represents the infrastructure environment of the test,
 // such as the kubernetes cluster(s) the test is running against.
 type TestEnvironment interface {
-	DefaultContext(t *testing.T) TestContext
-	Context(t *testing.T, index int) TestContext
+	DefaultContext(t testutil.TestingTB) TestContext
+	Context(t testutil.TestingTB, index int) TestContext
 }
 
 // TestContext represents a specific context a test needs,
 // for example, information about a specific Kubernetes cluster.
 type TestContext interface {
-	KubectlOptions(t *testing.T) *k8s.KubectlOptions
-	// TODO: I don't love this.
+	KubectlOptions(t testutil.TestingTB) *k8s.KubectlOptions
 	KubectlOptionsForNamespace(ns string) *k8s.KubectlOptions
 	KubernetesClient(t *testing.T) kubernetes.Interface
 }
@@ -56,13 +55,13 @@ func NewKubernetesEnvironmentFromConfig(config *config.TestConfig) *KubernetesEn
 	return kenv
 }
 
-func (k *KubernetesEnvironment) Context(t *testing.T, index int) TestContext {
+func (k *KubernetesEnvironment) Context(t testutil.TestingTB, index int) TestContext {
 	lenContexts := len(k.contexts)
 	require.Greater(t, lenContexts, index, fmt.Sprintf("context list does not contain an index %d, length is %d", index, lenContexts))
 	return k.contexts[index]
 }
 
-func (k *KubernetesEnvironment) DefaultContext(t *testing.T) TestContext {
+func (k *KubernetesEnvironment) DefaultContext(t testutil.TestingTB) TestContext {
 	lenContexts := len(k.contexts)
 	require.Greater(t, lenContexts, DefaultContextIndex, fmt.Sprintf("context list does not contain an index %d, length is %d", DefaultContextIndex, lenContexts))
 	return k.contexts[DefaultContextIndex]
@@ -80,9 +79,7 @@ type kubernetesContext struct {
 // KubernetesContextFromOptions returns the Kubernetes context from options.
 // If context is explicitly set in options, it returns that context.
 // Otherwise, it returns the current context.
-func KubernetesContextFromOptions(t *testing.T, options *k8s.KubectlOptions) string {
-	t.Helper()
-
+func KubernetesContextFromOptions(t testutil.TestingTB, options *k8s.KubectlOptions) string {
 	// First, check if context set in options and return that
 	if options.ContextName != "" {
 		return options.ContextName
@@ -98,7 +95,7 @@ func KubernetesContextFromOptions(t *testing.T, options *k8s.KubectlOptions) str
 	return rawConfig.CurrentContext
 }
 
-func (k kubernetesContext) KubectlOptions(t *testing.T) *k8s.KubectlOptions {
+func (k kubernetesContext) KubectlOptions(t testutil.TestingTB) *k8s.KubectlOptions {
 	if k.options != nil {
 		return k.options
 	}
@@ -137,7 +134,7 @@ func (k kubernetesContext) KubectlOptionsForNamespace(ns string) *k8s.KubectlOpt
 }
 
 // KubernetesClientFromOptions takes KubectlOptions and returns Kubernetes API client.
-func KubernetesClientFromOptions(t *testing.T, options *k8s.KubectlOptions) kubernetes.Interface {
+func KubernetesClientFromOptions(t testutil.TestingTB, options *k8s.KubectlOptions) kubernetes.Interface {
 	configPath, err := options.GetConfigPath(t)
 	require.NoError(t, err)
 
@@ -150,7 +147,7 @@ func KubernetesClientFromOptions(t *testing.T, options *k8s.KubectlOptions) kube
 	return client
 }
 
-func (k kubernetesContext) KubernetesClient(t *testing.T) kubernetes.Interface {
+func (k kubernetesContext) KubernetesClient(t testutil.TestingTB) kubernetes.Interface {
 	if k.client != nil {
 		return k.client
 	}

acceptance/framework/helpers/helpers.go

@@ -40,7 +40,7 @@ func CheckForPriorInstallations(t *testing.T, client kubernetes.Interface, optio
 		// NOTE: It's okay to pass in `t` to RunHelmCommandAndGetOutputE despite being in a retry
 		// because we're using RunHelmCommandAndGetOutputE (not RunHelmCommandAndGetOutput) so the `t` won't
 		// get used to fail the test, just for logging.
-		helmListOutput, err = helm.RunHelmCommandAndGetOutputE(t, options, "list", "--output", "json")
+		helmListOutput, err = helm.RunHelmCommandAndGetOutputE(r, options, "list", "--output", "json")
 		require.NoError(r, err)
 	})
 

acceptance/framework/k8s/deploy.go

@@ -102,7 +102,7 @@ func CheckStaticServerConnectionMultipleFailureMessages(t *testing.T, options *k
 	args = append(args, curlArgs...)
 
 	retry.RunWith(retrier, t, func(r *retry.R) {
-		output, err := RunKubectlAndGetOutputE(t, options, args...)
+		output, err := RunKubectlAndGetOutputE(r, options, args...)
 		if expectSuccess {
 			require.NoError(r, err)
 			require.Contains(r, output, expectedOutput)

acceptance/framework/k8s/helpers.go

@@ -110,9 +110,9 @@ func ServiceHost(t *testing.T, cfg *config.TestConfig, ctx environment.TestConte
 		var host string
 		// It can take some time for the load balancers to be ready and have an IP/Hostname.
 		// Wait for 5 minutes before failing.
-		retry.RunWith(&retry.Counter{Wait: 1 * time.Second, Count: 600}, t, func(r *retry.R) {
-			svc, err := ctx.KubernetesClient(t).CoreV1().Services(ctx.KubectlOptions(t).Namespace).Get(context.Background(), serviceName, metav1.GetOptions{})
-			require.NoError(t, err)
+		retry.RunWith(&retry.Counter{Wait: 2 * time.Second, Count: 600}, t, func(r *retry.R) {
+			svc, err := ctx.KubernetesClient(r).CoreV1().Services(ctx.KubectlOptions(r).Namespace).Get(context.Background(), serviceName, metav1.GetOptions{})
+			require.NoError(r, err)
 			require.NotEmpty(r, svc.Status.LoadBalancer.Ingress)
 			// On AWS, load balancers have a hostname for ingress, while on Azure and GCP
 			// load balancers have IPs.
@@ -132,7 +132,7 @@ func CopySecret(t *testing.T, sourceContext, destContext environment.TestContext
 	var secret *corev1.Secret
 	var err error
 	retry.Run(t, func(r *retry.R) {
-		secret, err = sourceContext.KubernetesClient(t).CoreV1().Secrets(sourceContext.KubectlOptions(t).Namespace).Get(context.Background(), secretName, metav1.GetOptions{})
+		secret, err = sourceContext.KubernetesClient(r).CoreV1().Secrets(sourceContext.KubectlOptions(r).Namespace).Get(context.Background(), secretName, metav1.GetOptions{})
 		secret.ResourceVersion = ""
 		require.NoError(r, err)
 	})

acceptance/framework/k8s/kubectl.go

@@ -9,6 +9,7 @@ import (
 	terratestLogger "github.com/gruntwork-io/terratest/modules/logger"
 	"github.com/gruntwork-io/terratest/modules/shell"
 	"github.com/hashicorp/consul-k8s/acceptance/framework/logger"
+	"github.com/hashicorp/consul/sdk/testutil"
 	"github.com/hashicorp/consul/sdk/testutil/retry"
 	"github.com/stretchr/testify/require"
 )
@@ -26,14 +27,14 @@ var kubeAPIConnectErrs = []string{
 
 // RunKubectlAndGetOutputE runs an arbitrary kubectl command provided via args
 // and returns its output and error.
-func RunKubectlAndGetOutputE(t *testing.T, options *k8s.KubectlOptions, args ...string) (string, error) {
+func RunKubectlAndGetOutputE(t testutil.TestingTB, options *k8s.KubectlOptions, args ...string) (string, error) {
 	return RunKubectlAndGetOutputWithLoggerE(t, options, terratestLogger.New(logger.TestLogger{}), args...)
 }
 
 // RunKubectlAndGetOutputWithLoggerE is the same as RunKubectlAndGetOutputE but
 // it also allows you to provide a custom logger. This is useful if the command output
 // contains sensitive information, for example, when you can pass logger.Discard.
-func RunKubectlAndGetOutputWithLoggerE(t *testing.T, options *k8s.KubectlOptions, logger *terratestLogger.Logger, args ...string) (string, error) {
+func RunKubectlAndGetOutputWithLoggerE(t testutil.TestingTB, options *k8s.KubectlOptions, logger *terratestLogger.Logger, args ...string) (string, error) {
 	var cmdArgs []string
 	if options.ContextName != "" {
 		cmdArgs = append(cmdArgs, "--context", options.ContextName)
@@ -59,7 +60,7 @@ func RunKubectlAndGetOutputWithLoggerE(t *testing.T, options *k8s.KubectlOptions
 	var output string
 	var err error
 	retry.RunWith(counter, t, func(r *retry.R) {
-		output, err = shell.RunCommandAndGetOutputE(t, command)
+		output, err = shell.RunCommandAndGetOutputE(r, command)
 		if err != nil {
 			// Want to retry on errors connecting to actual Kube API because
 			// these are intermittent.

acceptance/framework/logger/logger.go

@@ -2,20 +2,19 @@ package logger
 
 import (
 	"fmt"
-	"testing"
+	"github.com/hashicorp/consul/sdk/testutil"
 	"time"
 
-	terratestTesting "github.com/gruntwork-io/terratest/modules/testing"
+	terratesting "github.com/gruntwork-io/terratest/modules/testing"
 )
 
-// TestLogger implements terratest's TestLogger interface
-// so that we can pass it to terratest objects to have consistent logging
-// across all tests.
+// TestLogger implements Terratest's TestLogger interface so that we can pass it to Terratest objects to have consistent
+// logging across all tests.
 type TestLogger struct{}
 
 // Logf takes a format string and args and calls Logf function.
-func (tl TestLogger) Logf(t terratestTesting.TestingT, format string, args ...interface{}) {
-	tt, ok := t.(*testing.T)
+func (tl TestLogger) Logf(t terratesting.TestingT, format string, args ...any) {
+	tt, ok := t.(testutil.TestingTB)
 	if !ok {
 		t.Error("failed to cast")
 	}
@@ -24,20 +23,18 @@ func (tl TestLogger) Logf(t terratestTesting.TestingT, format string, args ...in
 	Logf(tt, format, args...)
 }
 
-// Logf takes a format string and args and logs
-// formatted string with a timestamp.
-func Logf(t *testing.T, format string, args ...interface{}) {
+// Logf takes a format string and args and logs formatted string with a timestamp.
+func Logf(t testutil.TestingTB, format string, args ...any) {
 	t.Helper()
 
 	log := fmt.Sprintf(format, args...)
 	Log(t, log)
 }
 
-// Log calls t.Log, adding an RFC3339 timestamp to the beginning of the log line.
-func Log(t *testing.T, args ...interface{}) {
+// Log calls t.Log or r.Log, adding an RFC3339 timestamp to the beginning of the log line.
+func Log(t testutil.TestingTB, args ...any) {
 	t.Helper()
-
-	allArgs := []interface{}{time.Now().Format(time.RFC3339)}
+	allArgs := []any{time.Now().Format(time.RFC3339)}
 	allArgs = append(allArgs, args...)
 	t.Log(allArgs...)
 }

acceptance/framework/portforward/port_forward.go

@@ -28,7 +28,7 @@ func CreateTunnelToResourcePort(t *testing.T, resourceName string, remotePort in
 		// NOTE: It's okay to pass in `t` to ForwardPortE despite being in a retry
 		// because we're using ForwardPortE (not ForwardPort) so the `t` won't
 		// get used to fail the test, just for logging.
-		require.NoError(r, tunnel.ForwardPortE(t))
+		require.NoError(r, tunnel.ForwardPortE(r))
 	})
 
 	doneChan := make(chan bool)