Ingester: allow only POST method on `/ingester/shutdown` #7707

kevinmingtarja · 2024-03-23T20:43:32Z

What this PR does

Previously, we allow both POST and GET /ingester/shutdown, which makes it too easy to accidentally trigger, i.e. internal scanners that traverse links can trigger shutdown of ingester just by visiting the link.

This PR makes that less likely to happen by only allowing POST requests on /ingester/shutdown. At the same time, if users still want to opt in to the existing behavior, we allow that by adding a new -ingester.get-request-for-shutdown-enabled flag (defaults to false). But this flag is also temporary and will be removed after two Mimir releases (2.15).

Which issue(s) this PR fixes or relates to

Fixes #7623

Checklist

Tests updated.
Documentation added.
CHANGELOG.md updated - the order of entries should be [CHANGE], [FEATURE], [ENHANCEMENT], [BUGFIX].
about-versioning.md updated with experimental features.

CLAassistant · 2024-03-23T20:43:37Z

All committers have signed the CLA.

Signed-off-by: Kevin Mingtarja <[email protected]>

jhesketh

Thank you for your contribution!

There are a few tools and docs that will need updating to use POST:

https://github.com/grafana/mimir/blob/main/tools/migrate-ingester-statefulsets.sh#L46

https://github.com/grafana/mimir/blob/main/docs/sources/mimir/manage/run-production-environment/scaling-out.md?plain=1 (where it says invoke it should probably say POST).

https://github.com/grafana/mimir/blob/main/docs/sources/mimir/references/http-api/index.md?plain=1#L51 (and L412)

It would also be good to see a test to make sure GET is disabled unless the config flag is set.

pstibrany

Thank you for working on this!

pkg/api/api.go

CHANGELOG.md

pkg/api/api.go

Signed-off-by: Kevin Mingtarja <[email protected]>

…shutdown

kevinmingtarja · 2024-04-04T06:28:05Z

Hi @jhesketh, @pstibrany, thank you as well for the comments!

I have updated the docs/tools that I missed, and addressed the comments in a follow-up commit. Please let me know if this is okay.

pstibrany

Thank you very much for working on this! I've left few minor comments.

CHANGELOG.md

pkg/api/api.go

pkg/api/api_test.go

jdbaldry

docs changes LGTM

Signed-off-by: Kevin Mingtarja <[email protected]>

pkg/api/api.go

Signed-off-by: Kevin Mingtarja <[email protected]>

pstibrany

Thank you very much!

jhesketh

Thank you for the improvements!

jhesketh

I think this will fix the unit test

cmd/mimir/help-all.txt.tmpl

Fix missing [deprecated] tag

jhesketh

.

jhesketh · 2024-04-05T03:49:04Z

pkg/api/api.go

@@ -71,6 +74,7 @@ type Config struct {
 func (cfg *Config) RegisterFlags(f *flag.FlagSet) {
 	f.BoolVar(&cfg.SkipLabelNameValidationHeader, "api.skip-label-name-validation-header-enabled", false, "Allows to skip label name validation via X-Mimir-SkipLabelNameValidation header on the http write path. Use with caution as it breaks PromQL. Allowing this for external clients allows any client to send invalid label names. After enabling it, requests with a specific HTTP header set to true will not have label names validated.")
 	f.BoolVar(&cfg.EnableOtelMetadataStorage, "distributor.enable-otlp-metadata-storage", true, "If true, store metadata when ingesting metrics via OTLP. This makes metric descriptions and types available for metrics ingested via OTLP.")
+	f.BoolVar(&cfg.GETRequestForIngesterShutdownEnabled, "get-request-for-ingester-shutdown-enabled", false, "Enable GET requests to the /ingester/shutdown endpoint to trigger an ingester shutdown. This is a potentially dangerous operation and should only be enabled consciously.")


On re-review, this should probably be:

Suggested change

f.BoolVar(&cfg.GETRequestForIngesterShutdownEnabled, "get-request-for-ingester-shutdown-enabled", false, "Enable GET requests to the /ingester/shutdown endpoint to trigger an ingester shutdown. This is a potentially dangerous operation and should only be enabled consciously.")

f.BoolVar(&cfg.GETRequestForIngesterShutdownEnabled, "ingester.get-request-for-shutdown-enabled", false, "Enable GET requests to the /ingester/shutdown endpoint to trigger an ingester shutdown. This is a potentially dangerous operation and should only be enabled consciously.")

And the corresponding changes to the docs.

Got it, should the yaml field name stay the same though?

Right now it's get_request_for_ingester_shutdown_enabled

Actually, initially I did as you suggested, but then Peter suggested to make it consistent so I changed it: #7707 (comment)

Oh right, I missed that conversation sorry. Looking at the distributor.enable-otlp-metadata-storage flag (just above) it exists as both distributor.enable-otlp-metadata-storage as the flag and enable_otel_metadata_translation as the yaml. @pstibrany Just want to double check your opinion here please.

I'm sorry for confusion here. The problem here comes from the fact that config option is defined in api config block in YAML, but we're trying to use ingester. prefix for CLI flag. At the same time, we're triyng to make CLI flag and yaml field name consistent (this was goal of my previous comment, without realizing it's causing more confusion).

I think we should settle on:

-api.get-request-for-ingester-shutdown-enabled for CLI flag

get_request_for_ingester_shutdown_enabled as YAML field name (it's already under api section)

WDYT?

If we want to use -ingester. prefix for CLI flag, then we should put this into ingester config struct.

Unfortunately I think enable_otel_metadata_translation was a mistake, as it doesn't follow our practices. ("enabled" suffix, consistent CLI and YAML names)

@jhesketh thanks for catching this. My suggestion was not to remove -ingester. prefix (only to add ingester in the middle of the name), and I missed that this is what happened. 🤦

Ah sorry for the misunderstanding too. So should we keep going with the -ingester. prefix or -api. instead? As you mentioned, I think the latter makes more sense as it's already under the api section in the yaml.

Hi Kevin, with Josh we agreed to go with -api.get-request-for-ingester-shutdown-enabled CLI flag and get_request_for_ingester_shutdown_enabled YAML field name. Thank you very much for your patience with us!

Hi Peter, no worries! Let me quickly do that

Signed-off-by: Kevin Mingtarja <[email protected]>

jhesketh

LGTM! Thank you for persisting with our miss on the naming :-). Will leave it for Peter to do final look+merge.

kevinmingtarja · 2024-04-12T03:42:30Z

Hang on did i miss something, somehow one of the test fails with this, even though (default false) is there in help-all.txt.tmpl.

Diff:
            	            	--- Expected
            	            	+++ Actual
            	            	@@ -282,3 +282,3 @@
            	            	   -api.get-request-for-ingester-shutdown-enabled
            	            	-    	[deprecated] Enable GET requests to the /ingester/shutdown endpoint to trigger an ingester shutdown. This is a potentially dangerous operation and should only be enabled consciously. (default false)
            	            	+    	[deprecated] Enable GET requests to the /ingester/shutdown endpoint to trigger an ingester shutdown. This is a potentially dangerous operation and should only be enabled consciously.
            	            	   -api.skip-label-name-validation-header-enabled
            	Test:       	TestHelp/all
            	Messages:   	./cmd/mimir/mimir -help-all output changed; try `make reference-help`

Signed-off-by: Peter Štibraný <[email protected]>

pstibrany

Thank you very much for your contribution and patience with us!

pstibrany · 2024-04-12T07:30:17Z

Hang on did i miss something, somehow one of the test fails with this, even though (default false) is there in help-all.txt.tmpl.

I've regenerated help (using make reference-help) and pushed that change to resolve this problem.

kevinmingtarja · 2024-04-13T07:06:32Z

No worries, thanks as well for the review Peter and Joshua!

kevinmingtarja requested review from jdbaldry and a team as code owners March 23, 2024 20:43

Ingester: allow only POST method on /ingester/shutdown

26ea0af

Signed-off-by: Kevin Mingtarja <[email protected]>

kevinmingtarja force-pushed the kevinmingtarja/allow-only-post-for-ingester-shutdown branch from 20bcfa5 to 26ea0af Compare March 23, 2024 20:44

jhesketh requested changes Mar 25, 2024

View reviewed changes

pstibrany reviewed Mar 25, 2024

View reviewed changes

pkg/api/api.go Outdated Show resolved Hide resolved

CHANGELOG.md Outdated Show resolved Hide resolved

pkg/api/api.go Outdated Show resolved Hide resolved

pkg/api/api.go Outdated Show resolved Hide resolved

kevinmingtarja and others added 2 commits April 4, 2024 14:22

Add tests, address review comments

47f4424

Signed-off-by: Kevin Mingtarja <[email protected]>

Merge branch 'main' into kevinmingtarja/allow-only-post-for-ingester-…

d6b676c

…shutdown

kevinmingtarja requested review from jhesketh and pstibrany April 4, 2024 06:25

pstibrany reviewed Apr 4, 2024

View reviewed changes

CHANGELOG.md Outdated Show resolved Hide resolved

pkg/api/api.go Outdated Show resolved Hide resolved

pkg/api/api_test.go Outdated Show resolved Hide resolved

jdbaldry reviewed Apr 4, 2024

View reviewed changes

kevinmingtarja requested a review from pstibrany April 4, 2024 12:20

Minor fixes

366ad48

Signed-off-by: Kevin Mingtarja <[email protected]>

pstibrany reviewed Apr 4, 2024

View reviewed changes

pkg/api/api.go Outdated Show resolved Hide resolved

Fix CLI flag name

c89a030

Signed-off-by: Kevin Mingtarja <[email protected]>

pstibrany approved these changes Apr 4, 2024

View reviewed changes

jhesketh approved these changes Apr 5, 2024

View reviewed changes

jhesketh reviewed Apr 5, 2024

View reviewed changes

cmd/mimir/help-all.txt.tmpl Outdated Show resolved Hide resolved

Update cmd/mimir/help-all.txt.tmpl

1eb0294

Fix missing [deprecated] tag

jhesketh enabled auto-merge (squash) April 5, 2024 02:57

jhesketh disabled auto-merge April 5, 2024 03:50

jhesketh requested changes Apr 5, 2024

View reviewed changes

Set CLI flag to -api.get-request-for-ingester-shutdown-enabled

b3d6829

Signed-off-by: Kevin Mingtarja <[email protected]>

kevinmingtarja requested a review from jhesketh April 11, 2024 14:21

jhesketh approved these changes Apr 12, 2024

View reviewed changes

Regenerated help.

900c3fa

Signed-off-by: Peter Štibraný <[email protected]>

pstibrany approved these changes Apr 12, 2024

View reviewed changes

pstibrany enabled auto-merge (squash) April 12, 2024 07:30

pstibrany merged commit d484152 into grafana:main Apr 12, 2024
29 checks passed

kevinmingtarja deleted the kevinmingtarja/allow-only-post-for-ingester-shutdown branch April 13, 2024 03:59

eyenx mentioned this pull request May 28, 2024

chore(mimir): update to 5.3.0 adfinis/helm-charts#1256

Merged

7 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Ingester: allow only POST method on `/ingester/shutdown` #7707

Ingester: allow only POST method on `/ingester/shutdown` #7707

kevinmingtarja commented Mar 23, 2024 •

edited

Loading

CLAassistant commented Mar 23, 2024 •

edited

Loading

jhesketh left a comment

pstibrany left a comment

kevinmingtarja commented Apr 4, 2024

pstibrany left a comment

jdbaldry left a comment

pstibrany left a comment

jhesketh left a comment

jhesketh left a comment

jhesketh left a comment •

edited

Loading

jhesketh Apr 5, 2024

kevinmingtarja Apr 5, 2024 •

edited

Loading

kevinmingtarja Apr 5, 2024

jhesketh Apr 5, 2024

pstibrany Apr 5, 2024 •

edited

Loading

pstibrany Apr 5, 2024 •

edited

Loading

kevinmingtarja Apr 5, 2024

pstibrany Apr 8, 2024

kevinmingtarja Apr 11, 2024

kevinmingtarja Apr 11, 2024

jhesketh left a comment

kevinmingtarja commented Apr 12, 2024

pstibrany left a comment

pstibrany commented Apr 12, 2024 •

edited

Loading

kevinmingtarja commented Apr 13, 2024

	f.BoolVar(&cfg.GETRequestForIngesterShutdownEnabled, "get-request-for-ingester-shutdown-enabled", false, "Enable GET requests to the /ingester/shutdown endpoint to trigger an ingester shutdown. This is a potentially dangerous operation and should only be enabled consciously.")
	f.BoolVar(&cfg.GETRequestForIngesterShutdownEnabled, "ingester.get-request-for-shutdown-enabled", false, "Enable GET requests to the /ingester/shutdown endpoint to trigger an ingester shutdown. This is a potentially dangerous operation and should only be enabled consciously.")

Ingester: allow only POST method on /ingester/shutdown #7707

Ingester: allow only POST method on /ingester/shutdown #7707

Conversation

kevinmingtarja commented Mar 23, 2024 • edited Loading

What this PR does

Which issue(s) this PR fixes or relates to

Checklist

CLAassistant commented Mar 23, 2024 • edited Loading

jhesketh left a comment

Choose a reason for hiding this comment

pstibrany left a comment

Choose a reason for hiding this comment

kevinmingtarja commented Apr 4, 2024

pstibrany left a comment

Choose a reason for hiding this comment

jdbaldry left a comment

Choose a reason for hiding this comment

pstibrany left a comment

Choose a reason for hiding this comment

jhesketh left a comment

Choose a reason for hiding this comment

jhesketh left a comment

Choose a reason for hiding this comment

jhesketh left a comment • edited Loading

Choose a reason for hiding this comment

jhesketh Apr 5, 2024

Choose a reason for hiding this comment

kevinmingtarja Apr 5, 2024 • edited Loading

Choose a reason for hiding this comment

kevinmingtarja Apr 5, 2024

Choose a reason for hiding this comment

jhesketh Apr 5, 2024

Choose a reason for hiding this comment

pstibrany Apr 5, 2024 • edited Loading

Choose a reason for hiding this comment

pstibrany Apr 5, 2024 • edited Loading

Choose a reason for hiding this comment

kevinmingtarja Apr 5, 2024

Choose a reason for hiding this comment

pstibrany Apr 8, 2024

Choose a reason for hiding this comment

kevinmingtarja Apr 11, 2024

Choose a reason for hiding this comment

kevinmingtarja Apr 11, 2024

Choose a reason for hiding this comment

jhesketh left a comment

Choose a reason for hiding this comment

kevinmingtarja commented Apr 12, 2024

pstibrany left a comment

Choose a reason for hiding this comment

pstibrany commented Apr 12, 2024 • edited Loading

kevinmingtarja commented Apr 13, 2024

Ingester: allow only POST method on `/ingester/shutdown` #7707

Ingester: allow only POST method on `/ingester/shutdown` #7707

kevinmingtarja commented Mar 23, 2024 •

edited

Loading

CLAassistant commented Mar 23, 2024 •

edited

Loading

jhesketh left a comment •

edited

Loading

kevinmingtarja Apr 5, 2024 •

edited

Loading

pstibrany Apr 5, 2024 •

edited

Loading

pstibrany Apr 5, 2024 •

edited

Loading

pstibrany commented Apr 12, 2024 •

edited

Loading