Updating listSecrets to use the new Azure Client #4862

ytimocin · 2022-12-14T21:04:12Z

Description

Clients migrated:

Custom Action

Functions:

GetResourceLocation Function

Work in Progress:

Unit Test
Functional Test

Issue reference

Fixes: #issue_number

Checklist

Please make sure you've completed the relevant tasks for this PR, out of the following list:

Code compiles correctly
Adds necessary unit tests for change
Adds necessary E2E tests for change
Unit tests passing
Extended the documentation / Created issue for it

pkg/azure/clients/common.go

pkg/azure/clientv2/types.go

pkg/azure/clientv2/customaction.go

rynowak · 2022-12-15T18:41:30Z

pkg/rp/secretvalueclient.go

-	custom := clients.NewCustomActionClient(parsed.FindScope(resources.SubscriptionsSegment), c.ARM.Auth)
-	response, err := custom.InvokeCustomAction(ctx, arm.ID, arm.APIVersion, action, nil)
+	subscriptionID := parsed.FindScope(resources.SubscriptionsSegment)
+	client, err := clientv2.NewCustomActionClient(subscriptionID, c.ARM.ClientOption.Cred)


If we're taking a resource ID in BeginCustomAction do we need to pass the subscription id here? That would simplify life if we don't have to.

youngbupark · 2022-12-15T19:01:00Z

pkg/azure/clientv2/customaction.go

@@ -27,7 +28,7 @@ type CustomActionClient struct {
 }


Please address all missing feedbacks in #4851

About the unit tests; the tests in the current client (not v2) are as follows:

resourcedeploymentclient_test.go (https://github.com/project-radius/radius/blob/main/pkg/azure/clients/resourcedeploymentclient_test.go)

resourcedeploymentoperationsclient_test.go (https://github.com/project-radius/radius/blob/main/pkg/azure/clients/resourcedeploymentoperationsclient_test.go)

unfold_test.go (https://github.com/project-radius/radius/blob/main/pkg/azure/clients/unfold_test.go).

The first two only have one test in each. And those tests can't be moved over to the new client because those functions don't exist anymore in client v2.

unfold.go will be handled in another PR and therefore the tests will be also moved/changed accordingly in that PR.

I will also have another PR just for adding/polishing unit tests after we migrate to the new clients.

Please let me know if that doesn't for you @youngbupark @rynowak. I am open to suggestions of any kind.

Right now this PR is the step 1 of the migration to client v2.

pkg/azure/clientv2/customaction.go

pkg/azure/clientv2/clients.go

pkg/azure/clientv2/customaction.go

pkg/corerp/handlers/azure_federatedidentity.go

pkg/azure/clientv2/customaction.go

pkg/rp/secretvalueclient.go

pkg/azure/armauth/auth.go

pkg/azure/clientv2/common.go

pkg/azure/clientv2/customaction.go

youngbupark · 2022-12-19T18:21:08Z

pkg/azure/clientv2/customaction.go

 	if err != nil {
 		return nil, err
 	}

-	pipeline, err := armruntime.NewPipeline(moduleName, moduleVersion, credential, runtime.PipelineOptions{}, options)
+	pipeline, err := armruntime.NewPipeline(ModuleName, ModuleVersion, options.Cred, runtime.PipelineOptions{}, armClientOptions)


it means customactionclient always requires the credentials, but client.CustomActionClient does not use any authorizer. Can you please review CustomActionClient usage again?

Also if cred is not required, then how should we use newclient without cred?

pkg/azure/clientv2/resourcedeploymentclient.go

pkg/azure/clientv2/resourcedeploymentoperationsclient.go

youngbupark · 2022-12-19T22:26:52Z

pkg/rp/secretvalueclient.go

+		return nil, err
+	}
+
+	options := clientv2.NewClientBeginCustomActionOptions(arm.ID, action, arm.APIVersion)


Why do we need additional Begin* function ? I do not think we need to split InvokeCustomAction to NewClientBeginCustomActionOptions and BeginCustomAction.

ytimocin · 2022-12-20T00:10:54Z

pkg/azure/armauth/auth.go

-	// ClientOption is the client v2 option including new client credentials.
-	ClientOption clientv2.AzureClientOption
+	// ClientOptions is the client v2 options including new client credentials.
+	ClientOptions clientv2.Options


The reason I named this as ClientOptions is to specify that this is the Options struct that is in the Clients package.

pkg/azure/clientv2/clients.go

youngbupark · 2022-12-20T00:51:31Z

pkg/azure/clientv2/customaction.go

 	"github.com/Azure/azure-sdk-for-go/sdk/azcore/policy"
 	"github.com/Azure/azure-sdk-for-go/sdk/azcore/runtime"
 	"github.com/Azure/azure-sdk-for-go/sdk/resourcemanager/resources/armresources"
 	"github.com/project-radius/radius/pkg/ucp/resources"
 )

-type CustomActionClient struct {
-	*BaseClient
+type ClientCustomActionResponse struct {


Please add comments to all public struct/methods/members...

pkg/azure/clientv2/resourcedeploymentclient.go

pkg/azure/clientv2/customaction.go

youngbupark · 2022-12-20T18:16:06Z

pkg/azure/clientv2/clients.go

 	// Cred represents a credential for OAuth token.
 	Cred azcore.TokenCredential
+
+	BaseURI string


Do we still need this BaseURI here ? please add comment to public member.

We still use this as the way to pass in a BaseURI to the client. I am going to see in the other PRs if we actually need this, and then update if necessary.

youngbupark · 2022-12-20T19:47:02Z

pkg/azure/clientv2/resourcedeploymentclient.go

 }

-func NewClientBeginCreateOrUpdateOptions(resourceID, resumeToken, apiVersion string) *ClientBeginCreateOrUpdateOptions {
-	// FIXME: This is to validate the resourceID.
+func NewClientBeginCreateOrUpdateOptions(resourceID, apiVersion string) *ClientBeginCreateOrUpdateOptions {


Do you have the reason why you introduce Begin pattern? the older client doesn't have this begin. To me, begin* is an unnecessary helper. Please make it simple.

As we discussed, this clientv2 pkg wraps the client, but we do not need to follow the generated client pattern. Generator has to use the same pattern to generate client even though it is inefficient. I strongly recommend to follow the older client pattern although the implementation is different.

So, we do not need ClientBeginCreateOrUpdateOptions. We only need two methods like below.

// .... func (client *DeploymentsClient) CreateOrUpdate(ctx context.Context, resourceID string, parameters Deployment) (*runtime.Poller[ClientCreateOrUpdateResponse], error) { // validate resourceID req, err := client.createCreateOrUpdateRequest(ctx, resourceID, parameters) resp, err := client.Pipeline.Do(req) if err != nil { return nil, err } if !runtime.HasStatusCode(resp, http.StatusOK, http.StatusCreated, http.StatusAccepted) { return nil, runtime.NewResponseError(resp) } return runtime.NewPoller[ClientCreateOrUpdateResponse](resp, *client.Pipeline, nil) } func (client *DeploymentsClient) createCreateOrUpdateRequest(ctx context.Context, resourceID string, parameters Deployment) (*policy.Request, error) { // return Request }

youngbupark

lgtm

youngbupark · 2022-12-22T19:03:36Z

pkg/azure/clientv2/customaction.go

+	urlPath = strings.ReplaceAll(urlPath, "{resourceID}", url.PathEscape(resourceID))

-	if opts.action == "" {
+	if action == "" {
 		return nil, errors.New("action cannot be empty")
 	}
-	urlPath = strings.ReplaceAll(urlPath, "{action}", url.PathEscape(opts.action))
+	urlPath = strings.ReplaceAll(urlPath, "{action}", url.PathEscape(action))


QQ: is there a reason why we use ReplaceAll?
Can we just use fmt.sprintf ?

if resourceID == "" { return nil, errors.New("resourceID cannot be empty") } if action == "" { return nil, errors.New("action cannot be empty") } urlPath := fmt.Sprintf("%s/%s", url.PathEscape(resourceID), url.PathEscape(action))

I think we can also use runtime.JoinPaths instead of urlPath.

if resourceID == "" { return nil, errors.New("resourceID cannot be empty") } if action == "" { return nil, errors.New("action cannot be empty") } urlPath := runtime.JoinPaths(client.baseURI, url.PathEscape(resourceID), url.PathEscape(action)) req, err := runtime.NewRequest(ctx, http.MethodPost, urlPath)

youngbupark · 2022-12-22T19:34:57Z

pkg/azure/clientv2/customaction.go

-	}
-	return resp, nil
-}
+	urlPath := "{resourceID}/{action}"


We do not need this anymore.

# Description Clients migrated: 1. Custom Action Functions: 1. GetResourceLocation Function Work in Progress: 1. Unit Test 3. Functional Test ## Issue reference Fixes: #issue_number ## Checklist Please make sure you've completed the relevant tasks for this PR, out of the following list: * [ ] Code compiles correctly * [ ] Adds necessary unit tests for change * [ ] Adds necessary E2E tests for change * [ ] Unit tests passing * [ ] Extended the documentation / Created issue for it

ytimocin marked this pull request as ready for review December 14, 2022 22:56

ytimocin requested a review from a team as a code owner December 14, 2022 22:56

ytimocin force-pushed the ytimocin/clientV2/customAction branch 2 times, most recently from 9f447ac to 63a4a20 Compare December 15, 2022 00:18

youngbupark reviewed Dec 15, 2022

View reviewed changes

pkg/azure/clients/common.go Outdated Show resolved Hide resolved

ytimocin force-pushed the ytimocin/clientV2/customAction branch from 63a4a20 to adb9534 Compare December 15, 2022 02:59

ytimocin force-pushed the ytimocin/azureClientV2 branch from 360d44e to e9d5d18 Compare December 15, 2022 17:12

youngbupark reviewed Dec 15, 2022

View reviewed changes

pkg/azure/clientv2/types.go Outdated Show resolved Hide resolved

youngbupark reviewed Dec 15, 2022

View reviewed changes

pkg/azure/clientv2/customaction.go Outdated Show resolved Hide resolved

Base automatically changed from ytimocin/azureClientV2 to main December 15, 2022 18:04

ytimocin force-pushed the ytimocin/clientV2/customAction branch from adb9534 to cbd7f79 Compare December 15, 2022 18:08

rynowak reviewed Dec 15, 2022

View reviewed changes

youngbupark reviewed Dec 15, 2022

View reviewed changes

pkg/azure/clientv2/customaction.go Outdated Show resolved Hide resolved

ytimocin force-pushed the ytimocin/clientV2/customAction branch 4 times, most recently from 882e918 to c36dad9 Compare December 15, 2022 22:21

ytimocin requested review from rynowak and youngbupark December 15, 2022 22:22

youngbupark reviewed Dec 16, 2022

View reviewed changes

pkg/azure/clientv2/clients.go Outdated Show resolved Hide resolved

pkg/azure/clientv2/clients.go Outdated Show resolved Hide resolved

youngbupark reviewed Dec 16, 2022

View reviewed changes

pkg/azure/clientv2/customaction.go Outdated Show resolved Hide resolved

youngbupark reviewed Dec 16, 2022

View reviewed changes

pkg/corerp/handlers/azure_federatedidentity.go Outdated Show resolved Hide resolved

youngbupark reviewed Dec 16, 2022

View reviewed changes

pkg/azure/clientv2/customaction.go Outdated Show resolved Hide resolved

youngbupark reviewed Dec 16, 2022

View reviewed changes

pkg/azure/clientv2/customaction.go Outdated Show resolved Hide resolved

youngbupark reviewed Dec 16, 2022

View reviewed changes

pkg/rp/secretvalueclient.go Outdated Show resolved Hide resolved

ytimocin force-pushed the ytimocin/clientV2/customAction branch 2 times, most recently from 7090307 to f081ad6 Compare December 16, 2022 22:32

youngbupark reviewed Dec 19, 2022

View reviewed changes

pkg/azure/armauth/auth.go Outdated Show resolved Hide resolved

ytimocin force-pushed the ytimocin/clientV2/customAction branch from f081ad6 to 4127268 Compare December 19, 2022 17:46

ytimocin force-pushed the ytimocin/clientV2/customAction branch from 4127268 to baf08ef Compare December 19, 2022 18:19

youngbupark reviewed Dec 19, 2022

View reviewed changes

pkg/azure/clientv2/resourcedeploymentclient.go Show resolved Hide resolved

youngbupark reviewed Dec 19, 2022

View reviewed changes

pkg/azure/clientv2/resourcedeploymentoperationsclient.go Outdated Show resolved Hide resolved

youngbupark mentioned this pull request Dec 19, 2022

Migrating more clients to the v2 #4864

Merged

5 tasks

youngbupark reviewed Dec 19, 2022

View reviewed changes

ytimocin force-pushed the ytimocin/clientV2/customAction branch from baf08ef to 2e662b8 Compare December 20, 2022 00:09

ytimocin commented Dec 20, 2022

View reviewed changes

youngbupark reviewed Dec 20, 2022

View reviewed changes

pkg/azure/clientv2/clients.go Outdated Show resolved Hide resolved

ytimocin force-pushed the ytimocin/clientV2/customAction branch from 2e662b8 to c4593a7 Compare December 20, 2022 00:19

youngbupark reviewed Dec 20, 2022

View reviewed changes

ytimocin force-pushed the ytimocin/clientV2/customAction branch 3 times, most recently from 108a32f to b40e4cf Compare December 20, 2022 16:56

youngbupark reviewed Dec 20, 2022

View reviewed changes

ytimocin force-pushed the ytimocin/clientV2/customAction branch from b40e4cf to 9f76a56 Compare December 20, 2022 19:18

youngbupark reviewed Dec 20, 2022

View reviewed changes

ytimocin force-pushed the ytimocin/clientV2/customAction branch from 9f76a56 to c7b6c3b Compare December 20, 2022 19:48

youngbupark approved these changes Dec 22, 2022

View reviewed changes

youngbupark reviewed Dec 22, 2022

View reviewed changes

Migrating custom action client for client v2

5795f05

ytimocin force-pushed the ytimocin/clientV2/customAction branch 2 times, most recently from 0aff153 to 8a56a94 Compare December 22, 2022 19:27

youngbupark reviewed Dec 22, 2022

View reviewed changes

pkg/azure/clientv2/customaction.go Outdated

}

return resp, nil

}

urlPath := "{resourceID}/{action}"

Copy link

youngbupark Dec 22, 2022

Choose a reason for hiding this comment

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

We do not need this anymore.

Updating DeploymentsClient and DeploymentOperationsClient

f2631b4

ytimocin force-pushed the ytimocin/clientV2/customAction branch from 8a56a94 to f2631b4 Compare December 22, 2022 20:05

ytimocin merged commit 6bf7ff8 into main Dec 22, 2022

ytimocin deleted the ytimocin/clientV2/customAction branch December 22, 2022 20:37

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Updating listSecrets to use the new Azure Client #4862

Updating listSecrets to use the new Azure Client #4862

ytimocin commented Dec 14, 2022 •

edited

Loading

rynowak Dec 15, 2022

youngbupark Dec 15, 2022

ytimocin Dec 15, 2022

youngbupark Dec 19, 2022

youngbupark Dec 19, 2022

ytimocin Dec 20, 2022

youngbupark Dec 20, 2022

youngbupark Dec 20, 2022

ytimocin Dec 20, 2022

youngbupark Dec 20, 2022 •

edited

Loading

youngbupark Dec 20, 2022 •

edited

Loading

youngbupark left a comment

youngbupark Dec 22, 2022

youngbupark Dec 22, 2022 •

edited

Loading

youngbupark Dec 22, 2022

Updating listSecrets to use the new Azure Client #4862

Updating listSecrets to use the new Azure Client #4862

Conversation

ytimocin commented Dec 14, 2022 • edited Loading

Description

Issue reference

Checklist

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

youngbupark Dec 20, 2022 • edited Loading

Choose a reason for hiding this comment

youngbupark Dec 20, 2022 • edited Loading

Choose a reason for hiding this comment

youngbupark left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

youngbupark Dec 22, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ytimocin commented Dec 14, 2022 •

edited

Loading

youngbupark Dec 20, 2022 •

edited

Loading

youngbupark Dec 20, 2022 •

edited

Loading

youngbupark Dec 22, 2022 •

edited

Loading