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.

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

Add --disable-admin-operations flag in Compactor UI and Bucket UI #6604

Closed
wants to merge 33 commits into from

Conversation

harsh-ps-2003
Copy link
Contributor

@harsh-ps-2003 harsh-ps-2003 commented Aug 10, 2023

Signed-off-by: Harsh Pratap Singh [email protected]
Fixes #5390

  • I added CHANGELOG entry for this change.
  • Change is not relevant to the end user.

Changes

This PR adds a flag, named --disable-admin-operations, to the Compactor UI and Bucket Web UI to decide whether to disable the corresponding buttons Mark Deletion and Mark No Compaction in the UI as they are admin-level operations. It also restricts the API usage for both of them.

Verification

It was tested locally.
The UI test:
Screenshot 2023-08-10 at 9 09 08 PM
Screenshot 2023-08-10 at 9 09 20 PM
Screenshot 2023-08-10 at 9 08 42 PM
Screenshot 2023-08-10 at 9 08 52 PM

The API test:

❯ curl -X POST -d "id=_ulid&action=DELETION" http://localhost:8080/api/v1/blocks/mark
{"status":"error","errorType":"bad_data","error":"Admin operations are disabled"}
❯ curl -X POST -d "id=_ulid&action=DELETION" http://localhost:10922/api/v1/blocks/mark
{"status":"error","errorType":"bad_data","error":"Admin operations are disabled"}

cc @yeya24 @GiedriusS @saswatamcode

Signed-off-by: Harsh Pratap Singh <[email protected]>
@harsh-ps-2003 harsh-ps-2003 changed the title adding flags Add --disable-admin-operations flag in Compactor UI and Bucket UI Aug 10, 2023
@harsh-ps-2003
Copy link
Contributor Author

harsh-ps-2003 commented Aug 10, 2023

If everything goes okay with this PR, I will add the changelog. Sorry for messing up a lot of times and thanks for the patience. Also how to fix this Docs CI failing!? @saswatamcode @yeya24

@saswatamcode
Copy link
Member

@harsh-ps-2003 you don't need to raise a new PR every time after review. you can push commits to address feedback. 🙂
Also, you can run make doc and commit to fix docs CI

saswatamcode
saswatamcode previously approved these changes Aug 11, 2023
Copy link
Member

@saswatamcode saswatamcode left a comment

Choose a reason for hiding this comment

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

Thanks!
Generally LGTM mod some small nits. 🙂

Let's try to address comments here instead of opening new PR!

@@ -786,4 +787,6 @@ func (cc *compactConfig) registerFlag(cmd extkingpin.FlagClause) {
cc.webConf.registerFlag(cmd)

cmd.Flag("bucket-web-label", "External block label to use as group title in the bucket web UI").StringVar(&cc.label)

cmd.Flag("disable-admin-operations", "Restrict access to Block Mark deletion and no compaction").Default("false").BoolVar(&cc.disableAdminOperations)
Copy link
Member

Choose a reason for hiding this comment

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

Let's make description a bit clearer.

Suggested change
cmd.Flag("disable-admin-operations", "Restrict access to Block Mark deletion and no compaction").Default("false").BoolVar(&cc.disableAdminOperations)
cmd.Flag("disable-admin-operations", "Disable UI/API admin operations like marking blocks for deletion and no compaction.").Default("false").BoolVar(&cc.disableAdminOperations)

@@ -203,6 +204,8 @@ func (tbc *bucketWebConfig) registerBucketWebFlag(cmd extkingpin.FlagClause) *bu
cmd.Flag("timeout", "Timeout to download metadata from remote storage").Default("5m").DurationVar(&tbc.timeout)

cmd.Flag("label", "External block label to use as group title").StringVar(&tbc.label)

cmd.Flag("disable-admin-operations", "Restrict access to Block Mark deletion and no compaction").Default("false").BoolVar(&tbc.disableAdminOperations)
Copy link
Member

Choose a reason for hiding this comment

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

ditto

Suggested change
cmd.Flag("disable-admin-operations", "Restrict access to Block Mark deletion and no compaction").Default("false").BoolVar(&tbc.disableAdminOperations)
cmd.Flag("disable-admin-operations", "Disable UI/API admin operations like marking blocks for deletion and no compaction.").Default("false").BoolVar(&tbc.disableAdminOperations)

Comment on lines 119 to 120
)}
{!disableAdminOperations && (
Copy link
Member

Choose a reason for hiding this comment

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

We can probably render both buttons, under a single conditional?

Comment on lines 45 to 46
const { response: flagsRes } = useFetch<FlagMap>(`/api/v1/status/flags`);
const disableAdminOperations = flagsRes?.data?.['disable-admin-operations'] || false;
Copy link
Member

Choose a reason for hiding this comment

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

Let's actually call this in BlocksContent and pass it to BlocksDetails as a prop, that way we won't be calling this every time we open a block detail page. This also won't change from block to block.

@harsh-ps-2003
Copy link
Contributor Author

Thanks for the review and sorry for the closed PRs @saswatamcode

@harsh-ps-2003
Copy link
Contributor Author

harsh-ps-2003 commented Aug 12, 2023

@saswatamcode @yeya24 @GiedriusS when I did make docs, it gave me an error

>> cleaning up white noise
sed: 1: "./CODE_OF_CONDUCT.md": invalid command code .
make[1]: *** [white-noise-cleanup] Error 1
make: *** [docs] Error 2

as I am on Mac OS. The invalid sed command is happening as on macOS; the sed command requires an empty string argument after the -i option when doing in-place editing, while on Linux, this argument is optional. I have locally made changes to the cleanup-white-noise script to work on both Mac and Linux systems. And this was the reason the Docs CI was failing.
After fixing this when I do make docs I don't get any errors, but when I do make check-docs I get the errors! How should I go about it? make docs is not able to fix these error!! Sorry!

GiedriusS and others added 5 commits August 22, 2023 20:21
Document the why/what from
thanos-io#6257 in the Querier
documentation.

Signed-off-by: Giedrius Statkevičius <[email protected]>
From the Go docs:

  "If the map is nil, the number of iterations is 0." [1]

Therefore, an additional nil check for before the loop is unnecessary.

[1]: https://go.dev/ref/spec#For_range

Signed-off-by: Eng Zer Jun <[email protected]>
Signed-off-by: Harsh Pratap Singh <[email protected]>

Signed-off-by: Harsh Pratap Singh <[email protected]>
Signed-off-by: Harsh Pratap Singh <[email protected]>
harsh-ps-2003 and others added 24 commits August 22, 2023 20:21
Signed-off-by: Harsh Pratap Singh <[email protected]>
Signed-off-by: Harsh Pratap Singh <[email protected]>
Trying to build on go1.21 fails with a scary error about the GC moving
heap objects. This is due to a conservative assumption around future go
releases in go4.org/unsafe/assume-no-moving-gc

A more recent version actually checks with the go runtime, see:

https://go-review.googlesource.com/c/go/+/498121

Signed-off-by: Jon Johnson <[email protected]>
Signed-off-by: GitHub <[email protected]>
Co-authored-by: fpetkovski <[email protected]>
Chunk reader needs to wait until the chunk loading ends in Close()
because otherwise there will be a race between appending to r.chunkBytes
and reading from it.

Signed-off-by: Giedrius Statkevičius <[email protected]>
…abel (thanos-io#6605)

* vertically shard binary expression even if no matching labels

Signed-off-by: Ben Ye <[email protected]>

* add changelog

Signed-off-by: Ben Ye <[email protected]>

* fix test

Signed-off-by: Ben Ye <[email protected]>

---------

Signed-off-by: Ben Ye <[email protected]>
…#6612)

In the case that we have pushed down queries and internal labels that
are overriden by external labels we are not flushing the sorted response.

Signed-off-by: Michael Hoffmann <[email protected]>
The remote engine implementation currently converts warnings to errors.
This prevents using partial response with the distributed engine since every
warning will cause a query to fail.

Signed-off-by: Filip Petkovski <[email protected]>
This reverts commit 3e3d301.
Signed-off-by: Harsh Pratap Singh <[email protected]>
This reverts commit 268c6be.
Signed-off-by: Harsh Pratap Singh <[email protected]>
This reverts commit 59a5f9c.
Signed-off-by: Harsh Pratap Singh <[email protected]>
Signed-off-by: Harsh Pratap Singh <[email protected]>
Signed-off-by: Harsh Pratap Singh <[email protected]>
Signed-off-by: Harsh Pratap Singh <[email protected]>
Signed-off-by: Harsh Pratap Singh <[email protected]>
@harsh-ps-2003 harsh-ps-2003 deleted the addingflags branch August 24, 2023 12:50
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Provide option to disable block mark deletion and mark no compaction from bucket UI
9 participants