-
Notifications
You must be signed in to change notification settings - Fork 246
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
Conversation
Can one of the admins verify this patch? |
Reformat your commit message for d830bc7 to pass the tailor test. 😄 |
45c367a
to
66bfeb0
Compare
ok to test |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you add a thing to doc/development.md
detailing this functionality? Presumably in the "Writing Blackbox Tests" section.
Also I don't see the glide.yaml
file from the glide changes in this PR.
tests/filesystem.go
Outdated
func generateUUID(t *testing.T) (string, error) { | ||
out := uuid.NewRandom() | ||
|
||
/*out, err := run(t, ctx, "uuidgen") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Drop the commented out code
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Totally missed that hehe.
tests/blackbox_test.go
Outdated
var finalStr string | ||
|
||
tokenizedArr := strings.Split(str, " ") | ||
for _, str := range tokenizedArr { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It looks like this only finds the first $uuid
occurrence in the token. Perhaps running the below logic on the entire input (instead of tokenizing it) repeatedly until it doesn't contain $uuid
would work better?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this should handle all $uuid occurrences since the case of $uuid$uuid should never show up, right?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We could have a config with no whitespace in it, since whitespace isn't needed in JSON. As a contrived example, this is a valid config:
{"ignition":{"version":"2.1.0"},"storage":{"filesystems":[{"mount":{"device":"/dev/sda","wipeFilesystem":false,"format":"btrfs","label":"data","uuid":"8A7A6E26-5E8F-4CCA-A654-46215D4696AC"}},{"mount":{"device":"/dev/sdb","wipeFilesystem":false,"format":"ext4","label":"more-data","uuid":"3e343b34-da88-4bfd-b8c6-673d950d082d"}}]}}
If I'm reading your code right, I think it wouldn't find the $uuid
for the second uuid in there.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could use regexp to find all occurances which might be cleaner
Nit: commit messages should present tense imperitive. E.g. "fix problem with x" not "fixed problem with x" or "fixes problem with x" |
config/v2_2/translate_test.go
Outdated
@@ -161,15 +161,15 @@ func TestTranslateFromV2_1(t *testing.T) { | |||
Size: 100, | |||
Start: 50, | |||
TypeGUID: "HI", | |||
GUID: "4F68BCE3-E8CD-4DB1-96E7-FBCAF984B709", | |||
GUID: "$uuid36", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
unit tests (i.e. tests not under the /tests
directory are not run with the BB test runner, so they wont be replaced unless you modify the end of this function to do it there). I think that would be great, but it's seperable from the bb tests and should be either another commit or another PR.
tests/filesystem.go
Outdated
return "", err | ||
} | ||
return strings.TrimSpace(string(out)), nil | ||
func generateUUID(t *testing.T) (string, error) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can remove the t
argument
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actually this entire function can be dropped in favor of https://godoc.org/github.com/pborman/uuid#New
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In the future, I'd say do the replacement for all calls to this in a seperate commit, but it'd be hard to seperate now and is pretty minor, so I'd leave it for now.
@@ -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=$uuid12" ] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
since each test has its own UUID pool, we should keep the N in $uuidN
as low as possible so it's easy to keep track of it.
tests/blackbox_test.go
Outdated
var finalStr string | ||
|
||
tokenizedArr := strings.Split(str, " ") | ||
for _, str := range tokenizedArr { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could use regexp to find all occurances which might be cleaner
21558e1
to
44619e5
Compare
tests/blackbox_test.go
Outdated
@@ -17,12 +17,15 @@ package blackbox | |||
import ( | |||
"context" | |||
"fmt" | |||
"github.com/pborman/uuid" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nit: this should go after the sections break.
@Rahuls0720 sorry I'm a bit out of the loop, but I'm missing the reason behind this PR (not arguing against it, just looking for more background). Can you please add a couple of words in the first commit to explain why we are doing this uuid replacement? And if you have references to other tickets/bugs where this is coming from, please add them as well, thanks :) |
ccd0b34
to
0c7029b
Compare
I believe I have addressed all changes proposed thus far. @lucab I updated and split out my commits. Sorry about the lack of detail in my original commits. @dgonyeo I am not sure why my glide.yaml did not differ. I ran the make file but only got changes in my glide.lock file (and the addition of pborman's code for the uuid tool). |
tests/blackbox_test.go
Outdated
func replaceUUIDVars(str string, UUIDmap map[int]string) string { | ||
finalStr := str | ||
|
||
tokenizedArr := regexp.MustCompile("[$][u][u][i][d][0-9]+").FindAllStringSubmatchIndex(str, -1) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You can improve the regexp which will also allow you to clean up the code a bit:
pattern := regexp.MustCompile("\\$uuid([0-9]+)")
for _, match := range pattern.FindAllStringSubmatch(str, -1) {
num_int, _ := strconv.Atoi(match[1])
finalStr = strings.Replace(finalStr, match[0], getUUID(num_int, UUIDmap), 1)
}
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Totally missed this function when reading the doc. Nice check!!
@Rahuls0720 can you add |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM. One nit: we typically put glide.yaml changes in a separate commit so it's easy to see as opposed to it being tossed in with generated changes.
4870fd7
to
b6795a6
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Haven't gone through all the tests yet for minor things like typos. LGTM other than the nits I suggested. Can you please move the docs changes into a seperate commit and the changes to the tests themselves (not the test runner) into their own commit?
tests/blackbox_test.go
Outdated
@@ -123,7 +131,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 = strings.TrimSpace(uuid.New()) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
does uuid.New generate spaces?
tests/blackbox_test.go
Outdated
|
||
//Identify and replace $uuid<num> with correct UUID | ||
//Variables with matching <num> should have identical UUIDs | ||
func replaceUUIDVars(str string, UUIDmap map[int]string) string { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
haha nvm lets do this after we refactor the bb tests
tests/blackbox_test.go
Outdated
|
||
pattern := regexp.MustCompile("\\$uuid([0-9]+)") | ||
for _, match := range pattern.FindAllStringSubmatch(str, -1) { | ||
num_int, _ := strconv.Atoi(match[1]) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Handle this error. Take in the testing.T
and t.Fatal
if you encounter it. Best to have it fail hard than silently succeed wrongly.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
you probably also want to verify len(match) == 2
and fail out if there's anything unexpected.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Rather than having this t.Fatal
the function should return the error. That'll be more in line with the rest of the package.
tests/blackbox_test.go
Outdated
// Format: $uuid<num> where num is key; value is uuid | ||
func getUUID(num int, UUIDmap map[int]string) string { | ||
if _, ok := UUIDmap[num]; !ok { | ||
UUIDmap[num] = strings.TrimSpace(uuid.New()) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
same thing about the TrimSpace. I glanced at the uuid implementation and it doesn't look needed.
doc/development.md
Outdated
@@ -97,6 +97,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. | |||
|
|||
A GUID 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 number. GUID variables with identical `<num>` fields will be replaced with identical GUIDs. As an example, look at the following file: tests/positive/partitions/zeros.go. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
s/A GUID/UUIDs
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
s/positive number/positive integer
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
s/As an example.../For example, look at [test/.../zeroes.go](link to file on github)
01927cf
to
1b322ac
Compare
tests/blackbox_test.go
Outdated
pattern := regexp.MustCompile("\\$uuid([0-9]+)") | ||
for _, match := range pattern.FindAllStringSubmatch(str, -1) { | ||
if len(match) != 2 { | ||
t.Fatalf("find all string submatch error: want length of 2, got length of %d", len(match)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Rather than doing Fatalf
's inside this function return the error and handle it inside of the outer
function.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
NVM, I need to fix a few things.
|
tests/blackbox_test.go
Outdated
|
||
num_int, err := strconv.Atoi(match[1]) | ||
if err != nil { | ||
return str, fmt.Errorf("atoi failed: %v\n%s", err, num_int) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
should be %d or %v. Travis is failing because of this
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Updated.
ae12853
to
704f6c2
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would go through and double check all the tests/positive files are make sure the uuids are all
- assigned in the same order (e.g. you could start with the config and number them going up, or start in the In/Out structs). Just be consistent
- ensures no
in
out
orconfig
has duplicate uuids (typeGuids are ok to have dups)
@@ -43,7 +43,7 @@ func ReuseExistingFilesystem() types.Test { | |||
"wipeFilesystem": false, | |||
"format": "xfs", | |||
"label": "data", | |||
"uuid": "8A7A6E26-5E8F-4CCA-A654-46215D4696AC" | |||
"uuid": "$uuid2" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
s/2/0
@@ -58,7 +58,7 @@ func ReuseExistingFilesystem() types.Test { | |||
Length: 2621440, | |||
FilesystemType: "xfs", | |||
FilesystemLabel: "data", | |||
FilesystemUUID: "8A7A6E26-5E8F-4CCA-A654-46215D4696AC", | |||
FilesystemUUID: "$uuid2", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
s/2/0
@@ -80,7 +80,7 @@ func ReuseExistingFilesystem() types.Test { | |||
Length: 2621440, | |||
FilesystemType: "xfs", | |||
FilesystemLabel: "data", | |||
FilesystemUUID: "8A7A6E26-5E8F-4CCA-A654-46215D4696AC", | |||
FilesystemUUID: "$uuid2", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
s/2/0
"typeGuid": "B921B045-1DF0-41C3-AF44-4C6F280D3FAE", | ||
"guid": "8A7A6E26-5E8F-4CCA-A654-46215D4696AC" | ||
"typeGuid": "$uuid0", | ||
"guid": "$uuid2" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
why skip $uuid1
?
@@ -262,7 +262,7 @@ func ResizeRoot() types.Test { | |||
"number": 9, | |||
"size": 13008896, | |||
"typeGuid": "3884DD41-8582-4404-B9A8-E9B84F2DF50E", | |||
"guid": "3ED3993F-0016-422B-B134-09FCBA6F66EF", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
missed typeguid
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This typeGuid is special. I did not replace any guids that match:
"coreos-resize": "3884DD41-8582-4404-B9A8-E9B84F2DF50E",
"data": "0FC63DAF-8483-4772-8E79-3D69D8477DE4",
"coreos-rootfs": "5DFBF5F4-2848-4BAC-AA5E-0D9A20B745A6",
"bios": "21686148-6449-6E6F-744E-656564454649",
"efi": "C12A7328-F81F-11D2-BA4B-00A0C93EC93B",
"coreos-reserved": "C95DC21A-DF0E-4340-8D7B-26CBFA9A03E0"
}, | ||
{ | ||
Label: "ephemeral-data", | ||
Number: 2, | ||
Length: 65536, | ||
TypeGUID: "CA7D7CCB-63ED-4C53-861C-1742536059CC", | ||
GUID: "B921B045-1DF0-41C3-AF44-4C6F280D3FAE", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
we shouldn't have duplicate guids ever. This was a bug beforehand, but we should fix it in this PR
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed this and all above comments related to fixing UUID variables.
|
||
for _, disk := range test.In { | ||
for _, partition := range disk.Partitions { | ||
partition.TypeGUID, err = replaceUUIDVars(t, partition.TypeGUID, UUIDmap) |
There was a problem hiding this comment.
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
?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
"typeGuid": "B921B045-1DF0-41C3-AF44-4C6F280D3FAE", | ||
"guid": "8A7A6E26-5E8F-4CCA-A654-46215D4696AC" | ||
"typeGuid": "$uuid0", | ||
"guid": "$uuid1" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
see comment at the top of the test. This needs to be different than the input uuid
tests/positive/partitions/mixed.go
Outdated
@@ -154,13 +154,13 @@ func NothingMatches() types.Test { | |||
Label: "important-data", | |||
Number: 1, | |||
Length: 65536, | |||
TypeGUID: "0921B045-1DF0-41C3-AF44-4C6F280D3FAE", | |||
TypeGUID: "$uuid0", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this needs to be different from the input.
}, | ||
{ | ||
Label: "ephemeral-data", | ||
Number: 2, | ||
Length: 65536, | ||
GUID: "0921B045-1DF0-41C3-AF44-4C6F280D3FAE", | ||
GUID: "$uuid1", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
same here
@@ -43,8 +43,8 @@ func KitchenSink() types.Test { | |||
"number": 1, | |||
"start": 2048, | |||
"size": 65536, | |||
"typeGuid": "316f19f9-9e0f-431e-859e-ae6908dbe8ca", | |||
"guid": "53f2e871-f468-437c-b90d-f3c6409df81a", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There are a lot of guids/type guids that used to change but now do not. Can you make sure all partitions that used to change guids/typeguids between the input and output still do?
retest this please |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM with or without these comments being addressed. Just wanted to point them out.
tests/blackbox_test.go
Outdated
@@ -76,6 +81,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 such variables with the same <num> have identical UUID | |||
UUIDmap := make(map[int]string) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If you convert UUIDmap
to a map[string]string
you can drop the strconv.Atoi
and just make the keys $uuid<num>
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍 good catch
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I updated the code to use map[string]string. Can some review the code one last time?
|
||
pattern := regexp.MustCompile("\\$uuid([0-9]+)") | ||
for _, match := range pattern.FindAllStringSubmatch(str, -1) { | ||
if len(match) != 2 { |
There was a problem hiding this comment.
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).
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
Add ability to short hand uuid to $uuid<n> in blackbox tests. This makes it easier to write blackbox test without having to copy and paste specific uuids through out your test code.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
func replaceUUIDVars(t *testing.T, str string, UUIDmap map[string]string) (string, error) { | ||
finalStr := str | ||
|
||
pattern := regexp.MustCompile("\\$uuid([0-9]+)") |
There was a problem hiding this comment.
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]+
There was a problem hiding this comment.
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.
func replaceUUIDVars(t *testing.T, str string, UUIDmap map[string]string) (string, error) { | ||
finalStr := str | ||
|
||
pattern := regexp.MustCompile("\\$uuid([0-9]+)") |
There was a problem hiding this comment.
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.
Hardcoded UUIDs are now replaced with UUID variables of the format $uuid (ex. $uuid0). Variables with the same are updated with the same UUID once the tests run. The method of UUID generation is updated from shelling out to now using Pborman UUID tool (https://github.com/pborman/uuid).
This has been done in an effort to make reading and writing tests easier.