-
Notifications
You must be signed in to change notification settings - Fork 441
OCPBUGS-74498: Add UserAgent telemetry to CPO Azure SDK clients #7620
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
OCPBUGS-74498: Add UserAgent telemetry to CPO Azure SDK clients #7620
Conversation
|
@hypershift-jira-solve-ci[bot]: This pull request references Jira Issue OCPBUGS-74498, which is valid. The bug has been moved to the POST state. 3 validation(s) were run on this bug
Requesting review from QA contact: The bug has been updated to refer to the pull request using the external bug tracker. DetailsIn 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 openshift-eng/jira-lifecycle-plugin repository. |
WalkthroughAdded cloud-aware Azure ARM client options and telemetry to Key Vault KMS client construction and resource validation. Extended Azure utility functions to accept a cloudName and create ARM clients with cloud-specific options. Added a unit test for the new ARM client options behavior. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes ✨ Finishing touches
🧪 Generate unit tests (beta)
Comment |
|
/auto-cc |
|
Hi @hypershift-jira-solve-ci[bot]. Thanks for your PR. I'm waiting for a openshift member to verify that this patch is reasonable to test. If it is, they should reply with Once the patch is verified, the new status will be reflected by the I understand the commands that are listed here. DetailsInstructions 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. |
| vaultURL := fmt.Sprintf("https://%s.%s", azureKmsSpec.ActiveKey.KeyVaultName, azureKeyVaultDNSSuffix) | ||
| keysClient, err := azkeys.NewClient(vaultURL, cred, nil) | ||
| keysClient, err := azkeys.NewClient(vaultURL, cred, &azkeys.ClientOptions{ |
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.
Why not use NewARMClientOptions here for the clientOptions?
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.
Good question! The NewARMClientOptions function returns *arm.ClientOptions, which is specifically designed for Azure Resource Manager (ARM) clients like armnetwork, armresources, etc.
However, azkeys.NewClient expects *azkeys.ClientOptions, which is a different type - it's for Key Vault clients rather than ARM clients. While both types embed azcore.ClientOptions, they are distinct and not interchangeable:
arm.ClientOptions→ for ARM SDK clients (armnetwork.NewVirtualNetworksClient, etc.)azkeys.ClientOptions→ for Key Vault SDK clients (azkeys.NewClient)
The common configuration (cloud config and telemetry) is the same between them, but the container types differ.
I could create a similar helper function like NewKeyVaultClientOptions(cloudConfig) in the azureutil package to provide the same consistent pattern - would that be preferred? Alternatively, we could create a lower-level helper that returns azcore.ClientOptions which both ARM and KeyVault client options could embed.
AI-assisted response via Claude Code
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.
Oh no that's ok. I missed that part. No changes requested.
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.
Great, thanks for confirming! 👍
AI-assisted response via Claude Code
|
/assign @bryan-cox |
|
/ok-to-test |
|
/cherry-pick release-4.21 |
|
@celebdor: once the present PR merges, I will cherry-pick it on top of DetailsIn 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. |
|
/approve |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: bryan-cox, hypershift-jira-solve-ci[bot] The full list of commands accepted by this bot can be found here. The pull request process is described here DetailsNeeds approval from an approver in each of these files:
Approvers can indicate their approval by writing |
dbbaf31 to
4ef664b
Compare
|
/retest |
1 similar comment
|
/retest |
|
hypershift-jira-solve-ci - rebase the PR to fix the konflux issues |
The Control Plane Operator (CPO) was not setting the UserAgent in the
Azure SDK TelemetryOptions when creating Azure ARM SDK clients. This
meant Azure API requests from CPO did not include proper application
identification for request tracing and telemetry purposes.
This commit adds:
- CPOUserAgent constant ("hypershift-cpo") for application identification
- NewARMClientOptions helper function that creates Azure ARM client options
with proper cloud configuration and TelemetryOptions.ApplicationID set
- Updated GetVnetInfoFromVnetID, GetNetworkSecurityGroupInfo, and
GetResourceGroupInfo functions to accept a cloudName parameter and use
the new helper function for proper telemetry configuration
- Unit tests for the new NewARMClientOptions function
The ApplicationID is added to the User-Agent header of all Azure API
requests, enabling proper attribution of CPO requests in Azure
telemetry and monitoring.
Co-Authored-By: Claude (claude-opus-4-5) <noreply@anthropic.com>
Update the validateAzureKMSConfig function to include TelemetryOptions with CPOUserAgent when creating the Azure Key Vault keys client. This ensures that requests to Azure Key Vault from the CPO include the "hypershift-cpo" application identifier in the User-Agent header. Also updates calls to GetVnetInfoFromVnetID, GetNetworkSecurityGroupInfo, and GetResourceGroupInfo in verifyResourceGroupLocationsMatch to pass the cloud name parameter as required by the updated function signatures. Co-Authored-By: Claude (claude-opus-4-5) <noreply@anthropic.com>
4ef664b to
c279efc
Compare
|
@bryan-cox Done. Rebased the PR onto the latest AI-assisted response via Claude Code |
|
Done. Rebased onto the latest AI-assisted response via Claude Code |
|
hypershift-jira-solve-ci - the unit tests are failing. You need to fix this. |
|
/test review-agent-single-pr |
|
@bryan-cox I investigated the unit test failure. The failing test is This test connects to external registries ( The files changed by this PR ( A AI-assisted response via Claude Code |
|
/test unit |
jparrill
left a comment
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
|
/verified later @wewang58 |
|
@bryan-cox: This PR has been marked to be verified later by DetailsIn 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 openshift-eng/jira-lifecycle-plugin repository. |
|
/test verify-deps |
|
/test all |
|
/lgtm |
|
Scheduling required tests: Scheduling tests matching the |
|
@hypershift-jira-solve-ci[bot]: all tests passed! Full PR test history. Your PR dashboard. DetailsInstructions 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. I understand the commands that are listed here. |
|
@hypershift-jira-solve-ci[bot]: Jira Issue OCPBUGS-74498: All pull requests linked via external trackers have merged: This pull request has the DetailsIn 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 openshift-eng/jira-lifecycle-plugin repository. |
|
@celebdor: new pull request created: #7685 DetailsIn 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. |
|
@celebdor: #7620 failed to apply on top of branch "release-4.20": DetailsIn 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 this PR does / why we need it:
This PR adds the
TelemetryOptions.ApplicationIDto Azure SDK client options when the Control Plane Operator (CPO) creates Azure ARM SDK clients. This ensures Azure API requests from the CPO include proper application identification ("hypershift-cpo") in the User-Agent header for request tracing and telemetry purposes.Changes:
support/azureutil/azureutil.go:
CPOUserAgentconstant for application identificationNewARMClientOptionshelper function that creates Azure ARM client options with proper cloud configuration and telemetry settingsGetVnetInfoFromVnetID,GetNetworkSecurityGroupInfo, andGetResourceGroupInfofunctions to accept acloudNameparameter and use proper telemetry optionscontrol-plane-operator/controllers/hostedcontrolplane/hostedcontrolplane_controller.go:
validateAzureKMSConfigto include telemetry options when creating the Azure Key Vault keys clientverifyResourceGroupLocationsMatchto pass the cloud name to the updated azureutil functionssupport/azureutil/azureutil_test.go:
NewARMClientOptionsfunctionWhich issue(s) this PR fixes:
Fixes OCPBUGS-74498
Special notes for your reviewer:
Checklist:
🤖 Generated with Claude Code via
/jira-solve OCPBUGS-74498 origin --ciAlways review AI generated responses prior to use.