fix: [CI-20436] adding fallback in case of del con (#164)

* fix: [CI-20436] adding fallback in case of del con * fix: [CI-20436] adding fallback in case of del con testcase * fix: [CI-20436] adding fallback in case of del con testcase * fix: [CI-20436] adding fallback in case of del con testcase * fix: [CI-20436] adding fallback in case of del con testcase * fix: [CI-20436] adding fallback in case of del con testcase * fix: [CI-20436] reverting some changes
2026-06-04 18:23:50 +08:00 · 2026-02-07 12:55:04 +05:30
parent dd3c29c971
commit 16758bd8cc
2 changed files with 106 additions and 16 deletions
@@ -536,21 +536,21 @@ func setupAuth(tenantId, clientId, oidcIdToken, cert,
 		return "", fmt.Errorf("registry must be specified")
 	}

-	// Determine auth path: OIDC or Service Principal (secret/cert)
-	if tenantId == "" || clientId == "" {
-		if noPush {
-			logrus.Warnf("NO_PUSH mode: tenantId or clientId not provided")
-			return "", nil
-		}
-		return "", fmt.Errorf("tenantId and clientId must be provided")
-	}
-
 	var aadAccessToken string
 	var acrToken string
 	var publicUrl string
 	var err error

 	if oidcIdToken != "" {
+		// OIDC authentication flow requires tenantId and clientId
+		if tenantId == "" || clientId == "" {
+			if noPush {
+				logrus.Warnf("NO_PUSH mode: tenantId or clientId not provided for OIDC")
+				return "", nil
+			}
+			return "", fmt.Errorf("tenantId and clientId must be provided for OIDC authentication")
+		}
+		logrus.Debug("Using OIDC authentication flow")
 		// Exchange OIDC ID token for AAD access token via client_assertion
 		aadAccessToken, err = azureutil.GetAADAccessTokenViaClientAssertion(context.Background(), tenantId, clientId, oidcIdToken, authorityHost)
 		if err != nil {
@@ -565,16 +565,21 @@ func setupAuth(tenantId, clientId, oidcIdToken, cert,
 		if err != nil {
 			return handleError(noPush, err, "failed to fetch ACR token")
 		}
-	} else if clientSecret != "" || cert != "" {
+	} else {
+		logrus.Debug("Using traditional Azure AD authentication flow")
+		// Validate that if tenantId is provided, clientId must also be provided
+		// (unless using managed identity with no explicit tenantId)
+		if tenantId != "" && clientId == "" && clientSecret == "" && cert == "" {
+			if noPush {
+				logrus.Warnf("NO_PUSH mode: tenantId provided but clientId is missing")
+				return "", nil
+			}
+			return "", fmt.Errorf("tenantId and clientId must be provided")
+		}
 		acrToken, publicUrl, err = getACRToken(subscriptionId, tenantId, clientId, clientSecret, cert, registry)
 		if err != nil {
 			return handleError(noPush, err, "failed to fetch ACR Token")
 		}
-	} else {
-		if noPush {
-			return "", nil
-		}
-		return "", fmt.Errorf("managed authentication is not supported")
 	}

 	if err := setDockerAuth(username, acrToken, registry, dockerUsername, dockerPassword, dockerRegistry); err != nil {
@@ -593,10 +598,46 @@ func handleError(noPush bool, err error, msg string) (string, error) {
 }

 func getACRToken(subscriptionId, tenantId, clientId, clientSecret, cert, registry string) (string, string, error) {
+	// Handle managed identity (when no clientSecret or cert provided)
+	if clientSecret == "" && cert == "" {
+		if tenantId == "" {
+			tenantId = os.Getenv("AZURE_TENANT_ID")
+			if tenantId == "" {
+				tenantId = os.Getenv("TENANT_ID")
+			}
+		}
+		opts := &azidentity.DefaultAzureCredentialOptions{}
+		if tenantId != "" {
+			opts.TenantID = tenantId
+		}
+		cred, err := azidentity.NewDefaultAzureCredential(opts)
+		if err != nil {
+			return "", "", errors.Wrap(err, "failed to get credentials")
+		}
+		policy := policy.TokenRequestOptions{
+			Scopes: []string{"https://management.azure.com/.default"},
+		}
+		azToken, err := cred.GetToken(context.Background(), policy)
+		if err != nil {
+			return "", "", errors.Wrap(err, "failed to fetch access token")
+		}
+		publicUrl, err := getPublicUrl(azToken.Token, registry, subscriptionId)
+		if err != nil {
+			fmt.Fprintf(os.Stderr, "failed to get public url with error: %s\n", err)
+		}
+		if tenantId == "" {
+			return "", "", fmt.Errorf("tenantId cannot be empty for ACR token exchange")
+		}
+		ACRToken, err := fetchACRToken(tenantId, azToken.Token, registry)
+		if err != nil {
+			return "", "", errors.Wrap(err, "failed to fetch ACR token")
+		}
+		return ACRToken, publicUrl, nil
+	}
+
 	if tenantId == "" {
 		return "", "", fmt.Errorf("tenantId can't be empty for AAD authentication")
 	}
-
 	if clientId == "" {
 		return "", "", fmt.Errorf("clientId can't be empty for AAD authentication")
 	}
@@ -387,3 +387,52 @@ func TestSetupAuth_NoCreds_NoPushTrue(t *testing.T) {
 	assert.NoError(t, err)
 	assert.Equal(t, "", pub)
 }
+
+// Test cases for managed identity support
+
+func TestSetupAuth_ManagedIdentity_NoPush_Positive(t *testing.T) {
+	// Positive test: Managed identity flow with noPush=true should succeed
+	// This tests the new managed identity support when no credentials are provided
+	pub, err := setupAuth("tenant123", "", "", "", "", "sub", "myregistry.azurecr.io", "", "", "", "", true)
+	assert.NoError(t, err)
+	assert.Equal(t, "", pub)
+}
+
+func TestSetupAuth_TenantIdButNoClientId_ManagedIdentity(t *testing.T) {
+	// Negative test: When tenantId is provided but clientId is missing for managed identity,
+	// it should fail (unless noPush is true)
+	pub, err := setupAuth("tenant123", "", "", "", "", "sub", "myregistry.azurecr.io", "", "", "", "", false)
+	assert.Error(t, err)
+	assert.Contains(t, err.Error(), "tenantId and clientId must be provided")
+	assert.Equal(t, "", pub)
+}
+
+func TestGetACRToken_ManagedIdentity_NoTenantId(t *testing.T) {
+	// Negative test: Managed identity requires tenantId for ACR token exchange
+	// Clear environment variables to ensure tenantId is not available
+	originalTenantId := os.Getenv("AZURE_TENANT_ID")
+	originalTenantId2 := os.Getenv("TENANT_ID")
+	defer func() {
+		if originalTenantId != "" {
+			os.Setenv("AZURE_TENANT_ID", originalTenantId)
+		} else {
+			os.Unsetenv("AZURE_TENANT_ID")
+		}
+		if originalTenantId2 != "" {
+			os.Setenv("TENANT_ID", originalTenantId2)
+		} else {
+			os.Unsetenv("TENANT_ID")
+		}
+	}()
+	os.Unsetenv("AZURE_TENANT_ID")
+	os.Unsetenv("TENANT_ID")
+
+	// Managed identity path without tenantId should fail
+	// The failure occurs when DefaultAzureCredential tries to acquire a token
+	// since tenantId is required for ACR token exchange but not available
+	_, _, err := getACRToken("sub", "", "", "", "", "myregistry.azurecr.io")
+	assert.Error(t, err)
+	// The error will be from DefaultAzureCredential failing to acquire a token
+	// because tenantId is missing and no credentials are available
+	assert.Contains(t, err.Error(), "failed to fetch access token")
+}