diff --git a/pkg/cluster/clients/zz_gitlab.go b/pkg/cluster/clients/zz_gitlab.go index 5145f16f..4a36db22 100644 --- a/pkg/cluster/clients/zz_gitlab.go +++ b/pkg/cluster/clients/zz_gitlab.go @@ -21,6 +21,7 @@ package clients import ( "crypto/sha256" "encoding/hex" + "net/http" "time" "github.com/google/go-cmp/cmp" @@ -250,6 +251,16 @@ func IsResponseNotFound(res *gitlab.Response) bool { return false } +// IsResponseUnauthorized reports whether the GitLab response indicates the +// credential itself was rejected: 401 Unauthorized or 403 Forbidden. GitLab +// returns 401 when a token is revoked or expired, and escalates repeated +// unauthorized requests from the same IP to 403. Both mean the token can no +// longer authenticate (as opposed to a missing resource or transient failure). +func IsResponseUnauthorized(res *gitlab.Response) bool { + return res != nil && res.Response != nil && + (res.StatusCode == http.StatusUnauthorized || res.StatusCode == http.StatusForbidden) +} + // TokenHash returns a stable hex-encoded SHA-256 digest of the supplied token // value, or an empty string when no token is set. GitLab never returns webhook // secret tokens, so the digest is stored in status.atProvider to detect when a diff --git a/pkg/cluster/controller/groups/serviceaccountaccesstokens/zz_controller.go b/pkg/cluster/controller/groups/serviceaccountaccesstokens/zz_controller.go index e3b8b29e..d8b06b80 100644 --- a/pkg/cluster/controller/groups/serviceaccountaccesstokens/zz_controller.go +++ b/pkg/cluster/controller/groups/serviceaccountaccesstokens/zz_controller.go @@ -205,8 +205,15 @@ func (e *external) Observe(ctx context.Context, mg resource.Managed) (managed.Ex // token is whatever the ProviderConfig authenticates with, so the external name // is auto-adopted from the response. func (e *external) observeSelf(ctx context.Context, cr *v1alpha1.ServiceAccountAccessToken) (managed.ExternalObservation, error) { - at, _, err := e.client.GetServiceAccountSelf(gitlab.WithContext(ctx)) + at, res, err := e.client.GetServiceAccountSelf(gitlab.WithContext(ctx)) if err != nil { + // During deletion the self-revoke has already run, so the self + // endpoints reject the (now revoked) credential with 401/403. That is + // the expected terminal state: report the resource gone so the + // finalizer is removed instead of wedging the delete in a retry loop. + if meta.WasDeleted(cr) && clients.IsResponseUnauthorized(res) { + return managed.ExternalObservation{ResourceExists: false}, nil + } // A dead self-credential is unrecoverable by the provider: surface a // clear error rather than masquerading as "does not exist" (which would // push the resource into a doomed self-rotate loop). @@ -420,8 +427,18 @@ func (e *external) Delete(ctx context.Context, mg resource.Managed) (managed.Ext // Revoke the token the ProviderConfig authenticates with. This breaks // every resource using that ProviderConfig; use deletionPolicy: Orphan // to keep the token. - _, err := e.client.RevokeServiceAccountSelf(gitlab.WithContext(ctx)) - return managed.ExternalDelete{}, errors.Wrap(err, errDeleteFailed) + res, err := e.client.RevokeServiceAccountSelf(gitlab.WithContext(ctx)) + if err != nil { + // A second reconcile hitting an already-revoked token can no longer + // authenticate and gets 401/403. Treat that as an idempotent + // success so the delete does not wedge on an already-completed + // revoke. + if clients.IsResponseUnauthorized(res) { + return managed.ExternalDelete{}, nil + } + return managed.ExternalDelete{}, errors.Wrap(err, errDeleteFailed) + } + return managed.ExternalDelete{}, nil } accessTokenID, err := strconv.Atoi(meta.GetExternalName(cr)) diff --git a/pkg/cluster/controller/groups/serviceaccountaccesstokens/zz_controller_test.go b/pkg/cluster/controller/groups/serviceaccountaccesstokens/zz_controller_test.go index 4a812f61..74150d3c 100644 --- a/pkg/cluster/controller/groups/serviceaccountaccesstokens/zz_controller_test.go +++ b/pkg/cluster/controller/groups/serviceaccountaccesstokens/zz_controller_test.go @@ -87,6 +87,13 @@ func withExternalName(n string) tokenModifier { return func(r *v1alpha1.ServiceAccountAccessToken) { meta.SetExternalName(r, n) } } +func withDeletionTimestamp() tokenModifier { + return func(r *v1alpha1.ServiceAccountAccessToken) { + now := v1.Now() + r.SetDeletionTimestamp(&now) + } +} + func saToken(m ...tokenModifier) *v1alpha1.ServiceAccountAccessToken { cr := &v1alpha1.ServiceAccountAccessToken{} for _, f := range m { @@ -291,6 +298,48 @@ func TestObserveSelf(t *testing.T) { err: errors.Wrap(errBoom, errSelfInformFailed), }, }, + // Once the self-token is revoked during deletion, self-inform can no + // longer authenticate (401). While the resource is being deleted this is + // the expected terminal state, so report it gone to let the finalizer be + // removed instead of wedging the delete forever. + "DeletedAfterRevoke401ReportsGone": { + client: &fake.MockClient{ + MockGetServiceAccountSelf: func(_ ...gitlab.RequestOptionFunc) (*gitlab.PersonalAccessToken, *gitlab.Response, error) { + return nil, &gitlab.Response{Response: &http.Response{StatusCode: http.StatusUnauthorized}}, errBoom + }, + }, + cr: saToken(withSpec(v1alpha1.ServiceAccountAccessTokenParameters{}), withDeletionTimestamp()), + want: want{ + result: managed.ExternalObservation{ResourceExists: false}, + }, + }, + // GitLab escalates repeated unauthorized requests from the same IP to + // 403; a wedged delete poll would produce these, so treat 403 during + // deletion as gone too. + "DeletedAfterRevoke403ReportsGone": { + client: &fake.MockClient{ + MockGetServiceAccountSelf: func(_ ...gitlab.RequestOptionFunc) (*gitlab.PersonalAccessToken, *gitlab.Response, error) { + return nil, &gitlab.Response{Response: &http.Response{StatusCode: http.StatusForbidden}}, errBoom + }, + }, + cr: saToken(withSpec(v1alpha1.ServiceAccountAccessTokenParameters{}), withDeletionTimestamp()), + want: want{ + result: managed.ExternalObservation{ResourceExists: false}, + }, + }, + // A transient (non-auth) failure during deletion must still surface as an + // error rather than dropping the finalizer prematurely. + "DeletedTransientErrorStillFails": { + client: &fake.MockClient{ + MockGetServiceAccountSelf: func(_ ...gitlab.RequestOptionFunc) (*gitlab.PersonalAccessToken, *gitlab.Response, error) { + return nil, &gitlab.Response{Response: &http.Response{StatusCode: http.StatusInternalServerError}}, errBoom + }, + }, + cr: saToken(withSpec(v1alpha1.ServiceAccountAccessTokenParameters{}), withDeletionTimestamp()), + want: want{ + err: errors.Wrap(errBoom, errSelfInformFailed), + }, + }, "ServiceAccountMatch": { client: &fake.MockClient{ MockGetServiceAccountSelf: func(_ ...gitlab.RequestOptionFunc) (*gitlab.PersonalAccessToken, *gitlab.Response, error) { @@ -618,6 +667,33 @@ func TestDelete(t *testing.T) { }, want: want{cr: saToken(withExternalName(sAccessTokenID)), err: errors.Wrap(errBoom, errDeleteFailed)}, }, + // A second reconcile hitting an already-revoked self-token gets 401. + // Revoke is idempotent, so this must succeed rather than wedge the delete. + "SelfRevoke401Idempotent": { + self: true, + args: args{ + client: &fake.MockClient{ + MockRevokeServiceAccountSelf: func(_ ...gitlab.RequestOptionFunc) (*gitlab.Response, error) { + return &gitlab.Response{Response: &http.Response{StatusCode: http.StatusUnauthorized}}, errBoom + }, + }, + cr: saToken(withExternalName(sAccessTokenID)), + }, + want: want{cr: saToken(withExternalName(sAccessTokenID))}, + }, + // Rate-limited self-revoke (403) is likewise treated as an idempotent success. + "SelfRevoke403Idempotent": { + self: true, + args: args{ + client: &fake.MockClient{ + MockRevokeServiceAccountSelf: func(_ ...gitlab.RequestOptionFunc) (*gitlab.Response, error) { + return &gitlab.Response{Response: &http.Response{StatusCode: http.StatusForbidden}}, errBoom + }, + }, + cr: saToken(withExternalName(sAccessTokenID)), + }, + want: want{cr: saToken(withExternalName(sAccessTokenID))}, + }, } for name, tc := range cases { t.Run(name, func(t *testing.T) { diff --git a/pkg/cluster/controller/instance/serviceaccountaccesstokens/zz_controller.go b/pkg/cluster/controller/instance/serviceaccountaccesstokens/zz_controller.go index af069425..f2ab6986 100644 --- a/pkg/cluster/controller/instance/serviceaccountaccesstokens/zz_controller.go +++ b/pkg/cluster/controller/instance/serviceaccountaccesstokens/zz_controller.go @@ -206,8 +206,15 @@ func (e *external) Observe(ctx context.Context, mg resource.Managed) (managed.Ex // token is whatever the ProviderConfig authenticates with, so the external name // is auto-adopted from the response. func (e *external) observeSelf(ctx context.Context, cr *v1alpha1.ServiceAccountAccessToken) (managed.ExternalObservation, error) { - at, _, err := e.client.GetServiceAccountSelf(gitlab.WithContext(ctx)) + at, res, err := e.client.GetServiceAccountSelf(gitlab.WithContext(ctx)) if err != nil { + // During deletion the self-revoke has already run, so the self + // endpoints reject the (now revoked) credential with 401/403. That is + // the expected terminal state: report the resource gone so the + // finalizer is removed instead of wedging the delete in a retry loop. + if meta.WasDeleted(cr) && clients.IsResponseUnauthorized(res) { + return managed.ExternalObservation{ResourceExists: false}, nil + } // A dead self-credential is unrecoverable by the provider: surface a // clear error rather than masquerading as "does not exist" (which would // push the resource into a doomed self-rotate loop). @@ -456,8 +463,18 @@ func (e *external) Delete(ctx context.Context, mg resource.Managed) (managed.Ext // Revoke the token the ProviderConfig authenticates with. This breaks // every resource using that ProviderConfig; use deletionPolicy: Orphan // to keep the token. - _, err := e.client.RevokeServiceAccountSelf(gitlab.WithContext(ctx)) - return managed.ExternalDelete{}, errors.Wrap(err, errDeleteFailed) + res, err := e.client.RevokeServiceAccountSelf(gitlab.WithContext(ctx)) + if err != nil { + // A second reconcile hitting an already-revoked token can no longer + // authenticate and gets 401/403. Treat that as an idempotent + // success so the delete does not wedge on an already-completed + // revoke. + if clients.IsResponseUnauthorized(res) { + return managed.ExternalDelete{}, nil + } + return managed.ExternalDelete{}, errors.Wrap(err, errDeleteFailed) + } + return managed.ExternalDelete{}, nil } accessTokenID, err := strconv.ParseInt(meta.GetExternalName(cr), 10, 64) diff --git a/pkg/cluster/controller/instance/serviceaccountaccesstokens/zz_controller_test.go b/pkg/cluster/controller/instance/serviceaccountaccesstokens/zz_controller_test.go index 3345de0a..1ad00f15 100644 --- a/pkg/cluster/controller/instance/serviceaccountaccesstokens/zz_controller_test.go +++ b/pkg/cluster/controller/instance/serviceaccountaccesstokens/zz_controller_test.go @@ -148,6 +148,13 @@ func withExternalName(n string) tokenModifier { return func(r *v1alpha1.ServiceAccountAccessToken) { meta.SetExternalName(r, n) } } +func withDeletionTimestamp() tokenModifier { + return func(r *v1alpha1.ServiceAccountAccessToken) { + now := v1.Now() + r.SetDeletionTimestamp(&now) + } +} + func saToken(m ...tokenModifier) *v1alpha1.ServiceAccountAccessToken { cr := &v1alpha1.ServiceAccountAccessToken{} for _, f := range m { @@ -335,6 +342,43 @@ func TestObserveSelf(t *testing.T) { err: errors.Wrap(errBoom, errSelfInformFailed), }, }, + // During deletion the self-token is already revoked, so self-inform gets + // 401. Report gone so the finalizer is removed instead of wedging delete. + "DeletedAfterRevoke401ReportsGone": { + client: &mockClient{ + MockGetServiceAccountSelf: func(_ ...gitlab.RequestOptionFunc) (*gitlab.PersonalAccessToken, *gitlab.Response, error) { + return nil, &gitlab.Response{Response: &http.Response{StatusCode: http.StatusUnauthorized}}, errBoom + }, + }, + cr: saToken(withSpec(v1alpha1.ServiceAccountAccessTokenParameters{}), withDeletionTimestamp()), + want: want{ + result: managed.ExternalObservation{ResourceExists: false}, + }, + }, + // Repeated unauthorized polls escalate to 403; treat as gone during deletion too. + "DeletedAfterRevoke403ReportsGone": { + client: &mockClient{ + MockGetServiceAccountSelf: func(_ ...gitlab.RequestOptionFunc) (*gitlab.PersonalAccessToken, *gitlab.Response, error) { + return nil, &gitlab.Response{Response: &http.Response{StatusCode: http.StatusForbidden}}, errBoom + }, + }, + cr: saToken(withSpec(v1alpha1.ServiceAccountAccessTokenParameters{}), withDeletionTimestamp()), + want: want{ + result: managed.ExternalObservation{ResourceExists: false}, + }, + }, + // A transient failure during deletion must still surface as an error. + "DeletedTransientErrorStillFails": { + client: &mockClient{ + MockGetServiceAccountSelf: func(_ ...gitlab.RequestOptionFunc) (*gitlab.PersonalAccessToken, *gitlab.Response, error) { + return nil, &gitlab.Response{Response: &http.Response{StatusCode: http.StatusInternalServerError}}, errBoom + }, + }, + cr: saToken(withSpec(v1alpha1.ServiceAccountAccessTokenParameters{}), withDeletionTimestamp()), + want: want{ + err: errors.Wrap(errBoom, errSelfInformFailed), + }, + }, "ServiceAccountIDMatchUpToDate": { // serviceAccountId set and matches the self token's owner -> adopt it. client: &mockClient{ @@ -719,6 +763,33 @@ func TestDelete(t *testing.T) { }, want: want{cr: saToken(withExternalName(sAccessTokenID)), err: errors.Wrap(errBoom, errDeleteFailed)}, }, + // A second reconcile hitting an already-revoked self-token gets 401; + // revoke is idempotent, so this must succeed. + "SelfRevoke401Idempotent": { + self: true, + args: args{ + client: &mockClient{ + MockRevokeServiceAccountSelf: func(_ ...gitlab.RequestOptionFunc) (*gitlab.Response, error) { + return &gitlab.Response{Response: &http.Response{StatusCode: http.StatusUnauthorized}}, errBoom + }, + }, + cr: saToken(withExternalName(sAccessTokenID)), + }, + want: want{cr: saToken(withExternalName(sAccessTokenID))}, + }, + // Rate-limited self-revoke (403) is likewise idempotent success. + "SelfRevoke403Idempotent": { + self: true, + args: args{ + client: &mockClient{ + MockRevokeServiceAccountSelf: func(_ ...gitlab.RequestOptionFunc) (*gitlab.Response, error) { + return &gitlab.Response{Response: &http.Response{StatusCode: http.StatusForbidden}}, errBoom + }, + }, + cr: saToken(withExternalName(sAccessTokenID)), + }, + want: want{cr: saToken(withExternalName(sAccessTokenID))}, + }, } for name, tc := range cases { t.Run(name, func(t *testing.T) { diff --git a/pkg/cluster/controller/projects/serviceaccountaccesstokens/zz_controller.go b/pkg/cluster/controller/projects/serviceaccountaccesstokens/zz_controller.go index cbaa2aef..605c83b0 100644 --- a/pkg/cluster/controller/projects/serviceaccountaccesstokens/zz_controller.go +++ b/pkg/cluster/controller/projects/serviceaccountaccesstokens/zz_controller.go @@ -201,8 +201,15 @@ func (e *external) Observe(ctx context.Context, mg resource.Managed) (managed.Ex // token is whatever the ProviderConfig authenticates with, so the external name // is auto-adopted from the response. func (e *external) observeSelf(ctx context.Context, cr *v1alpha1.ServiceAccountAccessToken) (managed.ExternalObservation, error) { - at, _, err := e.client.GetServiceAccountSelf(gitlab.WithContext(ctx)) + at, res, err := e.client.GetServiceAccountSelf(gitlab.WithContext(ctx)) if err != nil { + // During deletion the self-revoke has already run, so the self + // endpoints reject the (now revoked) credential with 401/403. That is + // the expected terminal state: report the resource gone so the + // finalizer is removed instead of wedging the delete in a retry loop. + if meta.WasDeleted(cr) && clients.IsResponseUnauthorized(res) { + return managed.ExternalObservation{ResourceExists: false}, nil + } // A dead self-credential is unrecoverable by the provider: surface a // clear error rather than masquerading as "does not exist". return managed.ExternalObservation{}, errors.Wrap(err, errSelfInformFailed) @@ -416,8 +423,18 @@ func (e *external) Delete(ctx context.Context, mg resource.Managed) (managed.Ext // Revoke the token the ProviderConfig authenticates with. This breaks // every resource using that ProviderConfig; use deletionPolicy: Orphan // to keep the token. - _, err := e.client.RevokeServiceAccountSelf(gitlab.WithContext(ctx)) - return managed.ExternalDelete{}, errors.Wrap(err, errDeleteFailed) + res, err := e.client.RevokeServiceAccountSelf(gitlab.WithContext(ctx)) + if err != nil { + // A second reconcile hitting an already-revoked token can no longer + // authenticate and gets 401/403. Treat that as an idempotent + // success so the delete does not wedge on an already-completed + // revoke. + if clients.IsResponseUnauthorized(res) { + return managed.ExternalDelete{}, nil + } + return managed.ExternalDelete{}, errors.Wrap(err, errDeleteFailed) + } + return managed.ExternalDelete{}, nil } accessTokenID, err := strconv.Atoi(meta.GetExternalName(cr)) diff --git a/pkg/cluster/controller/projects/serviceaccountaccesstokens/zz_controller_test.go b/pkg/cluster/controller/projects/serviceaccountaccesstokens/zz_controller_test.go index 1bf4350f..3d31b49c 100644 --- a/pkg/cluster/controller/projects/serviceaccountaccesstokens/zz_controller_test.go +++ b/pkg/cluster/controller/projects/serviceaccountaccesstokens/zz_controller_test.go @@ -107,6 +107,12 @@ func withSpec(fp v1alpha1.ServiceAccountAccessTokenParameters) tokenModifier { func withExternalName(n string) tokenModifier { return func(r *v1alpha1.ServiceAccountAccessToken) { meta.SetExternalName(r, n) } } +func withDeletionTimestamp() tokenModifier { + return func(r *v1alpha1.ServiceAccountAccessToken) { + now := v1.Now() + r.SetDeletionTimestamp(&now) + } +} func saToken(m ...tokenModifier) *v1alpha1.ServiceAccountAccessToken { cr := &v1alpha1.ServiceAccountAccessToken{} for _, f := range m { @@ -215,6 +221,31 @@ func TestObserveSelf(t *testing.T) { cr: saToken(withSpec(v1alpha1.ServiceAccountAccessTokenParameters{})), err: errors.Wrap(errBoom, errSelfInformFailed), }, + // During deletion the self-token is already revoked, so self-inform gets + // 401. Report gone so the finalizer is removed instead of wedging delete. + "DeletedAfterRevoke401ReportsGone": { + client: &mockClient{MockGetSelf: func(_ ...gitlab.RequestOptionFunc) (*gitlab.PersonalAccessToken, *gitlab.Response, error) { + return nil, &gitlab.Response{Response: &http.Response{StatusCode: http.StatusUnauthorized}}, errBoom + }}, + cr: saToken(withSpec(v1alpha1.ServiceAccountAccessTokenParameters{}), withDeletionTimestamp()), + result: managed.ExternalObservation{ResourceExists: false}, + }, + // Repeated unauthorized polls escalate to 403; treat as gone during deletion too. + "DeletedAfterRevoke403ReportsGone": { + client: &mockClient{MockGetSelf: func(_ ...gitlab.RequestOptionFunc) (*gitlab.PersonalAccessToken, *gitlab.Response, error) { + return nil, &gitlab.Response{Response: &http.Response{StatusCode: http.StatusForbidden}}, errBoom + }}, + cr: saToken(withSpec(v1alpha1.ServiceAccountAccessTokenParameters{}), withDeletionTimestamp()), + result: managed.ExternalObservation{ResourceExists: false}, + }, + // A transient failure during deletion must still surface as an error. + "DeletedTransientErrorStillFails": { + client: &mockClient{MockGetSelf: func(_ ...gitlab.RequestOptionFunc) (*gitlab.PersonalAccessToken, *gitlab.Response, error) { + return nil, &gitlab.Response{Response: &http.Response{StatusCode: http.StatusInternalServerError}}, errBoom + }}, + cr: saToken(withSpec(v1alpha1.ServiceAccountAccessTokenParameters{}), withDeletionTimestamp()), + err: errors.Wrap(errBoom, errSelfInformFailed), + }, "ServiceAccountIDMatchUpToDate": { // serviceAccountId set and matches the self token's owner -> adopt it. client: &mockClient{MockGetSelf: func(_ ...gitlab.RequestOptionFunc) (*gitlab.PersonalAccessToken, *gitlab.Response, error) { @@ -390,6 +421,23 @@ func TestDelete(t *testing.T) { cr: saToken(withExternalName(sAccessTokenID)), wantErr: errors.Wrap(errBoom, errDeleteFailed), }, + // A second reconcile hitting an already-revoked self-token gets 401; + // revoke is idempotent, so this must succeed. + "SelfRevoke401Idempotent": { + self: true, + client: &mockClient{MockRevokeSelf: func(_ ...gitlab.RequestOptionFunc) (*gitlab.Response, error) { + return &gitlab.Response{Response: &http.Response{StatusCode: http.StatusUnauthorized}}, errBoom + }}, + cr: saToken(withExternalName(sAccessTokenID)), + }, + // Rate-limited self-revoke (403) is likewise idempotent success. + "SelfRevoke403Idempotent": { + self: true, + client: &mockClient{MockRevokeSelf: func(_ ...gitlab.RequestOptionFunc) (*gitlab.Response, error) { + return &gitlab.Response{Response: &http.Response{StatusCode: http.StatusForbidden}}, errBoom + }}, + cr: saToken(withExternalName(sAccessTokenID)), + }, } for name, tc := range cases { t.Run(name, func(t *testing.T) { diff --git a/pkg/namespaced/clients/gitlab.go b/pkg/namespaced/clients/gitlab.go index e5fd51c6..514fb104 100644 --- a/pkg/namespaced/clients/gitlab.go +++ b/pkg/namespaced/clients/gitlab.go @@ -19,6 +19,7 @@ package clients import ( "crypto/sha256" "encoding/hex" + "net/http" "time" "github.com/google/go-cmp/cmp" @@ -248,6 +249,16 @@ func IsResponseNotFound(res *gitlab.Response) bool { return false } +// IsResponseUnauthorized reports whether the GitLab response indicates the +// credential itself was rejected: 401 Unauthorized or 403 Forbidden. GitLab +// returns 401 when a token is revoked or expired, and escalates repeated +// unauthorized requests from the same IP to 403. Both mean the token can no +// longer authenticate (as opposed to a missing resource or transient failure). +func IsResponseUnauthorized(res *gitlab.Response) bool { + return res != nil && res.Response != nil && + (res.StatusCode == http.StatusUnauthorized || res.StatusCode == http.StatusForbidden) +} + // TokenHash returns a stable hex-encoded SHA-256 digest of the supplied token // value, or an empty string when no token is set. GitLab never returns webhook // secret tokens, so the digest is stored in status.atProvider to detect when a diff --git a/pkg/namespaced/controller/groups/serviceaccountaccesstokens/controller.go b/pkg/namespaced/controller/groups/serviceaccountaccesstokens/controller.go index f882bc0e..28b427a2 100644 --- a/pkg/namespaced/controller/groups/serviceaccountaccesstokens/controller.go +++ b/pkg/namespaced/controller/groups/serviceaccountaccesstokens/controller.go @@ -203,8 +203,15 @@ func (e *external) Observe(ctx context.Context, mg resource.Managed) (managed.Ex // token is whatever the ProviderConfig authenticates with, so the external name // is auto-adopted from the response. func (e *external) observeSelf(ctx context.Context, cr *v1alpha1.ServiceAccountAccessToken) (managed.ExternalObservation, error) { - at, _, err := e.client.GetServiceAccountSelf(gitlab.WithContext(ctx)) + at, res, err := e.client.GetServiceAccountSelf(gitlab.WithContext(ctx)) if err != nil { + // During deletion the self-revoke has already run, so the self + // endpoints reject the (now revoked) credential with 401/403. That is + // the expected terminal state: report the resource gone so the + // finalizer is removed instead of wedging the delete in a retry loop. + if meta.WasDeleted(cr) && clients.IsResponseUnauthorized(res) { + return managed.ExternalObservation{ResourceExists: false}, nil + } // A dead self-credential is unrecoverable by the provider: surface a // clear error rather than masquerading as "does not exist" (which would // push the resource into a doomed self-rotate loop). @@ -418,8 +425,18 @@ func (e *external) Delete(ctx context.Context, mg resource.Managed) (managed.Ext // Revoke the token the ProviderConfig authenticates with. This breaks // every resource using that ProviderConfig; use deletionPolicy: Orphan // to keep the token. - _, err := e.client.RevokeServiceAccountSelf(gitlab.WithContext(ctx)) - return managed.ExternalDelete{}, errors.Wrap(err, errDeleteFailed) + res, err := e.client.RevokeServiceAccountSelf(gitlab.WithContext(ctx)) + if err != nil { + // A second reconcile hitting an already-revoked token can no longer + // authenticate and gets 401/403. Treat that as an idempotent + // success so the delete does not wedge on an already-completed + // revoke. + if clients.IsResponseUnauthorized(res) { + return managed.ExternalDelete{}, nil + } + return managed.ExternalDelete{}, errors.Wrap(err, errDeleteFailed) + } + return managed.ExternalDelete{}, nil } accessTokenID, err := strconv.Atoi(meta.GetExternalName(cr)) diff --git a/pkg/namespaced/controller/groups/serviceaccountaccesstokens/controller_test.go b/pkg/namespaced/controller/groups/serviceaccountaccesstokens/controller_test.go index d3614aa9..0199f426 100644 --- a/pkg/namespaced/controller/groups/serviceaccountaccesstokens/controller_test.go +++ b/pkg/namespaced/controller/groups/serviceaccountaccesstokens/controller_test.go @@ -85,6 +85,13 @@ func withExternalName(n string) tokenModifier { return func(r *v1alpha1.ServiceAccountAccessToken) { meta.SetExternalName(r, n) } } +func withDeletionTimestamp() tokenModifier { + return func(r *v1alpha1.ServiceAccountAccessToken) { + now := v1.Now() + r.SetDeletionTimestamp(&now) + } +} + func saToken(m ...tokenModifier) *v1alpha1.ServiceAccountAccessToken { cr := &v1alpha1.ServiceAccountAccessToken{} for _, f := range m { @@ -289,6 +296,48 @@ func TestObserveSelf(t *testing.T) { err: errors.Wrap(errBoom, errSelfInformFailed), }, }, + // Once the self-token is revoked during deletion, self-inform can no + // longer authenticate (401). While the resource is being deleted this is + // the expected terminal state, so report it gone to let the finalizer be + // removed instead of wedging the delete forever. + "DeletedAfterRevoke401ReportsGone": { + client: &fake.MockClient{ + MockGetServiceAccountSelf: func(_ ...gitlab.RequestOptionFunc) (*gitlab.PersonalAccessToken, *gitlab.Response, error) { + return nil, &gitlab.Response{Response: &http.Response{StatusCode: http.StatusUnauthorized}}, errBoom + }, + }, + cr: saToken(withSpec(v1alpha1.ServiceAccountAccessTokenParameters{}), withDeletionTimestamp()), + want: want{ + result: managed.ExternalObservation{ResourceExists: false}, + }, + }, + // GitLab escalates repeated unauthorized requests from the same IP to + // 403; a wedged delete poll would produce these, so treat 403 during + // deletion as gone too. + "DeletedAfterRevoke403ReportsGone": { + client: &fake.MockClient{ + MockGetServiceAccountSelf: func(_ ...gitlab.RequestOptionFunc) (*gitlab.PersonalAccessToken, *gitlab.Response, error) { + return nil, &gitlab.Response{Response: &http.Response{StatusCode: http.StatusForbidden}}, errBoom + }, + }, + cr: saToken(withSpec(v1alpha1.ServiceAccountAccessTokenParameters{}), withDeletionTimestamp()), + want: want{ + result: managed.ExternalObservation{ResourceExists: false}, + }, + }, + // A transient (non-auth) failure during deletion must still surface as an + // error rather than dropping the finalizer prematurely. + "DeletedTransientErrorStillFails": { + client: &fake.MockClient{ + MockGetServiceAccountSelf: func(_ ...gitlab.RequestOptionFunc) (*gitlab.PersonalAccessToken, *gitlab.Response, error) { + return nil, &gitlab.Response{Response: &http.Response{StatusCode: http.StatusInternalServerError}}, errBoom + }, + }, + cr: saToken(withSpec(v1alpha1.ServiceAccountAccessTokenParameters{}), withDeletionTimestamp()), + want: want{ + err: errors.Wrap(errBoom, errSelfInformFailed), + }, + }, "ServiceAccountMatch": { client: &fake.MockClient{ MockGetServiceAccountSelf: func(_ ...gitlab.RequestOptionFunc) (*gitlab.PersonalAccessToken, *gitlab.Response, error) { @@ -616,6 +665,33 @@ func TestDelete(t *testing.T) { }, want: want{cr: saToken(withExternalName(sAccessTokenID)), err: errors.Wrap(errBoom, errDeleteFailed)}, }, + // A second reconcile hitting an already-revoked self-token gets 401. + // Revoke is idempotent, so this must succeed rather than wedge the delete. + "SelfRevoke401Idempotent": { + self: true, + args: args{ + client: &fake.MockClient{ + MockRevokeServiceAccountSelf: func(_ ...gitlab.RequestOptionFunc) (*gitlab.Response, error) { + return &gitlab.Response{Response: &http.Response{StatusCode: http.StatusUnauthorized}}, errBoom + }, + }, + cr: saToken(withExternalName(sAccessTokenID)), + }, + want: want{cr: saToken(withExternalName(sAccessTokenID))}, + }, + // Rate-limited self-revoke (403) is likewise treated as an idempotent success. + "SelfRevoke403Idempotent": { + self: true, + args: args{ + client: &fake.MockClient{ + MockRevokeServiceAccountSelf: func(_ ...gitlab.RequestOptionFunc) (*gitlab.Response, error) { + return &gitlab.Response{Response: &http.Response{StatusCode: http.StatusForbidden}}, errBoom + }, + }, + cr: saToken(withExternalName(sAccessTokenID)), + }, + want: want{cr: saToken(withExternalName(sAccessTokenID))}, + }, } for name, tc := range cases { t.Run(name, func(t *testing.T) { diff --git a/pkg/namespaced/controller/instance/serviceaccountaccesstokens/controller.go b/pkg/namespaced/controller/instance/serviceaccountaccesstokens/controller.go index df32ed4b..caa744b4 100644 --- a/pkg/namespaced/controller/instance/serviceaccountaccesstokens/controller.go +++ b/pkg/namespaced/controller/instance/serviceaccountaccesstokens/controller.go @@ -204,8 +204,15 @@ func (e *external) Observe(ctx context.Context, mg resource.Managed) (managed.Ex // token is whatever the ProviderConfig authenticates with, so the external name // is auto-adopted from the response. func (e *external) observeSelf(ctx context.Context, cr *v1alpha1.ServiceAccountAccessToken) (managed.ExternalObservation, error) { - at, _, err := e.client.GetServiceAccountSelf(gitlab.WithContext(ctx)) + at, res, err := e.client.GetServiceAccountSelf(gitlab.WithContext(ctx)) if err != nil { + // During deletion the self-revoke has already run, so the self + // endpoints reject the (now revoked) credential with 401/403. That is + // the expected terminal state: report the resource gone so the + // finalizer is removed instead of wedging the delete in a retry loop. + if meta.WasDeleted(cr) && clients.IsResponseUnauthorized(res) { + return managed.ExternalObservation{ResourceExists: false}, nil + } // A dead self-credential is unrecoverable by the provider: surface a // clear error rather than masquerading as "does not exist" (which would // push the resource into a doomed self-rotate loop). @@ -454,8 +461,18 @@ func (e *external) Delete(ctx context.Context, mg resource.Managed) (managed.Ext // Revoke the token the ProviderConfig authenticates with. This breaks // every resource using that ProviderConfig; use deletionPolicy: Orphan // to keep the token. - _, err := e.client.RevokeServiceAccountSelf(gitlab.WithContext(ctx)) - return managed.ExternalDelete{}, errors.Wrap(err, errDeleteFailed) + res, err := e.client.RevokeServiceAccountSelf(gitlab.WithContext(ctx)) + if err != nil { + // A second reconcile hitting an already-revoked token can no longer + // authenticate and gets 401/403. Treat that as an idempotent + // success so the delete does not wedge on an already-completed + // revoke. + if clients.IsResponseUnauthorized(res) { + return managed.ExternalDelete{}, nil + } + return managed.ExternalDelete{}, errors.Wrap(err, errDeleteFailed) + } + return managed.ExternalDelete{}, nil } accessTokenID, err := strconv.ParseInt(meta.GetExternalName(cr), 10, 64) diff --git a/pkg/namespaced/controller/instance/serviceaccountaccesstokens/controller_test.go b/pkg/namespaced/controller/instance/serviceaccountaccesstokens/controller_test.go index badde85f..98500162 100644 --- a/pkg/namespaced/controller/instance/serviceaccountaccesstokens/controller_test.go +++ b/pkg/namespaced/controller/instance/serviceaccountaccesstokens/controller_test.go @@ -146,6 +146,13 @@ func withExternalName(n string) tokenModifier { return func(r *v1alpha1.ServiceAccountAccessToken) { meta.SetExternalName(r, n) } } +func withDeletionTimestamp() tokenModifier { + return func(r *v1alpha1.ServiceAccountAccessToken) { + now := v1.Now() + r.SetDeletionTimestamp(&now) + } +} + func saToken(m ...tokenModifier) *v1alpha1.ServiceAccountAccessToken { cr := &v1alpha1.ServiceAccountAccessToken{} for _, f := range m { @@ -333,6 +340,43 @@ func TestObserveSelf(t *testing.T) { err: errors.Wrap(errBoom, errSelfInformFailed), }, }, + // During deletion the self-token is already revoked, so self-inform gets + // 401. Report gone so the finalizer is removed instead of wedging delete. + "DeletedAfterRevoke401ReportsGone": { + client: &mockClient{ + MockGetServiceAccountSelf: func(_ ...gitlab.RequestOptionFunc) (*gitlab.PersonalAccessToken, *gitlab.Response, error) { + return nil, &gitlab.Response{Response: &http.Response{StatusCode: http.StatusUnauthorized}}, errBoom + }, + }, + cr: saToken(withSpec(v1alpha1.ServiceAccountAccessTokenParameters{}), withDeletionTimestamp()), + want: want{ + result: managed.ExternalObservation{ResourceExists: false}, + }, + }, + // Repeated unauthorized polls escalate to 403; treat as gone during deletion too. + "DeletedAfterRevoke403ReportsGone": { + client: &mockClient{ + MockGetServiceAccountSelf: func(_ ...gitlab.RequestOptionFunc) (*gitlab.PersonalAccessToken, *gitlab.Response, error) { + return nil, &gitlab.Response{Response: &http.Response{StatusCode: http.StatusForbidden}}, errBoom + }, + }, + cr: saToken(withSpec(v1alpha1.ServiceAccountAccessTokenParameters{}), withDeletionTimestamp()), + want: want{ + result: managed.ExternalObservation{ResourceExists: false}, + }, + }, + // A transient failure during deletion must still surface as an error. + "DeletedTransientErrorStillFails": { + client: &mockClient{ + MockGetServiceAccountSelf: func(_ ...gitlab.RequestOptionFunc) (*gitlab.PersonalAccessToken, *gitlab.Response, error) { + return nil, &gitlab.Response{Response: &http.Response{StatusCode: http.StatusInternalServerError}}, errBoom + }, + }, + cr: saToken(withSpec(v1alpha1.ServiceAccountAccessTokenParameters{}), withDeletionTimestamp()), + want: want{ + err: errors.Wrap(errBoom, errSelfInformFailed), + }, + }, "ServiceAccountIDMatchUpToDate": { // serviceAccountId set and matches the self token's owner -> adopt it. client: &mockClient{ @@ -717,6 +761,33 @@ func TestDelete(t *testing.T) { }, want: want{cr: saToken(withExternalName(sAccessTokenID)), err: errors.Wrap(errBoom, errDeleteFailed)}, }, + // A second reconcile hitting an already-revoked self-token gets 401; + // revoke is idempotent, so this must succeed. + "SelfRevoke401Idempotent": { + self: true, + args: args{ + client: &mockClient{ + MockRevokeServiceAccountSelf: func(_ ...gitlab.RequestOptionFunc) (*gitlab.Response, error) { + return &gitlab.Response{Response: &http.Response{StatusCode: http.StatusUnauthorized}}, errBoom + }, + }, + cr: saToken(withExternalName(sAccessTokenID)), + }, + want: want{cr: saToken(withExternalName(sAccessTokenID))}, + }, + // Rate-limited self-revoke (403) is likewise idempotent success. + "SelfRevoke403Idempotent": { + self: true, + args: args{ + client: &mockClient{ + MockRevokeServiceAccountSelf: func(_ ...gitlab.RequestOptionFunc) (*gitlab.Response, error) { + return &gitlab.Response{Response: &http.Response{StatusCode: http.StatusForbidden}}, errBoom + }, + }, + cr: saToken(withExternalName(sAccessTokenID)), + }, + want: want{cr: saToken(withExternalName(sAccessTokenID))}, + }, } for name, tc := range cases { t.Run(name, func(t *testing.T) { diff --git a/pkg/namespaced/controller/projects/serviceaccountaccesstokens/controller.go b/pkg/namespaced/controller/projects/serviceaccountaccesstokens/controller.go index 5f3d5fae..4f45225a 100644 --- a/pkg/namespaced/controller/projects/serviceaccountaccesstokens/controller.go +++ b/pkg/namespaced/controller/projects/serviceaccountaccesstokens/controller.go @@ -199,8 +199,15 @@ func (e *external) Observe(ctx context.Context, mg resource.Managed) (managed.Ex // token is whatever the ProviderConfig authenticates with, so the external name // is auto-adopted from the response. func (e *external) observeSelf(ctx context.Context, cr *v1alpha1.ServiceAccountAccessToken) (managed.ExternalObservation, error) { - at, _, err := e.client.GetServiceAccountSelf(gitlab.WithContext(ctx)) + at, res, err := e.client.GetServiceAccountSelf(gitlab.WithContext(ctx)) if err != nil { + // During deletion the self-revoke has already run, so the self + // endpoints reject the (now revoked) credential with 401/403. That is + // the expected terminal state: report the resource gone so the + // finalizer is removed instead of wedging the delete in a retry loop. + if meta.WasDeleted(cr) && clients.IsResponseUnauthorized(res) { + return managed.ExternalObservation{ResourceExists: false}, nil + } // A dead self-credential is unrecoverable by the provider: surface a // clear error rather than masquerading as "does not exist". return managed.ExternalObservation{}, errors.Wrap(err, errSelfInformFailed) @@ -414,8 +421,18 @@ func (e *external) Delete(ctx context.Context, mg resource.Managed) (managed.Ext // Revoke the token the ProviderConfig authenticates with. This breaks // every resource using that ProviderConfig; use deletionPolicy: Orphan // to keep the token. - _, err := e.client.RevokeServiceAccountSelf(gitlab.WithContext(ctx)) - return managed.ExternalDelete{}, errors.Wrap(err, errDeleteFailed) + res, err := e.client.RevokeServiceAccountSelf(gitlab.WithContext(ctx)) + if err != nil { + // A second reconcile hitting an already-revoked token can no longer + // authenticate and gets 401/403. Treat that as an idempotent + // success so the delete does not wedge on an already-completed + // revoke. + if clients.IsResponseUnauthorized(res) { + return managed.ExternalDelete{}, nil + } + return managed.ExternalDelete{}, errors.Wrap(err, errDeleteFailed) + } + return managed.ExternalDelete{}, nil } accessTokenID, err := strconv.Atoi(meta.GetExternalName(cr)) diff --git a/pkg/namespaced/controller/projects/serviceaccountaccesstokens/controller_test.go b/pkg/namespaced/controller/projects/serviceaccountaccesstokens/controller_test.go index 0346cabf..554bf3ec 100644 --- a/pkg/namespaced/controller/projects/serviceaccountaccesstokens/controller_test.go +++ b/pkg/namespaced/controller/projects/serviceaccountaccesstokens/controller_test.go @@ -105,6 +105,12 @@ func withSpec(fp v1alpha1.ServiceAccountAccessTokenParameters) tokenModifier { func withExternalName(n string) tokenModifier { return func(r *v1alpha1.ServiceAccountAccessToken) { meta.SetExternalName(r, n) } } +func withDeletionTimestamp() tokenModifier { + return func(r *v1alpha1.ServiceAccountAccessToken) { + now := v1.Now() + r.SetDeletionTimestamp(&now) + } +} func saToken(m ...tokenModifier) *v1alpha1.ServiceAccountAccessToken { cr := &v1alpha1.ServiceAccountAccessToken{} for _, f := range m { @@ -213,6 +219,31 @@ func TestObserveSelf(t *testing.T) { cr: saToken(withSpec(v1alpha1.ServiceAccountAccessTokenParameters{})), err: errors.Wrap(errBoom, errSelfInformFailed), }, + // During deletion the self-token is already revoked, so self-inform gets + // 401. Report gone so the finalizer is removed instead of wedging delete. + "DeletedAfterRevoke401ReportsGone": { + client: &mockClient{MockGetSelf: func(_ ...gitlab.RequestOptionFunc) (*gitlab.PersonalAccessToken, *gitlab.Response, error) { + return nil, &gitlab.Response{Response: &http.Response{StatusCode: http.StatusUnauthorized}}, errBoom + }}, + cr: saToken(withSpec(v1alpha1.ServiceAccountAccessTokenParameters{}), withDeletionTimestamp()), + result: managed.ExternalObservation{ResourceExists: false}, + }, + // Repeated unauthorized polls escalate to 403; treat as gone during deletion too. + "DeletedAfterRevoke403ReportsGone": { + client: &mockClient{MockGetSelf: func(_ ...gitlab.RequestOptionFunc) (*gitlab.PersonalAccessToken, *gitlab.Response, error) { + return nil, &gitlab.Response{Response: &http.Response{StatusCode: http.StatusForbidden}}, errBoom + }}, + cr: saToken(withSpec(v1alpha1.ServiceAccountAccessTokenParameters{}), withDeletionTimestamp()), + result: managed.ExternalObservation{ResourceExists: false}, + }, + // A transient failure during deletion must still surface as an error. + "DeletedTransientErrorStillFails": { + client: &mockClient{MockGetSelf: func(_ ...gitlab.RequestOptionFunc) (*gitlab.PersonalAccessToken, *gitlab.Response, error) { + return nil, &gitlab.Response{Response: &http.Response{StatusCode: http.StatusInternalServerError}}, errBoom + }}, + cr: saToken(withSpec(v1alpha1.ServiceAccountAccessTokenParameters{}), withDeletionTimestamp()), + err: errors.Wrap(errBoom, errSelfInformFailed), + }, "ServiceAccountIDMatchUpToDate": { // serviceAccountId set and matches the self token's owner -> adopt it. client: &mockClient{MockGetSelf: func(_ ...gitlab.RequestOptionFunc) (*gitlab.PersonalAccessToken, *gitlab.Response, error) { @@ -388,6 +419,23 @@ func TestDelete(t *testing.T) { cr: saToken(withExternalName(sAccessTokenID)), wantErr: errors.Wrap(errBoom, errDeleteFailed), }, + // A second reconcile hitting an already-revoked self-token gets 401; + // revoke is idempotent, so this must succeed. + "SelfRevoke401Idempotent": { + self: true, + client: &mockClient{MockRevokeSelf: func(_ ...gitlab.RequestOptionFunc) (*gitlab.Response, error) { + return &gitlab.Response{Response: &http.Response{StatusCode: http.StatusUnauthorized}}, errBoom + }}, + cr: saToken(withExternalName(sAccessTokenID)), + }, + // Rate-limited self-revoke (403) is likewise idempotent success. + "SelfRevoke403Idempotent": { + self: true, + client: &mockClient{MockRevokeSelf: func(_ ...gitlab.RequestOptionFunc) (*gitlab.Response, error) { + return &gitlab.Response{Response: &http.Response{StatusCode: http.StatusForbidden}}, errBoom + }}, + cr: saToken(withExternalName(sAccessTokenID)), + }, } for name, tc := range cases { t.Run(name, func(t *testing.T) {