scheduler: fix perm diff on appEngineRouting #2270

tmshn · 2019-08-29T08:27:53Z

Problem

Cloud Scheduler Job created by terraform permanently claims "new resource required" and recreated every time.

resource "google_cloud_scheduler_job" "job" {
  // ...(snip)...
  app_engine_http_target {
    app_engine_routing {
      service = "my-service"
    }
  }
}

-/+ google_cloud_scheduler_job.job (new resource required)
      // ...(snip)...
      app_engine_http_target.0.app_engine_routing.0.service: "" => "my-service" (forces new resource)

This is due to API behavior; appEngineRouting field returned by API only contains read-only host field, without any of service, version nor instance fields.

{
  // ...(snip)...
  "appEngineHttpTarget": {
    "appEngineRouting": {
      "host": "my-service.my-project.appspot.com"
    },
  },
}

What's changed

Fixed flatten function (used in Read function) for appEngineRouting to use config value rather than API response.

Release Note for Downstream PRs (will be copied)

`cloudscheduler`: Fixed permadiff for `app_engine_http_target.app_engine_routing` on `google_cloud_scheduler_job`

modular-magician · 2019-08-29T08:31:20Z

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.

tmshn · 2019-08-31T09:48:49Z

templates/terraform/custom_flatten/cloudscheduler_job_appenginerouting.go.erb

+	if targets := d.Get("app_engine_http_target").([]interface{}); len(targets) > 0 {
+		return targets[0].(map[string]interface{})["app_engine_routing"]
+	}
+	return nil


Another choice is doing like this; I don't know which is better...

return []interface{}{map[string]interface{}{ "service": d.Get("app_engine_http_target.0.app_engine_routing.0.service"), "version": d.Get("app_engine_http_target.0.app_engine_routing.0.version"), "instance": d.Get("app_engine_http_target.0.app_engine_routing.0.instance"), }}

i'm pretty sure the easiest might actually be

if v, ok := d.GetOk("app_engine_http_target"); ok && len(v.([]interface{})) > 0 { return d.Get("app_engine_http_target.0.app_engine_routing") } return nil

Looks better, I adopted your suggestion! ea6171a

tmshn · 2019-09-05T07:11:44Z

@emilymye I have no idea why my ci is pending for so long, but...should I do anything?

emilymye · 2019-09-12T20:18:58Z

@tmshn sorry! for community PRs we actually ahve to manually approve runs on our end, so that's on me

modular-magician · 2019-09-12T20:33:34Z

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

Pull request statuses

No diff detected in terraform-google-conversion.
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#1131
depends: hashicorp/terraform-provider-google#4444

products/cloudscheduler/terraform.yaml

emilymye · 2019-09-16T16:24:10Z

@tmshn Thanks for contributing, sorry for the delay in review! I just looked at this and if the value isn't being returned by the API, we can just set it to ignore_read (see region property on L68). If you don't mind, please use that instead!

emilymye · 2019-09-18T00:39:00Z

@tmshn thanks for being patient! So, after some inspection, it appears that

we do need to have a custom flatten - I went ahead and suggested what I think would probably be best for setting the value to state, and this PR should be good to go. If you feel strongly about your code, they all do the same thing.
(not requiring changes on your end) it turns out we have a test that sets service/version/instance and in that case, the API returns all three. It's only if you specify service-only or version-only that the API returns only host. Interestingly, if you set instance/version, it returns version as service! So that's not great. I think I'm ok with not reading the server value.

tmshn · 2019-09-18T12:30:52Z

2. (not requiring changes on your end) it turns out we have a test that sets service/version/instance and in that case, the API returns all three. It's only if you specify service-only or version-only that the API returns only host. Interestingly, if you set instance/version, it returns version as service! So that's not great. I think I'm ok with not reading the server value.

That sound so curios to me, so I've tried out that too, and found:

If only one of service or version specified, or none specified
- API returns only a host of format $x.$project.appspot.com
If two of service, version and instance specified
- API returns a host of format $x.$y.$project.appspot.com, a version (the value is from $x, which might not be an actual version) and a service (again, the value is from $y)
If all three of service, version and instance specified
- API returns a host, correct instance, correct version and correct service

Ooh...so weird behavior! 😕

For later reference, I paste the result here.

None specified

app_engine_routing {
}

$ terraform apply -auto-approve
google_cloud_scheduler_job.tmshn_test: Creating...
google_cloud_scheduler_job.tmshn_test: Creation complete after 2s [id=tmshn-test-scheduler-job]

Apply complete! Resources: 1 added, 0 changed, 0 destroyed.

Outputs:

tmshn-test-res = {"host":"myproject.appspot.com"}

only `service` specified

app_engine_routing {
    service = "default"
}

$ terraform apply -auto-approve
google_cloud_scheduler_job.tmshn_test: Creating...
google_cloud_scheduler_job.tmshn_test: Creation complete after 5s [id=tmshn-test-scheduler-job]

Apply complete! Resources: 1 added, 0 changed, 0 destroyed.

Outputs:

tmshn-test-res = {"host":"default.myproject.appspot.com"}

only `version` specified

app_engine_routing {
    version = "v2"
}

$ terraform apply -auto-approve
google_cloud_scheduler_job.tmshn_test: Refreshing state... [id=tmshn-test-scheduler-job]
google_cloud_scheduler_job.tmshn_test: Destroying... [id=tmshn-test-scheduler-job]
google_cloud_scheduler_job.tmshn_test: Destruction complete after 1s
google_cloud_scheduler_job.tmshn_test: Creating...
google_cloud_scheduler_job.tmshn_test: Creation complete after 2s [id=tmshn-test-scheduler-job]

Apply complete! Resources: 1 added, 0 changed, 1 destroyed.

Outputs:

tmshn-test-res = {"host":"v2.myproject.appspot.com"}

only `instance` specified (error)

app_engine_routing {
  instance = "some-instance-id-xxx"
}

$ terraform apply -auto-approve
google_cloud_scheduler_job.tmshn_test: Refreshing state... [id=tmshn-test-scheduler-job]
google_cloud_scheduler_job.tmshn_test: Destroying... [id=tmshn-test-scheduler-job]
google_cloud_scheduler_job.tmshn_test: Destruction complete after 2s
google_cloud_scheduler_job.tmshn_test: Creating...

Error: Error creating Job: googleapi: Error 400: If instance is set then service or version must also be set. For more information, see https://cloud.google.com/appengine/docs/standard/python/how-requests-are-routed#targeted_routing

  on main.tf line 10, in resource "google_cloud_scheduler_job" "tmshn_test":
  10: resource "google_cloud_scheduler_job" "tmshn_test" {

`service` and `version` specified

app_engine_routing {
  service  = "default"
  version  = "v2"
}

$ terraform apply -auto-approve
google_cloud_scheduler_job.tmshn_test: Refreshing state... [id=tmshn-test-scheduler-job]
google_cloud_scheduler_job.tmshn_test: Destroying... [id=tmshn-test-scheduler-job]
google_cloud_scheduler_job.tmshn_test: Destruction complete after 1s
google_cloud_scheduler_job.tmshn_test: Creating...
google_cloud_scheduler_job.tmshn_test: Creation complete after 2s [id=tmshn-test-scheduler-job]

Apply complete! Resources: 1 added, 0 changed, 1 destroyed.

Outputs:

tmshn-test-res = {"host":"v2.default.myproject.appspot.com","service":"default","version":"v2"}

`service` and `instance` specified

app_engine_routing {
  service  = "default"
  instance = "some-instance-id-xxx"
}

$ terraform apply -auto-approve
google_cloud_scheduler_job.tmshn_test: Refreshing state... [id=tmshn-test-scheduler-job]
google_cloud_scheduler_job.tmshn_test: Destroying... [id=tmshn-test-scheduler-job]
google_cloud_scheduler_job.tmshn_test: Destruction complete after 1s
google_cloud_scheduler_job.tmshn_test: Creating...
google_cloud_scheduler_job.tmshn_test: Creation complete after 2s [id=tmshn-test-scheduler-job]

Apply complete! Resources: 1 added, 0 changed, 1 destroyed.

Outputs:

tmshn-test-res = {"host":"some-instance-id-xxx.default.myproject.appspot.com","service":"default","version":"some-instance-id-xxx"}

`version` and `instance` specified

app_engine_routing {
  version  = "v2"
  instance = "some-instance-id-xxx"
}

$ terraform apply -auto-approve
google_cloud_scheduler_job.tmshn_test: Refreshing state... [id=tmshn-test-scheduler-job]
google_cloud_scheduler_job.tmshn_test: Destroying... [id=tmshn-test-scheduler-job]
google_cloud_scheduler_job.tmshn_test: Destruction complete after 1s
google_cloud_scheduler_job.tmshn_test: Creating...
google_cloud_scheduler_job.tmshn_test: Creation complete after 2s [id=tmshn-test-scheduler-job]

Apply complete! Resources: 1 added, 0 changed, 1 destroyed.

Outputs:

tmshn-test-res = {"host":"some-instance-id-xxx.v2.myproject.appspot.com","service":"v2","version":"some-instance-id-xxx"}

`service`, `version` and `instance` specified

app_engine_routing {
  service  = "default"
  version  = "v2"
  instance = "some-instance-id-xxx"
}

$ terraform apply -auto-approve
google_cloud_scheduler_job.tmshn_test: Refreshing state... [id=tmshn-test-scheduler-job]
google_cloud_scheduler_job.tmshn_test: Destroying... [id=tmshn-test-scheduler-job]
google_cloud_scheduler_job.tmshn_test: Destruction complete after 2s
google_cloud_scheduler_job.tmshn_test: Creating...
google_cloud_scheduler_job.tmshn_test: Creation complete after 2s [id=tmshn-test-scheduler-job]

Apply complete! Resources: 1 added, 0 changed, 1 destroyed.

Outputs:

tmshn-test-res = {"host":"some-instance-id-xxx.v2.default.myproject.appspot.com","instance":"some-instance-id-xxx","service":"default","version":"v2"}

tmshn · 2019-09-18T12:34:40Z

we do need to have a custom flatten - I went ahead and suggested what I think would probably be best for setting the value to state, and this PR should be good to go. If you feel strongly about your code, they all do the same thing.

@emilymye Sorry I couldn't fully understand what you mean here (oh my poor English!)
Are you satisfied with the changeset I'm proposing in this pull request? Or are you requesting some change?

emilymye · 2019-09-19T19:22:13Z

@tmshn sorry about that! my comments didnt actually submit - submitting now

emilymye

@tmshn Looks good to me! I'll go ahead and start generation again to confirm, and will merge if all goes well. Thanks for your patience!

templates/terraform/custom_flatten/cloudscheduler_job_appenginerouting.go.erb

tmshn · 2019-09-24T02:36:29Z

Let me know if I need to merge latest master

modular-magician · 2019-09-24T17:01:58Z

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, f06e040.

Pull request statuses

terraform-provider-google-beta already has an open PR.
No diff detected in terraform-google-conversion.
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.

emilymye · 2019-09-24T18:28:19Z

@tmshn (deleted my previous comment) so I ran our tests for cloud scheduler job and it looks like import will cause an issue because there is no value in state, but any diff causes a recreation which is technically a breaking change. Thus, I was wondering if you'd be willing to change your flatten method one more time so we fallback on API value if it's not set in state, like:

	if stateV, ok := d.GetOk("app_engine_http_target"); ok && len(stateV.([]interface{})) > 0 {
		return d.Get("app_engine_http_target.0.app_engine_routing")
	} else {
		original := v.(map[string]interface{})
		if len(original) == 0 {
			return nil
		}
		transformed := make(map[string]interface{})
		transformed["service"] = original["service"]
		transformed["version"] = original["version"]
		transformed["instance"] = original["instance"]
		return []interface{}{transformed}
	}
	return nil

Once this is in, the tests should be fine and we should be able to submit finally :) Sorry for the trouble! This resource is not very straightforward so we appreciate that you're helping us to debug its behavior.

tmshn · 2019-09-25T05:08:01Z

I'm okay to fix this in that way; e.g.: fallback to the original method that read data from api.
But still, only a few cases will work as we confirmed earlier:

Following cases has appropriate response fields, so expected to work
- none of service, version nor instance specified
- service and version specified
- all of service, version and instance specified

@emilymye Can I see the test result you refer to?

emilymye · 2019-09-25T22:17:47Z

Yeah, I'm not sure we can do anything about the case where someone is importing the API state into config with weird cases. I think as long as we don't make those cases worse (since they would permadiff anyways) and we fix the cases for

no perma-diff post initial apply
importing a valid state with the configurations that stay the same (i.e. all three fields are returned by API or similar)

we should be ok? The fallback should only be triggered on import, so if you create it in Terraform with one of the weird cases, it'll still be fine. As for the weird cases, I think we probably shouldn't do any special logic and should just ask the upstream API to fix it (I filed a bug against them).

The test command:

 make testacc TEST="./google" TESTARGS="--run='TestAccCloudSchedulerJob_schedulerJobAppEngineExample'"

gives the following error without the state changes:

--- FAIL: TestAccCloudSchedulerJob_schedulerJobAppEngineExample (6.56s)
    testing.go:569: Step 1 error: ImportStateVerify attributes not equivalent. Difference is shown below. Top is actual, bottom is expected.

        (map[string]string) {
        }


        (map[string]string) (len=4) {
         (string) (len=45) "app_engine_http_target.0.app_engine_routing.#": (string) (len=1) "1",
         (string) (len=54) "app_engine_http_target.0.app_engine_routing.0.instance": (string) (len=15) "my-instance-001",
         (string) (len=53) "app_engine_http_target.0.app_engine_routing.0.service": (string) (len=3) "web",
         (string) (len=53) "app_engine_http_target.0.app_engine_routing.0.version": (string) (len=4) "prod"
        }

Our test infrastructure verifies test resources by attempting to import the newly created test resources into an empty state and seeing if they differ. Because the original state had the appEngineRouting set, but the imported state did not, we got this error.

tmshn · 2019-09-26T12:49:37Z

Thank you for the clear explanation, I got what's happening!
(So IIUC, if the test case was any of "weird cases", this patch broke the import test; e.g.: both "original state" and "imported state" were wrong before this fix, while former became correct after this fix).

But anyway, I agree with you that we are improving things as far as we can, and doesn't make the other cases worse.

I filed a bug against them

Thank you!

tmshn · 2019-09-26T12:58:05Z

Fixed 194b7d9 ✅

modular-magician · 2019-09-26T21:24:19Z

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, 194b7d9.

Pull request statuses

terraform-provider-google-beta already has an open PR.
No diff detected in terraform-google-conversion.
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.

emilymye · 2019-09-27T06:34:51Z

@tmshn had to rebase, but should be going in!

modular-magician · 2019-09-27T07:06:14Z

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, 6b158f8.

Pull request statuses

No diff detected in terraform-google-conversion.
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#1201
depends: hashicorp/terraform-provider-google#4561

Tracked submodules are build/terraform-beta build/terraform-mapper build/terraform build/ansible build/inspec.

googlebot added the cla: yes label Aug 29, 2019

modular-magician added the community label Aug 29, 2019

rileykarson mentioned this pull request Aug 30, 2019

Magic Modules KCC Provider #2279

Closed

tmshn commented Aug 31, 2019

View reviewed changes

emilymye self-assigned this Sep 12, 2019

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

emilymye reviewed Sep 16, 2019

View reviewed changes

products/cloudscheduler/terraform.yaml Show resolved Hide resolved

tmshn force-pushed the cloudscheduler-appenginerouting branch from 701c4aa to 7e3743f Compare September 17, 2019 12:47

emilymye approved these changes Sep 23, 2019

View reviewed changes

emilymye reviewed Sep 23, 2019

View reviewed changes

templates/terraform/custom_flatten/cloudscheduler_job_appenginerouting.go.erb Outdated Show resolved Hide resolved

emilymye self-requested a review September 23, 2019 19:47

emilymye added the changelog: bug-fix label Sep 24, 2019

emilymye approved these changes Sep 24, 2019

View reviewed changes

modular-magician added the automerged label Sep 26, 2019

tmshn added 8 commits September 26, 2019 23:30

scheduler: fix perm diff on appEngineRouting

6fedcaf

fix cloudscheduler override

60e73b0

fix cloudscheduler override

94db91d

scheduler_job: add ignore_read just for clarity

8d59924

removed ignore_read and fixed comment

49c2eca

use d.Get() directly

fbfddc4

fix typo

f8cf28f

[cloudscheduler] fallback to read api response if no state

6b158f8

emilymye force-pushed the cloudscheduler-appenginerouting branch from 194b7d9 to 6b158f8 Compare September 27, 2019 06:34

Update tracked submodules -> HEAD on Fri Sep 27 07:09:06 UTC 2019

529f461

Tracked submodules are build/terraform-beta build/terraform-mapper build/terraform build/ansible build/inspec.

modular-magician merged commit 1238376 into GoogleCloudPlatform:master Sep 27, 2019

tmshn deleted the cloudscheduler-appenginerouting branch September 27, 2019 07:55

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

scheduler: fix perm diff on appEngineRouting #2270

scheduler: fix perm diff on appEngineRouting #2270

tmshn commented Aug 29, 2019 •

edited by emilymye

Loading

modular-magician commented Aug 29, 2019

tmshn Aug 31, 2019

emilymye Sep 18, 2019

tmshn Sep 20, 2019

tmshn commented Sep 5, 2019

emilymye commented Sep 12, 2019

modular-magician commented Sep 12, 2019

emilymye commented Sep 16, 2019

emilymye commented Sep 18, 2019

tmshn commented Sep 18, 2019

None specified

only `service` specified

only `version` specified

only `instance` specified (error)

`service` and `version` specified

`service` and `instance` specified

`version` and `instance` specified

`service`, `version` and `instance` specified

tmshn commented Sep 18, 2019

emilymye commented Sep 19, 2019

emilymye left a comment

tmshn commented Sep 24, 2019

modular-magician commented Sep 24, 2019

emilymye commented Sep 24, 2019 •

edited

Loading

tmshn commented Sep 25, 2019

emilymye commented Sep 25, 2019 •

edited

Loading

tmshn commented Sep 26, 2019

tmshn commented Sep 26, 2019

modular-magician commented Sep 26, 2019

emilymye commented Sep 27, 2019

modular-magician commented Sep 27, 2019

scheduler: fix perm diff on appEngineRouting #2270

scheduler: fix perm diff on appEngineRouting #2270

Conversation

tmshn commented Aug 29, 2019 • edited by emilymye Loading

Problem

What's changed

Release Note for Downstream PRs (will be copied)

modular-magician commented Aug 29, 2019

tmshn Aug 31, 2019

Choose a reason for hiding this comment

emilymye Sep 18, 2019

Choose a reason for hiding this comment

tmshn Sep 20, 2019

Choose a reason for hiding this comment

tmshn commented Sep 5, 2019

emilymye commented Sep 12, 2019

modular-magician commented Sep 12, 2019

Pull request statuses

New Pull Requests

emilymye commented Sep 16, 2019

emilymye commented Sep 18, 2019

tmshn commented Sep 18, 2019

None specified

only service specified

only version specified

only instance specified (error)

service and version specified

service and instance specified

version and instance specified

service, version and instance specified

tmshn commented Sep 18, 2019

emilymye commented Sep 19, 2019

emilymye left a comment

Choose a reason for hiding this comment

tmshn commented Sep 24, 2019

modular-magician commented Sep 24, 2019

Pull request statuses

New Pull Requests

emilymye commented Sep 24, 2019 • edited Loading

tmshn commented Sep 25, 2019

emilymye commented Sep 25, 2019 • edited Loading

tmshn commented Sep 26, 2019

tmshn commented Sep 26, 2019

modular-magician commented Sep 26, 2019

Pull request statuses

New Pull Requests

emilymye commented Sep 27, 2019

modular-magician commented Sep 27, 2019

Pull request statuses

New Pull Requests

tmshn commented Aug 29, 2019 •

edited by emilymye

Loading

only `service` specified

only `version` specified

only `instance` specified (error)

`service` and `version` specified

`service` and `instance` specified

`version` and `instance` specified

`service`, `version` and `instance` specified

emilymye commented Sep 24, 2019 •

edited

Loading

emilymye commented Sep 25, 2019 •

edited

Loading