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

Internal: Replaced hardcoded UUIDs with randomly generated UUIDs #560

Merged
merged 4 commits into from
Jun 29, 2018
Merged

Internal: Replaced hardcoded UUIDs with randomly generated UUIDs #560

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
bdf297b
doc/development.md: add uuid doc
Rahuls0720 Jun 20, 2018
b73d7ab
tests: add uuid variable parser
Rahuls0720 Jun 20, 2018
3b3e40f
tests/bb: replace hardcoded uuid with uuid variables
Rahuls0720 Jun 20, 2018
e8ef14d
vendor: updates from glide
Rahuls0720 Jun 9, 2018
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: 2 additions & 1 deletion doc/development.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -72,7 +72,6 @@ The following packages are required by the Blackbox Test:
* `e2fsprogs`
* `btrfs-progs`
* `xfsprogs`
* `uuid-runtime`
* `gdisk`
* `coreutils`
* `mdadm`
Expand All @@ -97,6 +96,8 @@ Config: `string`

The test should be added to the init function inside of the test file. If the test module is being created then an `init` function should be created which registers the tests and the package must be imported inside of `tests/registry/registry.go` to allow for discovery.

UUIDs may be required in the following fields of a Test object: In, Out, and Config. Replace all GUIDs with GUID varaibles which take on the format `$uuid<num>` (e.g. $uuid123). Where `<num>` must be a positive integer. GUID variables with identical `<num>` fields will be replaced with identical GUIDs. For example, look at [tests/positive/partitions/zeros.go](https://github.com/coreos/ignition/blob/master/tests/positive/partitions/zeros.go).

## Marking an experimental spec as stable

When an experimental version of the Ignition config spec (e.g.: `2.3.0-experimental`) is to be declared stable (e.g. `2.3.0`), there are a handful of changes that must be made to the code base. These changes should have the following effects:
Expand Down
9 changes: 6 additions & 3 deletions glide.lock
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

2 changes: 2 additions & 0 deletions glide.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -39,5 +39,7 @@ import:
testImport:
- package: github.com/stretchr/testify
version: 4d4bfba8f1d1027c4fdbe371823030df51419987
- package: github.com/pborman/uuid
version: v1.1
subpackages:
- assert
74 changes: 73 additions & 1 deletion tests/blackbox_test.go
View file
Open in desktop
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 •
edited
Loading

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
View file
Open in desktop
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
28 changes: 14 additions & 14 deletions tests/positive/filesystems/reformat_filesystem.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -49,7 +49,7 @@ func ReformatToBTRFS_2_0_0() types.Test {
"format": "btrfs",
"create": {
"force": true,
"options": [ "--label=OEM", "--uuid=CA7D7CCB-63ED-4C53-861C-1742536059D4" ]
"options": [ "--label=OEM", "--uuid=$uuid0" ]
}
}
}]
Expand Down Expand Up @@ -85,7 +85,7 @@ func ReformatToXFS_2_0_0() types.Test {
"format": "xfs",
"create": {
"force": true,
"options": [ "-L", "OEM", "-m", "uuid=CA7D7CCB-63ED-4C53-861C-1742536059CD" ]
"options": [ "-L", "OEM", "-m", "uuid=$uuid0" ]
}
}
}]
Expand Down Expand Up @@ -121,14 +121,14 @@ func ReformatToVFAT_2_0_0() types.Test {
"format": "vfat",
"create": {
"force": true,
"options": [ "-n", "OEM", "-i", "CA7D7CCB-63ED-4C53-861C-1742536059CE" ]
"options": [ "-n", "OEM", "-i", "$uuid0" ]
}
}
}]
}
}`
out[0].Partitions.GetPartition("OEM").FilesystemType = "vfat"
out[0].Partitions.GetPartition("OEM").FilesystemUUID = "CA7D7CCB-63ED-4C53-861C-1742536059CE"
out[0].Partitions.GetPartition("OEM").FilesystemUUID = "$uuid0"

return types.Test{
Name: name,
Expand Down Expand Up @@ -158,15 +158,15 @@ func ReformatToEXT4_2_0_0() types.Test {
"format": "ext4",
"create": {
"force": true,
"options": [ "-L", "OEM", "-U", "CA7D7CCB-63ED-4C53-861C-1742536059CF" ]
"options": [ "-L", "OEM", "-U", "$uuid0" ]
}
}
}]
}
}`
in[0].Partitions.GetPartition("OEM").FilesystemType = "ext2"
out[0].Partitions.GetPartition("OEM").FilesystemType = "ext4"
out[0].Partitions.GetPartition("OEM").FilesystemUUID = "CA7D7CCB-63ED-4C53-861C-1742536059CF"
out[0].Partitions.GetPartition("OEM").FilesystemUUID = "$uuid0"

return types.Test{
Name: name,
Expand Down Expand Up @@ -195,14 +195,14 @@ func ReformatToBTRFS_2_1_0() types.Test {
"device": "$DEVICE",
"format": "btrfs",
"label": "OEM",
"uuid": "CA7D7CCB-63ED-4C53-861C-1742536059D0",
"uuid": "$uuid0",
"wipeFilesystem": true
}
}]
}
}`
out[0].Partitions.GetPartition("OEM").FilesystemType = "btrfs"
out[0].Partitions.GetPartition("OEM").FilesystemUUID = "CA7D7CCB-63ED-4C53-861C-1742536059D0"
out[0].Partitions.GetPartition("OEM").FilesystemUUID = "$uuid0"

return types.Test{
Name: name,
Expand Down Expand Up @@ -231,14 +231,14 @@ func ReformatToXFS_2_1_0() types.Test {
"device": "$DEVICE",
"format": "xfs",
"label": "OEM",
"uuid": "CA7D7CCB-63ED-4C53-861C-1742536059D1",
"uuid": "$uuid0",
"wipeFilesystem": true
}
}]
}
}`
out[0].Partitions.GetPartition("OEM").FilesystemType = "xfs"
out[0].Partitions.GetPartition("OEM").FilesystemUUID = "CA7D7CCB-63ED-4C53-861C-1742536059D1"
out[0].Partitions.GetPartition("OEM").FilesystemUUID = "$uuid0"

return types.Test{
Name: name,
Expand Down Expand Up @@ -303,15 +303,15 @@ func ReformatToEXT4_2_1_0() types.Test {
"device": "$DEVICE",
"format": "ext4",
"label": "OEM",
"uuid": "CA7D7CCB-63ED-4C53-861C-1742536059D2",
"uuid": "$uuid0",
"wipeFilesystem": true
}
}]
}
}`
in[0].Partitions.GetPartition("OEM").FilesystemType = "ext2"
out[0].Partitions.GetPartition("OEM").FilesystemType = "ext4"
out[0].Partitions.GetPartition("OEM").FilesystemUUID = "CA7D7CCB-63ED-4C53-861C-1742536059D2"
out[0].Partitions.GetPartition("OEM").FilesystemUUID = "$uuid0"

return types.Test{
Name: name,
Expand Down Expand Up @@ -340,15 +340,15 @@ func ReformatToSWAP_2_1_0() types.Test {
"device": "$DEVICE",
"format": "swap",
"label": "OEM",
"uuid": "CA7D7CCB-63ED-4C53-861C-1742536059D3",
"uuid": "$uuid0",
"wipeFilesystem": true
}
}]
}
}`
in[0].Partitions.GetPartition("OEM").FilesystemType = "ext2"
out[0].Partitions.GetPartition("OEM").FilesystemType = "swap"
out[0].Partitions.GetPartition("OEM").FilesystemUUID = "CA7D7CCB-63ED-4C53-861C-1742536059D3"
out[0].Partitions.GetPartition("OEM").FilesystemUUID = "$uuid0"

return types.Test{
Name: name,
Expand Down
6 changes: 3 additions & 3 deletions tests/positive/filesystems/reuse_filesystem.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -43,7 +43,7 @@ func ReuseExistingFilesystem() types.Test {
"wipeFilesystem": false,
"format": "xfs",
"label": "data",
"uuid": "8A7A6E26-5E8F-4CCA-A654-46215D4696AC"
"uuid": "$uuid0"
}
}
]
Expand All @@ -58,7 +58,7 @@ func ReuseExistingFilesystem() types.Test {
Length: 2621440,
FilesystemType: "xfs",
FilesystemLabel: "data",
FilesystemUUID: "8A7A6E26-5E8F-4CCA-A654-46215D4696AC",
FilesystemUUID: "$uuid0",
Files: []types.File{
{
Node: types.Node{
Expand All @@ -80,7 +80,7 @@ func ReuseExistingFilesystem() types.Test {
Length: 2621440,
FilesystemType: "xfs",
FilesystemLabel: "data",
FilesystemUUID: "8A7A6E26-5E8F-4CCA-A654-46215D4696AC",
FilesystemUUID: "$uuid0",
Files: []types.File{
{
Node: types.Node{
Expand Down
Loading