Skip to content
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

Jump to bottom

Provide option to enforce network connection rate limiting #13891

Merged
merged 3 commits into from
Oct 18, 2022
Merged

Provide option to enforce network connection rate limiting #13891

merged 3 commits into from
Oct 18, 2022

Conversation

Furisto
Copy link
Member

@Furisto Furisto commented Oct 16, 2022 •
edited
Loading

Description

Provide an option to drop traffic if network connections go over the configured limit. Also changes to the netlimit configuration can now be applied without having to restart ws-daemon.

Related Issue(s)

n.a.

How to test

  • Ensure that ws-daemon config map looks like this
 "netlimit": {
          "enabled": true,
          "enforce": true,
          "connectionsPerMinute": 1000,
          "bucketSize": 1000
        },
  • Open workspace in preview environment
  • Workspace should have label gitpod.io/netConnLimitPerMinute
  • SSH into preview vm and find workspace ring 0 process
  • Use the process id to enter the network namespace
  • Check that nftable rules will drop traffic
  • Edit the config map of ws-daemon e.g. change connections per minute
  • Wait ~1 minute for changes to propagate
  • Start new workspace and check nftables rules again

Release Notes

Provide option to enforce network connection rate limiting

Werft options:

  • /werft with-local-preview
    If enabled this will build install/preview
  • /werft with-preview
  • /werft with-integration-tests=all
    Valid options are all, workspace, webapp, ide

@Furisto Furisto added team: workspace Issue belongs to the Workspace team feature: connection-limiting labels Oct 16, 2022
@Furisto Furisto requested review from a team October 16, 2022 10:29
@Furisto Furisto self-assigned this Oct 16, 2022
jenting
jenting reviewed Oct 17, 2022
View reviewed changes
Copy link
Contributor

@jenting jenting left a comment

Choose a reason for hiding this comment

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

Once question, others LGTM

components/ws-daemon/pkg/daemon/daemon.go
Comment on lines +116 to +119
if config.NetLimit.Enabled {
netlimiter.Update(config.NetLimit)
}
return nil
Copy link
Contributor

Choose a reason for hiding this comment

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

Are we able to disable netlimit without manually restarting ws-daemon?

Copy link
Member Author

Choose a reason for hiding this comment

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

We are not able to completely disable it without restarting ws-daemon, but we can disable that traffic is dropped. In other words it would work like it does at the moment where the traffic is monitored but no action is taken.

@kylos101
Copy link
Contributor

kylos101 commented Oct 18, 2022 •
edited by werft-gitpod-dev-com bot
Loading

/werft run with-integration-tests=workspace with-large-vm=true

👍 started the job as gitpod-build-fo-enforce-limiting.3
(with .werft/ from main)

@kylos101
Copy link
Contributor

@Furisto @jenting it's best to assert that tests pass prior to approving PRs.

Otherwise we could potentially break main. 🙏 Please refer to our reviewing code section for background.

@kylos101
Copy link
Contributor

/hold

Let's wait for tests to pass. Please remove this hold once tests are passing.

@kylos101
Copy link
Contributor

/unhold

Tests are passing

@roboquat roboquat merged commit 0a38956 into main Oct 18, 2022
@roboquat roboquat deleted the fo/enforce-limiting branch October 18, 2022 13:24
@roboquat roboquat added the deployed: workspace Workspace team change is running in production label Oct 19, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jenting jenting jenting approved these changes

@ArthurSens ArthurSens ArthurSens approved these changes

Assignees

@Furisto Furisto

Labels
deployed: workspace Workspace team change is running in production feature: connection-limiting release-note size/M team: SID team: workspace Issue belongs to the Workspace team
Projects
No open projects
🌌 Workspace Team
Status: Done
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@Furisto @kylos101 @ArthurSens @jenting @roboquat