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

docs: add proposal for Security Enhanced Mode #23223

Merged
merged 13 commits into from
Mar 19, 2021

Conversation

morgo
Copy link
Contributor

@morgo morgo commented Mar 10, 2021

What problem does this PR solve?

Issue Number: Adds markdown doc for #22373

Problem Summary:

This is the design doc for security enhanced mode.

The original PR was closed because the feature was removed from Sprint 2. I have since been advised that we can add features as long as they are disabled by default, so I am reopening this. SEM is always disabled by default.

What is changed and how it works?

What's Changed:

Add a new proposal. Since it was last opened, a section for status variables has been added.

Related changes

  • PR to update pingcap/docs/pingcap/docs-cn:
  • Need to cherry-pick to the release branch

Check List

Tests

  • No code

Side effects

  • N/A

Release note

  • No release note

@ti-chi-bot ti-chi-bot added the size/L Denotes a PR that changes 100-499 lines, ignoring generated files. label Mar 10, 2021
@morgo morgo requested a review from SunRunAway March 10, 2021 01:14
@morgo morgo requested a review from bb7133 March 10, 2021 01:16
@morgo morgo changed the title proposals: add Security Enhanced Mode docs: add proposal for Security Enhanced Mode Mar 10, 2021
Copy link
Member

@zz-jason zz-jason left a comment

Choose a reason for hiding this comment

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

@pengfeiwang-cn PTAL as well.

* variable.TiDBSlowQueryFile,
* variable.TiDBSlowLogThreshold,
* variable.TiDBEnableCollectExecutionInfo,
* variable.TiDBMemoryUsageAlarmRatio,
Copy link
Contributor

Choose a reason for hiding this comment

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

I wish to talk about wait_timeout and interactive_timeout on the cloud.

  1. The TiDB's default wait_timeout is unlimited, but MySQL and Aurora MySQL's default are 8 hours.

  2. Aurora does not follow a too low wait_timeout and interactive_timeout, to avoid users' misconfiguration.

Aurora evaluates the minimum value of interactive_timeout and wait_timeout, then uses that minimum as the timeout to end all idle sessions, both interactive and noninteractive.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

.. but in Aurora's case I'm sure they let users modify these values? They just don't permit very low values (but use the semantic of ignore instead of error, which I assume is for compatibility reasons).

I think we can lower the defaults, but I don't think it needs to be grouped in with security.

Copy link
Contributor

Choose a reason for hiding this comment

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

Sorry, I can't get it. Cloud you explain why not ignore the very low value?

Copy link
Contributor Author

@morgo morgo Mar 15, 2021

Choose a reason for hiding this comment

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

I think ignoring in this context is because some connector or connection pool might set a low value, and Aurora still wants to work with it out of the box.

Unless there is a known compatibility reason (or likely reason), ignore is a risky semantic because users may think they've made a change, but it does not take effect. For example see my comment on temporary tables and read-only transactions.

So ignoring should be the exception and not the rule. In TiDB it has historically been the rule, which is problematic. My comment was about evaluating their decision here, which I think is that it looks like it might have been because of a compatibility issue.

@bb7133
Copy link
Member

bb7133 commented Mar 16, 2021

/lgtm

@ti-chi-bot ti-chi-bot added the status/LGT1 Indicates that a PR has LGTM 1. label Mar 16, 2021
Copy link
Contributor

@SunRunAway SunRunAway left a comment

Choose a reason for hiding this comment

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

/lgtm

@SunRunAway
Copy link
Contributor

/lgtm

@ti-chi-bot
Copy link
Member

[REVIEW NOTIFICATION]

This pull request has been approved by:

  • SunRunAway
  • bb7133

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 writing /lgtm in a comment.
Reviewer can cancel approval by writing /lgtm cancel in a comment.

@ti-chi-bot 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 Mar 18, 2021
@bb7133
Copy link
Member

bb7133 commented Mar 18, 2021

/merge

@ti-chi-bot
Copy link
Member

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

Commit hash: 0d89b0b

@ti-chi-bot ti-chi-bot added the status/can-merge Indicates a PR has been approved by a committer. label Mar 18, 2021
@ti-chi-bot ti-chi-bot removed the status/can-merge Indicates a PR has been approved by a committer. label Mar 18, 2021
@SunRunAway
Copy link
Contributor

/lgtm

@bb7133
Copy link
Member

bb7133 commented Mar 19, 2021

/merge

@ti-chi-bot
Copy link
Member

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

Commit hash: d6dbc27

@ti-chi-bot ti-chi-bot added the status/can-merge Indicates a PR has been approved by a committer. label Mar 19, 2021
@bb7133
Copy link
Member

bb7133 commented Mar 19, 2021

/merge

@ti-chi-bot
Copy link
Member

@morgo: 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.

@ti-chi-bot ti-chi-bot merged commit 02837f4 into pingcap:master Mar 19, 2021
SabaPing pushed a commit to SabaPing/tidb that referenced this pull request Mar 25, 2021
@morgo morgo deleted the proposal-sem branch April 13, 2021 02:14
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
size/L Denotes a PR that changes 100-499 lines, ignoring generated files. status/can-merge Indicates a PR has been approved by a committer. status/LGT2 Indicates that a PR has LGTM 2.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants