Firewallrules creation #1538
Firewallrules creation #1538
Conversation
k8s-ci-robot
added
release-note
|
@barbacbd: The label(s) In response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. |
k8s-ci-robot
added
the
cncf-cla: yes
k8s-ci-robot
added
the
needs-ok-to-test
|
Hi @barbacbd. Thanks for your PR. I'm waiting for a kubernetes-sigs member to verify that this patch is reasonable to test. If it is, they should reply with Once the patch is verified, the new status will be reflected by the I understand the commands that are listed here. Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. |
✅ Deploy Preview for kubernetes-sigs-cluster-api-gcp ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
k8s-ci-robot
added
the
size/XXL
|
Note this currently requires #1532 so the commits look a bit wonky. These will be removed when 1532 merges and it should look cleaner. |
| // lowercase letter, and all following characters (except for the last | ||
| // character) must be a dash, lowercase letter, or digit. The last character | ||
| // must be a lowercase letter or digit. | ||
| Name *string `json:"name,omitempty"` |
There was a problem hiding this comment.
Thinking about collisions here - should we use this as a suffix with the cluster name?
There was a problem hiding this comment.
What collision are you thinking @justinsb ? This should be nested so name collision shouldn't really matter and the hard part with nesting names is the max of 63 characters. Cluster names can get quite large too.
|
I like this from a functionality and code perspective. Does comparable functionality already exist in (for example) CAPA? If so, we're just "catching up" and that's great, if not we should probably discuss at the CAPI level to make sure we want to include this functionality here. As an alternative, I believe CAPZ lets you define additional resources using ASO. The GCP equivalent is KCC (and the AWS equivalent in ACK). I work on KCC so I am obviously biased here, but it is a good way to avoid having to reimplement (potentially) every GCP API in our CAPG API. |
Yes! CAPA allows you to specify |
|
I'm not a maintainer of this project, but I'm very interested in it. So I'll enable the test as a first step. |
k8s-ci-robot
added
ok-to-test
There was a problem hiding this comment.
WDYT about adding also a UserManaged mode where no firewall rules are added at all (I need it to pass some security checks) ?
Here is the full patch I'm running:
From a7a90b617903652c3c9931ee1656e7ea7f78ba76 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?R=C3=A9mi=20BUISSON?= <[email protected]>
Date: Fri, 10 Oct 2025 07:57:14 +0200
Subject: [PATCH 4/4] fix: review
---
api/v1beta1/types.go | 7 ++++++-
cloud/scope/cluster.go | 15 +++++++++------
cloud/scope/managedcluster.go | 16 ++++++++++------
...rastructure.cluster.x-k8s.io_gcpclusters.yaml | 2 ++
...ure.cluster.x-k8s.io_gcpclustertemplates.yaml | 2 ++
...ture.cluster.x-k8s.io_gcpmanagedclusters.yaml | 2 ++
6 files changed, 31 insertions(+), 13 deletions(-)
diff --git a/api/v1beta1/types.go b/api/v1beta1/types.go
index 92918a35..e2a2fd37 100644
--- a/api/v1beta1/types.go
+++ b/api/v1beta1/types.go
@@ -226,6 +226,7 @@ type FirewallSpec struct {
// "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.
+ // "UserManaged": The controller will create not create any default firewall rules.
// Defaults to "Managed".
// +optional
// +kubebuilder:default:="Managed"
@@ -249,7 +250,7 @@ const (
)
// RulesManagementPolicy is a string enum type for managing firewall rules.
-// +kubebuilder:validation:Enum=Managed;Unmanaged
+// +kubebuilder:validation:Enum=Managed;Unmanaged;UserManaged
type RulesManagementPolicy string
const (
@@ -260,6 +261,10 @@ const (
// 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"
+
+ // RulesManagementUserManaged indicates that the controller should only create and manage
+ // user specified firewall rules.
+ RulesManagementUserManaged RulesManagementPolicy = "UserManaged"
)
// NetworkSpec encapsulates all things related to a GCP network.
diff --git a/cloud/scope/cluster.go b/cloud/scope/cluster.go
index 634fc29a..bd3b7f0b 100644
--- a/cloud/scope/cluster.go
+++ b/cloud/scope/cluster.go
@@ -20,6 +20,7 @@ import (
"context"
"fmt"
"strconv"
+ "strings"
"time"
"github.com/pkg/errors"
@@ -280,8 +281,10 @@ func (s *ClusterScope) SubnetSpecs() []*compute.Subnetwork {
// FirewallRulesSpec returns google compute firewall spec.
func (s *ClusterScope) FirewallRulesSpec() []*compute.Firewall {
- firewallRules := []*compute.Firewall{
- {
+ firewallRules := make([]*compute.Firewall, 0)
+
+ if s.GCPCluster.Spec.Network.FirewallSpec.RulesManagement == infrav1.RulesManagementManaged {
+ firewallRules = append(firewallRules, &compute.Firewall{
Name: fmt.Sprintf("allow-%s-healthchecks", s.Name()),
Network: s.NetworkLink(),
Allowed: []*compute.FirewallAllowed{
@@ -300,8 +303,8 @@ func (s *ClusterScope) FirewallRulesSpec() []*compute.Firewall {
TargetTags: []string{
s.Name() + "-control-plane",
},
- },
- {
+ })
+ firewallRules = append(firewallRules, &compute.Firewall{
Name: fmt.Sprintf("allow-%s-cluster", s.Name()),
Network: s.NetworkLink(),
Allowed: []*compute.FirewallAllowed{
@@ -318,7 +321,7 @@ func (s *ClusterScope) FirewallRulesSpec() []*compute.Firewall {
s.Name() + "-control-plane",
s.Name() + "-node",
},
- },
+ })
}
// Add user defined firewall rules.
@@ -341,7 +344,7 @@ func (s *ClusterScope) FirewallRulesSpec() []*compute.Firewall {
direction := string(ptr.Deref(rule.Direction, infrav1.FirewallRuleDirectionIngress))
firewallRules = append(firewallRules, &compute.Firewall{
- Name: ptr.Deref(rule.Name, fmt.Sprintf("%s-%s", s.Name(), direction)),
+ Name: ptr.Deref(rule.Name, fmt.Sprintf("%s-%s", s.Name(), strings.ToLower(direction))),
Description: ptr.Deref(rule.Description, fmt.Sprintf("Firewall rule %s is created by Cluster API GCP Provider.", s.Name())),
Network: s.NetworkLink(),
Allowed: allowed,
diff --git a/cloud/scope/managedcluster.go b/cloud/scope/managedcluster.go
index 56f6e267..0017a940 100644
--- a/cloud/scope/managedcluster.go
+++ b/cloud/scope/managedcluster.go
@@ -20,6 +20,7 @@ import (
"context"
"fmt"
"strconv"
+ "strings"
"github.com/pkg/errors"
"google.golang.org/api/compute/v1"
@@ -264,8 +265,10 @@ func (s *ManagedClusterScope) SubnetSpecs() []*compute.Subnetwork {
// FirewallRulesSpec returns google compute firewall spec.
func (s *ManagedClusterScope) FirewallRulesSpec() []*compute.Firewall {
- firewallRules := []*compute.Firewall{
- {
+ firewallRules := make([]*compute.Firewall, 0)
+
+ if s.GCPManagedCluster.Spec.Network.FirewallSpec.RulesManagement == infrav1.RulesManagementManaged {
+ firewallRules = append(firewallRules, &compute.Firewall{
Name: fmt.Sprintf("allow-%s-healthchecks", s.Name()),
Network: s.NetworkLink(),
Allowed: []*compute.FirewallAllowed{
@@ -284,8 +287,9 @@ func (s *ManagedClusterScope) FirewallRulesSpec() []*compute.Firewall {
TargetTags: []string{
s.Name() + "-control-plane",
},
- },
- {
+ })
+
+ firewallRules = append(firewallRules, &compute.Firewall{
Name: fmt.Sprintf("allow-%s-cluster", s.Name()),
Network: s.NetworkLink(),
Allowed: []*compute.FirewallAllowed{
@@ -302,7 +306,7 @@ func (s *ManagedClusterScope) FirewallRulesSpec() []*compute.Firewall {
s.Name() + "-control-plane",
s.Name() + "-node",
},
- },
+ })
}
// Add user defined firewall rules.
@@ -325,7 +329,7 @@ func (s *ManagedClusterScope) FirewallRulesSpec() []*compute.Firewall {
direction := string(ptr.Deref(rule.Direction, infrav1.FirewallRuleDirectionIngress))
firewallRules = append(firewallRules, &compute.Firewall{
- Name: ptr.Deref(rule.Name, fmt.Sprintf("%s-%s", s.Name(), direction)),
+ Name: ptr.Deref(rule.Name, fmt.Sprintf("%s-%s", s.Name(), strings.ToLower(direction))),
Description: ptr.Deref(rule.Description, fmt.Sprintf("Firewall rule %s is created by Cluster API GCP Provider.", s.Name())),
Network: s.NetworkLink(),
Allowed: allowed,
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 a9f869e0..62c933fc 100644
--- a/config/crd/bases/infrastructure.cluster.x-k8s.io_gcpclusters.yaml
+++ b/config/crd/bases/infrastructure.cluster.x-k8s.io_gcpclusters.yaml
@@ -374,10 +374,12 @@ spec:
"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.
+ "UserManaged": The controller will create not create any default firewall rules.
Defaults to "Managed".
enum:
- Managed
- Unmanaged
+ - UserManaged
type: string
type: object
hostProject:
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 98774396..9c8e3b12 100644
--- a/config/crd/bases/infrastructure.cluster.x-k8s.io_gcpclustertemplates.yaml
+++ b/config/crd/bases/infrastructure.cluster.x-k8s.io_gcpclustertemplates.yaml
@@ -394,10 +394,12 @@ spec:
"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.
+ "UserManaged": The controller will create not create any default firewall rules.
Defaults to "Managed".
enum:
- Managed
- Unmanaged
+ - UserManaged
type: string
type: object
hostProject:
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 cf79bd3e..b8b3c080 100644
--- a/config/crd/bases/infrastructure.cluster.x-k8s.io_gcpmanagedclusters.yaml
+++ b/config/crd/bases/infrastructure.cluster.x-k8s.io_gcpmanagedclusters.yaml
@@ -370,10 +370,12 @@ spec:
"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.
+ "UserManaged": The controller will create not create any default firewall rules.
Defaults to "Managed".
enum:
- Managed
- Unmanaged
+ - UserManaged
type: string
type: object
hostProject:
--
2.51.0Here are few minimal firewall rules needed then:
apiVersion: infrastructure.cluster.x-k8s.io/v1beta1
kind: GCPCluster
metadata:
name: test-cluster
namespace: flux-system
spec:
network:
name: test-cluster-network
firewall:
rulesManagement: "UserManaged"
firewallRules:
- name: test-cluster-healthchecks
targetTags:
- test-cluster-control-plane
sourceRanges:
- "35.191.0.0/16"
- "130.211.0.0/22"
allowed:
- IPProtocol: tcp
ports:
- "6443"
- name: test-cluster-cluster
targetTags:
- test-cluster-control-plane
- test-cluster-node
sourceTags:
- test-cluster-control-plane
- test-cluster-node
allowed:
- IPProtocol: tcp
ports:
- "53"
- "80"
- "179" # BGP
- "443"
- "10250"
- "1024-65535"
- IPProtocol: udp
ports:
- "53"
- "4789" # VXLAN
project: xxx
region: us-east4
failureDomains:
- us-east4-b
cloud/scope/cluster.go
Outdated
|
|
||
| direction := string(ptr.Deref(rule.Direction, infrav1.FirewallRuleDirectionIngress)) | ||
| firewallRules = append(firewallRules, &compute.Firewall{ | ||
| Name: ptr.Deref(rule.Name, fmt.Sprintf("%s-%s", s.Name(), direction)), |
There was a problem hiding this comment.
Hi there :-)
I tested your PR in a real use case, direction should be lowered otherwize google API returns a 400:
| Name: ptr.Deref(rule.Name, fmt.Sprintf("%s-%s", s.Name(), direction)), | |
| Name: ptr.Deref(rule.Name, fmt.Sprintf("%s-%s", s.Name(), strings.ToLower(direction))), |
cloud/scope/managedcluster.go
Outdated
|
|
||
| direction := string(ptr.Deref(rule.Direction, infrav1.FirewallRuleDirectionIngress)) | ||
| firewallRules = append(firewallRules, &compute.Firewall{ | ||
| Name: ptr.Deref(rule.Name, fmt.Sprintf("%s-%s", s.Name(), direction)), |
There was a problem hiding this comment.
| Name: ptr.Deref(rule.Name, fmt.Sprintf("%s-%s", s.Name(), direction)), | |
| Name: ptr.Deref(rule.Name, fmt.Sprintf("%s-%s", s.Name(), strings.ToLower(direction))), |
barbacbd
force-pushed
the
firewallrules_creation
branch
from
226f310 to
ca4f916
Compare
k8s-ci-robot
added
the
needs-rebase
… created. ** Currently the basic/default/required firewall rules are created by CAPG. Users should be given the ability to create the firewall rules associated with VPC that CAPG will create.
barbacbd
force-pushed
the
firewallrules_creation
branch
from
ca4f916 to
5803939
Compare
k8s-ci-robot
removed
the
needs-rebase
|
@barbacbd: The following test failed, say
Full PR test history. Your PR dashboard. Please help us cut down on flakes by linking to an open issue when you hit one in your PR. Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. I understand the commands that are listed here. |
|
@darkweaver87 There was some discussion about this in #1532. Instead of UserManaged the user should provide no firewall rules AND they should also set to |
|
Thanks @barbacbd for the info. I can understand the direction but then it should be possible to have the same "bring your own infrastructure" mode as AWS provider does. My proposition was just a quick win because providing all the infrastructure is another story I suppose :-) |
@darkweaver87 I think that this can be a start to a bring your own infrastructure. This is a change to help the downstream project. I think its a good start that maybe we should open an issue for a complete BYOI. What do you think? |
|
@darkweaver87: changing LGTM is restricted to collaborators In response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. |
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: barbacbd, darkweaver87 The full list of commands accepted by this bot can be found here.
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
6 participants
What type of PR is this?
/kind feature
/kind /api-change
What this PR does / why we need it:
Currently the basic/default/required firewall rules are created by CAPG.
Users should be given the ability to create the firewall rules associated with
VPC that CAPG will create.
Which issue(s) this PR fixes (optional, in
fixes #<issue number>(, fixes #<issue_number>, ...)format, will close the issue(s) when PR gets merged):Fixes #
Special notes for your reviewer:
Please confirm that if this PR changes any image versions, then that's the sole change this PR makes.
TODOs:
Release note: