auxiliary tenant IDs in the authentication process #610
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
added 2 commits
March 1, 2025 07:02
…c test for auxtenant.
ianjensenisme
requested review from
mawasile,
mattdot,
leighatami,
polatengin and
eduardodfmex
as code owners
There was a problem hiding this comment.
PR Overview
This PR adds support for auxiliary tenant IDs to enhance the authentication process and updates the provider configuration accordingly.
- Added a function to extract list string values from configuration and environment variables.
- Updated authentication methods to pass auxiliary tenant IDs to underlying credential providers.
- Enhanced provider schema and configuration to incorporate auxiliary tenant IDs.
Reviewed Changes
| File | Description |
|---|---|
| internal/helpers/config.go | Added GetListStringValues function for extracting list values from config or env vars. |
| internal/api/client_test.go | Introduced a new unit test to verify auxiliary tenant IDs configuration. |
| internal/api/auth.go | Updated authentication methods to include auxiliary tenant IDs in credential options. |
| internal/provider/provider.go | Added "auxiliary_tenant_ids" to the provider schema and adjusted configuration for MSI. |
| internal/constants/constants.go | Introduced new environment variable constants for auxiliary tenant IDs. |
| internal/config/config.go | Extended configuration structs with the auxiliary tenant IDs field. |
| internal/services/environment/resource_environment.go | Removed an extra empty line. |
Copilot reviewed 8 out of 8 changed files in this pull request and generated no comments.
Comments suppressed due to low confidence (2)
internal/helpers/config.go:51
- [nitpick] Avoid shadowing the 'value' parameter by renaming the local variable (e.g., 'envVal') to improve clarity.
if value, ok := os.LookupEnv(k); ok && value != "" {
internal/provider/provider.go:312
- [nitpick] Consider refactoring the conversion of types.List to []string for clarity and efficiency; using a dedicated helper or built-in conversion (if available) might improve maintainability.
// TODO there has to be a better way than starting with the slice and converting it..
|
Would love reviewer input on the types.List-to-[]string debacle, as well as whether I can set up any more robust tests with our current test tenant resources. |
mattdot
reviewed
Mar 3, 2025
| @@ -45,6 +45,35 @@ func TestUnitApiClient_GetConfig(t *testing.T) { | |||
| } | |||
| } | |||
|
|
|||
| func TestUnitApiClient_AuxTenants_GetConfig(t *testing.T) { | |||
There was a problem hiding this comment.
I don't understand what you're testing for.
mattdot
changed the title
|
You are missing changie record |
mattdot
reviewed
Mar 3, 2025
Comment on lines
+311
to
+317
| // Convert the slice to an array | ||
| // TODO there has to be a better way than starting with the slice and converting it.. | ||
| auxiliaryTenantIDsList := make([]string, len(auxiliaryTenantIDs.Elements())) | ||
| for i, v := range auxiliaryTenantIDs.Elements() { | ||
| auxiliaryTenantIDsList[i] = v.String() | ||
| } | ||
| p.Config.AuxiliaryTenantIDs = auxiliaryTenantIDsList |
There was a problem hiding this comment.
Does this work?
var tenantIDs []string
diags := auxiliaryTenantIDs.ValueAs(ctx, &tenantIDs)
if diags.HasError() {
// handle conversion errors appropriately
}
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 pull request focuses on adding support for auxiliary tenant IDs in the authentication process and updating related configurations. The most important changes include modifications to authentication methods, configuration structures, and provider setup.
Resolves #566
Authentication Enhancements:
AuthenticateClientCertificate,AuthenticateClientSecret,AuthenticateUsingCli,AuthenticateOIDC, andAuthenticateAzDOWorkloadIdentityFederationmethods to includeAdditionallyAllowedTenantsfromclient.config.AuxiliaryTenantIDs. (internal/api/auth.go) [1] [2] [3] [4] [5]Configuration Updates:
AuxiliaryTenantIDsfield toProviderConfigandProviderConfigModelstructs. (internal/config/config.go) [1] [2]ENV_VAR_POWER_PLATFORM_AUXILIARY_TENANT_IDSandENV_VAR_ARM_AUXILIARY_TENANT_IDS. (internal/constants/constants.go) [1] [2]Provider Configuration:
auxiliary_tenant_idsin the provider schema and configuration setup. (internal/provider/provider.go) [1] [2] [3] [4]Helper Functions:
GetListStringValuesfunction to handle list values from configuration or environment variables. (internal/helpers/config.go)Testing:
TestUnitApiClient_AuxTenants_GetConfigto verify the auxiliary tenant IDs configuration. (internal/api/client_test.go)