openapi(dm): add openapi conifg in dm-master config file #3836

Ehco1996 · 2021-12-10T09:03:41Z

What problem does this PR solve?

Issue Number: ref #3583

What is changed and how it works?

add new config part openapi for dm-master, default value is false
depreated openapi in experimental config

Check List

Tests

Unit test
Integration test

Code changes

Has exported function/method change

Side effects

Possible performance regression
Increased code complexity
Breaking backward compatibility

Related changes

Need to update the documentation

Release note

`turn on the openapi for DM by default`.

ti-chi-bot · 2021-12-10T09:03:42Z

[REVIEW NOTIFICATION]

This pull request has been approved by:

GMHDBJD
okJiang

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.

Ehco1996 · 2021-12-10T09:26:47Z

/run-dm-integration-tests

lance6716 · 2021-12-12T07:16:19Z

not sure we plan to turn on it by default or remove it from experimental features.

Ehco1996 · 2021-12-13T00:54:51Z

not sure we plan to turn on it by default or remove it from experimental features.

ptal @sunzhaoyang

sunzhaoyang · 2021-12-21T03:04:28Z

not sure we plan to turn on it by default or remove it from experimental features.

ptal @sunzhaoyang

I prefer to remove from experimental features and turn off by default (for security), if dmctl depend on openapi in the future, perhaps it should be implemented with api authentication. @Ehco1996 @lance6716

ljun0712 · 2021-12-21T03:13:13Z

基于安全考虑，没有认证鉴权的openapi默认不要打开，后续实现了api的认证鉴权后再打开会比较合适

Ehco1996 · 2021-12-21T03:16:15Z

not sure we plan to turn on it by default or remove it from experimental features.

ptal @sunzhaoyang

I prefer to remove from experimental features and turn off by default (for security), if dmctl depend on openapi in the future, perhaps it should be implemented with api authentication. @Ehco1996 @lance6716

so we need add a new config like openapi=true in dm-master's conig file ?

sunzhaoyang · 2021-12-21T03:50:28Z

Again, I considered the necessity and safety.
It might be better to keep the configuration item under [experimental] and default openapi = false, maybe we can change it to default true when the authentication is implemented.

@lance6716 Any better advice?

Ehco1996 · 2021-12-21T05:38:21Z

/hold

we are waiting for tidb to come up with a solution to the security issue.

Ehco1996 · 2021-12-28T02:44:13Z

/unhold

update final solution in PR description

Ehco1996 · 2021-12-28T02:45:01Z

/run-dm-integration-tests

Ehco1996 · 2021-12-28T03:02:18Z

/run-dm-integration-tests

GMHDBJD · 2021-12-28T05:30:50Z

dm/dm/master/config.go

@@ -313,6 +314,10 @@ func (c *Config) adjust() error {
 		c.QuotaBackendBytes = quotaBackendBytesLowerBound
 	}

+	if c.ExperimentalFeatures.OpenAPI {
+		log.L().Warn("openapi is a GA feature and removed from experimental features, so this configuration will have no affect, please set openapi=true in dm-master config file")


Should enable OpenAPI if it is upgraded from an old cluster

but how should i do this in code ? do i need to update dm-master's config file for user ? 🙄

or what if user set

openapi = false [experimental] openapi = true

what is the expected heavior when enable openapi in v5.3 but forget to add new added config in dm-master after upgrade ? @sunzhaoyang ptal

after talked with pm, both cfg.OpenAPI and s.cfg.ExperimentalFeatures.OpenAPI will take effects when user want to enable openapi, added in 4c20104

ptal @GMHDBJD

what is the expected heavior when enable openapi in v5.3 but forget to add new added config in dm-master after upgrade ? @sunzhaoyang ptal

Disabled. He has to enable manually.

GMHDBJD · 2021-12-28T06:52:32Z

dm/tests/dmctl_basic/conf/get_master1.toml

@@ -20,6 +20,7 @@ max-request-bytes = 1572864
 auto-compaction-mode = "periodic"
 auto-compaction-retention = "1h"
 quota-backend-bytes = 2147483648
+openapi = false


Can we omit the below lines when by get-config at L29? looks so confused.

[experimental] openapi = false

GMHDBJD · 2021-12-28T09:26:32Z

dm/dm/master/config.go

@@ -128,6 +128,7 @@ type Config struct {
 	AutoCompactionMode      string `toml:"auto-compaction-mode" json:"auto-compaction-mode"`
 	AutoCompactionRetention string `toml:"auto-compaction-retention" json:"auto-compaction-retention"`
 	QuotaBackendBytes       int64  `toml:"quota-backend-bytes" json:"quota-backend-bytes"`
+	OpenAPI                 bool   `toml:"openapi"`


GMHDBJD · 2021-12-28T09:27:54Z

dm/dm/master/config.go

@@ -91,7 +91,7 @@ func NewConfig() *Config {
 }


--openapi for command line config?

now i think config file is enough becasue we will enable OpenAPI as default in the feature

I don't think so.

ok added in 0463672

GMHDBJD · 2021-12-28T09:29:53Z

dm/tests/dmctl_basic/conf/get_master1.toml

 v1-sources-path = ""
 ssl-ca = ""
 ssl-cert = ""
 ssl-key = ""

 [experimental]
-  openapi = false


if experimental.openapi==true, still print it? Why not set it to false when adjust?

yes we should adjust this value , added in 0964771

GMHDBJD

rest LGTM

GMHDBJD · 2021-12-29T05:34:12Z

dm/dm/master/dm-master.toml

 # some experimental features
 [experimental]
 openapi = false


This file used for --print-sample-config, so I prefer to remove these lines.

GMHDBJD

LGTM

Ehco1996 · 2021-12-30T03:19:16Z

/merge

ti-chi-bot · 2021-12-30T03:19:19Z

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

Commit hash: b469dbf

ti-chi-bot · 2021-12-30T03:19:32Z

@Ehco1996: Your PR was out of date, I have automatically updated it for you.

At the same time I will also trigger all tests for you:

/run-all-tests

If the CI test fails, you just re-trigger the test that failed and the bot will merge the PR for you after the CI passes.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the ti-community-infra/tichi repository.

Ehco1996 · 2021-12-30T03:44:25Z

/run-leak-test
/run-verify

ti-chi-bot added release-note Denotes a PR that will be considered when it comes time to generate release notes. size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Dec 10, 2021

Ehco1996 mentioned this pull request Dec 10, 2021

Tracking issue for DM WebUI #3583

Closed

14 tasks

lance6716 added the area/dm Issues or PRs related to DM. label Dec 13, 2021

ti-chi-bot added needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. and removed needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. labels Dec 15, 2021

add openapi config in dm-master's config

43acd76

ti-chi-bot added size/S Denotes a PR that changes 10-29 lines, ignoring generated files. and removed needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. labels Dec 28, 2021

ti-chi-bot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Dec 28, 2021

Ehco1996 changed the title ~~openapi(dm): turn on the openapi for DM by default~~ openapi(dm): add openapi conifg in dm-master config file Dec 28, 2021

fix test

ea5cc00

fix again

53273c6

okJiang approved these changes Dec 28, 2021

View reviewed changes

ti-chi-bot added the status/LGT1 Indicates that a PR has LGTM 1. label Dec 28, 2021

GMHDBJD reviewed Dec 28, 2021

View reviewed changes

address commnet

4c20104

GMHDBJD reviewed Dec 28, 2021

View reviewed changes

Ehco1996 added this to the v5.4.0 milestone Dec 28, 2021

ti-chi-bot added size/M Denotes a PR that changes 30-99 lines, ignoring generated files. and removed size/S Denotes a PR that changes 10-29 lines, ignoring generated files. labels Dec 28, 2021

address comment

da3b8dd

ti-chi-bot added size/S Denotes a PR that changes 10-29 lines, ignoring generated files. and removed size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Dec 28, 2021

remove not used comment

23faf9c

GMHDBJD reviewed Dec 28, 2021

View reviewed changes

address comment

0964771

ti-chi-bot added size/M Denotes a PR that changes 30-99 lines, ignoring generated files. and removed size/S Denotes a PR that changes 10-29 lines, ignoring generated files. labels Dec 28, 2021

add ctl flags

0463672

GMHDBJD reviewed Dec 29, 2021

View reviewed changes

address comment

b469dbf

GMHDBJD approved these changes Dec 30, 2021

View reviewed changes

ti-chi-bot added status/LGT2 Indicates that a PR has LGTM 2. and removed status/LGT1 Indicates that a PR has LGTM 1. labels Dec 30, 2021

ti-chi-bot added the status/can-merge Indicates a PR has been approved by a committer. label Dec 30, 2021

Merge branch 'master' into oepenapi-cfg

f24db80

ti-chi-bot merged commit 1d376cd into pingcap:master Dec 30, 2021

Ehco1996 mentioned this pull request Jan 4, 2022

dm: update openapi config field pingcap/docs-cn#8107

Merged

15 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

openapi(dm): add openapi conifg in dm-master config file #3836

openapi(dm): add openapi conifg in dm-master config file #3836

Ehco1996 commented Dec 10, 2021 •

edited

Loading

ti-chi-bot commented Dec 10, 2021 •

edited

Loading

Ehco1996 commented Dec 10, 2021

lance6716 commented Dec 12, 2021

Ehco1996 commented Dec 13, 2021

sunzhaoyang commented Dec 21, 2021

ljun0712 commented Dec 21, 2021

Ehco1996 commented Dec 21, 2021

sunzhaoyang commented Dec 21, 2021

Ehco1996 commented Dec 21, 2021

Ehco1996 commented Dec 28, 2021

Ehco1996 commented Dec 28, 2021

Ehco1996 commented Dec 28, 2021

GMHDBJD Dec 28, 2021

Ehco1996 Dec 28, 2021

Ehco1996 Dec 28, 2021

Ehco1996 Dec 28, 2021

sunzhaoyang Dec 29, 2021

GMHDBJD Dec 28, 2021 •

edited

Loading

GMHDBJD Dec 28, 2021

GMHDBJD Dec 28, 2021

Ehco1996 Dec 28, 2021

GMHDBJD Dec 28, 2021

Ehco1996 Dec 29, 2021

GMHDBJD Dec 28, 2021

Ehco1996 Dec 28, 2021

GMHDBJD left a comment

GMHDBJD Dec 29, 2021

GMHDBJD left a comment

Ehco1996 commented Dec 30, 2021

ti-chi-bot commented Dec 30, 2021

ti-chi-bot commented Dec 30, 2021

Ehco1996 commented Dec 30, 2021

openapi(dm): add openapi conifg in dm-master config file #3836

openapi(dm): add openapi conifg in dm-master config file #3836

Conversation

Ehco1996 commented Dec 10, 2021 • edited Loading

What problem does this PR solve?

What is changed and how it works?

Check List

Release note

ti-chi-bot commented Dec 10, 2021 • edited Loading

Ehco1996 commented Dec 10, 2021

lance6716 commented Dec 12, 2021

Ehco1996 commented Dec 13, 2021

sunzhaoyang commented Dec 21, 2021

ljun0712 commented Dec 21, 2021

Ehco1996 commented Dec 21, 2021

sunzhaoyang commented Dec 21, 2021

Ehco1996 commented Dec 21, 2021

Ehco1996 commented Dec 28, 2021

Ehco1996 commented Dec 28, 2021

Ehco1996 commented Dec 28, 2021

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

GMHDBJD Dec 28, 2021 • 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

GMHDBJD left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

GMHDBJD left a comment

Choose a reason for hiding this comment

Ehco1996 commented Dec 30, 2021

ti-chi-bot commented Dec 30, 2021

ti-chi-bot commented Dec 30, 2021

Ehco1996 commented Dec 30, 2021

Ehco1996 commented Dec 10, 2021 •

edited

Loading

ti-chi-bot commented Dec 10, 2021 •

edited

Loading

GMHDBJD Dec 28, 2021 •

edited

Loading