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

Share settings for sole-tenant node groups. #7059

Merged
merged 2 commits into from
Jan 18, 2023
Merged

Share settings for sole-tenant node groups. #7059

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
3674b7e
Share settings for sole-tenant node groups.
Dec 22, 2022
a61354f
Fixes after review.
Jan 12, 2023
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
29 changes: 29 additions & 0 deletions mmv1/products/compute/api.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8902,6 +8902,35 @@ objects:
Maximum size of the node group. Set to a value less than or equal
to 100 and greater than or equal to min-nodes.
required: true
- !ruby/object:Api::Type::NestedObject
name: 'shareSettings'
description: |
Share settings for the node group.
properties:
- !ruby/object:Api::Type::Enum
name: 'shareType'
required: true
description: |
Node group sharing type.
values:
- :ORGANIZATION
- :SPECIFIC_PROJECTS
- :LOCAL
- !ruby/object:Api::Type::Map
name: 'projectMap'
description: |
A map of project id and project config. This is only valid when shareType's value is SPECIFIC_PROJECTS.
key_name: id
key_description: |
The project ID.
value_type: !ruby/object:Api::Type::NestedObject
name: projectConfig
properties:
- !ruby/object:Api::Type::String
name: 'projectId'
idyczko marked this conversation as resolved.
Show resolved Hide resolved
required: true
description: |
The project id/number should be the same as the key of this project config in the project map.
Copy link
Member

@c2thorn c2thorn Jan 13, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

This seems to be implemented correctly in terms of the API, but I cannot understand the design. The project_id is the only relevant information being passed here, why is it duplicated in both the key and value within a map? Am I missing something? Do you know if the API is setting up to have other values within the projectConfig nested object?

We rarely deviate from the API, but it seems like a frivolous user experience to have to set the project id twice. We could relatively simply implement custom code to take a list of project id's from the user and supply the API with the map in the correct format. From my perspective, this could prevent user error. More context would help me understand however

Copy link
Contributor Author

Choose a reason for hiding this comment

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

To be honest, I cannot defend this decision, but I don't think it's been designed while working on this particular feature. The semantics of sharing node groups across projects was derived from how reservations are shared (take a look at the shareSettings field of the Reservation resource). Is there anything we can do with that at this point?

Also, I've stumbled upon some custom code that seems to be responsible for shareSettings modifications when updating the Reservation resource:
mmv1/templates/terraform/update_encoder/reservation.go.erb
Could you let me know if that is something we need for the NodeGroup resource as well?

Copy link
Member

Choose a reason for hiding this comment

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

We don't need to add update_encoder for shareSettings in the resource NodeGroup. The resource NodeGroup has property input: true. That means that a new resource is created no matter what field has changed (from the code, node_template is an exception).

https://github.com/GoogleCloudPlatform/magic-modules/blob/main/mmv1/products/compute/api.yaml#L8794

Waiting for @c2thorn 's opinions.

Copy link
Member

Choose a reason for hiding this comment

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

@idyczko thank you for pointing out the shareSettings field in Reservation. Considering that we've already gone forward with it there, we may as well keep consistent here.

@zli82016 is correct that a custom update encoder should not be needed since the resource cannot be updated in place at all. If this is no longer true, we can modify this in a separate PR.

- !ruby/object:Api::Resource
name: 'NetworkPeeringRoutesConfig'
base_url: 'projects/{{project}}/global/networks/{{network}}'
Expand Down
12 changes: 12 additions & 0 deletions mmv1/products/compute/terraform.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1776,6 +1776,16 @@ overrides: !ruby/object:Overrides::ResourceOverrides
vars:
group_name: "soletenant-group"
template_name: "soletenant-tmpl"
- !ruby/object:Provider::Terraform::Examples
name: "node_group_share_settings"
primary_resource_id: "nodes"
vars:
group_name: "soletenant-group"
template_name: "soletenant-tmpl"
guest_project_id: "project-id"
guest_project_name: "project-name"
test_env_vars:
org_id: :ORG_ID
properties:
zone: !ruby/object:Overrides::Terraform::PropertyOverride
required: false
Expand All @@ -1796,6 +1806,8 @@ overrides: !ruby/object:Overrides::ResourceOverrides
default_from_api: true
autoscalingPolicy.maxNodes: !ruby/object:Overrides::Terraform::PropertyOverride
default_from_api: true
shareSettings: !ruby/object:Overrides::Terraform::PropertyOverride
default_from_api: true
custom_code: !ruby/object:Provider::Terraform::CustomCode
pre_create: templates/terraform/pre_create/compute_node_group_url_replace.go.erb
NodeTemplate: !ruby/object:Overrides::Terraform::ResourceOverride
Expand Down
29 changes: 29 additions & 0 deletions mmv1/templates/terraform/examples/node_group_share_settings.tf.erb
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,29 @@
resource "google_project" "guest_project" {
project_id = "<%= ctx[:vars]['guest_project_id'] %>"
name = "<%= ctx[:vars]['guest_project_name'] %>"
org_id = "<%= ctx[:test_env_vars]['org_id'] %>"
idyczko marked this conversation as resolved.
Show resolved Hide resolved
}

resource "google_compute_node_template" "soletenant-tmpl" {
name = "<%= ctx[:vars]['template_name'] %>"
region = "us-central1"
node_type = "n1-node-96-624"
}

resource "google_compute_node_group" "<%= ctx[:primary_resource_id] %>" {
name = "<%= ctx[:vars]['group_name'] %>"
zone = "us-central1-f"
description = "example google_compute_node_group for Terraform Google Provider"

size = 1
node_template = google_compute_node_template.soletenant-tmpl.id

share_settings {
share_type = "SPECIFIC_PROJECTS"
project_map {
id = google_project.guest_project.project_id
project_id = google_project.guest_project.project_id
}
}
}