OCPBUGS-84239: fix Azure role assignment flag validation gaps that silently fail

cmd/cluster/azure/create.go

@@ -161,34 +161,20 @@ func BindProductCoreFlags(opts *core.RawCreateOptions, flags *pflag.FlagSet) {
 }
 
 // Validate validates the Azure create cluster command options
-func (o *RawCreateOptions) Validate(ctx context.Context, _ *core.CreateOptions) (core.PlatformCompleter, error) {
+func (o *RawCreateOptions) Validate(ctx context.Context, coreOpts *core.CreateOptions) (core.PlatformCompleter, error) {
 	var err error
 
 	// Check if the network security group is set and the resource group is not
 	if o.NetworkSecurityGroupID != "" && o.ResourceGroupName == "" {
 		return nil, fmt.Errorf("flag --resource-group-name is required when using --network-security-group-id")
 	}
 
-	// The DNS zone resource group name is required when assigning azure roles to the control plane components
-	// since several will need to be scoped to this resource group.
-	if o.AssignServicePrincipalRoles && o.DNSZoneRGName == "" {
-		return nil, fmt.Errorf("flag --dns-zone-rg-name is required")
-	}
-
-	// Validate that workload identities file and managed identities files are mutually exclusive
-	if o.WorkloadIdentitiesFile != "" && o.ManagedIdentitiesFile != "" {
-		return nil, fmt.Errorf("flags --workload-identities-file and --managed-identities-file are mutually exclusive")
-	}
-	if o.WorkloadIdentitiesFile != "" && o.DataPlaneIdentitiesFile != "" {
-		return nil, fmt.Errorf("flags --workload-identities-file and --data-plane-identities-file are mutually exclusive")
+	if err := o.validateRoleAssignmentFlags(coreOpts); err != nil {
+		return nil, err
 	}
 
-	// Validate that data plane identities file requires managed identities file
-	if o.DataPlaneIdentitiesFile != "" && o.ManagedIdentitiesFile == "" {
-		return nil, fmt.Errorf("--data-plane-identities-file requires --managed-identities-file")
-	}
-	if o.ManagedIdentitiesFile != "" && o.DataPlaneIdentitiesFile == "" {
-		return nil, fmt.Errorf("--managed-identities-file requires --data-plane-identities-file")
+	if err := o.validateIdentitiesFiles(); err != nil {
+		return nil, err
 	}
 
 	// Validate the endpoint access value if provided
@@ -222,6 +208,11 @@ func (o *RawCreateOptions) Validate(ctx context.Context, _ *core.CreateOptions)
 		}
 	}
 
+	if coreOpts != nil {
+		if err := validateExternalDNSDomain(coreOpts.ExternalDNSDomain, coreOpts.Name, coreOpts.BaseDomain); err != nil {
+			return nil, err
+		}
+	}
 	validOpts := &ValidatedCreateOptions{
 		validatedCreateOptions: &validatedCreateOptions{
 			RawCreateOptions: o,
@@ -252,6 +243,74 @@ func (o *RawCreateOptions) Validate(ctx context.Context, _ *core.CreateOptions)
 	return validOpts, nil
 }
 
+// validateRoleAssignmentFlags validates that role assignment flags (--assign-service-principal-roles,
+// --assign-custom-hcp-roles) are not used with --infra-json and that --dns-zone-rg-name is provided
+// when role assignment is requested.
+func (o *RawCreateOptions) validateRoleAssignmentFlags(coreOpts *core.CreateOptions) error {
+	wantsRoleAssignment := o.AssignServicePrincipalRoles || o.AssignCustomHCPRoles
+	if !wantsRoleAssignment {
+		return nil
+	}
+
+	if coreOpts != nil && coreOpts.InfrastructureJSON != "" {
+		return fmt.Errorf("role assignment flags cannot be used with --infra-json; use --assign-identity-roles on 'create infra azure' instead")
+	}
+
+	if strings.TrimSpace(o.DNSZoneRGName) == "" {
+		return fmt.Errorf("--dns-zone-rg-name is required when --assign-service-principal-roles or --assign-custom-hcp-roles is set")
+	}
+
+	return nil
+}
+
+// validateIdentitiesFiles validates that workload identities and managed identities files are
+// mutually exclusive, and that data plane identities files are only used with managed identities.
+func (o *RawCreateOptions) validateIdentitiesFiles() error {
+	if o.WorkloadIdentitiesFile != "" && o.ManagedIdentitiesFile != "" {
+		return fmt.Errorf("flags --workload-identities-file and --managed-identities-file are mutually exclusive")
+	}
+	if o.WorkloadIdentitiesFile != "" && o.DataPlaneIdentitiesFile != "" {
+		return fmt.Errorf("flags --workload-identities-file and --data-plane-identities-file are mutually exclusive")
+	}
+	if o.DataPlaneIdentitiesFile != "" && o.ManagedIdentitiesFile == "" {
+		return fmt.Errorf("--data-plane-identities-file requires --managed-identities-file")
+	}
+	if o.ManagedIdentitiesFile != "" && o.DataPlaneIdentitiesFile == "" {
+		return fmt.Errorf("--managed-identities-file requires --data-plane-identities-file")
+	}
+	return nil
+}
+
+// validateExternalDNSDomain checks that the external DNS domain does not conflict with the cluster
+// domain. When a private Azure HostedCluster uses an externalDNSDomain that matches or is a parent
+// of the cluster domain (clusterName.baseDomain), the PLS controller creates an Azure Private DNS
+// zone that shadows *.apps DNS resolution.
+func validateExternalDNSDomain(externalDNSDomain, clusterName, baseDomain string) error {
+	if externalDNSDomain == "" {
+		return nil
+	}
+
+	if clusterName == "" || baseDomain == "" {
+		return nil
+	}
+
+	clusterDomain := clusterName + "." + baseDomain
+
+	extLower := strings.ToLower(strings.TrimSuffix(externalDNSDomain, "."))
+	clusterLower := strings.ToLower(strings.TrimSuffix(clusterDomain, "."))
+
+	// Check if the externalDNSDomain matches or is a parent of the cluster domain.
+	// An exact match means the Private DNS zone would directly shadow *.apps.
+	// A suffix match (with dot boundary) means the zone is a parent that would also shadow.
+	if extLower == clusterLower || strings.HasSuffix(clusterLower, "."+extLower) {
+		return fmt.Errorf("external DNS domain %q conflicts with cluster domain %q: "+
+			"this would create an Azure Private DNS zone that shadows *.apps DNS resolution. "+
+			"Use a different --external-dns-domain value", externalDNSDomain, clusterDomain)
+	}
+
+	return nil
+}
+
 // Complete completes the Azure create cluster command options
 func (o *ValidatedCreateOptions) Complete(ctx context.Context, opts *core.CreateOptions) (core.Platform, error) {
 	output := &CreateOptions{

cmd/cluster/azure/create_test.go

@@ -65,7 +65,7 @@ func TestValidateEndpointAccess(t *testing.T) {
 			opts.EndpointAccessPrivateNATSubnetID = test.endpointAccessPrivateNATSubnetID
 			opts.EndpointAccessPrivateAdditionalAllowedSubscriptions = test.endpointAccessPrivateAdditionalAllowedSubscriptions
 
-			_, err := opts.Validate(context.Background(), &core.CreateOptions{})
+			_, err := opts.Validate(context.Background(), nil)
 			if test.expectError {
 				if err == nil {
 					t.Fatalf("expected error but got nil")
@@ -82,13 +82,77 @@ func TestValidateEndpointAccess(t *testing.T) {
 	}
 }
 
-func TestCreateCluster(t *testing.T) {
+func TestDNSZoneRGValidation(t *testing.T) {
 	utilrand.Seed(1234567890)
 	certs.UnsafeSeed(1234567890)
 	ctx := framework.InterruptableContext(t.Context())
-	tempDir := t.TempDir()
 	t.Setenv("FAKE_CLIENT", "true")
 
+	credentialsFile, _, pullSecretFile := setupAzureTestFixtures(t)
+	tempDir := t.TempDir()
+
+	tests := map[string]struct {
+		extraArgs   []string
+		expectError bool
+		errContains string
+	}{
+		"When assign-service-principal-roles is set without dns-zone-rg-name it should return an error": {
+			extraArgs: []string{
+				"--assign-service-principal-roles",
+			},
+			expectError: true,
+			errContains: "--dns-zone-rg-name is required when --assign-service-principal-roles or --assign-custom-hcp-roles is set",
+		},
+		"When assign-custom-hcp-roles is set without dns-zone-rg-name it should return an error": {
+			extraArgs: []string{
+				"--assign-custom-hcp-roles",
+			},
+			expectError: true,
+			errContains: "--dns-zone-rg-name is required when --assign-service-principal-roles or --assign-custom-hcp-roles is set",
+		},
+	}
+
+	for name, tc := range tests {
+		t.Run(name, func(t *testing.T) {
+			g := NewGomegaWithT(t)
+
+			flags := pflag.NewFlagSet("test", pflag.ContinueOnError)
+			coreOpts := core.DefaultOptions()
+			core.BindDeveloperOptions(coreOpts, flags)
+			azureOpts := DefaultOptions()
+			azurenodepool.BindOptions(azureOpts.NodePoolOpts, flags)
+			BindDeveloperOptions(azureOpts, flags)
+
+			args := []string{
+				"--azure-creds=" + credentialsFile,
+				"--name=test-dns-zone",
+				"--pull-secret=" + pullSecretFile,
+				"--managed-identities-file", filepath.Join(tempDir, "managedIdentities.json"),
+				"--data-plane-identities-file", filepath.Join(tempDir, "dataPlaneIdentities.json"),
+			}
+			args = append(args, tc.extraArgs...)
+
+			err := flags.Parse(args)
+			g.Expect(err).NotTo(HaveOccurred())
+
+			coreOpts.Render = true
+			coreOpts.RenderInto = filepath.Join(t.TempDir(), "manifests.yaml")
+
+			err = core.CreateCluster(ctx, coreOpts, azureOpts)
+			if tc.expectError {
+				g.Expect(err).To(HaveOccurred())
+				g.Expect(err.Error()).To(ContainSubstring(tc.errContains))
+			} else {
+				g.Expect(err).NotTo(HaveOccurred())
+			}
+		})
+	}
+}
+
+func setupAzureTestFixtures(t *testing.T) (credentialsFile, infraFile, pullSecretFile string) {
+	t.Helper()
+	tempDir := t.TempDir()
+
 	rawCreds, err := yaml.Marshal(&util.AzureCreds{
 		SubscriptionID: "fakeSubscriptionID",
 		ClientID:       "fakeClientID",
@@ -98,7 +162,7 @@ func TestCreateCluster(t *testing.T) {
 	if err != nil {
 		t.Fatalf("failed to marshal creds: %v", err)
 	}
-	credentialsFile := filepath.Join(tempDir, "credentials.yaml")
+	credentialsFile = filepath.Join(tempDir, "credentials.yaml")
 	if err := os.WriteFile(credentialsFile, rawCreds, 0600); err != nil {
 		t.Fatalf("failed to write creds: %v", err)
 	}
@@ -119,17 +183,101 @@ func TestCreateCluster(t *testing.T) {
 	if err != nil {
 		t.Fatalf("failed to marshal infra: %v", err)
 	}
-	infraFile := filepath.Join(tempDir, "infra.json")
+	infraFile = filepath.Join(tempDir, "infra.json")
 	if err := os.WriteFile(infraFile, rawInfra, 0600); err != nil {
 		t.Fatalf("failed to write infra: %v", err)
 	}
 
-	pullSecretFile := filepath.Join(tempDir, "pull-secret.json")
-
+	pullSecretFile = filepath.Join(tempDir, "pull-secret.json")
 	if err := os.WriteFile(pullSecretFile, []byte(`fake`), 0600); err != nil {
 		t.Fatalf("failed to write pullSecret: %v", err)
 	}
 
+	return credentialsFile, infraFile, pullSecretFile
+}
+
+func TestRoleAssignmentWithInfraJSON(t *testing.T) {
+	utilrand.Seed(1234567890)
+	certs.UnsafeSeed(1234567890)
+	ctx := framework.InterruptableContext(t.Context())
+	t.Setenv("FAKE_CLIENT", "true")
+
+	credentialsFile, infraFile, pullSecretFile := setupAzureTestFixtures(t)
+	tempDir := t.TempDir()
+
+	tests := map[string]struct {
+		extraArgs   []string
+		expectError bool
+		errContains string
+	}{
+		"When assign-custom-hcp-roles is set with infra-json it should return an error": {
+			extraArgs: []string{
+				"--assign-custom-hcp-roles",
+			},
+			expectError: true,
+			errContains: "role assignment flags cannot be used with --infra-json",
+		},
+		"When assign-service-principal-roles is set with infra-json it should return an error": {
+			extraArgs: []string{
+				"--assign-service-principal-roles",
+				"--dns-zone-rg-name=my-dns-rg",
+			},
+			expectError: true,
+			errContains: "role assignment flags cannot be used with --infra-json",
+		},
+		"When role assignment flags are not set with infra-json it should succeed": {
+			extraArgs:   nil,
+			expectError: false,
+		},
+	}
+
+	for name, tc := range tests {
+		t.Run(name, func(t *testing.T) {
+			g := NewGomegaWithT(t)
+
+			flags := pflag.NewFlagSet("test", pflag.ContinueOnError)
+			coreOpts := core.DefaultOptions()
+			core.BindDeveloperOptions(coreOpts, flags)
+			azureOpts := DefaultOptions()
+			azurenodepool.BindOptions(azureOpts.NodePoolOpts, flags)
+			BindDeveloperOptions(azureOpts, flags)
+
+			args := []string{
+				"--azure-creds=" + credentialsFile,
+				"--infra-json=" + infraFile,
+				"--name=test-role-assignment",
+				"--pull-secret=" + pullSecretFile,
+				"--managed-identities-file", filepath.Join(tempDir, "managedIdentities.json"),
+				"--data-plane-identities-file", filepath.Join(tempDir, "dataPlaneIdentities.json"),
+			}
+			args = append(args, tc.extraArgs...)
+
+			err := flags.Parse(args)
+			g.Expect(err).NotTo(HaveOccurred())
+
+			coreOpts.Render = true
+			coreOpts.RenderInto = filepath.Join(t.TempDir(), "manifests.yaml")
+
+			err = core.CreateCluster(ctx, coreOpts, azureOpts)
+			if tc.expectError {
+				g.Expect(err).To(HaveOccurred())
+				g.Expect(err.Error()).To(ContainSubstring(tc.errContains))
+			} else {
+				g.Expect(err).NotTo(HaveOccurred())
+			}
+		})
+	}
+}
+
+func TestCreateCluster(t *testing.T) {
+	utilrand.Seed(1234567890)
+	certs.UnsafeSeed(1234567890)
+	ctx := framework.InterruptableContext(t.Context())
+	t.Setenv("FAKE_CLIENT", "true")
+
+	credentialsFile, infraFile, pullSecretFile := setupAzureTestFixtures(t)
+	tempDir := t.TempDir()
+
 	for _, testCase := range []struct {
 		name string
 		args []string
@@ -375,7 +523,7 @@ func TestValidateOAuthPublishingStrategy(t *testing.T) {
 			opts.ManagedIdentitiesFile = test.managedIdentitiesFile
 			opts.DataPlaneIdentitiesFile = test.dataPlaneIdentitiesFile
 
-			_, err := opts.Validate(context.Background(), &core.CreateOptions{})
+			_, err := opts.Validate(context.Background(), nil)
 			if test.expectError {
 				g.Expect(err).To(HaveOccurred())
 				g.Expect(err).To(MatchError(test.expectedErrorMsg))

cmd/infra/azure/create.go

@@ -5,6 +5,7 @@ import (
 	"encoding/json"
 	"fmt"
 	"os"
+	"strings"
 
 	hyperv1 "github.com/openshift/hypershift/api/hypershift/v1beta1"
 	"github.com/openshift/hypershift/cmd/log"
@@ -341,6 +342,11 @@ func (o *CreateInfraOptions) Validate() error {
 		return fmt.Errorf("--base-domain is required")
 	}
 
+	// dns-zone-rg-name is required for role assignment scoping
+	if (o.AssignServicePrincipalRoles || o.AssignCustomHCPRoles) && strings.TrimSpace(o.DNSZoneRG) == "" {
+		return fmt.Errorf("--dns-zone-rg-name is required when --assign-identity-roles or --assign-custom-hcp-roles is set")
+	}
+
 	return nil
 }