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

Related 89: Function to generate custom UUID for NAC objects #98

Closed
wants to merge 2 commits into from
Closed

Related 89: Function to generate custom UUID for NAC objects #98

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
93f7ae7
Related 89: Function to generate custom UUID for NAC objects
mpryc Oct 3, 2024
ecdc7f2
Rename BackupNameDelimiter to NameDelimiter
mpryc Oct 7, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 3 additions & 0 deletions internal/common/constant/constant.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -39,6 +39,9 @@ const (
// EmptyString defines a constant for the empty string
const EmptyString = ""

// NameDelimiter defines character that is used to separate name parts
const NameDelimiter = "-"

// TrueString defines a constant for the True string
const TrueString = "True"

Expand Down
46 changes: 46 additions & 0 deletions internal/common/function/function.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -24,6 +24,7 @@ import (
"fmt"

"github.com/go-logr/logr"
"github.com/google/uuid"
velerov1 "github.com/vmware-tanzu/velero/pkg/apis/velero/v1"
"k8s.io/apimachinery/pkg/types"
"k8s.io/apimachinery/pkg/util/validation"
Expand Down Expand Up @@ -139,6 +140,51 @@ func GenerateVeleroBackupName(namespace, nabName string) string {
return veleroBackupName
}

// GenerateVeleroBackupNameWithUUID generates a Velero backup name based on the provided namespace and NonAdminBackup name.
// It includes a UUID suffix. If the name exceeds the maximum length, it truncates nabName first, then namespace.
func GenerateVeleroBackupNameWithUUID(namespace, nabName string) string {
kaovilai marked this conversation as resolved.
Show resolved Hide resolved
Copy link
Member

Choose a reason for hiding this comment

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

I think we are gonna use a similar function for NorAdmin Restore Objects ,right ? Would it not make sense to modify this function to just GenerateNameWithUUID ? (Refactor it so that there is not context of Backup/Restore, just namespace and NacObjectName as parameters)

Copy link
Member

@kaovilai kaovilai Oct 10, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

We can always follow up with a refactor where this func calls a more generic func

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I am closing this PR, new function is generic and will be included in other PR

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

// GenerateNacObjectNameWithUUID generates a unique name based on the provided namespace and object origin name.
// It includes a UUID suffix. If the name exceeds the maximum length, it truncates nacName first, then namespace.
func GenerateNacObjectNameWithUUID(namespace, nacName string) string {
	// Generate UUID suffix
	uuidSuffix := uuid.New().String()

	// Build the initial name based on the presence of namespace and nacName
	nacObjectName := uuidSuffix
	if len(nacName) > 0 {
		nacObjectName = nacName + constant.NameDelimiter + nacObjectName
	}
	if len(namespace) > 0 {
		nacObjectName = namespace + constant.NameDelimiter + nacObjectName
	}

	if len(nacObjectName) > constant.MaximumNacObjectNameLength {
		// Calculate remaining length after UUID
		remainingLength := constant.MaximumNacObjectNameLength - len(uuidSuffix)

		delimeterLength := len(constant.NameDelimiter)

		// Subtract two delimiter lengths to avoid a corner case where the namespace
		// and delimiters leave no space for any part of nabName
		if len(namespace) > remainingLength-delimeterLength-delimeterLength {
			namespace = namespace[:remainingLength-delimeterLength-delimeterLength]
			nacObjectName = namespace + constant.NameDelimiter + uuidSuffix
		} else {
			remainingLength = remainingLength - len(namespace) - delimeterLength - delimeterLength
			nacName = nacName[:remainingLength]
			nacObjectName = uuidSuffix
			if len(nacName) > 0 {
				nacObjectName = nacName + constant.NameDelimiter + nacObjectName
			}
			if len(namespace) > 0 {
				nacObjectName = namespace + constant.NameDelimiter + nacObjectName
			}
		}
	}

	return nacObjectName
}

// Generate UUID suffix
uuidSuffix := uuid.New().String()

// Build the initial backup name based on the presence of namespace and nabName
veleroBackupName := uuidSuffix
if len(nabName) > 0 {
veleroBackupName = nabName + constant.NameDelimiter + veleroBackupName
}
if len(namespace) > 0 {
Copy link
Member

Choose a reason for hiding this comment

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

Maybe we can drop using namespace in VeleroBackupName. It will anyways be part of metadata/spec ?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I think it's useful to quickly filter the namespace from which Backup was created. So if we have multiple users with admin rights that have separate namesapces it allows to quickly sort/identify which Backups are from which ns. Of course this will be also available as additional info via annotation or label, but here is just another step to not have one list of unreadable by human backup names (uuid's only as example).

Copy link
Contributor

Choose a reason for hiding this comment

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

If we are using a constant prefix in NAB names, this function needs to change

way it is, we call it and then check if result is valid; if not, we will retry

but on retry, only thing that will change is name suffix, which has the UUID

Correct way should be to create a function that generates the prefix (in this case <nab-name>-<nab-namespace>-, truncated) if needed

then, inside retry loop, we create UUID and add result of previous function and try to create velero backup. If it fails, we just retry with new UUID, prefix was already created and will not change

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

It's essentially the same, just a different approach to writing the function. I prefer to encapsulate everything in a single function to generate the full name. If we go with your approach, we'd need to:

  1. Create function to get <nab-name>-<nab-namespace>- with additional parameter of max length (we do not know if UUID will always have static length)
  2. Make logic to merge above with generated UUID
  3. Make logic to retry if the name exists inside reconcile

With current implementation we skip entirely point 2 making it easier within reconcile.

Copy link
Contributor

Choose a reason for hiding this comment

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

from this https://pkg.go.dev/github.com/google/uuid#Parse it is always 36 chars length

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Correct if we take RFC 4122, then is always 36 but really it should not be split between two functions to create string.

veleroBackupName = namespace + constant.NameDelimiter + veleroBackupName
}

// Ensure the name is within the character limit
maxLength := validation.DNS1123SubdomainMaxLength

if len(veleroBackupName) > maxLength {
// Calculate remaining length after UUID
remainingLength := maxLength - len(uuidSuffix)

delimeterLength := len(constant.NameDelimiter)

// Subtract two delimiter lengths to avoid a corner case where the namespace
// and delimiters leave no space for any part of nabName
if len(namespace) > remainingLength-delimeterLength-delimeterLength {
namespace = namespace[:remainingLength-delimeterLength-delimeterLength]
veleroBackupName = namespace + constant.NameDelimiter + uuidSuffix
} else {
remainingLength = remainingLength - len(namespace) - delimeterLength - delimeterLength
nabName = nabName[:remainingLength]
veleroBackupName = uuidSuffix
if len(nabName) > 0 {
veleroBackupName = nabName + constant.NameDelimiter + veleroBackupName
}
if len(namespace) > 0 {
veleroBackupName = namespace + constant.NameDelimiter + veleroBackupName
}
}
}

return veleroBackupName
}

// CheckVeleroBackupMetadata return true if Velero Backup object has required Non Admin labels and annotations, false otherwise
func CheckVeleroBackupMetadata(obj client.Object) bool {
labels := obj.GetLabels()
Expand Down
77 changes: 77 additions & 0 deletions internal/common/function/function_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -23,6 +23,7 @@ import (
"strings"
"testing"

"github.com/google/uuid"
"github.com/onsi/ginkgo/v2"
"github.com/stretchr/testify/assert"
velerov1 "github.com/vmware-tanzu/velero/pkg/apis/velero/v1"
Expand Down Expand Up @@ -287,6 +288,82 @@ func TestGenerateVeleroBackupName(t *testing.T) {
}
}

func TestGenerateVeleroBackupNameWithUUID(t *testing.T) {
tests := []struct {
name string
namespace string
nabName string
}{
{
name: "Valid names without truncation",
namespace: "default",
nabName: "my-backup",
},
{
name: "Truncate nabName due to length",
namespace: "some",
nabName: strings.Repeat("q", validation.DNS1123SubdomainMaxLength+10), // too long for DNS limit
},
{
name: "Truncate very long namespace and very long name",
namespace: strings.Repeat("w", validation.DNS1123SubdomainMaxLength+10),
nabName: strings.Repeat("e", validation.DNS1123SubdomainMaxLength+10),
},
{
name: "nabName empty",
namespace: "example",
nabName: constant.EmptyString,
},
{
name: "namespace empty",
namespace: constant.EmptyString,
nabName: "my-backup",
},
{
name: "very long name and namespace empty",
namespace: constant.EmptyString,
nabName: strings.Repeat("r", validation.DNS1123SubdomainMaxLength+10),
},
{
name: "very long namespace and name empty",
namespace: strings.Repeat("t", validation.DNS1123SubdomainMaxLength+10),
nabName: constant.EmptyString,
},
{
name: "empty namespace and empty name",
namespace: constant.EmptyString,
nabName: constant.EmptyString,
},
}

for _, tt := range tests {
t.Run(tt.name, func(t *testing.T) {
result := GenerateVeleroBackupNameWithUUID(tt.namespace, tt.nabName)

// Check length
if len(result) > validation.DNS1123SubdomainMaxLength {
t.Errorf("Generated name is too long: %s", result)
}

// Extract the last 36 characters, which should be the UUID
if len(result) < 36 {
t.Errorf("Generated name is too short to contain a valid UUID: %s", result)
}
uuidPart := result[len(result)-36:] // The UUID is always the last 36 characters

// Attempt to parse the UUID part
if _, err := uuid.Parse(uuidPart); err != nil {
t.Errorf("Last part is not a valid UUID: %s", uuidPart)
}

// Check if no double hyphens are present
if strings.Contains(result, "--") {
t.Errorf("Generated name contains double hyphens: %s", result)
}
})
}
}

func TestGetNonAdminBackupFromVeleroBackup(t *testing.T) {
log := zap.New(zap.UseDevMode(true))
ctx := context.Background()
Expand Down
Loading