Clean up duplicate apitokens (#529)

Author: Starefossen

controllers/apitoken_controller.go

@@ -3,6 +3,7 @@ package controllers
 import (
 	"context"
 	"fmt"
+	"sort"
 	"time"
 
 	"github.com/go-logr/logr"
@@ -250,11 +251,23 @@ func (r *ApiTokenReconciler) Reconcile(ctx context.Context, req ctrl.Request) (c
 
 	var apiToken *unleashclient.ApiToken
 	log.WithValues("tokens", len(apiTokens.Tokens)).Info("Fetched token from Unleash for ApiToken")
+	// @TODO this is not entirely correct with regards to the environment
 	apiTokenExistingTokens.WithLabelValues(token.Namespace, token.Name, token.Spec.Environment).Set(float64(len(apiTokens.Tokens)))
 
+	// Sort tokens by created_at descending
+	// This is needed since Unleash can return tokens in any order
+	sort.Slice(apiTokens.Tokens, func(i, j int) bool {
+		ti, _ := time.Parse(time.RFC3339, apiTokens.Tokens[i].CreatedAt)
+		tj, _ := time.Parse(time.RFC3339, apiTokens.Tokens[j].CreatedAt)
+		return ti.After(tj)
+	})
+
 	// Delete outdated tokens in Unleash
 	for _, t := range apiTokens.Tokens {
-		if token.IsEqual(t) {
+		// Check if the token is up to date and that we have not already found an up to date token
+		if token.IsEqual(t) && apiToken == nil {
+			// This is the token we are looking for
+			// Continue to the next token
 			apiToken = &t
 			continue
 		}
@@ -264,17 +277,20 @@ func (r *ApiTokenReconciler) Reconcile(ctx context.Context, req ctrl.Request) (c
 		apiTokenDeletedCounter.WithLabelValues(token.Namespace, token.Name).Inc()
 		span.AddEvent(fmt.Sprintf("Deleting old token for %s created at %s in Unleash", t.TokenName, t.CreatedAt))
 
+		// If ApiTokenUpdateEnabled is false, prevent deletion of outdated tokens and subsequent creation of new tokens
+		// This also prevents deletion of duplicate tokens.
 		if !r.ApiTokenUpdateEnabled {
 			log.WithValues("token", t.TokenName, "created_at", t.CreatedAt).Info("Token update is disabled in operator config")
 
-			// Set ApiToken so we don't create a new token if update is disabled
+			// Prevent creation of new tokens further down
 			if apiToken == nil {
 				apiToken = &t
 			}
 
 			continue
 		}
 
+		// At this point we know that the token is outdated our duplicate and it is safe to delete it
 		log.WithValues("token", t.TokenName, "created_at", t.CreatedAt).Info("Deleting token in Unleash for ApiToken")
 		err = apiClient.DeleteApiToken(ctx, t.Secret)
 		if err != nil {

controllers/apitoken_controller_test.go

@@ -81,7 +81,7 @@ var _ = Describe("ApiToken controller", Ordered, func() {
 
 				existingTokens.Tokens = append(existingTokens.Tokens, unleashclient.ApiToken{
 					Secret:      ApiTokenSecret,
-					Username:    tokenRequest.Username,
+					TokenName:   tokenRequest.Username,
 					Type:        strings.ToLower(tokenRequest.Type),
 					Environment: tokenRequest.Environment,
 					Project:     tokenRequest.Projects[0],
@@ -219,7 +219,7 @@ var _ = Describe("ApiToken controller", Ordered, func() {
 
 			Expect(httpmock.GetCallCountInfo()[fmt.Sprintf("POST %s", unleashclient.ApiTokensEndpoint)]).Should(Equal(1))
 			Expect(existingTokens.Tokens).Should(HaveLen(1))
-			Expect(existingTokens.Tokens[0].Username).Should(Equal(apiTokenCreated.ApiTokenName("unleasherator")))
+			Expect(existingTokens.Tokens[0].TokenName).Should(Equal(apiTokenCreated.ApiTokenName("unleasherator")))
 			Expect(existingTokens.Tokens[0].Type).Should(Equal("client"))
 			Expect(existingTokens.Tokens[0].Environment).Should(Equal("development"))
 			Expect(existingTokens.Tokens[0].Projects).Should(Equal([]string{"default"}))
@@ -309,7 +309,7 @@ var _ = Describe("ApiToken controller", Ordered, func() {
 			existingTokens.Tokens = []unleashclient.ApiToken{
 				{
 					Secret:      ApiTokenSecret,
-					Username:    apiTokenCreated.ApiTokenName("unleasherator"),
+					TokenName:   apiTokenCreated.ApiTokenName("unleasherator"),
 					Type:        "CLIENT",
 					Environment: "development",
 					Project:     "default",
@@ -386,7 +386,7 @@ var _ = Describe("ApiToken controller", Ordered, func() {
 	})
 
 	Context("When dealing with duplicate Unleash tokens", func() {
-		It("Should count duplicate tokens", func() {
+		It("Should delete duplicate tokens", func() {
 			ctx := context.Background()
 
 			apiTokenName := "test-apitoken-duplicate"
@@ -401,23 +401,32 @@ var _ = Describe("ApiToken controller", Ordered, func() {
 
 			existingTokens.Tokens = []unleashclient.ApiToken{
 				{
-					Secret:      ApiTokenSecret,
-					Username:    "test-apitoken-duplicate-unleasherator",
+					Secret:      ApiTokenSecret + "-1",
+					TokenName:   "test-apitoken-duplicate-unleasherator",
 					Type:        "CLIENT",
 					Environment: "development",
 					Project:     "default",
 					Projects:    []string{"default"},
-					CreatedAt:   time.Date(2021, 1, 1, 0, 0, 0, 0, time.UTC).Format(time.RFC3339),
+					CreatedAt:   time.Date(2021, 1, 1, 2, 0, 0, 0, time.UTC).Format(time.RFC3339),
 				},
 				{
-					Secret:      ApiTokenSecret,
-					Username:    "test-apitoken-duplicate-unleasherator",
+					Secret:      ApiTokenSecret + "-2",
+					TokenName:   "test-apitoken-duplicate-unleasherator",
 					Type:        "CLIENT",
-					Environment: "development",
+					Environment: "production",
 					Project:     "default",
 					Projects:    []string{"default"},
 					CreatedAt:   time.Date(2021, 1, 1, 1, 0, 0, 0, time.UTC).Format(time.RFC3339),
 				},
+				{
+					Secret:      ApiTokenSecret + "-3",
+					TokenName:   "test-apitoken-duplicate-unleasherator",
+					Type:        "CLIENT",
+					Environment: "development",
+					Project:     "default",
+					Projects:    []string{"default"},
+					CreatedAt:   time.Date(2021, 1, 1, 3, 0, 0, 0, time.UTC).Format(time.RFC3339),
+				},
 			}
 
 			By("By creating a new ApiToken")
@@ -427,7 +436,20 @@ var _ = Describe("ApiToken controller", Ordered, func() {
 
 			fmt.Printf("existingTokens=%+v\n", existingTokens)
 
-			Expect(promGaugeVecVal(apiTokenExistingTokens, ApiTokenNamespace, apiTokenName, "development")).Should(Equal(2.0))
+			Expect(promGaugeVecVal(apiTokenExistingTokens, ApiTokenNamespace, apiTokenName, "development")).Should(Equal(3.0))
+			Expect(promCounterVecVal(apiTokenDeletedCounter, ApiTokenNamespace, apiTokenName)).Should(Equal(2.0))
+
+			Expect(httpmock.GetCallCountInfo()[fmt.Sprintf("DELETE %s", fmt.Sprintf("=~%s/.*", unleashclient.ApiTokensEndpoint))]).Should(Equal(2))
+
+			// Verify that the older tokens were deleted
+			for _, token := range existingTokens.Tokens {
+				Expect(token.Secret).ShouldNot(Equal(ApiTokenSecret + "-1"))
+				Expect(token.Secret).ShouldNot(Equal(ApiTokenSecret + "-2"))
+			}
+
+			// Verify that we kept the newest duplicate token
+			Expect(existingTokens.Tokens).Should(HaveLen(1))
+			Expect(existingTokens.Tokens[0].Secret).Should(Equal(ApiTokenSecret + "-3"))
 		})
 	})
 })