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

Validate restore name label length #1392

Merged
merged 1 commit into from May 1, 2019

Conversation

Projects
None yet
4 participants
@canshul08
Copy link
Contributor

commented Apr 24, 2019

Velero should handle cases when the label length exceeds 63 characters.

  • if the length of the backup/restore name is <= 63 characters, use it as the value of the label
  • if it's > 63 characters, take the SHA256 hash of the name. the value of
    the label will be the first 57 characters of the backup/restore name
    plus the first six characters of the SHA256 hash.

Fixes #1021

Signed-off-by: Anshul Chandra anshulc@vmware.com

@canshul08 canshul08 force-pushed the canshul08:restore_labels branch 2 times, most recently from d4df312 to 930dcfa Apr 24, 2019

@nrb
Copy link
Member

left a comment

Thanks for this! I haven't looked at the test cases in detail yet, but thank you for creating them! I have one comment on naming, though curious what others think.

Show resolved Hide resolved pkg/labels/labels.go Outdated

@canshul08 canshul08 force-pushed the canshul08:restore_labels branch from 930dcfa to 301c8e7 Apr 24, 2019

@carlisia
Copy link
Member

left a comment

You did an amazing job with tests, thank you! A few comments for now. 👍

Show resolved Hide resolved pkg/labels/labels.go Outdated
Show resolved Hide resolved pkg/labels/labels.go Outdated

@canshul08 canshul08 force-pushed the canshul08:restore_labels branch from 301c8e7 to b42b3a9 Apr 25, 2019

@skriss

This comment has been minimized.

Copy link
Contributor

commented Apr 29, 2019

@canshul08 this looks like it's in pretty good shape. Have you been able to do any manual testing at all?

@canshul08

This comment has been minimized.

Copy link
Contributor Author

commented Apr 29, 2019

@canshul08 this looks like it's in pretty good shape. Have you been able to do any manual testing at all?
@skriss Yes, I tried testing the functionality by creating backups with name more than 63 characters and later restoring the same. However, I'm not very sure of Restic, I'll check with you on that.

@skriss

This comment has been minimized.

Copy link
Contributor

commented Apr 29, 2019

OK - I can help out with some testing there

Show resolved Hide resolved pkg/label/label.go Outdated

@canshul08 canshul08 force-pushed the canshul08:restore_labels branch from b42b3a9 to 4ec0f3c Apr 30, 2019

@nrb
Copy link
Member

left a comment

This is looking really good, thanks!

A couple small comments.

// If length exceeds, we trim the label name to contain only max allowed characters
// Additionally, the last 6 characters of the label name are replaced by the first 6 characters of the sha256 of original label
func GetValidName(label string) string {
maxLength := validation.DNS1035LabelMaxLength

This comment has been minimized.

Copy link
@nrb

nrb Apr 30, 2019

Member

This is a nitpick, but is the variable assignment necessary here?

This comment has been minimized.

Copy link
@canshul08

canshul08 Apr 30, 2019

Author Contributor

I'll remove the assignment.


// GetValidName converts an input string to valid kubernetes label string in accordance to rfc1035 DNS Label spec
// (https://github.com/kubernetes/community/blob/master/contributors/design-proposals/architecture/identifiers.md)
// Length of the label is adjusted basis the MaxLength (i.e. 63 characters)

This comment has been minimized.

Copy link
@nrb

nrb Apr 30, 2019

Member

// Length of the label is adjusted based on the MaxLength (63 characters)

This comment has been minimized.

Copy link
@canshul08

canshul08 Apr 30, 2019

Author Contributor

Will fix the comment.

@canshul08 canshul08 force-pushed the canshul08:restore_labels branch from 4ec0f3c to 58a1739 Apr 30, 2019

@skriss
Copy link
Contributor

left a comment

Two minor comments, otherwise LGTM. I was able to do some testing and everything looked good to me!

Show resolved Hide resolved changelogs/unreleased/1392-anshulc Outdated
Show resolved Hide resolved pkg/label/label.go Outdated
Validate restore name label length
Velero should handle cases when the label length exceeds 63 characters.

- if the length of the backup/restore name is <= 63 characters, use it as the value of the label
- if it's > 63 characters, take the SHA256 hash of the name. the value of
  the label will be the first 57 characters of the backup/restore name
  plus the first six characters of the SHA256 hash.

Fixes #1021

Signed-off-by: Anshul Chandra <anshulc@vmware.com>

@canshul08 canshul08 force-pushed the canshul08:restore_labels branch from 58a1739 to 4e12b08 May 1, 2019

@skriss

skriss approved these changes May 1, 2019

Copy link
Contributor

left a comment

LGTM - @nrb looks like your comments were resolved, but I'll let you take another look before merge

@nrb

This comment has been minimized.

Copy link
Member

commented May 1, 2019

Awesome, thanks for the PR and your patience, @canshul08!

@nrb nrb merged commit 0205a43 into heptio:master May 1, 2019

2 checks passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
signed-off-by Commit has Signed-off-by
Details

@canshul08 canshul08 deleted the canshul08:restore_labels branch May 1, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.