builder: fix image label sanity checks

[lbajolet-hashicorp]

When the image sanity checks were introduced in v1.1.7, an error slipped in as the same regex was used to validate keys and labels, however the rules are slightly different between the two, as keys are mandated to start with [a-z], while values are not.

This commit addresses this bug by changing to using two regexes, so we can still validate that both are valid before attempting to build the image, but we are not unnecessarily rejecting valid values in the map.

Closes #248

[chrisstaite]

The new regex allows empty label values which is invalid.

[lbajolet-hashicorp]

@chrisstaite that is surprising, according to the GCP docs, it does state that Values can be empty, and have a maximum length of 63 characters., is this not accurate?

[chrisstaite]

You also assume lowercase ASCII (i.e. a-z) whereas the requirements from Google state lowercase letters implying lowercase non-latin characters are also allowed, specifically in UTF-8 encoding.

[chrisstaite]

Absolutely, I misread that.

However: "All characters must use UTF-8 encoding, and international characters are allowed. "

[lbajolet-hashicorp]

You do make a point regarding lowercase letters though, I wonder what is the exact character set they allow there, I assume we can use a Unicode-friendly lowercase collation algorithm for those, but a regexp supporting those is likely tricky.
I can think of another way to do this kind of check, but in the meantime I wonder if reverting the original commit may not be better. It has the undesirable effect of rejecting an image after it's been built, but it's better than preventing builds until then.

I'll amend the PR to do that.

[chrisstaite]

You should be able to use the regex \p{Ll} although I've not tested this

[lbajolet-hashicorp]

I might add that even the length check is somewhat broken in this case, as the length of the string according to their documentation is up-to 63 characters, not 63 bytes, so the length checks are likely broken too here.

[lbajolet-hashicorp]

You are right though, it seems that \p{Ll} might work here, assuming the repetition works on the characters and not bytes (which I would assume is the case), we should be good to use that.
I'll experiment with that a bit, and if I cannot make it work easily, I'll resort to reverting the commit instead.

[lbajolet-hashicorp]

@chrisstaite thanks for the pointer to \p{Ll}, this seems to be working with international lower-case characters now, hopefully this won't be a problem since I worry that between unicode versions there might be incompabtibilities in terms of determining what is a lower-case letter, though those blocks are fairly stable these days and only the astral plan tends to be updated, so I assume this will be good enough in the current state.
If you still have time, might I ask you to test it once more, and give it a quick second review?

[jkoelndorfer]

I encountered #248 today, also. It appears that this validation happens after the image build process.

If this validation is going to be performed by Packer, it would be much more useful for it to happen before the image build starts. If the build process has already completed, at that point you might as well just ask GCP to create the image. If there is an error, GCP's API will say so and that can be reported to the user.

[lbajolet-hashicorp]

@jkoelndorfer this PR performs this check during the Prepare step, which happens before the builder starts, so this is indeed run before anything else. This was the intent with the change that was rolled into v1.1.7 as well, I mistakenly assumed that the check was done during this step, but it was done during the step_create_image, which as you point out, does not prevent the plugin from doing the actual build beforehand, that was my mistake, apologies for this.

If you test this change on your build and an invalid key/value, you will see that the error is immediate now.

[DocEmmetBrown]

Thanks a lot @lbajolet-hashicorp for quickly fixing the bug I introduced 🙏

[mogrogan]

LGTM !