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

@@ -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.HasExpirationTime() {
-			if token.CreateTime.After(token.ExpirationTime) {
+			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
+}

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) {
+	if expect == nil && got == nil {
-		t.Fatalf("expected=%q != got=%q", expect, got)
+		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)
 	}
 }
 

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 != nil && !token.ExpirationTime.Before(asOf) {
-		if !token.ExpirationTime.Before(asOf) {
 			return tokens, nil, nil
 		}
 

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,

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,7 +413,7 @@ type ACLTokenListStub struct {
 	Policies          []ACLTokenPolicyLink  `json:",omitempty"`
 	ServiceIdentities []*ACLServiceIdentity `json:",omitempty"`
 	Local             bool
-	ExpirationTime    time.Time `json:",omitempty"`
+	ExpirationTime    *time.Time `json:",omitempty"`
 	CreateTime        time.Time  `json:",omitempty"`
 	Hash              []byte
 	CreateIndex       uint64

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

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() {
+	if token.ExpirationTime != nil && !token.ExpirationTime.IsZero() {
-		ui.Info(fmt.Sprintf("Expiration Time:  %v", token.ExpirationTime))
+		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() {
+	if token.ExpirationTime != nil && !token.ExpirationTime.IsZero() {
-		ui.Info(fmt.Sprintf("Expiration Time:  %v", token.ExpirationTime))
+		ui.Info(fmt.Sprintf("Expiration Time:  %v", *token.ExpirationTime))
 	}
 	ui.Info(fmt.Sprintf("Legacy:           %t", token.Legacy))
 	if showMeta {