Skip to content

Conversation

@schaafito
Copy link

@schaafito schaafito commented Feb 28, 2025

Thank you for contributing to Velero!

Please add a summary of your change

Added support for AzureStackHub and private Azure cloud environments by updating the getCloudConfiguration() logic to fall-through to the kubernetes cloud-provider-azure module logic for identifying private cloud environments. This fall-through supports both AZURE_RESOURCE_MANAGER_ENDPOINT cloud discovery as well as AZURE_ENVIRONMENT_FILEPATH discovery.

The private cloud logic can (partially) be tested on any Azure environment by setting the AZURE_CLOUD_NAME to AZURESTACKCLOUD and providing a valid azurestackcloud.json configuration file as documented here. You can set the cloud endpoints to valid endpoints for a public cloud offering to test fallthrough logic.

The bulk of the documentation updates will need to be done on the velero-plugin-for-microsoft-azure project which I plan during the PR cycle to recompile the plugin with these changes. The proposed documentation changes can be seen on my branch

I provided unit tests for both endpoint and filepath cloud discovery, as well as tested in my own Azure environments to confirm functionality.

Note: This feature requires golang1.23 support so I bumped those as well. I see an open PR #8717 which I suspect will need to be merged before this.

Does your change fix a particular issue?

Fixes #3159

Please indicate you've done the following:

@github-actions github-actions bot requested review from sseago and ywk253100 February 28, 2025 11:13
@github-actions github-actions bot added Dependencies Pull requests that update a dependency file has-unit-tests labels Feb 28, 2025
@schaafito schaafito changed the title Issuer 3159: Added support for AzureStackHub and Azure private cloud identification Issue 3159: Added support for AzureStackHub and Azure private cloud identification Feb 28, 2025
Signed-off-by: Ryan Schaaf <[email protected]>
@anshulahuja98
Copy link
Collaborator

@ywk253100 do you have any additional thoughts on this PR?
Overall looks okay to me. I don't have a test environment ready to test this right now.
But I don't anticipate this breaking existing scenarios.

CredentialKeyPassword = "AZURE_PASSWORD" // #nosec

credentialFile = "credentialsFile"
CredentialResourceManagerEndpoint = "AZURE_RESOURCE_MANAGER_ENDPOINT" // #nosec
Copy link
Contributor

Choose a reason for hiding this comment

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

Let's rename it as CredentialKeyResourceManagerEndpoint to follow the same pattern with other variables.

Copy link
Author

Choose a reason for hiding this comment

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

Done!

@ywk253100
Copy link
Contributor

@ywk253100 do you have any additional thoughts on this PR? Overall looks okay to me. I don't have a test environment ready to test this right now. But I don't anticipate this breaking existing scenarios.

I'm OK to the changes.
But let's merge it after 1.16 if it's not a high priority.

module github.com/vmware-tanzu/velero

go 1.23.0
go 1.23.1
Copy link
Collaborator

Choose a reason for hiding this comment

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

This is coming from kubernetes-sigs/cloud-provider-azure@5548c42 ("sigs.k8s.io/cloud-provider-azure/pkg/azclient")

Filed issue there to hopefully starve off future unnecessary bumps.

@codecov
Copy link

codecov bot commented Mar 19, 2025

Codecov Report

❌ Patch coverage is 83.33333% with 3 lines in your changes missing coverage. Please review.
✅ Project coverage is 59.58%. Comparing base (d086cb2) to head (6bfb6ae).
⚠️ Report is 261 commits behind head on main.

Files with missing lines Patch % Lines
pkg/util/azure/util.go 83.33% 2 Missing and 1 partial ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #8738      +/-   ##
==========================================
+ Coverage   59.56%   59.58%   +0.01%     
==========================================
  Files         370      370              
  Lines       40229    40246      +17     
==========================================
+ Hits        23964    23981      +17     
  Misses      14766    14766              
  Partials     1499     1499              

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@schaafito
Copy link
Author

schaafito commented Mar 24, 2025

@anshulahuja98 - I was able to take this branch to an AzureStackHub environment and unfortunately the APIVersion flag didn't work as expected due to the microsoft golang SDK. I opened an issue but until they fix that in the upstream for this to be portable to those disconnected environments we would need a small policy_api_version.go esque implementation. Any concerns if I basically take my sample posted in that issue and implement it within velero until the upstream SDK is updated?

@anshulahuja98
Copy link
Collaborator

@schaafito can we consider using an older SDK version? Or do we need to the latest SDK with older API version.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Dependencies Pull requests that update a dependency file has-changelog has-unit-tests

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Enable use with Azure Stack

4 participants