From 428aea5094473059aebcfaaaae2485aeed344b47 Mon Sep 17 00:00:00 2001 From: Armon Dadgar Date: Sat, 19 Aug 2017 15:30:01 -0700 Subject: [PATCH] Address @dadgar feedback --- command/agent/config-test-fixtures/basic.hcl | 4 ++-- command/agent/config.go | 14 +++++------ command/agent/config_parse_test.go | 4 ++-- nomad/leader.go | 25 +++++++++++++------- 4 files changed, 26 insertions(+), 21 deletions(-) diff --git a/command/agent/config-test-fixtures/basic.hcl b/command/agent/config-test-fixtures/basic.hcl index f80e249b4..7e4a84309 100644 --- a/command/agent/config-test-fixtures/basic.hcl +++ b/command/agent/config-test-fixtures/basic.hcl @@ -85,8 +85,8 @@ server { } acl { enabled = true - token_ttl = "30s" - policy_ttl = "30s" + token_ttl = "60s" + policy_ttl = "60s" } telemetry { statsite_address = "127.0.0.1:1234" diff --git a/command/agent/config.go b/command/agent/config.go index f4aa50aa3..6aa0461b5 100644 --- a/command/agent/config.go +++ b/command/agent/config.go @@ -240,15 +240,13 @@ type ACLConfig struct { // how stale they can be when we are enforcing policies. Defaults // to "30s". Reducing this impacts performance by forcing more // frequent resolution. - TokenTTL string `mapstructure:"token_ttl"` - tokenTTL time.Duration `mapstructure:"-"` + TokenTTL time.Duration `mapstructure:"token_ttl"` // PolicyTTL controls how long we cache ACL policies. This controls // how stale they can be when we are enforcing policies. Defaults // to "30s". Reducing this impacts performance by forcing more // frequent resolution. - PolicyTTL string `mapstructure:"policy_ttl"` - policyTTL time.Duration `mapstructure:"-"` + PolicyTTL time.Duration `mapstructure:"policy_ttl"` } // ServerConfig is configuration specific to the server mode @@ -594,8 +592,8 @@ func DefaultConfig() *Config { }, ACL: &ACLConfig{ Enabled: false, - TokenTTL: "30s", - PolicyTTL: "30s", + TokenTTL: 30 * time.Second, + PolicyTTL: 30 * time.Second, }, SyslogFacility: "LOCAL0", Telemetry: &Telemetry{ @@ -949,10 +947,10 @@ func (a *ACLConfig) Merge(b *ACLConfig) *ACLConfig { if b.Enabled { result.Enabled = true } - if b.TokenTTL != "" { + if b.TokenTTL != 0 { result.TokenTTL = b.TokenTTL } - if b.PolicyTTL != "" { + if b.PolicyTTL != 0 { result.PolicyTTL = b.PolicyTTL } return &result diff --git a/command/agent/config_parse_test.go b/command/agent/config_parse_test.go index aa69f09f0..dcf7a05b7 100644 --- a/command/agent/config_parse_test.go +++ b/command/agent/config_parse_test.go @@ -106,8 +106,8 @@ func TestConfig_Parse(t *testing.T) { }, ACL: &ACLConfig{ Enabled: true, - TokenTTL: "30s", - PolicyTTL: "30s", + TokenTTL: 60 * time.Second, + PolicyTTL: 60 * time.Second, }, Telemetry: &Telemetry{ StatsiteAddr: "127.0.0.1:1234", diff --git a/nomad/leader.go b/nomad/leader.go index 70e13105b..9c92d9692 100644 --- a/nomad/leader.go +++ b/nomad/leader.go @@ -672,10 +672,13 @@ REMOVE: // replicateACLPolicies is used to replicate ACL policies from // the authoritative region to this region. func (s *Server) replicateACLPolicies(stopCh chan struct{}) { - req := structs.ACLPolicyListRequest{} - req.Region = s.config.AuthoritativeRegion + req := structs.ACLPolicyListRequest{ + QueryOptions: structs.QueryOptions{ + Region: s.config.AuthoritativeRegion, + }, + } limiter := rate.NewLimiter(replicationRateLimit, int(replicationRateLimit)) - s.logger.Printf("[DEBUG] nomad: starting ACL policy replication from authoritative region '%s'", req.Region) + s.logger.Printf("[DEBUG] nomad: starting ACL policy replication from authoritative region %q", req.Region) START: for { @@ -711,6 +714,7 @@ START: } // Fetch any outdated policies + // TODO: Optimize this fetching to batch all the requests. var fetched []*structs.ACLPolicy for _, policyName := range update { req := structs.ACLPolicySpecificRequest{ @@ -721,7 +725,7 @@ START: err := s.forwardRegion(s.config.AuthoritativeRegion, "ACL.GetPolicy", &req, &reply) if err != nil { - s.logger.Printf("[ERR] nomad: failed to fetch policy '%s' from authoritative region: %v", policyName, err) + s.logger.Printf("[ERR] nomad: failed to fetch policy %q from authoritative region: %v", policyName, err) goto ERR_WAIT } if reply.Policy != nil { @@ -809,10 +813,12 @@ func diffACLPolicies(state *state.StateStore, minIndex uint64, remoteList []*str func (s *Server) replicateACLTokens(stopCh chan struct{}) { req := structs.ACLTokenListRequest{ GlobalOnly: true, + QueryOptions: structs.QueryOptions{ + Region: s.config.AuthoritativeRegion, + }, } - req.Region = s.config.AuthoritativeRegion limiter := rate.NewLimiter(replicationRateLimit, int(replicationRateLimit)) - s.logger.Printf("[DEBUG] nomad: starting ACL token replication from authoritative region '%s'", req.Region) + s.logger.Printf("[DEBUG] nomad: starting ACL token replication from authoritative region %q", req.Region) START: for { @@ -847,7 +853,8 @@ START: } } - // Fetch any outdated policies + // Fetch any outdated policies. + // TODO: Optimize this fetching to batch all the requests. var fetched []*structs.ACLToken for _, tokenID := range update { req := structs.ACLTokenSpecificRequest{ @@ -858,7 +865,7 @@ START: err := s.forwardRegion(s.config.AuthoritativeRegion, "ACL.GetToken", &req, &reply) if err != nil { - s.logger.Printf("[ERR] nomad: failed to fetch token '%s' from authoritative region: %v", tokenID, err) + s.logger.Printf("[ERR] nomad: failed to fetch token %q from authoritative region: %v", tokenID, err) goto ERR_WAIT } if reply.Token != nil { @@ -866,7 +873,7 @@ START: } } - // Update local tokensj + // Update local tokens if len(fetched) > 0 { args := &structs.ACLTokenUpsertRequest{ Tokens: fetched,