Fix for #89 - Use custom UUID for NAB Object #101
Conversation
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: mpryc 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:
Approvers can indicate their approval by writing |
|
@shubham-pampattiwar FYI please review |
| const VeleroBackupNamePrefix = "nab" | ||
| // MaximumNacObjectNameLength represents Generated Non Admin Object Name and | ||
| // must be below 63 characters, because it's used within object Label Value | ||
| const MaximumNacObjectNameLength = validation.DNS1123LabelMaxLength |
There was a problem hiding this comment.
why redefine const?
There was a problem hiding this comment.
It's not rename. I've removed const VeleroBackupNamePrefix completely and added new one MaximumNacObjectNameLength
| return false | ||
| } | ||
| // TODO what is a valid uuid? |
There was a problem hiding this comment.
I think this is a bad change
First, the function does check for max label length
It also does not check if UUID value is valid
I would not change checkAnnotationValueIsValid function logic
Correct approach here is to create new function to check if label value is valid and also check if UUID value is valid, by using Parse function https://pkg.go.dev/github.com/google/[email protected]#Parse, right?
There was a problem hiding this comment.
The Non admin UUID is not part of annotation, it's part of label now. Also it's not a valid UUID as we add more to it (prefix).
| return fmt.Errorf("failed to list objects in namespace '%s': %w", namespace, err) | ||
| } | ||
|
|
||
| return nil |
There was a problem hiding this comment.
I would call this function something like GetSingleObjectByLabel and do validation here
For case no items are found, I would use NewNotFound function https://pkg.go.dev/k8s.io/[email protected]/pkg/api/errors#NewNotFound to keep behavior in controllers
There was a problem hiding this comment.
No, this is not how it works. This particular function is generic one that returns all the objects within that label. Later the function GetVeleroBackupByLabel is consuming that generic function and have single object logic. I don't want to call it Single, because it's single by name and not the GetVeleroBackupsByLabel which would be plural.
There was a problem hiding this comment.
I've moved this function to be more generic for use in restore as well in the future.
There was a problem hiding this comment.
I don't want to use proposed function, because we do not have knowledge why the error when listing the objects was raised. It's an error from the List() function, so there may be multiple reasons why it failed to List and even if the objects are in the cluster the error still may happen.
With this change a new UUID is generated to reference parent/child relationship between objects in the Non Admin Controller use cases. The first consumer of this UUID is a Velero Backup, created when the NonAdminBackup object is reconciled. The NonAdminBackup object generates the NAC UUID and stores it in its Status. This prevents users from modifying it. The UUID is later used to create the Velero Backup during reconciliation. While the NAC UUID is currently used as the Velero Backup name, this is not required, as the UUID is also stored as a Velero Backup label, which is used during the reconcile loop. Usage of NAC UUID as Velero Backup name is to easy it's creation. This PR also includes small changes to fix linting issues of the code, as well reworks the tests to properly take advantage of gingko BeforeEach function. Signed-off-by: Michal Pryc <[email protected]>
mpryc
force-pushed
the
velero_nab_ownership
branch
from
ac7eba1
to
b20a4cc
Compare
With this change a new UUID is generated to reference parent/child relationship between objects in the Non Admin Controller use cases.
The first consumer of this UUID is a Velero Backup, created when the NonAdminBackup object is reconciled.
The NonAdminBackup object generates the NAC UUID and stores it in its Status. This prevents users from modifying it. The UUID is later used to create the Velero Backup during reconciliation.
While the NAC UUID is currently used as the Velero Backup name, this is not required, as the UUID is also stored as a Velero Backup label, which is used during the reconcile loop. Usage of NAC UUID as Velero Backup name is to easy it's creation.
This PR also includes small changes to fix linting issues of the code, as well reworks the tests to properly take advantage of gingko BeforeEach function.
Why the changes were made
To fix #89