From ce2a2f7998dc5e51e51f422e5c73db887ec97fef Mon Sep 17 00:00:00 2001 From: Nitya Dhanushkodi Date: Mon, 8 Aug 2022 22:05:34 -0700 Subject: [PATCH 1/3] update unit tests to not reuse peering token --- .github/workflows/test.yml | 4 +- .../peering_dialer_controller.go | 4 +- .../peering_dialer_controller_test.go | 84 +++++++------------ 3 files changed, 35 insertions(+), 57 deletions(-) diff --git a/.github/workflows/test.yml b/.github/workflows/test.yml index d962e5f33c..9b828c8739 100644 --- a/.github/workflows/test.yml +++ b/.github/workflows/test.yml @@ -146,7 +146,7 @@ jobs: working-directory: control-plane run: | mkdir -p $HOME/bin - wget https://github.com/ndhanushkodi/binaries/releases/download/v4.1oss/consul -O consulbin && \ + wget https://github.com/ndhanushkodi/binaries/releases/download/v6oss/consul -O consulbin && \ mv consulbin $HOME/bin/consul && chmod +x $HOME/bin/consul @@ -194,7 +194,7 @@ jobs: working-directory: control-plane run: | mkdir -p $HOME/bin - wget https://github.com/ndhanushkodi/binaries/releases/download/v4.1ent/consul -O consulbin && \ + wget https://github.com/ndhanushkodi/binaries/releases/download/v6ent/consul -O consulbin && \ mv consulbin $HOME/bin/consul && chmod +x $HOME/bin/consul diff --git a/control-plane/connect-inject/peering_dialer_controller.go b/control-plane/connect-inject/peering_dialer_controller.go index ffa88e3d61..848c7418cb 100644 --- a/control-plane/connect-inject/peering_dialer_controller.go +++ b/control-plane/connect-inject/peering_dialer_controller.go @@ -151,7 +151,7 @@ func (r *PeeringDialerController) Reconcile(ctx context.Context, req ctrl.Reques // Or, if the peering in Consul does exist, compare it to the spec's secret. If there's any // differences, initiate peering. if r.specStatusSecretsDifferent(dialer, specSecret) { - r.Log.Info("the version annotation was incremented; re-establishing peering with spec.peer.secret", "secret-name", dialer.Secret().Name, "secret-namespace", dialer.Namespace) + r.Log.Info("the spec.peer.secret is different from the status secret, re-establishing peering", "secret-name", dialer.Secret().Name, "secret-namespace", dialer.Namespace) peeringToken := specSecret.Data[dialer.Secret().Key] if err := r.establishPeering(ctx, dialer.Name, string(peeringToken)); err != nil { r.updateStatusError(ctx, dialer, ConsulAgentError, err) @@ -163,7 +163,7 @@ func (r *PeeringDialerController) Reconcile(ctx context.Context, req ctrl.Reques } if updated, err := r.versionAnnotationUpdated(dialer); err == nil && updated { - r.Log.Info("status.secret exists, but the peering doesn't exist in Consul; establishing peering with the existing spec.peer.secret", "secret-name", dialer.Secret().Name, "secret-namespace", dialer.Namespace) + r.Log.Info("the version annotation was incremented; re-establishing peering with spec.peer.secret", "secret-name", dialer.Secret().Name, "secret-namespace", dialer.Namespace) peeringToken := specSecret.Data[dialer.Secret().Key] if err := r.establishPeering(ctx, dialer.Name, string(peeringToken)); err != nil { r.updateStatusError(ctx, dialer, ConsulAgentError, err) diff --git a/control-plane/connect-inject/peering_dialer_controller_test.go b/control-plane/connect-inject/peering_dialer_controller_test.go index d10937db6c..c5a6a6bdf0 100644 --- a/control-plane/connect-inject/peering_dialer_controller_test.go +++ b/control-plane/connect-inject/peering_dialer_controller_test.go @@ -2,11 +2,7 @@ package connectinject import ( "context" - "encoding/base64" - "encoding/json" "errors" - "fmt" - "strings" "testing" "time" @@ -270,34 +266,22 @@ func TestReconcile_CreateUpdatePeeringDialer(t *testing.T) { // Create fake k8s client k8sObjects := append(tt.k8sObjects(), &ns) - // This is responsible for updating the token generated by the acceptor side with the IP - // of the Consul server as the generated token currently does not have that set on it. + // Generate a token. var encodedPeeringToken string if tt.peeringName != "" { - var token struct { - CA string - ServerAddresses []string - ServerName string - PeerID string - } // Create the initial token. baseToken, _, err := acceptorClient.Peerings().GenerateToken(context.Background(), api.PeeringGenerateTokenRequest{PeerName: tt.peeringName}, nil) require.NoError(t, err) - // Decode the token to extract the ServerName and PeerID from the token. CA is always NULL. - decodeBytes, err := base64.StdEncoding.DecodeString(baseToken.PeeringToken) - require.NoError(t, err) - err = json.Unmarshal(decodeBytes, &token) - require.NoError(t, err) - // Get the IP of the Consul server. - addr := strings.Split(acceptorPeerServer.HTTPAddr, ":")[0] - port := strings.Split(acceptorPeerServer.GRPCAddr, ":")[1] - // Generate expected token for Peering Initiate. - tokenString := fmt.Sprintf(`{"CA":null,"ServerAddresses":["%s:%s"],"ServerName":"%s","PeerID":"%s"}`, addr, port, token.ServerName, token.PeerID) - // Create peering initiate secret in Kubernetes. - encodedPeeringToken = base64.StdEncoding.EncodeToString([]byte(tokenString)) + encodedPeeringToken = baseToken.PeeringToken + } + + // If the peering is not supposed to already exist in Consul, then create a secret with the generated token. + if !tt.peeringExists { secret := tt.peeringSecret(encodedPeeringToken) - secret.SetResourceVersion("latest-version") - k8sObjects = append(k8sObjects, secret) + if secret != nil { + secret.SetResourceVersion("latest-version") + k8sObjects = append(k8sObjects, secret) + } } // Create test consul server. @@ -314,12 +298,19 @@ func TestReconcile_CreateUpdatePeeringDialer(t *testing.T) { dialerClient, err := api.NewClient(cfg) require.NoError(t, err) + // If the peering is supposed to already exist in Consul, then establish a peering with the existing token, so the peering will exist on the dialing side. if tt.peeringExists { _, _, err := dialerClient.Peerings().Establish(context.Background(), api.PeeringEstablishRequest{PeerName: tt.peeringName, PeeringToken: encodedPeeringToken}, nil) require.NoError(t, err) k8sObjects = append(k8sObjects, createSecret("dialer-token-old", "default", "token", "old-token")) + // Create a new token to be used by Reconcile(). The original token has already been + // used once to simulate establishing an existing peering. + baseToken, _, err := acceptorClient.Peerings().GenerateToken(context.Background(), api.PeeringGenerateTokenRequest{PeerName: tt.peeringName}, nil) + require.NoError(t, err) + secret := tt.peeringSecret(baseToken.PeeringToken) + secret.SetResourceVersion("latest-version") + k8sObjects = append(k8sObjects, secret) } - s := scheme.Scheme s.AddKnownTypes(v1alpha1.GroupVersion, &v1alpha1.PeeringDialer{}, &v1alpha1.PeeringDialerList{}) fakeClient := fake.NewClientBuilder().WithScheme(s).WithRuntimeObjects(k8sObjects...).Build() @@ -481,32 +472,11 @@ func TestReconcile_VersionAnnotationPeeringDialer(t *testing.T) { // Create fake k8s client k8sObjects := []runtime.Object{dialer, ns} - // This is responsible for updating the token generated by the acceptor side with the IP - // of the Consul server as the generated token currently does not have that set on it. - var encodedPeeringToken string - var token struct { - CA string - ServerAddresses []string - ServerName string - PeerID string - } - // Create the initial token. - baseToken, _, err := acceptorClient.Peerings().GenerateToken(context.Background(), api.PeeringGenerateTokenRequest{PeerName: "peering"}, nil) - require.NoError(t, err) - // Decode the token to extract the ServerName and PeerID from the token. CA is always NULL. - decodeBytes, err := base64.StdEncoding.DecodeString(baseToken.PeeringToken) - require.NoError(t, err) - err = json.Unmarshal(decodeBytes, &token) + // Create a peering connection in Consul by generating and establishing a peering connection before calling + // Reconcile(). + // Generate a token. + generatedToken, _, err := acceptorClient.Peerings().GenerateToken(context.Background(), api.PeeringGenerateTokenRequest{PeerName: "peering"}, nil) require.NoError(t, err) - // Get the IP of the Consul server. - addr := strings.Split(acceptorPeerServer.HTTPAddr, ":")[0] - // Generate expected token for Peering Initiate. - tokenString := fmt.Sprintf(`{"CA":null,"ServerAddresses":["%s:8300"],"ServerName":"%s","PeerID":"%s"}`, addr, token.ServerName, token.PeerID) - // Create peering initiate secret in Kubernetes. - encodedPeeringToken = base64.StdEncoding.EncodeToString([]byte(tokenString)) - secret := createSecret("dialer-token", "default", "token", encodedPeeringToken) - secret.SetResourceVersion("latest-version") - k8sObjects = append(k8sObjects, secret) // Create test consul server. dialerPeerServer, err := testutil.NewTestServerConfigT(t, func(c *testutil.TestServerConfig) { @@ -522,10 +492,18 @@ func TestReconcile_VersionAnnotationPeeringDialer(t *testing.T) { dialerClient, err := api.NewClient(cfg) require.NoError(t, err) - _, _, err = dialerClient.Peerings().Establish(context.Background(), api.PeeringEstablishRequest{PeerName: "peering", PeeringToken: encodedPeeringToken}, nil) + // Establish a peering with the generated token. + _, _, err = dialerClient.Peerings().Establish(context.Background(), api.PeeringEstablishRequest{PeerName: "peering", PeeringToken: generatedToken.PeeringToken}, nil) require.NoError(t, err) k8sObjects = append(k8sObjects, createSecret("dialer-token-old", "default", "token", "old-token")) + // Create a new token to be potentially used by Reconcile(). The original token has already been + // used once to simulate establishing an existing peering. + token, _, err := acceptorClient.Peerings().GenerateToken(context.Background(), api.PeeringGenerateTokenRequest{PeerName: "peering"}, nil) + secret := createSecret("dialer-token", "default", "token", token.PeeringToken) + secret.SetResourceVersion("latest-version") + k8sObjects = append(k8sObjects, secret) + s := scheme.Scheme s.AddKnownTypes(v1alpha1.GroupVersion, &v1alpha1.PeeringDialer{}, &v1alpha1.PeeringDialerList{}) fakeClient := fake.NewClientBuilder().WithScheme(s).WithRuntimeObjects(k8sObjects...).Build() From 61d9c1e3d823bd0f890b2313c6b1acefc7166f9b Mon Sep 17 00:00:00 2001 From: Nitya Dhanushkodi Date: Mon, 8 Aug 2022 22:14:06 -0700 Subject: [PATCH 2/3] fix lint --- control-plane/connect-inject/peering_dialer_controller_test.go | 1 + 1 file changed, 1 insertion(+) diff --git a/control-plane/connect-inject/peering_dialer_controller_test.go b/control-plane/connect-inject/peering_dialer_controller_test.go index c5a6a6bdf0..94cc38bcf8 100644 --- a/control-plane/connect-inject/peering_dialer_controller_test.go +++ b/control-plane/connect-inject/peering_dialer_controller_test.go @@ -500,6 +500,7 @@ func TestReconcile_VersionAnnotationPeeringDialer(t *testing.T) { // Create a new token to be potentially used by Reconcile(). The original token has already been // used once to simulate establishing an existing peering. token, _, err := acceptorClient.Peerings().GenerateToken(context.Background(), api.PeeringGenerateTokenRequest{PeerName: "peering"}, nil) + require.NoError(t, err) secret := createSecret("dialer-token", "default", "token", token.PeeringToken) secret.SetResourceVersion("latest-version") k8sObjects = append(k8sObjects, secret) From 4fafa90e27dfe3784b43dd8c73b9fd3fef20f058 Mon Sep 17 00:00:00 2001 From: Nitya Dhanushkodi Date: Tue, 9 Aug 2022 15:21:17 -0700 Subject: [PATCH 3/3] use released versions --- .github/workflows/test.yml | 14 ++++++++------ 1 file changed, 8 insertions(+), 6 deletions(-) diff --git a/.github/workflows/test.yml b/.github/workflows/test.yml index 9b828c8739..6b90795000 100644 --- a/.github/workflows/test.yml +++ b/.github/workflows/test.yml @@ -4,8 +4,8 @@ on: env: TEST_RESULTS: /tmp/test-results # path to where test results are saved - CONSUL_VERSION: 1.13.0-alpha2 # Consul's OSS version to use in tests - CONSUL_ENT_VERSION: 1.13.0-alpha2+ent # Consul's enterprise version to use in tests + CONSUL_VERSION: 1.13.0 # Consul's OSS version to use in tests + CONSUL_ENT_VERSION: 1.13.0+ent # Consul's enterprise version to use in tests GOTESTSUM_VERSION: 1.8.1 # You cannot use environment variables with workflows. The gotestsum version is hardcoded in the reusable workflows too. jobs: @@ -146,8 +146,9 @@ jobs: working-directory: control-plane run: | mkdir -p $HOME/bin - wget https://github.com/ndhanushkodi/binaries/releases/download/v6oss/consul -O consulbin && \ - mv consulbin $HOME/bin/consul && + wget https://releases.hashicorp.com/consul/${{env.CONSUL_VERSION}}/consul_${{env.CONSUL_VERSION}}_linux_amd64.zip && \ + unzip consul_${{env.CONSUL_VERSION}}_linux_amd64.zip -d $HOME/bin && \ + rm consul_${{env.CONSUL_VERSION}}_linux_amd64.zip chmod +x $HOME/bin/consul - name: Run go tests @@ -194,8 +195,9 @@ jobs: working-directory: control-plane run: | mkdir -p $HOME/bin - wget https://github.com/ndhanushkodi/binaries/releases/download/v6ent/consul -O consulbin && \ - mv consulbin $HOME/bin/consul && + wget https://releases.hashicorp.com/consul/${{env.CONSUL_ENT_VERSION}}/consul_${{env.CONSUL_ENT_VERSION}}_linux_amd64.zip && \ + unzip consul_${{env.CONSUL_ENT_VERSION}}_linux_amd64.zip -d $HOME/bin && \ + rm consul_${{env.CONSUL_ENT_VERSION}}_linux_amd64.zip chmod +x $HOME/bin/consul - name: Run go tests