Skip to content

Commit 5092d4e

Browse files
barbacbdbarbacbd
committed
data/data/install.openshift.io_installconfigs.yaml:
pkg/types/gcp/platform.go: Add FirewallManagementPolicy. The policy will indicate whether the cluster or user will manage the firewall rules. Add validation to ensure that a network is provided when the install config is set to Unmanaged to FirewallManagement. pkg/types/gcp/metadata.go: Add the management policy to the metadata so that the bootstrap destroy process knows whether to delete the bootstrap firewall rules or not.
1 parent 68b4166 commit 5092d4e

File tree

16 files changed

+92
-153
lines changed

16 files changed

+92
-153
lines changed

‎data/data/install.openshift.io_installconfigs.yaml‎

Lines changed: 10 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -6099,6 +6099,16 @@ spec:
60996099
required:
61006100
- name
61016101
type: object
6102+
firewallRulesManagement:
6103+
default: Managed
6104+
description: |-
6105+
FirewallRulesManagement specifies the management policy for the cluster. Managed indicates that
6106+
the firewall rules will be created and destroyed by the cluster. Unmanaged indicates that the
6107+
user should create and destroy the firewall rules.
6108+
enum:
6109+
- Managed
6110+
- Unmanaged
6111+
type: string
61026112
network:
61036113
description: |-
61046114
Network specifies an existing VPC where the cluster should be created

‎pkg/asset/cluster/gcp/gcp.go‎

Lines changed: 7 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -23,11 +23,12 @@ func Metadata(config *types.InstallConfig) *gcp.Metadata {
2323
}
2424

2525
return &gcp.Metadata{
26-
Region: config.Platform.GCP.Region,
27-
ProjectID: config.Platform.GCP.ProjectID,
28-
NetworkProjectID: config.Platform.GCP.NetworkProjectID,
29-
PrivateZoneDomain: privateZoneDomain,
30-
PrivateZoneProjectID: privateZoneProject,
31-
Endpoint: config.Platform.GCP.Endpoint,
26+
Region: config.Platform.GCP.Region,
27+
ProjectID: config.Platform.GCP.ProjectID,
28+
NetworkProjectID: config.Platform.GCP.NetworkProjectID,
29+
PrivateZoneDomain: privateZoneDomain,
30+
PrivateZoneProjectID: privateZoneProject,
31+
Endpoint: config.Platform.GCP.Endpoint,
32+
FirewallRulesManagement: config.GCP.FirewallRulesManagement,
3233
}
3334
}

‎pkg/asset/installconfig/gcp/permissions.go‎

Lines changed: 0 additions & 49 deletions
This file was deleted.

‎pkg/asset/installconfig/gcp/validation.go‎

Lines changed: 0 additions & 23 deletions
Original file line numberDiff line numberDiff line change
@@ -71,7 +71,6 @@ func Validate(client API, ic *types.InstallConfig) error {
7171
allErrs = append(allErrs, validateMarketplaceImages(client, ic)...)
7272
allErrs = append(allErrs, validatePlatformKMSKeys(client, ic, field.NewPath("platform").Child("gcp"))...)
7373
allErrs = append(allErrs, validateServiceEndpointOverride(client, ic, field.NewPath("platform").Child("gcp"))...)
74-
allErrs = append(allErrs, validateFirewallPermissions(client, ic, field.NewPath("platform").Child("gcp"))...)
7574

7675
if err := validateUserTags(client, ic.Platform.GCP.ProjectID, ic.Platform.GCP.UserTags); err != nil {
7776
allErrs = append(allErrs, field.Invalid(field.NewPath("platform").Child("gcp").Child("userTags"), ic.Platform.GCP.UserTags, err.Error()))
@@ -877,25 +876,3 @@ func validateServiceEndpointOverride(client API, ic *types.InstallConfig, fieldP
877876

878877
return allErrs
879878
}
880-
881-
// validateFirewallPermissions validates that the user has firewall permissions OR when the user does not have
882-
// permissions to create firewall rules then a network is specified.
883-
func validateFirewallPermissions(client API, ic *types.InstallConfig, fieldPath *field.Path) field.ErrorList {
884-
allErrs := field.ErrorList{}
885-
886-
projectID := ic.GCP.ProjectID
887-
configField := "projectID"
888-
if ic.GCP.NetworkProjectID != "" {
889-
projectID = ic.GCP.NetworkProjectID
890-
configField = "networkProjectID"
891-
}
892-
893-
hasPermissions, err := client.ValidateServiceAccountHasPermissions(context.TODO(), projectID, []string{CreateGCPFirewallPermission})
894-
if err != nil {
895-
return append(allErrs, field.Invalid(fieldPath.Child(configField), projectID, err.Error()))
896-
}
897-
if !hasPermissions && ic.GCP.Network == "" {
898-
allErrs = append(allErrs, field.Required(fieldPath.Child("network"), "firewall rule creation permission is missing, an existing network is required"))
899-
}
900-
return allErrs
901-
}

‎pkg/asset/installconfig/gcp/validation_test.go‎

Lines changed: 1 addition & 18 deletions
Original file line numberDiff line numberDiff line change
@@ -51,7 +51,6 @@ var (
5151
invalidXpnSA = "[email protected]"
5252
validServiceEndpointURL = "https://computeexample.googleapis.com/compute/v1/"
5353
invalidServiceEndpointURL = "http://badstorage.googleapis"
54-
permissionsProject = "permissions-project"
5554

5655
// #nosec G101
5756
fakeCreds = `{
@@ -118,8 +117,6 @@ var (
118117
invalidateXpnSA = func(ic *types.InstallConfig) { ic.ControlPlane.Platform.GCP.ServiceAccount = invalidXpnSA }
119118
invalidateBaseDomain = func(ic *types.InstallConfig) { ic.BaseDomain = invalidBaseDomain }
120119
enableCustomDNS = func(ic *types.InstallConfig) { ic.GCP.UserProvisionedDNS = customDNS.UserProvisionedDNSEnabled }
121-
resetNetwork = func(ic *types.InstallConfig) { ic.GCP.Network = "" }
122-
validPermissionProject = func(ic *types.InstallConfig) { ic.GCP.ProjectID = permissionsProject }
123120

124121
invalidKeyRing = gcp.KMSKeyReference{
125122
Name: "invalidKeyName",
@@ -462,12 +459,6 @@ func TestGCPInstallConfigValidation(t *testing.T) {
462459
records: []*dns.ResourceRecordSet{},
463460
expectedError: false,
464461
},
465-
{
466-
name: "Invalid missing permissions and network",
467-
edits: editFunctions{resetNetwork, validPermissionProject},
468-
records: []*dns.ResourceRecordSet{},
469-
expectedError: true, // platform.gcp.network: Required value: firewall rule creation permission is missing, an existing network is required
470-
},
471462
}
472463
mockCtrl := gomock.NewController(t)
473464
defer mockCtrl.Finish()
@@ -477,9 +468,8 @@ func TestGCPInstallConfigValidation(t *testing.T) {
477468
errNotFound := &googleapi.Error{Code: http.StatusNotFound}
478469

479470
// Should get the list of projects.
480-
gcpClient.EXPECT().GetProjects(gomock.Any()).Return(map[string]string{"valid-project": "valid-project", permissionsProject: permissionsProject}, nil).AnyTimes()
471+
gcpClient.EXPECT().GetProjects(gomock.Any()).Return(map[string]string{"valid-project": "valid-project"}, nil).AnyTimes()
481472
gcpClient.EXPECT().GetProjectByID(gomock.Any(), "valid-project").Return(&cloudresourcemanager.Project{}, nil).AnyTimes()
482-
gcpClient.EXPECT().GetProjectByID(gomock.Any(), permissionsProject).Return(&cloudresourcemanager.Project{}, nil).AnyTimes()
483473
gcpClient.EXPECT().GetProjectByID(gomock.Any(), "invalid-project").Return(nil, errNotFound).AnyTimes()
484474
gcpClient.EXPECT().GetProjectByID(gomock.Any(), gomock.Any()).Return(nil, fmt.Errorf("error")).AnyTimes()
485475

@@ -490,7 +480,6 @@ func TestGCPInstallConfigValidation(t *testing.T) {
490480
gcpClient.EXPECT().GetRegions(gomock.Any(), invalidProjectName).Return(nil, fmt.Errorf("failed to get regions for project")).AnyTimes()
491481
// When passed a project that is valid but the region is not contained, an error should still occur
492482
gcpClient.EXPECT().GetRegions(gomock.Any(), validProjectName).Return([]string{validRegion}, nil).AnyTimes()
493-
gcpClient.EXPECT().GetRegions(gomock.Any(), permissionsProject).Return([]string{validRegion}, nil).AnyTimes()
494483

495484
// Should return the machine type as specified.
496485
for key, value := range machineTypeAPIResult {
@@ -509,7 +498,6 @@ func TestGCPInstallConfigValidation(t *testing.T) {
509498
// When passed a correct network, project, & region, returns valid subnets.
510499
// We will test incorrect subnets, by changing the install config.
511500
gcpClient.EXPECT().GetSubnetworks(gomock.Any(), validNetworkName, validProjectName, validRegion).Return(subnetAPIResult, nil).AnyTimes()
512-
gcpClient.EXPECT().GetSubnetworks(gomock.Any(), validNetworkName, permissionsProject, validRegion).Return(subnetAPIResult, nil).AnyTimes()
513501

514502
// When passed an incorrect network, project or region, return empty list.
515503
gcpClient.EXPECT().GetSubnetworks(gomock.Any(), gomock.Not(validNetworkName), gomock.Any(), gomock.Any()).Return([]*compute.Subnetwork{}, nil).AnyTimes()
@@ -544,14 +532,9 @@ func TestGCPInstallConfigValidation(t *testing.T) {
544532
gcpClient.EXPECT().GetKeyRing(gomock.Any(), &invalidKeyRing).Return(nil, fmt.Errorf("failed to find key ring invalidKeyRingName: data")).AnyTimes()
545533

546534
gcpClient.EXPECT().GetDNSZone(gomock.Any(), validProjectName, validBaseDomain, true).Return(&dns.ManagedZone{Name: validZone}, nil).AnyTimes()
547-
gcpClient.EXPECT().GetDNSZone(gomock.Any(), permissionsProject, validBaseDomain, true).Return(&dns.ManagedZone{Name: validZone}, nil).AnyTimes()
548535
gcpClient.EXPECT().GetDNSZone(gomock.Any(), invalidProjectName, validBaseDomain, true).Return(&dns.ManagedZone{Name: validZone}, nil).AnyTimes()
549536
gcpClient.EXPECT().GetDNSZone(gomock.Any(), validProjectName, invalidBaseDomain, true).Return(nil, fmt.Errorf("baseDomain: Not found: \"%s\"", invalidBaseDomain)).AnyTimes()
550537

551-
gcpClient.EXPECT().ValidateServiceAccountHasPermissions(gomock.Any(), permissionsProject, []string{CreateGCPFirewallPermission}).Return(false, nil).AnyTimes()
552-
gcpClient.EXPECT().ValidateServiceAccountHasPermissions(gomock.Any(), validProjectName, []string{CreateGCPFirewallPermission}).Return(true, nil).AnyTimes()
553-
gcpClient.EXPECT().ValidateServiceAccountHasPermissions(gomock.Any(), invalidProjectName, []string{CreateGCPFirewallPermission}).Return(false, fmt.Errorf("failed to get project permissions")).AnyTimes()
554-
555538
httpmock.Activate()
556539
defer httpmock.DeactivateAndReset()
557540

‎pkg/asset/manifests/cloudproviderconfig.go‎

Lines changed: 1 addition & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -15,7 +15,6 @@ import (
1515
"github.com/openshift/installer/pkg/asset"
1616
"github.com/openshift/installer/pkg/asset/installconfig"
1717
awsic "github.com/openshift/installer/pkg/asset/installconfig/aws"
18-
gcpic "github.com/openshift/installer/pkg/asset/installconfig/gcp"
1918
powervsconfig "github.com/openshift/installer/pkg/asset/installconfig/powervs"
2019
ibmcloudmachines "github.com/openshift/installer/pkg/asset/machines/ibmcloud"
2120
"github.com/openshift/installer/pkg/asset/manifests/azure"
@@ -180,17 +179,8 @@ func (cpc *CloudProviderConfig) Generate(ctx context.Context, dependencies asset
180179
subnet = installConfig.Config.GCP.ComputeSubnet
181180
}
182181

183-
projectID := installConfig.Config.Platform.GCP.ProjectID
184-
if networkProjectID := installConfig.Config.Platform.GCP.NetworkProjectID; networkProjectID != "" {
185-
projectID = networkProjectID
186-
}
187-
188-
hasFirewallRules, err := gcpic.HasPermissions(ctx, projectID, []string{gcpic.CreateGCPFirewallPermission}, installConfig.Config.GCP.Endpoint)
189-
if err != nil {
190-
return fmt.Errorf("failed to determine user firewall permissions: %w", err)
191-
}
192182
firewallManagement := gcpmanifests.FirewallManagementEnabled
193-
if !hasFirewallRules {
183+
if installConfig.Config.GCP.FirewallRulesManagement == gcptypes.UnmanagedFirewallRules {
194184
firewallManagement = gcpmanifests.FirewallManagementDisabled
195185
}
196186

‎pkg/asset/manifests/gcp/cluster.go‎

Lines changed: 3 additions & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -133,17 +133,9 @@ func GenerateClusterAssets(installConfig *installconfig.InstallConfig, clusterID
133133
capgLoadBalancerType = capg.Internal
134134
}
135135

136-
firewallRulesManagementPolicy := capg.RulesManagementUnmanaged
137-
projectID := installConfig.Config.GCP.ProjectID
138-
if installConfig.Config.GCP.NetworkProjectID != "" {
139-
projectID = installConfig.Config.GCP.NetworkProjectID
140-
}
141-
createFwRules, err := gcpic.HasPermissions(context.TODO(), projectID, []string{gcpic.CreateGCPFirewallPermission}, installConfig.Config.GCP.Endpoint)
142-
if err != nil {
143-
return nil, fmt.Errorf("failed to verify firewall rules: %w", err)
144-
}
145-
if createFwRules {
146-
firewallRulesManagementPolicy = capg.RulesManagementManaged
136+
firewallRulesManagementPolicy := capg.RulesManagementManaged
137+
if installConfig.Config.GCP.FirewallRulesManagement == gcp.UnmanagedFirewallRules {
138+
firewallRulesManagementPolicy = capg.RulesManagementUnmanaged
147139
}
148140

149141
gcpCluster := &capg.GCPCluster{

‎pkg/destroy/gcp/firewall.go‎

Lines changed: 5 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -30,8 +30,13 @@ func (o *ClusterUninstaller) listFirewalls(ctx context.Context) ([]cloudResource
3030
// that determines whether a particular result should be returned or not.
3131
func (o *ClusterUninstaller) listFirewallsWithFilter(ctx context.Context, fields string, filterFunc resourceFilterFunc) ([]cloudResource, error) {
3232
o.Logger.Debugf("Listing firewall rules")
33+
3334
results := []cloudResource{}
3435

36+
if o.firewallRulesManagement == gcp.UnmanagedFirewallRules {
37+
return results, nil
38+
}
39+
3540
findFirewallRules := func(projectID string) ([]cloudResource, error) {
3641
ctx, cancel := context.WithTimeout(ctx, defaultTimeout)
3742
defer cancel()

‎pkg/destroy/gcp/gcp.go‎

Lines changed: 14 additions & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -78,6 +78,8 @@ type ClusterUninstaller struct {
7878
// from metadata or by inferring it from existing cluster resources.
7979
cloudControllerUID string
8080

81+
firewallRulesManagement gcptypes.FirewallRulesManagementPolicy
82+
8183
errorTracker
8284
requestIDTracker
8385
pendingItemTracker
@@ -92,17 +94,18 @@ func New(logger logrus.FieldLogger, metadata *types.ClusterMetadata) (providers.
9294
}
9395

9496
return &ClusterUninstaller{
95-
Logger: logger,
96-
Region: metadata.ClusterPlatformMetadata.GCP.Region,
97-
ProjectID: metadata.ClusterPlatformMetadata.GCP.ProjectID,
98-
NetworkProjectID: metadata.ClusterPlatformMetadata.GCP.NetworkProjectID,
99-
PrivateZoneDomain: metadata.ClusterPlatformMetadata.GCP.PrivateZoneDomain,
100-
PrivateZoneProjectID: privateZoneProjectID,
101-
ClusterID: metadata.InfraID,
102-
cloudControllerUID: gcptypes.CloudControllerUID(metadata.InfraID),
103-
requestIDTracker: newRequestIDTracker(),
104-
pendingItemTracker: newPendingItemTracker(),
105-
endpoint: metadata.ClusterPlatformMetadata.GCP.Endpoint,
97+
Logger: logger,
98+
Region: metadata.ClusterPlatformMetadata.GCP.Region,
99+
ProjectID: metadata.ClusterPlatformMetadata.GCP.ProjectID,
100+
NetworkProjectID: metadata.ClusterPlatformMetadata.GCP.NetworkProjectID,
101+
PrivateZoneDomain: metadata.ClusterPlatformMetadata.GCP.PrivateZoneDomain,
102+
PrivateZoneProjectID: privateZoneProjectID,
103+
ClusterID: metadata.InfraID,
104+
cloudControllerUID: gcptypes.CloudControllerUID(metadata.InfraID),
105+
requestIDTracker: newRequestIDTracker(),
106+
pendingItemTracker: newPendingItemTracker(),
107+
endpoint: metadata.ClusterPlatformMetadata.GCP.Endpoint,
108+
firewallRulesManagement: metadata.ClusterPlatformMetadata.GCP.FirewallRulesManagement,
106109
}, nil
107110
}
108111

‎pkg/infrastructure/gcp/clusterapi/clusterapi.go‎

Lines changed: 4 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -283,18 +283,14 @@ func (p Provider) DestroyBootstrap(ctx context.Context, in clusterapi.BootstrapD
283283
return fmt.Errorf("failed to destroy storage: %w", err)
284284
}
285285

286+
if in.Metadata.GCP.FirewallRulesManagement == gcptypes.UnmanagedFirewallRules {
287+
return nil
288+
}
289+
286290
projectID := in.Metadata.GCP.ProjectID
287291
if in.Metadata.GCP.NetworkProjectID != "" {
288292
projectID = in.Metadata.GCP.NetworkProjectID
289293
}
290-
deleteFwRules, err := icgcp.HasPermissions(ctx, projectID, []string{icgcp.DeleteGCPFirewallPermission}, in.Metadata.GCP.Endpoint)
291-
if err != nil {
292-
return fmt.Errorf("failed to remove bootstrap firewall rules: %w", err)
293-
}
294-
if !deleteFwRules {
295-
return nil
296-
}
297-
298294
if err := removeBootstrapFirewallRules(ctx, in.Metadata.InfraID, projectID, in.Metadata.GCP.Endpoint); err != nil {
299295
return fmt.Errorf("failed to remove bootstrap firewall rules: %w", err)
300296
}

0 commit comments

Comments
 (0)