ACR minor fixes (#33222)

* Minor fixes
Azure · Jan 31, 2023 · 8f25c3d · 8f25c3d
1 parent 789b4f6
commit 8f25c3d
Show file tree

Hide file tree

Showing 9 changed files with 151 additions and 154 deletions.
diff --git a/sdk/containerregistry/azure-containers-containerregistry/CHANGELOG.md b/sdk/containerregistry/azure-containers-containerregistry/CHANGELOG.md
@@ -9,6 +9,7 @@
 ### Bugs Fixed
 
 ### Other Changes
+- `ContainerRegistryAudience.AZURE_RESOURCE_MANAGER_GERMANY` is deprecated following [Azure Germany cloud deprecation](https://learn.microsoft.com/azure/cloud-adoption-framework/migrate/azure-best-practices/multiple-regions)
 
 ## 1.1.0-beta.2 (2023-01-11)
 

diff --git a/...gistry/src/main/java/com/azure/containers/containerregistry/implementation/UtilsImpl.java b/...gistry/src/main/java/com/azure/containers/containerregistry/implementation/UtilsImpl.java
@@ -53,8 +53,6 @@
 import java.util.Map;
 import java.util.Objects;
 import java.util.function.Function;
-import java.util.regex.Matcher;
-import java.util.regex.Pattern;
 import java.util.stream.Collectors;
 
 import static com.azure.core.util.tracing.Tracer.AZ_TRACING_NAMESPACE_KEY;
@@ -70,7 +68,6 @@ public final class UtilsImpl {
     private static final int HTTP_STATUS_CODE_NOT_FOUND = 404;
     private static final int HTTP_STATUS_CODE_ACCEPTED = 202;
     private static final HttpHeaderName CONTINUATION_LINK_HEADER_NAME = HttpHeaderName.fromString("Link");
-    private static final Pattern CONTINUATION_LINK_PATTERN = Pattern.compile("<(.+)>;.*");
     private static final String HTTP_REST_PROXY_SYNC_PROXY_ENABLE = "com.azure.core.http.restproxy.syncproxy.enable";
 
     public static final HttpHeaderName DOCKER_DIGEST_HEADER_NAME = HttpHeaderName.fromString("docker-content-digest");
@@ -303,21 +300,7 @@ public static <T> PagedResponse<T> getPagedResponseWithContinuationToken(PagedRe
     public static <T, R> PagedResponse<T> getPagedResponseWithContinuationToken(PagedResponse<R> listResponse, Function<List<R>, List<T>> mapperFunction) {
         Objects.requireNonNull(mapperFunction);
 
-        String continuationLink = null;
-        HttpHeaders headers = listResponse.getHeaders();
-
-        if (headers != null) {
-            String continuationLinkHeader = headers.getValue(CONTINUATION_LINK_HEADER_NAME);
-            if (!CoreUtils.isNullOrEmpty(continuationLinkHeader)) {
-                Matcher matcher = CONTINUATION_LINK_PATTERN.matcher(continuationLinkHeader);
-                if (matcher.matches()) {
-                    if (matcher.groupCount() == 1) {
-                        continuationLink = matcher.group(1);
-                    }
-                }
-            }
-        }
-
+        String continuationLink = getContinuationLink(listResponse.getHeaders());
         List<T> values = mapperFunction.apply(listResponse.getValue());
 
         return new PagedResponseBase<String, T>(
@@ -330,6 +313,19 @@ public static <T, R> PagedResponse<T> getPagedResponseWithContinuationToken(Page
         );
     }
 
+    private static String getContinuationLink(HttpHeaders headers) {
+        String continuationLinkHeader = headers.getValue(CONTINUATION_LINK_HEADER_NAME);
+        if (!CoreUtils.isNullOrEmpty(continuationLinkHeader) && continuationLinkHeader.charAt(0) == '<') {
+            int endIndex = continuationLinkHeader.indexOf(">;");
+            if (endIndex < 2) {
+                LOGGER.warning("unexpected 'Link' header value - '{}'", continuationLinkHeader);
+            }
+            return continuationLinkHeader.substring(1, endIndex);
+        }
+
+        return null;
+    }
+
     public static List<ArtifactManifestProperties> mapManifestsProperties(List<ManifestAttributesBase> baseArtifacts,
                                                                           String repositoryName,
                                                                           String registryLoginServer) {

diff --git a/...s/containerregistry/implementation/authentication/ContainerRegistryCredentialsPolicy.java b/...s/containerregistry/implementation/authentication/ContainerRegistryCredentialsPolicy.java
@@ -14,11 +14,6 @@
 import com.azure.core.util.logging.ClientLogger;
 import reactor.core.publisher.Mono;
 
-import java.util.HashMap;
-import java.util.Map;
-import java.util.regex.Matcher;
-import java.util.regex.Pattern;
-
 /**
  * <p>Credential policy for the container registry. It follows the challenge based authorization scheme.</p>
  *
@@ -41,8 +36,6 @@
 public final class ContainerRegistryCredentialsPolicy extends BearerTokenAuthenticationPolicy {
     private static final ClientLogger LOGGER = new ClientLogger(ContainerRegistryCredentialsPolicy.class);
     private static final String BEARER = "Bearer";
-    public static final Pattern AUTHENTICATION_CHALLENGE_PARAMS_PATTERN =
-        Pattern.compile("(?:(\\w+)=\"([^\"\"]*)\")+");
     public static final String WWW_AUTHENTICATE = "WWW-Authenticate";
     public static final String SCOPES_PARAMETER = "scope";
     public static final String SERVICE_PARAMETER = "service";
@@ -140,14 +133,13 @@ public Mono<Boolean> authorizeRequestOnChallenge(HttpPipelineCallContext context
         if (!(response.getStatusCode() == 401 && authHeader != null)) {
             return Mono.just(false);
         } else {
-            Map<String, String> extractedChallengeParams = parseBearerChallenge(authHeader);
-            if (extractedChallengeParams != null && extractedChallengeParams.containsKey(SCOPES_PARAMETER)) {
-                String scope = extractedChallengeParams.get(SCOPES_PARAMETER);
-                String serviceName = extractedChallengeParams.get(SERVICE_PARAMETER);
+            String scope =  extractValue(authHeader, SCOPES_PARAMETER);
+            String serviceName = extractValue(authHeader, SERVICE_PARAMETER);
+
+            if (scope != null && serviceName != null) {
                 return setAuthorizationHeader(context, new ContainerRegistryTokenRequestContext(serviceName, scope))
                     .thenReturn(true);
             }
-
             return Mono.just(false);
         }
     }
@@ -224,29 +216,37 @@ public boolean authorizeRequestOnChallengeSync(HttpPipelineCallContext context,
         if (!(response.getStatusCode() == 401 && authHeader != null)) {
             return false;
         } else {
-            Map<String, String> extractedChallengeParams = parseBearerChallenge(authHeader);
-            if (extractedChallengeParams != null && extractedChallengeParams.containsKey(SCOPES_PARAMETER)) {
-                String scope = extractedChallengeParams.get(SCOPES_PARAMETER);
-                String serviceName = extractedChallengeParams.get(SERVICE_PARAMETER);
+            String scope =  extractValue(authHeader, SCOPES_PARAMETER);
+            String serviceName = extractValue(authHeader, SERVICE_PARAMETER);
+
+            if (scope != null && serviceName != null) {
                 setAuthorizationHeaderSync(context, new ContainerRegistryTokenRequestContext(serviceName, scope));
                 return true;
             }
-
-            return false;
         }
-    }
 
-    private Map<String, String> parseBearerChallenge(String header) {
-        if (header.startsWith(BEARER)) {
-            String challengeParams = header.substring(BEARER.length());
+        return false;
+    }
 
-            Matcher matcher2 = AUTHENTICATION_CHALLENGE_PARAMS_PATTERN.matcher(challengeParams);
+    /**
+     * Extracts value for given key in www-authenticate header.
+     * Expects key="value" format and return value without quotes.
+     *
+     * returns if value is not found
+     */
+    private String extractValue(String authHeader, String key) {
+        int start = authHeader.indexOf(key);
+        if (start < 0 || authHeader.length() - start < key.length() + 3) {
+            return null;
+        }
 
-            Map<String, String> challengeParameters = new HashMap<>();
-            while (matcher2.find()) {
-                challengeParameters.put(matcher2.group(1), matcher2.group(2));
+        start += key.length();
+        if (authHeader.charAt(start) == '=' && authHeader.charAt(start + 1) == '"') {
+            start += 2;
+            int end = authHeader.indexOf('"', start);
+            if (end > start) {
+                return authHeader.substring(start, end);
             }
-            return challengeParameters;
         }
 
         return null;

diff --git a/...rc/main/java/com/azure/containers/containerregistry/models/ContainerRegistryAudience.java b/...rc/main/java/com/azure/containers/containerregistry/models/ContainerRegistryAudience.java
@@ -13,7 +13,11 @@ public final class ContainerRegistryAudience extends ExpandableStringEnum<Contai
     /** Static value AZURE_RESOURCE_MANAGER_CHINA for ContainerRegistryAudience. */
     public static final ContainerRegistryAudience AZURE_RESOURCE_MANAGER_CHINA = fromString("https://management.chinacloudapi.cn");
 
-    /** Static value AZURE_RESOURCE_MANAGER_GERMANY for ContainerRegistryAudience. */
+    /**
+     * Static value AZURE_RESOURCE_MANAGER_GERMANY for ContainerRegistryAudience.
+     * @deprecated Germany government cloud is no longer supported.
+     */
+    @Deprecated
     public static final ContainerRegistryAudience AZURE_RESOURCE_MANAGER_GERMANY = fromString("https://management.microsoftazure.de");
 
     /** Static value AZURE_RESOURCE_MANAGER_GOVERNMENT for ContainerRegistryAudience. */

diff --git a/.../java/com/azure/containers/containerregistry/specialized/ContainerRegistryBlobClient.java b/.../java/com/azure/containers/containerregistry/specialized/ContainerRegistryBlobClient.java
@@ -40,6 +40,8 @@
 
 import java.util.Objects;
 
+import static com.azure.containers.containerregistry.implementation.UtilsImpl.DOCKER_DIGEST_HEADER_NAME;
+import static com.azure.containers.containerregistry.implementation.UtilsImpl.OCI_MANIFEST_MEDIA_TYPE;
 import static com.azure.containers.containerregistry.implementation.UtilsImpl.deleteResponseToSuccess;
 import static com.azure.containers.containerregistry.implementation.UtilsImpl.enableSync;
 import static com.azure.containers.containerregistry.implementation.UtilsImpl.getTracingContext;
@@ -269,7 +271,7 @@ public Response<DownloadManifestResult> downloadManifestWithResponse(DownloadMan
         try {
             response =
                 this.registriesImpl.getManifestWithResponse(repositoryName, tagOrDigest,
-                    UtilsImpl.OCI_MANIFEST_MEDIA_TYPE, enableSync(getTracingContext(context)));
+                    OCI_MANIFEST_MEDIA_TYPE, enableSync(getTracingContext(context)));
         } catch (AcrErrorsException exception) {
             throw LOGGER.logExceptionAsError(mapAcrErrorsException(exception));
         }
@@ -278,7 +280,8 @@ public Response<DownloadManifestResult> downloadManifestWithResponse(DownloadMan
 
         // The service wants us to validate the digest here since a lot of customers forget to do it before consuming
         // the contents returned by the service.
-        if (Objects.equals(digest, tagOrDigest) || Objects.equals(response.getValue().getTag(), tagOrDigest)) {
+        // TODO (limolkova) calculate digest from the contents - oops we don't have it anymore
+        if (options.getDigest() == null || Objects.equals(digest, options.getDigest())) {
             OciManifest ociManifest = new OciManifest()
                 .setAnnotations(wrapper.getAnnotations())
                 .setConfig(wrapper.getConfig())
@@ -289,10 +292,13 @@ public Response<DownloadManifestResult> downloadManifestWithResponse(DownloadMan
                 response.getRequest(),
                 response.getStatusCode(),
                 response.getHeaders(),
-                new DownloadManifestResult(digest, ociManifest, BinaryData.fromObject(ociManifest)));
+                new DownloadManifestResult(digest, ociManifest, BinaryData.fromObject(wrapper)));
         } else {
-            throw LOGGER.logExceptionAsError(
-                new ServiceResponseException("The digest in the response does not match the expected digest."));
+            throw LOGGER.atError()
+                .addKeyValue(DOCKER_DIGEST_HEADER_NAME.getCaseSensitiveName(), digest)
+                .addKeyValue("requestedDigest", options.getDigest())
+                .addKeyValue("actualDigest", digest)
+                .log(new ServiceResponseException("The digest in the response does not match the requested digest."));
         }
     }
 

diff --git a/...rc/test/java/com/azure/containers/containerregistry/ContainerRegistryClientsTestBase.java b/...rc/test/java/com/azure/containers/containerregistry/ContainerRegistryClientsTestBase.java
@@ -3,8 +3,10 @@
 
 package com.azure.containers.containerregistry;
 
+import com.azure.containers.containerregistry.models.ArtifactArchitecture;
 import com.azure.containers.containerregistry.models.ArtifactManifestPlatform;
 import com.azure.containers.containerregistry.models.ArtifactManifestProperties;
+import com.azure.containers.containerregistry.models.ArtifactOperatingSystem;
 import com.azure.containers.containerregistry.models.ArtifactTagProperties;
 import com.azure.containers.containerregistry.models.ContainerRegistryAudience;
 import com.azure.containers.containerregistry.models.ContainerRepositoryProperties;
@@ -39,7 +41,6 @@
 import static com.azure.containers.containerregistry.TestUtils.isSorted;
 import static org.junit.jupiter.api.Assertions.assertEquals;
 import static org.junit.jupiter.api.Assertions.assertNotNull;
-import static org.junit.jupiter.api.Assertions.assertNull;
 import static org.junit.jupiter.api.Assertions.assertTrue;
 
 public class ContainerRegistryClientsTestBase extends TestBase {
@@ -186,11 +187,10 @@ void validateListArtifacts(Collection<ArtifactManifestProperties> artifacts) {
 
         assertTrue(artifacts.stream().anyMatch(prop -> prop.getTags() != null));
 
-        // TODO (limolkova) uncomment when service is fixed
-        /* assertTrue(artifacts.stream().anyMatch(a -> ArtifactArchitecture.AMD64.equals(a.getArchitecture())
+        assertTrue(artifacts.stream().anyMatch(a -> ArtifactArchitecture.AMD64.equals(a.getArchitecture())
             && ArtifactOperatingSystem.WINDOWS.equals(a.getOperatingSystem())));
         assertTrue(artifacts.stream().anyMatch(a -> ArtifactArchitecture.ARM.equals(a.getArchitecture())
-            && ArtifactOperatingSystem.LINUX.equals(a.getOperatingSystem())));*/
+            && ArtifactOperatingSystem.LINUX.equals(a.getOperatingSystem())));
     }
 
     boolean validateListArtifactsByPage(Collection<PagedResponse<ArtifactManifestProperties>> pagedResList) {
@@ -249,10 +249,8 @@ void validateManifestProperties(ArtifactManifestProperties props, boolean hasTag
         assertNotNull(props.isWriteEnabled());
 
         if (isChild) {
-            // TODO (limolkova) uncomment when service is fixed
-            // assertNotNull(props.getArchitecture());
-            // assertNotNull(props.getOperatingSystem());
-            assertNull(props.getArchitecture());
+            assertNotNull(props.getArchitecture());
+            assertNotNull(props.getOperatingSystem());
         } else {
             assertNotNull(props.getTags());
             assertNotNull(props.getRelatedArtifacts());

diff --git a/...t/java/com/azure/containers/containerregistry/ContainerRegistryCredentialPolicyTests.java b/...t/java/com/azure/containers/containerregistry/ContainerRegistryCredentialPolicyTests.java
@@ -42,21 +42,17 @@ public class ContainerRegistryCredentialPolicyTests {
     public static final Integer SUCCESS = 200;
     public static final String AUTHORIZATION = "Authorization";
     public static final String BEARER = "Bearer";
-    public static final String TOKENVALUE = "tokenValue";
     public static final String SERVICENAME = "mytest.azurecr.io";
     public static final String SCOPENAME = "registry:catalog:*";
 
     private ContainerRegistryTokenService service;
-    private HttpRequest request;
     private HttpResponse unauthorizedHttpResponse;
     private HttpResponse unauthorizedHttpResponseWithoutHeader;
     private HttpPipelineCallContext callContext;
     private HttpResponse successResponse;
     private HttpPipelineNextPolicy nextPolicy;
 
     private HttpPipelineNextSyncPolicy nextSyncPolicy;
-    private HttpPipelineNextPolicy nextClonePolicy;
-
     @BeforeEach
     public void setup() {
         AccessToken accessToken = new AccessToken("tokenValue", OffsetDateTime.now().plusMinutes(30));
@@ -101,9 +97,7 @@ public void setup() {
         this.unauthorizedHttpResponse = unauthorizedResponseWithHeader;
         this.unauthorizedHttpResponseWithoutHeader = unauthorizedResponseWithoutHeader;
         this.callContext = context;
-        this.request = request;
         this.successResponse = successResponse;
-        this.nextClonePolicy = mockNextClone;
         this.nextPolicy = mockNext;
         this.nextSyncPolicy = mockNextSync;