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

Support UserAgent Attribution #962

Closed
wants to merge 3 commits into from
Closed
Show file tree
Hide file tree
Changes from 1 commit
Commits
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
1 change: 1 addition & 0 deletions pkg/azuredisk/azuredisk.go
Original file line number Diff line number Diff line change
Expand Up @@ -98,6 +98,7 @@ const (
networkAccessPolicyField = "networkaccesspolicy"
diskAccessIDField = "diskaccessid"
enableBurstingField = "enablebursting"
userAgentField = "useragent"

WellKnownTopologyKey = "topology.kubernetes.io/zone"
throttlingKey = "throttlingKey"
Expand Down
89 changes: 89 additions & 0 deletions pkg/azuredisk/controllerserver.go
Original file line number Diff line number Diff line change
Expand Up @@ -17,8 +17,13 @@ limitations under the License.
package azuredisk

import (
"bytes"
"context"
"encoding/json"
"fmt"
"io"
"io/ioutil"
"os"
"path"
"sort"
"strconv"
Expand All @@ -40,6 +45,7 @@ import (
"k8s.io/klog/v2"

"sigs.k8s.io/azuredisk-csi-driver/pkg/optimization"
"sigs.k8s.io/azuredisk-csi-driver/pkg/util"
volumehelper "sigs.k8s.io/azuredisk-csi-driver/pkg/util"
"sigs.k8s.io/cloud-provider-azure/pkg/metrics"
azure "sigs.k8s.io/cloud-provider-azure/pkg/provider"
Expand Down Expand Up @@ -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 {
Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Collaborator

Choose a reason for hiding this comment

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

@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.

Copy link
Member

@andyzhangx andyzhangx Aug 10, 2021

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

Copy link
Member

Choose a reason for hiding this comment

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

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.

Copy link
Member

Choose a reason for hiding this comment

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

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

This comment was marked as off-topic.

temp := d.cloud
d.cloud, err = SetupNewUserAgentCloud(v, d.cloud)
if err != nil {
klog.V(2).Infof("Unable to create new cloud for UserAgent, err: (%s)", err)
}
d.cloud = temp
}
}

if strings.EqualFold(writeAcceleratorEnabled, trueValue) {
Expand Down Expand Up @@ -1219,3 +1233,78 @@ func getEntriesAndNextToken(req *csi.ListSnapshotsRequest, snapshots []compute.S

return listSnapshotResp, nil
}

/// Creates a new cloud to configure the Useragent property of DiskClient
func SetupNewUserAgentCloud(userAgent string, currentAz *azure.Cloud) (*azure.Cloud, error) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

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.)

Copy link
Member

Choose a reason for hiding this comment

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

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

//TODO
az := &azure.Cloud{
InitSecretConfig: currentAz.InitSecretConfig,
}

//TODO how to check the kubeconfig to see if it is from secret or not?

credFile, ok := os.LookupEnv(DefaultAzureCredentialFileEnv)
if ok && strings.TrimSpace(credFile) != "" {
klog.V(2).Infof("%s env var set as %v", DefaultAzureCredentialFileEnv, credFile)
} else {
if util.IsWindowsOS() {
credFile = DefaultCredFilePathWindows
} else {
credFile = DefaultCredFilePathLinux
}

klog.V(2).Infof("use default %s env var: %v", DefaultAzureCredentialFileEnv, credFile)
}

var config io.Reader

config, err := UpdateConfigUserAgent(userAgent, credFile)
if err != nil {
klog.Errorf("updating azure config file(%s) with useragent(%s) failed with %v", credFile, userAgent, err)
return nil, fmt.Errorf("updating azure config file(%s) with useragent(%s) failed with %v", credFile, userAgent, err)
}

klog.V(2).Infof("read cloud config from file: %s and set useragent: %s successfully", credFile, userAgent)
if az, err = azure.NewCloudWithoutFeatureGates(config, false); err != nil {
return az, err
}

return az, nil
}

func UpdateConfigUserAgent(userAgent string, credFile string) (io.Reader, error) {
kassarl marked this conversation as resolved.
Show resolved Hide resolved
var configReader *os.File
configReader, err := os.Open(credFile)
if err != nil {
klog.Errorf("load azure config from file(%s) failed with %v", credFile, err)
return nil, err
}
var config azure.Config
if configReader == nil {
return nil, nil
}

configContents, err := ioutil.ReadAll(configReader)
if err != nil {
return nil, err
}

err = json.Unmarshal(configContents, &config)
if err != nil {
return nil, err
}

config.UserAgent = userAgent
kassarl marked this conversation as resolved.
Show resolved Hide resolved
newConfig, err := json.Marshal(config)
if err != nil {
return nil, err
}

var newReader io.Reader
buf := bytes.NewBuffer(newConfig)
newReader = buf

configReader.Close()

return newReader, nil
}