From 0d83ae4f89a3148594a02ec5515103a4ca272d95 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. Note: This commit has been cherry-picked --- api/v1beta1/types.go | 34 +++++++++++++ api/v1beta1/zz_generated.deepcopy.go | 16 ++++++ cloud/interfaces.go | 1 + cloud/scope/cluster.go | 8 +++ cloud/scope/managedcluster.go | 8 +++ cloud/services/compute/firewalls/reconcile.go | 4 +- .../compute/firewalls/reconcile_test.go | 50 +++++++++++++++++++ ...tructure.cluster.x-k8s.io_gcpclusters.yaml | 20 ++++++++ ....cluster.x-k8s.io_gcpclustertemplates.yaml | 20 ++++++++ ...e.cluster.x-k8s.io_gcpmanagedclusters.yaml | 20 ++++++++ 10 files changed, 179 insertions(+), 2 deletions(-) diff --git a/api/v1beta1/types.go b/api/v1beta1/types.go index a346e3f98..3e0178fd6 100644 --- a/api/v1beta1/types.go +++ b/api/v1beta1/types.go @@ -107,6 +107,36 @@ type Network struct { APIInternalForwardingRule *string `json:"apiInternalForwardingRule,omitempty"` } +// FirewallSpec contains configuration for the firewall. +type FirewallSpec struct { + // DefaultRulesManagement determines the management policy for the default firewall rules + // created by the controller. DefaultRulesManagement has no effect on user specified firewall + // rules. DefaultRulesManagement has no effect when a HostProject is specified. + // "Managed": The controller will create and manage firewall rules. + // "Unmanaged": The controller will not create or modify any firewall rules. If + // the RulesManagement is changed from Managed to Unmanaged after the firewall rules + // have been created, then the firewall rules will not be deleted. + // + // Defaults to "Managed". + // +optional + // +kubebuilder:default:="Managed" + DefaultRulesManagement RulesManagementPolicy `json:"defaultRulesManagement,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 +167,10 @@ type NetworkSpec struct { // +optional HostProject *string `json:"hostProject,omitempty"` + // Firewall configuration. + // +optional + Firewall FirewallSpec `json:"firewall,omitempty,omitzero"` + // 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 cf0ce0bf5..1bedc1d8e 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 @@ -887,6 +902,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 70cd3aec1..09df86fb5 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 977031e13..197c1a575 100644 --- a/cloud/scope/cluster.go +++ b/cloud/scope/cluster.go @@ -106,6 +106,14 @@ 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. If the RulesManagement for the default firewall rules is +// set to unmanaged or when the cluster will include a shared VPC, the default firewall +// rule creation will be skipped. +func (s *ClusterScope) SkipFirewallRuleCreation() bool { + return (s.GCPCluster.Spec.Network.Firewall.DefaultRulesManagement == 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 88767e537..c12ddd59d 100644 --- a/cloud/scope/managedcluster.go +++ b/cloud/scope/managedcluster.go @@ -129,6 +129,14 @@ 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. If the RulesManagement for the default firewall rules is +// set to unmanaged or when the cluster will include a shared VPC, the default firewall +// rule creation will be skipped. +func (s *ManagedClusterScope) SkipFirewallRuleCreation() bool { + return (s.GCPManagedCluster.Spec.Network.Firewall.DefaultRulesManagement == 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 e1fea076c..1430b07bd 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{ + DefaultRulesManagement: 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 3e14f9569..605b386ba 100644 --- a/config/crd/bases/infrastructure.cluster.x-k8s.io_gcpclusters.yaml +++ b/config/crd/bases/infrastructure.cluster.x-k8s.io_gcpclusters.yaml @@ -173,6 +173,26 @@ spec: Defaults to true. type: boolean + firewall: + description: Firewall configuration. + properties: + defaultRulesManagement: + default: Managed + description: |- + DefaultRulesManagement determines the management policy for the default firewall rules + created by the controller. DefaultRulesManagement has no effect on user specified firewall + rules. DefaultRulesManagement has no effect when a HostProject is specified. + "Managed": The controller will create and manage firewall rules. + "Unmanaged": The controller will not create or modify any firewall rules. If + the RulesManagement is changed from Managed to Unmanaged after the firewall rules + have been created, then the firewall rules 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 58ebe27b3..3d0dad279 100644 --- a/config/crd/bases/infrastructure.cluster.x-k8s.io_gcpclustertemplates.yaml +++ b/config/crd/bases/infrastructure.cluster.x-k8s.io_gcpclustertemplates.yaml @@ -192,6 +192,26 @@ spec: Defaults to true. type: boolean + firewall: + description: Firewall configuration. + properties: + defaultRulesManagement: + default: Managed + description: |- + DefaultRulesManagement determines the management policy for the default firewall rules + created by the controller. DefaultRulesManagement has no effect on user specified firewall + rules. DefaultRulesManagement has no effect when a HostProject is specified. + "Managed": The controller will create and manage firewall rules. + "Unmanaged": The controller will not create or modify any firewall rules. If + the RulesManagement is changed from Managed to Unmanaged after the firewall rules + have been created, then the firewall rules 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 f92fd9bc3..157239234 100644 --- a/config/crd/bases/infrastructure.cluster.x-k8s.io_gcpmanagedclusters.yaml +++ b/config/crd/bases/infrastructure.cluster.x-k8s.io_gcpmanagedclusters.yaml @@ -169,6 +169,26 @@ spec: Defaults to true. type: boolean + firewall: + description: Firewall configuration. + properties: + defaultRulesManagement: + default: Managed + description: |- + DefaultRulesManagement determines the management policy for the default firewall rules + created by the controller. DefaultRulesManagement has no effect on user specified firewall + rules. DefaultRulesManagement has no effect when a HostProject is specified. + "Managed": The controller will create and manage firewall rules. + "Unmanaged": The controller will not create or modify any firewall rules. If + the RulesManagement is changed from Managed to Unmanaged after the firewall rules + have been created, then the firewall rules 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.