App Engine Service Split Traffic resource #2269

khanali21 · 2019-08-28T22:37:45Z

Added custom_create in custom_code.
Added appengine service resource

Release Note for Downstream PRs (will be copied)

`google_app_engine_service_split_traffic`

modular-magician · 2019-08-28T22:40:49Z

Hello! I am a robot who works on Magic Modules PRs.

I have detected that you are a community contributor, so your PR will be assigned to someone with a commit-bit on this repo for initial review. They will authorize it to run through our CI pipeline, which will generate downstream PRs.

Thanks for your contribution! A human will be with you soon.

@emilymye, please review this PR or find an appropriate assignee.

khanali21 · 2019-08-28T22:40:55Z

@rileykarson I noticed that there is no option for the custom_create code in the generator. I thought it fitting to add this. This helps in Appengine Service Resource which does not have Create method.

If custom_create was not deliberately added, then I can move the resource under third_party as the custom resource.

Thanks
Ali

khanali21 · 2019-08-28T22:42:11Z

Example template to follow.

rileykarson · 2019-08-29T17:18:57Z

For Terraform, I think we want to expose this under another name than Service. Since a service can't be (directly) created, I think it would be confusing to have a google_app_engine_service resource defined that depended on a version creating the service.

As a user, I'd expect my config to look like:

resource "google_app_engine_service" "frontend" {
  id = "my-frontend-service"
  
} 

resource "google_app_engine_standard_app_version" "frontend_v2" {
  version_id = "v2"
  service = google_app_engine_service.frontend.id

  ...
}

When in reality, it's more likely to be like this, because the dependency is flipped:

resource "google_app_engine_standard_app_version" "frontend_v2" {
  version_id = "v2"
  service = "my-frontend-service"

  ...
}

resource "google_app_engine_service" "frontend" {
  id = google_app_engine_standard_version.frontend_v2.service
  
}

Instead, I'd lean towards exposing the resource as google_app_engine_service_traffic_split. I believe we can override the name value in terraform.yaml to accomplish that, but if not we'll need to add a duplicate entry in api.yaml.

rileykarson

Like StandardVersion, this is going to end up a bit of an atypical resource. Per my comment above, I think we should focus on the traffic splitting scenario since it's the only configurable portion of a service, and we should make this resource apply the specified traffic split on Create as well as Update.

rileykarson · 2019-08-29T17:19:41Z

products/appengine/api.yaml

+        name: 'split'
+        description: |
+           Mapping that defines fractional HTTP traffic diversion to different versions within the service.
+        required: false


Suggested change

required: false

required: true

Since the resource doesn't do anything else, I'm incline to make the traffic split required: true

rileykarson · 2019-08-29T17:21:27Z

products/appengine/api.yaml

@@ -73,24 +73,58 @@ objects:
      For example, an application that handles customer requests might include separate services to handle tasks such as backend data analysis or API requests from mobile devices. 
      Each service has a collection of versions that define a specific set of code used to implement the functionality of that service.
    base_url: 'apps/{{project}}/services'
-    self_link: 'apps/{{project}}/services/{{id}}'
+    update_url: 'apps/{{project}}/services/{{service_id}}?migrateTraffic=False&updateMask=split'


Suggested change

update_url: 'apps/{{project}}/services/{{service_id}}?migrateTraffic=False&updateMask=split'

update_url: 'apps/{{project}}/services/{{service_id}}?migrateTraffic={{migrateTraffic}}'

We can expose migrateTraffic as configurable. If you add it to parameters: (instead of properties:), and make it url_param_only: true in terraform.yaml, this should work as intended.

The update mask should be appended automatically by setting update_mask: true.

rileykarson · 2019-08-29T17:22:46Z

provider/terraform/custom_code.rb

+      # This code replaces the entire create method.  Since the create
+      # method's function header can't be changed, the template
+      # inserts that for you - do not include it in your custom code.
+      attr_reader :custom_create


To answer your question, this not existing was just a matter of not needing it yet.

rileykarson · 2019-08-29T17:24:53Z

products/appengine/terraform.yaml

+    import_format: ["apps/{{project}}/services/{{service_id}}"]
+    mutex: "apps/{{project}}/services/{{service_id}}"
+    custom_code: !ruby/object:Provider::Terraform::CustomCode
+      custom_create: templates/terraform/custom_create/noop_on_create_appengine_version.go.erb


Instead of doing nothing on update, what if we set the traffic split on create as well? This would mirror the IAM Policy resources that immediately set their values on create.

Setting immediately on singleton resources like this is a little unfortunate- Terraform won't show the old values in plan. On the other hand, it means that creating the resource actually does something, and the resource won't effectively always permadiff. IMO that matches to our expectations better, especially given the precedent in IAM.

To accomplish this, you can set the create_url to the same value as the update_url and create_verb to PATCH in api.yaml.

Doing this may require using a custom encoder to add an update mask on create, since we haven't added a resource doing an HTTP PATCH on Terraform Create action before.

Not sure whether I need to add create_encoder in this case. So there are two things:

Having updateMask in the create_url, I think this can be handled by just adding the constant like:

create_url: 'apps/{{project}}/services/{{service_id}}?{{migrateTraffic}}&updateMask=split'

Identifying if we need to add shardBy also in the updateMask (I am looking into it)

rileykarson · 2019-08-29T17:27:10Z

products/appengine/api.yaml

+        required: false
+        properties:      
+          - !ruby/object:Api::Type::Enum
+            name: 'shardBy'


Is one of these values automatically selected by the API? Or does it actually return UNSPECIFIED?

I am going to test it shortly

Did you get a chance to test?

rileykarson · 2019-08-29T17:27:30Z

products/appengine/api.yaml

+              - :IP
+              - :RANDOM
+          - !ruby/object:Api::Type::KeyValuePairs
+            name: 'allocations'


This feels required: true. Did you find it wasn't the case?

khanali21 · 2019-09-04T09:49:43Z

@rileykarson Struggling with few things:

The create_verb can only be PUT and POST. Do you think I should modify ruby code to allow PATCH as the create_verb too.
The update_mask true includes the id field as well, however I want only one field "split" to be part of the updateMask, i.e. updateMask=split
I don't see the need for overriding name, as I changed the name of the resource and then also updated the resourceref for the Version resource. Let me know if there is any other need for name override. BTW I could not find how override the name, I did find legacy_name but could not make it work as intended. Can you tell me if that is the right one.

thanks

khanali21 · 2019-09-05T12:51:45Z

@rileykarson Kindly review this. I have made all the required changes, One question is still open.
Q. Should I just override the delete with no op as there is no deletion applicable in this resource.

khanali21 · 2019-09-05T12:52:07Z

resource "google_storage_bucket" "bucket" {
	name = "<%= ctx[:vars]['bucket_name'] %>"
}

resource "google_storage_bucket_object" "object" {
	name   = "hello-world.zip"
	bucket = "${google_storage_bucket.bucket.name}"
	source = "./test-fixtures/appengine/hello-world.zip"
}

resource "google_app_engine_standard_app_version" "myapp_v1" {
  version_id = "v1"
  service = "myapp2"
  runtime = "nodejs10"
  noop_on_destroy = true
  entrypoint {
    shell = "node ./app.js"
  }
  deployment {
    zip {
      source_url = "https://storage.googleapis.com/${google_storage_bucket.bucket.name}/hello-world.zip"
    }  
  }
  env_variables = {
    port = "8080"
  } 

}
resource "google_app_engine_standard_app_version" "myapp_v2" {
  version_id = "v2"
  service = "myapp2"
  runtime = "nodejs10"
  noop_on_destroy = true
  entrypoint {
    shell = "node ./app.js"
  }
  deployment {
    zip {
      source_url = "https://storage.googleapis.com/${google_storage_bucket.bucket.name}/hello-world.zip"
    }  
  }
  env_variables = {
    port = "8080"
  } 

}

resource "google_app_engine_service_split_traffic" "myapp2" {
  service_id = "${google_app_engine_standard_app_version.myapp_v2.service}"
  migrate_traffic = false
  split {
    shard_by = "IP"
    allocations = {
      v1 = 0.75
      v2 = 0.25
    }
  }
}

rileykarson

Sorry! I had some other work to get done yesterday and didn't have time to make a review pass.

Per your questions;

Yep, adding it was correct. I'm not sure why we restrict the list of verbs allowed.
If you make the id / service (renamed based on my comment below) field url_param_only, it will be excluded from the update_mask.
Sorry! I changed my mind a little since before, do you actually mind defining it as an entirely separate resource in api.yaml?
Sounds good to me- I don't think there's a meaningful deletion state for a service's traffic, so deletion doing nothing makes the most sense.

rileykarson · 2019-09-05T17:20:28Z

products/appengine/api.yaml

@@ -67,30 +67,74 @@ objects:
          - ALLOW
          - DENY
  - !ruby/object:Api::Resource
-    name: 'Service'
+    name: 'ServiceSplitTraffic'


Instead of renaming Service, would you mind duplicating this into a separate resource definition? It feels a little silly, but it's the best way to represent fine-grained resources right now.

This will also require exclusions in Ansible / InSpec for the Magician to work.

rileykarson · 2019-09-05T17:22:37Z

products/appengine/api.yaml

    properties:
      - !ruby/object:Api::Type::String
        name: 'name'
-        output: true
        description: |
          Full path to the Service resource in the API. Example apps/myapp/services/default.
          This field is used in responses only. Any value specified here in a request is ignored.
      - !ruby/object:Api::Type::String
        name: 'id'


We have some liberty in terms of what we can call this field because it doesn't actually correspond to any fields we care about in the API. What do you think of service instead of id/service_id?

khanali21 · 2019-09-11T12:06:00Z

@rileykarson Please review now.

Also here is my loud thinking on this:

Use case 1:
a) Enable appengine using google_app_engine_application resource
b) Create a new service named e.g. "myapp" and upload the code as v1 using google_app_engine_standard_app_version resource. (Please note that the service is automatically created if it is not existing). Use the noop_on_destroy to ensure that this version does not get deleted.
c) Create another version v2 for service myapp using google_app_engine_standard_app_version resource.
d) Split the traffic using the google_app_engine_service_split_traffic resource.

Problem: In this use case terraform destroy will leave the service myapp and version v1 when destroying all the resources.

Solution: Use the google_app_engine_service resource to delete the final version. For this we shall need the google_app_engine_service resource as I did in the first iteration which will ignore the create but allow deletion.

khanali21 · 2019-09-13T09:43:27Z

@rileykarson Can you please review this. And give your comments.

modular-magician · 2019-09-13T19:43:16Z

Hi! I'm the modular magician, I work on Magic Modules.
This PR seems not to have generated downstream PRs before, as of b044582.

Pull request statuses

No diff detected in Ansible.
No diff detected in Inspec.

New Pull Requests

I built this PR into one or more new PRs on other repositories, and when those are closed, this PR will also be merged and closed.
depends: hashicorp/terraform-provider-google-beta#1135
depends: GoogleCloudPlatform/terraform-google-conversion#199
depends: hashicorp/terraform-provider-google#4451

rileykarson

Instead of an approach that requires adding a google_app_engine_service resource that's only really used for deletion, I'd prefer we add delete_service_on_destroy to google_app_engine_standard_app_version. That boolean would be mutually exclusive with noop_on_destroy, and would delete the service at the same time as that version.

Terraform will process deletion of versions in parallel for most configs; that means that the version that deletes the parent may delete it before the other version is "deleted" by Terraform. That'll be fine, though, deletion succeeds if the parent / resource isn't found.

rileykarson · 2019-09-13T19:35:43Z

products/appengine/api.yaml

    properties:
      - !ruby/object:Api::Type::String
        name: 'name'
-        output: true
        description: |
          Full path to the Service resource in the API. Example apps/myapp/services/default.
          This field is used in responses only. Any value specified here in a request is ignored.
      - !ruby/object:Api::Type::String
        name: 'id'


rileykarson · 2019-09-13T19:35:52Z

products/appengine/api.yaml

+        required: false
+        properties:      
+          - !ruby/object:Api::Type::Enum
+            name: 'shardBy'


Did you get a chance to test?

templates/terraform/examples/app_engine_service_split_traffic.tf.erb

modular-magician · 2019-09-13T19:44:42Z

Hi! I'm the modular magician, I work on Magic Modules.
I see that this PR has already had some downstream PRs generated. Any open downstreams are already updated to your most recent commit, b044582.

Pull request statuses

terraform-provider-google-beta already has an open PR.
terraform-google-conversion already has an open PR.
terraform-provider-google already has an open PR.
No diff detected in Ansible.
No diff detected in Inspec.

New Pull Requests

I didn't open any new pull requests because of this PR.

rileykarson · 2019-09-13T19:46:07Z

provider/terraform/custom_code.rb

@@ -96,6 +96,10 @@ class CustomCode < Api::Object
      # method's function header can't be changed, the template
      # inserts that for you - do not include it in your custom code.
      attr_reader :custom_delete
+      # This code replaces the entire create method.  Since the create


As long as we're not using this, do you mind removing it + the changes in resource.erb? I'd rather not add unused code.

modular-magician · 2020-02-20T10:52:36Z

Hi! I'm the modular magician. Your PR generated some diffs in downstreams - here they are.

Diff report:

Terraform GA: Diff ( 20 files changed, 697 insertions(+), 94 deletions(-))
Terraform Beta: Diff ( 22 files changed, 705 insertions(+), 113 deletions(-))
TF Conversion: Diff ( 1 file changed, 101 insertions(+))

khanali21 · 2020-02-20T10:59:30Z

@rileykarson DONE :) Had a bad time with this one.... even the changes from resource.erb were not going away git acting up with resolving merge conflict.

modular-magician · 2020-02-20T11:00:57Z

Hi! I'm the modular magician. Your PR generated some diffs in downstreams - here they are.

Diff report:

Terraform GA: Diff ( 5 files changed, 647 insertions(+), 2 deletions(-))
Terraform Beta: Diff ( 5 files changed, 647 insertions(+), 2 deletions(-))
TF Conversion: Diff ( 1 file changed, 101 insertions(+))

rileykarson

LGTM. Thanks for working through me on the drawn out process to get this resource added! Since AppEngine is one of GCP's oldest APIs, it doesn't tend to fit the model of API behaviour we built MM with, or the model of declarative resources that's evolved since AppEngine's creation.

…#2269) * Appengine Service resource * Added custom_create in custom_code. * Added appengine service resource * Updates based on review * Updates based on review * update based on review * Updates * Update * Update * Updated the example * Updated the example and website link * Skip Delete on test and noop * Rebased and merged the appengine terraform.yaml * Updates * Fixes * updated the example and excluded split from reading * updated the example * Reverted custom_create related code * Reverted resource.erb * updates * removed the error check * Updates * Fixes * Fixed the import in test script * Updates as per review * chanded the operation to OpAsync * Updated the Objects from Async to OpAsync * Fixed the example primary resource name * Testing by removing id_format and import_format * reverted import and id format and also the example * Fixed the id_format * Updates based on review * Typo fix * Changed url_param_only to api_name * Updated the resource.erb from upstream/master

googlebot added the cla: yes label Aug 28, 2019

modular-magician added the community label Aug 28, 2019

rileykarson self-requested a review August 28, 2019 23:03

rileykarson reviewed Aug 29, 2019

View reviewed changes

khanali21 force-pushed the master branch from 58c27e2 to 5dea8cc Compare September 3, 2019 09:17

khanali21 force-pushed the master branch from 5dea8cc to dda8ba5 Compare September 4, 2019 11:51

khanali21 changed the title ~~App Engine Service resource~~ App Engine Service Split Traffic resource Sep 4, 2019

khanali21 force-pushed the master branch from 65d2784 to 468b607 Compare September 5, 2019 12:49

rileykarson reviewed Sep 5, 2019

View reviewed changes

khanali21 force-pushed the master branch 2 times, most recently from 8e35ff1 to 6cb1cfc Compare September 11, 2019 09:48

modular-magician added downstream-generated terraform-google-conversion terraform-provider-google-beta terraform-provider-google labels Sep 13, 2019

rileykarson reviewed Sep 13, 2019

View reviewed changes

khanali21 force-pushed the master branch 2 times, most recently from 94e9be6 to a8210ad Compare September 17, 2019 12:23

khanali21 added 19 commits February 20, 2020 10:37

updated the example and excluded split from reading

8b00e74

updated the example

47f1d5b

Reverted custom_create related code

0a50129

Reverted resource.erb

9a3cc01

updates

c87a9ee

removed the error check

e4b07b3

Updates

6892b5d

Fixes

ef5dbb1

Fixed the import in test script

8dd6780

Updates as per review

63b58d3

chanded the operation to OpAsync

9572ff6

Updated the Objects from Async to OpAsync

c4edda6

Fixed the example primary resource name

40166ea

Testing by removing id_format and import_format

5236fca

reverted import and id format and also the example

e729635

Fixed the id_format

11e084a

Updates based on review

4fab555

Typo fix

bb6e57d

Changed url_param_only to api_name

91585c6

khanali21 force-pushed the master branch from 650a301 to 91585c6 Compare February 20, 2020 10:47

Updated the resource.erb from upstream/master

7e54453

rileykarson approved these changes Feb 20, 2020

View reviewed changes

rileykarson merged commit a9cabe8 into GoogleCloudPlatform:master Feb 20, 2020

This pull request was closed.

	update_url: 'apps/{{project}}/services/{{service_id}}?migrateTraffic=False&updateMask=split'
	update_url: 'apps/{{project}}/services/{{service_id}}?migrateTraffic={{migrateTraffic}}'

App Engine Service Split Traffic resource #2269

App Engine Service Split Traffic resource #2269

Conversation

khanali21 commented Aug 28, 2019 • edited by rileykarson Loading

Release Note for Downstream PRs (will be copied)

modular-magician commented Aug 28, 2019

khanali21 commented Aug 28, 2019

khanali21 commented Aug 28, 2019

rileykarson commented Aug 29, 2019

rileykarson left a 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

khanali21 commented Sep 4, 2019

khanali21 commented Sep 5, 2019

khanali21 commented Sep 5, 2019

rileykarson left a 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

khanali21 commented Sep 11, 2019 • edited Loading

khanali21 commented Sep 13, 2019

modular-magician commented Sep 13, 2019

Pull request statuses

New Pull Requests

rileykarson left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

modular-magician commented Sep 13, 2019

Pull request statuses

New Pull Requests

Choose a reason for hiding this comment

modular-magician commented Feb 20, 2020

Diff report:

khanali21 commented Feb 20, 2020

modular-magician commented Feb 20, 2020

Diff report:

rileykarson left a comment

Choose a reason for hiding this comment

khanali21 commented Aug 28, 2019 •

edited by rileykarson

Loading

khanali21 commented Sep 11, 2019 •

edited

Loading