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

Allow for IPV6_ONLY stackType configurations #12485

Merged
merged 5 commits into from
Jan 8, 2025

Conversation

karolgorc
Copy link
Contributor

@karolgorc karolgorc commented Dec 4, 2024

reopen of #12283
related to b/360733056

This got rolled back because it was failing TestAccComputeInstanceNetworkIntefaceWithSecurityPolicy. I don't have Cloud Armor on my private cloud environment so can i get a detailed test log to see why it fails and debug it? Preferably with TF_LOG=DEBUG while the test is running

  • Supports IPV6_ONLY stackType for instances and subnetworks
  • ipv6_access_config is now Computed because when providing an IPV6 subnet the access config will get autofilled from the API

Release Note Template for Downstream PRs (will be copied)

See Write release notes for guidance.

compute: added support for `IPV6_ONLY` stack_type to `google_compute_subnetwork`, `google_compute_instance`, `google_compute_instance_template` and `google_compute_region_instance_template`.

@karolgorc karolgorc changed the title allow for ipv6 only stacktype configurations Allow for IPV6_ONLY stackType configurations Dec 4, 2024
Copy link

github-actions bot commented Dec 4, 2024

Hello! I am a robot. Tests will require approval from a repository maintainer to run.

@zli82016, a repository maintainer, has been assigned to review your changes. If you have not received review feedback within 2 business days, please leave a comment on this PR asking them to take a look.

You can help make sure that review is quick by doing a self-review and by running impacted tests locally.

@github-actions github-actions bot requested a review from zli82016 December 4, 2024 09:47
@modular-magician modular-magician added awaiting-approval Pull requests that need reviewer's approval to run presubmit tests service/compute-instances service/compute-vpc and removed awaiting-approval Pull requests that need reviewer's approval to run presubmit tests labels Dec 4, 2024
@modular-magician
Copy link
Collaborator

Hi there, I'm the Modular magician. I've detected the following information about your changes:

Diff report

Your PR generated some diffs in downstreams - here they are.

google provider: Diff ( 8 files changed, 12 insertions(+), 9 deletions(-))
google-beta provider: Diff ( 8 files changed, 12 insertions(+), 9 deletions(-))

@modular-magician
Copy link
Collaborator

Tests analytics

Total tests: 1071
Passed tests: 997
Skipped tests: 73
Affected tests: 1

Click here to see the affected service packages
  • compute

Action taken

Found 1 affected test(s) by replaying old test recordings. Starting RECORDING based on the most recent commit. Click here to see the affected tests
  • TestAccComputeInstanceNetworkIntefaceWithSecurityPolicy

Get to know how VCR tests work

@modular-magician
Copy link
Collaborator

🔴 Tests failed during RECORDING mode:
TestAccComputeInstanceNetworkIntefaceWithSecurityPolicy [Error message] [Debug log]

🔴 Errors occurred during RECORDING mode. Please fix them to complete your PR.

View the build log or the debug log for each test

Copy link
Member

@zli82016 zli82016 left a comment

Choose a reason for hiding this comment

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

The test TestAccComputeInstanceNetworkIntefaceWithSecurityPolicy still failed.

@karolgorc
Copy link
Contributor Author

karolgorc commented Dec 6, 2024

I don't have Cloud Armor on my private cloud environment so can i get a detailed test log to see why it fails and debug it? Preferably with TF_LOG=DEBUG while the test is running

as it should, i need the debug log from the test to start fixing this. (the links provided by the bot don't work)

This one is crucial for the VM because closing the feature gap between the provider and API on NIC's is mostly made of IPV6 features, and we need this one to even begin working on the rest

@github-actions github-actions bot requested a review from zli82016 December 6, 2024 07:10
@zli82016
Copy link
Member

zli82016 commented Dec 6, 2024

/gcbrun

@modular-magician modular-magician added awaiting-approval Pull requests that need reviewer's approval to run presubmit tests and removed awaiting-approval Pull requests that need reviewer's approval to run presubmit tests labels Dec 6, 2024
@modular-magician
Copy link
Collaborator

Hi there, I'm the Modular magician. I've detected the following information about your changes:

Diff report

Your PR generated some diffs in downstreams - here they are.

google provider: Diff ( 8 files changed, 12 insertions(+), 9 deletions(-))
google-beta provider: Diff ( 8 files changed, 12 insertions(+), 9 deletions(-))

@modular-magician
Copy link
Collaborator

Tests analytics

Total tests: 1071
Passed tests: 997
Skipped tests: 73
Affected tests: 1

Click here to see the affected service packages
  • compute

Action taken

Found 1 affected test(s) by replaying old test recordings. Starting RECORDING based on the most recent commit. Click here to see the affected tests
  • TestAccComputeInstanceNetworkIntefaceWithSecurityPolicy

Get to know how VCR tests work

@modular-magician
Copy link
Collaborator

🔴 Tests failed during RECORDING mode:
TestAccComputeInstanceNetworkIntefaceWithSecurityPolicy [Error message] [Debug log]

🔴 Errors occurred during RECORDING mode. Please fix them to complete your PR.

View the build log or the debug log for each test

@zli82016
Copy link
Member

zli82016 commented Dec 6, 2024

https://paste.googleplex.com/4789365243641856#l=10517

This is the logs for the test TestAccComputeInstanceNetworkIntefaceWithSecurityPolicy/access_config_update_access_config. Please let me know if you can't open it.

@karolgorc
Copy link
Contributor Author

don't have access

@zli82016
Copy link
Member

zli82016 commented Dec 9, 2024

These are the logs.

@karolgorc
Copy link
Contributor Author

karolgorc commented Dec 10, 2024

I'm currently seeing that our schema is not aligned with the actual API schema here.

We have security_policy under network_interface where as in the API it looks like this.

image

This is most likely why the Computed: true causes problems. Especially with IPV4_IPV6 . We can only have one sec policy per NIC where in reality it's one sec policy per access config. This is probably a makeshift solution to support the IPV4_IPV6 and have them set the same values across all the access configs.

I'm guessing that we've hit some case where the IPV6 sec policy gets left off while updating and it's not covered by this logic.

This is most likely to get changed and is a breaking change because most of the feature gap between provider and API on NICs is IPV6 related

Copy link

@zli82016 This PR has been waiting for review for 3 weekdays. Please take a look! Use the label disable-review-reminders to disable these notifications.

@zli82016
Copy link
Member

zli82016 commented Dec 11, 2024

This is most likely why the Computed: true causes problems. Especially with IPV4_IPV6 . We can only have one sec policy per NIC where in reality it's one sec policy per access config. This is probably a makeshift solution to support the IPV4_IPV6 and have them set the same values across all the access configs.

I'm guessing that we've hit some case where the IPV6 sec policy gets left off while updating and it's not covered by this logic.

This is most likely to get changed and is a breaking change because most of the feature gap between provider and API on NICs is IPV6 related

Right, computed + optional on this field ipv6_access_config is the reason for the test failing.

In this test step the expected error Error setting security policy to the instance since at least one access config must exist is from the Terraform provider code and doesn't happen. The reason is that when ipv6_access_config is not specified in the configuration, with computed + optional, the previous value of this field in the Terraform state is used for API request. It means that ipv6_access_config exists and this error doesn't occur.

@zli82016
Copy link
Member

zli82016 commented Dec 11, 2024

Is making ipv6_access_config computed a requirement to support IPV6_ONLY stackType?

Based on the comment, it looks like ipv6_access_config is filled by API if the subnetwork given to the network_interface is a subnetwork of external IPv6 addresses. This case can happen in both IPV6_ONLY and IPV4_IPV6 stackTypes? Or it only happens for IPV6_ONLY stackType?

@karolgorc
Copy link
Contributor Author

Will test IPV4_IPV6. But for IPV6_ONLY not having this as computed and providing an IPV6 external subnetwork will cause permadiff for the user, as the access config will be set by the API. The entire block will initialize itself.

Copy link

@GoogleCloudPlatform/terraform-team @zli82016 This PR has been waiting for review for 1 week. Please take a look! Use the label disable-review-reminders to disable these notifications.

@zli82016
Copy link
Member

Will test IPV4_IPV6. But for IPV6_ONLY not having this as computed and providing an IPV6 external subnetwork will cause permadiff for the user, as the access config will be set by the API. The entire block will initialize itself.

@karolgorc , have you tested IPV4_IPV6?

@zli82016
Copy link
Member

Also, can you add a test for IPV6_ONLY stackType? Thanks.

@modular-magician modular-magician removed the awaiting-approval Pull requests that need reviewer's approval to run presubmit tests label Dec 26, 2024
@modular-magician
Copy link
Collaborator

Hi there, I'm the Modular magician. I've detected the following information about your changes:

Diff report

Your PR generated some diffs in downstreams - here they are.

google provider: Diff ( 10 files changed, 156 insertions(+), 9 deletions(-))
google-beta provider: Diff ( 10 files changed, 156 insertions(+), 9 deletions(-))
Open in Cloud Shell: Diff ( 8 files changed, 226 insertions(+))

@modular-magician
Copy link
Collaborator

Tests analytics

Total tests: 1080
Passed tests: 1003
Skipped tests: 73
Affected tests: 4

Click here to see the affected service packages
  • compute

Action taken

Found 4 affected test(s) by replaying old test recordings. Starting RECORDING based on the most recent commit. Click here to see the affected tests
  • TestAccComputeINstance_NicStackType_IPV6
  • TestAccComputeInstanceNetworkIntefaceWithSecurityPolicy
  • TestAccComputeSubnetwork_subnetworkIpv6OnlyExternalExample
  • TestAccComputeSubnetwork_subnetworkIpv6OnlyInternalExample

Get to know how VCR tests work

@modular-magician
Copy link
Collaborator

🟢 Tests passed during RECORDING mode:
TestAccComputeINstance_NicStackType_IPV6 [Debug log]

🟢 No issues found for passed tests after REPLAYING rerun.


🔴 Tests failed during RECORDING mode:
TestAccComputeInstanceNetworkIntefaceWithSecurityPolicy [Error message] [Debug log]
TestAccComputeSubnetwork_subnetworkIpv6OnlyExternalExample [Error message] [Debug log]
TestAccComputeSubnetwork_subnetworkIpv6OnlyInternalExample [Error message] [Debug log]

🔴 Errors occurred during RECORDING mode. Please fix them to complete your PR.

View the build log or the debug log for each test

@github-actions github-actions bot requested a review from zli82016 January 3, 2025 11:33
@modular-magician modular-magician added awaiting-approval Pull requests that need reviewer's approval to run presubmit tests and removed awaiting-approval Pull requests that need reviewer's approval to run presubmit tests labels Jan 3, 2025
@modular-magician
Copy link
Collaborator

Hi there, I'm the Modular magician. I've detected the following information about your changes:

Diff report

Your PR generated some diffs in downstreams - here they are.

google provider: Diff ( 10 files changed, 154 insertions(+), 9 deletions(-))
google-beta provider: Diff ( 10 files changed, 154 insertions(+), 9 deletions(-))
Open in Cloud Shell: Diff ( 8 files changed, 226 insertions(+))

@modular-magician
Copy link
Collaborator

Tests analytics

Total tests: 1080
Passed tests: 1004
Skipped tests: 73
Affected tests: 3

Click here to see the affected service packages
  • compute

Action taken

Found 3 affected test(s) by replaying old test recordings. Starting RECORDING based on the most recent commit. Click here to see the affected tests
  • TestAccComputeINstance_NicStackType_IPV6
  • TestAccComputeSubnetwork_subnetworkIpv6OnlyExternalExample
  • TestAccComputeSubnetwork_subnetworkIpv6OnlyInternalExample

Get to know how VCR tests work

@modular-magician
Copy link
Collaborator

🔴 Tests failed during RECORDING mode:
TestAccComputeINstance_NicStackType_IPV6 [Error message] [Debug log]
TestAccComputeSubnetwork_subnetworkIpv6OnlyExternalExample [Error message] [Debug log]
TestAccComputeSubnetwork_subnetworkIpv6OnlyInternalExample [Error message] [Debug log]

🔴 Errors occurred during RECORDING mode. Please fix them to complete your PR.

View the build log or the debug log for each test

Copy link
Member

@zli82016 zli82016 left a comment

Choose a reason for hiding this comment

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

Tests failed.

@karolgorc
Copy link
Contributor Author

These tests were made for computed to be true. Will update them and maybe specify this behaviour in the docs

@github-actions github-actions bot requested a review from zli82016 January 7, 2025 07:58
@modular-magician modular-magician added awaiting-approval Pull requests that need reviewer's approval to run presubmit tests and removed awaiting-approval Pull requests that need reviewer's approval to run presubmit tests labels Jan 7, 2025
@modular-magician
Copy link
Collaborator

Hi there, I'm the Modular magician. I've detected the following information about your changes:

Diff report

Your PR generated some diffs in downstreams - here they are.

google provider: Diff ( 10 files changed, 159 insertions(+), 9 deletions(-))
google-beta provider: Diff ( 10 files changed, 159 insertions(+), 9 deletions(-))
Open in Cloud Shell: Diff ( 8 files changed, 227 insertions(+))

@modular-magician
Copy link
Collaborator

Tests analytics

Total tests: 1080
Passed tests: 1004
Skipped tests: 73
Affected tests: 3

Click here to see the affected service packages
  • compute

Action taken

Found 3 affected test(s) by replaying old test recordings. Starting RECORDING based on the most recent commit. Click here to see the affected tests
  • TestAccComputeInstance_NicStackType_IPV6
  • TestAccComputeSubnetwork_subnetworkIpv6OnlyExternalExample
  • TestAccComputeSubnetwork_subnetworkIpv6OnlyInternalExample

Get to know how VCR tests work

@modular-magician
Copy link
Collaborator

🟢 Tests passed during RECORDING mode:
TestAccComputeInstance_NicStackType_IPV6 [Debug log]
TestAccComputeSubnetwork_subnetworkIpv6OnlyExternalExample [Debug log]
TestAccComputeSubnetwork_subnetworkIpv6OnlyInternalExample [Debug log]

🟢 No issues found for passed tests after REPLAYING rerun.


🟢 All tests passed!

View the build log or the debug log for each test

Copy link
Member

@zli82016 zli82016 left a comment

Choose a reason for hiding this comment

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

LGTM

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.

4 participants