Skip to content
This repository has been archived by the owner on Dec 8, 2021. It is now read-only.

Support TLS; Reduce the need of config.toml in integration tests #270

Merged
merged 11 commits into from
Feb 27, 2020

Conversation

kennytm
Copy link
Collaborator

@kennytm kennytm commented Feb 16, 2020

What problem does this PR solve?

Fix #262.

What is changed and how it works?

  1. Code changes:

    • Added the [security] section in the config to read CA, cert and key. These are used to construct the standard *tls.Config.
    • This config is used to:
    • All of these operations are grouped into a struct common.TLS for simplified management. This struct mainly acts as an http.Client to fetch JSON objects, plus methods to produce options for securing gRPC and MySQL protocols.
  2. Test changes:

    • The entire integration test is changed to use TLS for all communications.
    • The certs need be to pass into every tidb-lightning and tidb-lightning-ctl invocations. To simplify future changes, the run_lightning and run_lightning_ctl helper scripts now define most common settings in the command line.
    • Existing config.tomls are simplified to retain only the essential settings.

Check List

Tests

  • Unit test
  • Integration test
  • Manual test (add detailed scripts or steps below)
    • check that web interface does serve HTTPS if required.

Side effects

Related changes

  • Need to update the documentation
    • Describe the new config and CLI parameters
  • Need to update the tidb-ansible repository
    • Copy the global TLS settings to config
  • Need to be included in the release note
    • Note TLS support

@kennytm kennytm added status/DNM Do not merge, test is failing or blocked by another PR Should Update Docs Should update docs after this PR is merged. Remove this label once the docs are updated priority/important type/feature New feature Should Update Ansible The config in TiDB-Ansible should be updated labels Feb 16, 2020
@kennytm kennytm added 3.0-release-note Should include in release note for next 3.0 release. Remove after release. 3.1-release-note Should include in release note for next 3.1 release. Remove after release. 4.0-release-note Should include in release note for next 4.0 release. Remove after release. labels Feb 17, 2020
@kennytm kennytm force-pushed the kennytm/tls branch 4 times, most recently from 4a876f4 to cc67957 Compare February 20, 2020 10:59
The `curl` on CI is too old to handle ECC keys. But `wget` somehow works.
@kennytm kennytm added status/PTAL This PR is ready for review. Add this label back after committing new changes and removed status/DNM Do not merge, test is failing or blocked by another PR labels Feb 20, 2020
@kennytm
Copy link
Collaborator Author

kennytm commented Feb 20, 2020

/run-all-tests

@kennytm
Copy link
Collaborator Author

kennytm commented Feb 20, 2020

PTAL @july2993 @3pointer

Copy link
Contributor

@july2993 july2993 left a comment

Choose a reason for hiding this comment

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

rest LGTM

lightning/common/util.go Outdated Show resolved Hide resolved
tidb-lightning.toml Show resolved Hide resolved
lightning/common/security.go Outdated Show resolved Hide resolved
@kennytm
Copy link
Collaborator Author

kennytm commented Feb 26, 2020

PTAL @july2993 @3pointer again thanks.

Copy link
Contributor

@july2993 july2993 left a comment

Choose a reason for hiding this comment

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

LGTM

@kennytm kennytm added status/LGT1 One reviewer already commented LGTM (LGTM1) and removed status/PTAL This PR is ready for review. Add this label back after committing new changes labels Feb 26, 2020
@3pointer
Copy link
Contributor

there are two kind of security, global security and tidb security, if pass different security here, it seems that importer and tikv will use global security and tidb, pd will use another. shoud tidb,pd,tikv use the same one?

@kennytm
Copy link
Collaborator Author

kennytm commented Feb 26, 2020

Only SQL connection uses [tidb.security] (corresponding to ssl-ca/cert/key in TiDB). Everything else should use [security] (corresponding to cluster-ssl-ca/cert/key in TiDB). If not it's a bug.

Copy link
Contributor

@3pointer 3pointer left a comment

Choose a reason for hiding this comment

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

LGTM

@3pointer 3pointer added status/LGT2 Two reviewers already commented LGTM, ready for merge (LGTM2) and removed status/LGT1 One reviewer already commented LGTM (LGTM1) labels Feb 27, 2020
@kennytm kennytm merged commit 9068e31 into master Feb 27, 2020
@kennytm kennytm deleted the kennytm/tls branch February 27, 2020 05:21
@kennytm kennytm removed the Should Update Docs Should update docs after this PR is merged. Remove this label once the docs are updated label Jun 17, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
3.0-release-note Should include in release note for next 3.0 release. Remove after release. 3.1-release-note Should include in release note for next 3.1 release. Remove after release. 4.0-release-note Should include in release note for next 4.0 release. Remove after release. Should Update Ansible The config in TiDB-Ansible should be updated status/LGT2 Two reviewers already commented LGTM, ready for merge (LGTM2) type/feature New feature
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Support TLS for security
3 participants