Add IAP support for backend services #471

Cidan · 2017-09-27T04:46:29Z

WIP PR, do not merge yet

Adds IAP support for #444

Cidan · 2017-09-27T05:32:59Z

I think this is more or less good to go, though I might be missing something. @danawillow, whatcha think?

Cidan · 2017-09-27T05:42:14Z

sorry, one last change, wrote a reciprocal function for flatten to keep in line with previous work. now it should be good!

danawillow

Thanks @Cidan! Can you add tests in resource_compute_backend_service_test.go? I think a test with one step that sets up the backend service with IAP enabled and another that disables IAP would be good.

danawillow · 2017-09-27T17:04:18Z

google/resource_compute_backend_service.go

@@ -37,6 +37,32 @@ func resourceComputeBackendService() *schema.Resource {
 				MaxItems: 1,
 			},

+			"iap": &schema.Schema{
+				Type: schema.TypeSet,


Since it's MaxItems: 1 anyway, I'd recommend just making this a list. Then you can also get rid of the Set fn on line 63

danawillow · 2017-09-27T17:06:01Z

google/resource_compute_backend_service.go

@@ -37,6 +37,32 @@ func resourceComputeBackendService() *schema.Resource {
 				MaxItems: 1,
 			},

+			"iap": &schema.Schema{
+				Type: schema.TypeSet,
+				Elem: &schema.Resource{


Most of our other resources have Elem as the last item in the struct. I'm not sure why this resource does it the other way but it would be awesome if you could move this one and change some of the others in this file too!

danawillow · 2017-09-27T17:14:09Z

google/resource_compute_backend_service.go

+					Schema: map[string]*schema.Schema{
+						"enabled": &schema.Schema{
+							Type:     schema.TypeBool,
+							Optional: true,


I wonder whether we should make this Required, that way people don't accidentally try to enable it by putting in the block but forgetting this field

I considered this as well. I think about how I work, and commenting out IAP enabled would "disable" it, without having to edit the line and set it as false. What's the standard here?

Something else to consider here, should we even have the enabled flag? Should it be implicit by providing the keys?

danawillow · 2017-09-27T17:39:56Z

google/resource_compute_backend_service.go

+func expandIap(configured []interface{}) *compute.BackendServiceIAP {
+	data := configured[0].(map[string]interface{})
+	iap := &compute.BackendServiceIAP{
+		Enabled:            data["enabled"].(bool),


Without also setting ForceSendFields, these three fields won't appear in the request if they're set to empty (or false). It might still work since the outer block will still be there, but if you could check that you can disable iap using this code that would be great.

I will double check and also add tests.

danawillow · 2017-09-27T17:40:22Z

google/resource_compute_backend_service.go

@@ -203,6 +229,7 @@ func resourceComputeBackendServiceRead(d *schema.ResourceData, meta interface{})
 	d.Set("self_link", service.SelfLink)
 	d.Set("backend", flattenBackends(service.Backends))
 	d.Set("connection_draining_timeout_sec", service.ConnectionDraining.DrainingTimeoutSec)
+	d.Set("iap", flattenIap(service.Iap))


will service.Iap ever be nil?

Only in-so-far as the service object doesn't initialize it a few lines up from here. I'll add a check in the flatten function.

Removed the enabled flag for IAP IAP is now enabled when the client id and secret are set IAP now correctly disables when IAP stanza is removed Client secret is now correctly hashed against the secret hash stored on the server

Cidan · 2017-09-28T05:17:07Z

Alright, had to make quite a few changes here. I'll try to explain my reasoning.

There seems to be a bug with the IAP API it self where if you disable IAP and attempt to zero out the secret (ie, remove the entire stanza), only the Enable -> false flag is set and the client secret is not cleared. As a result, the next time an apply is called, the state doesn't match and the apply will happen a second time, correctly clearing the key.

So, what I did was removed the notion of enable as an option. When IAP is added to a backend, you must provide a key and secret to enable it. Dropping the stanza for IAP disables it -- this is somewhat cleaner since the API it self requires the id and secret when enabling anyway. I've also made the service send up a default empty IAP object via ForceSendFields when the IAP stanza is missing, thus disabling the service if the previous state was enabled.

The IAP API never returns the secret once it's sent, only its' SHA256 sum, so I've added a function for comparing the SHA256 sum we get from the API vs the sum of the locally defined key.

Tests still to come.

Cidan · 2017-09-29T01:39:19Z

Tests written and pass acceptance tests. @danawillow, how does it look now?

danawillow

Looks like this got a bit out-of-date, mind merging master in? Also it looks like the test failed:

$ make testacc TEST=./google TESTARGS='-run=TestAccComputeBackendService_withBackendAndIAP'
==> Checking that code complies with gofmt requirements...
TF_ACC=1 go test ./google -v -run=TestAccComputeBackendService_withBackendAndIAP -timeout 120m
=== RUN   TestAccComputeBackendService_withBackendAndIAP
--- FAIL: TestAccComputeBackendService_withBackendAndIAP (92.66s)
	testing.go:435: Step 0 error: After applying this step, the plan was not empty:

		DIFF:

		UPDATE: google_compute_instance_group_manager.foobar
		  instance_template: "https://www.googleapis.com/compute/v1/projects/graphite-test-danahoffman-tf/global/instanceTemplates/tf-test-louv6hr7uj" => "<computed>"
		DESTROY/CREATE: google_compute_instance_template.foobar
		  can_ip_forward:                         "false" => "false"
		  disk.#:                                 "1" => "1"
		  disk.0.auto_delete:                     "true" => "true"
		  disk.0.boot:                            "true" => "true"
		  disk.0.device_name:                     "persistent-disk-0" => "" (forces new resource)
		  disk.0.disk_type:                       "pd-standard" => "<computed>"
		  disk.0.interface:                       "SCSI" => "<computed>"
		  disk.0.mode:                            "READ_WRITE" => "<computed>"
		  disk.0.source_image:                    "debian-8-jessie-v20160803" => "debian-8-jessie-v20160803"
		  disk.0.type:                            "PERSISTENT" => "<computed>"
		  machine_type:                           "n1-standard-1" => "n1-standard-1"
		  metadata_fingerprint:                   "zxKvLKF-n4o=" => "<computed>"
		  name:                                   "tf-test-louv6hr7uj" => "tf-test-louv6hr7uj"
		  network_interface.#:                    "1" => "1"
		  network_interface.0.network:            "default" => "default"
		  network_interface.0.subnetwork_project: "" => "<computed>"
		  project:                                "graphite-test-danahoffman-tf" => "<computed>"
		  scheduling.#:                           "1" => "<computed>"
		  self_link:                              "https://www.googleapis.com/compute/v1/projects/graphite-test-danahoffman-tf/global/instanceTemplates/tf-test-louv6hr7uj" => "<computed>"
		  tags_fingerprint:                       "" => "<computed>"

		STATE:

		google_compute_backend_service.lipsum:
		  ID = tf-test-0cr9fil17g
		  backend.# = 1
		  backend.2480256626.balancing_mode = UTILIZATION
		  backend.2480256626.capacity_scaler = 1
		  backend.2480256626.description =
		  backend.2480256626.group = https://www.googleapis.com/compute/v1/projects/graphite-test-danahoffman-tf/zones/us-central1-f/instanceGroups/tf-test-j0hrqsyeol
		  backend.2480256626.max_rate = 0
		  backend.2480256626.max_rate_per_instance = 0
		  backend.2480256626.max_utilization = 0.8
		  connection_draining_timeout_sec = 300
		  description = Hello World 1234
		  enable_cdn = false
		  fingerprint = ogC68Pi2rHI=
		  health_checks.# = 1
		  health_checks.4210293762 = https://www.googleapis.com/compute/v1/projects/graphite-test-danahoffman-tf/global/httpHealthChecks/tf-test-dx7gfqocw2
		  iap.# = 1
		  iap.0.oauth2_client_id = test
		  iap.0.oauth2_client_secret = test
		  name = tf-test-0cr9fil17g
		  port_name = http
		  protocol = HTTP
		  self_link = https://www.googleapis.com/compute/v1/projects/graphite-test-danahoffman-tf/global/backendServices/tf-test-0cr9fil17g
		  session_affinity = NONE
		  timeout_sec = 10

		  Dependencies:
		    google_compute_http_health_check.default
		    google_compute_instance_group_manager.foobar
		google_compute_http_health_check.default:
		  ID = tf-test-dx7gfqocw2
		  check_interval_sec = 1
		  description =
		  healthy_threshold = 2
		  host =
		  name = tf-test-dx7gfqocw2
		  port = 80
		  project = graphite-test-danahoffman-tf
		  request_path = /
		  self_link = https://www.googleapis.com/compute/v1/projects/graphite-test-danahoffman-tf/global/httpHealthChecks/tf-test-dx7gfqocw2
		  timeout_sec = 1
		  unhealthy_threshold = 2
		google_compute_instance_group_manager.foobar:
		  ID = tf-test-j0hrqsyeol
		  auto_healing_policies.# = 0
		  base_instance_name = foobar
		  description =
		  fingerprint = 8ibhjaSOFBE=
		  instance_group = https://www.googleapis.com/compute/v1/projects/graphite-test-danahoffman-tf/zones/us-central1-f/instanceGroups/tf-test-j0hrqsyeol
		  instance_template = https://www.googleapis.com/compute/v1/projects/graphite-test-danahoffman-tf/global/instanceTemplates/tf-test-louv6hr7uj
		  name = tf-test-j0hrqsyeol
		  named_port.# = 0
		  project = graphite-test-danahoffman-tf
		  self_link = https://www.googleapis.com/compute/v1/projects/graphite-test-danahoffman-tf/zones/us-central1-f/instanceGroupManagers/tf-test-j0hrqsyeol
		  target_pools.# = 0
		  target_size = 1
		  update_strategy = RESTART
		  zone = us-central1-f

		  Dependencies:
		    google_compute_instance_template.foobar
		google_compute_instance_template.foobar:
		  ID = tf-test-louv6hr7uj
		  can_ip_forward = false
		  description =
		  disk.# = 1
		  disk.0.auto_delete = true
		  disk.0.boot = true
		  disk.0.device_name = persistent-disk-0
		  disk.0.disk_name =
		  disk.0.disk_size_gb = 0
		  disk.0.disk_type = pd-standard
		  disk.0.interface = SCSI
		  disk.0.mode = READ_WRITE
		  disk.0.source =
		  disk.0.source_image = debian-8-jessie-v20160803
		  disk.0.type = PERSISTENT
		  instance_description =
		  machine_type = n1-standard-1
		  metadata.% = 0
		  metadata_fingerprint = zxKvLKF-n4o=
		  name = tf-test-louv6hr7uj
		  network_interface.# = 1
		  network_interface.0.access_config.# = 0
		  network_interface.0.network = default
		  network_interface.0.network_ip =
		  network_interface.0.subnetwork =
		  network_interface.0.subnetwork_project =
		  project = graphite-test-danahoffman-tf
		  scheduling.# = 1
		  scheduling.0.automatic_restart = true
		  scheduling.0.on_host_maintenance = MIGRATE
		  scheduling.0.preemptible = false
		  self_link = https://www.googleapis.com/compute/v1/projects/graphite-test-danahoffman-tf/global/instanceTemplates/tf-test-louv6hr7uj
FAIL
exit status 1
FAIL	github.com/terraform-providers/terraform-provider-google/google	92.894s
make: *** [testacc] Error 1

danawillow · 2017-10-17T16:54:39Z

google/resource_compute_backend_service.go

+
+func flattenIap(iap *compute.BackendServiceIAP) []map[string]interface{} {
+	if iap == nil {
+		return make([]map[string]interface{}, 1, 1)


just curious, why 1, 1?

Because this is called directly on the ResourceData object via .Set, which will always have at most one IAP object. I'm not sure why the IAP API has this as a list -- perhaps future expansion of IAP options and credentials?

Sorry, my question wasn't "why is this a list?", but "why is it being initialized with 1, 1 instead of 0, 1?"
0, 1 will have the list not have anything in it, whereas 1, 1 will have the list have an empty map in it (see https://play.golang.org/p/SFCt6l5Fsw). The flatten functions for all other resources are currently using 0, 1.

Ah, my apologies, I misread your question. Will adjust, :)

danawillow · 2017-10-17T16:55:40Z

google/resource_compute_backend_service.go

+		"oauth2_client_id":     iap.Oauth2ClientId,
+		"oauth2_client_secret": iap.Oauth2ClientSecretSha256,
+	}
+	result = append(result, iapMap)


Totally up to you, but a shorter way to do this would be:

iapMap := map[string]interface{}{ // properties } return []map[string]interface{}{iapMap}

I hadn't considered that -- I agree, much cleaner.

danawillow · 2017-10-17T16:56:39Z

google/resource_compute_backend_service.go

@@ -326,6 +379,13 @@ func expandBackendService(d *schema.ResourceData) compute.BackendService {
 	service := compute.BackendService{
 		Name:         d.Get("name").(string),
 		HealthChecks: healthChecks,
+		Iap: &compute.BackendServiceIAP{
+			ForceSendFields: []string{"Enabled", "Oauth2ClientId", "Oauth2ClientSecret"},


Can you add a comment as to why Iap has to be set (and why the three fields are force)?

danawillow · 2017-10-17T16:57:51Z

google/resource_compute_backend_service_test.go

+		},
+	})
+
+	if svc.TimeoutSec != 10 {


Eh, I don't necessarily mind having these checks here but they're not really what the test is testing.

Other tests do this too, even though it's not relevant (and I agree, I found it odd), so I decided to leave in for this test as well.

danawillow · 2017-10-17T17:01:42Z

google/resource_compute_backend_service_test.go

@@ -273,6 +314,38 @@ func testAccCheckComputeBackendServiceExists(n string, svc *compute.BackendServi
 	}
 }

+func testAccCheckComputeBackendServiceExistsWithIAP(n string, svc *compute.BackendService) resource.TestCheckFunc {


Instead of a full testcheck here you could just use the existing one and then do checks on the returned svc like you're already doing.

I actually prefer the testcheck personally versus the svc object test, as it feels more complete -- I actually would rather rip the svc checks out since they seem like they aren't really needed.

In what way is it more complete? Both ways would do the exact same thing, except one is duplicating a lot of code between functions and the other is calling an existing function.

I see your point. Will adjust.

Actually, I take it back. I started to implement the svc object check when I remembered why I don't like it, particularly for multi-stage tests. The svc object is ambiguous in both timing, and in scope. When is the svc object actually set? After destruction? Before? What about when you have multiple stages, which stage are we checking, IAP being set, or not? The answer may seem obvious, and it is (it's after all tests run), but it's not explicit and self contained. What we end up with is two different ways to validate a test run, and picking which way to test now follows a branching pattern (single stage? test svc. multi stage? test all but last stage with svc, and other stages with a check function).

It makes more sense to me to follow a single pattern -- check each stage with a function and be explicit about intent.

danawillow · 2017-10-17T17:02:08Z

google/resource_compute_backend_service_test.go

+  timeout_sec = %v
+
+  backend {
+    group = "${google_compute_instance_group_manager.foobar.instance_group}"


can you fix the formatting here? I think it's a tabs vs spaces issue

Cidan · 2017-10-21T02:48:35Z

@danawillow That's interesting on the test failure -- I can't reproduce it. Any guidance on your setup?

==> Checking that code complies with gofmt requirements...
TF_ACC=1 go test ./google -v -run=TestAccComputeBackendService_withBackendAndIAP -timeout 120m
=== RUN   TestAccComputeBackendService_withBackendAndIAP
--- PASS: TestAccComputeBackendService_withBackendAndIAP (128.16s)
PASS
ok  	github.com/terraform-providers/terraform-provider-google/google	128.190s

danawillow · 2017-10-23T22:13:01Z

google/resource_compute_backend_service.go

+
+func flattenIap(iap *compute.BackendServiceIAP) []map[string]interface{} {
+	if iap == nil {
+		return make([]map[string]interface{}, 1, 1)


Sorry, my question wasn't "why is this a list?", but "why is it being initialized with 1, 1 instead of 0, 1?"
0, 1 will have the list not have anything in it, whereas 1, 1 will have the list have an empty map in it (see https://play.golang.org/p/SFCt6l5Fsw). The flatten functions for all other resources are currently using 0, 1.

danawillow · 2017-10-23T22:15:29Z

google/resource_compute_backend_service_test.go

+				Config: testAccComputeBackendService_withBackend(
+					serviceName, igName, itName, checkName, 10),
+				Check: resource.ComposeTestCheckFunc(
+					testAccCheckComputeBackendServiceExists(


This checks that the backend service exists, but it doesn't check that IAP actually got disabled (I'd highly recommend just adding that check in below with the other checks on svc)

danawillow · 2017-10-23T22:16:48Z

google/resource_compute_backend_service_test.go

@@ -273,6 +314,38 @@ func testAccCheckComputeBackendServiceExists(n string, svc *compute.BackendServi
 	}
 }

+func testAccCheckComputeBackendServiceExistsWithIAP(n string, svc *compute.BackendService) resource.TestCheckFunc {


In what way is it more complete? Both ways would do the exact same thing, except one is duplicating a lot of code between functions and the other is calling an existing function.

Cidan · 2017-10-31T16:51:10Z

@danawillow Alright, let's try this one more time!

Made the recommended changes, and testing now checks for IAP enabled and disabled properly.

danawillow · 2017-10-31T23:26:56Z

Thanks @Cidan!

Signed-off-by: Modular Magician <magic-modules@google.com>

ghost · 2020-03-30T14:05:51Z

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 👉 hashibot-feedback@hashicorp.com. Thanks!

Antonio Lobato added 3 commits September 26, 2017 21:40

initial work on adding IAP support for backend services

ae5ad95

readback of IAP

79df895

flatten IAP + static set id

7b02cdb

expandIap function

b67c08e

danawillow self-requested a review September 27, 2017 16:54

danawillow self-assigned this Sep 27, 2017

danawillow suggested changes Sep 27, 2017

View reviewed changes

removed enabled flag/state rework

e865ea2

Removed the enabled flag for IAP IAP is now enabled when the client id and secret are set IAP now correctly disables when IAP stanza is removed Client secret is now correctly hashed against the secret hash stored on the server

Tests for IAP

285b10b

Cidan changed the title ~~initial work on adding IAP support for backend services~~ Add IAP support for backend services Sep 29, 2017

radeksimko added the enhancement label Oct 8, 2017

danawillow suggested changes Oct 17, 2017

View reviewed changes

Cidan force-pushed the add_iap branch from 64d69fa to 285b10b Compare October 17, 2017 21:19

Antonio Lobato added 2 commits October 20, 2017 19:31

added comments, fixed tabs.

9de4d86

merge master

ec4afb2

danawillow reviewed Oct 23, 2017

View reviewed changes

testing for IAP disabled

95bb1c6

danawillow approved these changes Oct 31, 2017

View reviewed changes

danawillow merged commit d37f352 into hashicorp:master Oct 31, 2017

rosbo mentioned this pull request Dec 11, 2017

Add IAP documentation #839

Merged

Cidan deleted the add_iap branch December 11, 2017 17:47

andyshinn mentioned this pull request Apr 3, 2018

Resource to create OAuth 2.0 credentials #1287

Closed

luis-silva pushed a commit to luis-silva/terraform-provider-google that referenced this pull request May 21, 2019

[Terraform] Fix our usage of file in tests. (hashicorp#471)

ee1e071

Signed-off-by: Modular Magician <magic-modules@google.com>

ghost locked and limited conversation to collaborators Mar 30, 2020

Add IAP support for backend services #471

Add IAP support for backend services #471

Conversation

Cidan commented Sep 27, 2017

Cidan commented Sep 27, 2017

Cidan commented Sep 27, 2017

danawillow 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

Cidan commented Sep 28, 2017 • edited Loading

Cidan commented Sep 29, 2017

danawillow 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

Cidan commented Oct 21, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Cidan commented Oct 31, 2017

danawillow commented Oct 31, 2017

ghost commented Mar 30, 2020

Cidan commented Sep 28, 2017 •

edited

Loading