Merge pull request #7524 from sadasu/backport-7308-attempt-2

OCPBUGS-19670: [release-4.13] Allow different service account for xpn installs in gcp

data/data/gcp/cluster/dns/base.tf

@@ -9,6 +9,7 @@ resource "google_dns_managed_zone" "int" {
   description = local.description
   dns_name    = "${var.cluster_domain}."
   visibility  = "private"
+  project     = var.project_id
 
   private_visibility_config {
     networks {
@@ -27,6 +28,7 @@ resource "google_dns_record_set" "api_external" {
   ttl          = "60"
   managed_zone = var.public_zone_name
   rrdatas      = [var.api_external_lb_ip]
+  project      = var.project_id
 }
 
 resource "google_dns_record_set" "api_internal" {
@@ -35,6 +37,7 @@ resource "google_dns_record_set" "api_internal" {
   ttl          = "60"
   managed_zone = var.private_zone_name != "" ? var.private_zone_name : google_dns_managed_zone.int[0].name
   rrdatas      = [var.api_internal_lb_ip]
+  project      = var.project_id
 }
 
 resource "google_dns_record_set" "api_external_internal_zone" {
@@ -43,4 +46,5 @@ resource "google_dns_record_set" "api_external_internal_zone" {
   ttl          = "60"
   managed_zone = var.private_zone_name != "" ? var.private_zone_name : google_dns_managed_zone.int[0].name
   rrdatas      = [var.api_internal_lb_ip]
+  project      = var.project_id
 }

data/data/gcp/cluster/dns/variables.tf

@@ -38,3 +38,7 @@ variable "public_endpoints" {
   description = "If the cluster should have externally accessible resources."
 }
 
+variable "project_id" {
+  type        = string
+  description = "The target GCP project for the cluster."
+}

data/data/gcp/cluster/iam/main.tf

@@ -3,22 +3,19 @@ locals {
 }
 
 resource "google_service_account" "worker-node-sa" {
-  count        = var.service_account == "" ? 1 : 0
   account_id   = "${var.cluster_id}-w"
   display_name = "${var.cluster_id}-worker-node"
   description  = local.description
 }
 
 resource "google_project_iam_member" "worker-compute-viewer" {
-  count   = var.service_account == "" ? 1 : 0
   project = var.project_id
   role    = "roles/compute.viewer"
-  member  = "serviceAccount:${google_service_account.worker-node-sa[0].email}"
+  member  = "serviceAccount:${google_service_account.worker-node-sa.email}"
 }
 
 resource "google_project_iam_member" "worker-storage-admin" {
-  count   = var.service_account == "" ? 1 : 0
   project = var.project_id
   role    = "roles/storage.admin"
-  member  = "serviceAccount:${google_service_account.worker-node-sa[0].email}"
+  member  = "serviceAccount:${google_service_account.worker-node-sa.email}"
 }

data/data/gcp/cluster/main.tf

@@ -84,6 +84,7 @@ module "dns" {
   api_external_lb_ip = module.network.cluster_public_ip
   api_internal_lb_ip = module.network.cluster_ip
   public_endpoints   = local.public_endpoints
+  project_id         = var.gcp_project_id
 }
 
 resource "google_compute_image" "cluster" {

data/data/install.openshift.io_installconfigs.yaml

@@ -439,6 +439,13 @@ spec:
                           - Enabled
                           - Disabled
                           type: string
+                        serviceAccount:
+                          description: ServiceAccount is the email of a gcp service
+                            account to be used for shared vpn installations. The provided
+                            service account will be attached to control-plane nodes
+                            in order to provide the permissions required by the cloud
+                            provider in the host project.
+                          type: string
                         tags:
                           description: Tags defines a set of network tags which will
                             be added to instances in the machineset
@@ -1357,6 +1364,13 @@ spec:
                         - Enabled
                         - Disabled
                         type: string
+                      serviceAccount:
+                        description: ServiceAccount is the email of a gcp service
+                          account to be used for shared vpn installations. The provided
+                          service account will be attached to control-plane nodes
+                          in order to provide the permissions required by the cloud
+                          provider in the host project.
+                        type: string
                       tags:
                         description: Tags defines a set of network tags which will
                           be added to instances in the machineset
@@ -2903,6 +2917,13 @@ spec:
                         - Enabled
                         - Disabled
                         type: string
+                      serviceAccount:
+                        description: ServiceAccount is the email of a gcp service
+                          account to be used for shared vpn installations. The provided
+                          service account will be attached to control-plane nodes
+                          in order to provide the permissions required by the cloud
+                          provider in the host project.
+                        type: string
                       tags:
                         description: Tags defines a set of network tags which will
                           be added to instances in the machineset

pkg/asset/installconfig/gcp/client.go

@@ -11,6 +11,7 @@ import (
 	"google.golang.org/api/cloudresourcemanager/v1"
 	compute "google.golang.org/api/compute/v1"
 	dns "google.golang.org/api/dns/v1"
+	iam "google.golang.org/api/iam/v1"
 	"google.golang.org/api/option"
 	"google.golang.org/api/serviceusage/v1"
 	"k8s.io/apimachinery/pkg/util/sets"
@@ -39,6 +40,7 @@ type API interface {
 	GetRecordSets(ctx context.Context, project, zone string) ([]*dns.ResourceRecordSet, error)
 	GetZones(ctx context.Context, project, filter string) ([]*compute.Zone, error)
 	GetEnabledServices(ctx context.Context, project string) ([]string, error)
+	GetServiceAccount(ctx context.Context, project, serviceAccount string) (string, error)
 	GetCredentials() *googleoauth.Credentials
 	GetImage(ctx context.Context, name string, project string) (*compute.Image, error)
 	GetProjectPermissions(ctx context.Context, project string, permissions []string) (sets.Set[string], error)
@@ -368,6 +370,24 @@ func (c *Client) getServiceUsageService(ctx context.Context) (*serviceusage.Serv
 	return svc, nil
 }
 
+// GetServiceAccount retrieves a service account from a project if it exists.
+func (c *Client) GetServiceAccount(ctx context.Context, project, serviceAccount string) (string, error) {
+	svc, err := iam.NewService(ctx)
+	if err != nil {
+		return "", errors.Wrapf(err, "failed create IAM service")
+	}
+
+	ctx, cancel := context.WithTimeout(ctx, 1*time.Minute)
+	defer cancel()
+
+	fullServiceAccountPath := fmt.Sprintf("projects/%s/serviceAccounts/%s", project, serviceAccount)
+	rsp, err := svc.Projects.ServiceAccounts.Get(fullServiceAccountPath).Context(ctx).Do()
+	if err != nil {
+		return "", errors.Wrapf(err, fmt.Sprintf("failed to find resource %s", fullServiceAccountPath))
+	}
+	return rsp.Name, nil
+}
+
 // GetCredentials returns the credentials used to authenticate the GCP session.
 func (c *Client) GetCredentials() *googleoauth.Credentials {
 	return c.ssn.Credentials

pkg/asset/installconfig/gcp/validation.go

@@ -57,6 +57,8 @@ func Validate(client API, ic *types.InstallConfig) error {
 	allErrs = append(allErrs, validateNetworks(client, ic, field.NewPath("platform").Child("gcp"))...)
 	allErrs = append(allErrs, validateInstanceTypes(client, ic)...)
 	allErrs = append(allErrs, validateCredentialMode(client, ic)...)
+	allErrs = append(allErrs, validatePreexistingServiceAccountXpn(client, ic)...)
+	allErrs = append(allErrs, validateServiceAccountPresent(client, ic)...)
 	allErrs = append(allErrs, validateMarketplaceImages(client, ic)...)
 
 	return allErrs.ToAggregate()
@@ -86,6 +88,21 @@ func ValidateInstanceType(client API, fieldPath *field.Path, project, zone, inst
 	return allErrs
 }
 
+func validateServiceAccountPresent(client API, ic *types.InstallConfig) field.ErrorList {
+	allErrs := field.ErrorList{}
+
+	if ic.GCP.NetworkProjectID != "" {
+		creds := client.GetCredentials()
+		if creds != nil && creds.JSON == nil {
+			if ic.ControlPlane.Platform.GCP != nil && ic.ControlPlane.Platform.GCP.ServiceAccount == "" {
+				errMsg := "service account must be provided when authentication credentials do not provide a service account"
+				allErrs = append(allErrs, field.Required(field.NewPath("controlPlane").Child("platform").Child("gcp").Child("serviceAccount"), errMsg))
+			}
+		}
+	}
+	return allErrs
+}
+
 // validateInstanceTypes checks that the user-provided instance types are valid.
 func validateInstanceTypes(client API, ic *types.InstallConfig) field.ErrorList {
 	allErrs := field.ErrorList{}
@@ -125,6 +142,27 @@ func validateInstanceTypes(client API, ic *types.InstallConfig) field.ErrorList
 	return allErrs
 }
 
+func validatePreexistingServiceAccountXpn(client API, ic *types.InstallConfig) field.ErrorList {
+	allErrs := field.ErrorList{}
+
+	if ic.GCP.NetworkProjectID != "" {
+		if ic.ControlPlane.Platform.GCP != nil && ic.ControlPlane.Platform.GCP.ServiceAccount != "" {
+			fldPath := field.NewPath("controlPlane").Child("platform").Child("gcp").Child("serviceAccount")
+
+			// The service account is required for resources in the host project.
+			serviceAccount, err := client.GetServiceAccount(context.Background(), ic.GCP.ProjectID, ic.ControlPlane.Platform.GCP.ServiceAccount)
+			if err != nil {
+				return append(allErrs, field.InternalError(fldPath, err))
+			}
+			if serviceAccount == "" {
+				return append(allErrs, field.NotFound(fldPath, ic.ControlPlane.Platform.GCP.ServiceAccount))
+			}
+		}
+	}
+
+	return allErrs
+}
+
 // ValidatePreExistingPublicDNS ensure no pre-existing DNS record exists in the public
 // DNS zone for cluster's Kubernetes API. If a PublicDNSZone is provided, the provided
 // zone is verified against the BaseDomain. If no zone is provided, the base domain is

pkg/asset/installconfig/gcp/validation_test.go

@@ -42,6 +42,8 @@ var (
 	validPublicZone    = "valid-short-public-zone"
 	invalidPublicZone  = "invalid-short-public-zone"
 	validBaseDomain    = "example.installer.domain."
+	validXpnSA         = "valid-example-sa@gcloud.serviceaccount.com"
+	invalidXpnSA       = "invalid-example-sa@gcloud.serviceaccount.com"
 
 	validPrivateDNSZone = dns.ManagedZone{
 		Name:    validPrivateZone,
@@ -90,6 +92,9 @@ var (
 	removeVPC                = func(ic *types.InstallConfig) { ic.GCP.Network = "" }
 	removeSubnets            = func(ic *types.InstallConfig) { ic.GCP.ComputeSubnet, ic.GCP.ControlPlaneSubnet = "", "" }
 	invalidClusterName       = func(ic *types.InstallConfig) { ic.ObjectMeta.Name = "testgoogletest" }
+	validNetworkProject      = func(ic *types.InstallConfig) { ic.GCP.NetworkProjectID = validProjectName }
+	validateXpnSA            = func(ic *types.InstallConfig) { ic.ControlPlane.Platform.GCP.ServiceAccount = validXpnSA }
+	invalidateXpnSA          = func(ic *types.InstallConfig) { ic.ControlPlane.Platform.GCP.ServiceAccount = invalidXpnSA }
 
 	machineTypeAPIResult = map[string]*compute.MachineType{
 		"n1-standard-1": {GuestCpus: 1, MemoryMb: 3840},
@@ -278,6 +283,17 @@ func TestGCPInstallConfigValidation(t *testing.T) {
 			expectedError:  true,
 			expectedErrMsg: "platform.gcp.region: Invalid value: \"us-east4\": invalid region",
 		},
+		{
+			name:          "Valid XPN Service Account",
+			edits:         editFunctions{validNetworkProject, validateXpnSA},
+			expectedError: false,
+		},
+		{
+			name:           "Invalid XPN Service Account",
+			edits:          editFunctions{validNetworkProject, invalidateXpnSA},
+			expectedError:  true,
+			expectedErrMsg: "controlPlane.platform.gcp.serviceAccount: Internal error\"",
+		},
 	}
 	mockCtrl := gomock.NewController(t)
 	defer mockCtrl.Finish()
@@ -331,6 +347,9 @@ func TestGCPInstallConfigValidation(t *testing.T) {
 	gcpClient.EXPECT().GetDNSZoneByName(gomock.Any(), gomock.Any(), validPrivateZone).Return(&validPrivateDNSZone, nil).AnyTimes()
 	gcpClient.EXPECT().GetDNSZoneByName(gomock.Any(), gomock.Any(), invalidPublicZone).Return(nil, fmt.Errorf("no matching DNS Zone found")).AnyTimes()
 
+	gcpClient.EXPECT().GetServiceAccount(gomock.Any(), validProjectName, validXpnSA).Return(validXpnSA, nil).AnyTimes()
+	gcpClient.EXPECT().GetServiceAccount(gomock.Any(), validProjectName, invalidXpnSA).Return("", fmt.Errorf("controlPlane.platform.gcp.serviceAccount: Internal error\"")).AnyTimes()
+
 	for _, tc := range cases {
 		t.Run(tc.name, func(t *testing.T) {
 			editedInstallConfig := validInstallConfig()
@@ -743,3 +762,69 @@ func TestValidateMarketplaceImages(t *testing.T) {
 		})
 	}
 }
+
+func TestValidateServiceAccountPresent(t *testing.T) {
+	cases := []struct {
+		name             string
+		creds            *googleoauth.Credentials
+		serviceAccount   string
+		networkProjectID string
+		expectedError    string
+	}{
+		{
+			name:  "Test no network project ID",
+			creds: &googleoauth.Credentials{},
+		},
+		{
+			name:             "Test network project ID with service account",
+			creds:            &googleoauth.Credentials{},
+			serviceAccount:   "test-service-account",
+			networkProjectID: "test-network-project",
+		},
+		{
+			name:             "Test network project ID service account and creds",
+			creds:            &googleoauth.Credentials{JSON: []byte("{}")},
+			serviceAccount:   "test-service-account",
+			networkProjectID: "test-network-project",
+		},
+		{
+			name:             "Test network project ID no creds",
+			creds:            &googleoauth.Credentials{JSON: nil},
+			networkProjectID: "test-network-project",
+			expectedError:    "controlPlane.platform.gcp.serviceAccount: Required value: service account must be provided when authentication credentials do not provide a service account",
+		},
+	}
+
+	mockCtrl := gomock.NewController(t)
+	defer mockCtrl.Finish()
+
+	for _, test := range cases {
+		gcpClient := mock.NewMockAPI(mockCtrl)
+		if test.networkProjectID != "" {
+			gcpClient.EXPECT().GetCredentials().Return(test.creds)
+		}
+
+		t.Run(test.name, func(t *testing.T) {
+			ic := types.InstallConfig{
+				ObjectMeta:      metav1.ObjectMeta{Name: "cluster-name"},
+				BaseDomain:      "base-domain",
+				Platform:        types.Platform{GCP: &gcp.Platform{ProjectID: "project-id", NetworkProjectID: test.networkProjectID}},
+				CredentialsMode: types.PassthroughCredentialsMode,
+				ControlPlane: &types.MachinePool{
+					Platform: types.MachinePoolPlatform{
+						GCP: &gcp.MachinePool{
+							ServiceAccount: test.serviceAccount,
+						},
+					},
+				},
+			}
+
+			errorList := validateServiceAccountPresent(gcpClient, &ic)
+			if errorList == nil && test.expectedError == "" {
+				assert.NoError(t, errorList.ToAggregate())
+			} else {
+				assert.Regexp(t, test.expectedError, errorList.ToAggregate())
+			}
+		})
+	}
+}