Skip to content

Commit

Permalink
Fix token update/delete edge case (#399)
Browse files Browse the repository at this point in the history
  • Loading branch information
@Starefossen
Starefossen authored Jun 4, 2024
1 parent d6d8134 commit 3f31a0b
Show file tree
Hide file tree
Showing 7 changed files with 369 additions and 112 deletions.
16 changes: 15 additions & 1 deletion api/v1/apitoken_types.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -124,12 +124,26 @@ func (t *ApiToken) ApiTokenRequest(suffix string) unleashclient.ApiTokenRequest
}
}

func (t *ApiToken) ApiTokenIsEqual(token *unleashclient.ApiToken) bool {
// IsEqual checks if the token equals another token by comparing the type,
// environment, and projects attributes of the token.
func (t *ApiToken) IsEqual(token unleashclient.ApiToken) bool {
return strings.EqualFold(t.Spec.Type, token.Type) &&
t.Spec.Environment == token.Environment &&
utils.StringSliceEquals(t.Spec.Projects, token.Projects)
}

// ExistsInList checks if the token equals any token in the list by comparing
// the type, environment, and projects attributes of the token.
// It returns the matching token and a boolean indicating if a match was found.
func (t *ApiToken) ExistsInList(list []unleashclient.ApiToken) (*unleashclient.ApiToken, bool) {
for _, token := range list {
if t.IsEqual(token) {
return &token, true
}
}
return nil, false
}

func init() {
SchemeBuilder.Register(&ApiToken{}, &ApiTokenList{})
}
225 changes: 188 additions & 37 deletions api/v1/apitoken_types_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -63,47 +63,198 @@ func TestApiToken_NamespacedName(t *testing.T) {
assert.Equal(t, expectedNamespacedName, actualNamespacedName)
}
func TestApiToken_IsEqual(t *testing.T) {
apiToken := &ApiToken{
Spec: ApiTokenSpec{
Type: "CLIENT",
Environment: "development",
Projects: []string{"project1", "project2"},
testCases := []struct {
name string
apiToken *ApiToken
unleashToken unleashclient.ApiToken
expectedEqual bool
}{
{
name: "Token types are case different",
apiToken: &ApiToken{
Spec: ApiTokenSpec{
Type: "CLIENT",
Environment: "development",
Projects: []string{"project1", "project2"},
},
},
unleashToken: unleashclient.ApiToken{
Type: "client",
Environment: "development",
Projects: []string{"project1", "project2"},
},
expectedEqual: true,
},
{
name: "Tokens are equal",
apiToken: &ApiToken{
Spec: ApiTokenSpec{
Type: "CLIENT",
Environment: "development",
Projects: []string{"project1", "project2"},
},
},
unleashToken: unleashclient.ApiToken{
Type: "client",
Environment: "development",
Projects: []string{"project1", "project2"},
},
expectedEqual: true,
},
{
name: "Tokens have different types",
apiToken: &ApiToken{
Spec: ApiTokenSpec{
Type: "CLIENT",
Environment: "development",
Projects: []string{"project1", "project2"},
},
},
unleashToken: unleashclient.ApiToken{
Type: "FRONTEND",
Environment: "development",
Projects: []string{"project1", "project2"},
},
expectedEqual: false,
},
{
name: "Tokens have different environments",
apiToken: &ApiToken{
Spec: ApiTokenSpec{
Type: "CLIENT",
Environment: "development",
Projects: []string{"project1", "project2"},
},
},
unleashToken: unleashclient.ApiToken{
Type: "CLIENT",
Environment: "production",
Projects: []string{"project1", "project2"},
},
expectedEqual: false,
},
{
name: "Tokens have same projects but in different order",
apiToken: &ApiToken{
Spec: ApiTokenSpec{
Type: "CLIENT",
Environment: "development",
Projects: []string{"project1", "project2"},
},
},
unleashToken: unleashclient.ApiToken{
Type: "CLIENT",
Environment: "development",
Projects: []string{"project2", "project1"},
},
expectedEqual: true,
},
{
name: "Tokens have different projects",
apiToken: &ApiToken{
Spec: ApiTokenSpec{
Type: "CLIENT",
Environment: "development",
Projects: []string{"project1", "project2"},
},
},
unleashToken: unleashclient.ApiToken{
Type: "CLIENT",
Environment: "development",
Projects: []string{"project1", "project3"},
},
expectedEqual: false,
},
{
name: "Tokens are wildcards",
apiToken: &ApiToken{
Spec: ApiTokenSpec{
Type: "CLIENT",
Environment: "development",
Projects: []string{"*"},
},
},
unleashToken: unleashclient.ApiToken{
Type: "CLIENT",
Environment: "development",
Projects: []string{"*"},
},
expectedEqual: true,
},
}

unleashToken := &unleashclient.ApiToken{
Type: "client",
Environment: "development",
Projects: []string{"project1", "project2"},
for _, tc := range testCases {
t.Run(tc.name, func(t *testing.T) {
actualEqual := tc.apiToken.IsEqual(tc.unleashToken)
assert.Equal(t, tc.expectedEqual, actualEqual)
})
}
}

// Test when the token types are case different
apiToken.Spec.Type = "ClIeNt"
assert.True(t, apiToken.ApiTokenIsEqual(unleashToken))

// Test when the tokens are equal
assert.True(t, apiToken.ApiTokenIsEqual(unleashToken))

// Test when the tokens have different types
unleashToken.Type = "FRONTEND"
assert.False(t, apiToken.ApiTokenIsEqual(unleashToken))

// Test when the tokens have different environments
unleashToken.Type = "CLIENT"
unleashToken.Environment = "production"
assert.False(t, apiToken.ApiTokenIsEqual(unleashToken))

// Test when the tokens have same projects but in different order
unleashToken.Environment = "development"
unleashToken.Projects = []string{"project2", "project1"}
assert.True(t, apiToken.ApiTokenIsEqual(unleashToken))

// Test when the tokens have different projects
unleashToken.Projects = []string{"project1", "project3"}
assert.False(t, apiToken.ApiTokenIsEqual(unleashToken))
func TestApiToken_ExistsInList(t *testing.T) {
testCases := []struct {
name string
apiToken *ApiToken
tokenList []unleashclient.ApiToken
exists bool
}{
{
name: "Token exists in the list",
apiToken: &ApiToken{
Spec: ApiTokenSpec{
Type: "CLIENT",
Environment: "development",
Projects: []string{"project1", "project2"},
},
},
tokenList: []unleashclient.ApiToken{
{
Type: "client",
Environment: "development",
Projects: []string{"project1", "project2"},
},
{
Type: "client",
Environment: "production",
Projects: []string{"project3", "project4"},
},
},
exists: true,
},
{
name: "Token does not exist in the list",
apiToken: &ApiToken{
Spec: ApiTokenSpec{
Type: "CLIENT",
Environment: "staging",
Projects: []string{"project1", "project2"},
},
},
tokenList: []unleashclient.ApiToken{
{
Type: "client",
Environment: "development",
Projects: []string{"project1", "project2"},
},
{
Type: "client",
Environment: "production",
Projects: []string{"project3", "project4"},
},
},
exists: false,
},
}

// Test when the tokens are wildcards
apiToken.Spec.Projects = []string{"*"}
unleashToken.Projects = []string{"*"}
assert.True(t, apiToken.ApiTokenIsEqual(unleashToken))
for _, tc := range testCases {
t.Run(tc.name, func(t *testing.T) {
token, exists := tc.apiToken.ExistsInList(tc.tokenList)
assert.Equal(t, tc.exists, exists)
if exists {
assert.NotNil(t, token)
} else {
assert.Nil(t, token)
}
})
}
}
82 changes: 46 additions & 36 deletions controllers/apitoken_controller.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -175,6 +175,7 @@ func (r *ApiTokenReconciler) Reconcile(ctx context.Context, req ctrl.Request) (c
}

// Check if marked for deletion
// @TODO won't work if Unleash is unavailable
if token.GetDeletionTimestamp() != nil {
log.Info("ApiToken marked for deletion")
if controllerutil.ContainsFinalizer(token, tokenFinalizer) {
Expand Down Expand Up @@ -212,9 +213,9 @@ func (r *ApiTokenReconciler) Reconcile(ctx context.Context, req ctrl.Request) (c
return ctrl.Result{}, nil
}

// Check if token exists in Unleash
// Get token(s) from Unleash
log.Info("Fetching token from Unleash for ApiToken")
apiToken, err := apiClient.GetAPIToken(ctx, token.ApiTokenName(r.ApiTokenNameSuffix))
apiTokens, err := apiClient.GetAPITokensByName(ctx, token.ApiTokenName(r.ApiTokenNameSuffix))
if err != nil {
log.Error(err, "Failed to get token from Unleash")
if err := r.updateStatusFailed(ctx, token, err, "TokenCheckFailed", "Failed to check if token exists in Unleash"); err != nil {
Expand All @@ -223,6 +224,32 @@ func (r *ApiTokenReconciler) Reconcile(ctx context.Context, req ctrl.Request) (c
return ctrl.Result{}, err
}

var apiToken *unleashclient.ApiToken

// Delete outdated tokens in Unleash
log.Info("Deleting outdated tokens in Unleash for ApiToken")
for _, t := range apiTokens.Tokens {
if !token.IsEqual(t) {
if r.ApiTokenUpdateEnabled {
span.AddEvent(fmt.Sprintf("Deleting old token for %s created at %s in Unleash", t.TokenName, t.CreatedAt))
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 {
if err := r.updateStatusFailed(ctx, token, err, "TokenUpdateFailed", "Failed to delete old token in Unleash"); err != nil {
return ctrl.Result{}, err
}
return ctrl.Result{}, err
}
} else {
log.WithValues("token", t.TokenName, "created_at", t.CreatedAt).Info("Token update is disabled in operator config")
}

continue
}

apiToken = &t
}

// Create token if it does not exist in Unleash
if apiToken == nil {
span.AddEvent("Creating token in Unleash")
Expand All @@ -236,37 +263,6 @@ func (r *ApiTokenReconciler) Reconcile(ctx context.Context, req ctrl.Request) (c
}
}

// Update token if it differs from the desired state
// Tokens in Unleash are immutable, so we need to delete the old token and create a new one
if apiToken != nil && !token.ApiTokenIsEqual(apiToken) {
span.AddEvent("Updating token in Unleash")

if r.ApiTokenUpdateEnabled {
span.AddEvent("Deleting old token in Unleash")
log.Info("Deleting old token in Unleash for ApiToken")
err = apiClient.DeleteApiToken(ctx, token.ApiTokenName(r.ApiTokenNameSuffix))
if err != nil {
if err := r.updateStatusFailed(ctx, token, err, "TokenUpdateFailed", "Failed to delete old token in Unleash"); err != nil {
return ctrl.Result{}, err
}
return ctrl.Result{}, err
}

span.AddEvent("Creating new token in Unleash")
log.Info("Creating new token in Unleash for ApiToken")
apiToken, err = apiClient.CreateAPIToken(ctx, token.ApiTokenRequest(r.ApiTokenNameSuffix))
if err != nil {
if err := r.updateStatusFailed(ctx, token, err, "TokenUpdateFailed", "Failed to create new token in Unleash"); err != nil {
return ctrl.Result{}, err
}
return ctrl.Result{}, err
}
} else {
log.Info("Token update is disabled in operator config")
}
}

span.AddEvent("Fetching token secret")
secret := resources.ApiTokenSecret(unleash, token, apiToken)
if err := controllerutil.SetControllerReference(token, secret, r.Scheme); err != nil {
log.Error(err, "Failed to set controller reference on token secret")
Expand Down Expand Up @@ -391,11 +387,25 @@ func (r *ApiTokenReconciler) updateStatusFailed(ctx context.Context, apiToken *u
// doFinalizerOperationsForToken will delete the ApiToken from Unleash
func (r *ApiTokenReconciler) doFinalizerOperationsForToken(ctx context.Context, token *unleashv1.ApiToken, unleashClient *unleashclient.Client, log logr.Logger) {
tokenName := token.ApiTokenName(r.ApiTokenNameSuffix)
err := unleashClient.DeleteApiToken(ctx, tokenName)
tokens, err := unleashClient.GetAPITokensByName(ctx, tokenName)
if err != nil {
log.Error(err, fmt.Sprintf("Failed to delete ApiToken %s from Unleash", tokenName))
log.Error(err, fmt.Sprintf("Failed to get ApiToken %s from Unleash", tokenName))
return
}

if tokens == nil || len(tokens.Tokens) == 0 {
log.Info(fmt.Sprintf("ApiToken %s not found in Unleash", tokenName))
return
}

for _, t := range tokens.Tokens {
log.Info(fmt.Sprintf("Deleting ApiToken %s from Unleash", t.TokenName))
err = unleashClient.DeleteApiToken(ctx, t.Secret)
if err != nil {
log.Error(err, fmt.Sprintf("Failed to delete ApiToken %s from Unleash", t.TokenName))
}
log.Info(fmt.Sprintf("Successfully deleted ApiToken %s from Unleash", t.TokenName))
}
log.Info(fmt.Sprintf("Successfully deleted ApiToken %s from Unleash", tokenName))
}

// SetupWithManager sets up the controller with the Manager.
Expand Down
Loading

0 comments on commit 3f31a0b

Please sign in to comment.