Add disable-instance-discovery option in interactive pop mode #593
Conversation
Aijing2333
changed the title
Aijing2333
changed the title
pkg/internal/pop/msal_public.go
Outdated
| @@ -18,10 +18,11 @@ func AcquirePoPTokenInteractive( | |||
| authority, | |||
| clientID string, | |||
| options *azcore.ClientOptions, | |||
| isWinfieldEnv bool, | |||
There was a problem hiding this comment.
This parameter is too specific to winfield here, this should be more generic such as WithInstanceDiscovery for example.
I also think that we should start passing optional option or struct here.
pkg/internal/token/provider.go
Outdated
| @@ -34,14 +35,14 @@ func NewTokenProvider(o *Options) (TokenProvider, error) { | |||
| case DeviceCodeLogin: | |||
| return newDeviceCodeTokenProvider(*oAuthConfig, o.ClientID, o.ServerID, o.TenantID) | |||
| case InteractiveLogin: | |||
| return newInteractiveTokenProvider(*oAuthConfig, o.ClientID, o.ServerID, o.TenantID, popClaimsMap) | |||
| return newInteractiveTokenProvider(*oAuthConfig, o.ClientID, o.ServerID, o.TenantID, popClaimsMap, isWinfieldEnvironment(o.Environment)) | |||
There was a problem hiding this comment.
overall I would like to avoid naming winfield here, and find a broader approach.
pkg/internal/token/provider.go
Outdated
| @@ -87,3 +88,10 @@ func getAzureEnvironment(environment string) (azure.Environment, error) { | |||
| } | |||
| return azure.EnvironmentFromName(environment) | |||
| } | |||
|
|
|||
| func isPrivateEnvironment(environment string) bool { | |||
There was a problem hiding this comment.
isAirGappedCloud is likely better naming
pkg/internal/token/options.go
Outdated
| @@ -38,6 +38,7 @@ type Options struct { | |||
|
|
|||
| const ( | |||
| defaultEnvironmentName = "AzurePublicCloud" | |||
| privateEnvironmentName = "AzureStackCloud" | |||
There was a problem hiding this comment.
who sets the value AzureStackCloud? where is this being consumed?
There was a problem hiding this comment.
When using cli cmd az aksarc get-credentials, the AKSArc operator will return a kubeconfig file, the environment name is in this file.
This is local access(can access API server) for AKSArc cluster:
az aksarc get-credentials --name "aksarc-test1" --resource-group $resource_group --file C:\Aksarc\config-aksarc-test1-user
kubectl --kubeconfig C:\Aksarc\config-aksarc-test1-user get ns
kubelogin info:
users:
name: aad-user
user:
exec:
apiVersion: client.authentication.k8s.io/v1beta1
args:
- get-token
- --login
- interactive
- --server-id
- xxx
- --client-id
- xxx
- --tenant-id
- xxx
- --environment
- AzureStackCloud
- --pop-enabled
- --pop-claims
- xxxxxxxx
command: kubelogin
env: null
There was a problem hiding this comment.
I suppose AKS should also set this argument in AGC environment?
There was a problem hiding this comment.
Also, I don't think AzureStackCloud is appropriate for Airgapped environment.
There was a problem hiding this comment.
This name is hardcode in https://github.com/Azure/go-autorest/blob/main/autorest/azure/environments.go#L293 to load env from file(retrieve ActiveDirectoryEndpoint), so we set this name.
For AKS in AGC environment, not very familiar with it, not sure if it needs to load own ActiveDirectoryEndpoint and disable discovery instance
There was a problem hiding this comment.
thinking back, AirGappedCloud is a bit overloaded. The proposed change here is specifically for the environment Azure Local Disconnected (https://learn.microsoft.com/en-us/azure/azure-local/manage/disconnected-operations-overview)
It is also referred internally as Arc Autonomous. This disconnected environment has its own simple Identity Provider(not AAD) which does not have instance metadata discovery endpoint.
@Aijing2333 Is it passible to parameterize DisableInstanceDiscovery to kubelogin.exe ? who will be setting that option?
There was a problem hiding this comment.
for now, exposing this parameter directly in cli and api and let the end user decide is most appropriate, rather than deriving it from the environment
There was a problem hiding this comment.
Thanks @weinong @bganapa for your suggestion, I will make the change to expose the DisableInstanceDiscovery option.
The name AzureStackCloud is still needed as it is the only way to load env from file https://github.com/Azure/go-autorest/blob/main/autorest/azure/environments.go#L293, is this fine?
There was a problem hiding this comment.
Do we need to refer AzureStackCloud within Kubelogin codebase or outside of it ? I know we have this reference of AzureStackCloud in many codebases, CLI, Custom Location RP etc.. If we cant avoid it, I think we will have to make that case and take it forward..
There was a problem hiding this comment.
Never mind, I suddenly realize after I expose the DisableInstanceDiscovery option, this code change removed. Kubelogin already contains the logic to the load env file when env name is azurestackcloud
| type PublicClientOptions struct { | ||
| Authority string | ||
| ClientID string | ||
| DisableInstanceDiscovery bool |
There was a problem hiding this comment.
Dont we also need TenantId here? wondering as I introduced MsalClientOptions in my other PR and the possibility of reusing it here..
There was a problem hiding this comment.
+1. why don't we have https://pkg.go.dev/github.com/AzureAD/[email protected]/apps/public#WithTenantID
Aijing2333
changed the title
| @@ -20,6 +20,7 @@ ZURE_CLIENT_CERTIFICATE_PASSWORD environment variable | |||
| --client-id string AAD client application ID. It may be specified in AAD_SERVICE_PRINCIPAL_CLIENT_ID or AZURE_CLIENT_ID environment variable | |||
| --client-secret string AAD client application secret. Used in spn login. It may be specified in AAD_SERVICE_PRINCIPAL_CLIENT_SECRET or AZURE_CLIENT_S | |||
| ECRET environment variable | |||
| --disable-instance-discovery set to true to disable instance discovery in environments with their own simple Identity Provider (not AAD) that do not have instance metadata discovery endpoint. | |||
There was a problem hiding this comment.
what do you mean by simple?
| @@ -20,6 +20,7 @@ ZURE_CLIENT_CERTIFICATE_PASSWORD environment variable | |||
| --client-id string AAD client application ID. It may be specified in AAD_SERVICE_PRINCIPAL_CLIENT_ID or AZURE_CLIENT_ID environment variable | |||
| --client-secret string AAD client application secret. Used in spn login. It may be specified in AAD_SERVICE_PRINCIPAL_CLIENT_SECRET or AZURE_CLIENT_S | |||
| ECRET environment variable | |||
| --disable-instance-discovery set to true to disable instance discovery in environments with their own simple Identity Provider (not AAD) that do not have instance metadata discovery endpoint. | |||
There was a problem hiding this comment.
| --disable-instance-discovery set to true to disable instance discovery in environments with their own simple Identity Provider (not AAD) that do not have instance metadata discovery endpoint. | |
| --disable-instance-discovery set to true to disable instance discovery in environments with their own Identity Provider (not Entra ID/AAD) that does not have instance metadata discovery endpoint. |
For Winfield, we need to disable instance discovery in the auth flow, or we will hit error. See details in #555
Context
Project Winfield is an appliance Virtual Machine (VM) that provides a local control plane for your datacenter. It allows you to deploy and manage the appliance easily and use Azure Arc and Azure management capabilities in a disconnected or air-gapped environment. Project Winfield is designed to help you meet specific regulatory requirements when you need a local control plane.