Refactor feature flag to support more modules #1057

shhdgit · 2021-11-11T03:27:01Z

ti-chi-bot · 2021-11-11T03:27:02Z

[REVIEW NOTIFICATION]

This pull request has been approved by:

baurine

To complete the pull request process, please ask the reviewers in the list to review by filling /cc @reviewer in the comment.
After your PR has acquired the required number of LGTMs, you can assign this pull request to the committer in the list by filling /assign @committer in the comment to help you merge this pull request.

The full list of commands accepted by this bot can be found here.

Reviewer can indicate their review by submitting an approval review.
Reviewer can cancel approval by submitting a request changes review.

shhdgit · 2021-11-11T03:30:43Z

/cc @baurine @breeswish
PTAL, thanks~

pkg/apiserver/conprof/service.go

pkg/utils/feature_flag.go

pkg/apiserver/user/module.go

shhdgit · 2021-11-12T02:34:08Z

@breeswish
Updated, PTAL.

pkg/apiserver/info/info.go

util/versionutil/version.go

util/versionutil/feature_flag.go

pkg/apiserver/utils/mw_feature_flag.go

pkg/apiserver/conprof/service.go

baurine · 2021-11-15T02:06:04Z

pkg/config/config.go

@@ -40,7 +42,7 @@ func Default() *Config {
 		TiDBTLSConfig:      nil,
 		EnableTelemetry:    true,
 		EnableExperimental: false,
-		FeatureVersion:     "",
+		FeatureVersion:     version.PDVersion,


It's a nice improvement.

baurine

LGTM!

breezewish

the rest lgtm

util/featureflag/registry.go

pkg/apiserver/conprof/service.go

util/featureflag/featureflag.go

pkg/apiserver/info/info.go

pkg/apiserver/apiserver.go

util/featureflag/version_guard.go

breezewish · 2021-12-07T15:02:26Z

pkg/apiserver/user/auth.go

@@ -88,8 +91,9 @@ func NewAuthService() *AuthService {
 	}

 	service := &AuthService{
-		middleware:     nil,
-		authenticators: map[utils.AuthType]Authenticator{},
+		FeatureFlagNonRootLogin: featureFlagRegistry.Register("nonRootLogin", ">= 5.3.0"),


Seems that this FeatureFlagNonRootLogin is not used. It doesn't look good as this may imply that this protection is purely happening at frontend and there is no backend protection for corresponding features.

Create a new PR #1089 to refactor the "non root login" for better review experience.

breezewish · 2021-12-07T15:04:27Z

pkg/apiserver/user/auth.go

@@ -30,6 +31,8 @@ var (
 )

 type AuthService struct {
+	FeatureFlagNonRootLogin *featureflag.FeatureFlag


This feature should be placed inside SQL auth, as only SQL auth needs it and other kind of auth does not care about the "non root login".

go.sum

pkg/apiserver/apiserver.go

breezewish · 2021-12-08T06:32:17Z

util/featureflag/featureflag.go

+			_ = c.Error(ErrFeatureUnsupported.New(f.name))
+			c.AbortWithStatus(http.StatusForbidden)


Seems that there is an "Abort". I'm worrying whether this is compatible with the standard rest.ErrorHandlerFn. Could you add a test to verify it?

breezewish · 2021-12-08T06:37:00Z

util/featureflag/featureflag.go

+	"github.com/pingcap/tidb-dashboard/util/rest"
+)
+
+var ErrFeatureUnsupported = rest.ErrForbidden.NewSubtype("feature_unsupported")


Oh. I'm sorry, I misled you. There is no way to attach the error status code into the error type for now.

Please use HTTPCodeProperty when initializing errors:

var ErrFeatureUnsupported = ErrNS.xxxx() _ = c.Error(ErrFeatureUnsupported.NewWithNoMessage().WithProperty(HTTPCodeProperty(http.StatusForbidden)))

or:

_ = c.Error(rest.ErrForbidden.New("<ErrorMessages>"))

The whole point is to avoid the necessarilty to call c.Status() or c.AbortWithStatus() manually.

…oard into refactor/feature-flag

breezewish · 2021-12-08T10:34:05Z

/merge

ti-chi-bot · 2021-12-08T10:34:07Z

This pull request has been accepted and is ready to merge.

Commit hash: 0623ecc

breezewish · 2021-12-08T10:36:25Z

util/featureflag/featureflag_test.go

 	e2.Use(func(c *gin.Context) {
 		c.Next()


You can use CreateTestContext to test context directly. No need to use a middleware to test it. Example:

func TestDownload(t *testing.T) { handler := New() token := handler.mustGetDownloadToken(t, "foobar", "file.txt", time.Second*5) r := httptest.NewRecorder() c, _ := gin.CreateTestContext(r) c.Request, _ = http.NewRequest(http.MethodGet, "/download?token="+token, nil) handler.HandleDownloadRequest(c) require.Len(t, c.Errors, 0) require.Equal(t, http.StatusOK, r.Code) require.Equal(t, `attachment; filename="file.txt"`, r.Header().Get("Content-Disposition")) require.Equal(t, `application/octet-stream`, r.Header().Get("Content-Type")) require.Equal(t, "foobar", r.Body.String()) // Download again r = httptest.NewRecorder() c, _ = gin.CreateTestContext(r) c.Request, _ = http.NewRequest(http.MethodGet, "/download?token="+token, nil) handler.HandleDownloadRequest(c) require.Len(t, c.Errors, 1) require.True(t, errorx.IsOfType(c.Errors[0].Err, rest.ErrBadRequest)) require.Contains(t, c.Errors[0].Error(), "Download file not found") }

Thanks! This is really helpful.

breezewish · 2021-12-08T10:38:48Z

util/featureflag/featureflag_test.go

+	req2, _ := http.NewRequest("GET", "/ping", nil)
+	e.ServeHTTP(w2, req2)
+
+	r.Equal(http.StatusForbidden, w2.Code)


Forbidden means Unauthorized: The HTTP 403 Forbidden response status code indicates that the server understands the request but refuses to authorize it.

However there is no way for this router to be authorized. So maybe BadRequest is better.

Oh, yes. I misunderstood the "forbidden".

* Revert "Release v2021.12.06.1 (#1084)" This reverts commit bcc43a0. * Compitable with different TiDB versions for conprof and non-root-login features (#1047) * make conprof independent * check feature enable * add check feature enable middleware * hide menu if feature is not enabled * refactor non root login switch by new design * i18n * yarn fmt * renaming * adjust fe code * refine * remove unused log * build(deps): bump ws from 5.2.2 to 5.2.3 in /ui (#1055) * CICD: Update the release pipeline for recent PD format policies (#1054) * fix i18n wording (#1056) * Refactor: Change util module to util package (#1052) * Refactor: Fix godot incorrectly add dot suffix to annotations (#1059) * lint: Add goheader for copyright lints (#1062) * Refactor: Migrate to use the `rest` package in util/ (#1060) * fix(*): globally delete/update data by GORM (#1065) * ui: bump dependencies (#1066) * refactor: Switch to use ziputil, netutil, reflectutil and fileswap (#1067) * Fix request header being pinned after pd profiling (#1069) * Integrate speedscope (#1064) * fix potential panic when GetPDInstances (#1075) Signed-off-by: crazycs <[email protected]> * Refactor: a new httpclient (#1073) * Refactor: Switch to use util/distro in all places (#1078) * chore: support import relative file URL (#1082) * Refactor: Move tools into a standalone module (#1079) * Fix script to embed the ui (#1088) * Fix script to embed the ui * Hack write_strings * Refactor feature flag to support more modules (#1057) * Drop sysutil dependency (#1093) * chore: add graph generation (#1085) * Refactor: Add TopologyProvider (#1098) * esbuild: i18n + dep (#1101) * script: Add a script to generate version matrix (#1104) * distro: support dynamic config (#1094) * chore: support multiple profiling types (#1095) * fix(distro): check distro_strings.json fmt by prettier (#1106) * script: fix generate assets (#1107) * Add integration test (#1083) * debug_api: Switch to use the new util (#1103) * refactor(ui): auto refresh button (#1105) * refactor(ui): auto refresh button * chore: update translation * fix: remain seconds * refine: refresh button * fix: onRefresh * fix: auto refresh * fix: continue tick * chore: add some comments * tweak: remaining refresh seconds * chore: clean code Co-authored-by: Wenxuan <[email protected]> * ui: refine conprof (#1102) * update wording * not check prom any more * replace time range component * i18n * support view profile by diffrent ways * extract ActionsButton * change download data format * refine * comments * Revert "comments" This reverts commit 3b03fdb. * fix view cpu profile fail * update state * hide action button if disable * address feedback * update release-version * sync with master Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com> Co-authored-by: Wenxuan <[email protected]> Co-authored-by: Suhaha <[email protected]> Co-authored-by: Yini Xu <[email protected]> Co-authored-by: crazycs <[email protected]>

ti-chi-bot requested review from baurine and breezewish November 11, 2021 03:30

refactor(*): feature flag

fd7dd71

shhdgit force-pushed the refactor/feature-flag branch from 1794cbe to fd7dd71 Compare November 11, 2021 03:31

breezewish reviewed Nov 11, 2021

View reviewed changes

pkg/apiserver/conprof/service.go Outdated Show resolved Hide resolved

breezewish reviewed Nov 11, 2021

View reviewed changes

shhdgit force-pushed the refactor/feature-flag branch from 58edb58 to 5d2db5d Compare November 11, 2021 04:40

chore: refine feature flag

8bd2e44

shhdgit force-pushed the refactor/feature-flag branch from 5d2db5d to 8bd2e44 Compare November 11, 2021 04:41

shhdgit requested a review from breezewish November 11, 2021 04:42

refine: feature flag middleware

74a0937

shhdgit force-pushed the refactor/feature-flag branch from 6e1f58d to 74a0937 Compare November 11, 2021 05:03

shhdgit added 3 commits November 12, 2021 09:45

Merge branch 'master' into refactor/feature-flag

178a377

refactor: move version utils to util package

f6ec213

chore: make lint happy

beafba3

breezewish reviewed Nov 12, 2021

View reviewed changes

pkg/apiserver/info/info.go Outdated Show resolved Hide resolved

breezewish reviewed Nov 12, 2021

View reviewed changes

ti-chi-bot added the needs-rebase label Nov 12, 2021

Merge branch 'master' into refactor/feature-flag

0b4dd21

ti-chi-bot removed the needs-rebase label Nov 14, 2021

shhdgit force-pushed the refactor/feature-flag branch from cfa75dc to 31f4aa4 Compare November 14, 2021 17:07

refine(feature): flag & manager

4857960

shhdgit force-pushed the refactor/feature-flag branch from 31f4aa4 to 4857960 Compare November 14, 2021 17:13

test(feature_flag): add manager tests

8c9c89c

baurine reviewed Nov 15, 2021

View reviewed changes

baurine approved these changes Nov 15, 2021

View reviewed changes

*: update feature flag

43b297d

shhdgit force-pushed the refactor/feature-flag branch from c5f06d5 to 43b297d Compare December 7, 2021 07:23

shhdgit added 4 commits December 7, 2021 16:06

chore: make lint happy

cc55adc

chore: revert lint

15490d5

Update index.tsx

3401a7f

Update Detail.tsx

c7e50e0

breezewish reviewed Dec 7, 2021

View reviewed changes

refine: feature flag

c4e1a03

breezewish reviewed Dec 8, 2021

View reviewed changes

go.sum Outdated Show resolved Hide resolved

breezewish reviewed Dec 8, 2021

View reviewed changes

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

fix: fx & indirect x/tools version

94e5e95

shhdgit mentioned this pull request Dec 8, 2021

Refactor auth feature flag #1089

Closed

Merge branch 'master' into refactor/feature-flag

194a4ed

breezewish reviewed Dec 8, 2021

View reviewed changes

shhdgit added 2 commits December 8, 2021 15:41

refine: version guard error

8087baf

Merge branch 'refactor/feature-flag' of github.com:shhdgit/tidb-dashb…

61a7d73

…oard into refactor/feature-flag

shhdgit force-pushed the refactor/feature-flag branch 2 times, most recently from 8cfd8e1 to c6c075a Compare December 8, 2021 08:43

test: adjust feature flag version guard test

0623ecc

shhdgit force-pushed the refactor/feature-flag branch from c6c075a to 0623ecc Compare December 8, 2021 08:43

breezewish approved these changes Dec 8, 2021

View reviewed changes

ti-chi-bot added the status/can-merge label Dec 8, 2021

ti-chi-bot merged commit c32eb49 into pingcap:master Dec 8, 2021

breezewish reviewed Dec 8, 2021

View reviewed changes

baurine pushed a commit to baurine/tidb-dashboard that referenced this pull request Dec 30, 2021

Refactor feature flag to support more modules (pingcap#1057)

b858495

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Refactor feature flag to support more modules #1057

Refactor feature flag to support more modules #1057

shhdgit commented Nov 11, 2021 •

edited

Loading

ti-chi-bot commented Nov 11, 2021 •

edited

Loading

shhdgit commented Nov 11, 2021

shhdgit commented Nov 12, 2021

baurine Nov 15, 2021

baurine left a comment

breezewish left a comment

breezewish Dec 7, 2021

shhdgit Dec 8, 2021

breezewish Dec 7, 2021

breezewish Dec 8, 2021 •

edited

Loading

breezewish Dec 8, 2021 •

edited

Loading

breezewish commented Dec 8, 2021

ti-chi-bot commented Dec 8, 2021

breezewish Dec 8, 2021

shhdgit Dec 9, 2021

breezewish Dec 8, 2021

shhdgit Dec 9, 2021 •

edited

Loading

		_ = c.Error(ErrFeatureUnsupported.New(f.name))
		c.AbortWithStatus(http.StatusForbidden)

Refactor feature flag to support more modules #1057

Refactor feature flag to support more modules #1057

Conversation

shhdgit commented Nov 11, 2021 • edited Loading

ti-chi-bot commented Nov 11, 2021 • edited Loading

shhdgit commented Nov 11, 2021

shhdgit commented Nov 12, 2021

baurine Nov 15, 2021

Choose a reason for hiding this comment

baurine left a comment

Choose a reason for hiding this comment

breezewish left a comment

Choose a reason for hiding this comment

breezewish Dec 7, 2021

Choose a reason for hiding this comment

shhdgit Dec 8, 2021

Choose a reason for hiding this comment

breezewish Dec 7, 2021

Choose a reason for hiding this comment

breezewish Dec 8, 2021 • edited Loading

Choose a reason for hiding this comment

breezewish Dec 8, 2021 • edited Loading

Choose a reason for hiding this comment

breezewish commented Dec 8, 2021

ti-chi-bot commented Dec 8, 2021

breezewish Dec 8, 2021

Choose a reason for hiding this comment

shhdgit Dec 9, 2021

Choose a reason for hiding this comment

breezewish Dec 8, 2021

Choose a reason for hiding this comment

shhdgit Dec 9, 2021 • edited Loading

Choose a reason for hiding this comment

shhdgit commented Nov 11, 2021 •

edited

Loading

ti-chi-bot commented Nov 11, 2021 •

edited

Loading

breezewish Dec 8, 2021 •

edited

Loading

breezewish Dec 8, 2021 •

edited

Loading

shhdgit Dec 9, 2021 •

edited

Loading