Skip to content
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.

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

CCO-372: Azure Workload Identity info in CredsRequests creates a Secret #587

Merged
merged 1 commit into from Nov 29, 2023
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
4 changes: 4 additions & 0 deletions Makefile
Expand Up @@ -137,6 +137,10 @@ test-e2e-sts:
go test -mod=vendor -race -tags e2e ./test/e2e/aws/sts/...
.PHONY: test-e2e-sts

test-e2e-azident:
go test -mod=vendor -race -tags e2e ./test/e2e/azure/azident/...
.PHONY: test-e2e-azident

vet: verify-govet
.PHONY: vet

Expand Down
18 changes: 18 additions & 0 deletions pkg/apis/cloudcredential/v1/types_azure.go
Expand Up @@ -47,6 +47,24 @@ type AzureProviderSpec struct {
// and RoleBindings.
// +optional
DataPermissions []string `json:"dataPermissions,omitempty"`

// The following fields are only required for Azure Workload Identity.
// AzureClientID is the ID of the specific application you created in Azure
// +optional
AzureClientID string `json:"azureClientID,omitempty"`

// AzureRegion is the geographic region of the Azure service.
// +optional
AzureRegion string `json:"azureRegion,omitempty"`

// Each Azure subscription has an ID associated with it, as does the tenant to which a subscription belongs.
// AzureSubscriptionID is the ID of the subscription.
// +optional
AzureSubscriptionID string `json:"azureSubscriptionID,omitempty"`

// AzureTenantID is the ID of the tenant to which the subscription belongs.
// +optional
AzureTenantID string `json:"azureTenantID,omitempty"`
}

// RoleBinding models part of the Azure RBAC Role Binding
Expand Down
155 changes: 127 additions & 28 deletions pkg/azure/actuator.go
Expand Up @@ -36,6 +36,7 @@ import (
operatorv1 "github.com/openshift/api/operator/v1"

minterv1 "github.com/openshift/cloud-credential-operator/pkg/apis/cloudcredential/v1"
"github.com/openshift/cloud-credential-operator/pkg/cmd/provisioning"
"github.com/openshift/cloud-credential-operator/pkg/operator/constants"
actuatoriface "github.com/openshift/cloud-credential-operator/pkg/operator/credentialsrequest/actuator"
"github.com/openshift/cloud-credential-operator/pkg/operator/utils"
Expand Down Expand Up @@ -68,20 +69,6 @@ func NewFakeActuator(c, rootCredClient client.Client,
}
}

func (a *Actuator) IsValidMode() error {
mode, err := a.client.Mode(context.Background())
if err != nil {
return err
}

switch mode {
case constants.PassthroughAnnotation:
return nil
default:
return errors.New("invalid mode")
}
}

func isAzureCredentials(providerSpec *runtime.RawExtension) (bool, error) {
var err error
unknown := runtime.Unknown{}
Expand All @@ -108,6 +95,19 @@ func (a *Actuator) needsUpdate(ctx context.Context, cr *minterv1.CredentialsRequ
if !exists {
return true, nil
}

// Manual mode just update
credentialsMode, _, err := utils.GetOperatorConfiguration(a.client, logger)
if err != nil {
logger.WithError(err).Error("error loading CCO configuration to determine valid mode")
return true, err
}
if credentialsMode == operatorv1.CloudCredentialsModeManual {
return true, nil
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Note to reviewers: what's in the credReq should be the source of truth propagated down to the secret.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ideally, we would actually check if the secret differs from the source of truth similar to AWS mint mode. However, this can be taken away as tech debt.

}
if credentialsMode == operatorv1.CloudCredentialsModeMint {
return false, errors.New("mint mode is invalid")
}
// passthrough-specifc checks here (now the only kind of checks...)

credentialsRootSecret, err := a.GetCredentialsRootSecret(ctx, cr)
Expand Down Expand Up @@ -160,11 +160,17 @@ func (a *Actuator) Delete(ctx context.Context, cr *minterv1.CredentialsRequest)
if isAzure, err := isAzureCredentials(cr.Spec.ProviderSpec); !isAzure {
return err
}
if err := a.IsValidMode(); err != nil {
logger := a.getLogger(cr)
credentialsMode, _, err := utils.GetOperatorConfiguration(a.client, logger)
if err != nil {
logger.WithError(err).Error("error loading CCO configuration to determine valid mode")
return err
}
if credentialsMode == operatorv1.CloudCredentialsModeManual {
logger.Debug("running delete in manual mode")
return nil
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Note to reviewers: the delete works because of the annotation on the secret

}

logger := a.getLogger(cr)
logger.Debug("running delete")

credentialsRootSecret, err := a.GetCredentialsRootSecret(ctx, cr)
Expand Down Expand Up @@ -230,6 +236,52 @@ func (a *Actuator) sync(ctx context.Context, cr *minterv1.CredentialsRequest) er
logger.Debug("credentials already up to date")
return nil
}
stsDetected, err := utils.IsTimedTokenCluster(a.client, ctx, logger)

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit on sts - maybe tokenServiceDetected?

if err != nil {
return err
}
if stsDetected {
logger.Debug("actuator detected Azure AD Workload Identity enabled cluster, enabling Workload Identity secret brokering for CredentialsRequests providing a Managed Identity")
azureProviderSpec, err := decodeProviderSpec(minterv1.Codec, cr)
if err != nil {
return err
}
azureFederatedTokenFile := cr.Spec.CloudTokenPath
if cr.Spec.CloudTokenPath == "" {
logger.Debugf("CredentialsRequest has no cloudTokenPath, defaulting azure_federated_token_file to %s", provisioning.OidcTokenPath)
azureFederatedTokenFile = provisioning.OidcTokenPath
}
// Check for old Manual Mode where all 4 fields are empty - defaulting to old behavior
// where CCO exists and the secret is created manually
if azureProviderSpec.AzureClientID == "" && azureProviderSpec.AzureTenantID == "" && azureProviderSpec.AzureSubscriptionID == "" && azureProviderSpec.AzureRegion == "" {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Note to reviewers: if none of the fields are provided, we assume the creator of the credReq is intending to use the old manual mode where the secrets are created by the ccoctl tool and CCO doesn't do anything (as is the case with openshift operators today).

return nil
}
err = validateAzureProviderSpec(*azureProviderSpec)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Note to reviewers: if some but not all fields required for this new manual mode workload identity workflow are set then we should error

if err != nil {
// At least one of the fields was set indicating that the new workload identity
// behavior of creating the secret is desired but not all fields required were
// provided.
msg := "error validating credentials request Azure AD Workload Identity fields"
return &actuatoriface.ActuatorError{
ErrReason: minterv1.CredentialsProvisionFailure,
Message: fmt.Sprintf("%v: %v", msg, err),
}
}
desiredSecret := &corev1.Secret{
ObjectMeta: metav1.ObjectMeta{
Name: cr.Spec.SecretRef.Name,
Namespace: cr.Spec.SecretRef.Namespace,
},
StringData: map[string]string{
AzureClientID: azureProviderSpec.AzureClientID,
AzureTenantID: azureProviderSpec.AzureTenantID,
AzureRegion: azureProviderSpec.AzureRegion,
AzureSubscriptionID: azureProviderSpec.AzureSubscriptionID,
AzureFederatedTokenFile: azureFederatedTokenFile,
},
}
return a.syncCredentialSecrets(ctx, cr, desiredSecret, logger)
}

credentialsRootSecret, err := a.GetCredentialsRootSecret(ctx, cr)
if err != nil {
Expand Down Expand Up @@ -259,7 +311,6 @@ func (a *Actuator) sync(ctx context.Context, cr *minterv1.CredentialsRequest) er
Message: msg,
}
}

return nil
}

Expand Down Expand Up @@ -370,7 +421,25 @@ func (a *Actuator) cleanupAfterPassthroughPivot(ctx context.Context, cr *minterv

return nil
}
func (a *Actuator) syncCredentialSecrets(ctx context.Context, cr *minterv1.CredentialsRequest, cloudCredsSecret *corev1.Secret, logger log.FieldLogger) error {

// For Azure Workload Identity, the generated Secret needs to look like this:
/*
apiVersion: v1
stringData:
azure_client_id: 0420bfd1-ab26-4b80-a9ac-deadbeeff1f9
azure_tenant_id: 6047c7e9-b2ad-488d-a54e-deadbeefa7ee
azure_region: centralus
azure_subscription_id: 8c20ec23-8478-4f46-96f4-deadbeeff185
azure_federated_token_file: /var/run/secrets/openshift/serviceaccount/token
kind: Secret
metadata:
name: azure-cloud-credentials
namespace: openshift-machine-api
type: Opaque
*/
// The first 4 fields need to come from `spec.ProviderSpec` in the CredentialsRequest with
// spec.cloudTokenPath matching up to: `azure_federated_token_file`
func (a *Actuator) syncCredentialSecrets(ctx context.Context, cr *minterv1.CredentialsRequest, desiredSecret *corev1.Secret, logger log.FieldLogger) error {
sLog := logger.WithFields(log.Fields{
"targetSecret": fmt.Sprintf("%s/%s", cr.Spec.SecretRef.Namespace, cr.Spec.SecretRef.Name),
"cr": fmt.Sprintf("%s/%s", cr.Namespace, cr.Name),
Expand All @@ -391,16 +460,24 @@ func (a *Actuator) syncCredentialSecrets(ctx context.Context, cr *minterv1.Crede
secret.Annotations = map[string]string{}
}
secret.Annotations[minterv1.AnnotationCredentialsRequest] = fmt.Sprintf("%s/%s", cr.Namespace, cr.Name)
if desiredSecret.Data == nil {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Note to reviewers: this if statement checks that the desiredSecret is from the new manual mode workload identity workflow since it does not set the Data field

if secret.StringData == nil {
secret.StringData = map[string]string{}
}
secret.StringData = desiredSecret.StringData
secret.Type = corev1.SecretTypeOpaque
return nil
}
if secret.Data == nil {
secret.Data = map[string][]byte{}
}
secret.Data[AzureClientID] = cloudCredsSecret.Data[AzureClientID]
secret.Data[AzureClientSecret] = cloudCredsSecret.Data[AzureClientSecret]
secret.Data[AzureRegion] = cloudCredsSecret.Data[AzureRegion]
secret.Data[AzureResourceGroup] = cloudCredsSecret.Data[AzureResourceGroup]
secret.Data[AzureResourcePrefix] = cloudCredsSecret.Data[AzureResourcePrefix]
secret.Data[AzureSubscriptionID] = cloudCredsSecret.Data[AzureSubscriptionID]
secret.Data[AzureTenantID] = cloudCredsSecret.Data[AzureTenantID]
secret.Data[AzureClientID] = desiredSecret.Data[AzureClientID]
secret.Data[AzureClientSecret] = desiredSecret.Data[AzureClientSecret]
secret.Data[AzureRegion] = desiredSecret.Data[AzureRegion]
secret.Data[AzureResourceGroup] = desiredSecret.Data[AzureResourceGroup]
secret.Data[AzureResourcePrefix] = desiredSecret.Data[AzureResourcePrefix]
secret.Data[AzureSubscriptionID] = desiredSecret.Data[AzureSubscriptionID]
secret.Data[AzureTenantID] = desiredSecret.Data[AzureTenantID]
return nil
})
sLog.WithField("operation", op).Info("processed secret")
Expand Down Expand Up @@ -463,9 +540,6 @@ func (a *Actuator) Exists(ctx context.Context, cr *minterv1.CredentialsRequest)
if isAzure, err := isAzureCredentials(cr.Spec.ProviderSpec); !isAzure {
return false, err
}
if err := a.IsValidMode(); err != nil {
return false, err
}

existingSecret := &corev1.Secret{}
err := a.client.Get(ctx, types.NamespacedName{Namespace: cr.Spec.SecretRef.Namespace, Name: cr.Spec.SecretRef.Name}, existingSecret)
Expand All @@ -492,3 +566,28 @@ func (a *Actuator) getLogger(cr *minterv1.CredentialsRequest) log.FieldLogger {
func (a *Actuator) Upgradeable(mode operatorv1.CloudCredentialsMode) *configv1.ClusterOperatorStatusCondition {
return utils.UpgradeableCheck(a.client.RootCredClient, mode, a.GetCredentialsRootSecretLocation())
}

func validateAzureProviderSpec(azureProviderSpec minterv1.AzureProviderSpec) error {
var errors []error
isEmptyAzureClientID := azureProviderSpec.AzureClientID == ""
isEmptyAzureTenantID := azureProviderSpec.AzureTenantID == ""
isEmptyAzureSubscriptionID := azureProviderSpec.AzureSubscriptionID == ""
isEmptyAzureRegion := azureProviderSpec.AzureRegion == ""

if isEmptyAzureClientID {
errors = append(errors, fmt.Errorf("AzureClientID must not be empty"))
}
if isEmptyAzureTenantID {
errors = append(errors, fmt.Errorf("AzureTenantID must not be empty"))
}
if isEmptyAzureRegion {
errors = append(errors, fmt.Errorf("AzureRegion must not be empty"))
}
if isEmptyAzureSubscriptionID {
errors = append(errors, fmt.Errorf("AzureSubscriptionID must not be empty"))
}
if len(errors) > 0 {
return fmt.Errorf("AzureProviderSpec validation failed: %v", errors)
}
return nil
}