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

Jump to bottom

Secure Web Proxy fields gatewaySecurityPolicy and certificateUrls supports updates. #10549

Merged
merged 8 commits into from
May 10, 2024
Merged

Secure Web Proxy fields gatewaySecurityPolicy and certificateUrls supports updates. #10549

Show file tree
Hide file tree
Changes from 5 commits
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
0ed869b
SWP fields not immutable
Samir-Cit Apr 24, 2024
6920f0f
pre_update with mandatory fields. test working
Samir-Cit Apr 26, 2024
551ec18
Merge remote-tracking branch 'origin' into fix/swp-fields
Samir-Cit Apr 26, 2024
ec3e1ec
Fix test
Samir-Cit Apr 26, 2024
0c462d3
Fix end of line
Samir-Cit Apr 29, 2024
7a42ea5
Merge remote-tracking branch 'origin/main' into fix/swp-fields
Samir-Cit May 6, 2024
00cef59
Change back beta to v1
Samir-Cit May 7, 2024
3be7686
Merge remote-tracking branch 'origin/main' into fix/swp-fields
Samir-Cit May 7, 2024
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
3 changes: 1 addition & 2 deletions mmv1/products/networkservices/Gateway.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -97,6 +97,7 @@ virtual_fields:
If there is no other gateway of type 'SECURE_WEB_GATEWAY' remaining for that region and network it will be deleted.
custom_code: !ruby/object:Provider::Terraform::CustomCode
post_delete: 'templates/terraform/post_delete/network_services_gateway.go.erb'
pre_update: 'templates/terraform/pre_update/network_services_gateway.erb'
constants: 'templates/terraform/constants/network_services_gateway.go.erb'
parameters:
- !ruby/object:Api::Type::String
Expand Down Expand Up @@ -192,14 +193,12 @@ properties:
Currently, this field is specific to gateways of type 'SECURE_WEB_GATEWAY'.
- !ruby/object:Api::Type::String
name: 'gatewaySecurityPolicy'
immutable: true
description: |
A fully-qualified GatewaySecurityPolicy URL reference. Defines how a server should apply security policy to inbound (VM to Proxy) initiated connections.
For example: `projects/*/locations/*/gatewaySecurityPolicies/swg-policy`.
This policy is specific to gateways of type 'SECURE_WEB_GATEWAY'.
- !ruby/object:Api::Type::Array
name: 'certificateUrls'
immutable: true
description: |
A fully-qualified Certificates URL reference. The proxy presents a Certificate (selected based on SNI) when establishing a TLS connection.
This feature only applies to gateways of type 'SECURE_WEB_GATEWAY'.
Expand Down
2 changes: 1 addition & 1 deletion mmv1/products/networkservices/product.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,7 +16,7 @@ name: NetworkServices
versions:
- !ruby/object:Api::Product::Version
name: beta
base_url: https://networkservices.googleapis.com/v1/
base_url: https://networkservices.googleapis.com/v1beta1/
Samir-Cit marked this conversation as resolved.
Show resolved Hide resolved
- !ruby/object:Api::Product::Version
name: ga
base_url: https://networkservices.googleapis.com/v1/
Expand Down
4 changes: 4 additions & 0 deletions mmv1/templates/terraform/pre_update/network_services_gateway.erb
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,4 @@
if d.Get("type") == "SECURE_WEB_GATEWAY" {
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 explain what this is doing and why it was added? From what I can tell, it was previously possible for a user to have a google_network_services_gateway with type = "SECURE_WEB_GATEWAY", and then change a field like server_tls_policy or description, which would trigger this condition, and presumably produce different results than before. Why do these 2 fields need to be set, and what happened before when they weren't set?

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 was just double checking with a terraform code using the actual provider and when trying to update the description of google_network_services_gateway with type = SECURE_WEB_GATEWAY it throw the following error:
Error: Error waiting for Updating Gateway: Error code 13, message: an internal error has occurred
The update send the name, the ID and the updated description fields only:

# google_network_services_gateway.foobar will be updated in-place
  ~ resource "google_network_services_gateway" "foobar" {
      ~ description                          = "my description" -> "new description"
        id                                   = "projects/{PROJECT}/locations/us-east1/gateways/{NAME}"
        name                                 = {NAME}
        # (18 unchanged attributes hidden)
    }

When updating through GCP console, the request sends all the fields that the Gateway contains:

gateway: {
  addresses: [1]
  certificate_urls: [1]
  description: "new description"
  gateway_security_policy: {POLICY}
  name: "projects/{PROJECT}/locations/us-east1/gateways/{NAME}"
  network: {NETWORK}
  ports: [1]
  subnetwork: {SUBNETWORK}
  type: "SECURE_WEB_GATEWAY"
}
update_mask: {
  paths: [
    0: "certificate_urls"
    1: "description"
    2: "gateway_security_policy"
]}

Also, GCP Console uses API v1beta1

To update using the API Explorer first I used all fields and then I was removing one by one to check which fields are required and those 2 (name and type) were required to update the Gateway through API Explorer

PATCH https://networkservices.googleapis.com/v1/projects/{PROJECT}/locations/us-east1/gateways/{NAME}?updateMask=description&key=[YOUR_API_KEY] HTTP/1.1

Authorization: Bearer [YOUR_ACCESS_TOKEN]
Accept: application/json
Content-Type: application/json

{
  "description": "new description",
  "name": "{NAME}",
  "type": "SECURE_WEB_GATEWAY"
}

Conclusion: Since GCP Console sends all fields it's able to update the gateway. So this modification that I did will send the required fields (name and type) to be able to update as well.

Copy link
Contributor

Choose a reason for hiding this comment

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

Hmm ok, that's interesting behavior, but seems to match your logic. I suppose it's safer to stick with the condition you've included, but I do wonder if it might be cleaner to always include name and type in the request.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

At first I did that... But if the type is other than SECURE_WEB_GATEWAY the request fail.
That broke some tests when I was starting the development.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@roaks3 regarding the fields description, it's still tracked on our radar but it was was deprioritized together with ngprabhu@

Copy link
Contributor

Choose a reason for hiding this comment

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

Awesome, thanks for those answers!

obj["name"] = d.Get("name")
obj["type"] = d.Get("type")
}
Loading