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

Add Service Attachment support for Secure Web Proxy #11107

Merged

Conversation

Samir-Cit
Copy link
Contributor

@Samir-Cit Samir-Cit commented Jul 3, 2024

This PR adds the support for Secure Web Proxy (Gateway) on the Service Attachment resource.
This is provided on the field _ targetService_ which now will have support for the Forwarding Rule and the Secure Web Proxy (Gateway).

Note 1: Test scenario was needed to verify if it is working with SWP.

Note 2: Diff Suppress function was needed because the fields maybe be slightly different when using self_link and this will generate a change on the resource and break the test. For example:

OLD = "https://networkservices.googleapis.com/beta/projects/{PROJECT_ID}/locations/us-east1/gateways/{SWP_ID}"
NEW = "https://networkservices.googleapis.com/v1alpha1/projects/{PROJECT_ID}/locations/us-east1/gateways/{SWP_ID}"

Release Note Template for Downstream PRs (will be copied)

compute: changed `target_service` field on the `google_compute_service_attachment` resource to accept a `ForwardingRule` or `Gateway` URL.

Copy link

github-actions bot commented Jul 3, 2024

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

@roaks3, 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 roaks3 July 3, 2024 21:42
@modular-magician modular-magician added the awaiting-approval Pull requests that needs reviewer's approval to run presubmit tests label Jul 3, 2024
Copy link

github-actions bot commented Jul 8, 2024

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

Copy link

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

@Samir-Cit
Copy link
Contributor Author

Hello @roaks3
Did you had a chance to take a look on this PR?

@roaks3
Copy link
Contributor

roaks3 commented Jul 10, 2024

@Samir-Cit this is interesting, I'll have to take a closer look at it tomorrow. Generally, I'm surprise the ResourceRefs need to be dropped, and I'm wondering if we've run into similar situations elsewhere.

I'll kick off the tests now to see how they do, and in the meantime, would you mind posting any related issues or prior discussion?

@modular-magician modular-magician added service/private-service-connect-published-service and removed awaiting-approval Pull requests that needs reviewer's approval to run presubmit tests labels Jul 10, 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 ( 4 files changed, 139 insertions(+), 18 deletions(-))
google-beta provider: Diff ( 4 files changed, 139 insertions(+), 18 deletions(-))
terraform-google-conversion: Diff ( 1 file changed, 19 insertions(+), 5 deletions(-))

@modular-magician
Copy link
Collaborator

Tests analytics

Total tests: 967
Passed tests: 891
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
  • TestAccComputeInstanceNetworkIntefaceWithSecurityPolicy
  • TestAccComputeRegionNetworkEndpointGroup_regionNetworkEndpointGroupPscServiceAttachmentExample
  • TestAccComputeServiceAttachment_serviceAttachmentBasicExampleGateway

Get to know how VCR tests work

@modular-magician
Copy link
Collaborator

$\textcolor{green}{\textsf{Tests passed during RECORDING mode:}}$
TestAccComputeRegionNetworkEndpointGroup_regionNetworkEndpointGroupPscServiceAttachmentExample[Debug log]
TestAccComputeServiceAttachment_serviceAttachmentBasicExampleGateway[Debug log]

$\textcolor{green}{\textsf{No issues found for passed tests after REPLAYING rerun.}}$


$\textcolor{red}{\textsf{Tests failed during RECORDING mode:}}$
TestAccComputeInstanceNetworkIntefaceWithSecurityPolicy[Error message] [Debug log]

$\textcolor{red}{\textsf{Errors occurred during RECORDING mode. Please fix them to complete your PR.}}$

View the build log or the debug log for each test

@Samir-Cit
Copy link
Contributor Author

@Samir-Cit this is interesting, I'll have to take a closer look at it tomorrow. Generally, I'm surprise the ResourceRefs need to be dropped, and I'm wondering if we've run into similar situations elsewhere.

@roaks3 I checked for other situations like that and I was able to find one PR that does kind of the same change for Router.
You can check the PR #5162 for more details. In that case the change was to accept ForwardingRule and IP Address (string), so it's not the very same, but it's two diferent types of values. And that was the only way I found of doing that.

I'll kick off the tests now to see how they do, and in the meantime, would you mind posting any related issues or prior discussion?

Regarding the test, thanks for runnig it. I did other change today, but I tested the function and the test should pass again!
The only test that is not working is one that is breaking on other PRs as well...

I don't have a prior discussion on this task but I found this other PR with changes kind of the same that I did.

@modular-magician modular-magician added the awaiting-approval Pull requests that needs reviewer's approval to run presubmit tests label Jul 11, 2024
Copy link
Contributor

@roaks3 roaks3 left a comment

Choose a reason for hiding this comment

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

Thanks for the additional context, that helped a lot. It looks like this does come up occasionally, and is a valid reason for ejecting from the ResourceRef.

I had one minor suggestion, but we should be able to run the test one last time and then merge once they are done.

mmv1/products/compute/ServiceAttachment.yaml Outdated Show resolved Hide resolved
@github-actions github-actions bot requested a review from roaks3 July 12, 2024 16:31
@github-actions github-actions bot requested a review from roaks3 July 12, 2024 19:31
@Samir-Cit
Copy link
Contributor Author

Hello @roaks3
Regarding the go_ServiceAttachment.yaml file:
On the file there is a warning to not edit directly.
Should I remove the change I did on this file or leave it there?
(I just saw this today)

@roaks3
Copy link
Contributor

roaks3 commented Jul 15, 2024

Let's see how the tests do, but I think it is probably fine. I think the go_ variants are probably generated from the originals, but updating both in lockstep like you've done seems ideal.

@modular-magician modular-magician removed the awaiting-approval Pull requests that needs reviewer's approval to run presubmit tests label Jul 15, 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 ( 4 files changed, 120 insertions(+), 17 deletions(-))
google-beta provider: Diff ( 4 files changed, 120 insertions(+), 17 deletions(-))
terraform-google-conversion: Diff ( 1 file changed, 1 insertion(+), 5 deletions(-))

@modular-magician
Copy link
Collaborator

Tests analytics

Total tests: 968
Passed tests: 894
Skipped tests: 74
Affected tests: 0

Click here to see the affected service packages
  • compute

$\textcolor{green}{\textsf{All tests passed!}}$

View the build log

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.

3 participants