Add `--disable-admin-operations` flag in Block Bucket UI #6601

harsh-ps-2003 · 2023-08-10T06:51:20Z

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 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.

Verification

It was tested locally.
The UI test:

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"}

cc @yeya24 @GiedriusS @saswatamcode

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

saswatamcode

Thanks! Some comments,

saswatamcode · 2023-08-10T06:59:51Z

pkg/api/blocks/v1.go

-		bkt:         bkt,
+		disableCORS:            disableCORS,
+		bkt:                    bkt,
+		disableAdminOperations: flagsMap["disable-admin-operations"] == "true",


Let's just pass the bool directly in NewBlockAPI, instead of using this?

actaully flagsMap map[string]string will give a string "true" with flagsMap["disable-admin-operations"]. thus I am simply converting it to boolean.

It would be cleaner and more consistent with other such options like disableCors, and we wouldn't need to check a string here at all
But actually, I think we don't even need any changes in BlocksAPI here. The flag value will just directly be used in UI anyway, so we don't need a separate API. See my other comment #6601 (comment)

okay! thanks for the review. i will look into it!

saswatamcode · 2023-08-10T07:04:10Z

pkg/api/blocks/v1.go

@@ -84,9 +86,18 @@ func (bapi *BlocksAPI) Register(r *route.Router, tracer opentracing.Tracer, logg

 	r.Get("/blocks", instr("blocks", bapi.blocks))
 	r.Post("/blocks/mark", instr("blocks_mark", bapi.markBlock))
+	r.Get("/flags", instr("flags_disableAdminOperations", bapi.disableAdminOperationsHandler))


I don't think you need to add a new API for this. We already register api/v1/status/flags as a common endpoint for all components in pkg/api

It would be cleaner and more consistent with other such options like disableCors, and we wouldn't need to check a string here at all
But actually, I think we don't even need any changes in BlocksAPI here. The flag value will just directly be used in UI anyway, so we don't need a separate API. See my other comment #6601 (comment)

@saswatamcode @GiedriusS @yeya24 one problem that I am facing when using /status/flags endpoint is that I am not able to figure out how to get the flags value inside the markBlock function of v1.go. I need it so as to restrict access to the API when the flag is enabled.

Ack! Reading the issue seems like we would need the option passed here as well. To restrict access, both via UI and API. Ignore this comment then,

But actually, I think we don't even need any changes in BlocksAPI here

thanks for the clarification!

saswatamcode · 2023-08-10T07:05:33Z

cmd/thanos/tools_bucket.go

@@ -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)


Shouldn't this flag be in Compactor (cmd/thanos/compact.go) instead? As that is the component that exposes blocks UI

I lack on a broader knowledge of codebase, but as far as I can understand, If i add the flag in compact.go then the flag will be available in Compactor UI instead of Bucket UI. I can tell this for sure that adding the flag in tools_bucket.go is only affecting Bucket UI. I think we want this flag specifically in Bucket UI not in the Compactor UI right? @yeya24 @GiedriusS @saswatamcode

Ack got confused for a bit. So Thanos users can access the Block UI via two ways I think,

tools bucket web (i.e which is covered by the flag you added)

compact (which isn't covered)

So let's add the same flag there too, as you can mark deletion and no compaction there too.

okay! i will add the same flag for Compactor as well as Bucket

saswatamcode · 2023-08-10T07:10:47Z

pkg/ui/react-app/src/thanos/pages/blocks/BlockDetails.tsx

+  const fetchDisableAdminOperations = async () => {
+    const response = await fetch('/api/v1/flags');
+    const result = await response.json();
+    setDisableAdminOperations(result.data.disableAdminOperations);
+    console.log('disableAdminOperations', result.data.disableAdminOperations);
+  };
+
+  useEffect(() => {
+    fetchDisableAdminOperations();
+  }, []);
+


Let's try to do this from /status/flags endpoint directly. Take a look at how we use it for defaultStep in PanelList : https://github.com/thanos-io/thanos/blob/main/pkg/ui/react-app/src/pages/graph/PanelList.tsx#L161

its actually very convinent. i use this in blockDetails.tsx and it worked like a charm :

const { response: flagsRes } = useFetch<FlagMap>(`/api/v1/status/flags`); const disableAdminOperations = flagsRes?.data?.['disable-admin-operations'] || false;

adding flag

f994d5c

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

pull-request-size bot added the size/L label Aug 10, 2023

saswatamcode reviewed Aug 10, 2023

View reviewed changes

harsh-ps-2003 closed this Aug 10, 2023

harsh-ps-2003 deleted the addingflag branch August 10, 2023 15:39

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add `--disable-admin-operations` flag in Block Bucket UI #6601

Add `--disable-admin-operations` flag in Block Bucket UI #6601

harsh-ps-2003 commented Aug 10, 2023 •

edited

Loading

saswatamcode left a comment

saswatamcode Aug 10, 2023

harsh-ps-2003 Aug 10, 2023

saswatamcode Aug 10, 2023

harsh-ps-2003 Aug 10, 2023

saswatamcode Aug 10, 2023

harsh-ps-2003 Aug 10, 2023 •

edited

Loading

saswatamcode Aug 10, 2023 •

edited

Loading

harsh-ps-2003 Aug 10, 2023

saswatamcode Aug 10, 2023

harsh-ps-2003 Aug 10, 2023 •

edited

Loading

saswatamcode Aug 10, 2023

harsh-ps-2003 Aug 10, 2023

saswatamcode Aug 10, 2023

harsh-ps-2003 Aug 10, 2023

Add --disable-admin-operations flag in Block Bucket UI #6601

Add --disable-admin-operations flag in Block Bucket UI #6601

Conversation

harsh-ps-2003 commented Aug 10, 2023 • edited Loading

Changes

Verification

saswatamcode left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

harsh-ps-2003 Aug 10, 2023 • edited Loading

Choose a reason for hiding this comment

saswatamcode Aug 10, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

harsh-ps-2003 Aug 10, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Add `--disable-admin-operations` flag in Block Bucket UI #6601

Add `--disable-admin-operations` flag in Block Bucket UI #6601

harsh-ps-2003 commented Aug 10, 2023 •

edited

Loading

harsh-ps-2003 Aug 10, 2023 •

edited

Loading

saswatamcode Aug 10, 2023 •

edited

Loading

harsh-ps-2003 Aug 10, 2023 •

edited

Loading