providers/google: Create and manage Google Cloud Platform Projects #10425

evandbrown · 2016-11-30T00:04:11Z

This proposal allows a GCP project to be created, modified, and deleted with Terraform. A project may be created, APIs enabled (e.g., Container Engine and GCS), and IAM policies applied.

evandbrown · 2016-11-30T00:06:03Z

cc @danawillow

evandbrown · 2016-11-30T00:09:24Z

@paddyforan @stack72 This change introduces a few new requirements for the GCP project that runs integration tests:

The project must be a member of an organization
The service account that runs the tests must have roles/resourcemanager.projectCreator in the organization
GOOGLE_ORG must be set to that org's idea when the tests run

I'm available to discuss here or off-thread at your leisure. Thank you!

paddycarver

Hey @evandbrown! Overall, I think this is great, and see no major issues. I'd like to have a quick conversation about backwards compatibility (if you don't mind), but I'd personally really like to see this in 0.8, as I don't see how we could do it before 0.9 otherwise.

A lot of my comments are nits that don't necessarily need to be fixed but that I'm drawing attention to, or requests for explanations on changes in behaviour, even if only for posterity (so we can track down why things changed later :D )

Finally, some changes in behaviour here may need to be called out elsewhere (e.g., the 0.8 upgrade guide) -- I'll get further guidance on that. But things like no longer merging policies and projects actually getting deleted are things I feel we should probably call out more prominently.

paddycarver · 2016-11-30T00:08:13Z

builtin/providers/google/resource_google_project.go

@@ -31,18 +32,29 @@ func resourceGoogleProject() *schema.Resource {
 		Delete: resourceGoogleProjectDelete,

 		Schema: map[string]*schema.Schema{
-			"id": &schema.Schema{
+			"project_id": &schema.Schema{


This is a backwards-incompatible change, isn't it? Do we need a state migration here?

Great catch. I'll get this added.

paddycarver · 2016-11-30T00:10:24Z

builtin/providers/google/resource_google_project.go

-			"policy_data": &schema.Schema{
+			"name": &schema.Schema{
+				Type:     schema.TypeString,
+				Required: true,


This looks like it's also a breaking change. Didn't this used to be computed? Is it not anymore? Why not?

The initial version of this resource didn't support create. It assumed the resource (project) already existed, and it read/computed the values from the existing resource.

I'm wondering if it's worth breaking this out into two resources (google_project and google_imported_project), or adding an imported attribute? The latter is simpler, but would have issues with computed vs non-computed values. Thoughts?

Fair enough, this makes a tonne more sense to me now. Thanks :D I'll address the import stuff all at once in a separate comment (so it's easier to find in the future) if that's ok. :)

paddycarver · 2016-11-30T00:18:35Z

builtin/providers/google/resource_google_project.go

 	if pString, ok := d.GetOk("policy_data"); ok {
 		// The policy string is just a marshaled cloudresourcemanager.Policy.
 		// Unmarshal it to a struct.
-		var policy cloudresourcemanager.Policy
+		var policy *cloudresourcemanager.Policy
 		if err = json.Unmarshal([]byte(pString.(string)), &policy); err != nil {


Now that you've changed policy to a pointer, aren't we taking the address of a pointer here?

I...don't know why I did this.

paddycarver · 2016-11-30T00:26:41Z

builtin/providers/google/resource_google_project.go

-		_, err = config.clientResourceManager.Projects.SetIamPolicy(project,
-			&cloudresourcemanager.SetIamPolicyRequest{Policy: p}).Do()
+		// Apply the policy
+		log.Printf("[DEBUG] Setting policy for project: %#v", policy)


This used to merge the old policy in before setting the new one. Can you help me understand this change in behaviour?

I mentioned this in an earlier comment, but basically we're assuming this resource creates new projects, vs importing existing ones now. I'm becoming more convinced that we want a google_project and google_imported_project to differentiate the two.

Ah, this makes total sense to me now. I'm less convinced, in terms of what the end result should look like, but am still thinking through the road to get there. :)

paddycarver · 2016-11-30T00:28:29Z

builtin/providers/google/resource_google_project.go

 	}

 	d.Set("number", strconv.FormatInt(int64(p.ProjectNumber), 10))
-	d.Set("name", p.Name)


Why is name no longer being read?

It's a required property now, and not computed. Is it best-practice to read-and-set all values?

Oh jeez, I don't know about "best practice". But I imagine for things like refreshes, it'd be helpful to have it? That way terraform import (I think) will work properly. The idea being that if we set all the properties in the state on read (even the ones we know are set in the config) then when we don't instantiate a resource from a config (e.g., when using terraform import) those properties will still be properly set. Or at least that's my rationale at the moment, I could be misunderstanding how any number of things work.

paddycarver · 2016-11-30T00:31:13Z

builtin/providers/google/resource_google_project.go

+		}
+
+		added = make([]string, 0)
+		removed = make([]string, 0)


nit: var added, removed []string does effectively the same thing and is a bit clearer, in my opinion.

Update: I lied. You don't even need these two lines at all, the named return variables already are the equivalent of var added, removed []string.

paddycarver · 2016-11-30T00:32:06Z

builtin/providers/google/resource_google_project.go

+		for k, _ := range newMap {
+			if !oldMap[k] {
+				added = append(added, k)
+				continue


nit: no real point to this continue, though it doesn't do any harm, either.

paddycarver · 2016-11-30T00:36:28Z

builtin/providers/google/resource_storage_object_acl.go

-				log.Printf("[DEBUG]: saving re %s-%s", role, entity)
+			if _, in := re_local_map[v.Entity]; in {
+				role_entity = append(role_entity, fmt.Sprintf("%s:%s", v.Role, v.Entity))
+				log.Printf("[DEBUG]: saving re %s-%s", v.Role, v.Entity)


These changes seem unrelated to the PR. Am I missing something?

The main feature of this PR required an update to a vendored library which required this change :-\

OH! Fair enough. :)

I'm not sure changing this file is necessary anymore- we've definitely revendored since November

paddycarver · 2016-11-30T00:37:02Z

builtin/providers/google/resourcemanager_operation.go

+func (w *ResourceManagerOperationWaiter) RefreshFunc() resource.StateRefreshFunc {
+	return func() (interface{}, string, error) {
+		var op *cloudresourcemanager.Operation
+		var err error


Why declare these at the top instead of just instantiating and assigning at once on line 22?

Good point!

paddycarver · 2016-11-30T00:42:01Z

website/source/docs/providers/google/r/google_project.html.markdown

+* `org_id` - (Required) The numeric ID of the organization this project belongs to.
+    Changing this forces a new project to be created.
+
+* `name` - (Optional) The display name of the project.


This says optional, but the schema says required. Which is right?

Schema is correct. Will correct.

cblecker · 2016-11-30T01:09:36Z

This is great, and I'd love to see this in 0.8 if timing works out :)

evandbrown · 2016-11-30T19:16:14Z

@paddyforan thank you for such a quick and thorough review! Really great feedback there.

I had a serendipitous meeting this morning with @wendorf. He asked about this exact feature, and had some great real-world feedback. In a nutshell, I'd like to break out the data_policy and services properties of google_project into separate resources (google_project_iam_policy and google_project_services) that each take a project ID and apply an IAM policy or service configuration. This simplifies how we think about a google_project, and would let us import an existing project or create a new one if it doesn't exist.

On that last point, how do you think about handling imports of something that already exists? Should we auto-detect existence on create? Add an imported property to the resource?

paddycarver · 2016-11-30T20:30:24Z

@evandbrown thanks for the wonderful PR! This is some exciting new functionality, and I'd really like to see it in Terraform.

I think breaking out data policies and services is a great idea, and simplifies thinking about projects a little bit.

As for importing, I've thought about it a bit, and I think it's important to stay with the way Terraform behaves with other resources. That is to say, assume Terraform owns the resource, and will blindly reshape it to match the config, overwriting any changes made outside of Terraform.

When we run into projects that already exist, things get trickier. The "pure" approach, from what I can tell, is to assume the user wants to create a Terraform-managed project any time the project resource is used. In practice, though, I think people will want to reference their existing projects using interpolation in their configs. terraform import could load those projects for people, and that's an option, but they'd have to write their own config for it... though I'm not sure "add three lines to your config" is too much to ask?

My gut says right now to have the config assume the user wants to set the project information (blowing away anything not in the config) and rely on terraform import for projects that exist already. Separating out the IAM policy and enabled services seems like it limits the impact of that--looking at the API, it seems like the most damage we could cause would be removing labels.

Of course, this would mean we need to make projects importable but that seems like a really minor amount of work. I can even supply a patch--I just tested it out locally to make sure I wasn't speaking nonsense. :)

What do you think?

evandbrown · 2016-11-30T22:12:26Z

@paddyforan This all sounds good! Here's my takeaway:

You'll send a patch for import (Thank you! Haven't done one of these and looking forward to seeing it.)
I'll break out IAM and services into google_project_iam_policy and google_project_services
I'll add a new google_project_labels resource to address the astute observation you made about overwriting labels
The google_project resources is for creating new projects. A user can use import to bind an existing project. Terraform owns that project.

Shout-out to @mitchellh and @armon for hiring @paddyforan. You're awesome, Paddy!

paddycarver · 2016-12-01T18:17:03Z

Hey @evandbrown, sorry for the late reply, especially after you said such nice things about me (I blushed 😊)

That takeaway sounds good to me. I'm hoping to open that PR this morning, I just need to add a test case--yesterday got away from me with non-code stuff :(

But that sounds like a great path forward. If you want me to tackle some of those items in the interest of getting support for this released soon, just shout, I'm happy to help--just don't want to step on toes or duplicate effort. :)

evandbrown · 2016-12-01T18:26:07Z

No worries and no rush! Wait, actually, what is the deadline for the next release?

…

On Thu, Dec 1, 2016 at 10:17 AM Paddy ***@***.***> wrote: Hey @evandbrown <https://github.com/evandbrown>, sorry for the late reply, especially after you said such nice things about me (I blushed 😊) That takeaway sounds good to me. I'm hoping to open that PR this morning, I just need to add a test case--yesterday got away from me with non-code stuff :( But that sounds like a great path forward. If you want me to tackle some of those items in the interest of getting support for this released soon, just shout, I'm happy to help--just don't want to step on toes or duplicate effort. :) — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#10425 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AAoGLekcsFCRJ3NAA6ryW_4LeN57rBMKks5rDw8mgaJpZM4K_pAV> .

This change doesn't make much sense now, as projects are read-only anyways, so there's not a lot that importing really does for you--you can already reference pre-existing projects just by defining them in your config. But as we discussed #10425, this change made more and more sense. In a world where projects can be created, we can no longer reference pre-existing projects just by defining them in config. We get that ability back by making projects importable.

paddycarver · 2016-12-02T01:58:10Z

~~Hm. I don't actually know if a date has been set yet.~~ [edit: @mitchellh has the answer below] Importing projects got merged in today, so that's now in 0.8. :) Shout if you want me to pitch in on some of these next steps.

mitchellh · 2016-12-02T01:59:00Z

@evandbrown A deadline for 0.8 final does exist internally, its mid-December. We have another RC coming out tomorrow. We're trying to not introduce any more core backwards incompats, but resources are okay still.

This change doesn't make much sense now, as projects are read-only anyways, so there's not a lot that importing really does for you--you can already reference pre-existing projects just by defining them in your config. But as we discussed hashicorp#10425, this change made more and more sense. In a world where projects can be created, we can no longer reference pre-existing projects just by defining them in config. We get that ability back by making projects importable.

evandbrown · 2016-12-13T05:44:36Z

Whew. That took a bit longer than I thought, @paddyforan! These latest changes:

Rebased off master, including your import changes (thanks!)
google_project continues to function as before (a user can embed IAM policy and services in that resource), but also adds google_project_iam_policy and google_project_services resources to allow easily modifying an existing project.
I didn't get labels in. Will do that in a separate PR.

Critical feedback welcome!

evandbrown · 2016-12-20T06:19:39Z

@paddyforan conflicts resolved. When you have a moment, would appreciate any feedback and see if we can get this merged. Thank you!

paddycarver · 2016-12-20T06:31:33Z

Taking a look at it right now. :)

paddycarver

Hey @evandbrown, sorry to send this back to you, I feel like we're super close. There's some stuff going on with IAM/Services that I don't understand. Also, I think we'll need the read parts for these resources, unless my understanding of things is off (which is totally possible).

Thanks for hanging in there on this PR, I know it's rough. If there's anything I can do to help, or you want me to jump in, I'm happy to roll my sleeves up and implement some of these things. Just let me know. :)

paddycarver · 2016-12-20T06:49:16Z

builtin/providers/google/resource_google_project.go

+	}
+
+	// Set the resourc ID
+	d.SetId(project.ProjectId)


I think we actually need this before the wait, so if the wait times out or takes too long, Terraform knows about the resource in unknown state--that's why the ID is set to an empty string in case of error.

paddycarver · 2016-12-20T07:05:03Z

builtin/providers/google/resource_google_project.go


-		// Merge the existing policy bindings with those defined in this manifest.
-		p.Bindings = mergeBindings(append(p.Bindings, policy.Bindings...))
+	d.Set("number", strconv.FormatInt(int64(p.ProjectNumber), 10))


I think we're missing a few attributes we should be setting here. I think we need to Set the rest of the properties, so we can properly diff.

Ahh, so this would read the actual state from the APIs (which accounts for the possibility that non-computed values were changed out-of-band?) Makes sense, will correct.

paddycarver · 2016-12-20T07:06:09Z

builtin/providers/google/resource_google_project.go

 		if err != nil {
-			return fmt.Errorf("Error applying IAM policy for project %q: %s", project, err)
+			return fmt.Errorf("Error updating project %q: %s", p.Name, err)
 		}
 	}


Should we check for policy data updating, too?

paddycarver · 2016-12-20T07:24:48Z

builtin/providers/google/resource_google_project_iam_policy.go

+
+func resourceGoogleProjectIamPolicyRead(d *schema.ResourceData, meta interface{}) error {
+	// Read is a no-op as we don't care about the actual state of the project's IAM
+	// policy. We always overwrite it during create or update


I think we need to read it anyways, just so we can set it in state to diff against. Will run some tests to find out if I'm wrong. :)

Ahh yes. I was assuming that since we're merging the policy expressed in Terraform with the actual project policy (unless it's overridden with authoritative), we wouldn't care about the actual state. But reading is required to know there's been a change, so update is triggered. Duh. Thanks!

paddycarver · 2016-12-20T07:26:15Z

builtin/providers/google/resource_google_project_services.go

+func resourceGoogleProjectServicesCreate(d *schema.ResourceData, meta interface{}) error {
+	config := meta.(*Config)
+	pid := d.Get("project_id").(string)
+	err := setProjectIamPolicy(d, config, pid)


Is that the right function to call here?

paddycarver · 2016-12-20T07:27:05Z

builtin/providers/google/resource_google_project_services.go

+
+func resourceGoogleProjectServicesRead(d *schema.ResourceData, meta interface{}) error {
+	// Read is a no-op as we don't care about the actual state of the project's IAM
+	// policy. We always overwrite it during create or update


We seem to have gotten IAM and services mixed up here, unless I'm misunderstanding. I think we always want a read though, for things like refreshing state and for importing.

paddycarver · 2016-12-20T07:28:26Z

builtin/providers/google/resource_google_project_services.go

+	log.Printf("[DEBUG]: Deleting google_project_iam_policy")
+	config := meta.(*Config)
+	pid := d.Get("project_id").(string)
+	return updateProjectIamPolicy(d, config, pid)


I'm confused about what's happening here. Why is updating deleting stuff? (Also, we have another rogue IAM reference here)

Blah, forgot to apply a stash and left all of the services stuff in a bad place. Will be fixed in next push.

paddycarver · 2016-12-20T07:30:12Z

builtin/providers/google/resource_google_project_services.go

+
+func resourceGoogleProjectServicesDelete(d *schema.ResourceData, meta interface{}) error {
+	log.Printf("[DEBUG]: Deleting google_project_iam_policy")
+	if err := resourceGoogleProjectServicesUpdate(d, meta); err != nil {


Can you walk me through why we're updating when deleting? There's a logic here I'm not quite following, and feeling pretty sure I missed something.

Deletion is really dissociating a service from a project, and accounts for services that were bound outside of Terraform. The update code supports adding or removing services from existing projects that may have out-of-band service bindings. We're treating delete like an "update project to remove of service bindings".

evandbrown · 2016-12-21T07:28:20Z

Thanks for catching those things, @paddyforan! I'll submit fixes in a new commit tomorrow.

evandbrown · 2017-01-01T07:52:37Z

@paddyforan Took longer than I thought, but I think we're there. We now handle services and IAM policy as separate resources, which is simpler when binding those to existing projects.

When running all of the acceptance tests in the "GoogleProject*" namespace, I noticed the import test fails. It seems like it tries to create a project that already exists. I'm unfamiliar with the import process, and since you added that feature I thought you might have some insight.

Very happy that my last commit of 2016 is to Terraform. Wishing everyone at HashiCorp a happy 2017!

paddycarver · 2017-01-03T23:46:36Z

Hey @evandbrown! Thanks for the update. I'm going to take a look at this today. And super honoured to be your last commit in 2016, of course! Thanks so much for all the work you've put in. A very happy 2017 to you, as well--I hope we hear a lot from each other this year. 😀

When running all of the acceptance tests in the "GoogleProject*" namespace, I noticed the import test fails. It seems like it tries to create a project that already exists. I'm unfamiliar with the import process, and since you added that feature I thought you might have some insight.

That is interesting indeed. I'm going to go ahead and dig into this and report back with my findings. In the meantime, we'll try to get this merged. I'm realising the original projects resource probably should have been a data source, which exacerbates this PR because of backwards compatibility concerns, but no sense in crying over spilt milk. :)

I'm going to have to look for guidance from the rest of the team as to whether this needs to be entirely backwards compatible or wait for 0.9, or if we can live with some backwards incompatibilities in the name of getting this in for 0.8. I'll report back with that, too.

evandbrown · 2017-01-05T06:54:55Z

Thanks, @paddyforan! I just pushed a change that preserves backwards-compatibility. I'm working on a separate commit to do a small migration, as well as a change that should fix the importer. Will ping when that's in.

evandbrown · 2017-01-19T07:10:30Z

@paddyforan Thanks for the latest review. Addressed all of your points, chief of which is making service management authoritative.

paddycarver · 2017-01-19T22:10:31Z

Hey @evandbrown! Thanks for the updates. And man, I kinda feel like the worst right now.

I'm using this config:

resource "google_project" "acctest" {
  id = "hashicorp-terraform-testing"
}

[edit] I should probably mention that I'm using a different project ID in the GOOGLE_PROJECT environment variable, which is how I'm determining when environment variables are used and when the resource attribute is used. [/edit]

removing getProject broke the existing behaviour--if I run terraform apply on that config under 0.8.4, then run it against your branch, I get the following error:
```
* google_project.acctest: One of 'project_id' or 'id' must be set in the config
```
The upshot of this is that merging this as-is would mean that anyone using the google_project resource would have to update their config, which I'd like to avoid. I'm pretty sure if, instead of trying to read the id key, you use getProject like the old code did, that will work as intended (using project_id when set, but falling back on old behaviour otherwise).
I was talking about it last night a bit after hours with some of the other core team, and my understanding is we really want to keep Terraform's behaviour consistent across providers, which means the conf file should be considered authoritative pretty much everywhere. This means we should drop the merging of IAM policies. If you want to talk about it some more, that's cool, and I'm happy to have the conversation, but from my understanding the core team's stance on the issue is any time the provider can drift from the config file without Terraform noticing or trying to bring it back inline, it's a bug. If there's a special case for IAM in this, I'd love to hear more about it.

Really sorry to move the goalposts on that one on you. :(

For the sake of getting this merged (I still want to hit that merge button this week), I'm going to take a crack at updating this with the changes above to do some more backwards compatibility testing. I'm happy to contribute those changes back to the PR (if you enable edits from maintainers on the PR or want me to just PR them into your branch) but I also don't want to step on your toes--you've worked on this for a long time and been very patient, so I'm not bothered by doing some exploratory coding to uncover all the potential problems, then just deleting it and letting you push this across the finish line. Just let me know how you want to proceed. :)

evandbrown · 2017-01-19T22:24:37Z

Hey @paddyforan! The getProject piece sounds good. I'll get that added back today.

I do think IAM policy merging is a behavior we want for project IAM. If you get an IAM policy wrong (and it happens a lot), there's a good chance you lose access to your project and have to find an admin to re-enable it. This is particularly true with the google_project_iam_policy resource that allows attachment to an existing project. The workflow we're supporting here is to set authoritative=false while you're building the config. Your project would have some existing policy that Terraform won't damage. When you're sure the Terraform config is right, flip authoritative=true. Sound good?

evandbrown · 2017-01-19T22:53:28Z

@paddyforan Focusing on project ID here:

Using getProject restores the behavior where the project ID is retrieved from the env (GOOGLE_PROJECT) or the provider configuration config (if it exists). This is that bug where I thought the id attribute of the config was working, but it was actually that project was specified in the provider config and was being used instead of d.Get("id")

So we can restore that behavior, but the create lifecycle still won't work with the template you showed. The reason for that is that the old version was using the create event to effectively import an existing project (this was before importing resources existed, and before the project API supported creation.) Now we're using Terraform's import functionality, and the create function actually really creates a project now.

So from a backwards compatibility perspective, this change should work fine with existing deployments/state. But you can't use that old template to create a new deployment since the meaning of create has changed (and been replaced with import).

Thoughts?

paddycarver · 2017-01-19T23:08:55Z

Re: authoritative: I can see both sides of this, and it feels like a gray area to me. I'm trying to get more guidance on it from people who have more experience than I do, trying to avoid further confusion and goalpost moving. :) I'll update you here when I get more info, hopefully today.

Re: project ID:

So from a backwards compatibility perspective, this change should work fine with existing deployments/state. But you can't use that old template to create a new deployment since the meaning of create has changed (and been replaced with import).

I think this matches what I'm hoping for. My goal is to let people that have configs that work for them right now (even if they only work because of a bug) continue to use those configs and not need to update everything as soon as this is merged. But also, we should probably tell those people that the configs don't actually work the way they think they do. Which is what the deprecation message does.

So reading environment variables on create doesn't feel super important, because any config that already exists won't have the create run for it--it already has the project in state. But on read, it's important, because those configs would think they can't find the ID anymore, which breaks things. On update and destroy, I'd argue it's not important, because that should only happen when the user is changing things, and if they're changing things, they can change to use the project_id attribute anyways. I just want to avoid having terraform apply break immediately after you upgrade when your config hasn't changed at all, if that makes sense.

The way I'm testing this is by using that config file, using the Terraform 0.8.4 binary, and running terraform apply. Then I run terraform apply using a binary built from your branch. Ideally, what I'd see is that no change is necessary, because the config didn't change, but also a warning that id doesn't work. I get the warning, but at the moment, it fails with the error I pasted above.

Hopefully that clears things up! Sorry for the confusion. Let me know if you have questions, or if a quick Hangout would help.

evandbrown · 2017-01-19T23:31:43Z

Cool! So looking at the terraform apply approach you're using, it seems like the state migration func for resource_google_project isn't executing. The second apply sees that project_id is missing, and because of force_new tries to delete then re-create the resource.

The migration func should have taken care of that. Any thoughts on why that's not running?

paddycarver · 2017-01-19T23:38:58Z

I think, if I understand what I'm seeing correctly, it's saying that because project_id is not in the config file, it needs to be removed from the state, which prompts the force_new. We want it to be OK that project_id isn't set in the config, but we also want switching from id to project_id to not force a delete/create cycle.

The answer to the delete/create cycle is the state migration. The answer to the diff... I'm not sure. My gut says we can do a DiffSuppressFunc that ignores diffs where the project_id is set in state but not in the config, but I need to think through the ramifications of that. Is probably worth trying out, though?

evandbrown · 2017-01-20T07:05:47Z

I believe the DiffSupressFunc in my latest push gets us there:

terraform apply this config with 0.8.4:

resource "google_project" "acctest" {
  id = "evandbrown17"
}

Result:

provider.google.region
  Enter a value: 

google_project.acctest: Creating...
  name:   "" => "<computed>"
  number: "" => "<computed>"
google_project.acctest: Creation complete

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

The state of your infrastructure has been saved to the path
below. This state is required to modify and destroy your
infrastructure, so keep it safe. To inspect the complete state
use the `terraform show` command.

State path: terraform.tfstate

terraform apply the same config with dev build.

Result:

terraform apply
[WARN] /home/evanbrown/dev/go-workspaces/terraform/bin/terraform-provider-google overrides an internal plugin for google-provider.
  If you did not expect to see this message you will need to remove the old plugin.
  See https://www.terraform.io/docs/internals/internal-plugins.html
provider.google.region
  Enter a value: 

There are warnings and/or errors related to your configuration. Please
fix these before continuing.

Warnings:

  * google_project.acctest: "id": [DEPRECATED] Please replace this attribute with `project_id`. It will be removed from the schema in Terraform 0.9 and will be computed automatically. See https://www.terraform.io/docs/providers/google/r/google_project.html for a complete explanation.

No errors found. Continuing with 1 warning(s).

google_project.acctest: Refreshing state... (ID: evandbrown17)

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

Migration function is applied and state file contains new attributes, including project_id.

Add project_id to config:

resource "google_project" "acctest" {
  id = "evandbrown17"
  project_id = "evandbrown17"
}

Result: no diff.

Remove id from config:

resource "google_project" "acctest" {
  id = "evandbrown17"
  project_id = "evandbrown17"
}

Result: no diff

paddycarver

Hey @evandbrown! Great work on this. It's probably discouraging to get another review of this with like 19 comments, but the good news is they're all incredibly minor, save two. They're basically all "let's tweak this documentation/wording" or "let's log here so if there's a problem we have all the data and can debug it". The two exceptions:

Let's set policy_data on read for projects, unless I'm missing something. Being consistent about "everything in the config ends up in the state" makes sense to me, unless you left that out for a reason.
Let's use d.Get("project") instead of getProject() for the new resources, so we keep configs as the source of truth--weird things can happen when we start reading from environment variables. At the very least, it means that plan becomes basically worthless--if you set an environment variable when running plan, then don't set it or set it to something else before apply, Terraform will do something different from what's in the plan, which is Bad™

The only other thing is adding a more detailed explanation about how "id" doesn't work as people expect it to, and what actually happens instead.

I'm going to go ahead and push those changes to this right now. If you get a chance tonight to review my changes, let me know if they make sense to you. Then I'll do some last-minute backwards-compatibility testing and we'll get this merged.

paddycarver · 2017-01-24T00:05:30Z

builtin/providers/google/resource_google_project.go

+				Type:       schema.TypeString,
+				Optional:   true,
+				Computed:   true,
+				Deprecated: "Please replace this attribute with `project_id`. It will be removed from the schema in Terraform 0.9 and will be computed automatically. See https://www.terraform.io/docs/providers/google/r/google_project.html for a complete explanation.",


This feels a bit ambiguous to me. Not to bikeshed, but I think something like

The id field has unexpected behaviour and probably doesn't do what you expect. See https://www.terraform.io/docs/providers/google/r/google_project.html#id-field for more information. Please use project_id instead; future versions of Terraform will remove the id field.

paddycarver · 2017-01-24T00:06:12Z

builtin/providers/google/resource_google_project.go

+		}
+	}
+	if pid == "" {
+		return fmt.Errorf("One of 'project_id' or 'id' must be set in the config")


I'd remove or 'id' from this--we should try to discourage any new use of id.

paddycarver · 2017-01-24T00:07:02Z

builtin/providers/google/resource_google_project.go

 	}

 	// Apply the IAM policy if it is set
 	if pString, ok := d.GetOk("policy_data"); ok {
 		// The policy string is just a marshaled cloudresourcemanager.Policy.
 		// Unmarshal it to a struct.
 		var policy cloudresourcemanager.Policy
-		if err = json.Unmarshal([]byte(pString.(string)), &policy); err != nil {
+		if err := json.Unmarshal([]byte(pString.(string)), &policy); err != nil {
 			return err
 		}


For debug purposes, it'd be great to log policy.Bindings here, just so we can track any potential weirdness in people's merging.

paddycarver · 2017-01-24T00:07:57Z

builtin/providers/google/resource_google_project.go

+	if err != nil {
+		return err
+	}
+	d.Set("policy_etag", pol.Etag)


Should we be setting policy_data here, as well?

paddycarver · 2017-01-24T00:10:35Z

builtin/providers/google/resource_google_project_iam_policy.go

+
+func resourceGoogleProjectIamPolicyCreate(d *schema.ResourceData, meta interface{}) error {
+	config := meta.(*Config)
+	pid, err := getProject(d, config)


This is new behaviour, right? There are no backwards compatibility concerns here? If so, I think we can just use d.Get("project"), right? Then we keep configs authoritative, and since the config requires a project to be set, there's no reason we'd need to fall back on anything, right?

paddycarver · 2017-01-24T00:31:11Z

website/source/docs/providers/google/r/google_project.html.markdown

+
+* `policy_data` - (Deprecated) This argument is no longer supported, and
+    an error will be thrown if it is present. It must be replaced with a 
+    `google_project_iam_policy` resource.


I think we should probably soften the language around this, because it makes it sound like it no longer works even though it used to, which isn't (AFAIK) actually the case. It still works, but we suggest moving off it, and people will get a warning to move off it.

paddycarver · 2017-01-24T00:31:52Z

website/source/docs/providers/google/r/google_project.html.markdown

 * `number` - The numeric identifier of the project.
+* `policy_etag` - (Deprecated) The etag of the project's IAM policy, used to
+    determine if the IAM policy has changed.


We should mention that people should use the google_project_iam_policy resource instead.

paddycarver · 2017-01-24T00:33:28Z

website/source/docs/providers/google/r/google_project_iam_policy.html.markdown

+* `project` - (Required) The project ID.
+    Changing this forces a new project to be created.
+
+* `policy_data` - (Optional) The `google_iam_policy` data source that represents


This is actually required, according to the Schema.

paddycarver · 2017-01-24T00:34:24Z

website/source/docs/providers/google/r/google_project_iam_policy.html.markdown

+    project policy and the data source policy, they will be removed.
+
+* `authoritative` - (Optional) A boolean value indicating if this policy
+    should overwrite any existing IAM policy on the project. If this argument


Let's mention here that this includes removing any IAM policies not specified in your config, and that that could mean you get locked out of your project.

paddycarver · 2017-01-24T00:35:16Z

website/source/docs/providers/google/r/google_project_services.html.markdown

+```js
+resource "google_project_services" "project" {
+    project_id = "your-project-id"
+	services = ["iam.googleapis.com", "cloudresourcemanager.googleapis.com"]


Weird indentation.

We've worked through all the issues suggested, and I've pushed a few commits of my own, which now need review, so I can't approve this PR, but it also shouldn't show that I'm asking for changes.

evandbrown · 2017-01-24T21:16:20Z

Haha I can't approve the changes either as I'm the PR author 😆

But here's my old-school approval: LGTM

danawillow · 2017-01-24T21:18:56Z

I can look at it, I'll do that now.

paddycarver · 2017-01-24T21:29:38Z

I'd be satisfied with @evandbrown checking my changes and verifying they make sense, and I've checked his changes and verified they make sense, so at least two pairs of eyes hit everything.

That said, having @danawillow check things is excellent / 💯 / even better, so if you have the bandwidth and don't mind doing it, that'd be fantastic, and we could then be super confident merging.

Merging still needs to block until we get the quota thing worked out, but it sounds like we're super close on that.

danawillow

LGTM. Reviewed mostly for style/readability since the two of you went through the design so heavily. Also it looks like there are a few comments that you left @paddyforan in the md files that haven't been dealt with yet.

danawillow · 2017-01-24T21:19:44Z

builtin/providers/google/config.go

@@ -151,6 +153,13 @@ func (c *Config) loadAndValidate() error {
 	}
 	c.clientIAM.UserAgent = userAgent

+	log.Printf("[INFO] Instatiating Google Cloud Service Management Client...")


nit: instantiating

danawillow · 2017-01-24T21:22:40Z

builtin/providers/google/import_google_project_test.go

@@ -23,7 +25,19 @@ func TestAccGoogleProject_importBasic(t *testing.T) {
 				ResourceName:      resourceName,
 				ImportState:       true,
 				ImportStateVerify: true,
+				ImportStateVerifyIgnore: []string{
+					"skip_delete",


Can you add a quick comment as to why we ignore this field when verifying?

I can't actually explain this one. @evandbrown said it was necessary to make tests pass, though I just ran the test now, and it passed without the ImportStateVerifyIgnore. I've removed it, and we can fix the test later if the bug pops up again.

danawillow · 2017-01-24T21:34:37Z

builtin/providers/google/resource_google_project.go

+			return fmt.Errorf("Error getting project ID: %v", err)
+		}
+	}
+	if pid == "" {


Optional: I'd put this inside the above if statement, since the only way pid could be empty in the second one is if it was empty in the first one.

danawillow · 2017-01-24T21:35:17Z

builtin/providers/google/resource_google_project.go

+	}
+
+	name, org_id := d.Get("name").(string), d.Get("org_id").(string)
+	if name == "" {


Add a comment as to why these statements are here rather than making the fields Required

danawillow · 2017-01-24T22:09:08Z

builtin/providers/google/resource_google_project.go

+		},
+	}
+
+	op, err := config.clientResourceManager.Projects.Create(project).Do()


does this actually build? (since err was already declared)

Yup! You only get errors about variables already being declared if they're the only variable being declared: https://play.golang.org/p/b0qIqFpVrv vs https://play.golang.org/p/L_G5UBLL4d

danawillow · 2017-01-24T22:17:05Z

builtin/providers/google/resource_google_project.go

-	project, err := getProject(d, config)
+	pid := d.Id()
+
+	// Read the project


Can you make this a comment in the code in case others have the same question?

danawillow · 2017-01-24T22:25:16Z

builtin/providers/google/resource_google_project_iam_policy.go

+		rp := subtractIamPolicy(ep, p)
+		rps, err := json.Marshal(rp)
+		if err != nil {
+			return fmt.Errorf("Error marhsaling restorable IAM policy: %v", err)


typo: marshaling

danawillow · 2017-01-24T22:52:11Z

builtin/providers/google/resource_google_project_services_test.go

+func testAccGoogleProjectAssociateServicesBasic(services []string, pid, name, org string) string {
+	return fmt.Sprintf(`
+resource "google_project" "acceptance" {
+    project_id = "%s"


fyi the reason the indentation looks messed up here is because it's a mix of tabs and spaces. up to you if you want to fix it.

danawillow · 2017-01-24T22:53:40Z

builtin/providers/google/resource_storage_object_acl.go

-				log.Printf("[DEBUG]: saving re %s-%s", role, entity)
+			if _, in := re_local_map[v.Entity]; in {
+				role_entity = append(role_entity, fmt.Sprintf("%s:%s", v.Role, v.Entity))
+				log.Printf("[DEBUG]: saving re %s-%s", v.Role, v.Entity)


I'm not sure changing this file is necessary anymore- we've definitely revendored since November

paddycarver · 2017-01-25T08:48:13Z

128 comments, 21 emails, and a Hangout later, I think this one is finally ready. :D My tests all pass, and manual testing has shown that UX is as expected, as far as I can tell. If either of y'all can verify that your LGTMs stand in light of the recent changes, we can merge this when ready. :D

danawillow · 2017-01-25T17:31:41Z

builtin/providers/google/resource_google_project.go

+				Type:             schema.TypeString,
+				Optional:         true,
+				Computed:         true,
+				Deprecated:       "Use the the 'google_project_iam_policy' resource to define policies for a Google Project",


typo: the the

danawillow · 2017-01-25T17:50:24Z

New changes LGTM

evandbrown · 2017-01-25T17:54:03Z

LGTM OMG WOOOO!

…

On Wed, Jan 25, 2017 at 7:50 AM Dana Hoffman ***@***.***> wrote: New changes LGTM — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#10425 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AAoGLT9fb8OXlkyDsFekj4o4fVoSy95Nks5rV4tsgaJpZM4K_pAV> .

Add support for creating, updating, and deleting projects, as well as their enabled services and their IAM policies. Various concessions were made for backwards compatibility, and will be removed in 0.9 or 0.10.

ghost · 2020-04-17T02:25:36Z

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 have found a problem that seems similar to this, please open a new issue and complete the issue template so we can capture all the details necessary to investigate further.

evandbrown added enhancement provider/google-cloud labels Nov 30, 2016

paddycarver suggested changes Nov 30, 2016

View reviewed changes

evandbrown changed the title ~~Create and manage Google Cloud Platform Projects~~ providers/google: Create and manage Google Cloud Platform Projects Dec 1, 2016

paddycarver mentioned this pull request Dec 1, 2016

providers/google: make projects importable. #10469

Merged

paddycarver added the waiting-response An issue/pull request is waiting for a response from the community label Dec 7, 2016

evandbrown force-pushed the google-project-templates branch from 0405f0f to 5f1dd34 Compare December 13, 2016 05:28

evandbrown force-pushed the google-project-templates branch from 74d82b7 to e7c8b71 Compare December 20, 2016 06:18

paddycarver previously requested changes Dec 20, 2016

View reviewed changes

evandbrown force-pushed the google-project-templates branch from e7c8b71 to 25b4d96 Compare January 1, 2017 07:48

evandbrown force-pushed the google-project-templates branch from d9f2546 to 25e9562 Compare January 6, 2017 05:08

paddycarver reviewed Jan 24, 2017

View reviewed changes

paddycarver force-pushed the google-project-templates branch from 792ac4f to f4695d3 Compare January 24, 2017 10:35

danawillow approved these changes Jan 24, 2017

View reviewed changes

paddycarver added new-resource and removed waiting-response An issue/pull request is waiting for a response from the community labels Jan 25, 2017

paddycarver approved these changes Jan 25, 2017

View reviewed changes

danawillow reviewed Jan 25, 2017

View reviewed changes

providers/google: Support managing projects

b9e9e77

Add support for creating, updating, and deleting projects, as well as their enabled services and their IAM policies. Various concessions were made for backwards compatibility, and will be removed in 0.9 or 0.10.

paddycarver force-pushed the google-project-templates branch from f029bc1 to b9e9e77 Compare January 25, 2017 23:04

paddycarver merged commit 54aca85 into hashicorp:master Jan 25, 2017

paddycarver mentioned this pull request Jan 30, 2017

Support for Google Cloud IAM #9929

Closed

paddycarver mentioned this pull request Feb 22, 2017

google cloud: regression - enable reading of existing project on create #11895

Closed

paddycarver mentioned this pull request Sep 6, 2017

FAIL: TestProvider: id is a reserved field name for a resource with latests github.com/hashicorp/terraform hashicorp/terraform-provider-google#353

Closed

ghost locked and limited conversation to collaborators Apr 17, 2020

providers/google: Create and manage Google Cloud Platform Projects #10425

providers/google: Create and manage Google Cloud Platform Projects #10425

Conversation

evandbrown commented Nov 30, 2016 • edited Loading

evandbrown commented Nov 30, 2016

evandbrown commented Nov 30, 2016

paddycarver 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

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

cblecker commented Nov 30, 2016

evandbrown commented Nov 30, 2016

paddycarver commented Nov 30, 2016

evandbrown commented Nov 30, 2016

paddycarver commented Dec 1, 2016

evandbrown commented Dec 1, 2016 via email

paddycarver commented Dec 2, 2016 • edited Loading

mitchellh commented Dec 2, 2016

evandbrown commented Dec 13, 2016

evandbrown commented Dec 20, 2016

paddycarver commented Dec 20, 2016

paddycarver 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

evandbrown commented Dec 21, 2016

evandbrown commented Jan 1, 2017

paddycarver commented Jan 3, 2017

evandbrown commented Jan 5, 2017 • edited Loading

evandbrown commented Jan 19, 2017

paddycarver commented Jan 19, 2017 • edited Loading

evandbrown commented Jan 19, 2017

evandbrown commented Jan 19, 2017

paddycarver commented Jan 19, 2017

evandbrown commented Jan 19, 2017

paddycarver commented Jan 19, 2017

evandbrown commented Jan 20, 2017

paddycarver 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

evandbrown commented Nov 30, 2016 •

edited

Loading

paddycarver commented Dec 2, 2016 •

edited

Loading

evandbrown commented Jan 5, 2017 •

edited

Loading

paddycarver commented Jan 19, 2017 •

edited

Loading