Skip to content

feat: add more rate limit control flags #5570

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

Merged
merged 6 commits into from
Jan 5, 2023
Merged

feat: add more rate limit control flags #5570

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
51c825b
feat: add more rate limit control flags
deansheather Jan 4, 2023
91da46f
feat: add flag to disable all rate limits
deansheather Jan 4, 2023
570431c
Merge branch 'main' into dean/more-rate-limit-flags
deansheather Jan 5, 2023
6b738c3
chore: remove new ratelimit flags
deansheather Jan 5, 2023
fea5eae
fix: add env var override field to deployconfig
deansheather Jan 5, 2023
f3d5166
fixup! fix: add env var override field to deployconfig
deansheather Jan 5, 2023
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
60 changes: 46 additions & 14 deletions cli/deployment/config.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -429,11 +429,22 @@ func newConfig() *codersdk.DeploymentConfig {
Default: 10 * time.Minute,
},
},
APIRateLimit: &codersdk.DeploymentConfigField[int]{
Name: "API Rate Limit",
Usage: "Maximum number of requests per minute allowed to the API per user, or per IP address for unauthenticated users. Negative values mean no rate limit. Some API endpoints are always rate limited regardless of this value to prevent denial-of-service attacks.",
Flag: "api-rate-limit",
Default: 512,
RateLimit: &codersdk.RateLimitConfig{
DisableAll: &codersdk.DeploymentConfigField[bool]{
Name: "Disable All Rate Limits",
Usage: "Disables all rate limits. This is not recommended in production.",
Flag: "dangerous-disable-rate-limits",
Default: false,
},
API: &codersdk.DeploymentConfigField[int]{
Name: "API Rate Limit",
Usage: "Maximum number of requests per minute allowed to the API per user, or per IP address for unauthenticated users. Negative values mean no rate limit. Some API endpoints have separate strict rate limits regardless of this value to prevent denial-of-service or brute force attacks.",
// Change the env from the auto-generated CODER_RATE_LIMIT_API to the
// old value to avoid breaking existing deployments.
EnvOverride: "CODER_API_RATE_LIMIT",
Flag: "api-rate-limit",
Default: 512,
},
},
Experimental: &codersdk.DeploymentConfigField[bool]{
Name: "Experimental",
Expand Down Expand Up @@ -498,21 +509,30 @@ func setConfig(prefix string, vip *viper.Viper, target interface{}) {
// assigned a value.
if strings.HasPrefix(typ.Name(), "DeploymentConfigField[") {
value := val.FieldByName("Value").Interface()

env, ok := val.FieldByName("EnvOverride").Interface().(string)
if !ok {
panic("DeploymentConfigField[].EnvOverride must be a string")
}
if env == "" {
env = formatEnv(prefix)
}

switch value.(type) {
case string:
vip.MustBindEnv(prefix, formatEnv(prefix))
vip.MustBindEnv(prefix, env)
val.FieldByName("Value").SetString(vip.GetString(prefix))
case bool:
vip.MustBindEnv(prefix, formatEnv(prefix))
vip.MustBindEnv(prefix, env)
val.FieldByName("Value").SetBool(vip.GetBool(prefix))
case int:
vip.MustBindEnv(prefix, formatEnv(prefix))
vip.MustBindEnv(prefix, env)
val.FieldByName("Value").SetInt(int64(vip.GetInt(prefix)))
case time.Duration:
vip.MustBindEnv(prefix, formatEnv(prefix))
vip.MustBindEnv(prefix, env)
val.FieldByName("Value").SetInt(int64(vip.GetDuration(prefix)))
case []string:
vip.MustBindEnv(prefix, formatEnv(prefix))
vip.MustBindEnv(prefix, env)
// As of October 21st, 2022 we supported delimiting a string
// with a comma, but Viper only supports with a space. This
// is a small hack around it!
Expand Down Expand Up @@ -580,6 +600,9 @@ func readSliceFromViper[T any](vip *viper.Viper, key string, value any) []T {

// Ensure the env entry for this key is registered
// before checking value.
//
// We don't support DeploymentConfigField[].EnvOverride for array flags so
// this is fine to just use `formatEnv` here.
vip.MustBindEnv(configKey, formatEnv(configKey))

value := vip.Get(configKey)
Expand Down Expand Up @@ -626,7 +649,7 @@ func setViperDefaults(prefix string, vip *viper.Viper, target interface{}) {
val := reflect.ValueOf(target).Elem()
val = reflect.Indirect(val)
typ := val.Type()
if strings.HasPrefix(typ.Name(), "DeploymentConfigField") {
if strings.HasPrefix(typ.Name(), "DeploymentConfigField[") {
value := val.FieldByName("Default").Interface()
vip.SetDefault(prefix, value)
return
Expand Down Expand Up @@ -663,7 +686,7 @@ func AttachFlags(flagset *pflag.FlagSet, vip *viper.Viper, enterprise bool) {
func setFlags(prefix string, flagset *pflag.FlagSet, vip *viper.Viper, target interface{}, enterprise bool) {
val := reflect.Indirect(reflect.ValueOf(target))
typ := val.Type()
if strings.HasPrefix(typ.Name(), "DeploymentConfigField") {
if strings.HasPrefix(typ.Name(), "DeploymentConfigField[") {
isEnt := val.FieldByName("Enterprise").Bool()
if enterprise != isEnt {
return
Expand All @@ -672,15 +695,24 @@ func setFlags(prefix string, flagset *pflag.FlagSet, vip *viper.Viper, target in
if flg == "" {
return
}

env, ok := val.FieldByName("EnvOverride").Interface().(string)
if !ok {
panic("DeploymentConfigField[].EnvOverride must be a string")
}
if env == "" {
env = formatEnv(prefix)
}

usage := val.FieldByName("Usage").String()
usage = fmt.Sprintf("%s\n%s", usage, cliui.Styles.Placeholder.Render("Consumes $"+formatEnv(prefix)))
usage = fmt.Sprintf("%s\n%s", usage, cliui.Styles.Placeholder.Render("Consumes $"+env))
shorthand := val.FieldByName("Shorthand").String()
hidden := val.FieldByName("Hidden").Bool()
value := val.FieldByName("Default").Interface()

// Allow currently set environment variables
// to override default values in help output.
vip.MustBindEnv(prefix, formatEnv(prefix))
vip.MustBindEnv(prefix, env)

switch value.(type) {
case string:
Expand Down
6 changes: 4 additions & 2 deletions cli/scaletest.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -265,7 +265,7 @@ func requireAdmin(ctx context.Context, client *codersdk.Client) (codersdk.User,
// Only owners can do scaletests. This isn't a very strong check but there's
// not much else we can do. Ratelimits are enforced for non-owners so
// hopefully that limits the damage if someone disables this check and runs
// it against a non-owner account.
// it against a non-owner account on a production deployment.
ok := false
for _, role := range me.Roles {
if role.Name == "owner" {
Expand Down Expand Up @@ -488,7 +488,9 @@ func scaletestCreateWorkspaces() *cobra.Command {
cmd := &cobra.Command{
Use: "create-workspaces",
Short: "Creates many workspaces and waits for them to be ready",
Long: "Creates many users, then creates a workspace for each user and waits for them finish building and fully come online. Optionally runs a command inside each workspace, and connects to the workspace over WireGuard.",
Long: `Creates many users, then creates a workspace for each user and waits for them finish building and fully come online. Optionally runs a command inside each workspace, and connects to the workspace over WireGuard.

It is recommended that all rate limits are disabled on the server before running this scaletest. This test generates many login events which will be rate limited against the (most likely single) IP.`,
RunE: func(cmd *cobra.Command, args []string) error {
ctx := cmd.Context()
client, err := CreateClient(cmd)
Expand Down
14 changes: 13 additions & 1 deletion cli/server.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -104,6 +104,16 @@ func Server(vip *viper.Viper, newAPI func(context.Context, *coderd.Options) (*co
return xerrors.Errorf("either HTTP or TLS must be enabled")
}

// Disable rate limits if the `--dangerous-disable-rate-limits` flag
// was specified.
loginRateLimit := 60
filesRateLimit := 12
if cfg.RateLimit.DisableAll.Value {
cfg.RateLimit.API.Value = -1
loginRateLimit = -1
filesRateLimit = -1
}

printLogo(cmd)
logger := slog.Make(sloghuman.Sink(cmd.ErrOrStderr()))
if ok, _ := cmd.Flags().GetBool(varVerbose); ok {
Expand Down Expand Up @@ -432,7 +442,9 @@ func Server(vip *viper.Viper, newAPI func(context.Context, *coderd.Options) (*co
AgentStatsRefreshInterval: cfg.AgentStatRefreshInterval.Value,
DeploymentConfig: cfg,
PrometheusRegistry: prometheus.NewRegistry(),
APIRateLimit: cfg.APIRateLimit.Value,
APIRateLimit: cfg.RateLimit.API.Value,
LoginRateLimit: loginRateLimit,
FilesRateLimit: filesRateLimit,
HTTPClient: httpClient,
}
if tlsConfig != nil {
Expand Down
2 changes: 2 additions & 0 deletions cli/testdata/coder_scaletest_create-workspaces_--help.golden
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,5 +1,7 @@
Creates many users, then creates a workspace for each user and waits for them finish building and fully come online. Optionally runs a command inside each workspace, and connects to the workspace over WireGuard.

It is recommended that all rate limits are disabled on the server before running this scaletest. This test generates many login events which will be rate limited against the (most likely single) IP.

Usage:
coder scaletest create-workspaces [flags]

Expand Down
10 changes: 7 additions & 3 deletions cli/testdata/coder_server_--help.golden
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,16 +18,20 @@ Flags:
allowed to the API per user, or per IP
address for unauthenticated users.
Negative values mean no rate limit. Some
API endpoints are always rate limited
regardless of this value to prevent
denial-of-service attacks.
API endpoints have separate strict rate
limits regardless of this value to
prevent denial-of-service or brute force
attacks.
Consumes $CODER_API_RATE_LIMIT (default 512)
--cache-dir string The directory to cache temporary files.
If unspecified and $CACHE_DIRECTORY is
set, it will be used for compatibility
with systemd.
Consumes $CODER_CACHE_DIRECTORY (default
"/tmp/coder-cli-test-cache")
--dangerous-disable-rate-limits Disables all rate limits. This is not
recommended in production.
Consumes $CODER_RATE_LIMIT_DISABLE_ALL
--derp-config-path string Path to read a DERP mapping from. See:
https://tailscale.com/kb/1118/custom-derp-servers/
Consumes $CODER_DERP_CONFIG_PATH
Expand Down
17 changes: 14 additions & 3 deletions coderd/apidoc/docs.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2325,9 +2325,6 @@ const docTemplate = `{
"agent_stat_refresh_interval": {
"$ref": "#/definitions/codersdk.DeploymentConfigField-time_Duration"
},
"api_rate_limit": {
"$ref": "#/definitions/codersdk.DeploymentConfigField-int"
},
"audit_logging": {
"$ref": "#/definitions/codersdk.DeploymentConfigField-bool"
},
Expand Down Expand Up @@ -2385,6 +2382,9 @@ const docTemplate = `{
"proxy_trusted_origins": {
"$ref": "#/definitions/codersdk.DeploymentConfigField-array_string"
},
"rate_limit": {
"$ref": "#/definitions/codersdk.RateLimitConfig"
},
"scim_api_key": {
"$ref": "#/definitions/codersdk.DeploymentConfigField-string"
},
Expand Down Expand Up @@ -2950,6 +2950,17 @@ const docTemplate = `{
}
}
},
"codersdk.RateLimitConfig": {
"type": "object",
"properties": {
"api": {
"$ref": "#/definitions/codersdk.DeploymentConfigField-int"
},
"disable_all": {
"$ref": "#/definitions/codersdk.DeploymentConfigField-bool"
}
}
},
"codersdk.Response": {
"type": "object",
"properties": {
Expand Down
17 changes: 14 additions & 3 deletions coderd/apidoc/swagger.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2058,9 +2058,6 @@
"agent_stat_refresh_interval": {
"$ref": "#/definitions/codersdk.DeploymentConfigField-time_Duration"
},
"api_rate_limit": {
"$ref": "#/definitions/codersdk.DeploymentConfigField-int"
},
"audit_logging": {
"$ref": "#/definitions/codersdk.DeploymentConfigField-bool"
},
Expand Down Expand Up @@ -2118,6 +2115,9 @@
"proxy_trusted_origins": {
"$ref": "#/definitions/codersdk.DeploymentConfigField-array_string"
},
"rate_limit": {
"$ref": "#/definitions/codersdk.RateLimitConfig"
},
"scim_api_key": {
"$ref": "#/definitions/codersdk.DeploymentConfigField-string"
},
Expand Down Expand Up @@ -2662,6 +2662,17 @@
}
}
},
"codersdk.RateLimitConfig": {
"type": "object",
"properties": {
"api": {
"$ref": "#/definitions/codersdk.DeploymentConfigField-int"
},
"disable_all": {
"$ref": "#/definitions/codersdk.DeploymentConfigField-bool"
}
}
},
"codersdk.Response": {
"type": "object",
"properties": {
Expand Down
Loading