Skip to content
This repository was archived by the owner on Nov 27, 2023. It is now read-only.

Add Azure sovereign cloud support#1255

Merged
gtardif merged 1 commit intodocker-archive:mainfrom
karolz-ms:658-azure-sovereign-clouds
Feb 25, 2021
Merged

Add Azure sovereign cloud support#1255
gtardif merged 1 commit intodocker-archive:mainfrom
karolz-ms:658-azure-sovereign-clouds

Conversation

@karolz-ms
Copy link
Copy Markdown
Contributor

@karolz-ms karolz-ms commented Feb 8, 2021

Signed-off-by: Karol Zadora-Przylecki karolz@microsoft.com

What I did
Added a notion of "cloud environment" for Azure integration. This allows Docker CLI to work with "sovereign" clouds like Azure China, Azure US Government etc.

Related issue
Fixes #658

This PR does not include documentation changes

This PR does not add new E2E (integration) tests--I was not sure if you want to have any that target non-public Azure clouds. Even for teams inside Microsoft it is not always the case that we have automated tests targeting non-public clouds, so I think unit tests are good enough for a start.

@gtardif gtardif self-assigned this Feb 9, 2021
@gtardif
Copy link
Copy Markdown
Contributor

gtardif commented Feb 9, 2021

Hi @karolz-ms, a couple of things that might help related to linter errors :

  • re no errcheck on defer statements, like these ones calling setenv, you can add a line above the defer satement // nolint errcheck, to avoid cumbersome code fitting into the defer statement
  • On the mock object, you need to defined your function with func (s *mockLoginService) (with a * for a pointer), to avoid pass by value.

@karolz-ms
Copy link
Copy Markdown
Contributor Author

Hi @karolz-ms, a couple of things that might help related to linter errors :

  • re no errcheck on defer statements, like these ones calling setenv, you can add a line above the defer satement // nolint errcheck, to avoid cumbersome code fitting into the defer statement
  • On the mock object, you need to defined your function with func (s *mockLoginService) (with a * for a pointer), to avoid pass by value.

Thanks, @gtardif Hopefully I got everything fixed with my last change.

I am now running make validate locally until it stops complaining--is there a better way to ensure that all code sanity checks are satisfied?

Copy link
Copy Markdown
Contributor

@gtardif gtardif left a comment

Choose a reason for hiding this comment

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

Mainly check if we can store the desired cloudEnvironment earlier in structs, rather than have a lazy compute.

Also, in terms of migration from previous versions, I tried to install this version and just run a command (without re-login), I received cannot get container group client: Cloud environment '' was not found.
We should assume azure public cloud by default if we can't find a previous cloud name in tokenInfo.

Comment thread .gitignore
Comment thread aci/login/cloud_environment.go Outdated
Comment thread aci/login/cloud_environment.go
Comment thread aci/login/helper.go
Comment thread aci/login/login.go
@gtardif
Copy link
Copy Markdown
Contributor

gtardif commented Feb 10, 2021

is there a better way to ensure that all code sanity checks are satisfied?

Looks all right with the linter. Sorry I added more remarks today (more useful regarding the actual PR content)
You'll need to sign your commits.
make validate lint should cover the same things as the CI, regarding formatting

@karolz-ms
Copy link
Copy Markdown
Contributor Author

I am working on suggested changes, many thanks @gtardif! Please hold off on reviewing again until I let you know the branch is ready.

Signed-off-by: Karol Zadora-Przylecki <karolz@microsoft.com>
@karolz-ms
Copy link
Copy Markdown
Contributor Author

@gtardif I think this should be better, please take another look. I did run the ACI E2E tests,

@gtardif
Copy link
Copy Markdown
Contributor

gtardif commented Feb 23, 2021

The only thing missing I think is setting public cloud as the default if we don't find any cloud name in the json file.
Trying to log in with a previous CLI, and installing this one on top, if I don't login again (which I should not need to do with refresh token), I get Cloud environment '' was not found.
(Users should not be impacted by this if they continue using the default cloud)

@karolz-ms
Copy link
Copy Markdown
Contributor Author

The only thing missing I think is setting public cloud as the default if we don't find any cloud name in the json file.
Trying to log in with a previous CLI, and installing this one on top, if I don't login again (which I should not need to do with refresh token), I get Cloud environment '' was not found.
(Users should not be impacted by this if they continue using the default cloud)

Hmm, I thought this addition would address this scenario, but apparently there is more to it. I will take a look

@gtardif
Copy link
Copy Markdown
Contributor

gtardif commented Feb 24, 2021

Hmm, I thought this addition would address this scenario, but apparently there is more to it

Indeed, that would seem to address this. I haven't debugged it to investigate more why this doesn't do the trick...

@karolz-ms
Copy link
Copy Markdown
Contributor Author

I will let you know what I find; I can probably figure it out on my own unless I cannot reproduce.

@karolz-ms
Copy link
Copy Markdown
Contributor Author

@gtardif so I cannot reproduce the problem. What I did:

  1. Create a context and log in to Azure using old CLI
  2. Verify the problem reproduces with the original submission code (I can see the cloud environment '' was not found error message)
  3. Verify the problem does not occur with the current submission. I also stepped through the code in the debugger and verified that the code path mentioned above is indeed hit.

Perhaps you were inadvertently running the older version of the code during your testing?

@gtardif
Copy link
Copy Markdown
Contributor

gtardif commented Feb 25, 2021

Oops sorry, my bad, I probably missed something when updating from your PR, and was running the previous code. Checked again, works all fine.

@gtardif gtardif merged commit 3420df6 into docker-archive:main Feb 25, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Please support Sovereign clouds like AzureUSGovernment

2 participants