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.

Sign up for GitHub

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

Jump to bottom

rpk: replace usage of md5 with sha256 in generating filename backup hash #17568

Merged
merged 14 commits into from
Apr 3, 2024
Merged

rpk: replace usage of md5 with sha256 in generating filename backup hash #17568

merged 14 commits into from
Apr 3, 2024

Conversation

andrewhsu
Copy link
Member

@andrewhsu andrewhsu commented Apr 3, 2024 •
edited

issue https://redpandadata.atlassian.net/browse/PESDLC-1040

when rpk does a backup on files that it modifies, e.g. /etc/default/grub files, the original file was copied to a new filename with a hash using md5, e.g. /etc/default/grub.vectorized.2c349a84043328ae3a9f2d021ff143c3.bk. Now they will use sha256 so the hash portion will likely look different, e.g.
/etc/default/grub.vectorized.9be9f2dfe19f13b03e09fcc75648d4ec.bk.

Backports Required

  • none - not a bug fix
  • none - this is a backport
  • none - issue does not exist in previous branches
  • none - papercut/not impactful enough to backport
  • v23.3.x
  • v23.2.x

Release Notes

Improvements

  • rpk: change hash for backup filenames to use sha256

@andrewhsu
rpk: remove redundant field wantErr in grub_test
ea9e072 
No functionality change, just test refactor.
@andrewhsu
rpk: remove redundant field in grub_test
5a01183 
No functionality change, just test refactor.
@andrewhsu
rpk: remove call to md5.Sum() in grub_test
018cea1 
No functionality change, just test refactor.
@andrewhsu
rpk: move grub_test initial grub contents into testdata
0709619 
No functionality change, just test refactor.
@andrewhsu
rpk: ensure grub options line is deterministic
36324cd 
No functionality change, just ensuring consistency in behaviour so a
static golden file can be used to validate.
@andrewhsu
rpk: validate grub_test transforms against golden file
0865603 
No functionality change, just test refactor.
@andrewhsu
rpk: remove redundant grub_test code
8867d75 
No functionality change, just test refactor.
@andrewhsu
rpk: remove call to md5.Sum() in balance_service_test
d35d1dc 
No functionality change, just test refactor.
@andrewhsu
rpk: use static filename for balance_service_test
d631b3c 
No functionality change, just test refactor.
@andrewhsu
rpk: remove unused arg in balance_service_test
365adac 
No functionality change, just test refactor.
@andrewhsu
rpk: give balance_service_test tests a number
87e9fc6
@andrewhsu
rpk: move balance_service_test initial irqbalance contents into testdata
9da7031
@andrewhsu
rpk: replace md5 with sha256 for filename backup hash
ae6e6ed
@andrewhsu
gha: lock to goreleaser v1.24.0
7fdaad4
@andrewhsu andrewhsu force-pushed the PESDLC-1040-replace-filename-backup-hash branch from 68b4b4d to 7fdaad4 Compare April 3, 2024 14:40
@andrewhsu andrewhsu marked this pull request as ready for review April 3, 2024 14:44
@andrewhsu andrewhsu requested review from gousteris and removed request for a team April 3, 2024 14:45
andrewhsu
andrewhsu commented Apr 3, 2024
View reviewed changes
src/go/rpk/pkg/utils/files.go
@@ -83,28 +83,28 @@ func WriteBytes(fs afero.Fs, bs []byte, path string) (int, error) {
return len(bs), afero.WriteFile(fs, path, bs, 0o600)
}

func FileMd5(fs afero.Fs, filePath string) (string, error) {
Copy link
Member Author

Choose a reason for hiding this comment

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

this is the only golang exported function that changed name. changed from FileMd5 to FilenameHash. from search in redpanda-data org, doesn't look like it is used anywhere.

@andrewhsu
Copy link
Member Author

tests are passing. ready for review.

twmb
twmb approved these changes Apr 3, 2024
View reviewed changes
Copy link
Contributor

@twmb twmb left a comment

Choose a reason for hiding this comment

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

+1 for Go changes
(don't know what the grub stuff is)

@deniscoady
Copy link

Are there any concerns about upgrades with existing files on disk? Do existing backups need to be modified with the new hash?

@andrewhsu
Copy link
Member Author

(don't know what the grub stuff is)

@twmb i kept the test cases in grub_test.go logically the same to be conservative, but i had to refactor the test cases to use golden files in testdata dir so sha256sum calculation is easier. at runtime the grub stuff is called when tune command is used and certain conditions are met:

sudo rpk redpanda tune cpu --reboot-allowed

example output:

Disabling CPU C-States 
Backup of GRUB config created '/etc/default/grub.vectorized.0045f30dc1f17bb71be8f965442b1dc1.bk'
Disabling CPU P-States
Backup of GRUB config created '/etc/default/grub.vectorized.3bada2a45a2faa298ce368920ecadf47.bk'
Updating GRUB configuration
TUNER         APPLIED        ENABLED        SUPPORTED
cpu  true  true  true
IMPORTANT: Reboot system and run 'rpk redpanda tune cpu' again

@andrewhsu
Copy link
Member Author

Are there any concerns about upgrades with existing files on disk? Do existing backups need to be modified with the new hash?

@deniscoady i don't believe so. it seems rpk will create the backup files before it modifies the original, but i did not notice any rpk code that subsequently looks up the backup file to do anything (like "undo" operation). i think the backup file is just there for humans to undo if they want. @twmb correct me if i'm wrong here.

@vbotbuildovich
Copy link
Collaborator

ducktape was retried in https://buildkite.com/redpanda/redpanda/builds/47300#018ea4c6-2c78-4a72-a0b4-b6e3496e9e32

@andrewhsu andrewhsu merged commit 5b58e73 into redpanda-data:dev Apr 3, 2024
23 checks passed
@andrewhsu andrewhsu deleted the PESDLC-1040-replace-filename-backup-hash branch April 3, 2024 17:58
@andrewhsu andrewhsu mentioned this pull request Apr 3, 2024
Merged
6 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@twmb twmb twmb approved these changes

@r-vasquez r-vasquez Awaiting requested review from r-vasquez r-vasquez is a code owner

@gene-redpanda gene-redpanda Awaiting requested review from gene-redpanda gene-redpanda is a code owner

@gousteris gousteris Awaiting requested review from gousteris gousteris is a code owner automatically assigned from redpanda-data/devprod

Assignees
No one assigned
Labels
area/build area/rpk
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
4 participants
@andrewhsu @deniscoady @vbotbuildovich @twmb