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

[receiver/azuremonitor]: Support Azure gov cloud #27573

Merged
merged 6 commits into from
Nov 17, 2023

Conversation

danelson
Copy link
Contributor

@danelson danelson commented Oct 10, 2023

Description:
Support Azure gov cloud

Link to tracking Issue: N/A

Testing: Ran against a government subscription

Documentation: Updated README

@@ -20,6 +20,10 @@ var (
errMissingClientID = errors.New(`ClientID" is not specified in config`)
errMissingClientSecret = errors.New(`ClientSecret" is not specified in config`)
errMissingFedTokenFile = errors.New(`FederatedTokenFile is not specified in config`)
errInvalidCloud = errors.New(`Cloud" is invalid`)

azureCloud = "AzureCloud"
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I am not sure these are the preferred names, but these are the names returned from running az cloud list. Other tools use different conventions (e.g. terraform).

@github-actions
Copy link
Contributor

This PR was marked stale due to lack of activity. It will be closed in 14 days.

@github-actions github-actions bot added the Stale label Oct 24, 2023
@danelson
Copy link
Contributor Author

@codeboten @altuner can I get some feedback on this?

@@ -18,6 +18,7 @@ import (

const (
defaultCollectionInterval = 10 * time.Second
defaultCloud = "AzureCloud"
Copy link
Contributor

Choose a reason for hiding this comment

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

please use const "azureCloud" from config here

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Had to move config vars to constants. Let me know if that doesn't work

@@ -45,6 +45,7 @@ func TestNewFactory(t *testing.T) {
CacheResourcesDefinitions: 24 * 60 * 60,
MaximumNumberOfMetricsInACall: 20,
Authentication: servicePrincipal,
Cloud: azureCloud,
Copy link
Contributor

Choose a reason for hiding this comment

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

should be defaultCloud?

```yaml
receivers:
azuremonitor:
subscription_id: "${subscription_id}"
tenant_id: "${tenant_id}"
client_id: "${client_id}"
client_secret: "${env:CLIENT_SECRET}"
cloud: AzureUSGovernment
Copy link
Contributor

Choose a reason for hiding this comment

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

should we put here default one?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Not sure. I wouldn't expect users to include this line at all when configuring the public cloud. They can also infer this from the bullets above but I thought a more complete example was nice. Happy to use the default tho if you think that makes sense.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@altuner just following up here and with the other changes.

@github-actions github-actions bot removed the Stale label Oct 27, 2023
@danelson danelson requested a review from altuner October 30, 2023 20:49
@danelson
Copy link
Contributor Author

@altuner @codeboten sorry to keep pinging but I would love to move this along so that I can use it in my collector. Anything I can do to make that happen please let me know!

Copy link
Contributor

@codeboten codeboten left a comment

Choose a reason for hiding this comment

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

See the suggestion

        	            	type "Config" from package "github.com/open-telemetry/opentelemetry-collector-contrib/receiver/azuremonitorreceiver" has invalid config settings: field "Cloud" has config tag "" which doesn't satisfy "^[a-z0-9][a-z0-9_]*$"

receiver/azuremonitorreceiver/config.go Outdated Show resolved Hide resolved
Co-authored-by: Alex Boten <[email protected]>
@codeboten codeboten merged commit 1934729 into open-telemetry:main Nov 17, 2023
75 checks passed
@github-actions github-actions bot added this to the next release milestone Nov 17, 2023
@danelson danelson deleted the azuremonitor-receiver-gov branch November 17, 2023 17:35
aishyandapalli pushed a commit to aishyandapalli/opentelemetry-collector-contrib that referenced this pull request Nov 17, 2023
Support Azure gov cloud

**Testing:** Ran against a government subscription

---------

Co-authored-by: Alex Boten <[email protected]>
RoryCrispin pushed a commit to ClickHouse/opentelemetry-collector-contrib that referenced this pull request Nov 24, 2023
Support Azure gov cloud

**Testing:** Ran against a government subscription

---------

Co-authored-by: Alex Boten <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants