Skip to content

api: Directly indicate credentials revocation in cluster #4796

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
mbarnes wants to merge 2 commits into
base: main
Choose a base branch
from
Open

api: Directly indicate credentials revocation in cluster #4796

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
8fa049f
api: Directly indicate credentials revocation in cluster
Feb 11, 2026
cb9bfad
backend: Conditionally clear RevokeCredentialsOperationID
Apr 9, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
26 changes: 26 additions & 0 deletions backend/pkg/controllers/operationcontrollers/operation_revoke_credentials.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -123,6 +123,32 @@ func (opsync *operationRevokeCredentials) SynchronizeOperation(ctx context.Conte
return nil
}

// FIXME May want a version of patchOperation that acts on a transaction
// so we can group these two writes together. For now, if the cluster
// replace fails we'll just retry later since the operation status
// will remain non-terminal.

dbClient := opsync.cosmosClient.HCPClusters(oldOperation.ExternalID.SubscriptionID, oldOperation.ExternalID.ResourceGroupName)
cluster, err := dbClient.Get(ctx, oldOperation.ExternalID.Name)
if err != nil {
return utils.TrackError(err)
}

// If the controller successfully clears the RevokeCredentialsOperationID field
// in the cluster document but fails to update the operation document, then the
// frontend is free to start a new RevokeCredentials operation, and may well do
// so before the failed operation update is retried. Account for this by making
// sure the field value still matches this operation's ID before clearing it.
if cluster.ServiceProviderProperties.RevokeCredentialsOperationID == oldOperation.OperationID.Name {
logger.Info("clearing RevokeCredentialsOperationID from cluster")
cluster.ServiceProviderProperties.RevokeCredentialsOperationID = ""
_, err = dbClient.Replace(ctx, cluster, nil)
if err != nil {
return utils.TrackError(err)
}
}

logger.Info("updating status")
err = patchOperation(ctx, opsync.cosmosClient, oldOperation, newOperationStatus, newOperationError, postAsyncNotificationFn(opsync.notificationClient))
if err != nil {
return utils.TrackError(err)
Expand Down
25 changes: 25 additions & 0 deletions frontend/pkg/frontend/frontend.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -358,6 +358,13 @@ func (f *Frontend) ArmResourceActionRequestAdminCredential(writer http.ResponseW

// New credential cannot be requested while credentials are being revoked.

// XXX Enable this once the backend change to clear RevokeCredentialsOperationID reaches production.
//if len(cluster.ServiceProviderProperties.RevokeCredentialsOperationID) > 0 {
// writer.Header().Set("Retry-After", strconv.Itoa(10))
// return arm.NewConflictError(clusterResourceID, "Cannot request credential while credentials are being revoked")
//}

// XXX Remove this once the backend change to clear RevokeCredentialsOperationID reaches production.
iterator := f.dbClient.Operations(clusterResourceID.SubscriptionID).ListActiveOperations(&database.DBClientListActiveOperationDocsOptions{
Request: api.Ptr(database.OperationRequestRevokeCredentials),
ExternalID: clusterResourceID,
Expand All @@ -372,6 +379,7 @@ func (f *Frontend) ArmResourceActionRequestAdminCredential(writer http.ResponseW
if err != nil {
return utils.TrackError(err)
}
// XXX End of code to remove.

csCredential, err := f.clusterServiceClient.PostBreakGlassCredential(ctx, cluster.ServiceProviderProperties.ClusterServiceID)
if err != nil {
Expand Down Expand Up @@ -460,6 +468,13 @@ func (f *Frontend) ArmResourceActionRevokeCredentials(writer http.ResponseWriter

// Credential revocation cannot be requested while another revocation is in progress.

// XXX Enable this once the backend change to clear RevokeCredentialsOperationID reaches production.
//if len(cluster.ServiceProviderProperties.RevokeCredentialsOperationID) > 0 {
// writer.Header().Set("Retry-After", strconv.Itoa(10))
// return arm.NewConflictError(clusterResourceID, "Credentials are already being revoked")
//}

// XXX Remove this once the backend change to clear RevokeCredentialsOperationID reaches production.
iterator := f.dbClient.Operations(clusterResourceID.SubscriptionID).ListActiveOperations(&database.DBClientListActiveOperationDocsOptions{
Request: api.Ptr(database.OperationRequestRevokeCredentials),
ExternalID: clusterResourceID,
Expand All @@ -474,6 +489,7 @@ func (f *Frontend) ArmResourceActionRevokeCredentials(writer http.ResponseWriter
if err != nil {
return utils.TrackError(err)
}
// XXX End of code to remove.

err = f.clusterServiceClient.DeleteBreakGlassCredentials(ctx, cluster.ServiceProviderProperties.ClusterServiceID)
if err != nil {
Expand Down Expand Up @@ -507,6 +523,15 @@ func (f *Frontend) ArmResourceActionRevokeCredentials(writer http.ResponseWriter
return utils.TrackError(err)
}

// Setting this field effectively blocks further credential
// requests or revocations until the backend clears the field.
cluster.ServiceProviderProperties.RevokeCredentialsOperationID = operationDoc.OperationID.Name

_, err = f.dbClient.HCPClusters(cluster.ID.SubscriptionID, cluster.ID.ResourceGroupName).AddReplaceToTransaction(ctx, transaction, cluster, nil)
if err != nil {
return utils.TrackError(err)
}

_, err = transaction.Execute(ctx, nil)
if err != nil {
return utils.TrackError(err)
Expand Down
60 changes: 29 additions & 31 deletions frontend/pkg/frontend/frontend_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -541,26 +541,24 @@ func TestDeploymentPreflight(t *testing.T) {

func TestRequestAdminCredential(t *testing.T) {
type testCase struct {
name string
clusterProvisioningState arm.ProvisioningState
revokeCredentialsStatus arm.ProvisioningState
statusCode int
name string
clusterProvisioningState arm.ProvisioningState
revokeCredentialsOperationID string
statusCode int
}

tests := []testCase{
{
name: "Request conflict: credentials revoking",
clusterProvisioningState: arm.ProvisioningStateSucceeded,
revokeCredentialsStatus: arm.ProvisioningStateDeleting,
statusCode: http.StatusConflict,
name: "Request conflict: credentials revoking",
clusterProvisioningState: arm.ProvisioningStateSucceeded,
revokeCredentialsOperationID: "revocation-in-progress",
statusCode: http.StatusConflict,
},
}

for clusterProvisioningState := range arm.ListProvisioningStates() {
test := testCase{
// Previously completed revocation does not interfere.
clusterProvisioningState: clusterProvisioningState,
revokeCredentialsStatus: arm.ProvisioningStateSucceeded,
}
if clusterProvisioningState.IsTerminal() {
test.name = "Request accepted: cluster state=" + string(clusterProvisioningState)
Expand Down Expand Up @@ -606,15 +604,16 @@ func TestRequestAdminCredential(t *testing.T) {
},
},
ServiceProviderProperties: api.HCPOpenShiftClusterServiceProviderProperties{
ProvisioningState: test.clusterProvisioningState,
ClusterServiceID: clusterInternalID,
ProvisioningState: test.clusterProvisioningState,
ClusterServiceID: clusterInternalID,
RevokeCredentialsOperationID: test.revokeCredentialsOperationID,
},
}
_, err := mockDBClient.HCPClusters(clusterResourceID.SubscriptionID, clusterResourceID.ResourceGroupName).Create(ctx, cluster, nil)
require.NoError(t, err)

// Add active revoke operation if needed
if test.clusterProvisioningState.IsTerminal() && !test.revokeCredentialsStatus.IsTerminal() {
if test.clusterProvisioningState.IsTerminal() && len(test.revokeCredentialsOperationID) > 0 {
operationID := api.Must(azcorearm.ParseResourceID(api.TestSubscriptionResourceID + "/providers/" + api.ProviderNamespace + "/locations/" + api.TestLocation + "/" + api.OperationStatusResourceTypeName + "/" + uuid.New().String()))
resourceID := api.Must(azcorearm.ParseResourceID(api.TestSubscriptionResourceID + "/providers/" + api.ProviderNamespace + "/hcpOperationStatuses/" + uuid.New().String()))
revokeOp := &api.Operation{
Expand All @@ -626,14 +625,14 @@ func TestRequestAdminCredential(t *testing.T) {
Request: database.OperationRequestRevokeCredentials,
ExternalID: clusterResourceID,
InternalID: clusterInternalID,
Status: test.revokeCredentialsStatus,
Status: arm.ProvisioningStateDeleting,
}
_, err := mockDBClient.Operations(clusterResourceID.SubscriptionID).Create(ctx, revokeOp, nil)
require.NoError(t, err)
}

// Set up cluster service expectations for success case
if test.clusterProvisioningState.IsTerminal() && test.revokeCredentialsStatus.IsTerminal() {
if test.clusterProvisioningState.IsTerminal() && len(test.revokeCredentialsOperationID) == 0 {
mockCSClient.EXPECT().
PostBreakGlassCredential(gomock.Any(), clusterInternalID).
Return(cmv1.NewBreakGlassCredential().
Expand Down Expand Up @@ -661,26 +660,24 @@ func TestRequestAdminCredential(t *testing.T) {

func TestRevokeCredentials(t *testing.T) {
type testCase struct {
name string
clusterProvisioningState arm.ProvisioningState
revokeCredentialsStatus arm.ProvisioningState
statusCode int
name string
clusterProvisioningState arm.ProvisioningState
revokeCredentialsOperationID string
statusCode int
}

tests := []testCase{
{
name: "Request conflict: credentials revoking",
clusterProvisioningState: arm.ProvisioningStateSucceeded,
revokeCredentialsStatus: arm.ProvisioningStateDeleting,
statusCode: http.StatusConflict,
name: "Request conflict: credentials revoking",
clusterProvisioningState: arm.ProvisioningStateSucceeded,
revokeCredentialsOperationID: "revocation-in-progress",
statusCode: http.StatusConflict,
},
}

for clusterProvisioningState := range arm.ListProvisioningStates() {
test := testCase{
// Previously completed revocation does not interfere.
clusterProvisioningState: clusterProvisioningState,
revokeCredentialsStatus: arm.ProvisioningStateSucceeded,
}
if clusterProvisioningState.IsTerminal() {
test.name = "Request accepted: cluster state=" + string(clusterProvisioningState)
Expand Down Expand Up @@ -726,15 +723,16 @@ func TestRevokeCredentials(t *testing.T) {
},
},
ServiceProviderProperties: api.HCPOpenShiftClusterServiceProviderProperties{
ProvisioningState: test.clusterProvisioningState,
ClusterServiceID: clusterInternalID,
ProvisioningState: test.clusterProvisioningState,
ClusterServiceID: clusterInternalID,
RevokeCredentialsOperationID: test.revokeCredentialsOperationID,
},
}
_, err := mockDBClient.HCPClusters(clusterResourceID.SubscriptionID, clusterResourceID.ResourceGroupName).Create(ctx, cluster, nil)
require.NoError(t, err)

// Add active revoke operation if needed
if test.clusterProvisioningState.IsTerminal() && !test.revokeCredentialsStatus.IsTerminal() {
if test.clusterProvisioningState.IsTerminal() && len(test.revokeCredentialsOperationID) > 0 {
operationID := api.Must(azcorearm.ParseResourceID(api.TestSubscriptionResourceID + "/providers/" + api.ProviderNamespace + "/locations/" + api.TestLocation + "/" + api.OperationStatusResourceTypeName + "/" + uuid.New().String()))
resourceID := api.Must(azcorearm.ParseResourceID(api.TestSubscriptionResourceID + "/providers/" + api.ProviderNamespace + "/hcpOperationStatuses/" + uuid.New().String()))
revokeOp := &api.Operation{
Expand All @@ -746,14 +744,14 @@ func TestRevokeCredentials(t *testing.T) {
Request: database.OperationRequestRevokeCredentials,
ExternalID: clusterResourceID,
InternalID: clusterInternalID,
Status: test.revokeCredentialsStatus,
Status: arm.ProvisioningStateDeleting,
}
_, err := mockDBClient.Operations(clusterResourceID.SubscriptionID).Create(ctx, revokeOp, nil)
require.NoError(t, err)
}

// Add active request credential operation (will be cancelled) for success case
if test.clusterProvisioningState.IsTerminal() && test.revokeCredentialsStatus.IsTerminal() {
if test.clusterProvisioningState.IsTerminal() && len(test.revokeCredentialsOperationID) == 0 {
operationID := api.Must(azcorearm.ParseResourceID(api.TestSubscriptionResourceID + "/providers/" + api.ProviderNamespace + "/locations/" + api.TestLocation + "/" + api.OperationStatusResourceTypeName + "/" + uuid.New().String()))
resourceID := api.Must(azcorearm.ParseResourceID(api.TestSubscriptionResourceID + "/providers/" + api.ProviderNamespace + "/hcpOperationStatuses/" + uuid.New().String()))
requestOp := &api.Operation{
Expand All @@ -772,7 +770,7 @@ func TestRevokeCredentials(t *testing.T) {
}

// Set up cluster service expectations for success case
if test.clusterProvisioningState.IsTerminal() && test.revokeCredentialsStatus.IsTerminal() {
if test.clusterProvisioningState.IsTerminal() && len(test.revokeCredentialsOperationID) == 0 {
mockCSClient.EXPECT().
DeleteBreakGlassCredentials(gomock.Any(), clusterInternalID).
Return(nil)
Expand Down
17 changes: 9 additions & 8 deletions internal/api/types_cluster.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -64,14 +64,15 @@ type HCPOpenShiftClusterCustomerProperties struct {

// HCPOpenShiftClusterCustomerProperties represents the property bag of a HCPOpenShiftCluster resource.
type HCPOpenShiftClusterServiceProviderProperties struct {
ExistingCosmosUID string `json:"-"`
ProvisioningState arm.ProvisioningState `json:"provisioningState,omitempty"`
ClusterServiceID InternalID `json:"clusterServiceID,omitempty"`
ActiveOperationID string `json:"activeOperationId,omitempty"`
DNS ServiceProviderDNSProfile `json:"dns,omitempty"`
Console ServiceProviderConsoleProfile `json:"console,omitempty"`
API ServiceProviderAPIProfile `json:"api,omitempty"`
Platform ServiceProviderPlatformProfile `json:"platform,omitempty"`
ExistingCosmosUID string `json:"-"`
ProvisioningState arm.ProvisioningState `json:"provisioningState,omitempty"`
ClusterServiceID InternalID `json:"clusterServiceID,omitempty"`
ActiveOperationID string `json:"activeOperationId,omitempty"`
RevokeCredentialsOperationID string `json:"revokeCredentialsOperationId,omitempty"`
DNS ServiceProviderDNSProfile `json:"dns,omitempty"`
Console ServiceProviderConsoleProfile `json:"console,omitempty"`
API ServiceProviderAPIProfile `json:"api,omitempty"`
Platform ServiceProviderPlatformProfile `json:"platform,omitempty"`

// ExperimentalFeatures captures experimental feature state evaluated from
// AFEC and per-resource tags. Stored in Cosmos but NOT exposed via ARM API.
Expand Down
2 changes: 2 additions & 0 deletions internal/api/v20240610preview/conversion_fuzz_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -51,6 +51,8 @@ func TestRoundTripInternalExternalInternal(t *testing.T) {
c.FillNoCustom(j)
// ActiveOperationID does not roundtrip through the external type because it is purely an internal detail
j.ActiveOperationID = ""
// RevokeCredentialsOperationID does not roundtrip through the external type because it is purely an internal detail
j.RevokeCredentialsOperationID = ""
// ClusterServiceID does not roundtrip through the external type because it is purely an internal detail
j.ClusterServiceID = ocm.InternalID{}
j.ExistingCosmosUID = ""
Expand Down
2 changes: 2 additions & 0 deletions internal/api/v20251223preview/conversion_fuzz_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -49,6 +49,8 @@ func TestRoundTripInternalExternalInternal(t *testing.T) {
c.FillNoCustom(j)
// ActiveOperationID does not roundtrip through the external type because it is purely an internal detail
j.ActiveOperationID = ""
// RevokeCredentialsOperationID does not roundtrip through the external type because it is purely an internal detail
j.RevokeCredentialsOperationID = ""
// ClusterServiceID does not roundtrip through the external type because it is purely an internal detail
j.ClusterServiceID = ocm.InternalID{}
j.ExistingCosmosUID = ""
Expand Down
Loading