From 7928305279788def909b2f7b54a9d394774a138a Mon Sep 17 00:00:00 2001 From: "R.B. Boyer" Date: Mon, 15 Apr 2019 13:35:55 -0500 Subject: [PATCH] making ACLToken.ExpirationTime a *time.Time value instead of time.Time (#5663) This is mainly to avoid having the API return "0001-01-01T00:00:00Z" as a value for the ExpirationTime field when it is not set. Unfortunately time.Time doesn't respect the json marshalling "omitempty" directive. --- agent/consul/acl_endpoint.go | 22 +++++++++++++++++----- agent/consul/acl_endpoint_test.go | 28 +++++++++++++++++----------- agent/consul/state/acl.go | 11 +++++------ agent/consul/state/acl_test.go | 2 +- agent/structs/acl.go | 18 +++++++++++++----- api/acl.go | 4 ++-- command/acl/acl_helpers.go | 8 ++++---- 7 files changed, 59 insertions(+), 34 deletions(-) diff --git a/agent/consul/acl_endpoint.go b/agent/consul/acl_endpoint.go index 65be9b9e8a..cd3abecb1a 100644 --- a/agent/consul/acl_endpoint.go +++ b/agent/consul/acl_endpoint.go @@ -358,16 +358,16 @@ func (a *ACL) tokenSetInternal(args *structs.ACLTokenSetRequest, reply *structs. if token.ExpirationTTL < 0 { return fmt.Errorf("Token Expiration TTL '%s' should be > 0", token.ExpirationTTL) } - if !token.ExpirationTime.IsZero() { + if token.HasExpirationTime() { return fmt.Errorf("Token Expiration TTL and Expiration Time cannot both be set") } - token.ExpirationTime = token.CreateTime.Add(token.ExpirationTTL) + token.ExpirationTime = timePointer(token.CreateTime.Add(token.ExpirationTTL)) token.ExpirationTTL = 0 } - if !token.ExpirationTime.IsZero() { - if token.CreateTime.After(token.ExpirationTime) { + if token.HasExpirationTime() { + if token.CreateTime.After(*token.ExpirationTime) { return fmt.Errorf("ExpirationTime cannot be before CreateTime") } @@ -417,7 +417,15 @@ func (a *ACL) tokenSetInternal(args *structs.ACLTokenSetRequest, reply *structs. return fmt.Errorf("cannot toggle local mode of %s", token.AccessorID) } - if token.ExpirationTTL != 0 || !token.ExpirationTime.Equal(existing.ExpirationTime) { + if token.ExpirationTTL != 0 { + return fmt.Errorf("Cannot change expiration time of %s", token.AccessorID) + } + + if !token.HasExpirationTime() { + token.ExpirationTime = existing.ExpirationTime + } else if !existing.HasExpirationTime() { + return fmt.Errorf("Cannot change expiration time of %s", token.AccessorID) + } else if !token.ExpirationTime.Equal(*existing.ExpirationTime) { return fmt.Errorf("Cannot change expiration time of %s", token.AccessorID) } @@ -1041,3 +1049,7 @@ func (a *ACL) ReplicationStatus(args *structs.DCSpecificRequest, a.srv.aclReplicationStatusLock.RUnlock() return nil } + +func timePointer(t time.Time) *time.Time { + return &t +} diff --git a/agent/consul/acl_endpoint_test.go b/agent/consul/acl_endpoint_test.go index f2d17fc49a..49943c9113 100644 --- a/agent/consul/acl_endpoint_test.go +++ b/agent/consul/acl_endpoint_test.go @@ -1054,7 +1054,7 @@ func TestACLEndpoint_TokenSet(t *testing.T) { Description: "foobar", Policies: nil, Local: false, - ExpirationTime: time.Now().Add(test.offset), + ExpirationTime: timePointer(time.Now().Add(test.offset)), }, WriteRequest: structs.WriteRequest{Token: "root"}, } @@ -1099,7 +1099,7 @@ func TestACLEndpoint_TokenSet(t *testing.T) { Description: "foobar", Policies: nil, Local: false, - ExpirationTime: time.Now().Add(4 * time.Second), + ExpirationTime: timePointer(time.Now().Add(4 * time.Second)), ExpirationTTL: 4 * time.Second, }, WriteRequest: structs.WriteRequest{Token: "root"}, @@ -1138,7 +1138,7 @@ func TestACLEndpoint_TokenSet(t *testing.T) { require.NotNil(t, token.AccessorID) require.Equal(t, token.Description, "foobar") require.Equal(t, token.AccessorID, resp.AccessorID) - requireTimeEquals(t, expectExpTime, resp.ExpirationTime) + requireTimeEquals(t, &expectExpTime, resp.ExpirationTime) tokenID = token.AccessorID }) @@ -1152,7 +1152,7 @@ func TestACLEndpoint_TokenSet(t *testing.T) { Description: "foobar", Policies: nil, Local: false, - ExpirationTime: expTime, + ExpirationTime: &expTime, }, WriteRequest: structs.WriteRequest{Token: "root"}, } @@ -1170,7 +1170,7 @@ func TestACLEndpoint_TokenSet(t *testing.T) { require.NotNil(t, token.AccessorID) require.Equal(t, token.Description, "foobar") require.Equal(t, token.AccessorID, resp.AccessorID) - requireTimeEquals(t, expTime, resp.ExpirationTime) + requireTimeEquals(t, &expTime, resp.ExpirationTime) tokenID = token.AccessorID }) @@ -1183,7 +1183,7 @@ func TestACLEndpoint_TokenSet(t *testing.T) { ACLToken: structs.ACLToken{ Description: "new-description", AccessorID: tokenID, - ExpirationTime: expTime.Add(-1 * time.Second), + ExpirationTime: timePointer(expTime.Add(-1 * time.Second)), }, WriteRequest: structs.WriteRequest{Token: "root"}, } @@ -1202,7 +1202,7 @@ func TestACLEndpoint_TokenSet(t *testing.T) { ACLToken: structs.ACLToken{ Description: "new-description", AccessorID: tokenID, - ExpirationTime: expTime, + ExpirationTime: &expTime, }, WriteRequest: structs.WriteRequest{Token: "root"}, } @@ -1220,7 +1220,7 @@ func TestACLEndpoint_TokenSet(t *testing.T) { require.NotNil(t, token.AccessorID) require.Equal(t, token.Description, "new-description") require.Equal(t, token.AccessorID, resp.AccessorID) - requireTimeEquals(t, expTime, resp.ExpirationTime) + requireTimeEquals(t, &expTime, resp.ExpirationTime) }) t.Run("cannot update a token that is past its expiration time", func(t *testing.T) { @@ -2217,10 +2217,16 @@ func retrieveTestPolicy(codec rpc.ClientCodec, masterToken string, datacenter st return &out, nil } -func requireTimeEquals(t *testing.T, expect, got time.Time) { +func requireTimeEquals(t *testing.T, expect, got *time.Time) { t.Helper() - if !expect.Equal(got) { - t.Fatalf("expected=%q != got=%q", expect, got) + if expect == nil && got == nil { + return + } else if expect == nil && got != nil { + t.Fatalf("expected=NIL != got=%q", *got) + } else if expect != nil && got == nil { + t.Fatalf("expected=%q != got=NIL", *expect) + } else if !expect.Equal(*got) { + t.Fatalf("expected=%q != got=%q", *expect, *got) } } diff --git a/agent/consul/state/acl.go b/agent/consul/state/acl.go index abeb1f7e22..1533dcbef7 100644 --- a/agent/consul/state/acl.go +++ b/agent/consul/state/acl.go @@ -79,14 +79,14 @@ func (s *TokenExpirationIndex) FromObject(obj interface{}) (bool, []byte, error) if s.LocalFilter != token.Local { return false, nil, nil } - if token.ExpirationTime.IsZero() { + if !token.HasExpirationTime() { return false, nil, nil } if token.ExpirationTime.Unix() < 0 { return false, nil, fmt.Errorf("token expiration time cannot be before the unix epoch: %s", token.ExpirationTime) } - buf := s.encodeTime(token.ExpirationTime) + buf := s.encodeTime(*token.ExpirationTime) return true, buf, nil } @@ -669,10 +669,10 @@ func (s *Store) ACLTokenMinExpirationTime(local bool) (time.Time, error) { token := item.(*structs.ACLToken) - return token.ExpirationTime, nil + return *token.ExpirationTime, nil } -// ACLTokenListExpires lists tokens that are expires as of the provided time. +// ACLTokenListExpires lists tokens that are expired as of the provided time. // The returned set will be no larger than the max value provided. func (s *Store) ACLTokenListExpired(local bool, asOf time.Time, max int) (structs.ACLTokens, <-chan struct{}, error) { tx := s.db.Txn(false) @@ -689,8 +689,7 @@ func (s *Store) ACLTokenListExpired(local bool, asOf time.Time, max int) (struct ) for raw := iter.Next(); raw != nil; raw = iter.Next() { token := raw.(*structs.ACLToken) - - if !token.ExpirationTime.Before(asOf) { + if token.ExpirationTime != nil && !token.ExpirationTime.Before(asOf) { return tokens, nil, nil } diff --git a/agent/consul/state/acl_test.go b/agent/consul/state/acl_test.go index 9ec57674bc..d39c71d099 100644 --- a/agent/consul/state/acl_test.go +++ b/agent/consul/state/acl_test.go @@ -1741,7 +1741,7 @@ func TestTokenPoliciesIndex(t *testing.T) { SecretID: newUUID(), Description: desc, Local: local, - ExpirationTime: expTime, + ExpirationTime: &expTime, CreateTime: baseTime, RaftIndex: structs.RaftIndex{ CreateIndex: 9, diff --git a/agent/structs/acl.go b/agent/structs/acl.go index fee2cd85ea..50d4df0b52 100644 --- a/agent/structs/acl.go +++ b/agent/structs/acl.go @@ -218,7 +218,11 @@ type ACLToken struct { // ExpirationTime represents the point after which a token should be // considered revoked and is eligible for destruction. The zero value // represents NO expiration. - ExpirationTime time.Time `json:",omitempty"` + // + // This is a pointer value so that the zero value is omitted properly + // during json serialization. time.Time does not respect json omitempty + // directives unfortunately. + ExpirationTime *time.Time `json:",omitempty"` // ExpirationTTL is a convenience field for helping set ExpirationTime to a // value of CreateTime+ExpirationTTL. This can only be set during @@ -293,17 +297,21 @@ func (t *ACLToken) ServiceIdentityList() []*ACLServiceIdentity { } func (t *ACLToken) IsExpired(asOf time.Time) bool { - if asOf.IsZero() || t.ExpirationTime.IsZero() { + if asOf.IsZero() || !t.HasExpirationTime() { return false } return t.ExpirationTime.Before(asOf) } +func (t *ACLToken) HasExpirationTime() bool { + return t.ExpirationTime != nil && !t.ExpirationTime.IsZero() +} + func (t *ACLToken) UsesNonLegacyFields() bool { return len(t.Policies) > 0 || len(t.ServiceIdentities) > 0 || t.Type == "" || - !t.ExpirationTime.IsZero() || + t.HasExpirationTime() || t.ExpirationTTL != 0 } @@ -405,8 +413,8 @@ type ACLTokenListStub struct { Policies []ACLTokenPolicyLink `json:",omitempty"` ServiceIdentities []*ACLServiceIdentity `json:",omitempty"` Local bool - ExpirationTime time.Time `json:",omitempty"` - CreateTime time.Time `json:",omitempty"` + ExpirationTime *time.Time `json:",omitempty"` + CreateTime time.Time `json:",omitempty"` Hash []byte CreateIndex uint64 ModifyIndex uint64 diff --git a/api/acl.go b/api/acl.go index 3cf0227621..e920c46d6e 100644 --- a/api/acl.go +++ b/api/acl.go @@ -31,7 +31,7 @@ type ACLToken struct { ServiceIdentities []*ACLServiceIdentity `json:",omitempty"` Local bool ExpirationTTL time.Duration `json:",omitempty"` - ExpirationTime time.Time `json:",omitempty"` + ExpirationTime *time.Time `json:",omitempty"` CreateTime time.Time `json:",omitempty"` Hash []byte `json:",omitempty"` @@ -48,7 +48,7 @@ type ACLTokenListEntry struct { Policies []*ACLTokenPolicyLink `json:",omitempty"` ServiceIdentities []*ACLServiceIdentity `json:",omitempty"` Local bool - ExpirationTime time.Time `json:",omitempty"` + ExpirationTime *time.Time `json:",omitempty"` CreateTime time.Time Hash []byte Legacy bool diff --git a/command/acl/acl_helpers.go b/command/acl/acl_helpers.go index d1e364acd6..843a91487a 100644 --- a/command/acl/acl_helpers.go +++ b/command/acl/acl_helpers.go @@ -15,8 +15,8 @@ func PrintToken(token *api.ACLToken, ui cli.Ui, showMeta bool) { ui.Info(fmt.Sprintf("Description: %s", token.Description)) ui.Info(fmt.Sprintf("Local: %t", token.Local)) ui.Info(fmt.Sprintf("Create Time: %v", token.CreateTime)) - if !token.ExpirationTime.IsZero() { - ui.Info(fmt.Sprintf("Expiration Time: %v", token.ExpirationTime)) + if token.ExpirationTime != nil && !token.ExpirationTime.IsZero() { + ui.Info(fmt.Sprintf("Expiration Time: %v", *token.ExpirationTime)) } if showMeta { ui.Info(fmt.Sprintf("Hash: %x", token.Hash)) @@ -46,8 +46,8 @@ func PrintTokenListEntry(token *api.ACLTokenListEntry, ui cli.Ui, showMeta bool) ui.Info(fmt.Sprintf("Description: %s", token.Description)) ui.Info(fmt.Sprintf("Local: %t", token.Local)) ui.Info(fmt.Sprintf("Create Time: %v", token.CreateTime)) - if !token.ExpirationTime.IsZero() { - ui.Info(fmt.Sprintf("Expiration Time: %v", token.ExpirationTime)) + if token.ExpirationTime != nil && !token.ExpirationTime.IsZero() { + ui.Info(fmt.Sprintf("Expiration Time: %v", *token.ExpirationTime)) } ui.Info(fmt.Sprintf("Legacy: %t", token.Legacy)) if showMeta {