Lightning: Changed data-source-dir to use StorageBackend to prevent leaking secret access key to JSON output #1392
Conversation
and don't always mkdirAll() when using local storage
…e-dir it prevents the secret-key from being leaked
|
[REVIEW NOTIFICATION] This pull request has not been approved. To complete the pull request process, please ask the reviewers in the list to review by filling The full list of commands accepted by this bot can be found here. Reviewer can indicate their review by submitting an approval review. |
| @@ -762,54 +765,6 @@ func (cfg *Config) CheckAndAdjustTiDBPort(ctx context.Context, mustHaveInternalC | |||
| return nil | |||
| } | |||
|
|
|||
| func (cfg *Config) CheckAndAdjustFilePath() error { | |||
There was a problem hiding this comment.
this check-and-adjust does several things:
- if the data-source-dir is a path, we
common.IsDirExistsit. this is going to be replaced by the ListObjects permission check. - turn the path into a URL. this comes for free by
storage.ParseBackend. - ban unsupported schemes. currently all schemes are supported (including GCS), so the check is useless now.
| if !taskCfg.App.CheckRequirements { | ||
| storagePermissions = nil | ||
| } | ||
| s, err := taskCfg.Mydumper.SourceDir.NewStorage(ctx, storagePermissions) |
There was a problem hiding this comment.
this replaces the StoragePermission pre-check
| message := fmt.Sprintf("chunk checkpoints path is not equal to config"+ | ||
| "checkpoint is %s, config source dir is %s", chunk.FileMeta.Path, rc.cfg.Mydumper.SourceDir) | ||
| msgs = append(msgs, message) | ||
| } |
There was a problem hiding this comment.
i don't know how this check can ever be valid, when SourceDir can be non-local
|
@kennytm: PR needs rebase. 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 kubernetes/test-infra repository. |
What problem does this PR solve?
When using Lightning to load from S3, the non-environmental access key and secret key have to be provided through URL parameters in
mydumper.data-source-dir. This is stored as a plain string.Lightning also prints the config as JSON before starting a task for debugging.
These two together means the access key and secret key will be leaked to the log.
What is changed and how it works?
The type of
mydumper.data-source-diris changed from a string to a custom type (*backuppb.StorageBackend) which scrubs all extra parameters when serializing as JSON.The change introduces some additional effects needing refactoring:
mydumper.data-source-direxists before running. This overlaps with theStoragePermissionspre-check for S3. These, along with GCS, are combined into the existingCheckPermissionscheck.SkipCheckPathfield for external storage is thus entirely ignored (it is already set totrueon Dumpling and TiCDC so it is safe to ignore). The BR flag--skip-check-pathis unaffected.Check List
Tests
Code changes
Side effects
Related changes
Release note