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

Added the capability to add custom endpoints for storage and azure active directory endpoint #195

Merged
merged 1 commit into from
Aug 18, 2023

Conversation

Jeremy-Boyle
Copy link
Contributor

@Jeremy-Boyle Jeremy-Boyle commented Jun 19, 2023

Why:
This feature pull request allows the user to define the storage endpoints and azure active directory endpoints. This is needed for azure gov / other regions where the directory and storage endpoints aren't public and do not fall under the default provided clouds.

Additions:

  • Go Int Tests to verify that the output is as expected
  • Added new backupstoragelocation config values
    • activeDirectoryAuthorityURI
  • Added new volumesnapshotlocation config values
    • activeDirectoryAuthorityURI

@shubham-pampattiwar
Copy link
Collaborator

@Jeremy-Boyle changes look same to me, maybe move the error handling to the init() function. Also, Do we need any other env vars that might be needed for such usecases ?
@ywk253100 thoughts ?

@Jeremy-Boyle
Copy link
Contributor Author

I can take a look and see if there is any other use cases that might need to be added. As far as I’m aware this would allow for the custom endpoints that velero needs.

I can look into updating the error handling to a more appropriate place.

@Jeremy-Boyle
Copy link
Contributor Author

Jeremy-Boyle commented Jun 22, 2023

@shubham-pampattiwar

I see the call here in init,

cloudConfig, err := cloudFromName(os.Getenv(cloudNameEnvVar))
if err != nil {
return errors.Wrap(err, "unable to parse azure cloud name environment variable")
}

However, since its contingent on the flag AZURE_CLOUD_NAME being set to AzureCustomCloud, the current solution may best otherwise additional logic would need to be setup in init to verify that AZURE_CLOUD_NAME = AzureCustomCloud and then check the environment variables before calling the cloudFromName function.

Any suggestions?

@codecov-commenter
Copy link

codecov-commenter commented Jun 22, 2023

Codecov Report

Merging #195 (6e3d49b) into main (98ec8ec) will decrease coverage by 0.14%.
The diff coverage is 0.00%.

@@            Coverage Diff             @@
##             main     #195      +/-   ##
==========================================
- Coverage   13.60%   13.46%   -0.14%     
==========================================
  Files           4        4              
  Lines         691      698       +7     
==========================================
  Hits           94       94              
- Misses        594      601       +7     
  Partials        3        3              
Files Changed Coverage Δ
velero-plugin-for-microsoft-azure/object_store.go 2.18% <0.00%> (-0.03%) ⬇️
...o-plugin-for-microsoft-azure/volume_snapshotter.go 23.45% <0.00%> (-0.24%) ⬇️

@ywk253100
Copy link
Contributor

ywk253100 commented Jun 27, 2023

@Jeremy-Boyle @shubham-pampattiwar
We introduced a new configuration item storageAccountURI for users to customize the storage account endpoint in PR #185, seems the changes in this PR contain overlap with that.
So how about that we introduce a new item activeDirectoryAuthorityHost to overwrite the default value if provided?

cc @anshulahuja98

@Jeremy-Boyle
Copy link
Contributor Author

storageAccountURI

This is a much better route, I will update this PR to fall in line.

@Jeremy-Boyle
Copy link
Contributor Author

@ywk253100 Updated based on your recommendations.

@Jeremy-Boyle
Copy link
Contributor Author

@ywk253100 @shubham-pampattiwar @aramprice

Any movement ?

Would like to get this updated ASAP

@ywk253100
Copy link
Contributor

@Jeremy-Boyle Will review it later

@Jeremy-Boyle
Copy link
Contributor Author

Please take a look at let me know @ywk253100

@Jeremy-Boyle Jeremy-Boyle force-pushed the feature/custom-endpoints branch 2 times, most recently from d729eec to 2b9ddf5 Compare August 2, 2023 14:07
@Jeremy-Boyle
Copy link
Contributor Author

Fixed Pull, there was some weird commits that were changing too much from the readme file and some merge conflicts, squashed into one commit.

Looks good now

@Jeremy-Boyle Jeremy-Boyle force-pushed the feature/custom-endpoints branch from 2b9ddf5 to 2faf911 Compare August 2, 2023 14:17
@Jeremy-Boyle
Copy link
Contributor Author

@ywk253100 @shubham-pampattiwar @aramprice

Does this PR look good now ? Would like to pull these changes in, thanks!

@ywk253100 ywk253100 merged commit 0a3adf7 into vmware-tanzu:main Aug 18, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants