[wip] feat: add SystemAdminCredentialRequest type, store credentials in Cos…#6196
[wip] feat: add SystemAdminCredentialRequest type, store credentials in Cos…#6196deads2k wants to merge 3 commits into
Conversation
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: deads2k The full list of commands accepted by this bot can be found here. The pull request process is described here DetailsNeeds approval from an approver in each of these files:
Approvers can indicate their approval by writing |
There was a problem hiding this comment.
Pull request overview
This PR introduces a new Cosmos-backed SystemAdminCredentialRequest resource to persist issued system admin credentials (including a prebuilt kubeconfig in the cluster-service break-glass path), adds a pure BuildKubeconfig helper for assembling kubeconfigs from CSR outputs, and wires a new backend GC controller to delete old credential request documents after 48 hours. It updates the backend OperationRequestCredential flow to create the credential request document on issuance and repoints Operation.InternalID to the new ARM resource ID, and updates the frontend OperationResult path to fetch/assemble the admin credential from Cosmos.
Changes:
- Add
SystemAdminCredentialRequestAPI type + resource ID helpers + DB CRUD interfaces (Cosmos + mocks) to store credential request documents under a cluster. - Update backend credential issuance flow to persist a
SystemAdminCredentialRequestin Cosmos and return it via frontendOperationResultby reading from Cosmos. - Add a
CredentialGCmismatch controller to periodically delete credential request documents older than 48 hours.
Reviewed changes
Copilot reviewed 14 out of 16 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| internal/systemadmincredential/kubeconfig.go | Adds BuildKubeconfig helper to assemble kubeconfigs from cert/key/API URL. |
| internal/databasetesting/mock_resources_db_client.go | Exposes mock DB client method for SystemAdminCredentialRequests CRUD. |
| internal/databasetesting/mock_resources_crud.go | Adds mock CRUD implementation for SystemAdminCredentialRequest resources. |
| internal/database/database.go | Adds Cosmos DB client method for SystemAdminCredentialRequests CRUD under clusters. |
| internal/database/crud_hcpcluster.go | Defines SystemAdminCredentialRequestsCRUD interface + implementation wrapper. |
| internal/api/zz_generated.deepcopy.go | Adds deepcopy support for the new API types. |
| internal/api/types_systemadmincredential.go | Introduces the SystemAdminCredentialRequest internal API type and condition constants. |
| internal/api/types_internalid.go | Adds InternalID validation support for ARM IDs pointing to SystemAdminCredentialRequest. |
| internal/api/types_cosmosdata.go | Adds helpers to construct/parse SystemAdminCredentialRequest ARM resource IDs. |
| internal/api/registry.go | Registers SystemAdminCredentialRequestResourceType and its name constant. |
| frontend/pkg/frontend/frontend.go | Updates OperationResult to return admin credential by reading/assembling it from Cosmos. |
| frontend/go.mod | Pulls in additional indirect deps needed by the new kubeconfig building path. |
| frontend/go.sum | Updates sums for newly pulled indirect deps. |
| backend/pkg/controllers/operationcontrollers/operation_request_credential.go | On issuance, creates SystemAdminCredentialRequest in Cosmos and repoints operation InternalID. |
| backend/pkg/controllers/mismatchcontrollers/credential_gc.go | Adds periodic GC controller to delete SystemAdminCredentialRequest older than 48 hours. |
| backend/pkg/app/backend.go | Wires the new CredentialGC controller into backend startup. |
Files not reviewed (1)
- internal/api/zz_generated.deepcopy.go: Generated file
| func BuildKubeconfig(signedCertificateBase64, privateKeyPEM, apiURL string) ([]byte, error) { | ||
| certPEM, err := base64.StdEncoding.DecodeString(signedCertificateBase64) | ||
| if err != nil { | ||
| return nil, fmt.Errorf("failed to decode signed certificate: %w", err) | ||
| } |
| logger := utils.LoggerFromContext(ctx) | ||
| logger = logger.WithValues(utils.LogValues{}.AddSubscriptionID(ref)...) | ||
| ctx = utils.ContextWithLogger(ctx, logger) |
| // SystemAdminCredentialRequestSpec contains the desired state of the credential request. | ||
| type SystemAdminCredentialRequestSpec struct { | ||
| // Username is the K8s username embedded in the cert CN. | ||
| Username string `json:"username,omitempty"` | ||
| // CreationTimestamp is when the credential request was created (server-set). | ||
| // The garbage collector deletes each request once it is older than the | ||
| // retention window, regardless of the request's status. | ||
| CreationTimestamp metav1.Time `json:"creationTimestamp"` | ||
| // ExpirationTimestamp is when the cert ceases to be valid (server-set, now + 24h). | ||
| ExpirationTimestamp metav1.Time `json:"expirationTimestamp"` | ||
| // OperationID is the ARM operation that created this credential request. | ||
| OperationID string `json:"operationID"` | ||
| // PublicKeyPEM is the public half of the keypair, PEM-encoded. | ||
| PublicKeyPEM string `json:"publicKeyPEM"` | ||
| // PrivateKeyPEM is the private half of the keypair, PEM-encoded. | ||
| // Treat as a secret in logs, dumps, and telemetry. | ||
| PrivateKeyPEM string `json:"privateKeyPEM"` | ||
| } |
| case cmv1.BreakGlassCredentialStatusIssued: | ||
| credResourceID, err := opsync.createSystemAdminCredentialRequest(ctx, key, oldOperation, breakGlassCredential) | ||
| if err != nil { | ||
| return utils.TrackError(err) | ||
| } | ||
| credInternalID, err := api.NewInternalID(credResourceID.String()) | ||
| if err != nil { | ||
| return utils.TrackError(fmt.Errorf("failed to create InternalID for credential: %w", err)) | ||
| } | ||
| oldOperation.InternalID = credInternalID | ||
| newOperationStatus = arm.ProvisioningStateSucceeded |
| case operation.InternalID.Kind() == cmv1.BreakGlassCredentialKind: | ||
| csBreakGlassCredential, err := f.clusterServiceClient.GetBreakGlassCredential(ctx, operation.InternalID) | ||
| case operation.InternalID.Kind() == api.SystemAdminCredentialRequestKind: | ||
| adminCred, err := f.assembleAdminCredentialFromCosmos(ctx, operation) |
There was a problem hiding this comment.
We need to figure out how to rollout this without impact on pre-existing resources. An issue we have now is that for older operations this would not return any result as the creation of the resource exists for the active operations which would have ended at that point.
Independently on that, the backend code (whatever we end up implementing) should be rolled out before the frontend code as it will depend on that
…mos on issuance, add 48h GC Brings in SystemAdminCredentialRequest API type and CRUD from cs-198-admincreds, plus BuildKubeconfig in internal/systemadmincredential. When the break-glass credential is issued by cluster service, operation_request_credential now creates a SystemAdminCredentialRequest in Cosmos (name = operationID) and updates the operation's InternalID to the ARM resource ID. The frontend reads the credential back from Cosmos via assembleAdminCredentialFromCosmos. A new CredentialGC controller runs every hour, iterates all clusters per subscription, and deletes any SystemAdminCredentialRequest older than 48 hours. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
…for keyvault late
|
/hold This lacks tagging and cleanup for vault resources. We can't merge without tagging and cleanup. |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 28 out of 30 changed files in this pull request and generated 5 comments.
Files not reviewed (1)
- internal/api/zz_generated.deepcopy.go: Generated file
Comments suppressed due to low confidence (1)
internal/api/types_systemadmincredential.go:34
- This new Cosmos-persisted type is missing the repo’s required field-level
// Written by:annotations (see e.g. internal/api/types_cluster.go:33+). Also, docs/cosmos-data-flow.md currently has no entry for SystemAdminCredentialRequest reads/writes, but this PR adds new Cosmos CRUD paths (backend controller write + frontend read). Please add the writer annotations and update docs/cosmos-data-flow.md accordingly.
// SystemAdminCredentialRequest represents a temporary admin credential request
// tracked in Cosmos, replacing the cluster-service break-glass credential flow.
//
// Lifecycle is tracked via metav1.Conditions rather than an explicit Phase enum
// so that individual aspects of the request (issuance, revocation, cleanup) can
// progress independently and callers can reason about each concern separately.
//
// +k8s:deepcopy-gen=true
type SystemAdminCredentialRequest struct {
CosmosMetadata `json:"cosmosMetadata"`
Spec SystemAdminCredentialRequestSpec `json:"spec"`
Status SystemAdminCredentialRequestStatus `json:"status"`
}
| _, err = credCRUD.Create(ctx, cred, nil) | ||
| if err != nil { | ||
| if database.IsConflictError(err) { | ||
| return credResourceID, nil | ||
| } | ||
| return nil, utils.TrackError(fmt.Errorf("failed to create SystemAdminCredentialRequest: %w", err)) | ||
| } |
| // PublicKeyPEM is the public half of the keypair, PEM-encoded. | ||
| PublicKeyPEM string `json:"publicKeyPEM"` | ||
| // PrivateKeyPEM is the private half of the keypair, PEM-encoded. | ||
| // Treat as a secret in logs, dumps, and telemetry. | ||
| PrivateKeyPEM string `json:"privateKeyPEM"` |
| // Kubeconfig is a pre-assembled kubeconfig string, set when the credential | ||
| // was produced by the cluster-service break-glass flow rather than the | ||
| // native CSR flow. When non-empty, the frontend returns it directly | ||
| // instead of calling BuildKubeconfig. | ||
| Kubeconfig string `json:"kubeconfig,omitempty"` |
| // The resulting kubeconfig deliberately carries no CertificateAuthorityData, so | ||
| // callers must fall back to their system trust bundle to verify the API server. | ||
| func BuildKubeconfig(signedCertificateBase64, privateKeyPEM, apiURL string) ([]byte, error) { | ||
| certPEM, err := base64.StdEncoding.DecodeString(signedCertificateBase64) | ||
| if err != nil { | ||
| return nil, fmt.Errorf("failed to decode signed certificate: %w", err) | ||
| } | ||
|
|
||
| config := clientcmdapi.NewConfig() | ||
| config.Clusters[kubeconfigClusterName] = &clientcmdapi.Cluster{ | ||
| Server: apiURL, | ||
| CertificateAuthorityData: nil, | ||
| } |
| credLogger.Info("deleting expired SystemAdminCredentialRequest") | ||
|
|
||
| if err := credCRUD.Delete(ctx, cred.ResourceID.Name); err != nil { | ||
| credLogger.Error(err, "unable to delete expired credential") | ||
| errs = append(errs, utils.TrackError(fmt.Errorf("deleting credential %s: %w", cred.ResourceID.String(), err))) | ||
| } |
| } | ||
|
|
||
| keyVaultSecretClient, err := opsync.keyVaultSecretClientFactory.KeyVaultSecretClient( | ||
| managementCluster.Status.HostedClustersSecretsKeyVaultManagedIdentityClientID, |
There was a problem hiding this comment.
are you allowed to do this? I was expecting to see the backend MSI given Azure RBAC permissions over those keyvaults. if there's no link between the backend pod/MSI and this other client ID, I don't think you'll be allowed to mint a token
There was a problem hiding this comment.
Here we shouldn't be using keyVaultSecretClientFactory. We should be using BackendIdentityClientsBuilder and then get a keyvault secrets client from it. That will ensure that the Backend's identity is what's being used, which is what we want.
|
@deads2k: The following tests failed, say
Full PR test history. Your PR dashboard. DetailsInstructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. I understand the commands that are listed here. |
| } | ||
|
|
||
| backendIdentityAzureClients, err := app.NewBackendIdentityAzureClients(ctx, azureConfig) | ||
| backendIdentityClientBuilder, err := app.NewBackendIdentityClientBuilder(ctx, azureConfig) |
There was a problem hiding this comment.
That seems to have changed the behavior of existing backend identity clients:
Before this PR we had some that were instantiated at startup time (like the blob one, because we always use that one instance), and now we have some that are instantiated on demand (as the keyvaults are different per mgmt cluster). This has changed and we are not instantiating at startup time the blob and the role one. We should have both.
| if err != nil { | ||
| return nil, utils.TrackError(fmt.Errorf("failed to create dataplane identities OIDC configuration blob storage client: %w", err)) | ||
| } | ||
| azureConfig.CloudEnvironment.AZCoreClientOptions(), |
There was a problem hiding this comment.
It is redundant to pass both AZCoreClientOptions and ARMClientOptions.
ARMClientOptions should be enough. Within it it has an attribute policy.ClientOptions which is essentially AZCoreClientOptions (they are type alias)
| "github.com/Azure/azure-sdk-for-go/sdk/security/keyvault/azsecrets" | ||
| ) | ||
|
|
||
| type KeyVaultSecretClient interface { |
There was a problem hiding this comment.
nitpick: Secret -> Secrets. The https://pkg.go.dev/github.com/Azure/azure-sdk-for-go/sdk/security/keyvault/azsecrets doc refers to it as "Azure Key Vault Secrets client module for Go" (plural)
| SMIClientBuilder: smiClientBuilder, | ||
| CheckAccessV2ClientBuilder: checkAccessV2ClientBuilder, | ||
| ClusterScopedIdentitiesConfig: clusterScopedIdentitiesConfig, | ||
| KeyVaultSecretClientFactory: azureclient.NewManagedIdentityKeyVaultSecretClientFactory(azCoreClientOptions), |
There was a problem hiding this comment.
Why do we need KeyVaultSecretClientFactory in backend? We should be able to use BackendIdentityClientBuilder which is what we want when we want to use the backend identity. It uses the default azure credential chain, which appropriately selects the backend identity
| } | ||
|
|
||
| func (f *managedIdentityKeyVaultSecretClientFactory) KeyVaultSecretClient(managedIdentityClientID string, vaultURL string) (KeyVaultSecretReader, error) { | ||
| cred, err := azidentity.NewManagedIdentityCredential(&azidentity.ManagedIdentityCredentialOptions{ |
There was a problem hiding this comment.
If the intention of this factory is to use the pod's identity, in this case the frontend identity, we should use the default azure credential https://learn.microsoft.com/en-us/azure/developer/go/sdk/authentication/credential-chains#defaultazurecredential-overview. We want to end up leveraging the pod's workload identity. (It also has the nice bonus where you can technically run it using other authentication methods transparently in other environments like local dev, ...)
There was a problem hiding this comment.
At this point, I would just have the concept of a FrontendIdentityClientsBuilder similar to backend. That builder would be instantiated containing the defaultazurecredentials similar to what it's being done in backend
| return "", utils.TrackError(fmt.Errorf("failed to create Key Vault secret client: %w", err)) | ||
| } | ||
|
|
||
| resp, err := keyVaultSecretClient.GetSecret(ctx, secretName, "", nil) |
There was a problem hiding this comment.
Does the Frontend identity have enough permissions to perform the desired actions in the desired key vault? If we are going to go to that route we need to make sure we do, in all aro-hcp environments
| return "", utils.TrackError(fmt.Errorf("failed to create Key Vault secret client: %w", err)) | ||
| } | ||
|
|
||
| resp, err := keyVaultSecretClient.GetSecret(ctx, secretName, "", nil) |
There was a problem hiding this comment.
To be aware of, this means that now the frontend for this endpoint on admincredentials operationresult now has more DB interactions (to figure out the vault url) as well as Azure API interaction for keyvault interaction, which adds latency
| } | ||
|
|
||
| func keyVaultSecretNameForCredential(credResourceID *azcorearm.ResourceID) string { | ||
| hash := sha256.Sum256([]byte(strings.ToLower(credResourceID.String()))) |
There was a problem hiding this comment.
Is there some reason we use the sha of the credResourceID? hashing works but it will make discoverability harder just from the name. I see that we are setting tags in the secret which would help but that's a bit more jumps to find it out.
Have we considered having a fixed prefix that signals that it's an admin credential + the resource name which is the operation id which is a uuid with fixed length?
something like system-admin-credential-kubeconfig-<operation_uuid> (35+36 = 71 chars)
As a note, the maximum number of characters for a key vault secret name is 127 characters (with alphanumeric characters and hyphens) https://learn.microsoft.com/en-us/azure/azure-resource-manager/management/resource-name-rules#microsoftkeyvault
| return credResourceID, nil | ||
| } | ||
|
|
||
| func keyVaultSecretNameForCredential(credResourceID *azcorearm.ResourceID) string { |
There was a problem hiding this comment.
Let's add a fixed length prefix to indicate just from the name that it's about the kubeconfig of a cluster's admin-credentials. For example:
system-admin-credential-kubeconfig-
This helps identify quickly just from the name that it's about that concept. That keyvault also contains other secrets that are not related to system admin credentials kubeconfig so this also allows to differentiate them from those others just from the name
| return nil, utils.TrackError(fmt.Errorf("failed to create Key Vault secret client: %w", err)) | ||
| } | ||
|
|
||
| _, err = keyVaultSecretClient.SetSecret(ctx, keyVaultSecretName, azsecrets.SetSecretParameters{ |
There was a problem hiding this comment.
In Azure some API interactions are asynchronous. Do we know if SetSecret guarantees that once the call is returned the secret is there? I am guessing that yes because the result is the secret response and when it's asynchronous it's usually some kind of poller that is returned as well as the method name is usually called Begin*.
Raising awareness about the different kind of behaviors as I think it's useful to be aware of it.
| return nil, utils.TrackError(fmt.Errorf("failed to build credential request resource ID: %w", err)) | ||
| } | ||
|
|
||
| serviceProviderCluster, err := opsync.resourcesDBClient.ServiceProviderClusters( |
There was a problem hiding this comment.
Would using SPC lister be more appropriate here? or are we concerned about caching and it being outdated?
| // progress independently and callers can reason about each concern separately. | ||
| // | ||
| // +k8s:deepcopy-gen=true | ||
| type SystemAdminCredentialRequest struct { |
There was a problem hiding this comment.
What's the reasoning of containing the "System" and the "Request" words?
There was a problem hiding this comment.
Would having the word "Cluster" as part of the name make sense? or do we think it's redundant because we will ever have cluster ones and it's implied?
| return nil, nil | ||
| } | ||
|
|
||
| kvClient, err := c.keyVaultSecretClientFactory.KeyVaultSecretClient( |
There was a problem hiding this comment.
Similar to https://github.com/Azure/ARO-HCP/pull/6196/changes#r3633971629, here we shouldn't be using keyVaultSecretClientFactory. We should be using BackendIdentityClientsBuilder
| func (c *clusterChildResourcesCleanupController) ensureKeyVaultSecretsDeleted(ctx context.Context, clusterResourceID *azcorearm.ResourceID) error { | ||
| logger := utils.LoggerFromContext(ctx) | ||
|
|
||
| spc, err := c.resourcesDBClient.ServiceProviderClusters(clusterResourceID.SubscriptionID, clusterResourceID.ResourceGroupName, clusterResourceID.Name).Get(ctx, api.ServiceProviderClusterResourceName) |
There was a problem hiding this comment.
Is usage of direct Cosmos intentional for SPC, or could we leverage lister? or are we worried about caching content being outdated here?
| return nil | ||
| } | ||
|
|
||
| pager := kvClient.NewListSecretPropertiesPager(nil) |
There was a problem hiding this comment.
Instead of listing, which could have a non small amount of elements, would retrieving the admincredentials from cosmos, calculating the name for each one of them and then doing a delete would be an alternative?
| } | ||
| secretName := secret.ID.Name() | ||
| logger.Info("deleting Key Vault secret for cluster", "secretName", secretName) | ||
| if _, err := kvClient.DeleteSecret(ctx, secretName, nil); err != nil { |
There was a problem hiding this comment.
Does DeleteSecret perform a soft or hard delete? I am wondering what are the consequences if it's a soft one
There was a problem hiding this comment.
Also wondering about this because if I recall correctly trying to create a secret with the same name as one that is still soft deleted would fail with a conflict
| return nil | ||
| } | ||
|
|
||
| func (c *clusterChildResourcesCleanupController) countKeyVaultSecretsForCluster(ctx context.Context, spc *api.ServiceProviderCluster, serviceProviderClusterResourceID *azcorearm.ResourceID) (int, error) { |
There was a problem hiding this comment.
nitpick: this doesn't count all the secrets in the keyvault, just a subset of some. We should be clear in the name of the method about this
| return true, nil | ||
| } | ||
|
|
||
| remaining, err := c.countKeyVaultSecretsForCluster(ctx, spc, serviceProviderClusterResourceID) |
There was a problem hiding this comment.
Is this logic needed? If ensureKeyVaultSecretsDeleted succeeds, doesn't that mean we've eliminated all of them?
|
|
||
| type emptyKeyVaultSecretClient struct{} | ||
|
|
||
| func (e *emptyKeyVaultSecretClient) GetSecret(_ context.Context, _ string, _ string, _ *azsecrets.GetSecretOptions) (azsecrets.GetSecretResponse, error) { |
There was a problem hiding this comment.
Have we evaluated using mockgen for this? It will also allow unit tests in general to set the desired inputs/outputs. We do that with other azure clients already like for example RoleDefinitionsClient (see the file for the mockgen marker)
| cluster.ID.Name, | ||
| ) | ||
|
|
||
| credIterator, err := credCRUD.List(ctx, nil) |
There was a problem hiding this comment.
Is there some reason we want to use the DB directly instead of a lister in this context?
| @@ -255,10 +255,17 @@ func (opts *FrontendOpts) Run() error { | |||
| utils.TracerName, | |||
| ) | |||
|
|
|||
There was a problem hiding this comment.
To improve at some point: on frontend right now we hardcode the azure cloud type to public. Once we need to support more clouds that will fall short. In backend we already make that configurable (part of the azureconfig file that it receives). Here we will want to do something similar (at the least a cli flag with the value)
| } | ||
| kubeconfigBytes, err := systemadmincredential.BuildKubeconfig( | ||
| cred.Status.SignedCertificate, | ||
| cred.Spec.PrivateKeyPEM, |
There was a problem hiding this comment.
Even though we store kubeconfig in key vault, PrivateKeyPEM would still need to be available somehow
…mos on issuance, add 48h GC
Brings in SystemAdminCredentialRequest API type and CRUD from cs-198-admincreds, plus BuildKubeconfig in internal/systemadmincredential. When the break-glass credential is issued by cluster service, operation_request_credential now creates a SystemAdminCredentialRequest in Cosmos (name = operationID) and updates the operation's InternalID to the ARM resource ID. The frontend reads the credential back from Cosmos via assembleAdminCredentialFromCosmos.
A new CredentialGC controller runs every hour, iterates all clusters per subscription, and deletes any SystemAdminCredentialRequest older than 48 hours.
Alternative to a larger port. This would try to simply copy admin credentials to the API we eventually want and use that for fast retrieval. It is a stepping stone to #6124 which is a stepping stone to a new retrieval API that requires client-side integration to build the credential itself.