From fac59df35392377d61f7dc0be27a5a854169d687 Mon Sep 17 00:00:00 2001 From: Luke Kysow <1034429+lkysow@users.noreply.github.com> Date: Mon, 4 May 2020 18:45:23 -0700 Subject: [PATCH] code review updates --- .../create-federation-secret/command.go | 104 ++++++------- .../create-federation-secret/command_test.go | 146 ++++++++++++------ 2 files changed, 153 insertions(+), 97 deletions(-) diff --git a/subcommand/create-federation-secret/command.go b/subcommand/create-federation-secret/command.go index a5f0b626e6..0ecefc6eb1 100644 --- a/subcommand/create-federation-secret/command.go +++ b/subcommand/create-federation-secret/command.go @@ -45,6 +45,15 @@ type Command struct { // token in the secret. flagExportReplicationToken bool flagGossipKeyFile string + + // flagServerCACertFile is the location of the file containing the CA cert + // for servers. We also accept a -ca-file flag. This will point to a different + // file when auto-encrypt is enabled, otherwise it will point to the same file + // as -server-ca-cert-file. + // When auto-encrypt is enabled, the clients + // use a different CA than the servers (since they piggy-back on the Connect CA) + // and so when talking to our local client we need to use the CA cert passed + // via -ca-file, not the server CA. flagServerCACertFile string flagServerCAKeyFile string flagResourcePrefix string @@ -87,7 +96,6 @@ func (c *Command) init() { flags.Merge(c.flags, c.http.ClientFlags()) flags.Merge(c.flags, c.http.ServerFlags()) flags.Merge(c.flags, c.k8s.Flags()) - } // Run creates a Kubernetes secret with data needed by secondary datacenters @@ -96,35 +104,7 @@ func (c *Command) init() { func (c *Command) Run(args []string) int { c.once.Do(c.init) - // Validate flags. - if err := c.flags.Parse(args); err != nil { - return 1 - } - if len(c.flags.Args()) > 0 { - c.UI.Error("Should have no non-flag arguments") - return 1 - } - if c.flagResourcePrefix == "" { - c.UI.Error("-resource-prefix must be set") - return 1 - } - if c.flagK8sNamespace == "" { - c.UI.Error("-k8s-namespace must be set") - return 1 - } - if c.flagServerCACertFile == "" { - c.UI.Error("-server-ca-cert-file must be set") - return 1 - } - if c.flagServerCAKeyFile == "" { - c.UI.Error("-server-ca-key-file must be set") - return 1 - } - if c.flagMeshGatewayServiceName == "" { - c.UI.Error("-mesh-gateway-service-name must be set") - return 1 - } - if err := c.validateCAFileFlag(); err != nil { + if err := c.validateFlags(args); err != nil { c.UI.Error(err.Error()) return 1 } @@ -159,18 +139,15 @@ func (c *Command) Run(args []string) int { c.UI.Error(fmt.Sprintf("Error reading gossip encryption key file: %s", err)) return 1 } + if len(gossipKey) == 0 { + c.UI.Error(fmt.Sprintf("gossip key file %q was empty", c.flagGossipKeyFile)) + return 1 + } federationSecret.Data[fedSecretGossipKey] = gossipKey logger.Info("Gossip encryption key retrieved successfully") } - // Add CA cert. - // Note: This CA cert may be different from the one passed into the -ca-file - // flag if auto-encrypt is enabled. When auto-encrypt is enabled, the clients - // use a different CA than the servers (since they piggy-back on the Connect CA). - // In this case, we need to include the server CA in our federation secret - // because federation is server to server communication but we need to use - // the auto-encrypt CA when we talk to our local Consul client to get data - // like the dc name and mesh gateway addresses. + // Add server CA cert. logger.Info("Retrieving server CA cert data") caCert, err := ioutil.ReadFile(c.flagServerCACertFile) if err != nil { @@ -180,7 +157,7 @@ func (c *Command) Run(args []string) int { federationSecret.Data[fedSecretCACertKey] = caCert logger.Info("Server CA cert retrieved successfully") - // Add CA key. + // Add server CA key. logger.Info("Retrieving server CA key data") caKey, err := ioutil.ReadFile(c.flagServerCAKeyFile) if err != nil { @@ -279,6 +256,34 @@ func (c *Command) Run(args []string) int { return 0 } +func (c *Command) validateFlags(args []string) error { + if err := c.flags.Parse(args); err != nil { + return err + } + if len(c.flags.Args()) > 0 { + return errors.New("should have no non-flag arguments") + } + if c.flagResourcePrefix == "" { + return errors.New("-resource-prefix must be set") + } + if c.flagK8sNamespace == "" { + return errors.New("-k8s-namespace must be set") + } + if c.flagServerCACertFile == "" { + return errors.New("-server-ca-cert-file must be set") + } + if c.flagServerCAKeyFile == "" { + return errors.New("-server-ca-key-file must be set") + } + if c.flagMeshGatewayServiceName == "" { + return errors.New("-mesh-gateway-service-name must be set") + } + if err := c.validateCAFileFlag(); err != nil { + return err + } + return nil +} + // replicationToken waits for the ACL replication token Kubernetes secret to // be created and then returns it. func (c *Command) replicationToken(logger hclog.Logger) ([]byte, error) { @@ -299,8 +304,8 @@ func (c *Command) replicationToken(logger hclog.Logger) ([]byte, error) { logger.Warn("secret not yet created, retrying", "secret", secretName, "ns", c.flagK8sNamespace) return errors.New("") } else if err != nil { - logger.Error("error retrieving secret", "secret", secretName, "ns", c.flagK8sNamespace, "err", err) - return err + unrecoverableErr = err + return nil } var ok bool token, ok = secret.Data[common.ACLTokenSecretKey] @@ -413,21 +418,12 @@ func (c *Command) consulDatacenter(logger hclog.Logger) string { // validateCAFileFlag returns an error if the -ca-file flag (or its env var // CONSUL_CACERT) isn't set or the file it points to can't be read. func (c *Command) validateCAFileFlag() error { - // In order to ensure the -ca-file or CONSUL_CACERT env var is set, we - // need to use the MergeOntoConfig function and then inspect the config struct - // because we can't access that flag directly from c.http. - consulCfg := &api.Config{ - TLSConfig: api.TLSConfig{}, - } - c.http.MergeOntoConfig(consulCfg) - caFile := consulCfg.TLSConfig.CAFile - if caFile == "" { - caFile = os.Getenv("CONSUL_CACERT") - } - if caFile == "" { + cfg := api.DefaultConfig() + c.http.MergeOntoConfig(cfg) + if cfg.TLSConfig.CAFile == "" { return errors.New("-ca-file or CONSUL_CACERT must be set") } - _, err := ioutil.ReadFile(caFile) + _, err := ioutil.ReadFile(cfg.TLSConfig.CAFile) if err != nil { return fmt.Errorf("error reading CA file: %s", err) } diff --git a/subcommand/create-federation-secret/command_test.go b/subcommand/create-federation-secret/command_test.go index c2984b375d..ecb4904258 100644 --- a/subcommand/create-federation-secret/command_test.go +++ b/subcommand/create-federation-secret/command_test.go @@ -7,6 +7,7 @@ import ( "os" "strconv" "strings" + "sync" "testing" "time" @@ -105,7 +106,7 @@ func TestRun_CAFileMissing(t *testing.T) { require.Contains(t, ui.ErrorWriter.String(), "error reading CA file") } -func TestRun_ServerCACertMissing(t *testing.T) { +func TestRun_ServerCACertFileMissing(t *testing.T) { t.Parallel() f, err := ioutil.TempFile("", "") require.NoError(t, err) @@ -127,7 +128,7 @@ func TestRun_ServerCACertMissing(t *testing.T) { require.Contains(t, ui.ErrorWriter.String(), "Error reading server CA cert file") } -func TestRun_ServerCAKeyMissing(t *testing.T) { +func TestRun_ServerCAKeyFileMissing(t *testing.T) { t.Parallel() f, err := ioutil.TempFile("", "") require.NoError(t, err) @@ -149,7 +150,7 @@ func TestRun_ServerCAKeyMissing(t *testing.T) { require.Contains(t, ui.ErrorWriter.String(), "Error reading server CA key file") } -func TestRun_GossipEncryptionKeyMissing(t *testing.T) { +func TestRun_GossipEncryptionKeyFileMissing(t *testing.T) { t.Parallel() f, err := ioutil.TempFile("", "") require.NoError(t, err) @@ -172,8 +173,31 @@ func TestRun_GossipEncryptionKeyMissing(t *testing.T) { require.Contains(t, ui.ErrorWriter.String(), "Error reading gossip encryption key file") } +func TestRun_GossipEncryptionKeyFileEmpty(t *testing.T) { + t.Parallel() + f, err := ioutil.TempFile("", "") + require.NoError(t, err) + defer os.Remove(f.Name()) + + ui := cli.NewMockUi() + cmd := Command{ + UI: ui, + } + exitCode := cmd.Run([]string{ + "-resource-prefix=prefix", + "-k8s-namespace=default", + "-mesh-gateway-service-name=name", + "-ca-file", f.Name(), + "-server-ca-cert-file", f.Name(), + "-server-ca-key-file", f.Name(), + "-gossip-key-file", f.Name(), + }) + require.Equal(t, 1, exitCode, ui.ErrorWriter.String()) + require.Contains(t, ui.ErrorWriter.String(), fmt.Sprintf("gossip key file %q was empty", f.Name())) +} + // Test when the replication secret exists but it's missing the expected -// token key. +// token key, we return error. func TestRun_ReplicationTokenMissingExpectedKey(t *testing.T) { t.Parallel() f, err := ioutil.TempFile("", "") @@ -185,7 +209,7 @@ func TestRun_ReplicationTokenMissingExpectedKey(t *testing.T) { k8sNS := "default" k8s.CoreV1().Secrets(k8sNS).Create(&v1.Secret{ ObjectMeta: metav1.ObjectMeta{ - Name: "prefix-acl-replication-acl-token", + Name: "prefix-" + common.ACLReplicationTokenName + "-acl-token", }, }) cmd := Command{ @@ -288,7 +312,11 @@ func TestRun_ACLs_K8SNamespaces_ResourcePrefixes(tt *testing.T) { // Construct Consul client. client, err := api.NewClient(&api.Config{ - Address: a.HTTPAddr, + Address: a.HTTPSAddr, + Scheme: "https", + TLSConfig: api.TLSConfig{ + CAFile: caFile, + }, }) require.NoError(t, err) @@ -299,18 +327,22 @@ func TestRun_ACLs_K8SNamespaces_ResourcePrefixes(tt *testing.T) { timer := &retry.Timer{Timeout: 10 * time.Second, Wait: 500 * time.Millisecond} // May need to retry bootstrapping until server has elected // leader. - retry.RunWith(timer, tt, func(r *retry.R) { + retry.RunWith(timer, t, func(r *retry.R) { bootstrapResp, _, err = client.ACL().Bootstrap() require.NoError(r, err) }) bootstrapToken := bootstrapResp.SecretID - require.NotEmpty(tt, bootstrapToken) + require.NotEmpty(t, bootstrapToken) // Redefine the client with the bootstrap token set so // subsequent calls will succeed. client, err = api.NewClient(&api.Config{ - Address: a.HTTPAddr, - Token: bootstrapToken, + Address: a.HTTPSAddr, + Scheme: "https", + TLSConfig: api.TLSConfig{ + CAFile: caFile, + }, + Token: bootstrapToken, }) require.NoError(t, err) @@ -356,7 +388,7 @@ func TestRun_ACLs_K8SNamespaces_ResourcePrefixes(tt *testing.T) { Name: c.ResourcePrefix + "-acl-replication-acl-token", }, Data: map[string][]byte{ - "token": []byte(replicationToken), + common.ACLTokenSecretKey: []byte(replicationToken), }, }) require.NoError(t, err) @@ -366,18 +398,6 @@ func TestRun_ACLs_K8SNamespaces_ResourcePrefixes(tt *testing.T) { gossipEncryptionKey := "oGaLv60gQ0E+Uvn+Lokz9APjbu5fJaYx7kglOmg4jZc=" var gossipKeyFile string if c.GossipKey { - gossipEncryptionSecretName := "gossip-encryption" - gossipEncryptionSecretKey := "key" - _, err := k8s.CoreV1().Secrets(c.K8SNS).Create(&v1.Secret{ - ObjectMeta: metav1.ObjectMeta{ - Name: gossipEncryptionSecretName, - }, - Data: map[string][]byte{ - gossipEncryptionSecretKey: []byte(gossipEncryptionKey), - }, - }) - require.NoError(t, err) - f, err := ioutil.TempFile("", "") require.NoError(t, err) err = ioutil.WriteFile(f.Name(), []byte(gossipEncryptionKey), 0644) @@ -471,7 +491,11 @@ func TestRun_WaitsForMeshGatewayInstances(t *testing.T) { go func() { time.Sleep(500 * time.Millisecond) client, err := api.NewClient(&api.Config{ - Address: a.HTTPAddr, + Address: a.HTTPSAddr, + Scheme: "https", + TLSConfig: api.TLSConfig{ + CAFile: caFile, + }, }) require.NoError(t, err) err = client.Agent().ServiceRegister(&api.AgentServiceRegistration{ @@ -530,7 +554,11 @@ func TestRun_MeshGatewayNoWANAddr(t *testing.T) { require.NoError(t, err) defer a.Stop() client, err := api.NewClient(&api.Config{ - Address: a.HTTPAddr, + Address: a.HTTPSAddr, + Scheme: "https", + TLSConfig: api.TLSConfig{ + CAFile: caFile, + }, }) require.NoError(t, err) err = client.Agent().ServiceRegister(&api.AgentServiceRegistration{ @@ -579,7 +607,11 @@ func TestRun_MeshGatewayNames(tt *testing.T) { // Create a mesh gateway instance. client, err := api.NewClient(&api.Config{ - Address: a.HTTPAddr, + Address: a.HTTPSAddr, + Scheme: "https", + TLSConfig: api.TLSConfig{ + CAFile: caFile, + }, }) require.NoError(t, err) meshGWIP := "192.168.0.1" @@ -668,7 +700,11 @@ func TestRun_MeshGatewayUniqueAddrs(tt *testing.T) { // Create mesh gateway instances. client, err := api.NewClient(&api.Config{ - Address: a.HTTPAddr, + Address: a.HTTPSAddr, + Scheme: "https", + TLSConfig: api.TLSConfig{ + CAFile: caFile, + }, }) require.NoError(t, err) for i, addr := range c.Addrs { @@ -744,7 +780,11 @@ func TestRun_ReplicationSecretDelay(t *testing.T) { // Construct Consul client. client, err := api.NewClient(&api.Config{ - Address: a.HTTPAddr, + Address: a.HTTPSAddr, + Scheme: "https", + TLSConfig: api.TLSConfig{ + CAFile: caFile, + }, }) require.NoError(t, err) @@ -765,14 +805,18 @@ func TestRun_ReplicationSecretDelay(t *testing.T) { // Redefine the client with the bootstrap token set so // subsequent calls will succeed. client, err = api.NewClient(&api.Config{ - Address: a.HTTPAddr, - Token: bootstrapToken, + Address: a.HTTPSAddr, + Scheme: "https", + TLSConfig: api.TLSConfig{ + CAFile: caFile, + }, + Token: bootstrapToken, }) require.NoError(t, err) // Create a token for the replication policy. _, _, err = client.ACL().PolicyCreate(&api.ACLPolicy{ - Name: "acl-replication-token", + Name: "acl-replication-policy", Rules: replicationPolicy, }, nil) require.NoError(t, err) @@ -780,7 +824,7 @@ func TestRun_ReplicationSecretDelay(t *testing.T) { resp, _, err := client.ACL().TokenCreate(&api.ACLToken{ Policies: []*api.ACLTokenPolicyLink{ { - Name: "acl-replication-token", + Name: "acl-replication-policy", }, }, }, nil) @@ -809,10 +853,10 @@ func TestRun_ReplicationSecretDelay(t *testing.T) { time.Sleep(400 * time.Millisecond) _, err := k8s.CoreV1().Secrets("default").Create(&v1.Secret{ ObjectMeta: metav1.ObjectMeta{ - Name: "prefix-acl-replication-acl-token", + Name: "prefix-" + common.ACLReplicationTokenName + "-acl-token", }, Data: map[string][]byte{ - "token": []byte(replicationToken), + common.ACLTokenSecretKey: []byte(replicationToken), }, }) require.NoError(t, err) @@ -844,7 +888,7 @@ func TestRun_ReplicationSecretDelay(t *testing.T) { require.Equal(t, replicationToken, string(secret.Data["replicationToken"])) } -// Test that re-running the command update the secret. In this test we'll +// Test that re-running the command updates the secret. In this test, we'll // update the addresses of the mesh gateways. func TestRun_UpdatesSecret(t *testing.T) { t.Parallel() @@ -865,7 +909,11 @@ func TestRun_UpdatesSecret(t *testing.T) { // Create a mesh gateway instance. client, err := api.NewClient(&api.Config{ - Address: a.HTTPAddr, + Address: a.HTTPSAddr, + Scheme: "https", + TLSConfig: api.TLSConfig{ + CAFile: caFile, + }, }) require.NoError(t, err) meshGWIP := "192.168.0.1" @@ -968,10 +1016,14 @@ func TestRun_ConsulClientDelay(t *testing.T) { k8s := fake.NewSimpleClientset() // Set up Consul server with TLS. Start after a 500ms delay. - var consulCleanup func() error + var a *testutil.TestServer + wg := sync.WaitGroup{} + wg.Add(1) go func() { + defer wg.Done() time.Sleep(500 * time.Millisecond) - a, err := testutil.NewTestServerConfigT(t, func(cfg *testutil.TestServerConfig) { + var err error + a, err = testutil.NewTestServerConfigT(t, func(cfg *testutil.TestServerConfig) { cfg.CAFile = caFile cfg.CertFile = certFile cfg.KeyFile = keyFile @@ -985,11 +1037,14 @@ func TestRun_ConsulClientDelay(t *testing.T) { } }) require.NoError(t, err) - consulCleanup = a.Stop // Construct Consul client. client, err := api.NewClient(&api.Config{ - Address: a.HTTPAddr, + Address: a.HTTPSAddr, + Scheme: "https", + TLSConfig: api.TLSConfig{ + CAFile: caFile, + }, }) require.NoError(t, err) @@ -1008,8 +1063,8 @@ func TestRun_ConsulClientDelay(t *testing.T) { require.NoError(t, err) }() defer func() { - if consulCleanup != nil { - consulCleanup() + if a != nil { + a.Stop() } }() @@ -1032,6 +1087,7 @@ func TestRun_ConsulClientDelay(t *testing.T) { require.Equal(t, 0, exitCode, ui.ErrorWriter.String()) // Check the secret is as expected. + wg.Wait() _, err := k8s.CoreV1().Secrets("default").Get("prefix-federation", metav1.GetOptions{}) require.NoError(t, err) } @@ -1057,7 +1113,11 @@ func TestRun_Autoencrypt(t *testing.T) { // Create a mesh gateway instance. client, err := api.NewClient(&api.Config{ - Address: a.HTTPAddr, + Address: a.HTTPSAddr, + Scheme: "https", + TLSConfig: api.TLSConfig{ + CAFile: caFile, + }, }) require.NoError(t, err) meshGWIP := "192.168.0.1"