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.

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

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

Merged
merged 21 commits into from
Aug 13, 2024
Merged

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

Show file tree
Hide file tree
Changes from 18 commits
Commits
Show all changes
21 commits
Select commit Hold shift + click to select a range
3357a06
FT WI
willdavsmith Aug 7, 2024
ddcf11c
FT WI
willdavsmith Aug 7, 2024
a231493
FT WI
willdavsmith Aug 7, 2024
442e778
FT WI
willdavsmith Aug 7, 2024
4be5bd4
FT WI
willdavsmith Aug 7, 2024
36cee08
FT WI
willdavsmith Aug 7, 2024
96ead44
FT WI
willdavsmith Aug 7, 2024
6427e2b
FT WI
willdavsmith Aug 7, 2024
efb4494
FT WI
willdavsmith Aug 7, 2024
9197ee1
FT WI
willdavsmith Aug 8, 2024
b203cd9
FT WI
willdavsmith Aug 8, 2024
bdfb399
FT WI
willdavsmith Aug 8, 2024
7356e43
FT WI
willdavsmith Aug 8, 2024
b1e6fc2
FT WI
willdavsmith Aug 8, 2024
53b5066
FT WI
willdavsmith Aug 8, 2024
88e2965
FT WI
willdavsmith Aug 8, 2024
ece0901
FT WI
willdavsmith Aug 8, 2024
1dc273a
FT WI
willdavsmith Aug 8, 2024
0bac281
FT WI
willdavsmith Aug 8, 2024
4993704
reverting secret variable names
willdavsmith Aug 9, 2024
5529ca1
Merge branch 'main' into willdavsmith/ft-wi-main
willdavsmith Aug 13, 2024
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
36 changes: 21 additions & 15 deletions .github/workflows/functional-test-cloud.yaml
View file
Open in desktop
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 @@ -95,7 +95,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 @@ -117,7 +117,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 @@ -352,7 +358,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 @@ -431,9 +437,9 @@ jobs:
- name: Login to Azure
uses: azure/login@v2
with:
client-id: ${{ secrets.AZURE_SP_TESTS_APPID }}
tenant-id: ${{ secrets.AZURE_SP_TESTS_TENANTID }}
subscription-id: ${{ secrets.AZURE_SUBSCRIPTIONID_TESTS }}
client-id: ${{ secrets.AZURE_TEST_APPID }}
tenant-id: ${{ secrets.AZURE_TEST_TENANTID }}
subscription-id: ${{ secrets.AZURE_TEST_SUBSCRIPTIONID }}
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 added these in the repo secrets, we should move them to org secrets

Copy link
Contributor Author

Choose a reason for hiding this comment

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

before we merge, I will figure this out with management. safe to assume that these will be populated with the correct values.

Copy link
Contributor

Choose a reason for hiding this comment

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

Why do we need to use new clientID, tenantID, and subscriptionID?

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 don't - I added them for testing. we can use the same variables but I would need to edit the existing app we use. I'll talk to mgmt


- uses: marocchino/sticky-pull-request-comment@v2
continue-on-error: true
Expand All @@ -451,7 +457,7 @@ jobs:
az group create \
--location ${{ env.AZURE_LOCATION }} \
--name $RESOURCE_GROUP \
--subscription ${{ secrets.AZURE_SUBSCRIPTIONID_TESTS }} \
--subscription ${{ secrets.AZURE_TEST_SUBSCRIPTIONID }} \
--tags creationTime=$current_time
while [ $(az group exists --name $RESOURCE_GROUP) = false ]; do sleep 2; done
env:
Expand Down Expand Up @@ -516,7 +522,7 @@ jobs:
- name: Install azure workload identity webhook chart
run: |
helm repo add azure-workload-identity https://azure.github.io/azure-workload-identity/charts
helm install workload-identity-webhook azure-workload-identity/workload-identity-webhook --namespace radius-default --create-namespace --version ${{ env.AZURE_WORKLOAD_IDENTITY_WEBHOOK_VER }} --set azureTenantID=${{ secrets.AZURE_SP_TESTS_TENANTID }}
helm install workload-identity-webhook azure-workload-identity/workload-identity-webhook --namespace radius-default --create-namespace --version ${{ env.AZURE_WORKLOAD_IDENTITY_WEBHOOK_VER }} --set azureTenantID=${{ secrets.AZURE_TEST_TENANTID }}

- name: Login to GitHub Container Registry
uses: docker/login-action@v3
Expand Down Expand Up @@ -556,7 +562,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 @@ -568,11 +574,11 @@ jobs:
rad env switch kind-radius

echo "*** Configuring Azure provider ***"
rad env update kind-radius --azure-subscription-id ${{ secrets.AZURE_SUBSCRIPTIONID_TESTS }} \
rad env update kind-radius --azure-subscription-id ${{ secrets.AZURE_TEST_SUBSCRIPTIONID }} \
--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 }} \
--tenant-id ${{ secrets.AZURE_SP_TESTS_TENANTID }}
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_TEST_APPID }} \
--tenant-id ${{ secrets.AZURE_TEST_TENANTID }}

echo "*** Configuring AWS provider ***"
rad env update kind-radius --aws-region ${{ env.AWS_REGION }} --aws-account-id ${{ secrets.FUNCTEST_AWS_ACCOUNT_ID }}
Expand Down Expand Up @@ -719,7 +725,7 @@ jobs:
run: |
# if deletion fails, purge workflow will purge the resource group and its resources later.
az group delete \
--subscription ${{ secrets.AZURE_SUBSCRIPTIONID_TESTS }} \
--subscription ${{ secrets.AZURE_TEST_SUBSCRIPTIONID }} \
--name ${{ env.AZURE_TEST_RESOURCE_GROUP }} \
--yes --verbose

Expand Down
13 changes: 10 additions & 3 deletions pkg/azure/credential/ucpcredentials.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -171,14 +171,21 @@ 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,
ClientOptions: *c.options.ClientOptions,
}
} else {
opt = &azidentity.WorkloadIdentityCredentialOptions{
ClientID: azureWorkloadIdentityCredential.ClientID,
TenantID: azureWorkloadIdentityCredential.TenantID,
}
}

azCred, err := azidentity.NewDefaultAzureCredential(opt)
azCred, err := azidentity.NewWorkloadIdentityCredential(opt)
if err != nil {
return err
}
Expand Down
4 changes: 2 additions & 2 deletions pkg/corerp/handlers/azure_userassigned_managedidentity.go
View file
Open in desktop
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
View file
Open in desktop
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 •
edited
Loading

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
View file
Open in desktop
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])
})
}
}
2 changes: 1 addition & 1 deletion test/functional-portable/corerp/cloud/resources/storage_test.go
View file
Open in desktop
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
View file
Open in desktop
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
2 changes: 1 addition & 1 deletion test/testrecipes/test-terraform-recipes/azure-storage/main.tf
View file
Open in desktop
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