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

Internal: Replaced hardcoded UUIDs with randomly generated UUIDs #560

Merged
merged 4 commits into from
Jun 29, 2018
Merged
Show file tree
Hide file tree
Changes from 1 commit
Commits
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
74 changes: 73 additions & 1 deletion tests/blackbox_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -20,6 +20,7 @@ import (
"io/ioutil"
"os"
"path/filepath"
"regexp"
"strings"
"testing"
"time"
Expand All @@ -30,6 +31,9 @@ import (

// Register the tests
_ "github.com/coreos/ignition/tests/registry"

// UUID generation tool
"github.com/pborman/uuid"
)

var (
Expand Down Expand Up @@ -76,6 +80,9 @@ func TestIgnitionBlackBoxNegative(t *testing.T) {
func outer(t *testing.T, test types.Test, negativeTests bool) error {
t.Log(test.Name)

// Maps $uuid<num> such that two variables with the same <num> have identical UUID
UUIDmap := make(map[string]string)

ctx, cancelFunc := context.WithDeadline(context.Background(), time.Now().Add(testTimeout))
defer cancelFunc()

Expand Down Expand Up @@ -123,7 +130,7 @@ func outer(t *testing.T, test types.Test, negativeTests bool) error {
// Finish data setup
for _, part := range disk.Partitions {
if part.GUID == "" {
part.GUID, err = generateUUID(t, ctx)
part.GUID = uuid.New()
if err != nil {
return err
}
Expand All @@ -143,6 +150,46 @@ func outer(t *testing.T, test types.Test, negativeTests bool) error {
}
test.Out[i].SetOffsets()

// Replace all UUID variables (format $uuid<num>) in configs and partitions with an UUID
test.Config, err = replaceUUIDVars(t, test.Config, UUIDmap)
if err != nil {
return err
}

for _, disk := range test.In {
for _, partition := range disk.Partitions {
partition.TypeGUID, err = replaceUUIDVars(t, partition.TypeGUID, UUIDmap)
Copy link
Contributor

Choose a reason for hiding this comment

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

Why are we running replace against TypeGUID?

Copy link
Contributor

Choose a reason for hiding this comment

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

Why not? As far as Ignition is concerned its just another thing that can match or not match.

Copy link
Contributor

@arithx arithx Jun 22, 2018

Choose a reason for hiding this comment

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

My understanding of the original driving force that led to this PR was that we wanted to eliminate duplicate UUIDs in fields where having non-unique entries across multiple tests could lead to parallelism issues.

I'm not particularly against doing this but I don't think it really provides much if any value to do so.

Copy link
Contributor

Choose a reason for hiding this comment

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

It's also to allow you to say "parition N should look like this, fix it if it doesnt" and type guids are just as much a part of that. Ignoring what this might be used for, its still a code path in Ignition that should get testing.

Copy link
Contributor

Choose a reason for hiding this comment

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

The code path is still being tested regardless of if the UUID is generated or not.

I will concede that it doesn't particularly matter what the TypeGUID is in most cases though and for ease of writing having the functionality applied makes sense.

Copy link
Contributor

Choose a reason for hiding this comment

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

Oh, yeah it doesn't really matter. The $uuidN is just much easier to read/write. If we really wanted to mock out real world use cases we could include all of the special type guid (e.g. coreos root on raid) in the templating function.

if err != nil {
return err
}
partition.GUID, err = replaceUUIDVars(t, partition.GUID, UUIDmap)
if err != nil {
return err
}
partition.FilesystemUUID, err = replaceUUIDVars(t, partition.FilesystemUUID, UUIDmap)
if err != nil {
return err
}
}
}

for _, disk := range test.Out {
for _, partition := range disk.Partitions {
partition.TypeGUID, err = replaceUUIDVars(t, partition.TypeGUID, UUIDmap)
if err != nil {
return err
}
partition.GUID, err = replaceUUIDVars(t, partition.GUID, UUIDmap)
if err != nil {
return err
}
partition.FilesystemUUID, err = replaceUUIDVars(t, partition.FilesystemUUID, UUIDmap)
if err != nil {
return err
}
}
}

// Creation
err = createVolume(t, ctx, tmpDirectory, i, disk.ImageFile, imageSize, disk.Partitions)
// Move value into the local scope, because disk.ImageFile will change
Expand Down Expand Up @@ -311,3 +358,28 @@ func outer(t *testing.T, test types.Test, negativeTests bool) error {
}
return nil
}

// Identify and replace $uuid<num> with correct UUID
// Variables with matching <num> should have identical UUIDs
func replaceUUIDVars(t *testing.T, str string, UUIDmap map[string]string) (string, error) {
finalStr := str

pattern := regexp.MustCompile("\\$uuid([0-9]+)")
Copy link
Contributor

Choose a reason for hiding this comment

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

We technically don't need to have a separate capture group for the digits anymore as we're not directly matching them. This could be shortened to \\$uuid[0-9]+

Copy link
Contributor

Choose a reason for hiding this comment

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

eh, I'm fine with it. doesn't much matter either way.

for _, match := range pattern.FindAllStringSubmatch(str, -1) {
if len(match) != 2 {
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't think this check is necessary. The regex only has 2 capture groups (both of which must be present for it to be returned by FindAllStringSubmatch afaik).

Copy link
Contributor

Choose a reason for hiding this comment

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

I'd prefer to have it. Doesn't hurt and might prevent us from shooting outselves in the foot later on.

Copy link
Contributor

Choose a reason for hiding this comment

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

Fair enough, this one I mostly brought up as if we end up switching the UUIDmap as mentioned in the other comment dropping this as well would allow us to no longer need to return an error in the replaceUUIDVars function.

Copy link
Contributor

Choose a reason for hiding this comment

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

meh, error handling is fine.

return str, fmt.Errorf("find all string submatch error: want length of 2, got length of %d", len(match))
}

finalStr = strings.Replace(finalStr, match[0], getUUID(match[0], UUIDmap), 1)
}
return finalStr, nil
}

// Format: $uuid<num> where the uuid variable (uuid<num>) is the key
// value is the UUID for this uuid variable
func getUUID(key string, UUIDmap map[string]string) string {
if _, ok := UUIDmap[key]; !ok {
UUIDmap[key] = uuid.New()
}
return UUIDmap[key]
}
8 changes: 0 additions & 8 deletions tests/filesystem.go
Original file line number Diff line number Diff line change
Expand Up @@ -376,14 +376,6 @@ func removeEmpty(strings []string) []string {
return r
}

func generateUUID(t *testing.T, ctx context.Context) (string, error) {
out, err := run(t, ctx, "uuidgen")
if err != nil {
return "", err
}
return strings.TrimSpace(string(out)), nil
}

func createFilesForPartitions(t *testing.T, partitions []*types.Partition) error {
for _, partition := range partitions {
err := createDirectoriesFromSlice(t, partition.MountPath, partition.Directories)
Expand Down