Don't use global credentials in low-level methods #1309
Merged
Conversation
This file contains 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
These are now retrieved using config.GlobalCredentials(), which will eventually be pushed further and further up the call stack, to make implementing flexible auth easier.
All functions that used the global credentials now require creds passed in - all callers are updated to pass in the global credentials. Eventually this will push all creds-getting up to the level of the reconcile loop so that we can change it to get them from somewhere else.
This allows tests to pass their own credentials without having to set the global ones.
babbageclunk
force-pushed
the
creds-refactor
branch
from
21a6d52 to
be3fa93
Compare
...Other than in tests.
babbageclunk
force-pushed
the
creds-refactor
branch
from
7b2d1ce to
b1a1c38
Compare
...Other than in tests. Credentials are now passed in when creating a Manager or APIMgmtServiceManager.
babbageclunk
force-pushed
the
creds-refactor
branch
6 times, most recently
from
0347fd8 to
14d3b5f
Compare
Credentials are now passed in when constructing a Manager.
Credentials are now passed in when constructing a manager.
Credentials are now passed in when constructing a Manager.
Credentials are now passed in when constructing Managers.
...Other than in tests. Credentials are now passed in when constructing a manager. Eliminate global KeyVaultManager used in tests - now it's constructed with credentials in the affected tests instead.
Credentials are now passed in when constructing a Client.
Credentials are now passed in when constructing managers/clients.
Credentials are now passed in when constructing the Client.
Credentials are now passed in when constructing a Client.
Credentials are now passed in when constructing a PollClient.
Credentials are now passed in when constructing a client or manager.
Credentials are now passed in when constructing managers.
Credentials are now passed in when constructing a manager. Converted package level functions ListGroups, GetGroup, DeleteAllGroupsWithPrefix and WaitForDeleteCompletion into methods on AzureResourceGroupManager. There didn't seem to be a reason that they were functions when they would now need credentials.
Credentials are now passed in when constructing a manager.
Credentials are now passed in when constructing the client.
Credentials are now passed in when constructing a client.
Credentials are now passed in when a client is constructed.
babbageclunk
force-pushed
the
creds-refactor
branch
from
fb4999a to
8b47a7f
Compare
Credentials are now passed in when constructing a manager.
babbageclunk
force-pushed
the
creds-refactor
branch
from
8b47a7f to
750f3e8
Compare
babbageclunk
changed the title
babbageclunk
requested review from
davefellows,
frodopwns,
jananivMS,
matthchr,
Porges,
theunrepentantgeek and
WilliamMortlMicrosoft
as code owners
matthchr
previously approved these changes
Nov 12, 2020
| // TODO(creds-refactor): can we just use the tenant and client id from | ||
| // the creds here? Depends whether the client code uses them to | ||
| // override the creds. | ||
| func getObjectID(ctx context.Context, creds config.Credentials, tenantID string, clientID string) (*string, error) { |
There was a problem hiding this comment.
This function is used in 2 places. One of those places is CreateVaultWithAccessPolicies which as far as I can tell is only used in tests... may be worth moving to tests directly if that's really a test helper method not used at all in production code.
The other location is ParseAccessPolicy which seems to use a clientId/tenantId other than the one in the credentials (supplied by the user?)
There was a problem hiding this comment.
I've removed the comment - I've left the method for now even though it's used from tests. If it was only used in the tests for keyvault I'd move it, but it's kind of tricky to share testing code between two packages.
| @@ -13,8 +13,6 @@ import ( | |||
| "github.com/Azure/azure-service-operator/pkg/resourcemanager" | |||
| ) | |||
|
|
|||
| var AzureKeyVaultManager KeyVaultManager = &azureKeyVaultManager{} | |||
There was a problem hiding this comment.
Instead just do var _ KeyVaultManager = &azureKeyVaultManager{} seems reasonable?
There was a problem hiding this comment.
This was being used by code in other packages - I needed to remove the global variable because it needs to have creds when it's constructed. I'll reinstate the interface assertion part though!
2 tasks
matthchr
approved these changes
Nov 16, 2020
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.
This is a refactoring to pull the use of global credentials up to the level of the reconcile loop for resources to enable switching where we look for credentials in a future PR. This is part of implementing #1173 based on the work in #1206.
This PR doesn't change the behaviour of the operator - the controller loops will still use global credentials. Making this mechanical change first in a separate PR to simplify reviewing.
Tips for reviewers - the first three commits are the most interesting ones, the others are basically just mechanical. Although the resource groups one was slightly fiddlier than the others.
How does this PR make you feel:
