flightcontroller: ensure more security group rules #601

databus23 · 2021-08-06T15:55:45Z

This PR refactors the EnsureKubernikusRuleInSecurityGroup to support reconciling more then one rule.
It now also creates the rule lazily only if no other broader rules are already covering the wanted rules.

In addition to the podCIDR rule we had, it now also added egress rules for ntp and the cluster apiserver.

This PR create the following rules lazily if there isn't already a rule allowing the traffic:

egress:

all, to same security group (pod-to-pod)
udp port 53 (dns)
udp port 123 (ntp)
port 443 keppel.$region.cloud.sap, objectstore-3.$region.cloud.sap, keppel.global.global.cloud.sap, objectstore-3.eu-de-1.cloud.sap (for pulling kube-proxy,kubelet, wormhole, flannel images)

ingress:

all, from same security group (pod-to-pod)
port 30000 - 32767, from private network (load balancer to members)

TODO:

Modify our e2e tests to use a fresh security group without any rules to make sure the e2e tests passes with our enforced rules only.
Also we should probaply expand our network tests to include host <-> pod tests and also test the loadbalancer datapath. --> Switch to kube-detective for thoroughly testing all combinitions
Add metric for failing reconcilitions
remove rule for objectstore.eu-de-1.cloud.sap

databus23 · 2021-08-20T08:49:04Z

pkg/client/openstack/kluster/client.go

+func createLoadbalancerSecurityGroup(kluster *v1.Kluster, client *gophercloud.ServiceClient) (*securitygroups.SecGroup, error) {
+	sg, err := securitygroups.Create(client, securitygroups.CreateOpts{
+		Name:        fmt.Sprintf(LoadBalancerSecurityGroupName, kluster.Spec.Name),
+		Description: fmt.Sprintf(`Kubernikus: SecurityGroup containing LoadBalancer Self-IPs for cluster "%s"`, kluster.Spec.Name),


Suggested change

Description: fmt.Sprintf(`Kubernikus: SecurityGroup containing LoadBalancer Self-IPs for cluster "%s"`, kluster.Spec.Name),

Description: fmt.Sprintf(`Kubernikus: SecurityGroup containing LoadBalancer VIP and Self-IPs for cluster "%s"`, kluster.Spec.Name),

databus23 · 2021-08-20T08:52:27Z

pkg/client/openstack/kluster/client.go

+		},
+	}
+
+	if lbGroup != nil && lbGroup.ID != "" {


Is this defensive programming intentional? Because it seems like we ensure the lbGroup is not nil on top (e.g. we error out if it is). So I would be fine in simplifying this and moving it in the "static" wantedRules above

This can be nil, eg. when there is not enough quota in the project for creating an additional security group. This ensures the other rules even if that's the case.

I see, good point but it looks to me like at the moment if creating the security group fails there is an early return (with en error) and no other security groups are enforced anyways.

Where is it returning? I think I'm ignoring the error while getting the security group.

Here: https://github.com/sapcc/kubernikus/pull/601/files#diff-b5e03987e634a7e5e1033109b07cba3c54a48db540f4694445c66794899fdabfR255

Isn't this only exiting EnsureLoadbalancerSecurityGroup()? To me it looks like EnsureKubernikusRulesInSecurityGroup() runs anyway.

ah yes, sorry I misread the code. Nvm

databus23 · 2021-08-20T08:55:39Z

pkg/client/openstack/kluster/client.go

-		}
+	allLbPages, err := loadbalancers.List(c.NetworkClient, loadbalancers.ListOpts{}).AllPages()
+	if err != nil {
+		return false, err


Suggested change

return false, err

return false, fmt.Errorf("Failed to list load balancers: %w", err)

databus23 · 2021-08-20T08:56:36Z

pkg/client/openstack/kluster/client.go

-		}
+	allLoadbalancers, err := loadbalancers.ExtractLoadBalancers(allLbPages)
+	if err != nil {
+		return false, err


Suggested change

return false, err

return false, fmt.Errorf("Failed to extract load balancers: %w", err)

databus23 · 2021-08-20T08:57:12Z

pkg/client/openstack/kluster/client.go

-		}
+	allPortPages, err := ports.List(c.NetworkClient, portListOpts{ProjectID: kluster.Labels["account"], SecurityGroups: lbGroup.ID}).AllPages()
+	if err != nil {
+		return false, err


Suggested change

return false, err

return false, fmt.Errorf("Failed to list ports: %w", err)

databus23 · 2021-08-20T08:57:39Z

pkg/client/openstack/kluster/client.go

-		}
+	allPorts, err := ports.ExtractPorts(allPortPages)
+	if err != nil {
+		return false, err


Suggested change

return false, err

return false, fmt.Errorf("Failed to extract ports: %w", err)

databus23 · 2021-08-20T09:00:11Z

pkg/client/openstack/kluster/client.go

+			if !found {
+				_, err := ports.Update(c.NetworkClient, lb.VipPortID, ports.UpdateOpts{SecurityGroups: &sg}).Extract()
+				if err != nil {
+					return false, err


Suggested change

return false, err

return false, fmt.Errorf("Failed to add security group %s to port %s: %w", err, lbGroup.ID, lb.VipPortID, err)

databus23 · 2021-08-20T09:11:16Z

pkg/client/openstack/kluster/client.go

+	sg := []string{lbGroup.ID}
+	for _, lb := range allLoadbalancers {
+		if strings.HasPrefix(lb.Name, fmt.Sprintf("kube_service_%s", kluster.Spec.Name)) {
+			found := false


I suggest to replace the inner loop with

if !portsOfSecurityGroup.Has(lb.VipPortID) { //update port }

And to this before the whole loop construct:

portsOfSecurityGroup := sets.NewString() for _, p := range allPorts { portsOfSecurityGroup.Insert(p.ID) }

using https://pkg.go.dev/k8s.io/apimachinery/pkg/util/sets

databus23 · 2021-08-20T09:27:11Z

pkg/client/openstack/kluster/client.go

-			continue
+	if lbGroup == nil {
+		lbGroup, err = createLoadbalancerSecurityGroup(kluster, c.NetworkClient)
+		if err != nil || lbGroup == nil {


not sure the || lbGroup == nil is required. Looking at createLoadbalancerSecurityGroup it returns an error when the create call fails. I don't see how err and lbGroup can be nil at the same time.

databus23 · 2021-08-20T09:56:24Z

pkg/client/openstack/kluster/client.go

+	return q.String(), err
+}
+
+func getLoadbalancerSecurityGroup(kluster *v1.Kluster, client *gophercloud.ServiceClient) (*securitygroups.SecGroup, error) {


As we are getting two different security groups in this controller maybe its worthwhile to have a single getSecurityGroupByName(name string, client *gophercloud.ServiceClient) function instead if this special purpose function?

Yeah, makes sense.

databus23

LGTM now. Would be awesome now also change the e2e test to actually test that all this works:

use an empty security group for e2e cluster, cleanup during teardown
add loadbalancer happy path url check to test loadbalancer <-> nodes connectivity is working.

databus23

Cool. One small nitpick but looks good otherwise

databus23 · 2021-08-24T06:32:16Z

test/e2e/setup_test.go

@@ -37,6 +39,12 @@ func (s *SetupTests) Run(t *testing.T) {
 	require.True(t, running, "The Kluster must be Running")
 }

+func (s *SetupTests) CreateSecurityGroup(t *testing.T) {
+	sg, err := securitygroups.Create(s.OpenStack.Network, securitygroups.CreateOpts{Name: "kubernikus"}).Extract()


I would create a dedicated security group that is named after the current soak test cluster.

stale · 2022-01-09T00:16:17Z

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

sandzwerg · 2022-06-29T13:02:09Z

I haven't checked the code but only your comment but IIRC for UDP you want a port 123 Ingress rule for NTP and port 53 Ingress for DNS as well (because it's stateless so the firewall can't track the state). Also for DNS you might want a TCP port 53 Egress rule as well as DNS now can use both.

databus23 · 2022-06-29T13:44:14Z

I haven't checked the code but only your comment but IIRC for UDP you want a port 123 Ingress rule for NTP and port 53 Ingress for DNS as well (because it's stateless so the firewall can't track the state).

Are you sure that's the case? Because allowing udp egress 53 and 123 definitlty fixes dns issues and time sync issues. So it seems to me nsx-t is doing some "connection tracking" for udp packets as well (like iptables does, e.g. state ESTABLISHED,RELATED)

Also for DNS you might want a TCP port 53 Egress rule as well as DNS now can use both.

That's a valid point

sandzwerg · 2022-06-29T13:55:54Z

At least for NTP a customer had this issue today (not in a kubernikus environment) so I assume that the ingress rules are needed as well, but I haven't tested it myself. Here is the discussion: https://convergedcloud.slack.com/archives/C374AQJ3W/p1656505078940829

* add metrics for security group reconciliation errors * add kluster event for securitu group reconcilicaiton errors * e2e: * create empty security group for cluster * run kube-detective * call loadbalancer url

databus23 requested review from BugRoger and SchwarzM August 6, 2021 15:55

databus23 requested review from jknipper and onuryilmaz as code owners August 6, 2021 15:55

databus23 force-pushed the securitygroup branch from e7a7e6b to 30ed716 Compare August 10, 2021 08:55

databus23 mentioned this pull request Aug 13, 2021

Improve lifecycle management of cluster security group #595

Open

databus23 commented Aug 20, 2021

View reviewed changes

databus23 requested a review from auhlig as a code owner August 23, 2021 14:01

databus23 commented Aug 24, 2021

View reviewed changes

databus23 force-pushed the securitygroup branch from 055b65c to 8b1d66d Compare September 10, 2021 12:04

stale bot added the wontfix label Jan 9, 2022

databus23 force-pushed the securitygroup branch from 8b1d66d to 48fa697 Compare April 21, 2022 13:45

stale bot removed the wontfix label Apr 21, 2022

databus23 force-pushed the securitygroup branch 3 times, most recently from 4b8c704 to b7c6e22 Compare May 12, 2022 11:30

databus23 force-pushed the securitygroup branch from 0de9f29 to e416930 Compare October 11, 2022 09:59

databus23 added 5 commits November 3, 2022 16:21

flightcontroller: ensure more security group rules

67f4d59

Ensure regional keppel and object-store are accesible from cluster nodes

51e340d

add more rules

9608be4

Add id to rule description

6965b4a

secgroups: add metrics, improve e2e tests

4ddfb44

* add metrics for security group reconciliation errors * add kluster event for securitu group reconcilicaiton errors * e2e: * create empty security group for cluster * run kube-detective * call loadbalancer url

databus23 force-pushed the securitygroup branch from 836ceaf to 4ddfb44 Compare November 3, 2022 15:21

databus23 force-pushed the securitygroup branch from 6cf0471 to 835adac Compare November 18, 2022 15:57

Fix linters and tests

94e35de

databus23 force-pushed the securitygroup branch from 835adac to 94e35de Compare November 22, 2022 12:00

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

flightcontroller: ensure more security group rules #601

flightcontroller: ensure more security group rules #601

databus23 commented Aug 6, 2021 •

edited

databus23 Aug 20, 2021

databus23 Aug 20, 2021

jknipper Aug 20, 2021

databus23 Aug 20, 2021 •

edited

jknipper Aug 20, 2021

databus23 Aug 20, 2021

jknipper Aug 20, 2021

databus23 Aug 20, 2021

databus23 Aug 20, 2021

databus23 Aug 20, 2021

databus23 Aug 20, 2021

databus23 Aug 20, 2021

databus23 Aug 20, 2021

databus23 Aug 20, 2021

databus23 Aug 20, 2021

databus23 Aug 20, 2021

jknipper Aug 20, 2021

databus23 left a comment •

edited

databus23 left a comment

databus23 Aug 24, 2021

stale bot commented Jan 9, 2022

sandzwerg commented Jun 29, 2022

databus23 commented Jun 29, 2022 •

edited

sandzwerg commented Jun 29, 2022

	Description: fmt.Sprintf(`Kubernikus: SecurityGroup containing LoadBalancer Self-IPs for cluster "%s"`, kluster.Spec.Name),
	Description: fmt.Sprintf(`Kubernikus: SecurityGroup containing LoadBalancer VIP and Self-IPs for cluster "%s"`, kluster.Spec.Name),

	return false, err
	return false, fmt.Errorf("Failed to list load balancers: %w", err)

	return false, err
	return false, fmt.Errorf("Failed to extract load balancers: %w", err)

	return false, err
	return false, fmt.Errorf("Failed to add security group %s to port %s: %w", err, lbGroup.ID, lb.VipPortID, err)

flightcontroller: ensure more security group rules #601

Are you sure you want to change the base?

flightcontroller: ensure more security group rules #601

Conversation

databus23 commented Aug 6, 2021 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

databus23 Aug 20, 2021 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

databus23 left a comment • edited

Choose a reason for hiding this comment

databus23 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

stale bot commented Jan 9, 2022

sandzwerg commented Jun 29, 2022

databus23 commented Jun 29, 2022 • edited

sandzwerg commented Jun 29, 2022

databus23 commented Aug 6, 2021 •

edited

databus23 Aug 20, 2021 •

edited

databus23 left a comment •

edited

databus23 commented Jun 29, 2022 •

edited