Skip to content

CORS-4230: Add a firewall spec and the ability to manage or unmanaged firewall rule creation #1532

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
+180 −2
Open

CORS-4230: Add a firewall spec and the ability to manage or unmanaged firewall rule creation #1532

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
31 changes: 31 additions & 0 deletions api/v1beta1/types.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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.
Comment on lines +112 to +116
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If you kubectl describe, does this formatting print out correctly?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Honestly I haven't tried. This is the same formatting someone used on a suggested entry of mine in the past.

// Defaults to "Managed".
// +optional
// +kubebuilder:default:="Managed"
RulesManagement RulesManagementPolicy `json:"rulesManagement,omitempty"`
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So one nit: do we want people to have to write rulesManagement: Managed or rules: Managed

I think @JoelSpeed suggested rules, and I like it better from the perspective of not repeating the word "managed", but OTOH if we do end up adding extensible rules, maybe we want to use firewall.rules for that?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@justinsb would you prefer rules or policy ?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@justinsb I do have a similar PR #1538 that does introduce rules.

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thinking about ways this could go

  • Users want no rules management at all, will DIY it all
  • Users want the default rules only
  • Users want only a custom set of rules
  • Users want default and custom rules (?)

So for those four choice to be valid, we are going to want a discriminated union (IMO). That could look something like:

firewall:
  rules: DefaultManaged | Unmanaged | CustomManaged
  customManaged:
    defaultRules: Include | Exclude
    rules:
    - ...

Would we expect the custom rules to be on the same level as the rules/policy selector? If so, we wouldn't want the custom rules to be allowed when the rules are unmanaged

We would also need a way to say exclude the default rules still, in which case, you have an oddity of rules: Managed but defaultRules: Exclude and customRules: [] meaning that it does nothing? We could validate around this but its less intuitive IMO than doing the DU approach described above

Thoughts?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we could keep a value specifically for the default rules that are included in CAPG

// DefaultRulesManagement is the policy that applies to the default rules only. When the management policy is set to Unmanaged or the installation is using a shared VPC, the default firewall rules will not be created. 
// Defaults to "Managed".
// +optional
// +kubebuilder:default:="Managed"
DefaultRulesManagement RulesManagementPolicy `json:"defaultRulesManagement,omitempty"`

We should also have a field (this is currently in another PR) for the custom firewall rules. This includes the firewall rules that a caller would like to have created by CAPG. I don't believe that this list needs a value of Managed or Unmanaged. When the caller provides any firewall rules in this field they should be considered managed. I don't believe that there is a need to fill in this field with firewall rules and then state that they want these to be managed or created. I think that this is a fair assumption since we have no other need for the firewall rules. If there was another reason for specifying the firewall rules that could make sense but we have no other use.

CustomFirewallRules []FirewallRule

}

// 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.
Expand Down Expand Up @@ -137,6 +164,10 @@ type NetworkSpec struct {
// +optional
HostProject *string `json:"hostProject,omitempty"`

// Firewall configuration.
// +optional
Firewall FirewallSpec `json:"firewall,omitempty"`
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is a struct so needs omitzero, or to be a pointer. Is this project using go 1.24?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It is using go 1.24 I believe.


// 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
Expand Down
16 changes: 16 additions & 0 deletions api/v1beta1/zz_generated.deepcopy.go
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

1 change: 1 addition & 0 deletions cloud/interfaces.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -58,6 +58,7 @@ type ClusterGetter interface {
NetworkName() string
NetworkProject() string
IsSharedVpc() bool
SkipFirewallRuleCreation() bool
Network() *infrav1.Network
AdditionalLabels() infrav1.Labels
FailureDomains() clusterv1.FailureDomains
Expand Down
6 changes: 6 additions & 0 deletions cloud/scope/cluster.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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()
Expand Down
6 changes: 6 additions & 0 deletions cloud/scope/managedcluster.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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()
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How would you explain this relationship in the API documentation/validations?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There are no custom rules in this PR so this should be taken only with the information in this PR. If the installation includes a shared VPC or if the user has explicitly said firewall rules are unmanaged then the firewall rules should not be created. It is important to note that this will ONLY apply to the default rules.

}

// IsSharedVpc returns true If sharedVPC used else , returns false.
func (s *ManagedClusterScope) IsSharedVpc() bool {
return s.NetworkProject() != s.Project()
Expand Down
4 changes: 2 additions & 2 deletions cloud/services/compute/firewalls/reconcile.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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() {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

BTW I like this a lot, even if SkipFirewallRuleCreation just called IsSharedVpc, it is much more self-documenting to have self-descriptive functions like this and put the logic in those functions, rather than put the logic in the caller.

log.V(2).Info("Ignore Reconciling firewall resources")
return nil
}
log.Info("Reconciling firewall resources")
Expand Down
50 changes: 50 additions & 0 deletions cloud/services/compute/firewalls/reconcile_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down Expand Up @@ -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",
Expand Down Expand Up @@ -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) {
Expand Down
17 changes: 17 additions & 0 deletions config/crd/bases/infrastructure.cluster.x-k8s.io_gcpclusters.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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.
Expand Down
17 changes: 17 additions & 0 deletions config/crd/bases/infrastructure.cluster.x-k8s.io_gcpclustertemplates.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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.
Expand Down
17 changes: 17 additions & 0 deletions config/crd/bases/infrastructure.cluster.x-k8s.io_gcpmanagedclusters.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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.
Expand Down
17 changes: 17 additions & 0 deletions config/crd/bases/infrastructure.cluster.x-k8s.io_gcpmanagedclustertemplates.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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.
Expand Down