diff --git a/.circleci/config.yml b/.circleci/config.yml index b1bcce48c7..9b1d4e951a 100644 --- a/.circleci/config.yml +++ b/.circleci/config.yml @@ -100,7 +100,9 @@ commands: << parameters.additional-flags >> \ ${ENABLE_ENTERPRISE:+-enable-enterprise} \ -enable-multi-cluster \ + -run TestPeering_* \ -debug-directory="$TEST_RESULTS/debug" \ + -consul-image=docker.mirror.hashicorp.services/hashicorppreview/consul-enterprise:1.13-dev \ -consul-k8s-image=<< parameters.consul-k8s-image >> then echo "Tests in ${pkg} failed, aborting early" @@ -132,6 +134,7 @@ commands: -enable-multi-cluster \ ${ENABLE_ENTERPRISE:+-enable-enterprise} \ -debug-directory="$TEST_RESULTS/debug" \ + -consul-image=docker.mirror.hashicorp.services/hashicorppreview/consul-enterprise:1.13-dev \ -consul-k8s-image=<< parameters.consul-k8s-image >> jobs: @@ -997,10 +1000,52 @@ workflows: - build-distros-linux # Run acceptance tests using the docker image built for the control plane - acceptance: + name: acceptance1 context: consul-ci requires: - dev-upload-docker - acceptance-tproxy: + name: acceptance2 + context: consul-ci + requires: + - dev-upload-docker + - acceptance: + name: acceptance3 + context: consul-ci + requires: + - dev-upload-docker + - acceptance-tproxy: + name: acceptance4 + context: consul-ci + requires: + - dev-upload-docker + - acceptance: + name: acceptance5 + context: consul-ci + requires: + - dev-upload-docker + - acceptance-tproxy: + name: acceptance6 + context: consul-ci + requires: + - dev-upload-docker + - acceptance: + name: acceptance7 + context: consul-ci + requires: + - dev-upload-docker + - acceptance-tproxy: + name: acceptance8 + context: consul-ci + requires: + - dev-upload-docker + - acceptance: + name: acceptance9 + context: consul-ci + requires: + - dev-upload-docker + - acceptance-tproxy: + name: acceptance10 context: consul-ci requires: - dev-upload-docker diff --git a/acceptance/framework/k8s/helpers.go b/acceptance/framework/k8s/helpers.go index 3f981336ee..c646fd2ab0 100644 --- a/acceptance/framework/k8s/helpers.go +++ b/acceptance/framework/k8s/helpers.go @@ -2,6 +2,8 @@ package k8s import ( "context" + "encoding/base64" + "encoding/json" "fmt" "strings" "testing" @@ -136,6 +138,14 @@ func CopySecret(t *testing.T, sourceContext, destContext environment.TestContext secret.ResourceVersion = "" require.NoError(r, err) }) + secretData := secret.Data["data"] + + var token map[string]interface{} + // Decode the token to extract the ServerName and PeerID from the token. CA is always NULL. + decodeBytes, err := base64.StdEncoding.DecodeString(string(secretData)) + require.NoError(t, err) + err = json.Unmarshal(decodeBytes, &token) + logger.Log(t, "peering token", token) _, err = destContext.KubernetesClient(t).CoreV1().Secrets(destContext.KubectlOptions(t).Namespace).Create(context.Background(), secret, metav1.CreateOptions{}) require.NoError(t, err) } diff --git a/acceptance/framework/k8s/kubectl.go b/acceptance/framework/k8s/kubectl.go index 318cde217e..7382cb0507 100644 --- a/acceptance/framework/k8s/kubectl.go +++ b/acceptance/framework/k8s/kubectl.go @@ -94,7 +94,7 @@ func KubectlApplyK(t *testing.T, options *k8s.KubectlOptions, kustomizeDir strin // deletes it from the cluster by running 'kubectl delete -f'. // If there's an error deleting the file, fail the test. func KubectlDelete(t *testing.T, options *k8s.KubectlOptions, configPath string) { - _, err := RunKubectlAndGetOutputE(t, options, "delete", "-f", configPath) + _, err := RunKubectlAndGetOutputE(t, options, "delete", "--timeout=60s", "-f", configPath) require.NoError(t, err) } @@ -102,7 +102,7 @@ func KubectlDelete(t *testing.T, options *k8s.KubectlOptions, configPath string) // deletes it from the cluster by running 'kubectl delete -k'. // If there's an error deleting the file, fail the test. func KubectlDeleteK(t *testing.T, options *k8s.KubectlOptions, kustomizeDir string) { - _, err := RunKubectlAndGetOutputE(t, options, "delete", "-k", kustomizeDir) + _, err := RunKubectlAndGetOutputE(t, options, "delete", "--timeout=60s", "-k", kustomizeDir) require.NoError(t, err) } diff --git a/acceptance/tests/peering/peering_connect_namespaces_test.go b/acceptance/tests/peering/peering_connect_namespaces_test.go index 78a1c58436..ba6e4dea15 100644 --- a/acceptance/tests/peering/peering_connect_namespaces_test.go +++ b/acceptance/tests/peering/peering_connect_namespaces_test.go @@ -54,36 +54,36 @@ func TestPeering_ConnectNamespaces(t *testing.T) { false, false, }, - { - "single destination namespace", - staticServerNamespace, - false, - false, - }, - { - "mirror k8s namespaces", - staticServerNamespace, - true, - false, - }, - { - "default destination namespace", - defaultNamespace, - false, - true, - }, - { - "single destination namespace", - staticServerNamespace, - false, - true, - }, - { - "mirror k8s namespaces", - staticServerNamespace, - true, - true, - }, + //{ + // "single destination namespace", + // staticServerNamespace, + // false, + // false, + //}, + //{ + // "mirror k8s namespaces", + // staticServerNamespace, + // true, + // false, + //}, + //{ + // "default destination namespace", + // defaultNamespace, + // false, + // true, + //}, + //{ + // "single destination namespace", + // staticServerNamespace, + // false, + // true, + //}, + //{ + // "mirror k8s namespaces", + // staticServerNamespace, + // true, + // true, + //}, } for _, c := range cases { @@ -95,8 +95,6 @@ func TestPeering_ConnectNamespaces(t *testing.T) { "global.peering.enabled": "true", "global.enableConsulNamespaces": "true", - "global.image": "thisisnotashwin/consul@sha256:b1d3f59406adf5fb9a3bee4ded058e619d3a186e83b2e2dc14d6da3f28a7073d", - "global.tls.enabled": "true", "global.tls.httpsOnly": strconv.FormatBool(c.ACLsAndAutoEncryptEnabled), "global.tls.enableAutoEncrypt": strconv.FormatBool(c.ACLsAndAutoEncryptEnabled), diff --git a/acceptance/tests/peering/peering_connect_test.go b/acceptance/tests/peering/peering_connect_test.go index d18d6bee70..f5c9722682 100644 --- a/acceptance/tests/peering/peering_connect_test.go +++ b/acceptance/tests/peering/peering_connect_test.go @@ -54,8 +54,6 @@ func TestPeering_Connect(t *testing.T) { commonHelmValues := map[string]string{ "global.peering.enabled": "true", - "global.image": "thisisnotashwin/consul@sha256:b1d3f59406adf5fb9a3bee4ded058e619d3a186e83b2e2dc14d6da3f28a7073d", - "global.tls.enabled": "true", "global.tls.httpsOnly": strconv.FormatBool(c.ACLsAndAutoEncryptEnabled), "global.tls.enableAutoEncrypt": strconv.FormatBool(c.ACLsAndAutoEncryptEnabled), diff --git a/charts/consul/templates/server-config-configmap.yaml b/charts/consul/templates/server-config-configmap.yaml index 113a1df22a..e35311a9c7 100644 --- a/charts/consul/templates/server-config-configmap.yaml +++ b/charts/consul/templates/server-config-configmap.yaml @@ -32,6 +32,11 @@ data: }, "recursors": {{ .Values.global.recursors | toJson }}, "retry_join": ["{{template "consul.fullname" . }}-server.{{ .Release.Namespace }}.svc:{{ .Values.server.ports.serflan.port }}"], + {{- if .Values.global.peering.enabled }} + "peering": { + "enabled": true + }, + {{- end }} "server": true } {{- $vaultConnectCAEnabled := and .Values.global.secretsBackend.vault.connectCA.address .Values.global.secretsBackend.vault.connectCA.rootPKIPath .Values.global.secretsBackend.vault.connectCA.intermediatePKIPath -}} diff --git a/charts/consul/test/unit/server-config-configmap.bats b/charts/consul/test/unit/server-config-configmap.bats index 9b6c4206de..d31cbe774c 100755 --- a/charts/consul/test/unit/server-config-configmap.bats +++ b/charts/consul/test/unit/server-config-configmap.bats @@ -905,3 +905,28 @@ load _helpers [ "${actual}" = null ] } + +#-------------------------------------------------------------------- +# peering + +@test "server/ConfigMap: peering configuration is unspecified by default" { + cd `chart_dir` + local actual=$(helm template \ + -s templates/server-config-configmap.yaml \ + . | tee /dev/stderr | + yq -r '.data["server.json"]' | jq -r .peering | tee /dev/stderr) + + [ "${actual}" = "null" ] +} + +@test "server/ConfigMap: peering configuration is set by if global.peering.enabled is true" { + cd `chart_dir` + local actual=$(helm template \ + -s templates/server-config-configmap.yaml \ + --set 'global.peering.enabled=true' \ + --set 'connectInject.enabled=true' \ + . | tee /dev/stderr | + yq -r '.data["server.json"]' | jq -r .peering.enabled | tee /dev/stderr) + + [ "${actual}" = "true" ] +} diff --git a/control-plane/connect-inject/peering_acceptor_controller.go b/control-plane/connect-inject/peering_acceptor_controller.go index da977d8e8a..76d2f3ad05 100644 --- a/control-plane/connect-inject/peering_acceptor_controller.go +++ b/control-plane/connect-inject/peering_acceptor_controller.go @@ -3,7 +3,9 @@ package connectinject import ( "context" "errors" + "fmt" "strconv" + "sync" "time" "github.com/go-logr/logr" @@ -32,6 +34,8 @@ type PeeringAcceptorController struct { Log logr.Logger Scheme *runtime.Scheme context.Context + + mutex sync.Mutex } const ( @@ -98,13 +102,24 @@ func (r *PeeringAcceptorController) Reconcile(ctx context.Context, req ctrl.Requ } } + // todo: we should check that the secret in the spec exists and just update status rather than regenerating a new token altogether statusSecretSet := acceptor.SecretRef() != nil - // existingStatusSecret will be nil if it doesn't exist, and have the contents of the secret if it does exist. - var existingStatusSecret *corev1.Secret + // existingSecret will be nil if it doesn't exist, and have the contents of the secret if it does exist. + var existingSecret *corev1.Secret if statusSecretSet { - existingStatusSecret, err = r.getExistingSecret(ctx, acceptor.SecretRef().Name, acceptor.Namespace) + r.Log.Info("secret status is set; retrieving secret") + existingSecret, err = r.getExistingSecret(ctx, acceptor.SecretRef().Name, acceptor.Namespace) + if err != nil { + r.Log.Error(err, "error retrieving existing secret", "name", acceptor.SecretRef().Name) + r.updateStatusError(ctx, acceptor, KubernetesError, err) // todo: why do set update status error here? + return ctrl.Result{}, err + } + } else { + // If status is not set, check if the secret from the spec already exists and update the status. + existingSecret, err = r.getExistingSecret(ctx, acceptor.Secret().Name, acceptor.Namespace) if err != nil { + r.Log.Error(err, "error retrieving existing secret", "name", acceptor.Secret().Name) r.updateStatusError(ctx, acceptor, KubernetesError, err) return ctrl.Result{}, err } @@ -124,14 +139,12 @@ func (r *PeeringAcceptorController) Reconcile(ctx context.Context, req ctrl.Requ if peering == nil { r.Log.Info("peering doesn't exist in Consul; creating new peering", "name", acceptor.Name) - if statusSecretSet { - if existingStatusSecret != nil { - r.Log.Info("stale secret in status; deleting stale secret", "name", acceptor.Name) - err := r.Client.Delete(ctx, existingStatusSecret) - if err != nil { - r.updateStatusError(ctx, acceptor, KubernetesError, err) - return ctrl.Result{}, err - } + if existingSecret != nil { + r.Log.Info("secret exists without a peering in Consul; deleting stale secret", "name", acceptor.Name) + err := r.Client.Delete(ctx, existingSecret) + if err != nil { + r.updateStatusError(ctx, acceptor, KubernetesError, err) + return ctrl.Result{}, err } } // Generate and store the peering token. @@ -148,7 +161,7 @@ func (r *PeeringAcceptorController) Reconcile(ctx context.Context, req ctrl.Requ } } // Store the state in the status. - err := r.updateStatus(ctx, acceptor, secretResourceVersion) + err := r.updateStatus(ctx, req.NamespacedName, secretResourceVersion) return ctrl.Result{}, err } else if err != nil { r.Log.Error(err, "failed to get Peering from Consul", "name", req.Name) @@ -157,18 +170,23 @@ func (r *PeeringAcceptorController) Reconcile(ctx context.Context, req ctrl.Requ // TODO(peering): Verify that the existing peering in Consul is an acceptor peer. If it is a dialing peer, an error should be thrown. + r.Log.Info("peering exists in Consul") // If the peering does exist in Consul, figure out whether to generate and store a new token by comparing the secret // in the status to the resource version of the secret. If no secret is specified in the status, shouldGenerate will // be set to true. var shouldGenerate bool var nameChanged bool - if statusSecretSet { - shouldGenerate, nameChanged, err = shouldGenerateToken(acceptor, existingStatusSecret) + if existingSecret != nil { + r.Log.Info("found existing secret; determining if we need to generate token again") + shouldGenerate, nameChanged, err = r.shouldGenerateToken(acceptor, existingSecret) if err != nil { + r.Log.Error(err, "error determining if we should generate token again") r.updateStatusError(ctx, acceptor, InternalError, err) return ctrl.Result{}, err } + r.Log.Info("finished determining if we should generate token", "shouldGenerate", shouldGenerate, "nameChanged", nameChanged) } else { + r.Log.Info("existing secret is nil; generating a new token") shouldGenerate = true } @@ -186,8 +204,8 @@ func (r *PeeringAcceptorController) Reconcile(ctx context.Context, req ctrl.Requ } // Delete the existing secret if the name changed. This needs to come before updating the status if we do generate a new token. if nameChanged { - if existingStatusSecret != nil { - err := r.Client.Delete(ctx, existingStatusSecret) + if existingSecret != nil { + err := r.Client.Delete(ctx, existingSecret) if err != nil { r.updateStatusError(ctx, acceptor, ConsulAgentError, err) return ctrl.Result{}, err @@ -196,24 +214,28 @@ func (r *PeeringAcceptorController) Reconcile(ctx context.Context, req ctrl.Requ } // Store the state in the status. - err := r.updateStatus(ctx, acceptor, secretResourceVersion) + err := r.updateStatus(ctx, req.NamespacedName, secretResourceVersion) return ctrl.Result{}, err } + r.Log.Info("finished reconcile") return ctrl.Result{}, nil } // shouldGenerateToken returns whether a token should be generated, and whether the name of the secret has changed. It // compares the spec secret's name/key/backend and resource version with the name/key/backend and resource version of the status secret's. -func shouldGenerateToken(acceptor *consulv1alpha1.PeeringAcceptor, existingStatusSecret *corev1.Secret) (shouldGenerate bool, nameChanged bool, err error) { +func (r *PeeringAcceptorController) shouldGenerateToken(acceptor *consulv1alpha1.PeeringAcceptor, existingStatusSecret *corev1.Secret) (shouldGenerate bool, nameChanged bool, err error) { if acceptor.SecretRef() == nil { + r.Log.Info("shouldGenerateToken; secretRef is nil") return false, false, errors.New("shouldGenerateToken was called with an empty fields in the existing status") } // Compare the existing name, key, and backend. if acceptor.SecretRef().Name != acceptor.Secret().Name { + r.Log.Info("shouldGenerateToken; names don't match", "secret-ref-name", acceptor.SecretRef().Name, "spec name", acceptor.Secret().Name) return true, true, nil } if acceptor.SecretRef().Key != acceptor.Secret().Key { + r.Log.Info("shouldGenerateToken; keys don't match", "secret-ref-key", acceptor.SecretRef().Key, "spec key", acceptor.Secret().Key) return true, false, nil } // TODO(peering): remove this when validation webhook exists. @@ -225,25 +247,43 @@ func shouldGenerateToken(acceptor *consulv1alpha1.PeeringAcceptor, existingStatu if err != nil { return false, false, err } + r.Log.Info("shouldGenerateToken; checking peering version annotation", "version", peeringVersion) if acceptor.Status.LatestPeeringVersion == nil || *acceptor.Status.LatestPeeringVersion < peeringVersion { + r.Log.Info("shouldGenerateToken; should regenerate is true because either the latest version is nil or lower than peering version", "latest-version", acceptor.Status.LatestPeeringVersion) return true, false, nil } } // Compare the existing secret resource version. // Get the secret specified by the status, make sure it matches the status' secret.ResourceVersion. if existingStatusSecret != nil { - if existingStatusSecret.ResourceVersion != acceptor.SecretRef().ResourceVersion { - return true, false, nil - } + r.Log.Info("shouldGenerateToken; comparing resource versions of exsiting secret with the one in secret ref") + // general question of whether we should regenerate it at all + // there should be three cases: + // 1. if version(existing secret from status) > the version in CR, should we just update the status in the CR? why do we regenerate the token in this case (which we do at the end) + // 2. if version(existing secret from) < the version in CR, that should be impossible? + // 3. + //if existingStatusSecret.ResourceVersion != acceptor.SecretRef().ResourceVersion { + // r.Log.Info("shouldGenerateToken; should generate is true because versions don't match", "existing-status-secret", existingStatusSecret.ResourceVersion, "secret-ref-version", acceptor.SecretRef().ResourceVersion) + // return true, false, nil + //} + return false, false, nil - } else { - return true, false, nil } - return false, false, nil + + r.Log.Info("shouldGenerateToken, should generate is true because existing status secret is nil") + return true, false, nil } // updateStatus updates the peeringAcceptor's secret in the status. -func (r *PeeringAcceptorController) updateStatus(ctx context.Context, acceptor *consulv1alpha1.PeeringAcceptor, secretResourceVersion string) error { +func (r *PeeringAcceptorController) updateStatus(ctx context.Context, acceptorObjKey types.NamespacedName, secretResourceVersion string) error { + //r.mutex.Lock() + //defer r.mutex.Unlock() + // Get the latest resource before we update it. + acceptor := &consulv1alpha1.PeeringAcceptor{} + err := r.Client.Get(ctx, acceptorObjKey, acceptor) + if err != nil { + return fmt.Errorf("error fetching acceptor resource before status update: %w", err) + } acceptor.Status.SecretRef = &consulv1alpha1.SecretRefStatus{ Secret: *acceptor.Secret(), ResourceVersion: secretResourceVersion, @@ -260,7 +300,8 @@ func (r *PeeringAcceptorController) updateStatus(ctx context.Context, acceptor * acceptor.Status.LatestPeeringVersion = pointerToUint64(peeringVersion) } } - err := r.Status().Update(ctx, acceptor) + // todo: does it need a read-write lock + err = r.Status().Update(ctx, acceptor) if err != nil { r.Log.Error(err, "failed to update PeeringAcceptor status", "name", acceptor.Name, "namespace", acceptor.Namespace) } @@ -343,6 +384,7 @@ func (r *PeeringAcceptorController) SetupWithManager(mgr ctrl.Manager) error { // generateToken is a helper function that calls the Consul api to generate a token for the peer. func (r *PeeringAcceptorController) generateToken(ctx context.Context, peerName string) (*api.PeeringGenerateTokenResponse, error) { + r.Log.Info("calling /peering/token to generate token") req := api.PeeringGenerateTokenRequest{ PeerName: peerName, }