Add cloud endpoints resource #933

nat-henderson · 2018-01-08T22:58:37Z

If merged, this closes #714. The resource works like this:

You provide the config text, either as a heredoc or by loading a file.
Terraform creates the service if necessary.
Terraform creates a new configuration for the service based on what you've provided.
Terraform creates and waits for a new rollout for the service, converting 100% of traffic to the new config.
Terraform computes and provides API names and types, endpoint names, config version, service name, and DNS address for the service.

This also includes a simple test, and an example which brings up an echo-api running on Compute Engine as in https://cloud.google.com/endpoints/docs/openapi/get-started-compute-engine.

Worth noting: this only works for the OpenAPI spec - gRPC is not implemented here, and Endpoints Frameworks is app-engine only. If someone needs / wants that, please post here or on #714.

This should deploy any OpenAPI spec you provide, including one that works with App Engine Standard or Kubernetes.

New Resource: - Endpoints Service Create/Read/Delete - Example terraform config To do: - Endpoints Service Update - Tests

nat-henderson · 2018-01-11T20:03:32Z

Hold off on this review - @ean commented in #714 about another feature they need that I didn't put into this PR.

rosbo · 2018-01-17T19:30:19Z

website/docs/r/endpoints_service.html.markdown

+
+# google_endpoints_service
+
+This resource creates and rolls out a Cloud Endpoints service using OpenAPI.  View the relevant docs [here](https://cloud.google.com/endpoints/docs/openapi/).


Add also a or gRPC

rosbo · 2018-01-17T19:31:01Z

website/docs/r/endpoints_service.html.markdown

+
+This resource creates and rolls out a Cloud Endpoints service using OpenAPI.  View the relevant docs [here](https://cloud.google.com/endpoints/docs/openapi/).
+
+## Example Usage


Consider putting two examples, one for gRPC and one for open API.

rosbo · 2018-01-17T19:34:33Z

website/docs/r/endpoints_service.html.markdown

+In addition to the arguments, the following attributes are available:
+* `config_id`: The autogenerated ID for the configuration that is rolled out as part of the creation of this resource.  Must be provided to compute engine instances as a tag.
+* `dns_address`: The address at which the service can be found - usually the same as the service name.
+* `apis`: A list of API objects; details below.


We often used Structure is documented below instead

rosbo · 2018-01-17T21:03:22Z

google/resource_endpoints_service.go

+	// rollouts or A/B testing is going to need a more precise tool than this resource.
+	config := meta.(*Config)
+	serviceName := d.Get("service_name").(string)
+	openapi_config, ok := d.GetOk("openapi_config")


We should be using camelcase for variable and method parameter names

haha, you can see where I went and wrote some python between starting and finishing this method. So embarrassing - thanks. Is there a go linter I should run to catch this sort of thing?

https://github.com/golang/lint

Unfortunately, there is a lot of noise because historically, we haven't been strict about it

rosbo · 2018-01-17T21:05:46Z

google/resource_endpoints_service.go

+			},
+			"openapi_config": &schema.Schema{
+				Type:     schema.TypeString,
+				Optional: true,


Add a ConflictsWith: []string{"grpc_config", "protoc_output"}

Nice, I didn't know about that.

rosbo · 2018-01-17T21:07:29Z

google/resource_endpoints_service.go

+				Type:     schema.TypeString,
+				Optional: true,
+			},
+			"protoc_output": &schema.Schema{


I am not sure I like the name protoc_output, seems like a computed field to me at first glance. Would protoc_config be a better name?

Well, it's not a config - it's just a giant raw binary file which is output by protoc. I can rename it to, maybe, protoc_service_descriptor_bytes or something like that - thoughts?

rosbo · 2018-01-17T21:08:02Z

google/resource_endpoints_service.go

+			"config_id": &schema.Schema{
+				Type:     schema.TypeString,
+				Computed: true,
+				ForceNew: true,


Which is this ForceNew if it is only an exported field?

You're right, that makes no sense. Removed - you can't get a new config_id unless the configs change, anyway.

rosbo · 2018-01-17T21:10:55Z

google/resource_endpoints_service.go

+				Type:     schema.TypeString,
+				Optional: true,
+			},
+			"grpc_config": &schema.Schema{


If the content of the config file changes, does it create a diff? If not, it should

It does, yep.

rosbo · 2018-01-17T21:12:17Z

google/resource_endpoints_service.go

+		}
+	}
+	// Do a rollout using the update mechanism.
+	err = resourceEndpointsServiceUpdate(d, meta)


Should it fail instead if the resource already exists? Like in all our other resources, if the resource already exists, the user should call terraform import.

The "resource" here is really the rollout, I think - the creation of the service object is incidental to the creation of the first rollout.

danawillow · 2018-01-10T21:35:57Z

google/resource_endpoints_service.go

+			},
+			"config_id": &schema.Schema{
+				Type:     schema.TypeString,
+				Computed: true,


we tend to use the order Required -> Optional -> Computed for schema elements (and if something is both optional and computed it just counts as optional for this ordering)

danawillow · 2018-01-10T21:50:31Z

google/resource_endpoints_service.go

+	}
+}
+
+func getServiceConfigSource(config_text string) servicemanagement.ConfigSource {


nit: s/config_text/configText

Done - and all the other ones.

danawillow · 2018-01-10T21:55:19Z

google/resource_endpoints_service.go

+	}
+	serviceName := d.Get("service_name").(string)
+	servicesService := servicemanagement.NewServicesService(config.clientServiceMan)
+	_, err = servicesService.Get(serviceName).Do()


So usually in tf we would assume that if we made it to the Create fn then the resource doesn't exist in GCP, or if it does the Create call should error (and then the user can import the resource if it does already exist). If there is something that makes endpoints special that we should still allow create to be called if it does already exist, then let's comment why that is.

Agreed! Added comment.

danawillow · 2018-01-17T21:25:16Z

examples/endpoints-on-compute-engine/main.tf

+  }
+
+  metadata {
+    endpoints-service-name = "${google_endpoints_service.endpoints_service.service_name}"


nit: alignment (terraform fmt in this directory should take care of it)

danawillow · 2018-01-17T21:30:27Z

google/resource_endpoints_service.go

+	}
+}
+
+func getGrpcConfigSource(service_config, proto_config string) servicemanagement.ConfigSource {


nit: initialisms are all caps: https://github.com/golang/go/wiki/CodeReviewComments#initialisms

Interesting! Okay, thanks.

danawillow · 2018-01-17T21:35:08Z

google/resource_endpoints_service.go

+	serviceName := d.Get("service_name").(string)
+	openapi_config, ok := d.GetOk("openapi_config")
+	var source servicemanagement.ConfigSource
+	if ok {


you could do if openapiConfig, ok := d.GetOk("openapi_config"); ok { here since that var is only used within the scope of the if/else

danawillow · 2018-01-17T21:38:02Z

google/resource_endpoints_service.go

+					Schema: map[string]*schema.Schema{
+						"name": &schema.Schema{
+							Type:     schema.TypeString,
+							Required: true,


This field isn't settable, so it should be computed and not required (likewise for all the others in here and endpoints)

danawillow · 2018-01-17T21:39:58Z

google/resource_endpoints_service.go

+	}
+	d.Set("config_id", service.Id)
+	d.Set("dns_address", service.Name)
+	d.Set("apis", flattenServiceManagementApi(service.Apis))


nit: flattenServiceManagementApis, perhaps?

danawillow · 2018-01-17T21:42:13Z

google/resource_endpoints_service_test.go

+	}
+}
+
+func TestAccEndpointsService_basic(t *testing.T) {


eh, we normally put the capital-T test functions at the top of the file and the helpers below it

danawillow · 2018-01-17T21:43:59Z

website/docs/r/endpoints_service.html.markdown

+}
+```
+
+The example in `examples/endpoints_on_compute_engine` shows the API from the quickstart running on a Compute Engine VM and reachable through Cloud Endpoints, which may also be useful.


I'd remove "which may also be useful" since that's implied, but you're welcome to keep it if you want

nat-henderson · 2018-01-23T20:57:52Z

Ping @danawillow and @rosbo - what else needs to be done? :)

OpenAPI & gRPC Endpoints on Compute Engine. New Resource: - Endpoints Service Create/Read/Delete - Example terraform config

add support for port_specification to resource `google_compute_health_check`

ghost · 2020-03-29T14:36:05Z

I'm going to lock this issue because it has been closed for 30 days ⏳. This helps our maintainers find and focus on the active issues.

If you feel this issue should be reopened, we encourage creating a new issue linking back to this one for added context. If you feel I made an error 🤖 🙉 , please reach out to my human friends 👉 [email protected]. Thanks!

nat-henderson added 3 commits January 8, 2018 21:55

Initial commit for OpenAPI Endpoints on Compute Engine.

2a9a8d9

New Resource: - Endpoints Service Create/Read/Delete - Example terraform config To do: - Endpoints Service Update - Tests

Added update functionality.

552d59c

Add a test for the endpoints service.

23192b9

nat-henderson requested a review from danawillow January 8, 2018 22:58

Added first draft of doc.

defe865

nat-henderson removed the request for review from danawillow January 11, 2018 20:03

nat-henderson self-assigned this Jan 11, 2018

Added support for GRPC, and a test.

67ff205

nat-henderson mentioned this pull request Jan 12, 2018

Feature Request: google_endpoints_endpoint resource #714

Closed

Added some comments.

a4a3332

nat-henderson requested a review from rosbo January 12, 2018 18:17

rosbo suggested changes Jan 17, 2018

View reviewed changes

Docs tweaks to make gRPC more clear.

7d97213

danawillow suggested changes Jan 17, 2018

View reviewed changes

Code review comments

437df02

danawillow approved these changes Jan 24, 2018

View reviewed changes

nat-henderson merged commit 9d3e64c into hashicorp:master Jan 24, 2018

chrisst pushed a commit to chrisst/terraform-provider-google that referenced this pull request Nov 9, 2018

Add cloud endpoints resource (hashicorp#933)

d33e163

OpenAPI & gRPC Endpoints on Compute Engine. New Resource: - Endpoints Service Create/Read/Delete - Example terraform config

modular-magician pushed a commit to modular-magician/terraform-provider-google that referenced this pull request Sep 27, 2019

Merge pull request hashicorp#933 from modular-magician/codegen-pr-2034

559da69

add support for port_specification to resource `google_compute_health_check`

ghost locked and limited conversation to collaborators Mar 29, 2020


		# google_endpoints_service

		This resource creates and rolls out a Cloud Endpoints service using OpenAPI. View the relevant docs [here](https://cloud.google.com/endpoints/docs/openapi/).


		This resource creates and rolls out a Cloud Endpoints service using OpenAPI. View the relevant docs [here](https://cloud.google.com/endpoints/docs/openapi/).

		## Example Usage

Add cloud endpoints resource #933

Add cloud endpoints resource #933

Conversation

nat-henderson commented Jan 8, 2018

nat-henderson commented Jan 11, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rosbo Jan 17, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nat-henderson commented Jan 23, 2018

ghost commented Mar 29, 2020

rosbo Jan 17, 2018 •

edited

Loading