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.

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

Finish converting ACM resources to use policy mutex lock #12735

Merged
merged 5 commits into from
Jan 14, 2025
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
1 change: 1 addition & 0 deletions mmv1/products/accesscontextmanager/AccessLevel.yaml
Original file line number Diff line number Diff line change
Expand Up @@ -39,6 +39,7 @@ self_link: '{{name}}'
create_url: '{{parent}}/accessLevels'
update_verb: 'PATCH'
update_mask: true
mutex: '{{parent}}'
import_format:
- '{{name}}'
timeouts:
Expand Down
9 changes: 8 additions & 1 deletion mmv1/products/accesscontextmanager/AccessLevelCondition.yaml
Original file line number Diff line number Diff line change
Expand Up @@ -47,7 +47,7 @@ create_verb: 'PATCH'
update_mask: true
delete_verb: 'PATCH'
immutable: true
mutex: '{{access_level}}'
mutex: '{{access_policy_id}}'
import_format:
- '{{access_level}}'
# no unique way to specify
Expand Down Expand Up @@ -78,6 +78,7 @@ nested_query:
is_list_of_ids: false
modify_by_patch: true
custom_code:
encoder: 'templates/terraform/encoders/access_context_manager_access_level_condition.go.tmpl'
exclude_tgc: true
# Skipping the sweeper due to the non-standard base_url and because this is fine-grained under AccessLevel
exclude_sweeper: true
Expand Down Expand Up @@ -248,3 +249,9 @@ properties:
description: 'CIDR block IP subnetwork specification. Must be IPv4.'
item_type:
type: String
- name: 'accessPolicyId'
type: String
description: |
The name of the Access Policy this resource belongs to.
ignore_read: true
Copy link
Contributor

Choose a reason for hiding this comment

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

Why is this field immutable and output true at the same time?

Can the user set the policyId from their TF config?

This comment also applies for all the cases below.

Copy link
Member Author

@coder-221 coder-221 Jan 13, 2025

Choose a reason for hiding this comment

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

Output because the field should never be set by the user. It's a substring automatically parsed from the perimeter parameter they set (varies slightly across different resources).

Ie users are setting a perimeter parameter to accessPolicies/123/servicePerimeters/abc and we now want the mutex to be the substring accessPolicies/123. I want this to be an invisible change for users, so instead of having them input another param we can just parse it from that.

Immutable since the accessPolicy can never change once it's set.

If there's an easier way to have a mutex lock on a substring of a parameter, please let me know. I was kind of hoping we'd establish the pattern in #12725 and then this PR was just copying that to all other resources, but if we need changes that's fine.

Copy link
Contributor

Choose a reason for hiding this comment

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

Thanks for the explanation.
If immutable is set to true, if there's any modification to this field, another new resource will be recreated. Is that something you'd expect?

Copy link
Member Author

@coder-221 coder-221 Jan 13, 2025

Choose a reason for hiding this comment

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

Ah I think I see what you're saying. I wouldn't want that to trigger a recreate. Really I want the field to be ignored except where I'm reading it in the pre_create. I've removed the immutable property from all of them but kept output since users should not input data themselves into the property. Thanks for the help!

output: true
1 change: 1 addition & 0 deletions mmv1/products/accesscontextmanager/AccessLevels.yaml
Original file line number Diff line number Diff line change
Expand Up @@ -37,6 +37,7 @@ update_url: '{{parent}}/accessLevels:replaceAll'
update_verb: 'POST'
import_format:
- '{{parent}}/accessLevels'
mutex: '{{parent}}'
timeouts:
insert_minutes: 20
update_minutes: 20
Expand Down
1 change: 1 addition & 0 deletions mmv1/products/accesscontextmanager/AccessPolicy.yaml
Original file line number Diff line number Diff line change
Expand Up @@ -37,6 +37,7 @@ update_verb: 'PATCH'
update_mask: true
import_format:
- '{{name}}'
mutex: 'accessPolicies/{{name}}'
timeouts:
insert_minutes: 20
update_minutes: 20
Expand Down
1 change: 1 addition & 0 deletions mmv1/products/accesscontextmanager/AuthorizedOrgsDesc.yaml
Original file line number Diff line number Diff line change
Expand Up @@ -37,6 +37,7 @@ create_url: '{{parent}}/authorizedOrgsDescs'
update_verb: 'PATCH'
import_format:
- '{{name}}'
mutex: '{{parent}}'
timeouts:
insert_minutes: 20
update_minutes: 20
Expand Down
8 changes: 8 additions & 0 deletions mmv1/products/accesscontextmanager/EgressPolicy.yaml
Original file line number Diff line number Diff line change
Expand Up @@ -28,6 +28,7 @@ create_verb: 'PATCH'
update_mask: true
delete_verb: 'PATCH'
immutable: true
mutex: '{{access_policy_id}}'
import_format:
- '{{egress_policy_name}}/{{resource}}'
timeouts:
Expand All @@ -51,6 +52,7 @@ nested_query:
is_list_of_ids: true
modify_by_patch: true
custom_code:
encoder: 'templates/terraform/encoders/access_context_manager_egress_policy.go.tmpl'
custom_import: 'templates/terraform/custom_import/access_context_manager_service_perimeter_egress_policy.go.tmpl'
exclude_tgc: true
# Skipping the sweeper due to the non-standard base_url and because this is fine-grained under ServicePerimeter/IngressPolicy
Expand All @@ -72,3 +74,9 @@ properties:
A GCP resource that is inside of the service perimeter.
required: true
immutable: true
- name: 'accessPolicyId'
type: String
description: |
The name of the Access Policy this resource belongs to.
ignore_read: true
output: true
8 changes: 8 additions & 0 deletions mmv1/products/accesscontextmanager/IngressPolicy.yaml
Original file line number Diff line number Diff line change
Expand Up @@ -28,6 +28,7 @@ create_verb: 'PATCH'
update_mask: true
delete_verb: 'PATCH'
immutable: true
mutex: '{{access_policy_id}}'
import_format:
- '{{ingress_policy_name}}/{{resource}}'
timeouts:
Expand All @@ -51,6 +52,7 @@ nested_query:
is_list_of_ids: true
modify_by_patch: true
custom_code:
encoder: 'templates/terraform/encoders/access_context_manager_ingress_policy.go.tmpl'
custom_import: 'templates/terraform/custom_import/access_context_manager_service_perimeter_ingress_policy.go.tmpl'
exclude_tgc: true
# Skipping the sweeper due to the non-standard base_url and because this is fine-grained under ServicePerimeter/IngressPolicy
Expand All @@ -72,3 +74,9 @@ properties:
A GCP resource that is inside of the service perimeter.
required: true
immutable: true
- name: 'accessPolicyId'
type: String
description: |
The name of the Access Policy this resource belongs to.
ignore_read: true
output: true
2 changes: 1 addition & 1 deletion mmv1/products/accesscontextmanager/ServicePerimeter.yaml
Original file line number Diff line number Diff line change
Expand Up @@ -47,7 +47,7 @@ self_link: '{{name}}'
create_url: '{{parent}}/servicePerimeters'
update_verb: 'PATCH'
update_mask: true
mutex: '{{name}}'
mutex: '{{parent}}'
import_format:
- '{{name}}'
timeouts:
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -43,7 +43,7 @@ create_verb: 'PATCH'
update_mask: true
delete_verb: 'PATCH'
immutable: true
mutex: '{{perimeter}}'
mutex: '{{access_policy_id}}'
import_format:
- '{{perimeter}}'
exclude_import: true
Expand All @@ -70,6 +70,7 @@ nested_query:
modify_by_patch: true
custom_code:
constants: 'templates/terraform/constants/access_context_manager.go.tmpl'
encoder: 'templates/terraform/encoders/access_context_manager_service_perimeter_dry_run_egress_policy.go.tmpl'
pre_create: 'templates/terraform/pre_create/access_context_manager_dry_run_resource.go.tmpl'
pre_update: 'templates/terraform/pre_create/access_context_manager_dry_run_resource.go.tmpl'
pre_delete: 'templates/terraform/pre_create/access_context_manager_dry_run_resource.go.tmpl'
Expand Down Expand Up @@ -194,3 +195,10 @@ properties:
description: |
Value for permission should be a valid Cloud IAM permission for the
corresponding `serviceName` in `ApiOperation`.
immutable: true
- name: 'accessPolicyId'
type: String
description: |
The name of the Access Policy this resource belongs to.
ignore_read: true
output: true
Original file line number Diff line number Diff line change
Expand Up @@ -44,7 +44,7 @@ create_verb: 'PATCH'
update_mask: true
delete_verb: 'PATCH'
immutable: true
mutex: '{{perimeter}}'
mutex: '{{access_policy_id}}'
import_format:
- '{{perimeter}}'
exclude_import: true
Expand All @@ -71,6 +71,7 @@ nested_query:
modify_by_patch: true
custom_code:
constants: 'templates/terraform/constants/access_context_manager.go.tmpl'
encoder: 'templates/terraform/encoders/access_context_manager_service_perimeter_dry_run_egress_policy.go.tmpl'
pre_create: 'templates/terraform/pre_create/access_context_manager_dry_run_resource.go.tmpl'
pre_update: 'templates/terraform/pre_create/access_context_manager_dry_run_resource.go.tmpl'
pre_delete: 'templates/terraform/pre_create/access_context_manager_dry_run_resource.go.tmpl'
Expand Down Expand Up @@ -203,3 +204,9 @@ properties:
description: |
Value for permission should be a valid Cloud IAM permission for the
corresponding `serviceName` in `ApiOperation`.
- name: 'accessPolicyId'
type: String
description: |
The name of the Access Policy this resource belongs to.
ignore_read: true
output: true
Original file line number Diff line number Diff line change
Expand Up @@ -43,7 +43,7 @@ create_verb: 'PATCH'
update_mask: true
delete_verb: 'PATCH'
immutable: true
mutex: '{{perimeter_name}}'
mutex: '{{access_policy_id}}'
import_format:
- '{{perimeter_name}}/{{resource}}'
timeouts:
Expand All @@ -67,6 +67,7 @@ nested_query:
is_list_of_ids: true
modify_by_patch: true
custom_code:
encoder: 'templates/terraform/encoders/access_context_manager_service_perimeter_dry_run_resource.go.tmpl'
pre_create: 'templates/terraform/pre_create/access_context_manager_dry_run_resource.go.tmpl'
pre_update: 'templates/terraform/pre_create/access_context_manager_dry_run_resource.go.tmpl'
pre_delete: 'templates/terraform/pre_create/access_context_manager_dry_run_resource.go.tmpl'
Expand Down Expand Up @@ -99,3 +100,9 @@ properties:
Format: projects/{project_number}
required: true
immutable: true
- name: 'accessPolicyId'
type: String
description: |
The name of the Access Policy this resource belongs to.
ignore_read: true
output: true
Original file line number Diff line number Diff line change
Expand Up @@ -43,7 +43,7 @@ create_verb: 'PATCH'
update_mask: true
delete_verb: 'PATCH'
immutable: true
mutex: '{{perimeter}}'
mutex: '{{access_policy_id}}'
import_format:
- '{{perimeter}}'
exclude_import: true
Expand Down Expand Up @@ -71,6 +71,7 @@ nested_query:
custom_code:
constants: 'templates/terraform/constants/access_context_manager.go.tmpl'
custom_import: 'templates/terraform/custom_import/access_context_manager_service_perimeter_egress_policy.go.tmpl'
encoder: 'templates/terraform/encoders/access_context_manager_service_perimeter_egress_policy.go.tmpl'
exclude_tgc: true
# Skipping the sweeper due to the non-standard base_url and because this is fine-grained under ServicePerimeter
exclude_sweeper: true
Expand Down Expand Up @@ -192,3 +193,9 @@ properties:
description: |
Value for permission should be a valid Cloud IAM permission for the
corresponding `serviceName` in `ApiOperation`.
- name: 'accessPolicyId'
type: String
description: |
The name of the Access Policy this resource belongs to.
ignore_read: true
output: true
Original file line number Diff line number Diff line change
Expand Up @@ -44,7 +44,7 @@ create_verb: 'PATCH'
update_mask: true
delete_verb: 'PATCH'
immutable: true
mutex: '{{perimeter}}'
mutex: '{{access_policy_id}}'
import_format:
- '{{perimeter}}'
exclude_import: true
Expand Down Expand Up @@ -72,6 +72,7 @@ nested_query:
custom_code:
constants: 'templates/terraform/constants/access_context_manager.go.tmpl'
custom_import: 'templates/terraform/custom_import/access_context_manager_service_perimeter_ingress_policy.go.tmpl'
encoder: 'templates/terraform/encoders/access_context_manager_service_perimeter_ingress_policy.go.tmpl'
exclude_tgc: true
# Skipping the sweeper due to the non-standard base_url and because this is fine-grained under ServicePerimeter
exclude_sweeper: true
Expand Down Expand Up @@ -203,3 +204,9 @@ properties:
description: |
Value for permission should be a valid Cloud IAM permission for the
corresponding `serviceName` in `ApiOperation`.
- name: 'accessPolicyId'
type: String
description: |
The name of the Access Policy this resource belongs to.
ignore_read: true
output: true
Original file line number Diff line number Diff line change
Expand Up @@ -103,5 +103,4 @@ properties:
description: |
The name of the Access Policy this resource belongs to.
ignore_read: true
immutable: true
output: true
1 change: 1 addition & 0 deletions mmv1/products/accesscontextmanager/ServicePerimeters.yaml
Original file line number Diff line number Diff line change
Expand Up @@ -30,6 +30,7 @@ base_url: '{{parent}}/servicePerimeters:replaceAll'
self_link: '{{parent}}/servicePerimeters'
update_url: '{{parent}}/servicePerimeters:replaceAll'
update_verb: 'POST'
mutex: '{{parent}}'
import_format:
- '{{parent}}/servicePerimeters'
timeouts:
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,9 @@
return nil, err
}

if err := d.Set("access_policy_id", fmt.Sprintf("accessPolicies/%s", parts["accessPolicy"])); err != nil {
return nil, fmt.Errorf("Error setting access_policy_id: %s", err)
}
if err := d.Set("perimeter", fmt.Sprintf("accessPolicies/%s/servicePerimeters/%s", parts["accessPolicy"], parts["perimeter"])); err != nil {
return nil, fmt.Errorf("Error setting perimeter: %s", err)
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,9 @@
return nil, err
}

if err := d.Set("access_policy_id", fmt.Sprintf("accessPolicies/%s", parts["accessPolicy"])); err != nil {
return nil, fmt.Errorf("Error setting access_policy_id: %s", err)
}
if err := d.Set("perimeter", fmt.Sprintf("accessPolicies/%s/servicePerimeters/%s", parts["accessPolicy"], parts["perimeter"])); err != nil {
return nil, fmt.Errorf("Error setting perimeter: %s", err)
}
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,8 @@
// Set the access_policy_id field from part of the access_level parameter.

// The is logic is inside the encoder since the access_policy_id field is part of
// the mutex lock and encoders run before the lock is set.
parts := strings.Split(d.Get("access_level").(string), "/")
d.Set("access_policy_id", fmt.Sprintf("accessPolicies/%s", parts[1]))

return obj, nil
Original file line number Diff line number Diff line change
@@ -0,0 +1,8 @@
// Set the access_policy_id field from part of the egress_policy_name parameter.

// The is logic is inside the encoder since the access_policy_id field is part of
// the mutex lock and encoders run before the lock is set.
parts := strings.Split(d.Get("egress_policy_name").(string), "/")
d.Set("access_policy_id", fmt.Sprintf("accessPolicies/%s", parts[1]))

return obj, nil
Original file line number Diff line number Diff line change
@@ -0,0 +1,8 @@
// Set the access_policy_id field from part of the ingress_policy_name parameter.

// The is logic is inside the encoder since the access_policy_id field is part of
// the mutex lock and encoders run before the lock is set.
parts := strings.Split(d.Get("ingress_policy_name").(string), "/")
d.Set("access_policy_id", fmt.Sprintf("accessPolicies/%s", parts[1]))

return obj, nil
Original file line number Diff line number Diff line change
@@ -0,0 +1,8 @@
// Set the access_policy_id field from part of the perimeter parameter.

// The is logic is inside the encoder since the access_policy_id field is part of
// the mutex lock and encoders run before the lock is set.
parts := strings.Split(d.Get("perimeter").(string), "/")
d.Set("access_policy_id", fmt.Sprintf("accessPolicies/%s", parts[1]))

return obj, nil
Original file line number Diff line number Diff line change
@@ -0,0 +1,8 @@
// Set the access_policy_id field from part of the perimeter parameter.

// The is logic is inside the encoder since the access_policy_id field is part of
// the mutex lock and encoders run before the lock is set.
parts := strings.Split(d.Get("perimeter").(string), "/")
d.Set("access_policy_id", fmt.Sprintf("accessPolicies/%s", parts[1]))

return obj, nil
Original file line number Diff line number Diff line change
@@ -0,0 +1,8 @@
// Set the access_policy_id field from part of the perimeter_name parameter.

// The is logic is inside the encoder since the access_policy_id field is part of
// the mutex lock and encoders run before the lock is set.
parts := strings.Split(d.Get("perimeter_name").(string), "/")
d.Set("access_policy_id", fmt.Sprintf("accessPolicies/%s", parts[1]))

return obj, nil
Original file line number Diff line number Diff line change
@@ -0,0 +1,8 @@
// Set the access_policy_id field from part of the perimeter parameter.

// The is logic is inside the encoder since the access_policy_id field is part of
// the mutex lock and encoders run before the lock is set.
parts := strings.Split(d.Get("perimeter").(string), "/")
d.Set("access_policy_id", fmt.Sprintf("accessPolicies/%s", parts[1]))

return obj, nil
Original file line number Diff line number Diff line change
@@ -0,0 +1,8 @@
// Set the access_policy_id field from part of the perimeter parameter.

// The is logic is inside the encoder since the access_policy_id field is part of
// the mutex lock and encoders run before the lock is set.
parts := strings.Split(d.Get("perimeter").(string), "/")
d.Set("access_policy_id", fmt.Sprintf("accessPolicies/%s", parts[1]))

return obj, nil
Loading