Improve registry detection logic (#752)

Author: MicroFish91

src/commands/image/deployImageApi/deployImageApi.ts

@@ -7,7 +7,7 @@ import { callWithMaskHandling, createSubscriptionContext, type ExecuteActivityCo
 import { ImageSource, acrDomain } from "../../../constants";
 import { type SetTelemetryProps } from "../../../telemetry/SetTelemetryProps";
 import { type DeployImageApiTelemetryProps as TelemetryProps } from "../../../telemetry/commandTelemetryProps";
-import { detectRegistryDomain, getRegistryFromAcrName } from "../../../utils/imageNameUtils";
+import { getDomainFromRegistryName, getRegistryFromAcrName } from "../../../utils/imageNameUtils";
 import { pickContainerApp } from "../../../utils/pickItem/pickContainerApp";
 import { type ImageSourceBaseContext } from "../imageSource/ImageSourceContext";
 import { type ContainerRegistryImageSourceContext } from "../imageSource/containerRegistry/ContainerRegistryImageSourceContext";
@@ -35,7 +35,7 @@ export async function deployImageApi(context: IActionContext & Partial<Container
 
     Object.assign(context, { ...createSubscriptionContext(subscription), imageSource: ImageSource.ContainerRegistry }, deployImageOptions);
 
-    context.registryDomain = detectRegistryDomain(deployImageOptions.registryName);
+    context.registryDomain = getDomainFromRegistryName(deployImageOptions.registryName);
     if (context.registryDomain === acrDomain) {
         context.registry = await getRegistryFromAcrName(<ISubscriptionActionContext>context, deployImageOptions.registryName);
     }

src/commands/registryCredentials/RegistryCredentialsAddConfigurationListStep.ts

@@ -5,7 +5,7 @@
 
 import { AzureWizardPromptStep, nonNullProp, type AzureWizardExecuteStep, type IAzureQuickPickItem, type IWizardOptions } from "@microsoft/vscode-azext-utils";
 import { acrDomain, type SupportedRegistries } from "../../constants";
-import { detectRegistryDomain } from "../../utils/imageNameUtils";
+import { getRegistryDomainFromContext } from "../../utils/imageNameUtils";
 import { localize } from "../../utils/localize";
 import { AcrEnableAdminUserConfirmStep } from "./dockerLogin/AcrEnableAdminUserConfirmStep";
 import { AcrEnableAdminUserStep } from "./dockerLogin/AcrEnableAdminUserStep";
@@ -23,24 +23,34 @@ export enum RegistryCredentialType {
 }
 
 export class RegistryCredentialsAddConfigurationListStep extends AzureWizardPromptStep<RegistryCredentialsContext> {
-    private hasExistingRegistry?: boolean;
+    private requiresRegistryConfiguration: boolean;
 
     public async configureBeforePrompt(context: RegistryCredentialsContext): Promise<void> {
-        this.hasExistingRegistry = !!context.containerApp?.configuration?.registries?.some(r => {
+        const registryDomain: SupportedRegistries | undefined = getRegistryDomainFromContext(context);
+        const hasExistingConfiguration: boolean = !!context.containerApp?.configuration?.registries?.some(r => {
             if (!r.server) {
                 return false;
             }
 
-            const registryDomain: SupportedRegistries | undefined = this.getRegistryDomain(context);
             if (registryDomain === acrDomain) {
                 return r.server === context.registry?.loginServer;
-            } else {
+            } else if (context.registryName) {
                 return r.server === DockerLoginRegistryCredentialsAddConfigurationStep.getThirdPartyLoginServer(
                     registryDomain,
                     nonNullProp(context, 'registryName'),
                 );
             }
-        })
+
+            // At this point, there is the small possibility of an existing configuration existing that we don't have
+            // enough information to match.  This edge case usually happens for public repositories that are under the same
+            // private account configuration.  The good news is that these public repositories don't actually need a configuration
+            // to be registered because they are public, so the 'requiresRegistryConfiguration' check should be sufficient to handle these cases.
+            return false;
+        });
+
+        // Rule 1: If we're configuring a new ACR and don't have an existing configuration, we need to create one
+        // Rule 2: If we're configuring a new third party registry and don't have an existing configuration -- only do so if it's not a public repository.  We can detect this with the registryName, username, and secret
+        this.requiresRegistryConfiguration = (registryDomain === acrDomain || (!!context.registryName && !!context.username && !!context.secret)) && !hasExistingConfiguration;
     }
 
     public async prompt(context: RegistryCredentialsContext): Promise<void> {
@@ -51,14 +61,14 @@ export class RegistryCredentialsAddConfigurationListStep extends AzureWizardProm
     }
 
     public shouldPrompt(context: RegistryCredentialsContext): boolean {
-        return !this.hasExistingRegistry && !context.newRegistryCredentialType;
+        return this.requiresRegistryConfiguration && !context.newRegistryCredentialType;
     }
 
     public async getSubWizard(context: RegistryCredentialsContext): Promise<IWizardOptions<RegistryCredentialsContext> | undefined> {
         const promptSteps: AzureWizardPromptStep<RegistryCredentialsContext>[] = [];
         const executeSteps: AzureWizardExecuteStep<RegistryCredentialsContext>[] = [];
 
-        const registryDomain: SupportedRegistries | undefined = this.getRegistryDomain(context);
+        const registryDomain: SupportedRegistries | undefined = getRegistryDomainFromContext(context);
         switch (context.newRegistryCredentialType) {
             case RegistryCredentialType.SystemAssigned:
                 executeSteps.push(
@@ -86,18 +96,11 @@ export class RegistryCredentialsAddConfigurationListStep extends AzureWizardProm
         };
     }
 
-    private getRegistryDomain(context: RegistryCredentialsContext): SupportedRegistries | undefined {
-        if (context.registry?.loginServer || context.registryName) {
-            return detectRegistryDomain(context.registry?.loginServer || nonNullProp(context, 'registryName'));
-        } else {
-            // If no registries exist, we can assume we're creating a new ACR
-            return acrDomain;
-        }
-    }
+
 
     public async getPicks(context: RegistryCredentialsContext): Promise<IAzureQuickPickItem<RegistryCredentialType>[]> {
         const picks: IAzureQuickPickItem<RegistryCredentialType>[] = [];
-        const registryDomain = this.getRegistryDomain(context);
+        const registryDomain = getRegistryDomainFromContext(context);
 
         if (registryDomain === acrDomain) {
             picks.push({

src/commands/registryCredentials/dockerLogin/AcrEnableAdminUserConfirmStep.ts

@@ -4,6 +4,8 @@
  *--------------------------------------------------------------------------------------------*/
 
 import { AzureWizardPromptStep } from "@microsoft/vscode-azext-utils";
+import { acrDomain } from "../../../constants";
+import { getRegistryDomainFromContext } from "../../../utils/imageNameUtils";
 import { localize } from "../../../utils/localize";
 import { type DockerLoginRegistryCredentialsContext } from "./DockerLoginRegistryCredentialsContext";
 
@@ -14,7 +16,6 @@ export class AcrEnableAdminUserConfirmStep extends AzureWizardPromptStep<DockerL
     }
 
     public shouldPrompt(context: DockerLoginRegistryCredentialsContext): boolean {
-        // For further clarification, see: https://github.com/microsoft/vscode-azurecontainerapps/pull/723/files#r1712152541
-        return !context.registryName && !context.registry?.adminUserEnabled;
+        return getRegistryDomainFromContext(context) === acrDomain && !context.registry?.adminUserEnabled;
     }
 }

src/commands/registryCredentials/dockerLogin/DockerLoginRegistryCredentialsContext.ts

@@ -4,11 +4,13 @@
  *--------------------------------------------------------------------------------------------*/
 
 import { type RegistryCredentials, type Secret } from "@azure/arm-appcontainers";
+import { type SupportedRegistries } from "../../../constants";
 import { type IContainerAppContext } from "../../IContainerAppContext";
 import { type CreateAcrContext } from "../../image/imageSource/containerRegistry/acr/createAcr/CreateAcrContext";
 
 export interface DockerLoginRegistryCredentialsContext extends CreateAcrContext, IContainerAppContext {
     // These values are often pre-populated from the Docker extension via the deployImage API layer
+    registryDomain?: SupportedRegistries;
     registryName?: string;
     username?: string;
     secret?: string;

src/commands/registryCredentials/identity/ManagedIdentityRegistryCredentialsContext.ts

@@ -4,11 +4,14 @@
  *--------------------------------------------------------------------------------------------*/
 
 import { type RegistryCredentials } from "@azure/arm-appcontainers";
+import { type SupportedRegistries } from "../../../constants";
 import { type IContainerAppContext } from "../../IContainerAppContext";
 import { type CreateAcrContext } from "../../image/imageSource/containerRegistry/acr/createAcr/CreateAcrContext";
 import { type ManagedEnvironmentRequiredContext } from "../../ManagedEnvironmentContext";
 
 export interface ManagedIdentityRegistryCredentialsContext extends CreateAcrContext, ManagedEnvironmentRequiredContext, IContainerAppContext {
+    registryDomain?: SupportedRegistries;
+
     hasAcrPullRole?: boolean;
     newRegistryCredential?: RegistryCredentials;
 }

src/utils/imageNameUtils.ts

@@ -5,7 +5,8 @@
 
 import { type ContainerRegistryManagementClient, type Registry } from "@azure/arm-containerregistry";
 import { uiUtils } from "@microsoft/vscode-azext-azureutils";
-import { type ISubscriptionActionContext } from "@microsoft/vscode-azext-utils";
+import { nonNullProp, type ISubscriptionActionContext } from "@microsoft/vscode-azext-utils";
+import { type ContainerRegistryImageSourceContext } from "../commands/image/imageSource/containerRegistry/ContainerRegistryImageSourceContext";
 import { acrDomain, dockerHubDomain, type SupportedRegistries } from "../constants";
 import { createContainerRegistryManagementClient } from "./azureClients";
 
@@ -42,7 +43,7 @@ export function parseImageName(imageName?: string): ParsedImageName {
     const match: RegExpMatchArray | null = imageName.match(/^(?:(?<registryName>[^/]+)\/)?(?:(?<namespace>[^/]+(?:\/[^/]+)*)\/)?(?<repositoryName>[^/:]+)(?::(?<tag>[^/]+))?$/);
     return {
         imageNameReference: imageName,
-        registryDomain: match?.groups?.registryName ? detectRegistryDomain(match.groups.registryName) : undefined,
+        registryDomain: match?.groups?.registryName ? getDomainFromRegistryName(match.groups.registryName) : undefined,
         registryName: match?.groups?.registryName,
         namespace: match?.groups?.namespace,
         repositoryName: match?.groups?.repositoryName,
@@ -53,7 +54,7 @@ export function parseImageName(imageName?: string): ParsedImageName {
 /**
  * @param registryName When parsed from a full image name, everything before the first slash
  */
-export function detectRegistryDomain(registryName: string): SupportedRegistries | undefined {
+export function getDomainFromRegistryName(registryName: string): SupportedRegistries | undefined {
     if (/\.azurecr\.io$/i.test(registryName)) {
         return acrDomain;
     } else if (/^docker\.io$/i.test(registryName)) {
@@ -63,6 +64,26 @@ export function detectRegistryDomain(registryName: string): SupportedRegistries
     }
 }
 
+/**
+ * A best effort attempt to obtain the registry domain using all available information on the context.
+ * This function should only be called when we expect to have the full set of inputs necessary to make an informed decision.
+ * It assumes that any missing or ambiguous information has already been addressed prior to the call.
+ */
+export function getRegistryDomainFromContext(context: Partial<ContainerRegistryImageSourceContext>): SupportedRegistries | undefined {
+    switch (true) {
+        case !!context.registryDomain:
+            return context.registryDomain;
+        case !!context.image:
+            const registryName: string | undefined = parseImageName(context.image).registryName;
+            return registryName ? getDomainFromRegistryName(registryName) : undefined;
+        case !!context.registry?.loginServer || !!context.registryName:
+            return getDomainFromRegistryName(context.registry?.loginServer || nonNullProp(context, 'registryName'));
+        default:
+            // If no image by this point, assume we're creating a new ACR
+            return acrDomain;
+    }
+}
+
 /**
  * @param acrName When parsed from a full ACR image name, everything before the first slash
  */