Alertmanager: Add grafana config size limit #9402
Conversation
pkg/alertmanager/api_grafana.go
Outdated
| if err != nil { | ||
| maxBytesErr := &http.MaxBytesError{} |
There was a problem hiding this comment.
No need for this to be a pointer (but see comment below, I don't think we need to keep it)
| maxBytesErr := &http.MaxBytesError{} | |
| var maxBytesErr http.MaxBytesError |
There was a problem hiding this comment.
It does need to be a pointer actually. errors.As takes a pointer to the target which implements error. Since http.MaxBytesError has a pointer receiver on Error, the double indirection is required. I've learned this today TBH 😅
Removing the pointer and running the tests panics:
| var input io.Reader | ||
| maxConfigSize := am.limits.AlertmanagerMaxGrafanaConfigSize(userID) | ||
| if maxConfigSize > 0 { | ||
| input = http.MaxBytesReader(w, r.Body, int64(maxConfigSize)) |
There was a problem hiding this comment.
Is there any practical difference between this and io.LimitReader which we use in api.go?
There was a problem hiding this comment.
I saw the io.LimitReader example but I found that a bit confusing TBH - i.e. the reader returns an EOF (with nil error), so we have to explicitly check for the size. http.MaxReader will return an error of type MaxBytesError - IMO this has a better readability.
But I can switch to the LimitReader for consistency if that's preferred 👍
There was a problem hiding this comment.
I'm happy as-is, maybe we switch the LimitReader usage over in a follow up.
| @@ -3666,6 +3666,11 @@ The `limits` block configures default and per-tenant limits imposed by component | |||
| # CLI flag: -alertmanager.notification-rate-limit-per-integration | |||
| [alertmanager_notification_rate_limit_per_integration: <map of string to float64> | default = {}] | |||
|
|
|||
| # Maximum size of the grafana configuration file for Alertmanager that tenant | |||
There was a problem hiding this comment.
| # Maximum size of the grafana configuration file for Alertmanager that tenant | |
| # Maximum size of the Grafana configuration file for Alertmanager that a tenant |
| @@ -3666,6 +3666,11 @@ The `limits` block configures default and per-tenant limits imposed by component | |||
| # CLI flag: -alertmanager.notification-rate-limit-per-integration | |||
| [alertmanager_notification_rate_limit_per_integration: <map of string to float64> | default = {}] | |||
|
|
|||
| # Maximum size of the grafana configuration file for Alertmanager that tenant | |||
| # can upload via Alertmanager API. 0 = no limit. | |||
There was a problem hiding this comment.
| # can upload via Alertmanager API. 0 = no limit. | |
| # can upload via the Alertmanager API. 0 = no limit. |
CHANGELOG.md
Outdated
| @@ -3236,6 +3236,8 @@ _Changes since `grafana/cortex-jsonnet` `1.9.0`._ | |||
| } | |||
| ``` | |||
| * [FEATURE] Added multi-zone ingesters and store-gateways support. #1352 #1552 | |||
| * [FEATURE] The following alertmanager limit was added: #9402 | |||
There was a problem hiding this comment.
This needs to go at the top of the file (and not in the jsonnet section)
There was a problem hiding this comment.
lol... Tks, done now 👍
titolins
force-pushed
the
titolins/implement_config_size_limit
branch
from
9841950 to
acf749e
Compare
|
The CHANGELOG has just been cut to prepare for the next release. Please rebase |
titolins
force-pushed
the
titolins/implement_config_size_limit
branch
from
acf749e to
9d9e501
Compare
titolins
force-pushed
the
titolins/implement_config_size_limit
branch
from
53c259a to
ee1d491
Compare
Awesome, tks! Done now 👍 |
Co-authored-by: Oleg Zaytsev <[email protected]>
Co-authored-by: Steve Simpson <[email protected]>
Remote AM already has limits in place for it's own configuration. However, when using grafana compat mode, it doesn't have any. This aims to add a new config limit for grafana configuration.