ingester: Support for disallowing Push API for ingest storage #7503

narqo · 2024-02-29T07:59:25Z

What this PR does

~~These changes are still in progress by I wanted to get some early feedback, on if the direction is right~~

The changes add a new flag -ingerster.push-grpc-method-enabled (default "true"). It allows a user to configure the app to block "push" gRPC API in the ingester module, IFF it has ingest-store enabled.

Which issue(s) this PR fixes or relates to

n/a

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.

pkg/ingester/ingester.go

Makefile

pracucci

Thanks for working on this! I left a major comment.

pkg/ingester/ingester.go

pkg/ingester/errors.go

pkg/ingester/ingester.go

github-actions · 2024-03-11T15:01:59Z

Building new version of mimir-build-image. After image is built and pushed to Docker Hub, a new commit will automatically be added to this PR with new image version grafana/mimir-build-image:pr7503-ae15c572b6. This can take up to 1 hour.

narqo · 2024-03-12T08:13:24Z

pkg/api/api.go

@@ -281,7 +279,6 @@ type Ingester interface {
 	ShutdownHandler(http.ResponseWriter, *http.Request)
 	PrepareShutdownHandler(http.ResponseWriter, *http.Request)
 	PreparePartitionDownscaleHandler(http.ResponseWriter, *http.Request)
-	PushWithCleanup(context.Context, *mimirpb.WriteRequest, func()) error


The method PushWithCleanup() shouldn't be required by the api.Ingester interface. Thus, I want to remove it here. IMHO, this makes the code a tiny bit simpler.

pstibrany

Thank you, this looks good overall. I have left some small comments.

I would not call this "Push API" because that is widely understood as HTTP push endpoint on distributors. Ingesters only have "Push" gRPC method, so I would suggest to use that name explicitely.

I would also like to see changes that are not necessary for this PR in ingester error mapping and grpc push check to be reverted.

pstibrany · 2024-03-12T09:26:03Z

pkg/ingester/errors.go

+	var errStatus errorWithStatus
+	if errors.As(err, &errStatus) {
+		// If the error was already mapped (or wrapped) into appropriate errorWithStatus,
+		// return it as is.
+		return errStatus
+	}


Why is this change required? Newly introduced errPushAPIDisabled is not mapped via this method. (We could map METHOD_NOT_ALLOWED to codes.Unimplemented if needed).

Same comment in mapPushErrorToErrorWithHTTPOrGRPCStatus. I would suggest to remove unnecessary changes from the PR.

pstibrany · 2024-03-12T09:30:14Z

pkg/ingester/ingester.go

@@ -206,6 +206,8 @@ type Config struct {
 	UpdateIngesterOwnedSeries       bool          `yaml:"track_ingester_owned_series" category:"experimental"`
 	OwnedSeriesUpdateInterval       time.Duration `yaml:"owned_series_update_interval" category:"experimental"`

+	EnablePushAPI bool `yaml:"enable_push_api" category:"experimental" doc:"hidden"`


We try to use "Enabled" suffix, in this case PushAPIEnabled or perhaps PushMethodEnabled or even PushGrpcMethodEnabled to indicate this is very low-level thing that nobody should touch. Same name should be used for YAML field and CLI flag.

pkg/ingester/ingester.go

pkg/mimir/grpc_push_check.go

pkg/mimir/mimir.go

Signed-off-by: Vladimir Varankin <[email protected]> Co-authored-by: Marco Pracucci <[email protected]>

Signed-off-by: Vladimir Varankin <[email protected]>

Co-authored-by: Peter Štibraný <[email protected]> Signed-off-by: Vladimir Varankin <[email protected]>

narqo · 2024-03-12T21:44:54Z

🆙

I think, I've addressed all the comments. PHAL

pstibrany

Thank you very much for this PR and addressing my feedback.

pracucci · 2024-03-18T09:23:55Z

pkg/mimirpb/mimir.proto

@@ -46,6 +46,7 @@ enum ErrorCause {
  TSDB_UNAVAILABLE = 8;
  TOO_BUSY = 9;
  CIRCUIT_BREAKER_OPEN = 10;
+  METHOD_NOT_ALLOWED = 11;


We've added a new error cause, but then this has to be explicitly mapped to HTTP status codes. As a reference, look all places where TSDB_UNAVAILABLE is used and do something similar for METHOD_NOT_ALLOWED too. I would return 405 for METHOD_NOT_ALLOWED.

Actually I see you proposed it in #7618 but got rejected with a good feedback. Let me follow up offline about it.

narqo requested review from pracucci and dimitarvdimitrov February 29, 2024 07:59

narqo requested a review from a team as a code owner February 29, 2024 07:59

narqo marked this pull request as draft February 29, 2024 07:59

narqo force-pushed the ingest-store-dont-push-api branch 2 times, most recently from f8262fb to 03bbc75 Compare February 29, 2024 21:25

narqo changed the title ~~wip! ingester: Support for disallowing Push API for ingest storage~~ ingester: Support for disallowing Push API for ingest storage Feb 29, 2024

narqo commented Feb 29, 2024

View reviewed changes

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

narqo force-pushed the ingest-store-dont-push-api branch from 03bbc75 to 4e482e0 Compare March 1, 2024 17:34

narqo marked this pull request as ready for review March 1, 2024 17:35

narqo requested review from wilfriedroset, vaxvms, bubu11e and jdbaldry as code owners March 1, 2024 17:35

narqo commented Mar 1, 2024

View reviewed changes

Makefile Show resolved Hide resolved

pracucci reviewed Mar 7, 2024

View reviewed changes

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

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

pkg/ingester/errors.go Outdated Show resolved Hide resolved

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

narqo requested review from grafanabot and a team as code owners March 11, 2024 14:58

narqo force-pushed the ingest-store-dont-push-api branch from a61556f to e6498a0 Compare March 11, 2024 15:01

narqo force-pushed the ingest-store-dont-push-api branch 5 times, most recently from 6e2599e to 41b691d Compare March 12, 2024 07:43

narqo commented Mar 12, 2024

View reviewed changes

narqo requested a review from pstibrany March 12, 2024 08:24

pstibrany reviewed Mar 12, 2024

View reviewed changes

narqo and others added 2 commits March 12, 2024 21:18

ingest: add flag to disable push API

a3f4e38

Signed-off-by: Vladimir Varankin <[email protected]> Co-authored-by: Marco Pracucci <[email protected]>

make: build config-inspector with same version of Go as mimir

0cb6dcc

Signed-off-by: Vladimir Varankin <[email protected]>

narqo added 5 commits March 12, 2024 21:18

re-build generated files

f29fa1f

Signed-off-by: Vladimir Varankin <[email protected]>

address review comments

f963101

Signed-off-by: Vladimir Varankin <[email protected]>

re-build generated files

8a4bbc0

Signed-off-by: Vladimir Varankin <[email protected]>

ingester: revert non-relevant changes

b547fc2

Signed-off-by: Vladimir Varankin <[email protected]>

ingester: rename config to PushGrpcMethodEnabled

f4e24c5

Signed-off-by: Vladimir Varankin <[email protected]>

narqo force-pushed the ingest-store-dont-push-api branch from 41b691d to f4e24c5 Compare March 12, 2024 20:37

Apply suggestions from code review

52bcfd7

Co-authored-by: Peter Štibraný <[email protected]> Signed-off-by: Vladimir Varankin <[email protected]>

narqo force-pushed the ingest-store-dont-push-api branch from bce5c21 to 52bcfd7 Compare March 12, 2024 21:01

narqo requested a review from pstibrany March 12, 2024 21:44

pstibrany approved these changes Mar 13, 2024

View reviewed changes

pstibrany merged commit 08a2bd2 into main Mar 13, 2024
29 checks passed

pstibrany deleted the ingest-store-dont-push-api branch March 13, 2024 09:33

narqo mentioned this pull request Mar 13, 2024

distributor, ruler: Expect METHOD_NOT_ALLOWED from pusher #7618

Closed

4 tasks

narqo mentioned this pull request Mar 13, 2024

ingester: Enforce limits for Push requests coming via ingest.Store #7621

Merged

4 tasks

pracucci reviewed Mar 18, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

ingester: Support for disallowing Push API for ingest storage #7503

ingester: Support for disallowing Push API for ingest storage #7503

narqo commented Feb 29, 2024 •

edited

Loading

pracucci left a comment

github-actions bot commented Mar 11, 2024

narqo Mar 12, 2024

pstibrany left a comment

pstibrany Mar 12, 2024

pstibrany Mar 12, 2024

narqo commented Mar 12, 2024

pstibrany left a comment

pracucci Mar 18, 2024

pracucci Mar 18, 2024

ingester: Support for disallowing Push API for ingest storage #7503

ingester: Support for disallowing Push API for ingest storage #7503

Conversation

narqo commented Feb 29, 2024 • edited Loading

What this PR does

Which issue(s) this PR fixes or relates to

Checklist

pracucci left a comment

Choose a reason for hiding this comment

github-actions bot commented Mar 11, 2024

narqo Mar 12, 2024

Choose a reason for hiding this comment

pstibrany left a comment

Choose a reason for hiding this comment

pstibrany Mar 12, 2024

Choose a reason for hiding this comment

pstibrany Mar 12, 2024

Choose a reason for hiding this comment

narqo commented Mar 12, 2024

pstibrany left a comment

Choose a reason for hiding this comment

pracucci Mar 18, 2024

Choose a reason for hiding this comment

pracucci Mar 18, 2024

Choose a reason for hiding this comment

narqo commented Feb 29, 2024 •

edited

Loading