From 2904f7009159b4111508f7e3819f60a781a4d1f5 Mon Sep 17 00:00:00 2001 From: Shaun Donnelly Date: Fri, 27 Sep 2024 15:22:32 -0700 Subject: [PATCH 1/9] update target resource uri --- .../AzureContainerRegistryAccessTokenProvider.cs | 2 +- .../Configs/ConvertDataConfiguration.cs | 9 +++++---- 2 files changed, 6 insertions(+), 5 deletions(-) diff --git a/src/Microsoft.Health.Fhir.Azure/ContainerRegistry/AzureContainerRegistryAccessTokenProvider.cs b/src/Microsoft.Health.Fhir.Azure/ContainerRegistry/AzureContainerRegistryAccessTokenProvider.cs index dfe1a42522..046bbfe81c 100644 --- a/src/Microsoft.Health.Fhir.Azure/ContainerRegistry/AzureContainerRegistryAccessTokenProvider.cs +++ b/src/Microsoft.Health.Fhir.Azure/ContainerRegistry/AzureContainerRegistryAccessTokenProvider.cs @@ -59,7 +59,7 @@ public async Task GetTokenAsync(string registryServer, CancellationToken { EnsureArg.IsNotNullOrEmpty(registryServer, nameof(registryServer)); - var aadResourceUri = new Uri(_convertDataConfiguration.ArmResourceManagerId); + var aadResourceUri = new Uri(_convertDataConfiguration.AcrTargetResourceUri); string aadToken; try { diff --git a/src/Microsoft.Health.Fhir.Core/Configs/ConvertDataConfiguration.cs b/src/Microsoft.Health.Fhir.Core/Configs/ConvertDataConfiguration.cs index 2b94ee4d1d..50929afb74 100644 --- a/src/Microsoft.Health.Fhir.Core/Configs/ConvertDataConfiguration.cs +++ b/src/Microsoft.Health.Fhir.Core/Configs/ConvertDataConfiguration.cs @@ -22,11 +22,12 @@ public class ConvertDataConfiguration public ICollection ContainerRegistryServers { get; } = new List(); /// - /// ArmResourceManagerId to aquire AAD token for ACR access token since ACR is not an AAD resource. - /// The value is "https://management.azure.com/" for AzureCloud and DogFood. - /// Could be changed to "https://management.usgovcloudapi.net/" for Azure Government and "https://management.chinacloudapi.cn/ " for Azure China. + /// AcrTargetResourceUri to acquire AAD token for ACR access token since ACR is not an AAD resource. + /// To enable Trusted Services scenarios, we must use the ACR-specific URI rather than the more generic ARM URI. + /// https://dev.azure.com/msazure/AzureContainerRegistry/_wiki/wikis/ACR%20Specs/480000/TrustedServicesPatterns + /// The value is "https://containerregistry.azure.net/" for AzureCloud and DogFood. /// - public string ArmResourceManagerId { get; set; } = "https://management.azure.com/"; + public string AcrTargetResourceUri { get; set; } = "https://containerregistry.azure.net/"; /// /// Configuration for templates. From 63ed8c942b2391ff7376208bb51ef0185b50c199 Mon Sep 17 00:00:00 2001 From: Shaun Donnelly Date: Sun, 29 Sep 2024 20:29:43 -0700 Subject: [PATCH 2/9] resolve build errors --- .../AzureContainerRegistryAccessTokenProvider.cs | 2 +- .../Configs/ConvertDataConfiguration.cs | 4 ++-- 2 files changed, 3 insertions(+), 3 deletions(-) diff --git a/src/Microsoft.Health.Fhir.Azure/ContainerRegistry/AzureContainerRegistryAccessTokenProvider.cs b/src/Microsoft.Health.Fhir.Azure/ContainerRegistry/AzureContainerRegistryAccessTokenProvider.cs index 046bbfe81c..8ee85e9d5e 100644 --- a/src/Microsoft.Health.Fhir.Azure/ContainerRegistry/AzureContainerRegistryAccessTokenProvider.cs +++ b/src/Microsoft.Health.Fhir.Azure/ContainerRegistry/AzureContainerRegistryAccessTokenProvider.cs @@ -59,7 +59,7 @@ public async Task GetTokenAsync(string registryServer, CancellationToken { EnsureArg.IsNotNullOrEmpty(registryServer, nameof(registryServer)); - var aadResourceUri = new Uri(_convertDataConfiguration.AcrTargetResourceUri); + var aadResourceUri = _convertDataConfiguration.AcrTargetResourceUri; string aadToken; try { diff --git a/src/Microsoft.Health.Fhir.Core/Configs/ConvertDataConfiguration.cs b/src/Microsoft.Health.Fhir.Core/Configs/ConvertDataConfiguration.cs index 50929afb74..45aa339742 100644 --- a/src/Microsoft.Health.Fhir.Core/Configs/ConvertDataConfiguration.cs +++ b/src/Microsoft.Health.Fhir.Core/Configs/ConvertDataConfiguration.cs @@ -22,12 +22,12 @@ public class ConvertDataConfiguration public ICollection ContainerRegistryServers { get; } = new List(); /// - /// AcrTargetResourceUri to acquire AAD token for ACR access token since ACR is not an AAD resource. + /// AcrTargetResourceUri to acquire AAD token for ACR access token since ACR is not an AAD resource. /// To enable Trusted Services scenarios, we must use the ACR-specific URI rather than the more generic ARM URI. /// https://dev.azure.com/msazure/AzureContainerRegistry/_wiki/wikis/ACR%20Specs/480000/TrustedServicesPatterns /// The value is "https://containerregistry.azure.net/" for AzureCloud and DogFood. /// - public string AcrTargetResourceUri { get; set; } = "https://containerregistry.azure.net/"; + public Uri AcrTargetResourceUri { get; set; } = new Uri("https://containerregistry.azure.net/"); /// /// Configuration for templates. From ccab1740e2fd8209edee20c6ea4b798c581f0dcc Mon Sep 17 00:00:00 2001 From: Shaun Donnelly Date: Mon, 30 Sep 2024 12:08:18 -0700 Subject: [PATCH 3/9] update comment --- .../Configs/ConvertDataConfiguration.cs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/Microsoft.Health.Fhir.Core/Configs/ConvertDataConfiguration.cs b/src/Microsoft.Health.Fhir.Core/Configs/ConvertDataConfiguration.cs index 45aa339742..a7c30b69b2 100644 --- a/src/Microsoft.Health.Fhir.Core/Configs/ConvertDataConfiguration.cs +++ b/src/Microsoft.Health.Fhir.Core/Configs/ConvertDataConfiguration.cs @@ -25,7 +25,7 @@ public class ConvertDataConfiguration /// AcrTargetResourceUri to acquire AAD token for ACR access token since ACR is not an AAD resource. /// To enable Trusted Services scenarios, we must use the ACR-specific URI rather than the more generic ARM URI. /// https://dev.azure.com/msazure/AzureContainerRegistry/_wiki/wikis/ACR%20Specs/480000/TrustedServicesPatterns - /// The value is "https://containerregistry.azure.net/" for AzureCloud and DogFood. + /// The value is "https://containerregistry.azure.net/" for all cloud environments. /// public Uri AcrTargetResourceUri { get; set; } = new Uri("https://containerregistry.azure.net/"); From 26f65414d911e39782ee90a62dbedf2eb5ca25b9 Mon Sep 17 00:00:00 2001 From: Shaun Donnelly Date: Mon, 30 Sep 2024 14:53:26 -0700 Subject: [PATCH 4/9] re-add ArmResourceManagerId --- .../Configs/ConvertDataConfiguration.cs | 7 +++++++ 1 file changed, 7 insertions(+) diff --git a/src/Microsoft.Health.Fhir.Core/Configs/ConvertDataConfiguration.cs b/src/Microsoft.Health.Fhir.Core/Configs/ConvertDataConfiguration.cs index a7c30b69b2..3059b96044 100644 --- a/src/Microsoft.Health.Fhir.Core/Configs/ConvertDataConfiguration.cs +++ b/src/Microsoft.Health.Fhir.Core/Configs/ConvertDataConfiguration.cs @@ -29,6 +29,13 @@ public class ConvertDataConfiguration /// public Uri AcrTargetResourceUri { get; set; } = new Uri("https://containerregistry.azure.net/"); + /// + /// ArmResourceManagerId to aquire AAD token for ACR access token since ACR is not an AAD resource. + /// The value is "https://management.azure.com/" for AzureCloud and DogFood. + /// Could be changed to "https://management.usgovcloudapi.net/" for Azure Government and "https://management.chinacloudapi.cn/ " for Azure China. + /// + public string ArmResourceManagerId { get; set; } = "https://management.azure.com/"; + /// /// Configuration for templates. /// From 4b65ec4a6d98c39e693d802c6b2da7204af84f08 Mon Sep 17 00:00:00 2001 From: Shaun Donnelly Date: Tue, 1 Oct 2024 09:12:44 -0700 Subject: [PATCH 5/9] add obsolete attribute --- .../Configs/ConvertDataConfiguration.cs | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/src/Microsoft.Health.Fhir.Core/Configs/ConvertDataConfiguration.cs b/src/Microsoft.Health.Fhir.Core/Configs/ConvertDataConfiguration.cs index 3059b96044..7ac9d12edd 100644 --- a/src/Microsoft.Health.Fhir.Core/Configs/ConvertDataConfiguration.cs +++ b/src/Microsoft.Health.Fhir.Core/Configs/ConvertDataConfiguration.cs @@ -30,10 +30,11 @@ public class ConvertDataConfiguration public Uri AcrTargetResourceUri { get; set; } = new Uri("https://containerregistry.azure.net/"); /// - /// ArmResourceManagerId to aquire AAD token for ACR access token since ACR is not an AAD resource. + /// ArmResourceManagerId to acquire AAD token for ACR access token since ACR is not an AAD resource. /// The value is "https://management.azure.com/" for AzureCloud and DogFood. /// Could be changed to "https://management.usgovcloudapi.net/" for Azure Government and "https://management.chinacloudapi.cn/ " for Azure China. /// + [Obsolete("Use AcrTargetResourceUri instead")] public string ArmResourceManagerId { get; set; } = "https://management.azure.com/"; /// From 674d713582526338a269e5a29337c76629fca653 Mon Sep 17 00:00:00 2001 From: Shaun Donnelly Date: Tue, 1 Oct 2024 09:29:45 -0700 Subject: [PATCH 6/9] make uri non-configurable --- .../AzureContainerRegistryAccessTokenProvider.cs | 4 +++- .../Configs/ConvertDataConfiguration.cs | 10 +--------- 2 files changed, 4 insertions(+), 10 deletions(-) diff --git a/src/Microsoft.Health.Fhir.Azure/ContainerRegistry/AzureContainerRegistryAccessTokenProvider.cs b/src/Microsoft.Health.Fhir.Azure/ContainerRegistry/AzureContainerRegistryAccessTokenProvider.cs index 8ee85e9d5e..2bb79adebd 100644 --- a/src/Microsoft.Health.Fhir.Azure/ContainerRegistry/AzureContainerRegistryAccessTokenProvider.cs +++ b/src/Microsoft.Health.Fhir.Azure/ContainerRegistry/AzureContainerRegistryAccessTokenProvider.cs @@ -33,6 +33,8 @@ public class AzureContainerRegistryAccessTokenProvider : IContainerRegistryToken private const string ExchangeAcrRefreshTokenUrl = "oauth2/exchange"; private const string GetAcrAccessTokenUrl = "oauth2/token"; + private static readonly Uri AcrTargetResourceUri = new Uri("https://containerregistry.azure.net/"); + private readonly IAccessTokenProvider _aadTokenProvider; private readonly IHttpClientFactory _httpClientFactory; private readonly ConvertDataConfiguration _convertDataConfiguration; @@ -59,7 +61,7 @@ public async Task GetTokenAsync(string registryServer, CancellationToken { EnsureArg.IsNotNullOrEmpty(registryServer, nameof(registryServer)); - var aadResourceUri = _convertDataConfiguration.AcrTargetResourceUri; + var aadResourceUri = AcrTargetResourceUri; string aadToken; try { diff --git a/src/Microsoft.Health.Fhir.Core/Configs/ConvertDataConfiguration.cs b/src/Microsoft.Health.Fhir.Core/Configs/ConvertDataConfiguration.cs index 7ac9d12edd..a3ad710958 100644 --- a/src/Microsoft.Health.Fhir.Core/Configs/ConvertDataConfiguration.cs +++ b/src/Microsoft.Health.Fhir.Core/Configs/ConvertDataConfiguration.cs @@ -21,20 +21,12 @@ public class ConvertDataConfiguration /// public ICollection ContainerRegistryServers { get; } = new List(); - /// - /// AcrTargetResourceUri to acquire AAD token for ACR access token since ACR is not an AAD resource. - /// To enable Trusted Services scenarios, we must use the ACR-specific URI rather than the more generic ARM URI. - /// https://dev.azure.com/msazure/AzureContainerRegistry/_wiki/wikis/ACR%20Specs/480000/TrustedServicesPatterns - /// The value is "https://containerregistry.azure.net/" for all cloud environments. - /// - public Uri AcrTargetResourceUri { get; set; } = new Uri("https://containerregistry.azure.net/"); - /// /// ArmResourceManagerId to acquire AAD token for ACR access token since ACR is not an AAD resource. /// The value is "https://management.azure.com/" for AzureCloud and DogFood. /// Could be changed to "https://management.usgovcloudapi.net/" for Azure Government and "https://management.chinacloudapi.cn/ " for Azure China. /// - [Obsolete("Use AcrTargetResourceUri instead")] + [Obsolete("Non-configurable ACR target resource URI will be used instead.")] public string ArmResourceManagerId { get; set; } = "https://management.azure.com/"; /// From 2a9dfdbfb70d8f8e1584214623918c38521921be Mon Sep 17 00:00:00 2001 From: Shaun Donnelly Date: Tue, 1 Oct 2024 09:45:09 -0700 Subject: [PATCH 7/9] remove local var --- .../AzureContainerRegistryAccessTokenProvider.cs | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/src/Microsoft.Health.Fhir.Azure/ContainerRegistry/AzureContainerRegistryAccessTokenProvider.cs b/src/Microsoft.Health.Fhir.Azure/ContainerRegistry/AzureContainerRegistryAccessTokenProvider.cs index 2bb79adebd..e507684a36 100644 --- a/src/Microsoft.Health.Fhir.Azure/ContainerRegistry/AzureContainerRegistryAccessTokenProvider.cs +++ b/src/Microsoft.Health.Fhir.Azure/ContainerRegistry/AzureContainerRegistryAccessTokenProvider.cs @@ -61,11 +61,10 @@ public async Task GetTokenAsync(string registryServer, CancellationToken { EnsureArg.IsNotNullOrEmpty(registryServer, nameof(registryServer)); - var aadResourceUri = AcrTargetResourceUri; string aadToken; try { - aadToken = await _aadTokenProvider.GetAccessTokenForResourceAsync(aadResourceUri, cancellationToken); + aadToken = await _aadTokenProvider.GetAccessTokenForResourceAsync(AcrTargetResourceUri, cancellationToken); } catch (AccessTokenProviderException ex) { From fe9b47f0b35e70de345640e78ee02b92aaa6b13e Mon Sep 17 00:00:00 2001 From: Shaun Donnelly Date: Tue, 1 Oct 2024 12:04:02 -0700 Subject: [PATCH 8/9] move uri to config --- .../AzureContainerRegistryAccessTokenProvider.cs | 5 ++--- .../Configs/ConvertDataConfiguration.cs | 10 +++++++++- 2 files changed, 11 insertions(+), 4 deletions(-) diff --git a/src/Microsoft.Health.Fhir.Azure/ContainerRegistry/AzureContainerRegistryAccessTokenProvider.cs b/src/Microsoft.Health.Fhir.Azure/ContainerRegistry/AzureContainerRegistryAccessTokenProvider.cs index e507684a36..8ee85e9d5e 100644 --- a/src/Microsoft.Health.Fhir.Azure/ContainerRegistry/AzureContainerRegistryAccessTokenProvider.cs +++ b/src/Microsoft.Health.Fhir.Azure/ContainerRegistry/AzureContainerRegistryAccessTokenProvider.cs @@ -33,8 +33,6 @@ public class AzureContainerRegistryAccessTokenProvider : IContainerRegistryToken private const string ExchangeAcrRefreshTokenUrl = "oauth2/exchange"; private const string GetAcrAccessTokenUrl = "oauth2/token"; - private static readonly Uri AcrTargetResourceUri = new Uri("https://containerregistry.azure.net/"); - private readonly IAccessTokenProvider _aadTokenProvider; private readonly IHttpClientFactory _httpClientFactory; private readonly ConvertDataConfiguration _convertDataConfiguration; @@ -61,10 +59,11 @@ public async Task GetTokenAsync(string registryServer, CancellationToken { EnsureArg.IsNotNullOrEmpty(registryServer, nameof(registryServer)); + var aadResourceUri = _convertDataConfiguration.AcrTargetResourceUri; string aadToken; try { - aadToken = await _aadTokenProvider.GetAccessTokenForResourceAsync(AcrTargetResourceUri, cancellationToken); + aadToken = await _aadTokenProvider.GetAccessTokenForResourceAsync(aadResourceUri, cancellationToken); } catch (AccessTokenProviderException ex) { diff --git a/src/Microsoft.Health.Fhir.Core/Configs/ConvertDataConfiguration.cs b/src/Microsoft.Health.Fhir.Core/Configs/ConvertDataConfiguration.cs index a3ad710958..65d52154b7 100644 --- a/src/Microsoft.Health.Fhir.Core/Configs/ConvertDataConfiguration.cs +++ b/src/Microsoft.Health.Fhir.Core/Configs/ConvertDataConfiguration.cs @@ -21,12 +21,20 @@ public class ConvertDataConfiguration /// public ICollection ContainerRegistryServers { get; } = new List(); + /// + /// AcrTargetResourceUri to acquire AAD token for ACR access token since ACR is not an AAD resource. + /// To enable Trusted Services scenarios, we must use the ACR-specific URI rather than the more generic ARM URI. + /// https://dev.azure.com/msazure/AzureContainerRegistry/_wiki/wikis/ACR%20Specs/480000/TrustedServicesPatterns + /// The value is "https://containerregistry.azure.net/" for all cloud environments. + /// + public Uri AcrTargetResourceUri { get; } = new Uri("https://containerregistry.azure.net/"); + /// /// ArmResourceManagerId to acquire AAD token for ACR access token since ACR is not an AAD resource. /// The value is "https://management.azure.com/" for AzureCloud and DogFood. /// Could be changed to "https://management.usgovcloudapi.net/" for Azure Government and "https://management.chinacloudapi.cn/ " for Azure China. /// - [Obsolete("Non-configurable ACR target resource URI will be used instead.")] + [Obsolete("Use AcrTargetResourceUri instead.")] public string ArmResourceManagerId { get; set; } = "https://management.azure.com/"; /// From 2269ec27d8cedcb2f6fda3a4563f4e63a48eff32 Mon Sep 17 00:00:00 2001 From: Shaun Donnelly Date: Tue, 1 Oct 2024 14:18:26 -0700 Subject: [PATCH 9/9] set as default ARM uri to avoid breaking OSS scenarios --- .../Configs/ConvertDataConfiguration.cs | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/src/Microsoft.Health.Fhir.Core/Configs/ConvertDataConfiguration.cs b/src/Microsoft.Health.Fhir.Core/Configs/ConvertDataConfiguration.cs index 65d52154b7..8df3ce6e74 100644 --- a/src/Microsoft.Health.Fhir.Core/Configs/ConvertDataConfiguration.cs +++ b/src/Microsoft.Health.Fhir.Core/Configs/ConvertDataConfiguration.cs @@ -23,11 +23,11 @@ public class ConvertDataConfiguration /// /// AcrTargetResourceUri to acquire AAD token for ACR access token since ACR is not an AAD resource. - /// To enable Trusted Services scenarios, we must use the ACR-specific URI rather than the more generic ARM URI. - /// https://dev.azure.com/msazure/AzureContainerRegistry/_wiki/wikis/ACR%20Specs/480000/TrustedServicesPatterns + /// The value is "https://management.azure.com/" for AzureCloud and DogFood. Could be changed to "https://management.usgovcloudapi.net/" for Azure Government and "https://management.chinacloudapi.cn/ " for Azure China. + /// To enable Trusted Services scenarios, we must use the ACR-specific URI rather than the more generic ARM URI. https://dev.azure.com/msazure/AzureContainerRegistry/_wiki/wikis/ACR%20Specs/480000/TrustedServicesPatterns /// The value is "https://containerregistry.azure.net/" for all cloud environments. /// - public Uri AcrTargetResourceUri { get; } = new Uri("https://containerregistry.azure.net/"); + public Uri AcrTargetResourceUri { get; set; } = new Uri("https://management.azure.com/"); /// /// ArmResourceManagerId to acquire AAD token for ACR access token since ACR is not an AAD resource.