Support UserAgent Attribution #962

kassarl · 2021-08-05T16:25:55Z

What type of PR is this?

/kind feature

What this PR does / why we need it:
Allows for customer attribution for PVCs when they are created
Which issue(s) this PR fixes:

Fixes #

Requirements:

uses conventional commit messages
includes documentation
adds unit tests
tested upgrade from previous version

Special notes for your reviewer:

Release note:

Allows UserAgent attribution on PVCs via tagging

k8s-ci-robot · 2021-08-05T16:26:03Z

Hi @kassarl. Thanks for your PR.

I'm waiting for a kubernetes-sigs member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

k8s-ci-robot · 2021-08-05T16:26:13Z

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: kassarl
To complete the pull request process, please assign feiskyer after the PR has been reviewed.
You can assign the PR to them by writing /assign @feiskyer in a comment when ready.

The full list of commands accepted by this bot can be found here.

Needs approval from an approver in each of these files:

OWNERS

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

pkg/azuredisk/controllerserver.go

andyzhangx

would you vendor the latest cloud provider first?

andyzhangx · 2021-08-07T01:31:56Z

pkg/azuredisk/controllerserver.go

@@ -293,6 +299,14 @@ func (d *Driver) CreateVolume(ctx context.Context, req *csi.CreateVolumeRequest)
 	contentSource := &csi.VolumeContentSource{}
 	for k, v := range customTagsMap {
 		tags[k] = v
+		if k == userAgentField {


user agent does not need to be set in storage class, we only need to hard code in our driver or set as driver parameter in driver start up

@andyzhangx We need a way to track Azure cloud operations initiated by partner ISVs on behalf of another customer. This is done through the user agent strings as described in Azure customer usage attribution.

I would suggest add a new parameter(userAgent) instead of customer tags in storage class to track csi driver operations. if not set, use hard coded userAgent in driver, the implementation in this PR is too redudunt, here is a clean example: kubernetes-sigs/azurefile-csi-driver@ac18a0b

The logic that was behind this was that ISVs would need a specific storage class created that tags useragent on operations, but wouldn't want the rest of the operations to be tagged with the Useragent. Vybava is verifying what method makes the most sense, but this was the implementation we initially decided to go for

while current implementation would set useragent for all operations done by the driver since this whole driver would use only one useragent. Is there a customer asking for this feature? specify useragent in storage class(no matter it's tags or userAgent field) is not a good idea: 1) PV does not use this storage class would also get the same useragent 2) if driver restart, userAgent setting would be gone.

I created another PR to change default userAgent(#969), though it's not related to this scenario.

…trollerserver.go to use storageclass variable and include config from secret

edreed · 2021-08-24T17:20:28Z

pkg/azuredisk/controllerserver.go

+		case userAgentField:
+			nonUserAgentCloud = d.cloud
+			newUserAgent := v
+			d.cloud, err = SetupNewUserAgentCloud(newUserAgent, d.cloud)


We don't want to change the global instance of the cloud provider as we only want the custom user agent string used for the duration of this CreateVolume call and not for any others. Use a local variable initialized to d.cloud for the CreateVolume call and override the value here instead.

actually in Line 219, it already changes the global instance. It's hard to set multiple cloud provider instances for multiple userAgents unless we set a big lock in CreateVolume to block other create volume activities until it succeeds, while set a lock would bring performance issue.

we could support one userAgent setting in global instance in first stage, and then check how to support multiple userAgents settings in next stage, that also lgtm for me. Anyway, it's up to you.

And pls reuse GetCloudProvider func as possible as you can, don't duplicate code, thanks.

edreed · 2021-08-24T17:25:59Z

pkg/azuredisk/controllerserver.go

@@ -1219,3 +1241,139 @@ func getEntriesAndNextToken(req *csi.ListSnapshotsRequest, snapshots []compute.S

 	return listSnapshotResp, nil
 }
+
+// Creates a new cloud during CreateVolume call to configure the Useragent property of DiskClient
+func SetupNewUserAgentCloud(userAgent string, currentAz *azure.Cloud) (*azure.Cloud, error) {


Consider GetCloudProviderWithUserAgent and move this and supporting functions to same file as GetCloudProvider. (This is currently pkg/azuredisk/azure.go, but #977 will be moving it to pkg/azureutils/azure_disk_utils.go.)

pls reuse existing functions, and don't duplicate functions, code duplication would make it difficult to maintain

edreed · 2021-08-24T17:28:22Z

vendor/sigs.k8s.io/cloud-provider-azure/pkg/provider/azure_backoff.go

@@ -384,7 +384,7 @@ func (az *Cloud) DeletePublicIP(service *v1.Service, pipResourceGroup string, pi
 }

 // DeleteLB invokes az.LoadBalancerClient.Delete with exponential backoff retry
-func (az *Cloud) DeleteLB(service *v1.Service, lbName string) error {


You have updates to the vendor directory, but I don't see any corresponding updates to go.mod. Are you missing this file in the PR?

I must have forgotten to add go.mod. When I updated the vendors, it changed almost all of the packages so I was trying to just include the ones relevant. I'll fix that

vendor change was already done in previous PR, just rebase to current master branch

pkg/azuredisk/controllerserver.go

andyzhangx

/ok-to-test

k8s-ci-robot · 2021-08-25T14:37:42Z

@kassarl: The following test failed, say /retest to rerun all failed tests or /retest-required to rerun all mandatory failed tests:

Test name	Commit	Details	Rerun command
pull-azuredisk-csi-driver-verify	`092b41d`	link	`/test pull-azuredisk-csi-driver-verify`

Full PR test history. Your PR dashboard. Please help us cut down on flakes by linking to an open issue when you hit one in your PR.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. I understand the commands that are listed here.

k8s-ci-robot · 2021-08-26T22:56:28Z

@kassarl: PR needs rebase.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

k8s-ci-robot added the size/M Denotes a PR that changes 30-99 lines, ignoring generated files. label Aug 5, 2021

k8s-ci-robot requested review from andyzhangx and ZeroMagic August 5, 2021 16:26

Created preliminary useragent cloud creation

d77ab72

kassarl force-pushed the useragent-support branch from 9ed1908 to d77ab72 Compare August 5, 2021 17:20

kassarl commented Aug 5, 2021

View reviewed changes

pkg/azuredisk/controllerserver.go Show resolved Hide resolved

andyzhangx reviewed Aug 7, 2021

View reviewed changes

Update cloud-provider-azure vendor

d0fc226

Updated vendor pkg for cloud-provider-azure/pkg/provider, updated con…

092b41d

…trollerserver.go to use storageclass variable and include config from secret

kassarl force-pushed the useragent-support branch from 15e89e2 to 092b41d Compare August 17, 2021 00:21

kassarl marked this pull request as ready for review August 24, 2021 15:22

k8s-ci-robot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Aug 24, 2021

edreed reviewed Aug 24, 2021

View reviewed changes

andyzhangx reviewed Aug 25, 2021

View reviewed changes

pkg/azuredisk/controllerserver.go Show resolved Hide resolved

andyzhangx reviewed Aug 25, 2021

View reviewed changes

k8s-ci-robot added ok-to-test Indicates a non-member PR verified by an org member that is safe to test. and removed needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. labels Aug 25, 2021

k8s-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Aug 26, 2021

kassarl closed this Sep 1, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Support UserAgent Attribution #962

Support UserAgent Attribution #962

kassarl commented Aug 5, 2021

k8s-ci-robot commented Aug 5, 2021

k8s-ci-robot commented Aug 5, 2021

andyzhangx left a comment

andyzhangx Aug 7, 2021

edreed Aug 9, 2021

andyzhangx Aug 10, 2021 •

edited

Loading

kassarl Aug 12, 2021

andyzhangx Aug 12, 2021

andyzhangx Aug 12, 2021

This comment was marked as off-topic.

edreed Aug 24, 2021

andyzhangx Aug 29, 2021

andyzhangx Aug 31, 2021

edreed Aug 24, 2021

andyzhangx Aug 25, 2021

edreed Aug 24, 2021

kassarl Aug 24, 2021

andyzhangx Aug 25, 2021

andyzhangx left a comment

k8s-ci-robot commented Aug 25, 2021

k8s-ci-robot commented Aug 26, 2021

Support UserAgent Attribution #962

Support UserAgent Attribution #962

Conversation

kassarl commented Aug 5, 2021

k8s-ci-robot commented Aug 5, 2021

k8s-ci-robot commented Aug 5, 2021

andyzhangx left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

andyzhangx Aug 10, 2021 • edited Loading

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

This comment was marked as off-topic.

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

andyzhangx left a comment

Choose a reason for hiding this comment

k8s-ci-robot commented Aug 25, 2021

k8s-ci-robot commented Aug 26, 2021

andyzhangx Aug 10, 2021 •

edited

Loading