Skip to content

Commit

Permalink
Code review updates
Browse files Browse the repository at this point in the history
  • Loading branch information
@lkysow
lkysow committed Mar 13, 2020
1 parent 34996f1 commit 0c3541c
Show file tree
Hide file tree
Showing 3 changed files with 16 additions and 29 deletions.
16 changes: 4 additions & 12 deletions subcommand/server-acl-init/command.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -42,7 +42,6 @@ type Command struct {
flagCreateSnapshotAgentToken bool
flagCreateMeshGatewayToken bool
flagCreateACLReplicationToken bool
flagEnableACLReplication bool
flagACLReplicationTokenFile string
flagConsulCACert string
flagConsulTLSServerName string
Expand Down Expand Up @@ -130,10 +129,8 @@ func (c *Command) init() {
c.flags.StringVar(&c.flagInjectK8SNSMirroringPrefix, "inject-k8s-namespace-mirroring-prefix", "",
"[Enterprise Only] Prefix that will be added to all k8s namespaces mirrored into Consul by Connect inject "+
"if mirroring is enabled.")
c.flags.BoolVar(&c.flagEnableACLReplication, "enable-acl-replication", false,
"Enables ACL replication. If true, -acl-replication-token-file must be set")
c.flags.StringVar(&c.flagACLReplicationTokenFile, "acl-replication-token-file", "",
"Path to file containing ACL token to be used for ACL replication.")
"Path to file containing ACL token to be used for ACL replication. If set, ACL replication is enabled.")
c.flags.StringVar(&c.flagTimeout, "timeout", "10m",
"How long we'll try to bootstrap ACLs for before timing out, e.g. 1ms, 2s, 3m")
c.flags.StringVar(&c.flagLogLevel, "log-level", "info",
Expand Down Expand Up @@ -192,12 +189,7 @@ func (c *Command) Run(args []string) int {
c.flagServerLabelSelector = fmt.Sprintf("app=consul,component=server,release=%s", c.flagReleaseName)
}
var aclReplicationToken string
if c.flagEnableACLReplication {
if c.flagACLReplicationTokenFile == "" {
c.UI.Error("If -enable-acl-replication is true, -acl-replication-token-file must be set")
return 1
}

if c.flagACLReplicationTokenFile != "" {
// Load the ACL replication token from file.
tokenBytes, err := ioutil.ReadFile(c.flagACLReplicationTokenFile)
if err != nil {
Expand Down Expand Up @@ -260,7 +252,7 @@ func (c *Command) Run(args []string) int {
var updateServerPolicy bool
var bootstrapToken string

if c.flagEnableACLReplication {
if c.flagACLReplicationTokenFile != "" {
// If ACL replication is enabled, we don't need to ACL bootstrap the servers
// since they will be performing replication.
// We can use the replication token as our bootstrap token because it
Expand Down Expand Up @@ -392,7 +384,7 @@ func (c *Command) Run(args []string) int {
// token. We don't want to modify the DNS policy in secondary datacenters
// because it is global and we can't create separate tokens for each
// secondary datacenter because the anonymous token is global.
if c.flagAllowDNS && !c.flagEnableACLReplication {
if c.flagAllowDNS && c.flagACLReplicationTokenFile == "" {
err := c.configureDNSPolicies(consulClient)
if err != nil {
c.Log.Error(err.Error())
Expand Down
23 changes: 9 additions & 14 deletions subcommand/server-acl-init/command_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -50,12 +50,8 @@ func TestRun_FlagValidation(t *testing.T) {
ExpErr: "if -server-label-selector is set -resource-prefix must also be set",
},
{
Flags: []string{"-enable-acl-replication", "-server-label-selector=hi", "-resource-prefix=prefix"},
ExpErr: "if -enable-acl-replication is true, -acl-replication-token-file must be set",
},
{
Flags: []string{"-enable-acl-replication", "-acl-replication-token-file=/notexist", "-server-label-selector=hi", "-resource-prefix=prefix"},
ExpErr: "Unable to read acl replication token from file \"/notexist\": open /notexist: no such file or directory",
Flags: []string{"-acl-replication-token-file=/notexist", "-server-label-selector=hi", "-resource-prefix=prefix"},
ExpErr: "Unable to read ACL replication token from file \"/notexist\": open /notexist: no such file or directory",
},
}

Expand Down Expand Up @@ -412,7 +408,6 @@ func TestRun_TokensReplicatedDC(t *testing.T) {
"-k8s-namespace=" + ns,
"-expected-replicas=1",
"-acl-replication-token-file", tokenFile,
"-enable-acl-replication",
"-server-label-selector=component=server,app=consul,release=" + releaseName,
"-resource-prefix=" + resourcePrefix,
c.TokenFlag,
Expand Down Expand Up @@ -1542,13 +1537,16 @@ func TestRun_ACLReplicationTokenValid(t *testing.T) {
require.NoError(t, err)
defer secondarySvr.Stop()

// When running via Helm, the WAN join config is set in the server config
// however that config isn't exposed by the testutil server so instead we need to
// make the join API call. This requires the bootstrap token permissions.
bootToken := getBootToken(t, primaryK8s, resourcePrefix, ns)
secondaryConsulClient, err := api.NewClient(&api.Config{
Address: secondarySvr.HTTPAddr,
Token: bootToken,
// When running via Helm, the WAN join config is set in the server config
// however that config isn't exposed by the testutil server so instead we need to
// make the join API call. This requires the bootstrap token permissions.
// The token here isn't that important because we're only using this
// client to make our own test assertions. The underlying code we're
// testing uses the replication token.
Token: bootToken,
})
err = secondaryConsulClient.Agent().Join(primarySvr.WANAddr, true)
require.NoError(t, err)
Expand All @@ -1570,7 +1568,6 @@ func TestRun_ACLReplicationTokenValid(t *testing.T) {
"-expected-replicas=1",
"-server-label-selector=component=server,app=consul,release=" + releaseName,
"-resource-prefix=" + resourcePrefix,
"-enable-acl-replication",
"-acl-replication-token-file", tokenFile,
"-create-client-token",
"-create-mesh-gateway-token",
Expand All @@ -1583,7 +1580,6 @@ func TestRun_ACLReplicationTokenValid(t *testing.T) {
replicationStatus, _, err := secondaryConsulClient.ACL().Replication(nil)
require.NoError(t, err)
require.True(t, replicationStatus.Enabled)
var uintZero uint64 = 0
require.Greater(t, replicationStatus.ReplicatedIndex, uint64(0))
})

Expand Down Expand Up @@ -1619,7 +1615,6 @@ func TestRun_AllowDNSFlag_IgnoredWithReplication(t *testing.T) {
cmdArgs := []string{
"-k8s-namespace=" + ns,
"-expected-replicas=1",
"-enable-acl-replication",
"-acl-replication-token-file", tokenFile,
"-server-label-selector=component=server,app=consul,release=" + releaseName,
"-resource-prefix=" + resourcePrefix,
Expand Down
6 changes: 3 additions & 3 deletions subcommand/server-acl-init/create_or_update.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -24,13 +24,13 @@ func (c *Command) createGlobalACL(name, rules, dc string, consulClient *api.Clie
}

// createACL creates a policy with rules and name. If localToken is true then
// the token will be a local token. If dc is not empty, the policy will
// be scoped to just that datacenter, otherwise it will be global.
// the token will be a local token and the policy will be scoped to only dc.
// If localToken is false, the policy will be global.
// The token will be written to a Kubernetes secret.
func (c *Command) createACL(name, rules string, localToken bool, dc string, consulClient *api.Client) error {
// Create policy with the given rules.
policyName := fmt.Sprintf("%s-token", name)
if c.flagEnableACLReplication {
if c.flagACLReplicationTokenFile != "" {
// If performing ACL replication, we must ensure policy names are
// globally unique so we append the datacenter name.
policyName += fmt.Sprintf("-%s", dc)
Expand Down

0 comments on commit 0c3541c

Please sign in to comment.