add --arm-endpoint and --aad-authority flags to grafana ctl to support FF#240
Open
Mustafa-Ali-code wants to merge 1 commit into
Open
add --arm-endpoint and --aad-authority flags to grafana ctl to support FF#240Mustafa-Ali-code wants to merge 1 commit into
Mustafa-Ali-code wants to merge 1 commit into
Conversation
There was a problem hiding this comment.
Pull request overview
This PR updates grafanactl to support sovereign clouds (e.g., Fairfax) by allowing callers to override the ARM endpoint and AAD authority used by Azure SDK clients/credentials, instead of implicitly defaulting to Public Azure endpoints.
Changes:
- Add
--arm-endpointand--aad-authorityflags, validate they’re set together, and resolve them into an Azure SDKcloud.Configuration. - Plumb resolved cloud configuration into Azure ARM clients (
armdashboard,armmonitor) via*arm.ClientOptions, and into auth viaDefaultAzureCredentialoptions. - Document the new flags in
tools/grafanactl/README.md.
Reviewed changes
Copilot reviewed 9 out of 9 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
| tools/grafanactl/README.md | Documents sovereign cloud usage and the two new flags. |
| tools/grafanactl/internal/azure/monitor.go | Allows passing ARM ClientOptions into Monitor Workspace ARM client construction. |
| tools/grafanactl/internal/azure/grafana.go | Allows passing ARM ClientOptions into Managed Grafana ARM client construction. |
| tools/grafanactl/cmd/sync/options.go | Threads resolved cloud config into credential + ARM client options for sync. |
| tools/grafanactl/cmd/modify/options.go | Threads resolved cloud config into credential + ARM client options for modify. |
| tools/grafanactl/cmd/list/options.go | Threads resolved cloud config into credential + ARM client options for list. |
| tools/grafanactl/cmd/clean/options.go | Threads resolved cloud config into credential + ARM client options for clean. |
| tools/grafanactl/cmd/base/options.go | Adds new flags, validates/normalizes endpoints, and produces completed cloud configuration + ARM client options helper. |
| tools/cmdutils/auth.go | Adds cloud-aware credential creation for non-GitHub Actions execution path. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
+158
to
+159
|
|
||
| return endpoint, nil |
| func NewManagedGrafanaClient(subscriptionID string, cred azcore.TokenCredential, clientOptions *arm.ClientOptions) (*ManagedGrafanaClient, error) { | ||
| grafanaClient, err := armdashboard.NewGrafanaClient(subscriptionID, cred, clientOptions) | ||
| if err != nil { | ||
| return nil, fmt.Errorf("failed to create Azure Monitor Workspaces client: %w", err) |
| monitorWorkspaceClient, err := azure.NewMonitorWorkspaceClient(o.SubscriptionID, cred) | ||
| monitorWorkspaceClient, err := azure.NewMonitorWorkspaceClient(o.SubscriptionID, cred, clientOpts) | ||
| if err != nil { | ||
| return nil, fmt.Errorf("failed to create managed Prometheus client: %w", err) |
Comment on lines
+21
to
27
| // GetAzureTokenCredentialsForCloud returns an Azure TokenCredential configured for | ||
| // a specific Azure cloud. The cloud config determines the AAD authority host used | ||
| // when requesting tokens. | ||
| func GetAzureTokenCredentialsForCloud(cloudConfig cloud.Configuration) (azcore.TokenCredential, error) { | ||
| if _, ok := os.LookupEnv("GITHUB_ACTIONS"); ok { | ||
| return azidentity.NewAzureCLICredential(nil) | ||
| } |
| - `--output` - Output format: `table` (default) or `json` | ||
| - `-v, --verbosity` - Set logging verbosity level (0-10) | ||
|
|
||
| For sovereign clouds (e.g. Fairfax ), pass the ARM endpoint and AAD |
| return "", fmt.Errorf("endpoint cannot be empty") | ||
| } | ||
|
|
||
| if !strings.Contains(endpoint, "://") { |
Collaborator
There was a problem hiding this comment.
Maybe a silly question. Is there a reason this is string.Contains "://", rather than startswith "https://"?
Collaborator
Author
There was a problem hiding this comment.
It was a non-valid assumption that "http" might be passed instead of "https". I changed it to HasPrefix "https://". Thanks for noticing that!
Mustafa-Ali-code
force-pushed
the
mustaali/addArmEndpointFlag
branch
from
4d77bbf to
55223ff
Compare
…t ff for classic Signed-off-by: Mustafa Ali <mustafaadel16010@gmail.com>
Mustafa-Ali-code
force-pushed
the
mustaali/addArmEndpointFlag
branch
from
55223ff to
3ffa5c4
Compare
| - `--output` - Output format: `table` (default) or `json` | ||
| - `-v, --verbosity` - Set logging verbosity level (0-10) | ||
|
|
||
| For sovereign clouds (e.g. Fairfax ), pass the ARM endpoint and AAD |
| func NewManagedGrafanaClient(subscriptionID string, cred azcore.TokenCredential, clientOptions *arm.ClientOptions) (*ManagedGrafanaClient, error) { | ||
| grafanaClient, err := armdashboard.NewGrafanaClient(subscriptionID, cred, clientOptions) | ||
| if err != nil { | ||
| return nil, fmt.Errorf("failed to create Azure Monitor Workspaces client: %w", err) |
| } | ||
| if parsed.Host == "" { | ||
| return "", fmt.Errorf("endpoint host cannot be empty") | ||
| } |
Comment on lines
+24
to
27
| func GetAzureTokenCredentialsForCloud(cloudConfig cloud.Configuration) (azcore.TokenCredential, error) { | ||
| if _, ok := os.LookupEnv("GITHUB_ACTIONS"); ok { | ||
| return azidentity.NewAzureCLICredential(nil) | ||
| } |
rachelvweber
approved these changes
May 26, 2026
janboll
approved these changes
May 27, 2026
janboll
reviewed
May 27, 2026
| // URL-form value with the https scheme. EV2 central config typically stores | ||
| // endpoint DNS values as hostnames, so we prepend the scheme when it is missing | ||
| // to make the input forgiving for callers. | ||
| func normalizeEndpoint(endpoint string) (string, error) { |
Collaborator
There was a problem hiding this comment.
could add unit test for this
janboll
reviewed
May 27, 2026
| // and AAD authority. When both inputs are empty, the public cloud configuration is | ||
| // returned. When both are set, a custom configuration is built. Each input may be | ||
| // a hostname or a full URL (see normalizeEndpoint). | ||
| func resolveCloudConfig(armEndpoint, aadAuthority string) (cloud.Configuration, error) { |
Collaborator
There was a problem hiding this comment.
could add unit test for this
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Why?
grafanactldoes not specify a cloud configuration when constructing its Azure clients and credential, it passesnilforClientOptions, which causes the Azure SDK to fall back to its public cloud defaults (management.azure.comfor ARM,login.microsoftonline.comfor AAD). This makes the tool not work in FF, where it fails withSubscriptionNotFoundbecause Gov subscriptions don't exist on those endpoints.This blocks Azure Red Hat OpenShift Classic rollouts to Fairfax, which use
grafanactlvia theGrafanaDashboardsandGrafanaDatasourcespipeline actions.What?
Adds two new optional flags to all
grafanactlsubcommands:--arm-endpointAzure Resource Manager endpoint for the target cloud--aad-authorityMicrosoft Entra ID authority for the target cloudBoth flags must be set together (or both omitted). When omitted, the tool defaults to the public Azure cloud, same behavior as before.
The values are passed through to:
DefaultAzureCredentialviaClientOptions.Cloud) so AAD token requests go to the correct authorityarmdashboard,armmonitor) so resource lookups hit the correct endpointEndpoints accept either bare hostnames (
management.usgovcloudapi.net) or full URLs (https://management.usgovcloudapi.net). Hostnames are normalized to URL form internally.