-
Notifications
You must be signed in to change notification settings - Fork 1.7k
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.
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 support for Cloud Armor Edge Policies #5794
Changes from all commits
d8de231
0c4c621
fd19a9a
3e4f72e
1070bea
f8576fc
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,18 @@ | ||
resource "google_compute_backend_bucket" "<%= ctx[:primary_resource_id] %>" { | ||
name = "<%= ctx[:vars]['backend_bucket_name'] %>" | ||
description = "Contains beautiful images" | ||
bucket_name = google_storage_bucket.<%= ctx[:primary_resource_id] %>.name | ||
enable_cdn = true | ||
edge_security_policy = google_compute_security_policy.policy.id | ||
} | ||
|
||
resource "google_storage_bucket" "<%= ctx[:primary_resource_id] %>" { | ||
name = "<%= ctx[:vars]['bucket_name'] %>" | ||
location = "EU" | ||
} | ||
|
||
resource "google_compute_security_policy" "policy" { | ||
name = "<%= ctx[:vars]['bucket_name'] %>" | ||
description = "basic security policy" | ||
type = "CLOUD_ARMOR_EDGE" | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,19 @@ | ||
// security_policy isn't set by Create / Update | ||
if o, n := d.GetChange("edge_security_policy"); o.(string) != n.(string) { | ||
pol, err := ParseSecurityPolicyFieldValue(n.(string), d, config) | ||
if err != nil { | ||
return errwrap.Wrapf("Error parsing Backend Service security policy: {{err}}", err) | ||
} | ||
|
||
spr := emptySecurityPolicyReference() | ||
spr.SecurityPolicy = pol.RelativeLink() | ||
op, err := config.NewComputeClient(userAgent).BackendBuckets.SetEdgeSecurityPolicy(project, obj["name"].(string), spr).Do() | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Can we do this through the HTTP client directly? It feels a little awkward to add a dependency on the client libraries to a mmv1-based resource There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This is already a pattern in another resource https://github.com/GoogleCloudPlatform/magic-modules/blob/main/mmv1/templates/terraform/post_create/compute_backend_service_security_policy.go.erb#L8-L9 There is where I adapted the code from |
||
if err != nil { | ||
return errwrap.Wrapf("Error setting Backend Service security policy: {{err}}", err) | ||
} | ||
// This uses the create timeout for simplicity, though technically this code appears in both create and update | ||
waitErr := computeOperationWaitTime(config, op, project, "Setting Backend Service Security Policy", userAgent, d.Timeout(schema.TimeoutCreate)) | ||
if waitErr != nil { | ||
return waitErr | ||
} | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This exists in the post_create, but what if the user goes from this field being unset on a bucket to setting it? Should the field be ForceNew or do we also need this in the Update call?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh this is used as post_update as well, makes sense
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can the update part of this be done by setting
update_url
on this specific field in the terraform.yaml? It allows calling a specific endpoint for a specific fieldThere was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this would complicate things further by having two separate pathways to do the same thing. This way unifies the codepaths. I would be against such a change.