From 56d42809918de81901c2ea2034aecef86aa0573c Mon Sep 17 00:00:00 2001 From: barbacbd Date: Thu, 18 Sep 2025 15:00:43 -0400 Subject: [PATCH] CORS-4230: Add a firewall spec and the ability to manage or unmanaged firewall rule creation api: Add API changes to Skip firewall rule creation. When unmanaged, the firewall rules will not be created. When this is the case, the firewall rules should exist prior to creating the network. This will allow ServiceAccounts to skip the rules: compute.firewalls.create cloud: Update the services and interfaces. The firewall service will no longer create firewall rules when the firewall policy is set to unmanaged OR when a shared vpc is used during installation and resource creation. --- api/v1beta1/types.go | 31 ++++++++++++ api/v1beta1/zz_generated.deepcopy.go | 16 ++++++ cloud/interfaces.go | 1 + cloud/scope/cluster.go | 6 +++ cloud/scope/managedcluster.go | 6 +++ cloud/services/compute/firewalls/reconcile.go | 4 +- .../compute/firewalls/reconcile_test.go | 50 +++++++++++++++++++ ...tructure.cluster.x-k8s.io_gcpclusters.yaml | 17 +++++++ ....cluster.x-k8s.io_gcpclustertemplates.yaml | 17 +++++++ ...e.cluster.x-k8s.io_gcpmanagedclusters.yaml | 17 +++++++ ...r.x-k8s.io_gcpmanagedclustertemplates.yaml | 17 +++++++ 11 files changed, 180 insertions(+), 2 deletions(-) diff --git a/api/v1beta1/types.go b/api/v1beta1/types.go index bbe110191..912c06e14 100644 --- a/api/v1beta1/types.go +++ b/api/v1beta1/types.go @@ -107,6 +107,33 @@ type Network struct { APIInternalForwardingRule *string `json:"apiInternalForwardingRule,omitempty"` } +// FirewallSpec contains configuration for the firewall. +type FirewallSpec struct { + // RulesManagement determines the management policy for firewall rules. + // "Managed": The controller will create and manage firewall rules. + // "Unmanaged": The controller will not touch any firewall rules. If this is + // changed to "Off" after rules have been created, they will not be + // deleted. + // Defaults to "Managed". + // +optional + // +kubebuilder:default:="Managed" + RulesManagement RulesManagementPolicy `json:"rulesManagement,omitempty"` +} + +// RulesManagementPolicy is a string enum type for managing firewall rules. +// +kubebuilder:validation:Enum=Managed;Unmanaged +type RulesManagementPolicy string + +const ( + // RulesManagementManaged indicates that the controller should create and manage + // firewall rules. This is the default behavior. + RulesManagementManaged RulesManagementPolicy = "Managed" + + // RulesManagementUnmanaged indicates that the controller should not create or manage + // any firewall rules. If rules already exist, they will be left as-is. + RulesManagementUnmanaged RulesManagementPolicy = "Unmanaged" +) + // NetworkSpec encapsulates all things related to a GCP network. type NetworkSpec struct { // Name is the name of the network to be used. @@ -137,6 +164,10 @@ type NetworkSpec struct { // +optional HostProject *string `json:"hostProject,omitempty"` + // Firewall configuration. + // +optional + Firewall FirewallSpec `json:"firewall,omitempty"` + // Mtu: Maximum Transmission Unit in bytes. The minimum value for this field is // 1300 and the maximum value is 8896. The suggested value is 1500, which is // the default MTU used on the Internet, or 8896 if you want to use Jumbo diff --git a/api/v1beta1/zz_generated.deepcopy.go b/api/v1beta1/zz_generated.deepcopy.go index 9d31ffc4f..328442236 100644 --- a/api/v1beta1/zz_generated.deepcopy.go +++ b/api/v1beta1/zz_generated.deepcopy.go @@ -133,6 +133,21 @@ func (in *Filter) DeepCopy() *Filter { return out } +// DeepCopyInto is an autogenerated deepcopy function, copying the receiver, writing into out. in must be non-nil. +func (in *FirewallSpec) DeepCopyInto(out *FirewallSpec) { + *out = *in +} + +// DeepCopy is an autogenerated deepcopy function, copying the receiver, creating a new FirewallSpec. +func (in *FirewallSpec) DeepCopy() *FirewallSpec { + if in == nil { + return nil + } + out := new(FirewallSpec) + in.DeepCopyInto(out) + return out +} + // DeepCopyInto is an autogenerated deepcopy function, copying the receiver, writing into out. in must be non-nil. func (in *GCPCluster) DeepCopyInto(out *GCPCluster) { *out = *in @@ -892,6 +907,7 @@ func (in *NetworkSpec) DeepCopyInto(out *NetworkSpec) { *out = new(string) **out = **in } + out.Firewall = in.Firewall } // DeepCopy is an autogenerated deepcopy function, copying the receiver, creating a new NetworkSpec. diff --git a/cloud/interfaces.go b/cloud/interfaces.go index 27abdcb42..c7cf79021 100644 --- a/cloud/interfaces.go +++ b/cloud/interfaces.go @@ -58,6 +58,7 @@ type ClusterGetter interface { NetworkName() string NetworkProject() string IsSharedVpc() bool + SkipFirewallRuleCreation() bool Network() *infrav1.Network AdditionalLabels() infrav1.Labels FailureDomains() clusterv1.FailureDomains diff --git a/cloud/scope/cluster.go b/cloud/scope/cluster.go index 706bdad3b..02db6af1a 100644 --- a/cloud/scope/cluster.go +++ b/cloud/scope/cluster.go @@ -106,6 +106,12 @@ func (s *ClusterScope) NetworkProject() string { return ptr.Deref(s.GCPCluster.Spec.Network.HostProject, s.Project()) } +// SkipFirewallRuleCreation returns whether the spec indicates that firewall rules +// should be created or not. +func (s *ClusterScope) SkipFirewallRuleCreation() bool { + return (s.GCPCluster.Spec.Network.Firewall.RulesManagement == infrav1.RulesManagementUnmanaged) || s.IsSharedVpc() +} + // IsSharedVpc returns true If sharedVPC used else , returns false. func (s *ClusterScope) IsSharedVpc() bool { return s.NetworkProject() != s.Project() diff --git a/cloud/scope/managedcluster.go b/cloud/scope/managedcluster.go index 51ec04a43..1a61e35b9 100644 --- a/cloud/scope/managedcluster.go +++ b/cloud/scope/managedcluster.go @@ -129,6 +129,12 @@ func (s *ManagedClusterScope) NetworkProject() string { return ptr.Deref(s.GCPManagedCluster.Spec.Network.HostProject, s.Project()) } +// SkipFirewallRuleCreation returns whether the spec indicates that firewall rules +// should be created or not. +func (s *ManagedClusterScope) SkipFirewallRuleCreation() bool { + return (s.GCPManagedCluster.Spec.Network.Firewall.RulesManagement == infrav1.RulesManagementUnmanaged) || s.IsSharedVpc() +} + // IsSharedVpc returns true If sharedVPC used else , returns false. func (s *ManagedClusterScope) IsSharedVpc() bool { return s.NetworkProject() != s.Project() diff --git a/cloud/services/compute/firewalls/reconcile.go b/cloud/services/compute/firewalls/reconcile.go index 4047f6548..0d65a2d9d 100644 --- a/cloud/services/compute/firewalls/reconcile.go +++ b/cloud/services/compute/firewalls/reconcile.go @@ -28,8 +28,8 @@ import ( // Reconcile reconcile cluster firewall compoenents. func (s *Service) Reconcile(ctx context.Context) error { log := log.FromContext(ctx) - if s.scope.IsSharedVpc() { - log.V(2).Info("Shared VPC enabled. Ignore Reconciling firewall resources") + if s.scope.SkipFirewallRuleCreation() { + log.V(2).Info("Ignore Reconciling firewall resources") return nil } log.Info("Reconciling firewall resources") diff --git a/cloud/services/compute/firewalls/reconcile_test.go b/cloud/services/compute/firewalls/reconcile_test.go index 7a927aed5..d1a508354 100644 --- a/cloud/services/compute/firewalls/reconcile_test.go +++ b/cloud/services/compute/firewalls/reconcile_test.go @@ -109,6 +109,34 @@ var fakeGCPClusterSharedVPC = &infrav1.GCPCluster{ }, } +var fakeGCPClusterUnmanagedFirewalls = &infrav1.GCPCluster{ + ObjectMeta: metav1.ObjectMeta{ + Name: "my-cluster", + Namespace: "default", + }, + Spec: infrav1.GCPClusterSpec{ + Project: "my-proj", + Region: "us-central1", + Network: infrav1.NetworkSpec{ + Name: ptr.To("my-network"), + Subnets: infrav1.Subnets{ + infrav1.SubnetSpec{ + Name: "workers", + CidrBlock: "10.0.0.1/28", + Region: "us-central1", + Purpose: ptr.To[string]("INTERNAL_HTTPS_LOAD_BALANCER"), + }, + }, + Firewall: infrav1.FirewallSpec{ + RulesManagement: infrav1.RulesManagementUnmanaged, + }, + }, + }, + Status: infrav1.GCPClusterStatus{ + Network: infrav1.Network{}, + }, +} + type testCase struct { name string scope func() Scope @@ -146,6 +174,18 @@ func TestService_Reconcile(t *testing.T) { t.Fatal(err) } + clusterScopeUnmanagedFirewalls, err := scope.NewClusterScope(context.TODO(), scope.ClusterScopeParams{ + Client: fakec, + Cluster: fakeCluster, + GCPCluster: fakeGCPClusterUnmanagedFirewalls, + GCPServices: scope.GCPServices{ + Compute: &compute.Service{}, + }, + }) + if err != nil { + t.Fatal(err) + } + tests := []testCase{ { name: "firewall rule does not exist successful create", @@ -211,6 +251,16 @@ func TestService_Reconcile(t *testing.T) { }, }, }, + { + name: "firewall return no error using unmanaged firewall settings", + scope: func() Scope { return clusterScopeUnmanagedFirewalls }, + mockFirewalls: &cloud.MockFirewalls{ + ProjectRouter: &cloud.SingleProjectRouter{ID: "my-proj"}, + Objects: map[meta.Key]*cloud.MockFirewallsObj{ + *meta.GlobalKey(fmt.Sprintf("allow-%s-healthchecks", fakeGCPCluster.Name)): {}, + }, + }, + }, } for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { diff --git a/config/crd/bases/infrastructure.cluster.x-k8s.io_gcpclusters.yaml b/config/crd/bases/infrastructure.cluster.x-k8s.io_gcpclusters.yaml index ea314885b..d25773cf3 100644 --- a/config/crd/bases/infrastructure.cluster.x-k8s.io_gcpclusters.yaml +++ b/config/crd/bases/infrastructure.cluster.x-k8s.io_gcpclusters.yaml @@ -179,6 +179,23 @@ spec: Defaults to true. type: boolean + firewall: + description: Firewall configuration. + properties: + rulesManagement: + default: Managed + description: |- + RulesManagement determines the management policy for firewall rules. + "Managed": The controller will create and manage firewall rules. + "Unmanaged": The controller will not touch any firewall rules. If this is + changed to "Off" after rules have been created, they will not be + deleted. + Defaults to "Managed". + enum: + - Managed + - Unmanaged + type: string + type: object hostProject: description: HostProject is the name of the project hosting the shared VPC network resources. diff --git a/config/crd/bases/infrastructure.cluster.x-k8s.io_gcpclustertemplates.yaml b/config/crd/bases/infrastructure.cluster.x-k8s.io_gcpclustertemplates.yaml index b55917eea..b5dac7cb9 100644 --- a/config/crd/bases/infrastructure.cluster.x-k8s.io_gcpclustertemplates.yaml +++ b/config/crd/bases/infrastructure.cluster.x-k8s.io_gcpclustertemplates.yaml @@ -198,6 +198,23 @@ spec: Defaults to true. type: boolean + firewall: + description: Firewall configuration. + properties: + rulesManagement: + default: Managed + description: |- + RulesManagement determines the management policy for firewall rules. + "Managed": The controller will create and manage firewall rules. + "Unmanaged": The controller will not touch any firewall rules. If this is + changed to "Off" after rules have been created, they will not be + deleted. + Defaults to "Managed". + enum: + - Managed + - Unmanaged + type: string + type: object hostProject: description: HostProject is the name of the project hosting the shared VPC network resources. diff --git a/config/crd/bases/infrastructure.cluster.x-k8s.io_gcpmanagedclusters.yaml b/config/crd/bases/infrastructure.cluster.x-k8s.io_gcpmanagedclusters.yaml index 573b66583..4ff821c5b 100644 --- a/config/crd/bases/infrastructure.cluster.x-k8s.io_gcpmanagedclusters.yaml +++ b/config/crd/bases/infrastructure.cluster.x-k8s.io_gcpmanagedclusters.yaml @@ -175,6 +175,23 @@ spec: Defaults to true. type: boolean + firewall: + description: Firewall configuration. + properties: + rulesManagement: + default: Managed + description: |- + RulesManagement determines the management policy for firewall rules. + "Managed": The controller will create and manage firewall rules. + "Unmanaged": The controller will not touch any firewall rules. If this is + changed to "Off" after rules have been created, they will not be + deleted. + Defaults to "Managed". + enum: + - Managed + - Unmanaged + type: string + type: object hostProject: description: HostProject is the name of the project hosting the shared VPC network resources. diff --git a/config/crd/bases/infrastructure.cluster.x-k8s.io_gcpmanagedclustertemplates.yaml b/config/crd/bases/infrastructure.cluster.x-k8s.io_gcpmanagedclustertemplates.yaml index d527cb3f2..05806ee3a 100644 --- a/config/crd/bases/infrastructure.cluster.x-k8s.io_gcpmanagedclustertemplates.yaml +++ b/config/crd/bases/infrastructure.cluster.x-k8s.io_gcpmanagedclustertemplates.yaml @@ -169,6 +169,23 @@ spec: Defaults to true. type: boolean + firewall: + description: Firewall configuration. + properties: + rulesManagement: + default: Managed + description: |- + RulesManagement determines the management policy for firewall rules. + "Managed": The controller will create and manage firewall rules. + "Unmanaged": The controller will not touch any firewall rules. If this is + changed to "Off" after rules have been created, they will not be + deleted. + Defaults to "Managed". + enum: + - Managed + - Unmanaged + type: string + type: object hostProject: description: HostProject is the name of the project hosting the shared VPC network resources.