-
Notifications
You must be signed in to change notification settings - Fork 4k
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
allow customizing user agent for Azure cluster-autoscaler provider #7033
Conversation
/assign @comtalyst |
@nojnhuh: GitHub didn't allow me to assign the following users: comtalyst. Note that only kubernetes members with read permissions, repo collaborators and people who have commented on this issue/PR can be assigned. Additionally, issues/PRs can only have 10 assignees at the same time. In response to this:
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-sigs/prow repository. |
@@ -244,7 +245,8 @@ func decodePkcs12(pkcs []byte, password string) (*x509.Certificate, *rsa.Private | |||
} | |||
|
|||
func getUserAgentExtension() string { | |||
return fmt.Sprintf("cluster-autoscaler/v%s", version.ClusterAutoscalerVersion) | |||
suffix := os.Getenv("AZURE_CLUSTER_AUTOSCALER_USER_AGENT_SUFFIX") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ideally, I think all configuration inputs should be in or near here: https://github.com/kubernetes/autoscaler/blob/master/cluster-autoscaler/cloudprovider/azure/azure_config.go.
Although, that place is not in the best state right now. One current problem is that the environment variables there won't get parsed if the cloud config file exists, even if that cloud config file doesn't contain that variable (look at the if statements there).
So, it is okay to leave this here for now, I think.
@@ -244,7 +245,8 @@ func decodePkcs12(pkcs []byte, password string) (*x509.Certificate, *rsa.Private | |||
} | |||
|
|||
func getUserAgentExtension() string { | |||
return fmt.Sprintf("cluster-autoscaler/v%s", version.ClusterAutoscalerVersion) | |||
suffix := os.Getenv("AZURE_CLUSTER_AUTOSCALER_USER_AGENT_SUFFIX") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do you think there should be some validation (e.g., no spaces/symbols, begin with '-') for this?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I can't think of any way this would obviously break with a weird input. Validation might be a reasonable follow-up though.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM. Let's also cherry-pick/make the same changes in 1.27-1.30.
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: comtalyst, nojnhuh, tallaxes The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
/cherry-pick cluster-autoscaler-release-1.30 |
@nojnhuh: once the present PR merges, I will cherry-pick it on top of cluster-autoscaler-release-1.30 in a new PR and assign it to you. In response to this:
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-sigs/prow repository. |
@tallaxes Could you please also give this a |
/lgtm |
@nojnhuh: new pull request created: #7053 In response to this:
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-sigs/prow repository. |
@nojnhuh: new pull request created: #7054 In response to this:
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-sigs/prow repository. |
@nojnhuh: new pull request created: #7055 In response to this:
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-sigs/prow repository. |
@nojnhuh: new pull request created: #7056 In response to this:
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-sigs/prow repository. |
What type of PR is this?
/kind feature
What this PR does / why we need it:
This PR allows for customizing the HTTP user agent header for cluster-autoscaler's Azure provider using an environment variable.
Installing the Helm chart with this:
Results in a user agent like this:
Which issue(s) this PR fixes:
Fixes #
Special notes for your reviewer:
Does this PR introduce a user-facing change?
Additional documentation e.g., KEPs (Kubernetes Enhancement Proposals), usage docs, etc.: