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

Replace Azure Service Principal auth with Azure Workload Identity auth in functional tests #7787

Merged
merged 21 commits into from
Aug 13, 2024
Merged
20 changes: 13 additions & 7 deletions .github/workflows/functional-test-cloud.yaml
Original file line number Diff line number Diff line change
Expand Up @@ -63,7 +63,7 @@ env:
# Azure Keyvault CSI driver chart version
AZURE_KEYVAULT_CSI_DRIVER_VER: "1.4.2"
# Azure workload identity webhook chart version
AZURE_WORKLOAD_IDENTITY_WEBHOOK_VER: "1.1.0"
AZURE_WORKLOAD_IDENTITY_WEBHOOK_VER: "1.3.0"
Copy link
Contributor Author

Choose a reason for hiding this comment

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

just bumping to latest

# Container registry for storing container images
CONTAINER_REGISTRY: ghcr.io/radius-project/dev
# Container registry for storing Bicep recipe artifacts
Expand Down Expand Up @@ -97,7 +97,7 @@ jobs:
build:
name: Build Radius for test
runs-on: ubuntu-latest
if: github.event_name == 'repository_dispatch' || (github.event_name == 'schedule' && github.repository == 'radius-project/radius') || github.event_name == 'workflow_run'
if: github.event_name == 'repository_dispatch' || (github.event_name == 'schedule' && github.repository == 'radius-project/radius') || github.event_name == 'workflow_run' || github.event_name == 'workflow_dispatch'
Copy link
Contributor Author

Choose a reason for hiding this comment

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

workflow_dispatch won't work yet unless you set up the federated credential to allow your branch. for testing this is what I did. in the future we should switch to github environments, but for now it doesn't hurt to have this.

env:
DE_IMAGE: "ghcr.io/radius-project/deployment-engine"
DE_TAG: "latest"
Expand All @@ -119,7 +119,13 @@ jobs:
private_key: ${{ secrets.FUNCTIONAL_TEST_APP_PRIVATE_KEY }}

- name: Set up checkout target (scheduled)
if: github.event_name == 'schedule' || github.event_name == 'repository_dispatch'
if: github.event_name == 'schedule'
run: |
echo "CHECKOUT_REPO=${{ github.repository }}" >> $GITHUB_ENV
echo "CHECKOUT_REF=refs/heads/main" >> $GITHUB_ENV

- name: Set up checkout target (repository_dispatch)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

separated for clarity

if: github.event_name == 'repository_dispatch'
run: |
echo "CHECKOUT_REPO=${{ github.repository }}" >> $GITHUB_ENV
echo "CHECKOUT_REF=refs/heads/main" >> $GITHUB_ENV
Expand Down Expand Up @@ -412,7 +418,7 @@ jobs:
tests:
name: Run ${{ matrix.name }} functional tests
needs: [build]
if: github.event_name == 'repository_dispatch' || (github.event_name == 'schedule' && github.repository == 'radius-project/radius') || github.event_name == 'workflow_run'
if: github.event_name == 'repository_dispatch' || (github.event_name == 'schedule' && github.repository == 'radius-project/radius') || github.event_name == 'workflow_run' || github.event_name == 'workflow_dispatch'
strategy:
fail-fast: true
matrix:
Expand Down Expand Up @@ -616,7 +622,7 @@ jobs:
echo "*** Installing Radius to Kubernetes ***"
rad install kubernetes \
--chart ${{ env.RADIUS_CHART_LOCATION }} \
--set rp.image=${{ env.CONTAINER_REGISTRY }}/applications-rp,rp.tag=${{ env.REL_VERSION }},controller.image=${{ env.CONTAINER_REGISTRY }}/controller,controller.tag=${{ env.REL_VERSION }},ucp.image=${{ env.CONTAINER_REGISTRY }}/ucpd,ucp.tag=${{ env.REL_VERSION }},de.image=${{ env.DE_IMAGE }},de.tag=${{ env.DE_TAG }}
--set rp.image=${{ env.CONTAINER_REGISTRY }}/applications-rp,rp.tag=${{ env.REL_VERSION }},controller.image=${{ env.CONTAINER_REGISTRY }}/controller,controller.tag=${{ env.REL_VERSION }},ucp.image=${{ env.CONTAINER_REGISTRY }}/ucpd,ucp.tag=${{ env.REL_VERSION }},de.image=${{ env.DE_IMAGE }},de.tag=${{ env.DE_TAG }},global.azureWorkloadIdentity.enabled=true

echo "*** Create workspace, group and environment for test ***"
rad workspace create kubernetes
Expand All @@ -630,8 +636,8 @@ jobs:
echo "*** Configuring Azure provider ***"
rad env update kind-radius --azure-subscription-id ${{ secrets.AZURE_SUBSCRIPTIONID_TESTS }} \
--azure-resource-group ${{ env.AZURE_TEST_RESOURCE_GROUP }}
rad credential register azure sp --client-id ${{ secrets.AZURE_SP_TESTS_APPID }} \
--client-secret ${{ secrets.INTEGRATION_TEST_SP_PASSWORD }} \
rad credential register azure wi \
Copy link
Contributor Author

Choose a reason for hiding this comment

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

here's the important change

--client-id ${{ secrets.AZURE_SP_TESTS_APPID }} \
--tenant-id ${{ secrets.AZURE_SP_TESTS_TENANTID }}

echo "*** Configuring AWS provider ***"
Expand Down
18 changes: 15 additions & 3 deletions pkg/azure/credential/ucpcredentials.go
Original file line number Diff line number Diff line change
Expand Up @@ -49,6 +49,9 @@ type UCPCredentialOptions struct {

// ClientOptions is the options for azure client.
ClientOptions *azcore.ClientOptions

// TokenFilePath is the path to the azure token file (for use with Azure workload identity)
TokenFilePath string
Copy link
Contributor Author

Choose a reason for hiding this comment

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

added this to make this file unit testable. the azure sdk library will use the environment variable in the real world

}

// UCPCredential authenticates service principal using UCP credential APIs.
Expand Down Expand Up @@ -171,14 +174,23 @@ func refreshAzureWorkloadIdentityCredentials(ctx context.Context, c *UCPCredenti

logger.Info("Retrieved Azure Credential - ClientID: " + azureWorkloadIdentityCredential.ClientID)

var opt *azidentity.DefaultAzureCredentialOptions
var opt *azidentity.WorkloadIdentityCredentialOptions
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I misunderstood what this code did on the first workload identity PR. this should have been WorkloadIdentityCredential instead.

if c.options.ClientOptions != nil {
opt = &azidentity.DefaultAzureCredentialOptions{
opt = &azidentity.WorkloadIdentityCredentialOptions{
ClientID: azureWorkloadIdentityCredential.ClientID,
TenantID: azureWorkloadIdentityCredential.TenantID,
TokenFilePath: c.options.TokenFilePath,
ClientOptions: *c.options.ClientOptions,
}
} else {
opt = &azidentity.WorkloadIdentityCredentialOptions{
TokenFilePath: c.options.TokenFilePath,
ClientID: azureWorkloadIdentityCredential.ClientID,
TenantID: azureWorkloadIdentityCredential.TenantID,
}
}

azCred, err := azidentity.NewDefaultAzureCredential(opt)
azCred, err := azidentity.NewWorkloadIdentityCredential(opt)
if err != nil {
return err
}
Expand Down
24 changes: 12 additions & 12 deletions pkg/azure/credential/ucpcredentials_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -43,8 +43,8 @@ func newServicePrincipalMockProvider() *mockProvider {
fakeCredential: &sdk_cred.AzureCredential{
Kind: sdk_cred.AzureServicePrincipalCredentialKind,
ServicePrincipal: &sdk_cred.AzureServicePrincipalCredential{
ClientID: "fakeid",
TenantID: "fakeid",
ClientID: "fakeClientID",
TenantID: "fakeTenantID",
ClientSecret: "fakeSecret",
},
},
Expand All @@ -56,8 +56,8 @@ func newWorkloadIdentityMockProvider() *mockProvider {
fakeCredential: &sdk_cred.AzureCredential{
Kind: sdk_cred.AzureWorkloadIdentityCredentialKind,
WorkloadIdentity: &sdk_cred.AzureWorkloadIdentityCredential{
ClientID: "fakeid",
TenantID: "fakeid",
ClientID: "fakeClientID",
TenantID: "fakeTenantID",
},
},
}
Expand All @@ -84,7 +84,7 @@ func Test_NewUCPCredential_WorkloadIdentity(t *testing.T) {
}

func Test_RefreshCredentials_ServicePrincipal(t *testing.T) {
t.Run("invalid credential", func(t *testing.T) {
t.Run("invalid service principal credential", func(t *testing.T) {
p := newServicePrincipalMockProvider()
c, err := NewUCPCredential(UCPCredentialOptions{Provider: p})
require.NoError(t, err)
Expand All @@ -94,7 +94,7 @@ func Test_RefreshCredentials_ServicePrincipal(t *testing.T) {
require.Error(t, err)
})

t.Run("do not refresh credential", func(t *testing.T) {
t.Run("do not refresh service principal credential", func(t *testing.T) {
p := newServicePrincipalMockProvider()
c, err := NewUCPCredential(UCPCredentialOptions{Provider: p})
require.NoError(t, err)
Expand All @@ -104,7 +104,7 @@ func Test_RefreshCredentials_ServicePrincipal(t *testing.T) {
require.False(t, c.isExpired())
})

t.Run("same credentials", func(t *testing.T) {
t.Run("same service principal credentials", func(t *testing.T) {
p := newServicePrincipalMockProvider()
c, err := NewUCPCredential(UCPCredentialOptions{Provider: p})
require.NoError(t, err)
Expand All @@ -125,7 +125,7 @@ func Test_RefreshCredentials_ServicePrincipal(t *testing.T) {
}

func Test_RefreshCredentials_WorkloadIdentity(t *testing.T) {
t.Run("invalid credential", func(t *testing.T) {
t.Run("invalid workload identity credential", func(t *testing.T) {
p := newWorkloadIdentityMockProvider()
c, err := NewUCPCredential(UCPCredentialOptions{Provider: p})
require.NoError(t, err)
Expand All @@ -135,19 +135,19 @@ func Test_RefreshCredentials_WorkloadIdentity(t *testing.T) {
require.Error(t, err)
})

t.Run("do not refresh credential", func(t *testing.T) {
t.Run("do not refresh workload identity credential", func(t *testing.T) {
p := newWorkloadIdentityMockProvider()
c, err := NewUCPCredential(UCPCredentialOptions{Provider: p})
c, err := NewUCPCredential(UCPCredentialOptions{Provider: p, TokenFilePath: "/var/run/secrets/azure/tokens/azure-identity-token"})
require.NoError(t, err)

err = c.refreshCredentials(context.TODO())
require.NoError(t, err)
require.False(t, c.isExpired())
})

t.Run("same credentials", func(t *testing.T) {
t.Run("same workload identity credentials", func(t *testing.T) {
p := newWorkloadIdentityMockProvider()
c, err := NewUCPCredential(UCPCredentialOptions{Provider: p})
c, err := NewUCPCredential(UCPCredentialOptions{Provider: p, TokenFilePath: "/var/run/secrets/azure/tokens/azure-identity-token"})
require.NoError(t, err)

err = c.refreshCredentials(context.TODO())
Expand Down
4 changes: 2 additions & 2 deletions pkg/corerp/handlers/azure_userassigned_managedidentity.go
Original file line number Diff line number Diff line change
Expand Up @@ -97,7 +97,7 @@ func (handler *azureUserAssignedManagedIdentityHandler) Put(ctx context.Context,

identity, err := msiClient.CreateOrUpdate(ctx, rgName, identityName, armmsi.Identity{Location: &resourceLocation}, nil)
if err != nil {
return properties, fmt.Errorf("failed to create user assigned managed identity: %w", err)
return properties, fmt.Errorf("failed to create user-assigned managed identity: %w", err)
}

properties[UserAssignedIdentityIDKey] = to.String(identity.ID)
Expand All @@ -111,7 +111,7 @@ func (handler *azureUserAssignedManagedIdentityHandler) Put(ctx context.Context,
}

options.Resource.ID = id
logger.Info("Created managed identity for KeyVault access", logging.LogFieldLocalID, rpv1.LocalIDUserAssignedManagedIdentity)
logger.Info("Created user-assigned managed identity", logging.LogFieldLocalID, rpv1.LocalIDUserAssignedManagedIdentity)

return properties, nil
}
Expand Down
30 changes: 19 additions & 11 deletions pkg/recipes/terraform/config/providers/azure.go
Original file line number Diff line number Diff line change
Expand Up @@ -39,13 +39,20 @@ import (
const (
AzureProviderName = "azurerm"

azureFeaturesParam = "features"
azureSubIDParam = "subscription_id"
azureClientIDParam = "client_id"
azureClientSecretParam = "client_secret"
azureTenantIDParam = "tenant_id"
azureUseAKSWorkloadIdentityParam = "use_aks_workload_identity"
azureUseCLIParam = "use_cli"
azureFeaturesParam = "features"
azureSubIDParam = "subscription_id"
azureClientIDParam = "client_id"
azureClientSecretParam = "client_secret"
azureTenantIDParam = "tenant_id"
azureUseOIDCParam = "use_oidc"
azureUseCLIParam = "use_cli"
azureOIDCTokenFilePathParam = "oidc_token_file_path"

// The Azure AD Workload Identity Mutating Admission Webhook projects a signed service account token to
// this well known path.
// https://azure.github.io/azure-workload-identity/docs/installation/mutating-admission-webhook.html
// https://registry.terraform.io/providers/hashicorp/azurerm/latest/docs#argument-reference
azureOIDCTokenFilePath = "/var/run/secrets/azure/tokens/azure-identity-token"
Copy link
Contributor Author

Choose a reason for hiding this comment

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

these are the changes to make Terraform + Azure WI work.

Copy link
Contributor

@lakshmimsft lakshmimsft Aug 9, 2024

Choose a reason for hiding this comment

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

Q: will we be able to retrieve the TokenFilePath in the fetchAzureCredentials() call (eg line 129 below) instead of the const value here, say, in a future PR?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

we totally could - if we have a user ask us to make this configurable then we can. for now it looks like most if not everyone uses the standard well known file path established by the azure ad workload identity project.

)

var _ Provider = (*azureProvider)(nil)
Expand Down Expand Up @@ -175,13 +182,14 @@ func (p *azureProvider) generateProviderConfigMap(configMap map[string]any, cred
if credentials.WorkloadIdentity != nil &&
credentials.WorkloadIdentity.ClientID != "" &&
credentials.WorkloadIdentity.TenantID != "" {

// Use OIDC for Workload Identity
// https://registry.terraform.io/providers/hashicorp/azurerm/latest/docs/guides/service_principal_oidc
configMap[azureClientIDParam] = credentials.WorkloadIdentity.ClientID
configMap[azureTenantIDParam] = credentials.WorkloadIdentity.TenantID

// Use AKS Workload Identity for Azure provider
// https://registry.terraform.io/providers/hashicorp/azurerm/latest/docs/guides/aks_workload_identity#configuring-with-environment-variables
configMap[azureUseAKSWorkloadIdentityParam] = true
configMap[azureUseCLIParam] = false
configMap[azureUseOIDCParam] = true
configMap[azureOIDCTokenFilePathParam] = azureOIDCTokenFilePath
Copy link
Contributor Author

Choose a reason for hiding this comment

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

now this should work for AKS and non-AKS clusters

Copy link
Contributor

Choose a reason for hiding this comment

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

good catch!

}
}

Expand Down
16 changes: 10 additions & 6 deletions pkg/recipes/terraform/config/providers/azure_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -314,12 +314,13 @@ func TestAzureProvider_generateProviderConfigMap(t *testing.T) {
subscription: testSubscription,
credentials: testAzureWorkloadIdentityCredential,
expectedConfig: map[string]any{
azureFeaturesParam: map[string]any{},
azureSubIDParam: testSubscription,
azureTenantIDParam: testAzureWorkloadIdentityCredential.WorkloadIdentity.TenantID,
azureClientIDParam: testAzureWorkloadIdentityCredential.WorkloadIdentity.ClientID,
azureUseAKSWorkloadIdentityParam: true,
azureUseCLIParam: false,
azureFeaturesParam: map[string]any{},
azureSubIDParam: testSubscription,
azureTenantIDParam: testAzureWorkloadIdentityCredential.WorkloadIdentity.TenantID,
azureClientIDParam: testAzureWorkloadIdentityCredential.WorkloadIdentity.ClientID,
azureUseCLIParam: false,
azureUseOIDCParam: true,
azureOIDCTokenFilePathParam: "/var/run/secrets/azure/tokens/azure-identity-token",
},
},
{
Expand Down Expand Up @@ -369,6 +370,9 @@ func TestAzureProvider_generateProviderConfigMap(t *testing.T) {
require.Equal(t, tt.expectedConfig[azureClientIDParam], config[azureClientIDParam])
require.Equal(t, tt.expectedConfig[azureClientSecretParam], config[azureClientSecretParam])
require.Equal(t, tt.expectedConfig[azureTenantIDParam], config[azureTenantIDParam])
require.Equal(t, tt.expectedConfig[azureUseOIDCParam], config[azureUseOIDCParam])
require.Equal(t, tt.expectedConfig[azureUseCLIParam], config[azureUseCLIParam])
require.Equal(t, tt.expectedConfig[azureOIDCTokenFilePathParam], config[azureOIDCTokenFilePathParam])
})
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -25,7 +25,7 @@ import (
"github.com/radius-project/radius/test/validation"
)

// Test_Storage tests if a container can be created and then deleted by the magpiego with the workload identity.
// Test_Storage tests if a container on an Azure Storage Account can be created and then deleted by the magpiego with the workload identity.
func Test_Storage(t *testing.T) {
template := "testdata/corerp-resources-container-workload.bicep"
name := "corerp-resources-container-workload"
Expand Down
2 changes: 1 addition & 1 deletion test/magpiego/go.mod
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
module github.com/radius-project/radius/test/magpiego

go 1.22
go 1.22.0
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I changed this because I was getting an error trying to build this image on arm64

Copy link
Contributor

Choose a reason for hiding this comment

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

Interesting. Can you share the error message? It was working fine for me.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

go: downloading go1.22 (darwin/arm64)
go: download go1.22 for darwin/arm64: toolchain not available


require (
github.com/Azure/azure-sdk-for-go/sdk/azcore v1.13.0
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,7 @@ terraform {
required_providers {
azurerm = {
source = "hashicorp/azurerm"
version = "~> 3.0.0"
version = "~> 3.114.0"
Copy link
Contributor Author

Choose a reason for hiding this comment

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

~> 3.0.0 will only match 3.0.*, but workload identity for azurerm needs greater than 3.7. 3.114 is the latest version as of today

}
}
}
Expand Down
Loading