Limit connections by Furisto · Pull Request #11255 · gitpod-io/gitpod

Furisto · 2022-07-09T19:30:01Z

Description

Introduce limiting of new connections created per minute. For more info see the RFC.

Related Issue(s)

Related to #10651

How to test

Open a workspace in the preview environment
Annotate the pod with gitpd.io/netConnLimit=true
This should add additonal nftable rules to the network namespace of the pod (ssh into the node and then use nsenter -t workspacekitPid -n and nft list ruleset)
Metrics can be obtained with kubectl port-forward ds/ws-daemon 9500:9500, then get the metrics with curl -XGET localhost:9500/metrics
Run roblox scanner

Release Notes

Limit the rate at which network connections can be made by a workspace

Werft options:

/werft with-preview

aledbf · 2022-07-11T18:36:30Z

+	rule := fmt.Sprintf("add rule ip gitpod ratelimit ip protocol tcp ct state new meter connmeter { ip daddr & 0.0.0.0 timeout 1m limit rate over %d/minute } counter drop", limit)
+
+	// the go library does not support meters, so use nft instead
+	cmd := exec.CommandContext(ctx, "/usr/sbin/nft", rule)


What's the behavior when the version of nft on the host is different? (this is a known issue with k8s kubelet)
Edit: is kube-proxy not kubelet. Sorry.

Can you explain what kind of issues have been observed in the context of kubelet? I was not able to find anything.

@aledbf Are you ok with merging this or would you like to discuss it further?

I am not sure we should merge this without knowing the behavior of different nftable versions and if there is any conflict with the CNI bandwidth plugin.
Did you test this change in an ephemeral cluster?

workspace preview

We used nftables to replace slirp4net, if there is an issue with versions, would we have more fundamental issues with workspace networking?
This makes me wonder if we should add related runtime tests for networking.
I ask because I'm not sure what dimensions to consider.
@aledbf I like the idea of testing with an ephemeral cluster. Can you add a related test here, so we can have a repeatable way to do in the future?
@Furisto let's leave on-hold till we've had a chance to test in an ephemeral cluster (to test for impacts to CNI bandwidth plugin).

sagor999

LGTM
adding hold in case if @aledbf comment requires resolving prior to merging
feel free to remove it.

/hold

jenting

LGTM, but I would ❤️ to have the release note for this one.

utam0k · 2022-07-19T01:17:33Z

+	rule := fmt.Sprintf("add rule ip gitpod ratelimit ip protocol tcp ct state new meter connmeter { ip daddr & 0.0.0.0 timeout 1m limit rate over %d/minute } counter drop", limit)
+
+	// the go library does not support meters, so use nft instead
+	cmd := exec.CommandContext(ctx, "/usr/sbin/nft", rule)


Since this function runs before pivot_root, I believe it is safe to specify the nftable bin with the complete path at this stage. Am I right? If yes, please add a comment that why it is safe to run.

I'm concerned that the user may be able to override this bin with a custom image.

We do not enter the mount namespace of the workspace, only the network namespace. The binary is part of ws-daemon.

💯 Can I ask you to add a comment about that? Why? I think this is a very important security point that is difficult to see from the code.

utam0k · 2022-07-19T04:27:56Z

+func addConnectionLimitRule(limit int) error {
+	ctx, cancel := context.WithTimeout(context.Background(), 3*time.Second)
+	defer cancel()
+	rule := fmt.Sprintf("add rule ip gitpod ratelimit ip protocol tcp ct state new meter connmeter { ip daddr & 0.0.0.0 timeout 1m limit rate over %d/minute } counter drop", limit)


Can we use set syntax instead of meter as official documentation says here?
https://wiki.nftables.org/wiki-nftables/index.php/Meters
Note that the meter keyword is obsolete, the dynamic set and map syntax is now preferred for consistency.

I think we can. Thanks for the hint. I also will update this rule to use a named counter, so that we can scrape metrics about dropped packets more easily.

stale · 2022-08-10T03:35:34Z

This pull request 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.

geropl

Reviewed ws-manager-api per code owners: change LGTM

Furisto · 2022-08-23T12:17:58Z

/werft run

👍 started the job as gitpod-build-fo-connlimit.32
(with .werft/ from main)

Furisto · 2022-08-23T13:20:39Z

/werft run with-clean-slate-deployment

👍 started the job as gitpod-build-fo-connlimit.37
(with .werft/ from main)

Furisto · 2022-08-23T16:21:37Z

@utam0k PTAL

utam0k

Awesome

Furisto · 2022-08-24T12:13:56Z

/unhold

roboquat added do-not-merge/work-in-progress do-not-merge/release-note-label-needed size/M labels Jul 9, 2022

Furisto force-pushed the fo/connlimit branch from 73ca822 to f4ef8c3Compare July 11, 2022 11:58

roboquat added size/L release-note-none and removed size/M do-not-merge/release-note-label-needed labels Jul 11, 2022

aledbf reviewed Jul 11, 2022
View reviewed changes

Furisto marked this pull request as ready for review July 11, 2022 19:39

Furisto requested review from a teamJuly 11, 2022 19:39

roboquat removed the do-not-merge/work-in-progress label Jul 11, 2022

github-actionsBot added team: delivery Issue belongs to the self-hosted team team: workspace Issue belongs to the Workspace team labels Jul 11, 2022

mrsimonemms approved these changes Jul 11, 2022
View reviewed changes

sagor999 approved these changes Jul 11, 2022
View reviewed changes

roboquat added the do-not-merge/hold label Jul 11, 2022

jenting reviewed Jul 12, 2022
View reviewed changes

roboquat added release-note and removed release-note-none labels Jul 12, 2022

jenting approved these changes Jul 13, 2022
View reviewed changes

Furisto marked this pull request as draft July 17, 2022 18:10

roboquat added the do-not-merge/work-in-progress label Jul 17, 2022

utam0k reviewed Jul 19, 2022
View reviewed changes

Comment threadcomponents/ws-daemon/nsinsider/main.go

utam0k reviewed Jul 19, 2022
View reviewed changes

roboquat added size/XL and removed size/L labels Jul 28, 2022

geropl approved these changes Aug 23, 2022
View reviewed changes

Furisto force-pushed the fo/connlimit branch from 14e0b28 to fdcdc03Compare August 23, 2022 11:56

Furisto force-pushed the fo/connlimit branch 3 times, most recently from 5cd2b02 to 4f415dcCompare August 23, 2022 12:54

utam0k approved these changes Aug 24, 2022
View reviewed changes

Furisto added 11 commits August 24, 2022 11:23

[nsinsider] Setup connection limit
8ffcd1f

[nsinsider] Make setup helpers for nsinsider reusable
f669ff5

[ws-daemon] Plugin for setting up network limits
2de7c45

[installer] Configuration for network limiting
87fa9d9

[installer] Add required dependencies
a9b38ce

[installer] Update golden files
30b52ba

[ws-manager-api] Add connection limiting feature
31e567d

[ws-manager] Apply network limiting annotation
e6634be

[nsinsider] Accept connections
796ccd2

[ws-daemon] Include time unit and use struct for map
89572d7

[installer] Update dependencies & golden files
d6f4d40

Furisto force-pushed the fo/connlimit branch from 4f415dc to d6f4d40Compare August 24, 2022 11:26

roboquat removed the do-not-merge/hold label Aug 24, 2022

roboquat merged commit 0a9e6f7 into mainAug 24, 2022

roboquat deleted the fo/connlimit branch August 24, 2022 12:14

roboquat added the deployed: webapp Meta team change is running in production label Aug 29, 2022

Furisto added the feature: connection-limiting label Aug 30, 2022

utam0k mentioned this pull request Sep 5, 2022
Epic: Limit TCP connections per workspace #10651
Closed
2 tasks

roboquat added the deployed: workspace Workspace team change is running in production label Sep 7, 2022

utam0k mentioned this pull request Oct 3, 2022
Validate or document breaking changes on Self-Hosted September Release #13494
Closed

Uh oh!

Conversation

Furisto commented Jul 9, 2022• edited by kylos101 Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Related Issue(s)

How to test

Release Notes

Werft options:

Uh oh!

aledbfJul 11, 2022• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

sagor999 left a comment

Choose a reason for hiding this comment

Uh oh!

jenting left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

staleBot commented Aug 10, 2022

Uh oh!

geropl left a comment

Choose a reason for hiding this comment

Uh oh!

Furisto commented Aug 23, 2022• edited by werft-gitpod-dev-com BotLoading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Furisto commented Aug 23, 2022• edited by werft-gitpod-dev-com BotLoading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Furisto commented Aug 23, 2022

Uh oh!

utam0k left a comment

Choose a reason for hiding this comment

Uh oh!

Furisto commented Aug 24, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

9 participants

Furisto commented Jul 9, 2022•
edited by kylos101
Loading

aledbfJul 11, 2022•
edited
Loading

Furisto commented Aug 23, 2022•
edited by werft-gitpod-dev-com Bot
Loading

Furisto commented Aug 23, 2022•
edited by werft-gitpod-dev-com Bot
Loading