From 1b3eb1c34dca0c6b2b4f82914e6b2171de1fc5ee Mon Sep 17 00:00:00 2001 From: Evan Brown Date: Wed, 13 Sep 2017 14:12:03 -0700 Subject: [PATCH 1/5] builder/googlecompute: Set default network_project_id If network_project_id is not specified in the GCE builder config, it should default to the project_id. --- builder/googlecompute/config.go | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/builder/googlecompute/config.go b/builder/googlecompute/config.go index 9605f2f4c..b09f1814e 100644 --- a/builder/googlecompute/config.go +++ b/builder/googlecompute/config.go @@ -86,6 +86,10 @@ func NewConfig(raws ...interface{}) (*Config, []string, error) { c.Network = "default" } + if c.NetworkProjectId == "" { + c.NetworkProjectId = c.ProjectId + } + if c.DiskSizeGb == 0 { c.DiskSizeGb = 10 } From bada7b73c1c235df53a833a68cd63747c52ac26e Mon Sep 17 00:00:00 2001 From: Evan Brown Date: Wed, 13 Sep 2017 14:13:47 -0700 Subject: [PATCH 2/5] builder/googlecompute: Selectively set default network If a network is not specified, it should only be set to "default" if a subnetwork is also not specified. --- builder/googlecompute/config.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/builder/googlecompute/config.go b/builder/googlecompute/config.go index b09f1814e..8ed3897d6 100644 --- a/builder/googlecompute/config.go +++ b/builder/googlecompute/config.go @@ -82,7 +82,7 @@ func NewConfig(raws ...interface{}) (*Config, []string, error) { var errs *packer.MultiError // Set defaults. - if c.Network == "" { + if c.Network == "" && c.Subnetwork == "" { c.Network = "default" } From f2fed94a715433b25ee0dfd4efc6e253ff9ee0bd Mon Sep 17 00:00:00 2001 From: Evan Brown Date: Wed, 13 Sep 2017 14:15:39 -0700 Subject: [PATCH 3/5] builder/googlecompute: Derive network and subnetwork IDs locally This change constructs partial URLs for networks and subnetworks if they are not already partial or full URLs (i.e., they do not contain a '/' in their name). Network and subnetwork self-links are no longer retrieved from the API. Previously, if a user did not provide the network or subnetwork as a fully-qualified URL (i.e., self-link), the builder would make compute.(sub)networks.get API calls with the provided identifier to discover the self-link. This requires the user or service account Packer is using to have permission to describe those network resources, which is becoming less common as IAM is used more. Specifically, a user may have permission to launch a VM into a network/subnetwork, but will not have permission to call APIs to describe network resources. --- builder/googlecompute/driver_gce.go | 92 ++++++++++++++--------------- 1 file changed, 45 insertions(+), 47 deletions(-) diff --git a/builder/googlecompute/driver_gce.go b/builder/googlecompute/driver_gce.go index 02a230cfe..e767ff6ce 100644 --- a/builder/googlecompute/driver_gce.go +++ b/builder/googlecompute/driver_gce.go @@ -10,7 +10,6 @@ import ( "fmt" "log" "net/http" - "net/url" "runtime" "strings" "time" @@ -299,55 +298,54 @@ func (d *driverGCE) RunInstance(c *InstanceConfig) (<-chan error, error) { } // TODO(mitchellh): deprecation warnings - networkSelfLink := "" - subnetworkSelfLink := "" + networkId := "" + subnetworkId := "" - if u, err := url.Parse(c.Network); err == nil && (u.Scheme == "https" || u.Scheme == "http") { - // Network is a full server URL - // Parse out Network and NetworkProjectId from URL - // https://www.googleapis.com/compute/v1/projects//global/networks/ - networkSelfLink = c.Network - parts := strings.Split(u.String(), "/") - if len(parts) >= 10 { - c.NetworkProjectId = parts[6] - c.Network = parts[9] + // Apply network naming requirements per + // https://cloud.google.com/compute/docs/reference/latest/instances#resource + switch c.Network { + // It is possible to omit the network property as long as a subnet is + // specified. That will be validated later. + case "": + d.ui.Message(fmt.Sprintf("Network: will be inferred from subnetwork")) + break + // This special short name should be expanded. + case "default": + networkId = "global/networks/default" + // A value other than "default" was provided for the network name. + default: + // If the value doesn't contain a slash, we assume it's not a full or + // partial URL. We will expand it into a partial URL here and avoid + // making an API call to discover the network as it's common for the + // caller to not have permission against network discovery APIs. + if !strings.Contains(c.Network, "/") { + networkId = "projects/" + c.NetworkProjectId + "/global/networks/" + c.Network + d.ui.Message(fmt.Sprintf("Network name: %q was expanded to the partial URL %q", c.Network, networkId)) } } - if u, err := url.Parse(c.Subnetwork); err == nil && (u.Scheme == "https" || u.Scheme == "http") { - // Subnetwork is a full server URL - subnetworkSelfLink = c.Subnetwork - } - // If subnetwork is ID's and not full service URL's look them up. - if subnetworkSelfLink == "" { - - // Get the network - if c.NetworkProjectId == "" { - c.NetworkProjectId = d.projectId + // Apply subnetwork naming requirements per + // https://cloud.google.com/compute/docs/reference/latest/instances#resource + switch c.Subnetwork { + case "": + // You can't omit both subnetwork and network + if networkId == "" { + return nil, fmt.Errorf("both network and subnetwork were empty.") } - d.ui.Message(fmt.Sprintf("Loading network: %s", c.Network)) - network, err := d.service.Networks.Get(c.NetworkProjectId, c.Network).Do() - if err != nil { - return nil, err - } - networkSelfLink = network.SelfLink - - // Subnetwork - // Validate Subnetwork config now that we have some info about the network - if !network.AutoCreateSubnetworks && len(network.Subnetworks) > 0 { - // Network appears to be in "custom" mode, so a subnetwork is required - if c.Subnetwork == "" { - return nil, fmt.Errorf("a subnetwork must be specified") - } - } - // Get the subnetwork - if c.Subnetwork != "" { - d.ui.Message(fmt.Sprintf("Loading subnetwork: %s for region: %s", c.Subnetwork, c.Region)) - subnetwork, err := d.service.Subnetworks.Get(c.NetworkProjectId, c.Region, c.Subnetwork).Do() - if err != nil { - return nil, err - } - subnetworkSelfLink = subnetwork.SelfLink + // An empty subnetwork is only valid for networks in legacy mode or + // auto-subnet mode. We could make an API call to get that information + // about the network, but it's common for the caller to not have + // permission to that API. We'll proceed assuming they're correct in + // omitting the subnetwork and let the compute.insert API surface an + // error about an invalid network configuration if it exists. + break + default: + // If the value doesn't contain a slash, we assume it's not a full or + // partial URL. We will expand it into a partial URL here and avoid + // making a call to discover the subnetwork. + if !strings.Contains(c.Subnetwork, "/") { + subnetworkId = "projects/" + c.NetworkProjectId + "/regions/" + c.Region + "/subnetworks/" + c.Subnetwork + d.ui.Message(fmt.Sprintf("Subnetwork: %q was expanded to the partial URL %q", c.Subnetwork, subnetworkId)) } } @@ -417,8 +415,8 @@ func (d *driverGCE) RunInstance(c *InstanceConfig) (<-chan error, error) { NetworkInterfaces: []*compute.NetworkInterface{ { AccessConfigs: []*compute.AccessConfig{accessconfig}, - Network: networkSelfLink, - Subnetwork: subnetworkSelfLink, + Network: networkId, + Subnetwork: subnetworkId, }, }, Scheduling: &compute.Scheduling{ From 74403ef91428c60e0f595a59bb2ea331e9ceb86b Mon Sep 17 00:00:00 2001 From: Evan Brown Date: Thu, 14 Sep 2017 10:14:28 -0700 Subject: [PATCH 4/5] website: Update googlecompute engine docs This change updates the documentation to describe how `network` and `subnetwork` properties are processed. --- website/source/docs/builders/googlecompute.html.md | 9 +++++++-- 1 file changed, 7 insertions(+), 2 deletions(-) diff --git a/website/source/docs/builders/googlecompute.html.md b/website/source/docs/builders/googlecompute.html.md index 8e89666bd..4ccfe4cb1 100644 --- a/website/source/docs/builders/googlecompute.html.md +++ b/website/source/docs/builders/googlecompute.html.md @@ -216,7 +216,10 @@ builder. instance. - `network` (string) - The Google Compute network id or URL to use for the - launched instance. Defaults to `"default"`. + launched instance. Defaults to `"default"`. If the value is not a URL, it + will be interpolated to `projects/((network_project_id))/global/networks/((network))`. + This value is not required if a `subnet` is specified. + - `network_project_id` (string) - The project ID for the network and subnetwork to use for launched instance. Defaults to `project_id`. @@ -259,7 +262,9 @@ builder. - `subnetwork` (string) - The Google Compute subnetwork id or URL to use for the launched instance. Only required if the `network` has been created with custom subnetting. Note, the region of the subnetwork must match the `region` - or `zone` in which the VM is launched. + or `zone` in which the VM is launched. If the value is not a URL, it + will be interpolated to `projects/((network_project_id))/regions/((region))/subnetworks/((subnetwork))` + - `tags` (array of strings) From 13e0c232d40283b76a120dfa78894f37ec591728 Mon Sep 17 00:00:00 2001 From: Evan Brown Date: Mon, 6 Nov 2017 21:07:56 -0800 Subject: [PATCH 5/5] builder/googlecompute: Test networking interpolation This change pulls the logic that interpolates network and subnetwork into its own func and adds tests. --- builder/googlecompute/driver_gce.go | 53 +---------------- builder/googlecompute/networking.go | 59 +++++++++++++++++++ builder/googlecompute/networking_test.go | 72 ++++++++++++++++++++++++ 3 files changed, 134 insertions(+), 50 deletions(-) create mode 100644 builder/googlecompute/networking.go create mode 100644 builder/googlecompute/networking_test.go diff --git a/builder/googlecompute/driver_gce.go b/builder/googlecompute/driver_gce.go index e767ff6ce..69af625e9 100644 --- a/builder/googlecompute/driver_gce.go +++ b/builder/googlecompute/driver_gce.go @@ -298,55 +298,9 @@ func (d *driverGCE) RunInstance(c *InstanceConfig) (<-chan error, error) { } // TODO(mitchellh): deprecation warnings - networkId := "" - subnetworkId := "" - - // Apply network naming requirements per - // https://cloud.google.com/compute/docs/reference/latest/instances#resource - switch c.Network { - // It is possible to omit the network property as long as a subnet is - // specified. That will be validated later. - case "": - d.ui.Message(fmt.Sprintf("Network: will be inferred from subnetwork")) - break - // This special short name should be expanded. - case "default": - networkId = "global/networks/default" - // A value other than "default" was provided for the network name. - default: - // If the value doesn't contain a slash, we assume it's not a full or - // partial URL. We will expand it into a partial URL here and avoid - // making an API call to discover the network as it's common for the - // caller to not have permission against network discovery APIs. - if !strings.Contains(c.Network, "/") { - networkId = "projects/" + c.NetworkProjectId + "/global/networks/" + c.Network - d.ui.Message(fmt.Sprintf("Network name: %q was expanded to the partial URL %q", c.Network, networkId)) - } - } - - // Apply subnetwork naming requirements per - // https://cloud.google.com/compute/docs/reference/latest/instances#resource - switch c.Subnetwork { - case "": - // You can't omit both subnetwork and network - if networkId == "" { - return nil, fmt.Errorf("both network and subnetwork were empty.") - } - // An empty subnetwork is only valid for networks in legacy mode or - // auto-subnet mode. We could make an API call to get that information - // about the network, but it's common for the caller to not have - // permission to that API. We'll proceed assuming they're correct in - // omitting the subnetwork and let the compute.insert API surface an - // error about an invalid network configuration if it exists. - break - default: - // If the value doesn't contain a slash, we assume it's not a full or - // partial URL. We will expand it into a partial URL here and avoid - // making a call to discover the subnetwork. - if !strings.Contains(c.Subnetwork, "/") { - subnetworkId = "projects/" + c.NetworkProjectId + "/regions/" + c.Region + "/subnetworks/" + c.Subnetwork - d.ui.Message(fmt.Sprintf("Subnetwork: %q was expanded to the partial URL %q", c.Subnetwork, subnetworkId)) - } + networkId, subnetworkId, err := getNetworking(c) + if err != nil { + return nil, err } var accessconfig *compute.AccessConfig @@ -609,7 +563,6 @@ func (d *driverGCE) refreshZoneOp(zone string, op *compute.Operation) stateRefre } } -// stateRefreshFunc is used to refresh the state of a thing and is // used in conjunction with waitForState. type stateRefreshFunc func() (string, error) diff --git a/builder/googlecompute/networking.go b/builder/googlecompute/networking.go new file mode 100644 index 000000000..aa59e77e0 --- /dev/null +++ b/builder/googlecompute/networking.go @@ -0,0 +1,59 @@ +package googlecompute + +import ( + "fmt" + "strings" +) + +// This method will build a network and subnetwork ID from the provided +// instance config, and return them in that order. +func getNetworking(c *InstanceConfig) (string, string, error) { + networkId := c.Network + subnetworkId := c.Subnetwork + + // Apply network naming requirements per + // https://cloud.google.com/compute/docs/reference/latest/instances#resource + switch c.Network { + // It is possible to omit the network property as long as a subnet is + // specified. That will be validated later. + case "": + break + // This special short name should be expanded. + case "default": + networkId = "global/networks/default" + // A value other than "default" was provided for the network name. + default: + // If the value doesn't contain a slash, we assume it's not a full or + // partial URL. We will expand it into a partial URL here and avoid + // making an API call to discover the network as it's common for the + // caller to not have permission against network discovery APIs. + if !strings.Contains(c.Network, "/") { + networkId = "projects/" + c.NetworkProjectId + "/global/networks/" + c.Network + } + } + + // Apply subnetwork naming requirements per + // https://cloud.google.com/compute/docs/reference/latest/instances#resource + switch c.Subnetwork { + case "": + // You can't omit both subnetwork and network + if networkId == "" { + return networkId, subnetworkId, fmt.Errorf("both network and subnetwork were empty.") + } + // An empty subnetwork is only valid for networks in legacy mode or + // auto-subnet mode. We could make an API call to get that information + // about the network, but it's common for the caller to not have + // permission to that API. We'll proceed assuming they're correct in + // omitting the subnetwork and let the compute.insert API surface an + // error about an invalid network configuration if it exists. + break + default: + // If the value doesn't contain a slash, we assume it's not a full or + // partial URL. We will expand it into a partial URL here and avoid + // making a call to discover the subnetwork. + if !strings.Contains(c.Subnetwork, "/") { + subnetworkId = "projects/" + c.NetworkProjectId + "/regions/" + c.Region + "/subnetworks/" + c.Subnetwork + } + } + return networkId, subnetworkId, nil +} diff --git a/builder/googlecompute/networking_test.go b/builder/googlecompute/networking_test.go new file mode 100644 index 000000000..85b481df3 --- /dev/null +++ b/builder/googlecompute/networking_test.go @@ -0,0 +1,72 @@ +package googlecompute + +import ( + "testing" +) + +func TestGetNetworking(t *testing.T) { + cases := []struct { + c *InstanceConfig + expectedNetwork string + expectedSubnetwork string + error bool + }{ + { + c: &InstanceConfig{ + Network: "default", + Subnetwork: "", + NetworkProjectId: "project-id", + Region: "region-id", + }, + expectedNetwork: "global/networks/default", + expectedSubnetwork: "", + error: false, + }, + { + c: &InstanceConfig{ + Network: "", + Subnetwork: "", + NetworkProjectId: "project-id", + Region: "region-id", + }, + expectedNetwork: "", + expectedSubnetwork: "", + error: true, + }, + { + c: &InstanceConfig{ + Network: "some/network/path", + Subnetwork: "some/subnetwork/path", + NetworkProjectId: "project-id", + Region: "region-id", + }, + expectedNetwork: "some/network/path", + expectedSubnetwork: "some/subnetwork/path", + error: false, + }, + { + c: &InstanceConfig{ + Network: "network-value", + Subnetwork: "subnetwork-value", + NetworkProjectId: "project-id", + Region: "region-id", + }, + expectedNetwork: "projects/project-id/global/networks/network-value", + expectedSubnetwork: "projects/project-id/regions/region-id/subnetworks/subnetwork-value", + error: false, + }, + } + + for _, tc := range cases { + n, sn, err := getNetworking(tc.c) + if n != tc.expectedNetwork { + t.Errorf("Expected network %q but got network %q", tc.expectedNetwork, n) + } + if sn != tc.expectedSubnetwork { + t.Errorf("Expected subnetwork %q but got subnetwork %q", tc.expectedSubnetwork, sn) + } + if !tc.error && err != nil { + t.Errorf("Did not expect an error but got: %v", err) + } + } +}