br: add option to test encryption for all br int tests #56434

Tristan1900 · 2024-10-01T16:11:08Z

What problem does this PR solve?

Issue Number: close #56433

Problem Summary:

What changed and how does it work?

1. Add option to enable full/log backup restore encryption for all integration tests
2. Validate files are actually encrypted by reading sst files and validate its magic number footer and read log files and validate its Zstd compression format. Encrypted files should be a random blob and not able to identify any structured format

Check List

Tests

Unit test
Integration test
Manual test (add detailed scripts or steps below)
No need to test
- I checked and no code files have been changed.

Side effects

Performance regression: Consumes more CPU
Performance regression: Consumes more Memory
Breaking backward compatibility

Documentation

Release note

Please refer to Release Notes Language Style Guide to write a quality release note.

None

Signed-off-by: Wenqi Mou <wenqimou@gmail.com>

tiprow · 2024-10-01T16:11:29Z

Hi @Tristan1900. Thanks for your PR.

PRs from untrusted users cannot be marked as trusted with /ok-to-test in this repo meaning untrusted PR authors can never trigger tests themselves. Collaborators can still trigger tests on the PR using /test all.

I understand the commands that are listed here.

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-sigs/prow repository.

Tristan1900 · 2024-10-01T16:15:33Z

/hold

Tristan1900 · 2024-10-01T16:15:56Z

/ok-to-test

codecov · 2024-10-01T16:29:06Z

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 57.7578%. Comparing base (74034d4) to head (28005d7).
Report is 59 commits behind head on master.

Additional details and impacted files

@@                Coverage Diff                @@
##             master     #56434         +/-   ##
=================================================
- Coverage   73.3650%   57.7578%   -15.6072%     
=================================================
  Files          1624       1777        +153     
  Lines        448069     647600     +199531     
=================================================
+ Hits         328726     374040      +45314     
- Misses        99206     248479     +149273     
- Partials      20137      25081       +4944

Flag	Coverage Δ
integration	`39.8647% <ø> (?)`
unit	`73.1558% <ø> (+0.6922%)`	⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Components	Coverage Δ
dumpling	`52.9478% <ø> (ø)`
parser	`∅ <ø> (∅)`
br	`61.6481% <ø> (+16.1351%)`	⬆️

Signed-off-by: Wenqi Mou <wenqimou@gmail.com>

Tristan1900 · 2024-10-04T17:47:25Z

/retest

Signed-off-by: Wenqi Mou <wenqimou@gmail.com>

YuJuncen

rest lgtm

YuJuncen · 2024-10-08T09:07:04Z

br/tests/utils.go

+)
+
+func main() {
+	if len(os.Args) < 2 {


You may use cobra.Command instead of manually parse the command.

YuJuncen · 2024-10-08T09:14:25Z

br/tests/utils.go

+	storagePath := ""
+
+	for i, arg := range args {
+		if arg == "backup" || arg == "restore" {


Perhaps verifying backup is enough? As restore in fact contributes nothing to the files to be checked.

I should add a comment for the code here.
For the full backup we can check the storage encryption status once the command is complete since it's a blocking call, but for log backup I believe we need some extra logic to wait for it to generate some files. So my approach is to verify log backup files before doing restore, making sure the files are actually encrypted. Let me know what you think!

YuJuncen · 2024-10-08T09:15:46Z

br/tests/utils.go

+		if arg == "backup" || arg == "restore" {
+			hasBackupOrRestore = true
+		}
+		if arg == "-s" && i+1 < len(args) && strings.HasPrefix(args[i+1], "local://") {


Maybe also handle --storage?

ah good call, all the tests are using -s so I thought that's the command.

YuJuncen · 2024-10-08T09:30:13Z

tests/_utils/run_br

@@ -20,4 +20,4 @@ br.test -test.coverprofile="$COV_DIR/cov.$TEST_NAME.$$.out.log" DEVEL "$@" \
    -L "info" \
    --ca "$TEST_DIR/certs/ca.pem" \
    --cert "$TEST_DIR/certs/br.pem" \
-    --key "$TEST_DIR/certs/br.key"
+    --key "$TEST_DIR/certs/br.key" \


Maybe keep the empty line at the end of file.

right, I added logic and later removed, need to revert it

YuJuncen · 2024-10-08T09:35:39Z

br/tests/run.sh

+ENCRYPTION_ARGS="--crypter.method aes128-ctr --crypter.key 0123456789abcdef0123456789abcdef --master-key-crypter-method AES256-CTR --master-key $MASTER_KEY_PATH"
+
+# plaintext data key
+#ENCRYPTION_ARGS="--crypter.method aes128-ctr --crypter.key 0123456789abcdef0123456789abcdef --log.crypter.method AES256-CTR --log.crypter.key 0123456789abcdef0123456789abcdef0123456789abcdef0123456789abcdef"


Maybe remove this if it isn't needed.

this is sort of a placeholder if we want to test the plaintext case, we can uncomment it and run the test. The entire tests will be run as needed, not by default for every commit per discussion with Brian, but I don't think it will hurt if we just enable encryption by default, let me know your thoughts.!

Perhaps add some hints about when and hot to use it(Say, # If you want to specify a plaintext data key, uncomment and modify this as you like.? Or it looks pretty like temporary code during developing...

sounds good!

Signed-off-by: Wenqi Mou <wenqimou@gmail.com>

purelind · 2024-10-09T02:01:55Z

Please wait for this PR #51126 to be merged first.

purelind · 2024-10-09T06:24:21Z

/retest

Signed-off-by: Wenqi Mou <wenqimou@gmail.com>

Tristan1900 · 2024-10-10T14:49:58Z

/unhold

ti-chi-bot · 2024-10-11T09:03:16Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: BornChanger, YuJuncen

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

The pull request process is described here

Needs approval from an approver in each of these files:

~~OWNERS~~ [BornChanger,YuJuncen]
~~br/OWNERS~~ [BornChanger,YuJuncen]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

ti-chi-bot · 2024-10-11T09:03:20Z

[LGTM Timeline notifier]

Timeline:

2024-10-09 07:50:04.245415946 +0000 UTC m=+1032359.665628959: ☑️ agreed by YuJuncen.
2024-10-11 09:03:19.226913708 +0000 UTC m=+2596.375823533: ☑️ agreed by BornChanger.

Tristan1900 · 2024-10-11T15:09:48Z

/retest

try to fix aws tests, load balance br int tests

1fa9521

Signed-off-by: Wenqi Mou <wenqimou@gmail.com>

ti-chi-bot bot added release-note-none Denotes a PR that doesn't merit a release note. do-not-merge/needs-tests-checked size/S Denotes a PR that changes 10-29 lines, ignoring generated files. and removed do-not-merge/needs-tests-checked labels Oct 1, 2024

ti-chi-bot bot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Oct 1, 2024

ti-chi-bot bot added the ok-to-test Indicates a PR is ready to be tested. label Oct 1, 2024

Tristan1900 added 2 commits October 1, 2024 14:13

fix typo

b0c87ce

Signed-off-by: Wenqi Mou <wenqimou@gmail.com>

run encryption everywhere, disable aws tests

1a0203e

Signed-off-by: Wenqi Mou <wenqimou@gmail.com>

ti-chi-bot 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 Oct 1, 2024

Tristan1900 added 3 commits October 2, 2024 17:21

skip compat tests

1b192f2

Signed-off-by: Wenqi Mou <wenqimou@gmail.com>

fix test

8b42327

Signed-off-by: Wenqi Mou <wenqimou@gmail.com>

add debug log for br-pitr

b10ccfe

Signed-off-by: Wenqi Mou <wenqimou@gmail.com>

Tristan1900 added 2 commits October 4, 2024 21:32

test local master key

aeca72c

Signed-off-by: Wenqi Mou <wenqimou@gmail.com>

better version to check on files

2c4622f

Signed-off-by: Wenqi Mou <wenqimou@gmail.com>

ti-chi-bot bot added size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. and removed size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Oct 7, 2024

Tristan1900 added 4 commits October 7, 2024 20:56

fix style

4f4571f

Signed-off-by: Wenqi Mou <wenqimou@gmail.com>

fix style

835d8ea

Signed-off-by: Wenqi Mou <wenqimou@gmail.com>

another try

720b7db

Signed-off-by: Wenqi Mou <wenqimou@gmail.com>

suppress link

de48d89

Signed-off-by: Wenqi Mou <wenqimou@gmail.com>

YuJuncen reviewed Oct 8, 2024

View reviewed changes

Tristan1900 added 2 commits October 8, 2024 15:09

address comments

c18be90

Signed-off-by: Wenqi Mou <wenqimou@gmail.com>

move around test to see if fails

c4a502d

Signed-off-by: Wenqi Mou <wenqimou@gmail.com>

Tristan1900 added 2 commits October 8, 2024 21:53

don't move br_other, see if it still fails

af6394d

Signed-off-by: Wenqi Mou <wenqimou@gmail.com>

revert back to original, no idea what's going on

7c6cf33

Signed-off-by: Wenqi Mou <wenqimou@gmail.com>

YuJuncen approved these changes Oct 9, 2024

View reviewed changes

ti-chi-bot bot added approved needs-1-more-lgtm Indicates a PR needs 1 more LGTM. labels Oct 9, 2024

Tristan1900 added 3 commits October 9, 2024 12:49

finally figure out what's going on...

fcfa309

Signed-off-by: Wenqi Mou <wenqimou@gmail.com>

forgot to revert

9d2dc50

Signed-off-by: Wenqi Mou <wenqimou@gmail.com>

confirm fixed

28005d7

Signed-off-by: Wenqi Mou <wenqimou@gmail.com>

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

BornChanger approved these changes Oct 11, 2024

View reviewed changes

ti-chi-bot bot added lgtm and removed needs-1-more-lgtm Indicates a PR needs 1 more LGTM. labels Oct 11, 2024

ti-chi-bot bot merged commit 99080c8 into pingcap:master Oct 11, 2024
24 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

br: add option to test encryption for all br int tests #56434

br: add option to test encryption for all br int tests #56434

Tristan1900 commented Oct 1, 2024 •

edited

Loading

tiprow bot commented Oct 1, 2024

Tristan1900 commented Oct 1, 2024

Tristan1900 commented Oct 1, 2024

codecov bot commented Oct 1, 2024 •

edited

Loading

Tristan1900 commented Oct 4, 2024

YuJuncen left a comment

YuJuncen Oct 8, 2024

Tristan1900 Oct 8, 2024

YuJuncen Oct 8, 2024

Tristan1900 Oct 8, 2024

YuJuncen Oct 9, 2024

YuJuncen Oct 8, 2024

Tristan1900 Oct 8, 2024

YuJuncen Oct 8, 2024

Tristan1900 Oct 8, 2024

YuJuncen Oct 8, 2024

Tristan1900 Oct 8, 2024

YuJuncen Oct 9, 2024 •

edited

Loading

Tristan1900 Oct 9, 2024

purelind commented Oct 9, 2024

purelind commented Oct 9, 2024

Tristan1900 commented Oct 10, 2024

ti-chi-bot bot commented Oct 11, 2024

ti-chi-bot bot commented Oct 11, 2024

Tristan1900 commented Oct 11, 2024

br: add option to test encryption for all br int tests #56434

br: add option to test encryption for all br int tests #56434

Conversation

Tristan1900 commented Oct 1, 2024 • edited Loading

What problem does this PR solve?

What changed and how does it work?

Check List

Release note

tiprow bot commented Oct 1, 2024

Tristan1900 commented Oct 1, 2024

Tristan1900 commented Oct 1, 2024

codecov bot commented Oct 1, 2024 • edited Loading

Codecov Report

Tristan1900 commented Oct 4, 2024

YuJuncen left a 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

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

YuJuncen Oct 9, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

purelind commented Oct 9, 2024

purelind commented Oct 9, 2024

Tristan1900 commented Oct 10, 2024

ti-chi-bot bot commented Oct 11, 2024

ti-chi-bot bot commented Oct 11, 2024

[LGTM Timeline notifier]

Tristan1900 commented Oct 11, 2024

Tristan1900 commented Oct 1, 2024 •

edited

Loading

codecov bot commented Oct 1, 2024 •

edited

Loading

YuJuncen Oct 9, 2024 •

edited

Loading