add use_internal_ip and omit_external_ip fields to export post-processor

[cmgsj]

After trying to use the googlecompute-export post-processor, it fails when provisioning the exporter instance because it violates the organization policy constraints/compute.vmExternalIpAccess.

This is the packer code used:

source "googlecompute" "vm" {
  // ...
}

build {
  name = "main"
  sources = ["sources.googlecompute.vm"]
  // ...
  post-processor "googlecompute-export" {
    paths = [
      "gs://$BUCKET_NAME/$IMAGE_NAME.tar.gz",
    ]
    keep_input_artifact = true
  }
}

and a fragment of the build logs:

==> main.googlecompute.vm: Running post-processor: (type googlecompute-export)
==> main.googlecompute.vm (googlecompute-export): Exporting image $IMAGE_NAME to destination: [gs://$BUCKET_NAME/$IMAGE_NAME.tar.gz]
==> main.googlecompute.vm (googlecompute-export): Creating temporary RSA SSH key for instance...
==> main.googlecompute.vm (googlecompute-export): Using image: debian-9-worker-v20200616
==> main.googlecompute.vm (googlecompute-export): Creating instance...
    main.googlecompute.vm (googlecompute-export): Loading zone: $ZONE
    main.googlecompute.vm (googlecompute-export): Loading machine type: n1-highcpu-4
    main.googlecompute.vm (googlecompute-export): Requesting instance creation...
==> main.googlecompute.vm (googlecompute-export): Error creating instance: googleapi: Error 412: Constraint constraints/compute.vmExternalIpAccess violated for project $PROJECT_NUMBER. Add instance projects/$PROJECT_ID/zones/$ZONE/instances/$IMAGE_NAME-exporter to the constraint to use external IP with it., conditionNotMet

This happens because the provisioned export instance always has the fields omit_external_ip and use_internal_ip set to false, with no way to overwrite them.

This PR attempts to change that behavior, by exposing the aforementioned fields in the googlecompute-export post-processor config and then using them in the googlecompute.Config of the export instance.

Example:

source "googlecompute" "vm" {
  // ...
}

build {
  name = "main"
  sources = ["sources.googlecompute.vm"]
  // ...
  post-processor "googlecompute-export" {
    paths = [
      "gs://$BUCKET_NAME/$IMAGE_NAME.tar.gz",
    ]
    keep_input_artifact = true
    omit_external_ip    = true
    use_internal_ip     = true
  }
}

Please let me know if a way to circumvent this issue already exists. Any other feedback is welcome as well 🙂!

[hashicorp-cla]

All committers have signed the CLA.

[nywilken]

Hi there apologies for the delayed review here. Our focus has been on Packer itself trying to resolve issues with plugin loading. Looking at the changes and reference issue I can understand the need for this feature.

I left a few suggestions on the change based on my understanding on how Omit and Use internal are working. Please let me know if you have any questions or if my understanding is off.

[cmgsj]

@nywilken thank you so much for taking the time to review!

I initially thought of adding both fields use_internal_ip and omit_external_ip in order to be consistent with the fields the googlecompute source exposes.

I have identified three possible successful combinations of the above fields according to the following truth table:

case	use_internal_ip	omit_external_ip	result
1	true	true	✅ internal IP is used, instance does not have an external IP
2	true	false	✅ internal IP is used, instance has an external IP
3	false	true	❌ cannot use external IP because instance does not have one
4	false	false	✅ external IP is used, instance has an external IP

All the statements you made above are true except the following two, which are indeed possible, but not necessary:

• When UseInternalIP is true OmitExternalIP must be True.
• OmitExternalIP can not be false when UseInternal is True.

I think you might not have taken case 2 into account.

Having said that, I do think that your suggestion is very reasonable and will prevent any potential "build-time" errors from case 3, although it would also prevent case 2.

In the end I think the disjunction is between allowing all cases (with a the potential error case 3), or allowing only cases 1 and 4 (without the potential success case 2).

Please let me know if you agree, and/or what your suggestion would be, thanks in advance!

[cmgsj]

@nywilken whenever you have some time, please let me know if my comment above makes more sense, or if you think it would be better to go forward with the changes you had originally suggested.

Thanks.

[cmgsj]

@nywilken I pushed a commit for adding a validation error on case #3 in my comment above.

Please review it when you have the time. Thanks.

[nywilken]

@cmgsj Im no longer working on these PRs. Pinging @lbajolet-hashicorp to continue pushing this forward. Please keep in mind that his response may be slow as they work on different priorities.

[cmgsj]

Hi @lbajolet-hashicorp,

I've submitted this PR with some enhancements to the googlecompute-export post-processor, and I'd appreciate it if you could take a look whenever you have a moment.

Let me know if you have any questions or need additional context. Thanks in advance for your time!

[lbajolet-hashicorp]

Hi @cmgsj,

Sorry for the delay in the review here, I was busy working on other things, we're planning a release for the GCE plugin this week, so I'm going through the opened PRs now to see what we can include in it.

Regarding your change, while I understand the reason to add OmitExternalIP to the googlecompute-export post-processor here, as it's used by the step that creates the temporary instance, the UseInternalIP seems superfluous here.

UseInternalIP is only used by StepInstanceInfo, which while used in the builder for GCE for populating the instance's IP address to use for the remainder of the build steps. It is not used in the export post-processor at all since there's not a lot done in there, so I would think you can remove it, remove the extra check for both attributes set, and amend the comment so that OmitExternalIP is the only attribute added here.

Of course I might be missing something so please let me know if that's the case.

Marking as request changes for now, but hopefully this can be quickly resolved, thanks!

[lbajolet-hashicorp]

Oh, on another note, I see the CLA has not been accepted by your Github account yet, which means we cannot accept the contribution until this is done. Could you do this at the same time so we aren't blocked if/when it comes to merging this PR?

Edit: it seems there are more than one commit, which means that all commits must be linked to a Github account which has accepted the CLA, you can probably rebase and squash those together using the same email address as your account if you have already signed them.

[cmgsj]

@lbajolet-hashicorp thanks for the review!

The changes you suggested make much more sense, I have implemented them.
I realize that UseInternalIP is only used by the builder's StepInstanceInfo.

I also fixed the CLA issue.

[lbajolet-hashicorp]

Hey again @cmgsj,

Looks much simpler indeed :)

With that it seems good to me, we can merge it right away, thanks for the quick reroll, much appreciated!