Adding Bicep private registry support using basic auth, Azure workload identity and AWS IRSA #7850

vishwahiremat · 2024-08-27T15:40:13Z

Description

Adding Bicep Recipe Config changes in typespec, conversions and unit tests.
Implementing Bicep FindSecretIDs
Adding basic auth support to authenticate private registries and unit tests.
Adding workload identity support for ACR registries
Adding AWS IRSA support for ECR registries.
Updating Recipe Metadata and Recipe Execute functions in the driver to get the oras auth client

Type of change

This pull request fixes a bug in Radius and has an approved issue (issue link required).
This pull request adds or changes features of Radius and has an approved issue (issue link required).
This pull request is a minor refactor, code cleanup, test improvement, or other maintenance task and doesn't change the functionality of Radius (issue link optional).

#6917

Signed-off-by: Vishwanath Hiremath <[email protected]>

codecov · 2024-08-27T16:03:32Z

Codecov Report

Attention: Patch coverage is 27.54491% with 121 lines in your changes missing coverage. Please review.

Project coverage is 60.99%. Comparing base (cce1fe6) to head (6ee1404).
Report is 1 commits behind head on main.

Files with missing lines	Patch %	Lines
pkg/rp/util/authclient/awsirsa.go	9.80%	46 Missing ⚠️
pkg/rp/util/authclient/azureworkloadidentity.go	6.45%	29 Missing ⚠️
pkg/recipes/driver/bicep.go	9.67%	26 Missing and 2 partials ⚠️
pkg/rp/util/authclient/basicauthentication.go	15.38%	11 Missing ⚠️
pkg/rp/util/authclient/types.go	62.50%	5 Missing and 1 partial ⚠️
pkg/ucp/aws/ucpcredentialprovider.go	0.00%	1 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #7850      +/-   ##
==========================================
- Coverage   61.18%   60.99%   -0.19%     
==========================================
  Files         523      527       +4     
  Lines       27683    27844     +161     
==========================================
+ Hits        16938    16984      +46     
- Misses       9252     9366     +114     
- Partials     1493     1494       +1

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Signed-off-by: Vishwanath Hiremath <[email protected]>

willdavsmith · 2024-09-03T20:59:19Z

pkg/corerp/datamodel/recipe_types.go

@@ -21,6 +21,9 @@ type RecipeConfigProperties struct {
 	// Configuration for Terraform Recipes. Controls how Terraform plans and applies templates as part of Recipe deployment.
 	Terraform TerraformConfigProperties `json:"terraform,omitempty"`

+	//Configuration for Bicep Recipes. Controls how Bicep plans and applies templates as part of Recipe deployment.


Suggested change

//Configuration for Bicep Recipes. Controls how Bicep plans and applies templates as part of Recipe deployment.

// Configuration for Bicep Recipes. Controls how Bicep plans and applies templates as part of Recipe deployment.

willdavsmith · 2024-09-03T21:00:30Z

pkg/corerp/datamodel/recipe_types.go

+// RegistrySecretConfig - Registry Secret Configuration used to authenticate to private bicep registries.
+type RegistrySecretConfig struct {
+	// The ID of an Applications.Core/SecretStore resource containing credential information used to authenticate private container
+	// registry.The keys in the secretstore depends on the type.


Suggested change

// registry.The keys in the secretstore depends on the type.

// registry. The keys in the secretstore depends on the type.

willdavsmith · 2024-09-03T21:02:40Z

pkg/recipes/driver/bicep.go

+
+func (d *bicepDriver) FindSecretIDs(ctx context.Context, envConfig recipes.Configuration, definition recipes.EnvironmentDefinition) (secretStoreIDResourceKeys map[string][]string, err error) {
+	secretStoreIDResourceKeys = make(map[string][]string)
+	if envConfig.RecipeConfig.Bicep.Authentication != nil {


might want to consider nil-checking this in case Authentication, Bicep, or RecipeConfig are nil

willdavsmith · 2024-09-03T21:03:15Z

pkg/recipes/driver/bicep.go

@@ -403,3 +434,28 @@ func (d *bicepDriver) getGCOutputResources(current []string, previous []string)

 	return diff, nil
 }
+
+func (d *bicepDriver) FindSecretIDs(ctx context.Context, envConfig recipes.Configuration, definition recipes.EnvironmentDefinition) (secretStoreIDResourceKeys map[string][]string, err error) {


does this function get called?

yes, it is called from recipe Engine, we dont have to make any changes in Engine as we are just implementing a DriverWithSecret interface here , it automatically does the part of calling this function

willdavsmith · 2024-09-03T21:03:50Z

pkg/rp/util/authClient/awsIRSA.go

+}
+
+func (b *awsIRSA) GetAuthClient(ctx context.Context, templatePath string) (remote.Client, error) {
+	// To Do


let's put an issue number here to track the work

willdavsmith · 2024-09-03T21:04:46Z

pkg/rp/util/authClient/azureWorkloadIdentity.go

+
+func (b *azureWorkloadIdentity) GetAuthClient(ctx context.Context, templatePath string) (remote.Client, error) {
+	c := azcontainerregistry.AuthenticationClientOptions{
+		azcore.ClientOptions{


is there a reason we need to change the defaults? what's the max retry default?

makes sense, keeping the defaults for client options

willdavsmith · 2024-09-03T21:06:07Z

pkg/rp/util/authClient/azureWorkloadIdentity.go

+		return nil, err
+	}
+
+	rt, err := ac.ExchangeAADAccessTokenForACRRefreshToken(ctx, "access_token", registryHost, &azcontainerregistry.AuthenticationClientExchangeAADAccessTokenForACRRefreshTokenOptions{


did you reference this from a docs page somewhere? if so, maybe we should link it here so that future work here will take less ramp-up time

azure container registry team have not released it yet, its part of their main repo. I will add the referent once the next release of azcontainerregistry is out

willdavsmith · 2024-09-03T21:08:44Z

pkg/rp/util/authClient/azureWorkloadIdentity.go

+	tenantID string
+}
+
+func NewAzureWorkloadIdentity(clientID string, tenantID string) AuthClient {


is this difficult to add a functional test for or does it seem feasible?

yeah it is not straight forward, i am creating a different github issue for that.

Signed-off-by: Vishwanath Hiremath <[email protected]>

pkg/corerp/api/v20231001preview/environment_conversion.go

ytimocin · 2024-09-06T18:59:25Z

pkg/corerp/api/v20231001preview/environment_conversion.go

@@ -238,6 +250,17 @@ func fromRecipeConfigDatamodel(config datamodel.RecipeConfigProperties) *RecipeC

 			recipeConfig.Terraform.Providers = fromRecipeConfigTerraformProvidersDatamodel(config)
 		}
+		if !reflect.DeepEqual(config.Bicep, datamodel.BicepConfigProperties{}) {


nit: new line before this

ytimocin · 2024-09-06T19:00:27Z

pkg/corerp/api/v20231001preview/environment_conversion.go

+				for k, v := range config.Bicep.Authentication {
+					authConfig[k] = datamodel.RegistrySecretConfig{
+						Secret: to.String(v.Secret),
+					}
+				}
+				recipeConfig.Bicep.Authentication = authConfig


Is there a reason why we are not doing it this way like you did below?

Suggested change

for k, v := range config.Bicep.Authentication {

authConfig[k] = datamodel.RegistrySecretConfig{

Secret: to.String(v.Secret),

}

}

recipeConfig.Bicep.Authentication = authConfig

for k, v := range config.Bicep.Authentication {

recipeConfig.Bicep.Authentication[k] = datamodel.RegistrySecretConfig{

Secret: to.String(v.Secret),

}

}

because recipeConfig.Bicep.Authentication value would be nil and it will throw nil pointer.

We can do recipeConfig.Bicep.Authentication := map[string]datamodel.RegistrySecretConfig{} before the loop.

We can do that, but we will be using the recipeConfig.Bicep.Authentication multiple times , i thought its better to use authConfig variable.

pkg/corerp/api/v20231001preview/testdata/environmentresource.json

pkg/corerp/datamodel/recipe_types.go

pkg/rp/util/authClient/azureWorkloadIdentity.go

pkg/rp/util/authClient/basicAuthentication.go

ytimocin · 2024-09-06T19:18:44Z

pkg/rp/util/authClient/basicAuthentication.go

folder and file names shouldn't have capital letters

pkg/rp/util/registry.go

pkg/ucp/aws/ucpcredentialprovider.go

Signed-off-by: Vishwanath Hiremath <[email protected]>

ytimocin · 2024-09-06T21:50:43Z

pkg/corerp/api/v20231001preview/environment_conversion.go

+				for k, v := range config.Bicep.Authentication {
+					authConfig[k] = datamodel.RegistrySecretConfig{
+						Secret: to.String(v.Secret),
+					}
+				}
+				recipeConfig.Bicep.Authentication = authConfig


We can do recipeConfig.Bicep.Authentication := map[string]datamodel.RegistrySecretConfig{} before the loop.

ytimocin · 2024-09-06T21:51:26Z

pkg/corerp/datamodel/recipe_types.go

@@ -21,6 +21,9 @@ type RecipeConfigProperties struct {
 	// Configuration for Terraform Recipes. Controls how Terraform plans and applies templates as part of Recipe deployment.
 	Terraform TerraformConfigProperties `json:"terraform,omitempty"`

+	// BicepConfigProperties represent configuration for Bicep Recipes. Controls how Bicep plans and applies templates as part of Recipe deployment.


Suggested change

// BicepConfigProperties represent configuration for Bicep Recipes. Controls how Bicep plans and applies templates as part of Recipe deployment.

// BicepConfigProperties represents configuration for Bicep Recipes. Controls how Bicep plans and applies templates as part of Recipe deployment.

ytimocin · 2024-09-06T21:52:47Z

pkg/rp/util/authclient/azureworkloadIdentity.go

name should be changed

Signed-off-by: Vishwanath Hiremath <[email protected]>

radius-functional-tests · 2024-09-06T22:31:43Z

Radius functional test overview

🔍 Go to test action run

Name	Value
Repository	vishwahiremat/radius
Commit ref	`70ee597`
Unique ID	funcb60c74cc3f
Image tag	pr-funcb60c74cc3f

Click here to see the list of tools in the current test run

gotestsum 1.12.0
KinD: v0.20.0
Dapr: 1.12.0
Azure KeyVault CSI driver: 1.4.2
Azure Workload identity webhook: 1.3.0
Bicep recipe location ghcr.io/radius-project/dev/test/testrecipes/test-bicep-recipes/<name>:pr-funcb60c74cc3f
Terraform recipe location http://tf-module-server.radius-test-tf-module-server.svc.cluster.local/<name>.zip (in cluster)
applications-rp test image location: ghcr.io/radius-project/dev/applications-rp:pr-funcb60c74cc3f
controller test image location: ghcr.io/radius-project/dev/controller:pr-funcb60c74cc3f
ucp test image location: ghcr.io/radius-project/dev/ucpd:pr-funcb60c74cc3f
deployment-engine test image location: ghcr.io/radius-project/deployment-engine:latest

Test Status

⌛ Building Radius and pushing container images for functional tests...
✅ Container images build succeeded
⌛ Publishing Bicep Recipes for functional tests...
✅ Recipe publishing succeeded
⌛ Starting ucp-cloud functional tests...
⌛ Starting datastoresrp-cloud functional tests...
⌛ Starting corerp-cloud functional tests...
✅ ucp-cloud functional tests succeeded
✅ datastoresrp-cloud functional tests succeeded
✅ corerp-cloud functional tests succeeded

lakshmimsft · 2024-09-06T22:50:40Z

pkg/rp/util/authclient/awsirsa.go

+	return &awsIRSA{roleARN: roleARN}
+}
+
+func (b *awsIRSA) GetAuthClient(ctx context.Context, templatePath string) (remote.Client, error) {


Pls add comment for IRSA implementation. it was added for Azure WI, BasicAuth

lakshmimsft · 2024-09-06T23:12:14Z

pkg/rp/util/registry_test.go

+			exp: recipes.SecretData{},
+		},
+	}
+	for _, tc := range testset {


We need to add test verify returning error when say templatePath passed is not a valid path

added a test

lakshmimsft · 2024-09-09T16:32:16Z

pkg/rp/util/registry.go

@@ -133,3 +135,12 @@ func parsePath(path string) (repository string, tag string, err error) {
 	tag = reference.Tag()
 	return
 }
+
+func GetRegistrySecrets(definition recipes.Configuration, templatePath string, secrets map[string]recipes.SecretData) (recipes.SecretData, error) {


Pls add comment for public function

lakshmimsft · 2024-09-09T16:39:57Z

typespec/Applications.Core/secretstores.tsp

@@ -72,10 +72,10 @@ enum SecretStoreDataType {
  @doc("basicAuthentication type is used to represent username and password based authentication and the secretstore resource is expected to have the keys 'username' and 'password'.")
  basicAuthentication,

-  @doc("azureWorkloadIdentity type is used to represent registry authentication using azure federated identity and the secretstore resource is expected to have the keys 'clientId' and 'tenantId'.")
+  @doc("azureWorkloadIdentity type is used to represent authentication using azure federated identity and the secretstore resource is expected to have the keys 'clientId' and 'tenantId'.")


👍 thank u!

Signed-off-by: Vishwanath Hiremath <[email protected]>

radius-functional-tests · 2024-09-09T18:06:50Z

Radius functional test overview

🔍 Go to test action run

Name	Value
Repository	vishwahiremat/radius
Commit ref	`6ee1404`
Unique ID	func20ed078b04
Image tag	pr-func20ed078b04

Click here to see the list of tools in the current test run

gotestsum 1.12.0
KinD: v0.20.0
Dapr: 1.12.0
Azure KeyVault CSI driver: 1.4.2
Azure Workload identity webhook: 1.3.0
Bicep recipe location ghcr.io/radius-project/dev/test/testrecipes/test-bicep-recipes/<name>:pr-func20ed078b04
Terraform recipe location http://tf-module-server.radius-test-tf-module-server.svc.cluster.local/<name>.zip (in cluster)
applications-rp test image location: ghcr.io/radius-project/dev/applications-rp:pr-func20ed078b04
controller test image location: ghcr.io/radius-project/dev/controller:pr-func20ed078b04
ucp test image location: ghcr.io/radius-project/dev/ucpd:pr-func20ed078b04
deployment-engine test image location: ghcr.io/radius-project/deployment-engine:latest

Test Status

⌛ Building Radius and pushing container images for functional tests...
✅ Container images build succeeded
⌛ Publishing Bicep Recipes for functional tests...
✅ Recipe publishing succeeded
⌛ Starting datastoresrp-cloud functional tests...
⌛ Starting corerp-cloud functional tests...
⌛ Starting ucp-cloud functional tests...
✅ datastoresrp-cloud functional tests succeeded
✅ corerp-cloud functional tests succeeded
✅ ucp-cloud functional tests succeeded

vishwahiremat added 4 commits August 12, 2024 02:34

adding typespec and datamodel changes

2af6779

Signed-off-by: Vishwanath Hiremath <[email protected]>

initial commit

1c564a6

Signed-off-by: Vishwanath Hiremath <[email protected]>

adding different types

a11a668

Signed-off-by: Vishwanath Hiremath <[email protected]>

basic auth changes

d6346ba

Signed-off-by: Vishwanath Hiremath <[email protected]>

vishwahiremat added 2 commits August 30, 2024 11:15

resolving merge conflicts

c67ad90

Signed-off-by: Vishwanath Hiremath <[email protected]>

adding azure workload idenetity changes

da4f16f

Signed-off-by: Vishwanath Hiremath <[email protected]>

vishwahiremat marked this pull request as ready for review September 3, 2024 20:55

vishwahiremat requested review from a team as code owners September 3, 2024 20:55

fix typo

8c3538e

Signed-off-by: Vishwanath Hiremath <[email protected]>

willdavsmith reviewed Sep 3, 2024

View reviewed changes

vishwahiremat added 4 commits September 3, 2024 21:30

resolve merge conflicts

45e0d8e

Signed-off-by: Vishwanath Hiremath <[email protected]>

addressing comments

1b0da34

Signed-off-by: Vishwanath Hiremath <[email protected]>

adding comments

3e018a3

Signed-off-by: Vishwanath Hiremath <[email protected]>

adding ecr support with irsa changes

ba0dc87

Signed-off-by: Vishwanath Hiremath <[email protected]>

vishwahiremat changed the title ~~Adding Bicep Recipe Config changes and support for authenticating private bicep registries using basic auth~~ Adding Bicep private registry support using basic auth, Azure workload identity and AWS IRSA Sep 6, 2024

resolving merge conflicts

a7bd111

Signed-off-by: Vishwanath Hiremath <[email protected]>

ytimocin requested changes Sep 6, 2024

View reviewed changes

vishwahiremat added 3 commits September 6, 2024 14:23

addressing comments

2af8ba7

Signed-off-by: Vishwanath Hiremath <[email protected]>

renaming authClient folder

1dbd60f

Signed-off-by: Vishwanath Hiremath <[email protected]>

rename types in authClient

88523fb

Signed-off-by: Vishwanath Hiremath <[email protected]>

ytimocin requested changes Sep 6, 2024

View reviewed changes

vishwahiremat added 2 commits September 6, 2024 15:02

Merge branch 'main' into vishwahiremat/bicep-recipe-config-changes

f3abd24

Signed-off-by: Vishwanath Hiremath <[email protected]>

addressing comments

70ee597

Signed-off-by: Vishwanath Hiremath <[email protected]>

vishwahiremat temporarily deployed to functional-tests September 6, 2024 22:30 — with GitHub Actions Inactive

ytimocin previously approved these changes Sep 6, 2024

View reviewed changes

lakshmimsft reviewed Sep 6, 2024

View reviewed changes

lakshmimsft reviewed Sep 9, 2024

View reviewed changes

addressing comments

6ee1404

Signed-off-by: Vishwanath Hiremath <[email protected]>

vishwahiremat dismissed ytimocin’s stale review via 6ee1404 September 9, 2024 17:23

vishwahiremat temporarily deployed to functional-tests September 9, 2024 18:05 — with GitHub Actions Inactive

lakshmimsft approved these changes Sep 9, 2024

View reviewed changes

lakshmimsft merged commit f89f424 into radius-project:main Sep 9, 2024
29 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Adding Bicep private registry support using basic auth, Azure workload identity and AWS IRSA #7850

Adding Bicep private registry support using basic auth, Azure workload identity and AWS IRSA #7850

vishwahiremat commented Aug 27, 2024 •

edited

Loading

codecov bot commented Aug 27, 2024 •

edited

Loading

willdavsmith Sep 3, 2024

willdavsmith Sep 3, 2024

willdavsmith Sep 3, 2024

willdavsmith Sep 3, 2024

vishwahiremat Sep 4, 2024

willdavsmith Sep 3, 2024

willdavsmith Sep 3, 2024

vishwahiremat Sep 4, 2024

willdavsmith Sep 3, 2024

vishwahiremat Sep 4, 2024

willdavsmith Sep 3, 2024

vishwahiremat Sep 6, 2024

ytimocin Sep 6, 2024

ytimocin Sep 6, 2024

vishwahiremat Sep 6, 2024

ytimocin Sep 6, 2024

vishwahiremat Sep 6, 2024

ytimocin Sep 6, 2024

ytimocin Sep 6, 2024

ytimocin Sep 6, 2024

ytimocin Sep 6, 2024

radius-functional-tests bot commented Sep 6, 2024 •

edited

Loading

lakshmimsft Sep 6, 2024

vishwahiremat Sep 9, 2024

lakshmimsft Sep 6, 2024

vishwahiremat Sep 9, 2024

lakshmimsft Sep 9, 2024

vishwahiremat Sep 9, 2024

lakshmimsft Sep 9, 2024 •

edited

Loading

radius-functional-tests bot commented Sep 9, 2024 •

edited

Loading

	//Configuration for Bicep Recipes. Controls how Bicep plans and applies templates as part of Recipe deployment.
	// Configuration for Bicep Recipes. Controls how Bicep plans and applies templates as part of Recipe deployment.

	// registry.The keys in the secretstore depends on the type.
	// registry. The keys in the secretstore depends on the type.

	// BicepConfigProperties represent configuration for Bicep Recipes. Controls how Bicep plans and applies templates as part of Recipe deployment.
	// BicepConfigProperties represents configuration for Bicep Recipes. Controls how Bicep plans and applies templates as part of Recipe deployment.

Adding Bicep private registry support using basic auth, Azure workload identity and AWS IRSA #7850

Adding Bicep private registry support using basic auth, Azure workload identity and AWS IRSA #7850

Conversation

vishwahiremat commented Aug 27, 2024 • edited Loading

Description

Type of change

codecov bot commented Aug 27, 2024 • edited Loading

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

radius-functional-tests bot commented Sep 6, 2024 • edited Loading

Radius functional test overview

Test Status

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lakshmimsft Sep 9, 2024 • edited Loading

Choose a reason for hiding this comment

radius-functional-tests bot commented Sep 9, 2024 • edited Loading

Radius functional test overview

Test Status

vishwahiremat commented Aug 27, 2024 •

edited

Loading

codecov bot commented Aug 27, 2024 •

edited

Loading

radius-functional-tests bot commented Sep 6, 2024 •

edited

Loading

lakshmimsft Sep 9, 2024 •

edited

Loading

radius-functional-tests bot commented Sep 9, 2024 •

edited

Loading