Merge pull request #9922 from barbacbd/OCPBUGS-61111

OCPBUGS-61111: Allow user to BYO private zone without specifying name

Author: openshift-merge-bot[bot]

pkg/asset/cluster/tfvars/tfvars.go

@@ -517,10 +517,11 @@ func (t *TerraformVariables) Generate(ctx context.Context, parents asset.Parents
 			}
 
 			// Set the private zone
-			privateZoneName, _, err = manifests.GetGCPPrivateZoneName(ctx, client, installConfig, clusterID.InfraID)
+			params, err := manifests.GetGCPPrivateZoneInfo(ctx, client, installConfig, clusterID.InfraID)
 			if err != nil {
 				return fmt.Errorf("failed to find gcp private dns zone: %w", err)
 			}
+			privateZoneName = params.Name
 		}
 
 		ctx, cancel := context.WithTimeout(ctx, 60*time.Second)

pkg/asset/manifests/dns.go

@@ -181,11 +181,12 @@ func (d *DNS) Generate(ctx context.Context, dependencies asset.Parents) error {
 		// projects/{projectID}/managedZones/{zoneID}. This will allow
 		// the installer to pass the project without a new field in the
 		// DNSZone struct.
-		dnsZoneProject, privateZoneID := GetPrivateDNSZoneAndProject(installConfig)
-		if privateZoneID == "" {
-			privateZoneID = GCPDefaultPrivateZoneID(clusterID.InfraID)
+		params, err := GetGCPPrivateZoneInfo(ctx, client, installConfig, clusterID.InfraID)
+		if err != nil {
+			return fmt.Errorf("failed to get private zone info: %w", err)
 		}
-		privateZoneName := fmt.Sprintf("projects/%s/managedZones/%s", dnsZoneProject, privateZoneID)
+
+		privateZoneName := fmt.Sprintf("projects/%s/managedZones/%s", params.Project, params.Name)
 		logrus.Infof("generating GCP Private DNS Zone %s", privateZoneName)
 		config.Spec.PrivateZone = &configv1.DNSZone{ID: privateZoneName}
 
@@ -252,69 +253,62 @@ func GCPNetworkName(project, network string) string {
 	return fmt.Sprintf("https://www.googleapis.com/compute/v1/projects/%s/global/networks/%s", project, network)
 }
 
-// GetPrivateDNSZoneAndProject gets the private dns zone name and project where the dns records should reside.
-func GetPrivateDNSZoneAndProject(installConfig *installconfig.InstallConfig) (string, string) {
-	project := installConfig.Config.GCP.ProjectID
-	zone := ""
-	if installConfig.Config.GCP.Network == "" || installConfig.Config.GCP.NetworkProjectID == "" {
-		return project, zone
-	}
-
-	icdns := installConfig.Config.GCP.DNS
-	if icdns != nil && icdns.PrivateZone != nil {
-		if icdns.PrivateZone.ProjectID != "" {
-			project = icdns.PrivateZone.ProjectID
-		}
-		zone = icdns.PrivateZone.Name
-	}
-	return project, zone
-}
-
 // GCPDefaultPrivateZoneID returns the default name for a gcp private dns zone. This zone name will be used during
 // installations where the user has not provided a private zone name (xpn installs only), no
 // preexisting private dns zone is found (xpn installs only), and default installation cases.
 func GCPDefaultPrivateZoneID(clusterID string) string {
 	return fmt.Sprintf("%s-private-zone", clusterID)
 }
 
-// GetGCPPrivateZoneName attempts to find the name of the private zone for GCP installs. When a shared vpc install
+// GetGCPPrivateZoneInfo attempts to find the name of the private zone for GCP installs. When a shared vpc install
 // occurs, a precreated zone may be used. If a zone is found (in this instance), then the zone should be paired with
 // the network that is supplied through the install config (when applicable).
-func GetGCPPrivateZoneName(ctx context.Context, client *icgcp.Client, installConfig *installconfig.InstallConfig, clusterID string) (string, bool, error) {
-	privateZoneID := GCPDefaultPrivateZoneID(clusterID)
-	shouldCreateZone := true
+func GetGCPPrivateZoneInfo(ctx context.Context, client *icgcp.Client, installConfig *installconfig.InstallConfig, clusterID string) (gcptypes.DNSZoneParams, error) {
+	params := gcptypes.DNSZoneParams{
+		// Force set the private zone ID to an empty string to ensure
+		// the search for DNS zones looks for ANY not a specific zone.
+		// This is required, because the user may enter no zone information
+		// but still wish to bring a private zone during xpn installs (in this
+		// case it must exist in the `projectID`).
+		Name:             "",
+		InstallerCreated: true,
+		IsPublic:         false,
+		BaseDomain:       installConfig.Config.ClusterDomain(),
+		Project:          installConfig.Config.GCP.ProjectID,
+	}
 
-	if installConfig.Config.GCP.NetworkProjectID != "" {
-		project, privateZoneName := GetPrivateDNSZoneAndProject(installConfig)
-		if privateZoneName != "" {
+	if installConfig.Config.GCP.NetworkProjectID != "" && installConfig.Config.GCP.Network != "" {
+		icdns := installConfig.Config.GCP.DNS
+		if icdns != nil && icdns.PrivateZone != nil {
+			if icdns.PrivateZone.ProjectID != "" {
+				params.Project = icdns.PrivateZone.ProjectID
+			}
 			// Override the default with the name provided. If this zone does not exist, then
-			// this should still be returned.
-			privateZoneID = privateZoneName
+			// this should still be returned as valid.
+			params.Name = icdns.PrivateZone.Name
 		}
 
-		zone, err := client.GetDNSZoneFromParams(ctx, gcptypes.DNSZoneParams{
-			Project:    project,
-			Name:       privateZoneID,
-			IsPublic:   false,
-			BaseDomain: installConfig.Config.ClusterDomain(),
-		})
+		zone, err := client.GetDNSZoneFromParams(ctx, params)
 		if err != nil {
 			// Currently, the only time that a private zone lookup will produce an error is if we
 			// failed to find the dns zones. That should result in an error returned here too.
-			return privateZoneID, true, fmt.Errorf("private dns zone %s does not exist or is invalid: %w", privateZoneID, err)
+			return params, fmt.Errorf("private dns zone %s does not exist or is invalid: %w", params.Name, err)
 		}
 		if zone == nil {
 			// CORS-4012: The user may specify a zone to be created if it does not exist.
 			// Do not fail if the specified zone does not exist.
-			return privateZoneID, true, nil
+			if params.Name == "" {
+				params.Name = GCPDefaultPrivateZoneID(clusterID)
+			}
+			return params, nil
 		}
 
-		if installConfig.Config.GCP.Network != "" {
-			privateZoneID = zone.Name
-			shouldCreateZone = false
-		}
+		params.Name = zone.Name
+		params.InstallerCreated = false
+	} else {
+		params.Name = GCPDefaultPrivateZoneID(clusterID)
 	}
-	return privateZoneID, shouldCreateZone, nil
+	return params, nil
 }
 
 // Files returns the files generated by the asset.

pkg/infrastructure/gcp/clusterapi/clusterapi.go

@@ -238,7 +238,7 @@ func (p Provider) InfraReady(ctx context.Context, in clusterapi.InfraReadyInput)
 		}
 
 		// Create the public (optional) and private dns records
-		if err := createDNSRecords(ctx, in.InstallConfig, in.InfraID, apiIPAddress, apiIntIPAddress); err != nil {
+		if err := createDNSRecords(ctx, client, in.InstallConfig, in.InfraID, apiIPAddress, apiIntIPAddress); err != nil {
 			return fmt.Errorf("failed to create DNS records: %w", err)
 		}
 	}

pkg/infrastructure/gcp/clusterapi/dns.go

@@ -19,7 +19,7 @@ var (
 	errNotFound = errors.New("not found")
 )
 
-func getDNSZoneName(ctx context.Context, ic *installconfig.InstallConfig, isPublic bool) (string, error) {
+func getDNSZoneName(ctx context.Context, ic *installconfig.InstallConfig, clusterID string, isPublic bool) (string, error) {
 	ctx, cancel := context.WithTimeout(ctx, time.Minute*1)
 	defer cancel()
 
@@ -31,11 +31,16 @@ func getDNSZoneName(ctx context.Context, ic *installconfig.InstallConfig, isPubl
 	cctx, ccancel := context.WithTimeout(ctx, time.Minute*1)
 	defer ccancel()
 
-	domain := ic.Config.BaseDomain
-	project := ic.Config.GCP.ProjectID
-	if !isPublic {
-		project, _ = manifests.GetPrivateDNSZoneAndProject(ic)
-		domain = ic.Config.ClusterDomain()
+	params, err := manifests.GetGCPPrivateZoneInfo(ctx, client, ic, clusterID)
+	if err != nil {
+		return "", fmt.Errorf("failed to get private zone info: %w", err)
+	}
+
+	domain := params.BaseDomain
+	project := params.Project
+	if isPublic {
+		project = ic.Config.GCP.ProjectID
+		domain = ic.Config.BaseDomain
 	}
 
 	zone, err := client.GetDNSZone(cctx, project, domain, isPublic)
@@ -57,20 +62,20 @@ type recordSet struct {
 }
 
 // createRecordSets will create a list of records that will be created during the install.
-func createRecordSets(ctx context.Context, ic *installconfig.InstallConfig, clusterID, apiIP, apiIntIP string) ([]recordSet, error) {
+func createRecordSets(ctx context.Context, client *gcpic.Client, ic *installconfig.InstallConfig, clusterID, apiIP, apiIntIP string) ([]recordSet, error) {
 	ctx, cancel := context.WithTimeout(ctx, time.Minute*1)
 	defer cancel()
 
-	project, privateZoneName := manifests.GetPrivateDNSZoneAndProject(ic)
-	if privateZoneName == "" {
-		privateZoneName = manifests.GCPDefaultPrivateZoneID(clusterID)
+	privateZoneParams, err := manifests.GetGCPPrivateZoneInfo(ctx, client, ic, clusterID)
+	if err != nil {
+		return nil, fmt.Errorf("failed to get private zone info for record creation: %w", err)
 	}
 
 	records := []recordSet{
 		{
 			// api_internal
-			projectID: project,
-			zoneName:  privateZoneName,
+			projectID: privateZoneParams.Project,
+			zoneName:  privateZoneParams.Name,
 			record: &dns.ResourceRecordSet{
 				Name:    fmt.Sprintf("api-int.%s.", ic.Config.ClusterDomain()),
 				Type:    "A",
@@ -80,8 +85,8 @@ func createRecordSets(ctx context.Context, ic *installconfig.InstallConfig, clus
 		},
 		{
 			// api_external_internal_zone
-			projectID: project,
-			zoneName:  privateZoneName,
+			projectID: privateZoneParams.Project,
+			zoneName:  privateZoneParams.Name,
 			record: &dns.ResourceRecordSet{
 				Name:    fmt.Sprintf("api.%s.", ic.Config.ClusterDomain()),
 				Type:    "A",
@@ -92,7 +97,7 @@ func createRecordSets(ctx context.Context, ic *installconfig.InstallConfig, clus
 	}
 
 	if ic.Config.Publish == types.ExternalPublishingStrategy {
-		existingPublicZoneName, err := getDNSZoneName(ctx, ic, true)
+		existingPublicZoneName, err := getDNSZoneName(ctx, ic, clusterID, true)
 		if err != nil {
 			return nil, fmt.Errorf("failed to find a public zone: %w", err)
 		}
@@ -114,14 +119,14 @@ func createRecordSets(ctx context.Context, ic *installconfig.InstallConfig, clus
 }
 
 // createDNSRecords will get the list of records to be created and execute their creation through the gcp dns api.
-func createDNSRecords(ctx context.Context, ic *installconfig.InstallConfig, clusterID, apiIP, apiIntIP string) error {
+func createDNSRecords(ctx context.Context, client *gcpic.Client, ic *installconfig.InstallConfig, clusterID, apiIP, apiIntIP string) error {
 	// TODO: use the opts for the service to restrict scopes see google.golang.org/api/option.WithScopes
 	dnsService, err := gcpic.GetDNSService(ctx, ic.Config.GCP.ServiceEndpoints)
 	if err != nil {
 		return fmt.Errorf("failed to create the gcp dns service: %w", err)
 	}
 
-	records, err := createRecordSets(ctx, ic, clusterID, apiIP, apiIntIP)
+	records, err := createRecordSets(ctx, client, ic, clusterID, apiIP, apiIntIP)
 	if err != nil {
 		return err
 	}
@@ -147,25 +152,23 @@ func createPrivateManagedZone(ctx context.Context, ic *installconfig.InstallConf
 		return err
 	}
 
-	privateZoneID := manifests.GCPDefaultPrivateZoneID(clusterID)
+	params, err := manifests.GetGCPPrivateZoneInfo(ctx, client, ic, clusterID)
+	if err != nil {
+		return err
+	}
+
 	if ic.Config.GCP.NetworkProjectID != "" {
-		privateZoneName, shouldCreateZone, err := manifests.GetGCPPrivateZoneName(ctx, client, ic, clusterID)
-		if err != nil {
-			return err
-		}
-		if !shouldCreateZone {
-			logrus.Debugf("found private zone %s, skipping creation of private zone", privateZoneName)
-			privateZoneProject, _ := manifests.GetPrivateDNSZoneAndProject(ic)
+		if !params.InstallerCreated {
+			logrus.Debugf("found private zone %s, skipping creation of private zone", params.Name)
 			// The private zone already exists, so we need to add the shared label to the zone.
 			labels := mergeLabels(ic, clusterID, sharedLabelValue)
-			if err := client.UpdateDNSPrivateZoneLabels(ctx, ic.Config.ClusterDomain(), privateZoneProject, privateZoneName, labels); err != nil {
+			if err := client.UpdateDNSPrivateZoneLabels(ctx, ic.Config.ClusterDomain(), params.Project, params.Name, labels); err != nil {
 				return fmt.Errorf("failed to update dns private zone labels: %w", err)
 			}
 			return nil
 		}
-		privateZoneID = privateZoneName
 	}
-	logrus.Debugf("creating private zone %s", privateZoneID)
+	logrus.Debugf("creating private zone %s", params.Name)
 
 	// TODO: use the opts for the service to restrict scopes see google.golang.org/api/option.WithScopes
 	dnsService, err := gcpic.GetDNSService(ctx, ic.Config.GCP.ServiceEndpoints)
@@ -174,7 +177,7 @@ func createPrivateManagedZone(ctx context.Context, ic *installconfig.InstallConf
 	}
 
 	managedZone := &dns.ManagedZone{
-		Name:        privateZoneID,
+		Name:        params.Name,
 		Description: resourceDescription,
 		DnsName:     fmt.Sprintf("%s.", ic.Config.ClusterDomain()),
 		Visibility:  "private",
@@ -191,8 +194,7 @@ func createPrivateManagedZone(ctx context.Context, ic *installconfig.InstallConf
 	ctx, cancel := context.WithTimeout(ctx, time.Minute*1)
 	defer cancel()
 
-	project, _ := manifests.GetPrivateDNSZoneAndProject(ic)
-	if _, err = dnsService.ManagedZones.Create(project, managedZone).Context(ctx).Do(); err != nil {
+	if _, err = dnsService.ManagedZones.Create(params.Project, managedZone).Context(ctx).Do(); err != nil {
 		return fmt.Errorf("failed to create private managed zone: %w", err)
 	}
 

pkg/types/gcp/dns.go

@@ -17,4 +17,9 @@ type DNSZoneParams struct {
 	// BaseDomain is the base domain of the DNS zone.
 	// Note that either `Name` or `BaseDomain` must be provided.
 	BaseDomain string
+
+	// InstallerCreated is true when the DNS zone should be created
+	// by the OpenShift Installer (and will be owned by the
+	// OpenShift Installer).
+	InstallerCreated bool
 }