Add App Engine Application support #1503

paddycarver · 2018-05-17T22:48:36Z

This PR adds an app_engine block to the google_project resource. Adding the block creates an App Engine app associated with the project. App Engine apps cannot be deleted without deleting the project, so we tie the app's lifecycle to the project's. Removing the app_engine block from a config should force a project to be recreated, as there's no way to delete or disable it.

For whatever reason, we're currently getting a bunch of errors any time we try to update an App Engine app through the API, so every change forces a new resource. This is sub-optimal, as projects can't reuse their IDs, which makes recreating them problematic. This can still be accomplished using the random provider, but isn't very nice. I'll be pursuing better support there, but we need some upstream support on that. I'll follow up on the relevant issues.

The serving_status field is also less useful than it could be, as changing it will force new because of our problems updating. I figure it can at least be read now, and can be set at create, so I included it; when we get the update problems resolved, we can do some more with it, I think.

This should have all the code, but who really knows if it works or not, tbh.

Add a test case that exercises the obvious path, and fix the some of the bugs it exposed.

IAP has no reasonable support policy, because PATCH is broken, and IAP must be configured with an OAuth2 client ID and secret that belongs to the project the app is associated with. There's no programmatic way to create Clients. But we create the project and the app at the same time, and we can't update because PATCH is broken. So this just drops IAP. It also forces all our updates to ForceNew, because we can't update. Also, adds more test coverage and docs, and fixes import by not relying on the config for setting app engine info in state.

nat-henderson · 2018-05-17T23:42:16Z

google/appengine_operation.go

+)
+
+var (
+	appEngineOperationIdRE = regexp.MustCompile(fmt.Sprintf("apps/%s/operations/(.*)", ProjectRegex))


I'm sorry this is such a nit to pick, but IdRE bugs me - can it be IdRegex or IdRegEx or IDReg or IdReg or something?

It can be! I'll push an update for you to approve momentarily.

nat-henderson · 2018-05-18T23:40:49Z

google/resource_google_project.go

+			"serving_status": &schema.Schema{
+				Type:     schema.TypeString,
+				Optional: true,
+				ForceNew: true,


I remember why this is set this way, and why that's not desirable, but if you could add a comment explaining, that would probably be best.

Absolutely. I'll also look into just enabling update, because the issues I had with it seem to be gone now that I'm trying to reproduce them o.O

nat-henderson · 2018-05-18T23:42:38Z

google/resource_google_project.go

+	if err != nil {
+		return err
+	}
+	log.Printf("[DEBUG] Creating App Engine if %v is true", app != nil)


This one might be a little chatty - since we already have "Enabling App Engine", the lack of that tells us everything that this statement tells us.

Oops, sorry! This was a debug message from when I was figuring out errors. I'll remove it.

paddycarver · 2018-05-19T00:02:41Z

Did the last commit fix the suggestions, @ndmckinley?

danawillow · 2018-05-19T00:13:59Z

google/resource_google_project.go

+	if app != nil {
+		log.Printf("[DEBUG] Enabling App Engine")
+		// enable the app engine APIs so we can create stuff
+		if err = enableService("appengine.googleapis.com", project.ProjectId, config); err != nil {


Won't this mess with people using google_project_services?

It's a similar approach taken as for disabling the default network, and is documented similarly. I don't know that there's an alternative unless we don't allow people to create App Engine apps in the same apply they create projects :/

But wouldn't they already need to enable the resourcemanager api in order to create the project? Couldn't they do that at the same time as the app engine api?

APIs get enabled on a per-project basis. The project doesn't exist before the project resource's Create function is called, so we can't enable APIs on it beforehand. But by the time the project's Create function is finished, we need the App Engine Admin API enabled, because we're using it. This is the same rationale, I believe, for enabling the compute API when deleting the default network.

The workaround here, if they want to use google_project_services, is to just add the App Engine Admin API to the google_project_services resource, which should (I believe) leave things alone if the API is already enabled.

Ohhh that makes sense. Thanks!

morgante · 2018-05-21T22:36:52Z

I'm personally not a big fan of embedding this inside the project creation block. For many organizations, projects are centrally created/managed in their own repo while the actual development teams manage what happens inside of them, including app engine.

Could we consider separating this into its own resource, like how we have project services separately?

paultyng · 2018-05-21T22:51:20Z

@morgante my understanding is that the implementation you described was attempted, but due to limitations in how destroy would work on an app engine application, you have to destroy the project to destroy the application in the API unfortunately, so for now its coupled so the terraform lifecycle will work properly.

App Engine apps cannot be deleted without deleting the project, so we tie the app's lifecycle to the project's.

That being said, I believe this issue is being raised with Google to see if we can figure out some other way of addressing it.

paddycarver · 2018-05-21T23:01:06Z

Hi @morgante! In this case, I feel like there's a strong case for considering App Engine apps and Google Cloud projects to be the same resource:

You can only create an App Engine app, you can't delete it, which feels more to me like upgrading the capabilities of a project than adding a resource.
You can only have one App Engine app per project.
The App Engine app needs to share an ID with the project.

Both 1 and 2 of those are not currently possible with Terraform in an elegant way. If we were to make this a separate resource, Terraform would happily report it was deleted when it was, in fact, not, and we have no way of indicating otherwise, except a line in a log that users would mostly never see. And if the user added multiple google_app_engine_app resources to their project, we'd have no way of indicating that's incorrect; they'd just hit an API error, which is less than ideal.

Because of those reasons, I thought adding it as a sub-block within google_project was the more elegant solution, because what Terraform reports to the user is always accurate, and there are no caveats. If you need to delete your project to do what you're trying to do, Terraform will tell you that.

I do want to make the distinction that this is only for configuring the top-level settings of the App. Further App Engine resources are planned to manage things like versions, services, domain mappings, etc. that will all live as separate resources (or, at least, that's the plan at the moment, pending any implementation troubles). Keep in mind that this resource only allows the configuration of:

The domain users of the application authenticate with.
The location of the application.
The serving status of the application.
Whether the application uses health checks or a combination of liveness and readiness checks

I'm also in the process of gathering up the information I obtained implementing this to file an issue with the App Engine team, which will hopefully allow us to offer a google_app_engine_app in the future, as well, but I can't make any promises that will happen, or that it will happen in the immediate future.

morgante · 2018-05-23T21:08:40Z

Hi @paddycarver, I missed the part that there's no way to delete the app without deleting the project - in that case, it makes sense to couple their lifecycles.

Once you have the information or issue, please do ping me - I can help chase internally.

stevewolter · 2018-05-28T12:11:31Z

The rollout of this feature is rather disrupting for existing GAE users: If you have existing TF-managed projects that predate this change (don't have an app_engine block) and have GAE enabled, TF will try to delete the project and recreate it w/o GAE. Is this expected? For our team, this would have meant a bunch of accidentally deleted projects if we hadn't enabled the prevent_destroy option.

danawillow · 2018-05-29T18:08:05Z

@stevewolter I filed a bug on your behalf: #1561

stevewolter · 2018-05-30T09:50:00Z

@danawillow Thanks!

…engine_app Add App Engine Application support

ghost · 2018-11-18T14:00:04Z

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!

paddycarver added 3 commits May 10, 2018 15:01

Beginnings of App Engine app support.

765d9af

This should have all the code, but who really knows if it works or not, tbh.

Add basic test and fix bugs.

02a4259

Add a test case that exercises the obvious path, and fix the some of the bugs it exposed.

paddycarver added enhancement new-resource labels May 17, 2018

paddycarver requested review from nat-henderson, danawillow, rosbo and a team May 17, 2018 22:48

This was referenced May 17, 2018

Google appengine #125

Closed

Support Google App Engine #638

Closed

Vendor.

e74ab9d

nat-henderson approved these changes May 18, 2018

View reviewed changes

Fix suggestions from @ndmckinley.

98b6323

nat-henderson approved these changes May 19, 2018

View reviewed changes

paddycarver merged commit cd7364d into master May 19, 2018

danawillow reviewed May 19, 2018

View reviewed changes

morgante mentioned this pull request May 23, 2018

App engine API unnecessarily required for project creation #1529

Closed

danawillow mentioned this pull request May 29, 2018

TF tries to destroy/recreate older projects with GAE enabled upon upgrade #1561

Closed

lilithmooncohen mentioned this pull request Jul 25, 2018

Adding new App Engine support terraform-google-modules/terraform-google-project-factory#4

Merged

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

Merge pull request hashicorp#1503 from terraform-providers/paddy_app_…

a643130

…engine_app Add App Engine Application support

ghost locked and limited conversation to collaborators Nov 18, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add App Engine Application support #1503

Add App Engine Application support #1503

paddycarver commented May 17, 2018

nat-henderson May 17, 2018

paddycarver May 18, 2018

nat-henderson May 18, 2018

paddycarver May 18, 2018

nat-henderson May 18, 2018

paddycarver May 18, 2018

paddycarver commented May 19, 2018

danawillow May 19, 2018

paddycarver May 19, 2018

danawillow May 19, 2018

paddycarver May 21, 2018

danawillow May 21, 2018

morgante commented May 21, 2018

paultyng commented May 21, 2018 •

edited

Loading

paddycarver commented May 21, 2018

morgante commented May 23, 2018 •

edited

Loading

stevewolter commented May 28, 2018

danawillow commented May 29, 2018

stevewolter commented May 30, 2018

ghost commented Nov 18, 2018

Add App Engine Application support #1503

Add App Engine Application support #1503

Conversation

paddycarver commented May 17, 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

paddycarver commented May 19, 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

morgante commented May 21, 2018

paultyng commented May 21, 2018 • edited Loading

paddycarver commented May 21, 2018

morgante commented May 23, 2018 • edited Loading

stevewolter commented May 28, 2018

danawillow commented May 29, 2018

stevewolter commented May 30, 2018

ghost commented Nov 18, 2018

paultyng commented May 21, 2018 •

edited

Loading

morgante commented May 23, 2018 •

edited

Loading