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

feat: Add new submodule for service networking #569

Closed

Conversation

q2w
Copy link
Contributor

@q2w q2w commented Aug 7, 2024

This PR makes below changes,

  1. Adds a new submodule for service-networking
  2. Adds new example and test for the submodule

@q2w q2w force-pushed the feature/service_networking branch from 582a8ca to 303862d Compare August 9, 2024 07:37
@q2w q2w force-pushed the feature/service_networking branch 3 times, most recently from 3eca387 to 7000a7c Compare August 9, 2024 18:25
@q2w q2w force-pushed the feature/service_networking branch from 7000a7c to b3d6203 Compare August 11, 2024 13:50
@q2w q2w requested a review from bharathkkb August 12, 2024 13:30
@q2w q2w marked this pull request as ready for review August 12, 2024 13:30
@q2w q2w requested review from imrannayer and a team as code owners August 12, 2024 13:30
@q2w q2w force-pushed the feature/service_networking branch 2 times, most recently from 675b9b8 to 7990fae Compare August 16, 2024 19:24
modules/service-networking/metadata.yaml Outdated Show resolved Hide resolved
modules/service-networking/variables.tf Outdated Show resolved Hide resolved
for_each = { for address in var.global_addresses : address.name => address }
project = var.project_id
name = each.value.name
purpose = each.value.purpose
Copy link
Member

Choose a reason for hiding this comment

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

IIUC for this private service access usecase isn't it always VPC_PEERING?

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 have used VPC_PEERING as default value. Should i remove the variable and use VPC_PEERING directly?

Copy link
Collaborator

Choose a reason for hiding this comment

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

@q2w In case of service networking it will always be vpc_peering.

modules/service-networking/main.tf Show resolved Hide resolved
@@ -0,0 +1,28 @@
# Terraform Google service networking

This module creates global network address and a service networking
Copy link
Member

Choose a reason for hiding this comment

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

Could we add some more context on the uses cases? IIUC this module would be used for private service access. Is there any other usecase cc @imrannayer

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Updated the module README and example README

@q2w q2w force-pushed the feature/service_networking branch from c3b896f to 015be39 Compare August 22, 2024 13:31
@imrannayer
Copy link
Collaborator

/gcbrun

@imrannayer
Copy link
Collaborator

@q2w @bharathkkb how are we going to handle this sub-module and current sub-module?

They both are same. Although Current module has a flaw that it accepts one IP address instead of list of ip addresses.

@q2w
Copy link
Contributor Author

q2w commented Sep 2, 2024

@q2w @bharathkkb how are we going to handle this sub-module and current sub-module?

They both are same. Although Current module has a flaw that it accepts one IP address instead of list of ip addresses.

@imrannayer Thanks for pointing to the current module! Before creating this PR i tried to check if some module exist which can create service networking. I missed finding out private_service_access sub-module. IMHO the current submodule can fulfill the requirements. In future we can update the current module to take multiple addresses. I would defer to @bharathkkb for final call on the current PR.

@bharathkkb
Copy link
Member

Good catch @imrannayer, yes let's close this and update current module as Imran pointed out. Sorry for the churn @q2w!

@bharathkkb bharathkkb closed this Sep 3, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants