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: Fix maglev.hashSeed byte size documentation #16690

Merged
merged 1 commit into from Jul 1, 2021
Merged

Docs: Fix maglev.hashSeed byte size documentation #16690

merged 1 commit into from Jul 1, 2021

Conversation

gaffneyd4
Copy link
Contributor

@gaffneyd4 gaffneyd4 commented Jun 29, 2021

Update documentation on maglev hash seed to instruct users to generate a 12 byte seed (instead of 16 bytes), according to this error message from cilium v1.9.4 (when a 16 byte seed had been used):

Failed to initialize maglev hash seeds: Decoded hash seed is 16 bytes (not 12 bytes)

@gaffneyd4 gaffneyd4 requested a review from a team as a code owner June 29, 2021 22:28
@gaffneyd4 gaffneyd4 requested a review from a team June 29, 2021 22:28
@maintainer-s-little-helper

This comment has been minimized.

@gaffneyd4 gaffneyd4 requested a review from qmonnet June 29, 2021 22:28
@maintainer-s-little-helper maintainer-s-little-helper bot added dont-merge/needs-sign-off The author needs to add signoff to their commits before merge. dont-merge/needs-release-note-label The author needs to describe the release impact of these changes. and removed dont-merge/needs-sign-off The author needs to add signoff to their commits before merge. labels Jun 29, 2021
Signed-off-by: Derek Gaffney <derekmgaffney@gmail.com>
@gaffneyd4 gaffneyd4 changed the title Docs: Fix maglev.hashSeed generation command Docs: Fix maglev.hashSeed byte size documentation Jun 29, 2021
Copy link
Member

@qmonnet qmonnet left a comment

Choose a reason for hiding this comment

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

Thanks a lot for the PR!

But I don't know if the change is correct. Looking at the history, I see that commit 4d9167f explicitly changed from 16 to 12 bytes, so the 16 byte length may apply to the base64 output and not to the initial bytes piped into base64? /Cc @brb

In any case one of the commands is wrong (either -c12 or -c16) and the text may need some clarification indeed, so thanks for working on this!

[Edit] You edited your PR while I was writing the review, and it looks good now I think. Asking a review from Martynas to be sure, but looks good from my side, thank you!

@qmonnet qmonnet added area/documentation Impacts the documentation, including textual changes, sphinx, or other doc generation code. needs-backport/1.9 labels Jun 29, 2021
@maintainer-s-little-helper maintainer-s-little-helper bot added this to Needs backport from master in 1.10.2 Jun 29, 2021
@maintainer-s-little-helper maintainer-s-little-helper bot added this to Needs backport from master in 1.9.9 Jun 29, 2021
@qmonnet qmonnet added the release-note/minor This PR changes functionality that users may find relevant to operating Cilium. label Jun 29, 2021
@maintainer-s-little-helper maintainer-s-little-helper bot removed the dont-merge/needs-release-note-label The author needs to describe the release impact of these changes. label Jun 29, 2021
Copy link
Member

@brb brb left a comment

Choose a reason for hiding this comment

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

The maglev hash seed is 16 bytes long. See:

$ head -c12 /dev/urandom | base64 -w0 | wc -c
16

Copy link
Member

@brb brb left a comment

Choose a reason for hiding this comment

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

Scratch my previous comment.

@qmonnet qmonnet added the ready-to-merge This PR has passed all tests and received consensus from code owners to merge. label Jul 1, 2021
@pchaigno
Copy link
Member

pchaigno commented Jul 1, 2021

Documentation-only change so fine to merge despite merge freeze.

@pchaigno pchaigno merged commit d972406 into cilium:master Jul 1, 2021
@aanm aanm added this to Needs backport from master in 1.10.3 Jul 2, 2021
@aanm aanm removed this from Needs backport from master in 1.10.2 Jul 2, 2021
@maintainer-s-little-helper maintainer-s-little-helper bot moved this from Needs backport from master to Backport pending to v1.9 in 1.9.9 Jul 5, 2021
@aanm aanm moved this from Needs backport from master to Backport done to v1.10 in 1.10.3 Jul 15, 2021
@aanm aanm added release-note/misc This PR makes changes that have no direct user impact. and removed release-note/minor This PR changes functionality that users may find relevant to operating Cilium. labels Jul 15, 2021
@joestringer joestringer added this to Backport pending to v1.9 in 1.9.10 Jul 19, 2021
@joestringer joestringer removed this from Backport pending to v1.9 in 1.9.9 Jul 19, 2021
@maintainer-s-little-helper maintainer-s-little-helper bot added this to Backport done to v1.9 in 1.9.9 Jul 19, 2021
@joestringer joestringer removed this from Backport pending to v1.9 in 1.9.10 Jul 19, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/documentation Impacts the documentation, including textual changes, sphinx, or other doc generation code. ready-to-merge This PR has passed all tests and received consensus from code owners to merge. release-note/misc This PR makes changes that have no direct user impact.
Projects
No open projects
1.10.3
Backport done to v1.10
1.9.9
Backport done to v1.9
Development

Successfully merging this pull request may close these issues.

None yet

8 participants