From 2af62fd83e27d490952cf09e9cca755312edfd20 Mon Sep 17 00:00:00 2001 From: Gerrit Date: Thu, 21 Nov 2024 15:49:01 +0100 Subject: [PATCH 01/17] Allow specifying an initial firewall ruleset. This is handy for the capi-provider. --- api/v2/types_firewall.go | 30 ++++++++++++++++++++ controllers/firewall/reconcile.go | 46 +++++++++++++++++++++++-------- 2 files changed, 65 insertions(+), 11 deletions(-) diff --git a/api/v2/types_firewall.go b/api/v2/types_firewall.go index 34b18e8..f5f82fb 100644 --- a/api/v2/types_firewall.go +++ b/api/v2/types_firewall.go @@ -74,6 +74,9 @@ type FirewallSpec struct { // EgressRules contains egress rules configured for this firewall. EgressRules []EgressRuleSNAT `json:"egressRules,omitempty"` + // InitialRuleSet is the initial firewall ruleset applied before the firewall-controller starts running. + InitialRuleSet *InitialRuleSet `json:"initialRuleSet,omitempty"` + // Interval on which rule reconciliation by the firewall-controller should happen. Interval string `json:"interval,omitempty"` // DryRun if set to true, firewall rules are not applied. For devel-purposes only. @@ -122,6 +125,33 @@ type FirewallTemplateSpec struct { Spec FirewallSpec `json:"spec,omitempty"` } +// InitialRuleSet is the initial rule set deployed on the firewall. +type InitialRuleSet struct { + Egress []EgressRule + Ingress []IngressRule +} + +type NetworkProtocol string + +const ( + NetworkProtocolTCP = "TCP" + NetworkProtocolUDP = "UDP" +) + +type EgressRule struct { + Comment string + Ports []int32 + Protocol NetworkProtocol + To []string +} + +type IngressRule struct { + Comment string + Ports []int32 + Protocol NetworkProtocol + From []string +} + // EgressRuleSNAT holds a Source-NAT rule type EgressRuleSNAT struct { // NetworkID is the network for which the egress rule will be configured. diff --git a/controllers/firewall/reconcile.go b/controllers/firewall/reconcile.go index 89c07bc..0692abc 100644 --- a/controllers/firewall/reconcile.go +++ b/controllers/firewall/reconcile.go @@ -142,18 +142,42 @@ func (c *controller) createFirewall(r *controllers.Ctx[*v2.Firewall]) (*models.V tags = append(tags, v2.FirewallSetTag(ref.Name)) } + var rules *models.V1FirewallRules + if r.Target.Spec.InitialRuleSet != nil { + rules = &models.V1FirewallRules{} + + for _, rule := range r.Target.Spec.InitialRuleSet.Egress { + rules.Egress = append(rules.Egress, &models.V1FirewallEgressRule{ + Comment: rule.Comment, + Ports: rule.Ports, + Protocol: string(rule.Protocol), + To: rule.To, + }) + } + + for _, rule := range r.Target.Spec.InitialRuleSet.Ingress { + rules.Ingress = append(rules.Ingress, &models.V1FirewallIngressRule{ + Comment: rule.Comment, + From: rule.From, + Ports: rule.Ports, + Protocol: string(rule.Protocol), + }) + } + } + createRequest := &models.V1FirewallCreateRequest{ - Description: "created by firewall-controller-manager", - Name: r.Target.Name, - Hostname: r.Target.Name, - Sizeid: &r.Target.Spec.Size, - Projectid: &r.Target.Spec.Project, - Partitionid: &r.Target.Spec.Partition, - Imageid: &r.Target.Spec.Image, - SSHPubKeys: r.Target.Spec.SSHPublicKeys, - Networks: networks, - UserData: r.Target.Spec.Userdata, - Tags: tags, + Description: "created by firewall-controller-manager", + Name: r.Target.Name, + Hostname: r.Target.Name, + Sizeid: &r.Target.Spec.Size, + Projectid: &r.Target.Spec.Project, + Partitionid: &r.Target.Spec.Partition, + Imageid: &r.Target.Spec.Image, + SSHPubKeys: r.Target.Spec.SSHPublicKeys, + Networks: networks, + UserData: r.Target.Spec.Userdata, + Tags: tags, + FirewallRules: rules, } resp, err := c.c.GetMetal().Firewall().AllocateFirewall(firewall.NewAllocateFirewallParams().WithBody(createRequest).WithContext(r.Ctx), nil) From cce6694d72646e43ffd17cf8ad8ab31e83a62622 Mon Sep 17 00:00:00 2001 From: Valentin Knabel Date: Thu, 21 Nov 2024 16:08:59 +0100 Subject: [PATCH 02/17] feat: ingress and egress parsing and crds --- api/v2/types_firewall.go | 33 +++++--- api/v2/zz_generated.deepcopy.go | 84 +++++++++++++++++++ ...ll.metal-stack.io_firewalldeployments.yaml | 69 +++++++++++++++ .../firewall.metal-stack.io_firewalls.yaml | 65 ++++++++++++++ .../firewall.metal-stack.io_firewallsets.yaml | 69 +++++++++++++++ 5 files changed, 310 insertions(+), 10 deletions(-) diff --git a/api/v2/types_firewall.go b/api/v2/types_firewall.go index f5f82fb..f0ffb2e 100644 --- a/api/v2/types_firewall.go +++ b/api/v2/types_firewall.go @@ -127,29 +127,42 @@ type FirewallTemplateSpec struct { // InitialRuleSet is the initial rule set deployed on the firewall. type InitialRuleSet struct { - Egress []EgressRule - Ingress []IngressRule + // Egress rules to be deployed initially on the firewall. + Egress []EgressRule `json:"egress,omitempty"` + // Ingress rules to be deployed initially on the firewall. + Ingress []IngressRule `json:"ingress,omitempty"` } +// NetworkProtocol represents the kind of network protocol. type NetworkProtocol string const ( + // NetworkProtocolTCP represents tcp connections. NetworkProtocolTCP = "TCP" + // NetworkProtocolUDP represents udp connections. NetworkProtocolUDP = "UDP" ) type EgressRule struct { - Comment string - Ports []int32 - Protocol NetworkProtocol - To []string + // Comment provides a human readable description of this rule. + Comment string `json:"comment,omitempty"` + // Ports contains all affected network ports. + Ports []int32 `json:"ports"` + // Protocol constraints the protocol this rule applies to. + Protocol NetworkProtocol `json:"protocol"` + // To target addresses this rule applies to. May contain IPs or dns names. + To []string `json:"to"` } type IngressRule struct { - Comment string - Ports []int32 - Protocol NetworkProtocol - From []string + // Comment provides a human readable description of this rule. + Comment string `json:"comment,omitempty"` + // Ports contains all affected network ports. + Ports []int32 `json:"ports"` + // Protocol constraints the protocol this rule applies to. + Protocol NetworkProtocol `json:"protocol"` + // From source addresses this rule applies to. May contain IPs or dns names. + From []string `json:"from"` } // EgressRuleSNAT holds a Source-NAT rule diff --git a/api/v2/zz_generated.deepcopy.go b/api/v2/zz_generated.deepcopy.go index b9d9399..802ad3b 100644 --- a/api/v2/zz_generated.deepcopy.go +++ b/api/v2/zz_generated.deepcopy.go @@ -161,6 +161,31 @@ func (in DeviceStatsByDevice) DeepCopy() DeviceStatsByDevice { return *out } +// DeepCopyInto is an autogenerated deepcopy function, copying the receiver, writing into out. in must be non-nil. +func (in *EgressRule) DeepCopyInto(out *EgressRule) { + *out = *in + if in.Ports != nil { + in, out := &in.Ports, &out.Ports + *out = make([]int32, len(*in)) + copy(*out, *in) + } + if in.To != nil { + in, out := &in.To, &out.To + *out = make([]string, len(*in)) + copy(*out, *in) + } +} + +// DeepCopy is an autogenerated deepcopy function, copying the receiver, creating a new EgressRule. +func (in *EgressRule) DeepCopy() *EgressRule { + if in == nil { + return nil + } + out := new(EgressRule) + in.DeepCopyInto(out) + return out +} + // DeepCopyInto is an autogenerated deepcopy function, copying the receiver, writing into out. in must be non-nil. func (in *EgressRuleSNAT) DeepCopyInto(out *EgressRuleSNAT) { *out = *in @@ -633,6 +658,11 @@ func (in *FirewallSpec) DeepCopyInto(out *FirewallSpec) { (*in)[i].DeepCopyInto(&(*out)[i]) } } + if in.InitialRuleSet != nil { + in, out := &in.InitialRuleSet, &out.InitialRuleSet + *out = new(InitialRuleSet) + (*in).DeepCopyInto(*out) + } if in.DNSPort != nil { in, out := &in.DNSPort, &out.DNSPort *out = new(uint) @@ -780,6 +810,60 @@ func (in IDSStatsByDevice) DeepCopy() IDSStatsByDevice { return *out } +// DeepCopyInto is an autogenerated deepcopy function, copying the receiver, writing into out. in must be non-nil. +func (in *IngressRule) DeepCopyInto(out *IngressRule) { + *out = *in + if in.Ports != nil { + in, out := &in.Ports, &out.Ports + *out = make([]int32, len(*in)) + copy(*out, *in) + } + if in.From != nil { + in, out := &in.From, &out.From + *out = make([]string, len(*in)) + copy(*out, *in) + } +} + +// DeepCopy is an autogenerated deepcopy function, copying the receiver, creating a new IngressRule. +func (in *IngressRule) DeepCopy() *IngressRule { + if in == nil { + return nil + } + out := new(IngressRule) + in.DeepCopyInto(out) + return out +} + +// DeepCopyInto is an autogenerated deepcopy function, copying the receiver, writing into out. in must be non-nil. +func (in *InitialRuleSet) DeepCopyInto(out *InitialRuleSet) { + *out = *in + if in.Egress != nil { + in, out := &in.Egress, &out.Egress + *out = make([]EgressRule, len(*in)) + for i := range *in { + (*in)[i].DeepCopyInto(&(*out)[i]) + } + } + if in.Ingress != nil { + in, out := &in.Ingress, &out.Ingress + *out = make([]IngressRule, len(*in)) + for i := range *in { + (*in)[i].DeepCopyInto(&(*out)[i]) + } + } +} + +// DeepCopy is an autogenerated deepcopy function, copying the receiver, creating a new InitialRuleSet. +func (in *InitialRuleSet) DeepCopy() *InitialRuleSet { + if in == nil { + return nil + } + out := new(InitialRuleSet) + in.DeepCopyInto(out) + return out +} + // DeepCopyInto is an autogenerated deepcopy function, copying the receiver, writing into out. in must be non-nil. func (in *InterfaceStat) DeepCopyInto(out *InterfaceStat) { *out = *in diff --git a/config/crds/firewall.metal-stack.io_firewalldeployments.yaml b/config/crds/firewall.metal-stack.io_firewalldeployments.yaml index 3100ea4..70fa79b 100644 --- a/config/crds/firewall.metal-stack.io_firewalldeployments.yaml +++ b/config/crds/firewall.metal-stack.io_firewalldeployments.yaml @@ -180,6 +180,75 @@ spec: Image is the os image of the firewall. An update on this field requires the recreation of the physical firewall and can therefore lead to traffic interruption for the cluster. type: string + initialRuleSet: + description: InitialRuleSet is the initial firewall ruleset + applied before the firewall-controller starts running. + properties: + egress: + description: Egress rules to be deployed initially on + the firewall. + items: + properties: + comment: + description: Comment provides a human readable description + of this rule. + type: string + ports: + description: Ports contains all affected network + ports. + items: + format: int32 + type: integer + type: array + protocol: + description: Protocol constraints the protocol this + rule applies to. + type: string + to: + description: To target addresses this rule applies + to. May contain IPs or dns names. + items: + type: string + type: array + required: + - ports + - protocol + - to + type: object + type: array + ingress: + description: Ingress rules to be deployed initially on + the firewall. + items: + properties: + comment: + description: Comment provides a human readable description + of this rule. + type: string + from: + description: From source addresses this rule applies + to. May contain IPs or dns names. + items: + type: string + type: array + ports: + description: Ports contains all affected network + ports. + items: + format: int32 + type: integer + type: array + protocol: + description: Protocol constraints the protocol this + rule applies to. + type: string + required: + - from + - ports + - protocol + type: object + type: array + type: object internalPrefixes: description: |- InternalPrefixes specify prefixes which are considered local to the partition or all regions. This is used for the traffic counters. diff --git a/config/crds/firewall.metal-stack.io_firewalls.yaml b/config/crds/firewall.metal-stack.io_firewalls.yaml index c82118b..0d32283 100644 --- a/config/crds/firewall.metal-stack.io_firewalls.yaml +++ b/config/crds/firewall.metal-stack.io_firewalls.yaml @@ -135,6 +135,71 @@ spec: Image is the os image of the firewall. An update on this field requires the recreation of the physical firewall and can therefore lead to traffic interruption for the cluster. type: string + initialRuleSet: + description: InitialRuleSet is the initial firewall ruleset applied + before the firewall-controller starts running. + properties: + egress: + description: Egress rules to be deployed initially on the firewall. + items: + properties: + comment: + description: Comment provides a human readable description + of this rule. + type: string + ports: + description: Ports contains all affected network ports. + items: + format: int32 + type: integer + type: array + protocol: + description: Protocol constraints the protocol this rule + applies to. + type: string + to: + description: To target addresses this rule applies to. May + contain IPs or dns names. + items: + type: string + type: array + required: + - ports + - protocol + - to + type: object + type: array + ingress: + description: Ingress rules to be deployed initially on the firewall. + items: + properties: + comment: + description: Comment provides a human readable description + of this rule. + type: string + from: + description: From source addresses this rule applies to. + May contain IPs or dns names. + items: + type: string + type: array + ports: + description: Ports contains all affected network ports. + items: + format: int32 + type: integer + type: array + protocol: + description: Protocol constraints the protocol this rule + applies to. + type: string + required: + - from + - ports + - protocol + type: object + type: array + type: object internalPrefixes: description: |- InternalPrefixes specify prefixes which are considered local to the partition or all regions. This is used for the traffic counters. diff --git a/config/crds/firewall.metal-stack.io_firewallsets.yaml b/config/crds/firewall.metal-stack.io_firewallsets.yaml index ae2878a..bbc7c44 100644 --- a/config/crds/firewall.metal-stack.io_firewallsets.yaml +++ b/config/crds/firewall.metal-stack.io_firewallsets.yaml @@ -172,6 +172,75 @@ spec: Image is the os image of the firewall. An update on this field requires the recreation of the physical firewall and can therefore lead to traffic interruption for the cluster. type: string + initialRuleSet: + description: InitialRuleSet is the initial firewall ruleset + applied before the firewall-controller starts running. + properties: + egress: + description: Egress rules to be deployed initially on + the firewall. + items: + properties: + comment: + description: Comment provides a human readable description + of this rule. + type: string + ports: + description: Ports contains all affected network + ports. + items: + format: int32 + type: integer + type: array + protocol: + description: Protocol constraints the protocol this + rule applies to. + type: string + to: + description: To target addresses this rule applies + to. May contain IPs or dns names. + items: + type: string + type: array + required: + - ports + - protocol + - to + type: object + type: array + ingress: + description: Ingress rules to be deployed initially on + the firewall. + items: + properties: + comment: + description: Comment provides a human readable description + of this rule. + type: string + from: + description: From source addresses this rule applies + to. May contain IPs or dns names. + items: + type: string + type: array + ports: + description: Ports contains all affected network + ports. + items: + format: int32 + type: integer + type: array + protocol: + description: Protocol constraints the protocol this + rule applies to. + type: string + required: + - from + - ports + - protocol + type: object + type: array + type: object internalPrefixes: description: |- InternalPrefixes specify prefixes which are considered local to the partition or all regions. This is used for the traffic counters. From d3362457f60bda4031635dfa782a61440d9f1a58 Mon Sep 17 00:00:00 2001 From: Valentin Knabel Date: Thu, 21 Nov 2024 16:13:52 +0100 Subject: [PATCH 03/17] fix(lint): false positive recvcheck Getting methods are non pointers, mutating ones are pointers. This is fine and expected. --- api/v2/types_utils.go | 1 + 1 file changed, 1 insertion(+) diff --git a/api/v2/types_utils.go b/api/v2/types_utils.go index 644be46..9d9dcc4 100644 --- a/api/v2/types_utils.go +++ b/api/v2/types_utils.go @@ -40,6 +40,7 @@ const ( ConditionUnknown ConditionStatus = "Unknown" ) +//nolint:recvcheck type Conditions []Condition // NewCondition creates a new condition. From b42f1195b81879865be518ea7dfa5e5f50addc73 Mon Sep 17 00:00:00 2001 From: Valentin Knabel Date: Thu, 21 Nov 2024 16:51:46 +0100 Subject: [PATCH 04/17] docs: promoted cidrs --- api/v2/types_firewall.go | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/api/v2/types_firewall.go b/api/v2/types_firewall.go index f0ffb2e..373107e 100644 --- a/api/v2/types_firewall.go +++ b/api/v2/types_firewall.go @@ -150,7 +150,7 @@ type EgressRule struct { Ports []int32 `json:"ports"` // Protocol constraints the protocol this rule applies to. Protocol NetworkProtocol `json:"protocol"` - // To target addresses this rule applies to. May contain IPs or dns names. + // To source address cidrs this rule applies to. To []string `json:"to"` } @@ -161,7 +161,7 @@ type IngressRule struct { Ports []int32 `json:"ports"` // Protocol constraints the protocol this rule applies to. Protocol NetworkProtocol `json:"protocol"` - // From source addresses this rule applies to. May contain IPs or dns names. + // From source address cidrs this rule applies to. From []string `json:"from"` } From 6df67bb6515954bd072f8aa7f24f8038bd3e5096 Mon Sep 17 00:00:00 2001 From: Valentin Knabel Date: Fri, 17 Jan 2025 10:38:56 +0100 Subject: [PATCH 05/17] feat: initial draft for multi namespaces --- controllers/deployment/controller.go | 12 ++++++++---- controllers/deployment/infrastructure_status.go | 5 +++-- controllers/deployment/reconcile.go | 2 +- controllers/firewall/controller.go | 12 ++++++++---- controllers/generic_controller.go | 2 +- controllers/monitor/controller.go | 12 ++++++++---- controllers/monitor/reconcile.go | 4 ++-- controllers/set/controller.go | 12 ++++++++---- controllers/update/controller.go | 12 ++++++++---- 9 files changed, 47 insertions(+), 26 deletions(-) diff --git a/controllers/deployment/controller.go b/controllers/deployment/controller.go index 6366b87..33f1c1d 100644 --- a/controllers/deployment/controller.go +++ b/controllers/deployment/controller.go @@ -31,7 +31,7 @@ func SetupWithManager(log logr.Logger, recorder record.EventRecorder, mgr ctrl.M lastSetCreation: map[string]time.Time{}, }) - return ctrl.NewControllerManagedBy(mgr). + controller := ctrl.NewControllerManagedBy(mgr). For( &v2.FirewallDeployment{}, builder.WithPredicates( @@ -57,9 +57,13 @@ func SetupWithManager(log logr.Logger, recorder record.EventRecorder, mgr ctrl.M ), ), ), - ). - WithEventFilter(predicate.NewPredicateFuncs(controllers.SkipOtherNamespace(c.GetSeedNamespace()))). - Complete(g) + ) + + if c.GetSeedNamespace() != "" { + controller = controller.WithEventFilter(predicate.NewPredicateFuncs(controllers.SkipOtherNamespace(c.GetSeedNamespace()))) + } + + return controller.Complete(g) } func SetupWebhookWithManager(log logr.Logger, mgr ctrl.Manager, c *config.ControllerConfig) error { diff --git a/controllers/deployment/infrastructure_status.go b/controllers/deployment/infrastructure_status.go index 1caa62b..93a9dcd 100644 --- a/controllers/deployment/infrastructure_status.go +++ b/controllers/deployment/infrastructure_status.go @@ -27,7 +27,7 @@ func (c *controller) updateInfrastructureStatus(r *controllers.Ctx[*v2.FirewallD }) err := c.c.GetSeedClient().Get(r.Ctx, client.ObjectKey{ - Namespace: c.c.GetSeedNamespace(), + Namespace: r.Target.Namespace, Name: infrastructureName, }, infraObj) if err != nil { @@ -129,7 +129,7 @@ func (c *controller) updateInfrastructureStatus(r *controllers.Ctx[*v2.FirewallD }) err = c.c.GetSeedClient().Get(r.Ctx, client.ObjectKey{ - Namespace: c.c.GetSeedNamespace(), + Namespace: r.Target.Namespace, Name: "acl", }, aclObj) if err != nil { @@ -150,6 +150,7 @@ func (c *controller) updateInfrastructureStatus(r *controllers.Ctx[*v2.FirewallD } func extractInfrastructureNameFromSeedNamespace(namespace string) (string, bool) { + // TODO: is this safe to not skip in the future? if !strings.HasPrefix(namespace, "shoot--") { return "", false } diff --git a/controllers/deployment/reconcile.go b/controllers/deployment/reconcile.go index 2b5625f..20f5056 100644 --- a/controllers/deployment/reconcile.go +++ b/controllers/deployment/reconcile.go @@ -83,7 +83,7 @@ func (c *controller) Reconcile(r *controllers.Ctx[*v2.FirewallDeployment]) error r.Log.Info("swapped latest set to shortest distance", "distance", v2.FirewallShortestDistance) } - infrastructureName, ok := extractInfrastructureNameFromSeedNamespace(c.c.GetSeedNamespace()) + infrastructureName, ok := extractInfrastructureNameFromSeedNamespace(r.Target.Namespace) if ok { var ownedFirewalls []*v2.Firewall for _, set := range ownedSets { diff --git a/controllers/firewall/controller.go b/controllers/firewall/controller.go index 6c18b9a..3ab23fd 100644 --- a/controllers/firewall/controller.go +++ b/controllers/firewall/controller.go @@ -88,7 +88,7 @@ func SetupWithManager(log logr.Logger, recorder record.EventRecorder, mgr ctrl.M }), }) - return ctrl.NewControllerManagedBy(mgr). + controller := ctrl.NewControllerManagedBy(mgr). For( &v2.Firewall{}, builder.WithPredicates( @@ -99,9 +99,13 @@ func SetupWithManager(log logr.Logger, recorder record.EventRecorder, mgr ctrl.M ), ). // don't think about owning the firewall monitor here, it's in the shoot cluster, we cannot watch two clusters with controller-runtime - Named("Firewall"). - WithEventFilter(predicate.NewPredicateFuncs(controllers.SkipOtherNamespace(c.GetSeedNamespace()))). - Complete(g) + Named("Firewall") + + if c.GetSeedNamespace() != "" { + controller = controller.WithEventFilter(predicate.NewPredicateFuncs(controllers.SkipOtherNamespace(c.GetSeedNamespace()))) + } + + return controller.Complete(g) } func SetupWebhookWithManager(log logr.Logger, mgr ctrl.Manager, c *config.ControllerConfig) error { diff --git a/controllers/generic_controller.go b/controllers/generic_controller.go index 0d60c16..2afbc3c 100644 --- a/controllers/generic_controller.go +++ b/controllers/generic_controller.go @@ -74,7 +74,7 @@ func (g *GenericController[O]) logger(req ctrl.Request) logr.Logger { } func (g GenericController[O]) Reconcile(ctx context.Context, req ctrl.Request) (ctrl.Result, error) { - if req.Namespace != g.namespace { // should already be filtered out through predicate, but we will check anyway + if g.namespace != "" && req.Namespace != g.namespace { // should already be filtered out through predicate, but we will check anyway return ctrl.Result{}, nil } diff --git a/controllers/monitor/controller.go b/controllers/monitor/controller.go index cc5d5ee..c84c2c4 100644 --- a/controllers/monitor/controller.go +++ b/controllers/monitor/controller.go @@ -22,7 +22,7 @@ func SetupWithManager(log logr.Logger, mgr ctrl.Manager, c *config.ControllerCon c: c, }).WithoutStatus() - return ctrl.NewControllerManagedBy(mgr). + controller := ctrl.NewControllerManagedBy(mgr). For(&v2.FirewallMonitor{}, builder.WithPredicates( predicate.Not( @@ -30,9 +30,13 @@ func SetupWithManager(log logr.Logger, mgr ctrl.Manager, c *config.ControllerCon ), ), ). - Named("FirewallMonitor"). - WithEventFilter(predicate.NewPredicateFuncs(controllers.SkipOtherNamespace(c.GetShootNamespace()))). - Complete(g) + Named("FirewallMonitor") + + if c.GetSeedNamespace() != "" { + controller = controller.WithEventFilter(predicate.NewPredicateFuncs(controllers.SkipOtherNamespace(c.GetSeedNamespace()))) + } + + return controller.Complete(g) } func (c *controller) New() *v2.FirewallMonitor { diff --git a/controllers/monitor/reconcile.go b/controllers/monitor/reconcile.go index 4bcf1cb..55b2e50 100644 --- a/controllers/monitor/reconcile.go +++ b/controllers/monitor/reconcile.go @@ -37,7 +37,7 @@ func (c *controller) updateFirewallStatus(r *controllers.Ctx[*v2.FirewallMonitor fw := &v2.Firewall{ ObjectMeta: metav1.ObjectMeta{ Name: r.Target.Name, - Namespace: c.c.GetSeedNamespace(), + Namespace: r.Target.Namespace, }, } err := c.c.GetSeedClient().Get(r.Ctx, client.ObjectKeyFromObject(fw), fw) @@ -72,7 +72,7 @@ func (c *controller) rollSetAnnotation(r *controllers.Ctx[*v2.FirewallMonitor]) fw := &v2.Firewall{ ObjectMeta: metav1.ObjectMeta{ Name: r.Target.Name, - Namespace: c.c.GetSeedNamespace(), + Namespace: r.Target.Namespace, }, } diff --git a/controllers/set/controller.go b/controllers/set/controller.go index 06909c5..426a2ee 100644 --- a/controllers/set/controller.go +++ b/controllers/set/controller.go @@ -26,7 +26,7 @@ func SetupWithManager(log logr.Logger, recorder record.EventRecorder, mgr ctrl.M c: c, }) - return ctrl.NewControllerManagedBy(mgr). + controller := ctrl.NewControllerManagedBy(mgr). For( &v2.FirewallSet{}, builder.WithPredicates( @@ -47,9 +47,13 @@ func SetupWithManager(log logr.Logger, recorder record.EventRecorder, mgr ctrl.M ), ), ), - ). - WithEventFilter(predicate.NewPredicateFuncs(controllers.SkipOtherNamespace(c.GetSeedNamespace()))). - Complete(g) + ) + + if c.GetSeedNamespace() != "" { + controller = controller.WithEventFilter(predicate.NewPredicateFuncs(controllers.SkipOtherNamespace(c.GetSeedNamespace()))) + } + + return controller.Complete(g) } func SetupWebhookWithManager(log logr.Logger, mgr ctrl.Manager, c *config.ControllerConfig) error { diff --git a/controllers/update/controller.go b/controllers/update/controller.go index 991adde..3b4ba53 100644 --- a/controllers/update/controller.go +++ b/controllers/update/controller.go @@ -34,16 +34,20 @@ func SetupWithManager(log logr.Logger, recorder record.EventRecorder, mgr ctrl.M imageCache: newImageCache(c.GetMetal()), }).WithoutStatus() - return ctrl.NewControllerManagedBy(mgr). + controller := ctrl.NewControllerManagedBy(mgr). For( &v2.FirewallDeployment{}, builder.WithPredicates( v2.AnnotationAddedPredicate(v2.MaintenanceAnnotation), ), ). - Named("FirewallDeployment"). - WithEventFilter(predicate.NewPredicateFuncs(controllers.SkipOtherNamespace(c.GetSeedNamespace()))). - Complete(g) + Named("FirewallDeployment") + + if c.GetSeedNamespace() != "" { + controller = controller.WithEventFilter(predicate.NewPredicateFuncs(controllers.SkipOtherNamespace(c.GetSeedNamespace()))) + } + + return controller.Complete(g) } func (c *controller) New() *v2.FirewallDeployment { From 3ae3066db5cd85d98596fb32695ce1f1629b9ab3 Mon Sep 17 00:00:00 2001 From: Gerrit Date: Wed, 5 Feb 2025 15:38:17 +0100 Subject: [PATCH 06/17] Typo. --- controllers/firewall/reconcile.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/controllers/firewall/reconcile.go b/controllers/firewall/reconcile.go index 0692abc..d3ba472 100644 --- a/controllers/firewall/reconcile.go +++ b/controllers/firewall/reconcile.go @@ -46,7 +46,7 @@ func (c *controller) Reconcile(r *controllers.Ctx[*v2.Firewall]) error { return err } - // requeueing in order to continue checking progression + // requeuing in order to continue checking progression return controllers.RequeueAfter(10*time.Second, "firewall creation is progressing") case 1: f = fws[0] From aeeda2a46608710c8172e7b20bf738659952f297 Mon Sep 17 00:00:00 2001 From: Gerrit Date: Fri, 21 Feb 2025 13:23:31 +0100 Subject: [PATCH 07/17] Watch all namespaces. --- main.go | 12 ++++++++++-- 1 file changed, 10 insertions(+), 2 deletions(-) diff --git a/main.go b/main.go index f1b9f67..24370d0 100644 --- a/main.go +++ b/main.go @@ -110,7 +110,7 @@ func main() { log.Fatalf("unable to create metal client %v", err) } - seedMgr, err := ctrl.NewManager(ctrl.GetConfigOrDie(), ctrl.Options{ + mgrConfig := ctrl.Options{ Scheme: scheme, Metrics: server.Options{ BindAddress: metricsAddr, @@ -129,7 +129,15 @@ func main() { LeaderElection: enableLeaderElection, LeaderElectionID: "firewall-controller-manager-leader-election", GracefulShutdownTimeout: &gracefulShutdownTimeout, - }) + } + + if namespace == "" { + mgrConfig.Cache.DefaultNamespaces = map[string]cache.Config{ + cache.AllNamespaces: {}, + } + } + + seedMgr, err := ctrl.NewManager(ctrl.GetConfigOrDie(), mgrConfig) if err != nil { log.Fatalf("unable to setup firewall-controller-manager %v", err) } From 7d0a690ecb37dda4a940be4390c3733f71f9d86c Mon Sep 17 00:00:00 2001 From: Gerrit Date: Fri, 21 Feb 2025 13:26:56 +0100 Subject: [PATCH 08/17] Prevent nil pointer. --- api/v2/validation/firewalldeployment.go | 13 +++++++------ 1 file changed, 7 insertions(+), 6 deletions(-) diff --git a/api/v2/validation/firewalldeployment.go b/api/v2/validation/firewalldeployment.go index ab19882..eaa2215 100644 --- a/api/v2/validation/firewalldeployment.go +++ b/api/v2/validation/firewalldeployment.go @@ -49,14 +49,15 @@ func (*firewallDeploymentValidator) validateSpec(log logr.Logger, f *v2.Firewall }) if err != nil { allErrs = append(allErrs, field.Invalid(fldPath.Child("selector"), f.Selector, "")) - } - - if !selector.Empty() { - labels := labels.Set(f.Template.Labels) - if !selector.Matches(labels) { - allErrs = append(allErrs, field.Invalid(fldPath.Child("template", "metadata", "labels"), f.Template.Labels, "`selector` does not match template `labels`")) + } else { + if !selector.Empty() { + labels := labels.Set(f.Template.Labels) + if !selector.Matches(labels) { + allErrs = append(allErrs, field.Invalid(fldPath.Child("template", "metadata", "labels"), f.Template.Labels, "`selector` does not match template `labels`")) + } } } + } allErrs = append(allErrs, NewFirewallValidator(log).Instance().validateSpec(&f.Template.Spec, fldPath.Child("template").Child("spec"))...) From 7914af91beea27efd2248d776813fcab4c40432d Mon Sep 17 00:00:00 2001 From: Gerrit Date: Fri, 21 Feb 2025 14:56:06 +0100 Subject: [PATCH 09/17] Remove default from namespace flag. --- main.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/main.go b/main.go index 24370d0..e971e41 100644 --- a/main.go +++ b/main.go @@ -72,7 +72,7 @@ func main() { flag.BoolVar(&enableLeaderElection, "enable-leader-election", false, "Enable leader election for controller manager. "+ "Enabling this will ensure there is only one active controller manager") - flag.StringVar(&namespace, "namespace", "default", "the namespace this controller is running") + flag.StringVar(&namespace, "namespace", "", "the namespace this controller is running") flag.DurationVar(&reconcileInterval, "reconcile-interval", 10*time.Minute, "duration after which a resource is getting reconciled at minimum") flag.DurationVar(&firewallHealthTimeout, "firewall-health-timeout", 20*time.Minute, "duration after a created firewall not getting ready is considered dead") flag.DurationVar(&createTimeout, "create-timeout", 10*time.Minute, "duration after which a firewall in the creation phase will be recreated") From a5c3a2eaf22d4a8ce7ff3a0ff9f907f5a3723f26 Mon Sep 17 00:00:00 2001 From: Gerrit Date: Fri, 21 Feb 2025 15:00:15 +0100 Subject: [PATCH 10/17] Remove from validation. --- api/v2/config/controller.go | 3 --- 1 file changed, 3 deletions(-) diff --git a/api/v2/config/controller.go b/api/v2/config/controller.go index f12a185..ab5ca92 100644 --- a/api/v2/config/controller.go +++ b/api/v2/config/controller.go @@ -132,9 +132,6 @@ func (c *NewControllerConfig) validate() error { if c.SeedConfig == nil { return fmt.Errorf("seed config must be specified") } - if c.SeedNamespace == "" { - return fmt.Errorf("seed namespace must be specified") - } if c.SeedAPIServerURL == "" { return fmt.Errorf("seed api server url must be specified") } From 67f2b643447858f435b1e83a3864f07d3916ebb1 Mon Sep 17 00:00:00 2001 From: Gerrit Date: Fri, 21 Feb 2025 15:07:49 +0100 Subject: [PATCH 11/17] Try fix. --- main.go | 8 +++++++- 1 file changed, 7 insertions(+), 1 deletion(-) diff --git a/main.go b/main.go index e971e41..d98fa99 100644 --- a/main.go +++ b/main.go @@ -52,6 +52,7 @@ func main() { shootTokenSecret string shootTokenPath string sshKeySecret string + sshKeySecretNamespace string namespace string gracefulShutdownTimeout time.Duration reconcileInterval time.Duration @@ -88,10 +89,15 @@ func main() { flag.StringVar(&shootKubeconfigSecret, "shoot-kubeconfig-secret-name", "", "the secret name of the generic kubeconfig for shoot access") flag.StringVar(&shootTokenSecret, "shoot-token-secret-name", "", "the secret name of the token for shoot access") flag.StringVar(&sshKeySecret, "ssh-key-secret-name", "", "the secret name of the ssh key for machine access") + flag.StringVar(&sshKeySecretNamespace, "ssh-key-secret-namespace", "", "the secret name of the ssh key for machine access") flag.StringVar(&shootTokenPath, "shoot-token-path", "", "the path where to store the token file for shoot access") flag.Parse() + if sshKeySecretNamespace == "" { + sshKeySecretNamespace = namespace + } + slogHandler, err := controllers.NewLogger(logLevel) if err != nil { ctrl.Log.WithName("setup").Error(err, "unable to parse log level") @@ -255,7 +261,7 @@ func main() { ShootAPIServerURL: shootApiURL, ShootAccess: externalShootAccess, SSHKeySecretName: sshKeySecret, - SSHKeySecretNamespace: namespace, + SSHKeySecretNamespace: sshKeySecretNamespace, ShootAccessHelper: internalShootAccessHelper, Metal: mclient, ClusterTag: fmt.Sprintf("%s=%s", tag.ClusterID, clusterID), From be8d742f544a97b2f99505a73ad6bbeb6a6e8679 Mon Sep 17 00:00:00 2001 From: Gerrit Date: Fri, 21 Feb 2025 15:14:39 +0100 Subject: [PATCH 12/17] Fix test. --- .../crds/firewall.metal-stack.io_firewalldeployments.yaml | 8 ++++---- config/crds/firewall.metal-stack.io_firewalls.yaml | 7 +++---- config/crds/firewall.metal-stack.io_firewallsets.yaml | 8 ++++---- controllers/deployment/infrastructure_status_test.go | 6 ++++++ 4 files changed, 17 insertions(+), 12 deletions(-) diff --git a/config/crds/firewall.metal-stack.io_firewalldeployments.yaml b/config/crds/firewall.metal-stack.io_firewalldeployments.yaml index 70fa79b..57cf768 100644 --- a/config/crds/firewall.metal-stack.io_firewalldeployments.yaml +++ b/config/crds/firewall.metal-stack.io_firewalldeployments.yaml @@ -205,8 +205,8 @@ spec: rule applies to. type: string to: - description: To target addresses this rule applies - to. May contain IPs or dns names. + description: To source address cidrs this rule applies + to. items: type: string type: array @@ -226,8 +226,8 @@ spec: of this rule. type: string from: - description: From source addresses this rule applies - to. May contain IPs or dns names. + description: From source address cidrs this rule + applies to. items: type: string type: array diff --git a/config/crds/firewall.metal-stack.io_firewalls.yaml b/config/crds/firewall.metal-stack.io_firewalls.yaml index 0d32283..8bd9940 100644 --- a/config/crds/firewall.metal-stack.io_firewalls.yaml +++ b/config/crds/firewall.metal-stack.io_firewalls.yaml @@ -158,8 +158,7 @@ spec: applies to. type: string to: - description: To target addresses this rule applies to. May - contain IPs or dns names. + description: To source address cidrs this rule applies to. items: type: string type: array @@ -178,8 +177,8 @@ spec: of this rule. type: string from: - description: From source addresses this rule applies to. - May contain IPs or dns names. + description: From source address cidrs this rule applies + to. items: type: string type: array diff --git a/config/crds/firewall.metal-stack.io_firewallsets.yaml b/config/crds/firewall.metal-stack.io_firewallsets.yaml index bbc7c44..1260fb7 100644 --- a/config/crds/firewall.metal-stack.io_firewallsets.yaml +++ b/config/crds/firewall.metal-stack.io_firewallsets.yaml @@ -197,8 +197,8 @@ spec: rule applies to. type: string to: - description: To target addresses this rule applies - to. May contain IPs or dns names. + description: To source address cidrs this rule applies + to. items: type: string type: array @@ -218,8 +218,8 @@ spec: of this rule. type: string from: - description: From source addresses this rule applies - to. May contain IPs or dns names. + description: From source address cidrs this rule + applies to. items: type: string type: array diff --git a/controllers/deployment/infrastructure_status_test.go b/controllers/deployment/infrastructure_status_test.go index 96fa1cb..53983b4 100644 --- a/controllers/deployment/infrastructure_status_test.go +++ b/controllers/deployment/infrastructure_status_test.go @@ -12,6 +12,7 @@ import ( "github.com/metal-stack/metal-lib/pkg/pointer" "github.com/metal-stack/metal-lib/pkg/testcommon" "github.com/stretchr/testify/require" + v1 "k8s.io/apimachinery/pkg/apis/meta/v1" "k8s.io/apimachinery/pkg/apis/meta/v1/unstructured" "k8s.io/apimachinery/pkg/runtime" "sigs.k8s.io/controller-runtime/pkg/client" @@ -454,6 +455,11 @@ func Test_controller_updateInfrastructureStatus(t *testing.T) { } err = ctrl.updateInfrastructureStatus(&controllers.Ctx[*v2.FirewallDeployment]{ + Target: &v2.FirewallDeployment{ + ObjectMeta: v1.ObjectMeta{ + Namespace: testNamespace, + }, + }, Ctx: ctx, Log: log, }, "mycluster1", tt.ownedFirewalls) From 8360b62b6702725a84c9e4459bd94728ecc0db61 Mon Sep 17 00:00:00 2001 From: Gerrit Date: Fri, 21 Feb 2025 15:23:07 +0100 Subject: [PATCH 13/17] Fix. --- main.go | 10 ++++------ 1 file changed, 4 insertions(+), 6 deletions(-) diff --git a/main.go b/main.go index d98fa99..c1f13fa 100644 --- a/main.go +++ b/main.go @@ -127,9 +127,6 @@ func main() { }), Cache: cache.Options{ SyncPeriod: &reconcileInterval, - DefaultNamespaces: map[string]cache.Config{ - namespace: {}, - }, }, HealthProbeBindAddress: healthAddr, LeaderElection: enableLeaderElection, @@ -137,9 +134,10 @@ func main() { GracefulShutdownTimeout: &gracefulShutdownTimeout, } - if namespace == "" { + if namespace != "" { + l.Info("running in dedicated namespace only", "namespace", namespace) mgrConfig.Cache.DefaultNamespaces = map[string]cache.Config{ - cache.AllNamespaces: {}, + namespace: {}, } } @@ -210,7 +208,7 @@ func main() { // secret for this controller and expose the access secrets through the firewall // status resource, which can be read by the firewall-controller // - the firewall-controller can then create a client from these secrets but - // it has to contiuously update the token file because the token will expire + // it has to continuously update the token file because the token will expire // - we can re-use the same approach for this controller as well and do not have // to do any additional mounts for the deployment of the controller // From 88a5bfb669e189f71881a365256f4020670580e5 Mon Sep 17 00:00:00 2001 From: Gerrit Date: Fri, 21 Feb 2025 15:45:12 +0100 Subject: [PATCH 14/17] Fix. --- controllers/monitor/reconcile.go | 20 +++++++++++++------- 1 file changed, 13 insertions(+), 7 deletions(-) diff --git a/controllers/monitor/reconcile.go b/controllers/monitor/reconcile.go index 55b2e50..0c5a9b8 100644 --- a/controllers/monitor/reconcile.go +++ b/controllers/monitor/reconcile.go @@ -3,6 +3,7 @@ package monitor import ( "context" "fmt" + "slices" "strconv" "time" @@ -34,17 +35,22 @@ func (c *controller) Reconcile(r *controllers.Ctx[*v2.FirewallMonitor]) error { } func (c *controller) updateFirewallStatus(r *controllers.Ctx[*v2.FirewallMonitor]) (*v2.Firewall, error) { - fw := &v2.Firewall{ - ObjectMeta: metav1.ObjectMeta{ - Name: r.Target.Name, - Namespace: r.Target.Namespace, - }, - } - err := c.c.GetSeedClient().Get(r.Ctx, client.ObjectKeyFromObject(fw), fw) + fws := &v2.FirewallList{} + + err := c.c.GetSeedClient().List(r.Ctx, fws) if err != nil { + return nil, fmt.Errorf("unable to list firewalls: %w", err) + } + + idx := slices.IndexFunc(fws.Items, func(fw v2.Firewall) bool { + return fw.Name == r.Target.Name // TODO: not sure if this is safe to do? + }) + if idx < 0 { return nil, fmt.Errorf("associated firewall of monitor not found: %w", err) } + fw := &fws.Items[idx] + old := fw.DeepCopy() firewall.SetFirewallStatusFromMonitor(fw, r.Target) From 866391729918041078fa94dc6abea1a683d29634 Mon Sep 17 00:00:00 2001 From: Gerrit Date: Fri, 21 Feb 2025 15:56:12 +0100 Subject: [PATCH 15/17] Inherit no controller connection annotation. --- controllers/deployment/reconcile.go | 6 ++++++ controllers/set/reconcile.go | 6 ++++++ 2 files changed, 12 insertions(+) diff --git a/controllers/deployment/reconcile.go b/controllers/deployment/reconcile.go index 20f5056..ec1faef 100644 --- a/controllers/deployment/reconcile.go +++ b/controllers/deployment/reconcile.go @@ -166,6 +166,12 @@ func (c *controller) createFirewallSet(r *controllers.Ctx[*v2.FirewallDeployment }, } + if r.Target.Annotations != nil { + if val, ok := r.Target.Annotations[v2.FirewallNoControllerConnectionAnnotation]; ok { + set.Annotations[v2.FirewallNoControllerConnectionAnnotation] = val + } + } + err = c.c.GetSeedClient().Create(r.Ctx, set, &client.CreateOptions{}) if err != nil { cond := v2.NewCondition(v2.FirewallDeplomentProgressing, v2.ConditionFalse, "FirewallSetCreateError", fmt.Sprintf("Error creating firewall set: %s.", err)) diff --git a/controllers/set/reconcile.go b/controllers/set/reconcile.go index 6591ea3..2d30189 100644 --- a/controllers/set/reconcile.go +++ b/controllers/set/reconcile.go @@ -157,6 +157,12 @@ func (c *controller) createFirewall(r *controllers.Ctx[*v2.FirewallSet]) (*v2.Fi meta.Annotations[v2.FirewallNoControllerConnectionAnnotation] = "true" } + if r.Target.Annotations != nil { + if val, ok := r.Target.Annotations[v2.FirewallNoControllerConnectionAnnotation]; ok { + meta.Annotations[v2.FirewallNoControllerConnectionAnnotation] = val + } + } + fw := &v2.Firewall{ ObjectMeta: *meta, Spec: r.Target.Spec.Template.Spec, From 700fb2eb94e5b2e1b812f2a1837704d637dbdb21 Mon Sep 17 00:00:00 2001 From: Valentin Knabel Date: Fri, 7 Mar 2025 10:36:30 +0100 Subject: [PATCH 16/17] fix: nil pointer --- controllers/set/reconcile.go | 3 +++ 1 file changed, 3 insertions(+) diff --git a/controllers/set/reconcile.go b/controllers/set/reconcile.go index 2d30189..c48d958 100644 --- a/controllers/set/reconcile.go +++ b/controllers/set/reconcile.go @@ -159,6 +159,9 @@ func (c *controller) createFirewall(r *controllers.Ctx[*v2.FirewallSet]) (*v2.Fi if r.Target.Annotations != nil { if val, ok := r.Target.Annotations[v2.FirewallNoControllerConnectionAnnotation]; ok { + if meta.Annotations == nil { + meta.Annotations = map[string]string{} + } meta.Annotations[v2.FirewallNoControllerConnectionAnnotation] = val } } From a1fedc70ea13feb81f6150522b081a0febb5865d Mon Sep 17 00:00:00 2001 From: Valentin Knabel Date: Fri, 7 Mar 2025 11:09:52 +0100 Subject: [PATCH 17/17] feat: roll fwset --- controllers/deployment/reconcile.go | 7 +++++++ 1 file changed, 7 insertions(+) diff --git a/controllers/deployment/reconcile.go b/controllers/deployment/reconcile.go index ec1faef..455d088 100644 --- a/controllers/deployment/reconcile.go +++ b/controllers/deployment/reconcile.go @@ -5,6 +5,7 @@ import ( "strconv" "time" + "github.com/google/go-cmp/cmp" "github.com/google/uuid" v2 "github.com/metal-stack/firewall-controller-manager/api/v2" "github.com/metal-stack/firewall-controller-manager/controllers" @@ -248,5 +249,11 @@ func (c *controller) isNewSetRequired(r *controllers.Ctx[*v2.FirewallDeployment] return true } + // TODO: improve and write tests + if !cmp.Equal(oldS.InitialRuleSet, newS.InitialRuleSet) { + r.Log.Info("firewall initial rule set have changed") + return true + } + return false }