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

Vpc sc allow null for identity type #1632

Conversation

LudovicEmo
Copy link
Contributor

Hello,

modules/vpc-sc is already supporting null for ingress_policies.identity_type.
This PR adds support for null for egress_policies.identity_type.

Example
This is required for example to implement an egress rule for using Connected sheets with Bigquery for a selected list of identities (refer to [1]).

In the following example, egressFrom.identityType is absent (hence null), and the list of identities is managed through identities.

- egressTo:
    operations:
    - serviceName: 'bigquery.googleapis.com'
      methodSelectors:
      - permission: 'bigquery.vpcsc.importData'
    resources:
    - projects/628550087766 # Sheets-owned Google Cloud project
  egressFrom:
    identities:
    - serviceAccount: sa1@[REDACTED]
    - serviceAccount: sa2@[REDACTED]

As mentioned here [2], the egress_from.identity_type is optional, specifies the type of identities that are allowed access to outside the perimeter. If left unspecified, then members of identities field will be allowed access.

Changes

updated vpc_sc/variables.tf variables default value & validation
updated the vpc_sc test result for the null capability
tfdoc for vpc_sc
updated fmt for cloudsql-instance/variables.tf (missing a blank line after comment)

References
[1] https://cloud.google.com/bigquery/docs/connected-sheets#vpc-service-controls
[2] https://registry.terraform.io/providers/hashicorp/google/latest/docs/resources/access_context_manager_service_perimeter

juliocc
juliocc previously approved these changes Aug 28, 2023
Copy link
Collaborator

@juliocc juliocc left a comment

Choose a reason for hiding this comment

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

Thanks @LudovicEmo. I left a comment below.

Can you please run tools/tfdoc.py to update the readme?

modules/vpc-sc/variables.tf Show resolved Hide resolved
@juliocc
Copy link
Collaborator

juliocc commented Aug 28, 2023

@LudovicEmo can you provide a code example where the module fails today. The example you listed in (1) doesn't use null but ANY_USER_ACCOUNT

@juliocc juliocc self-requested a review August 28, 2023 15:03
@juliocc juliocc dismissed their stale review August 28, 2023 15:03

re-read docs

@LudovicEmo
Copy link
Contributor Author

@juliocc

Thanks @LudovicEmo. I left a comment below.

Can you please run tools/tfdoc.py to update the readme?

not sure what is needed here, already updated README with tfdoc in commit 65b32ea

@LudovicEmo
Copy link
Contributor Author

LudovicEmo commented Aug 28, 2023

@LudovicEmo can you provide a code example where the module fails today. The example you listed in (1) doesn't use null but ANY_USER_ACCOUNT

@juliocc

The example of the Google documentation uses identityType: ANY_USER_ACCOUNT (https://cloud.google.com/bigquery/docs/connected-sheets#vpc-service-controls).

However here we are not opening for any user account, we are controlling the list of identities.
In my initial description the example is opening for a list of identities only with:

  • the identities list only,
  • identityType in this case must be null or absent.
- egressTo:
    operations:
    - serviceName: 'bigquery.googleapis.com'
      methodSelectors:
      - permission: 'bigquery.vpcsc.importData'
    resources:
    - projects/628550087766 # Sheets-owned Google Cloud project
  egressFrom:
    identities:
    - serviceAccount: sa1@[REDACTED]
    - serviceAccount: sa2@[REDACTED]

@juliocc
Copy link
Collaborator

juliocc commented Aug 28, 2023

However here we are not opening for any user account, we are controlling the list of identities.

Understood. Thanks for the context, I think it makes sense.

Thanks @LudovicEmo. I left a comment below.
Can you please run tools/tfdoc.py to update the readme?

not sure what is needed here, already updated README with tfdoc in commit 65b32ea

Run it again because the linter is complaining.

@ludoo
Copy link
Collaborator

ludoo commented Aug 29, 2023

Thank you for being thorough and making our job easy! :)

@ludoo ludoo merged commit c558d9b into GoogleCloudPlatform:master Aug 29, 2023
9 checks passed
@ludoo ludoo mentioned this pull request Jan 15, 2024
4 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants