Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add IAP support for backend services #471

Merged
merged 9 commits into from
Oct 31, 2017
Merged

Add IAP support for backend services #471

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
9 commits
Select commit Hold shift + click to select a range
ae5ad95
initial work on adding IAP support for backend services
Sep 27, 2017
79df895
readback of IAP
Sep 27, 2017
7b02cdb
flatten IAP + static set id
Sep 27, 2017
b67c08e
expandIap function
Sep 27, 2017
e865ea2
removed enabled flag/state rework
Sep 28, 2017
285b10b
Tests for IAP
Sep 29, 2017
9de4d86
added comments, fixed tabs.
Oct 21, 2017
ec4afb2
merge master
Oct 21, 2017
95bb1c6
testing for IAP disabled
Oct 31, 2017
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
73 changes: 69 additions & 4 deletions google/resource_compute_backend_service.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@ package google

import (
"bytes"
"crypto/sha256"
"errors"
"fmt"
"log"
Expand Down Expand Up @@ -39,8 +40,35 @@ func resourceComputeBackendService() *schema.Resource {
MaxItems: 1,
},

"iap": &schema.Schema{
Type: schema.TypeList,
Optional: true,
MaxItems: 1,
Elem: &schema.Resource{
Schema: map[string]*schema.Schema{
"oauth2_client_id": &schema.Schema{
Type: schema.TypeString,
Required: true,
},
"oauth2_client_secret": &schema.Schema{
Type: schema.TypeString,
Required: true,
Sensitive: true,
DiffSuppressFunc: func(k, old, new string, d *schema.ResourceData) bool {
if old == fmt.Sprintf("%x", sha256.Sum256([]byte(new))) {
return true
}
return false
},
},
},
},
},

"backend": &schema.Schema{
Type: schema.TypeSet,
Type: schema.TypeSet,
Optional: true,
Set: resourceGoogleComputeBackendServiceBackendHash,
Elem: &schema.Resource{
Schema: map[string]*schema.Schema{
"group": &schema.Schema{
Expand Down Expand Up @@ -77,8 +105,6 @@ func resourceComputeBackendService() *schema.Resource {
},
},
},
Optional: true,
Set: resourceGoogleComputeBackendServiceBackendHash,
},

"description": &schema.Schema{
Expand Down Expand Up @@ -209,6 +235,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))
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

will service.Iap ever be nil?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.


d.Set("health_checks", service.HealthChecks)

Expand Down Expand Up @@ -270,6 +297,31 @@ func resourceComputeBackendServiceDelete(d *schema.ResourceData, meta interface{
return nil
}

func expandIap(configured []interface{}) *compute.BackendServiceIAP {
data := configured[0].(map[string]interface{})
iap := &compute.BackendServiceIAP{
Enabled: true,
Oauth2ClientId: data["oauth2_client_id"].(string),
Oauth2ClientSecret: data["oauth2_client_secret"].(string),
ForceSendFields: []string{"Enabled", "Oauth2ClientId", "Oauth2ClientSecret"},
}

return iap
}

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

just curious, why 1, 1?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

}

iapMap := map[string]interface{}{
"enabled": iap.Enabled,
"oauth2_client_id": iap.Oauth2ClientId,
"oauth2_client_secret": iap.Oauth2ClientSecretSha256,
}
return []map[string]interface{}{iapMap}
}

func expandBackends(configured []interface{}) ([]*compute.Backend, error) {
backends := make([]*compute.Backend, 0, len(configured))

Expand Down Expand Up @@ -323,7 +375,6 @@ func flattenBackends(backends []*compute.Backend) []map[string]interface{} {
data["max_rate"] = b.MaxRate
data["max_rate_per_instance"] = b.MaxRatePerInstance
data["max_utilization"] = b.MaxUtilization

result = append(result, data)
}

Expand All @@ -337,9 +388,23 @@ func expandBackendService(d *schema.ResourceData) (*compute.BackendService, erro
healthChecks = append(healthChecks, v.(string))
}

// The IAP service is enabled and disabled by adding or removing
// the IAP configuration block (and providing the client id
// and secret). We are force sending the three required API fields
// to enable/disable IAP at all times here, and relying on Golang's
// type defaults to enable or disable IAP in the existance or absense
// of the block, instead of checking if the block exists, zeroing out
// fields, etc.
service := &compute.BackendService{
Name: d.Get("name").(string),
HealthChecks: healthChecks,
Iap: &compute.BackendServiceIAP{
ForceSendFields: []string{"Enabled", "Oauth2ClientId", "Oauth2ClientSecret"},
},
}

if v, ok := d.GetOk("iap"); ok {
service.Iap = expandIap(v.([]interface{}))
}

var err error
Expand Down
161 changes: 161 additions & 0 deletions google/resource_compute_backend_service_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -120,6 +120,48 @@ func TestAccComputeBackendService_withBackendAndUpdate(t *testing.T) {
}
}

func TestAccComputeBackendService_withBackendAndIAP(t *testing.T) {
serviceName := fmt.Sprintf("tf-test-%s", acctest.RandString(10))
igName := fmt.Sprintf("tf-test-%s", acctest.RandString(10))
itName := fmt.Sprintf("tf-test-%s", acctest.RandString(10))
checkName := fmt.Sprintf("tf-test-%s", acctest.RandString(10))
var svc compute.BackendService
resource.Test(t, resource.TestCase{
PreCheck: func() { testAccPreCheck(t) },
Providers: testAccProviders,
CheckDestroy: testAccCheckComputeBackendServiceDestroy,
Steps: []resource.TestStep{
resource.TestStep{
Config: testAccComputeBackendService_withBackendAndIAP(
serviceName, igName, itName, checkName, 10),
Check: resource.ComposeTestCheckFunc(
testAccCheckComputeBackendServiceExistsWithIAP(
"google_compute_backend_service.lipsum", &svc),
),
},
resource.TestStep{
Config: testAccComputeBackendService_withBackend(
serviceName, igName, itName, checkName, 10),
Check: resource.ComposeTestCheckFunc(
testAccCheckComputeBackendServiceExistsWithoutIAP(
"google_compute_backend_service.lipsum", &svc),
),
},
},
})

if svc.TimeoutSec != 10 {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

t.Errorf("Expected TimeoutSec == 10, got %d", svc.TimeoutSec)
}
if svc.Protocol != "HTTP" {
t.Errorf("Expected Protocol to be HTTP, got %q", svc.Protocol)
}
if len(svc.Backends) != 1 {
t.Errorf("Expected 1 backend, got %d", len(svc.Backends))
}

}

func TestAccComputeBackendService_updatePreservesOptionalParameters(t *testing.T) {
t.Parallel()

Expand Down Expand Up @@ -287,6 +329,71 @@ func testAccCheckComputeBackendServiceExists(n string, svc *compute.BackendServi
}
}

func testAccCheckComputeBackendServiceExistsWithIAP(n string, svc *compute.BackendService) resource.TestCheckFunc {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I see your point. Will adjust.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

return func(s *terraform.State) error {
rs, ok := s.RootModule().Resources[n]
if !ok {
return fmt.Errorf("Not found: %s", n)
}

if rs.Primary.ID == "" {
return fmt.Errorf("No ID is set")
}

config := testAccProvider.Meta().(*Config)

found, err := config.clientCompute.BackendServices.Get(
config.Project, rs.Primary.ID).Do()
if err != nil {
return err
}

if found.Name != rs.Primary.ID {
return fmt.Errorf("Backend service %s not found", rs.Primary.ID)
}

if found.Iap == nil || found.Iap.Enabled == false {
return fmt.Errorf("IAP not found or not enabled.")
}

*svc = *found

return nil
}
}

func testAccCheckComputeBackendServiceExistsWithoutIAP(n string, svc *compute.BackendService) resource.TestCheckFunc {
return func(s *terraform.State) error {
rs, ok := s.RootModule().Resources[n]
if !ok {
return fmt.Errorf("Not found: %s", n)
}

if rs.Primary.ID == "" {
return fmt.Errorf("No ID is set")
}

config := testAccProvider.Meta().(*Config)

found, err := config.clientCompute.BackendServices.Get(
config.Project, rs.Primary.ID).Do()
if err != nil {
return err
}

if found.Name != rs.Primary.ID {
return fmt.Errorf("Backend service %s not found", rs.Primary.ID)
}

if found.Iap != nil && found.Iap.Enabled == true {
return fmt.Errorf("IAP enabled when it should be disabled")
}

*svc = *found

return nil
}
}
func TestAccComputeBackendService_withCDNEnabled(t *testing.T) {
t.Parallel()

Expand Down Expand Up @@ -460,6 +567,60 @@ resource "google_compute_http_health_check" "default" {
`, serviceName, timeout, igName, itName, checkName)
}

func testAccComputeBackendService_withBackendAndIAP(
serviceName, igName, itName, checkName string, timeout int64) string {
return fmt.Sprintf(`
resource "google_compute_backend_service" "lipsum" {
name = "%s"
description = "Hello World 1234"
port_name = "http"
protocol = "HTTP"
timeout_sec = %v

backend {
group = "${google_compute_instance_group_manager.foobar.instance_group}"
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done!

}

iap {
oauth2_client_id = "test"
oauth2_client_secret = "test"
}

health_checks = ["${google_compute_http_health_check.default.self_link}"]
}

resource "google_compute_instance_group_manager" "foobar" {
name = "%s"
instance_template = "${google_compute_instance_template.foobar.self_link}"
base_instance_name = "foobar"
zone = "us-central1-f"
target_size = 1
}

resource "google_compute_instance_template" "foobar" {
name = "%s"
machine_type = "n1-standard-1"

network_interface {
network = "default"
}

disk {
source_image = "debian-8-jessie-v20160803"
auto_delete = true
boot = true
}
}

resource "google_compute_http_health_check" "default" {
name = "%s"
request_path = "/"
check_interval_sec = 1
timeout_sec = 1
}
`, serviceName, timeout, igName, itName, checkName)
}

func testAccComputeBackendService_withSessionAffinity(serviceName, checkName, description, affinityName string) string {
return fmt.Sprintf(`
resource "google_compute_backend_service" "foobar" {
Expand Down