-
Notifications
You must be signed in to change notification settings - Fork 1.5k
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
[#16400] Fix and simplify InternalIpDiffSuppress function #9423
[#16400] Fix and simplify InternalIpDiffSuppress function #9423
Conversation
Hello! I am a robot. It looks like you are a: @melinath, 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. |
cc @rileykarson FYI |
Hi there, I'm the Modular magician. I've detected the following information about your changes: Diff reportYour PR generated some diffs in downstreams - here they are. Terraform GA: Diff ( 2 files changed, 45 insertions(+), 13 deletions(-)) |
Tests analyticsTotal tests: Action takenFound 26 affected test(s) by replaying old test recordings. Starting RECORDING based on the most recent commit. Click here to see the affected testsTestAccComputeVpnTunnel_defaultTrafficSelectors|TestAccComputeVpnTunnel_vpnTunnelBetaExample|TestAccComputeVpnGateway_targetVpnGatewayBasicExample|TestAccComputeVpnTunnel_regionFromGateway|TestAccComputeVpnTunnel_vpnTunnelBasicExample|TestAccComputeRouterInterface_basic|TestAccComputeServiceAttachment_serviceAttachmentBasicExampleUpdate|TestAccComputeServiceAttachment_serviceAttachmentExplicitProjectsExample|TestAccComputeServiceAttachment_serviceAttachmentBasicExample|TestAccComputeBackendBucket_externalCdnLbWithBackendBucketExample|TestAccComputeGlobalForwardingRule_internalLoadBalancing|TestAccComputeGlobalForwardingRule_privateServiceConnectGoogleApisNoAutomateDnsExample|TestAccComputeGlobalForwardingRule_globalForwardingRuleInternalExample|TestAccComputeGlobalForwardingRule_externalHttpLbMigBackendCustomHeaderExample|TestAccComputeGlobalForwardingRule_privateServiceConnectGoogleApisExample|TestAccComputeGlobalForwardingRule_externalTcpProxyLbMigBackendExample|TestAccComputeForwardingRule_forwardingRuleIpAddressIpv6|TestAccComputeForwardingRule_forwardingRuleRegionalSteeringExampleUpdate|TestAccComputeForwardingRule_ip|TestAccComputeForwardingRule_forwardingRuleVpcPscExampleUpdate|TestAccComputeForwardingRule_forwardingRuleRegionalSteeringExample|TestAccComputeForwardingRule_forwardingRuleVpcPscNoAutomateDnsExample|TestAccComputeForwardingRule_forwardingRuleRegionalHttpXlbExample|TestAccComputeForwardingRule_forwardingRuleVpcPscExample|TestAccComputeForwardingRule_forwardingRulePscRecreate|TestAccLoggingProjectSink_updatePreservesCustomWriter |
Rerun these tests in REPLAYING mode to catch issues
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A couple questions below.
I'm also not sure that the tests for non-IP addresses are correct - I believe they should expect a format like projects/project_id/regions/region/addresses/address-name
rather than a resource reference (which would get expanded to some other value)
addr_netmask_new := strings.Split(new, "/") | ||
|
||
// Check if old or new are IPs (with or without netmask) | ||
addr_old := func() net.IP { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why are these being handled in closures? That's not a common pattern in this codebase.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done
} | ||
|
||
// If old and new have a netmask compare them, | ||
// otherwise always assume they are the same |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not super familiar with IPv6 - why is this a safe assumption to make? (A link to documentation explaining this would be great!)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Unfortunately, this is not safe and it's not documented anywhere but it's exactly what we're trying to solve with this patch. Please look at the bug we're referencing (#16400).
mmv1/third_party/terraform/tpgresource/common_diff_suppress_test.go
Outdated
Show resolved
Hide resolved
It looks like the unit tests are also currently failing - please make sure unit tests are passing locally before pushing a new commit. |
Curious thing is…they were passing locally!
I’ll look again into it.
Thanks Stephen.
…-Luca
Il giorno mar 7 nov 2023 alle 21:19 Stephen Lewis (Burrows) <
***@***.***> ha scritto:
It looks like the unit tests are also currently failing - please make sure
unit tests are passing locally before pushing a new commit.
—
Reply to this email directly, view it on GitHub
<#9423 (comment)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/AARY7UBMUXLLK2AYFUKPX4TYDKJU3AVCNFSM6AAAAAA7BI7MISVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMYTOOJZHEZDGNZSHE>
.
You are receiving this because you authored the thread.Message ID:
***@***.***>
|
Hi there, I'm the Modular magician. I've detected the following information about your changes: Diff reportYour PR generated some diffs in downstreams - here they are. Terraform GA: Diff ( 2 files changed, 101 insertions(+), 32 deletions(-)) |
Tests analyticsTotal tests: Action takenFound 13 affected test(s) by replaying old test recordings. Starting RECORDING based on the most recent commit. Click here to see the affected testsTestAccComputeServiceAttachment_serviceAttachmentBasicExampleUpdate|TestAccComputeServiceAttachment_serviceAttachmentExplicitProjectsExample|TestAccComputeServiceAttachment_serviceAttachmentBasicExample|TestAccComputeGlobalForwardingRule_privateServiceConnectGoogleApisNoAutomateDnsExample|TestAccComputeGlobalForwardingRule_privateServiceConnectGoogleApisExample|TestAccComputeForwardingRule_forwardingRuleRegionalSteeringExampleUpdate|TestAccComputeForwardingRule_forwardingRuleVpcPscExampleUpdate|TestAccComputeForwardingRule_ip|TestAccComputeForwardingRule_forwardingRuleVpcPscNoAutomateDnsExample|TestAccComputeForwardingRule_forwardingRuleVpcPscExample|TestAccComputeForwardingRule_forwardingRuleRegionalSteeringExample|TestAccComputeForwardingRule_forwardingRulePscRecreate|TestAccDataprocJobIamPolicy |
Rerun these tests in REPLAYING mode to catch issues
|
Hi there, I'm the Modular magician. I've detected the following information about your changes: Diff reportYour PR generated some diffs in downstreams - here they are. Terraform GA: Diff ( 2 files changed, 99 insertions(+), 35 deletions(-)) |
Tests analyticsTotal tests: Action takenFound 12 affected test(s) by replaying old test recordings. Starting RECORDING based on the most recent commit. Click here to see the affected testsTestAccComputeServiceAttachment_serviceAttachmentBasicExampleUpdate|TestAccComputeServiceAttachment_serviceAttachmentExplicitProjectsExample|TestAccComputeServiceAttachment_serviceAttachmentBasicExample|TestAccComputeGlobalForwardingRule_privateServiceConnectGoogleApisNoAutomateDnsExample|TestAccComputeGlobalForwardingRule_privateServiceConnectGoogleApisExample|TestAccComputeForwardingRule_forwardingRuleRegionalSteeringExampleUpdate|TestAccComputeForwardingRule_forwardingRuleVpcPscExampleUpdate|TestAccComputeForwardingRule_forwardingRulePscRecreate|TestAccComputeForwardingRule_forwardingRuleVpcPscExample|TestAccComputeForwardingRule_forwardingRuleRegionalSteeringExample|TestAccComputeForwardingRule_forwardingRuleVpcPscNoAutomateDnsExample|TestAccDataprocClusterIamPolicy |
Rerun these tests in REPLAYING mode to catch issues
|
@melinath I really don't understand why I see different results locally:
Any idea please? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@LucaPrete it looks like you're running TestOptionalPrefixSuppress
- but the test that's failing is TestInternalIpDiffSuppress
. Any chance that's the issue?
It looks like the failing VCR tests may be related to this change as well, but I'm not exactly sure how. The errors are like:
Error: Error creating ForwardingRule: googleapi: Error 400: Invalid value for field 'resource.IPAddress': ''. No IP address specified for Private Service Connect forwarding rule., invalid
Arg, sorry... not sure how I haven't noticed it. I'll fix the tests and get back, hopefully soon. |
Hi there, I'm the Modular magician. I've detected the following information about your changes: Diff reportYour PR generated some diffs in downstreams - here they are. Terraform GA: Diff ( 2 files changed, 94 insertions(+), 32 deletions(-)) |
Tests analyticsTotal tests: Action takenFound 8 affected test(s) by replaying old test recordings. Starting RECORDING based on the most recent commit. Click here to see the affected testsTestAccComputeForwardingRule_forwardingRuleIpAddressIpv6|TestAccComputeGlobalForwardingRule_externalHttpLbMigBackendCustomHeaderExample|TestAccComputeForwardingRule_forwardingRuleRegionalHttpXlbExample|TestAccComputeForwardingRule_ip|TestAccComputeGlobalForwardingRule_externalTcpProxyLbMigBackendExample|TestAccComputeBackendBucket_externalCdnLbWithBackendBucketExample|TestAccDataprocClusterIamPolicy|TestAccHealthcareDatasetIamPolicy |
Rerun these tests in REPLAYING mode to catch issues
|
@melinath replies to all your comments and fixed the code/tests |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM apart from a couple tweaks to comments
mmv1/third_party/terraform/tpgresource/common_diff_suppress.go.erb
Outdated
Show resolved
Hide resolved
mmv1/third_party/terraform/tpgresource/common_diff_suppress.go.erb
Outdated
Show resolved
Hide resolved
d75d147
to
8a69de3
Compare
Hi there, I'm the Modular magician. I've detected the following information about your changes: Diff reportYour PR generated some diffs in downstreams - here they are. Terraform GA: Diff ( 2 files changed, 94 insertions(+), 32 deletions(-)) |
Tests analyticsTotal tests:
|
mmv1/third_party/terraform/tpgresource/common_diff_suppress.go.erb
Outdated
Show resolved
Hide resolved
@melinath done! Thanks for the review |
@LucaPrete could you make sure the changes are pushed? |
Now I did. Sorry again. |
Hi there, I'm the Modular magician. I've detected the following information about your changes: Diff reportYour PR generated some diffs in downstreams - here they are. Terraform GA: Diff ( 2 files changed, 95 insertions(+), 32 deletions(-)) |
Tests analyticsTotal tests: Action takenFound 3 affected test(s) by replaying old test recordings. Starting RECORDING based on the most recent commit. Click here to see the affected testsTestAccDataprocJobIamPolicy|TestAccDataprocClusterIamPolicy|TestAccDataSourceGoogleServiceAccountJwt |
Rerun these tests in REPLAYING mode to catch issues
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks! LGTM
…dPlatform#9423) * [#16400] Fix and simplify InternalIpDiffSuppress function * Fixes as per PR comments * Adding new tests for different netmasks * fix test * Fixes to unit tests * Fixes per comments * Comments fixes --------- Co-authored-by: Luca Prete <lucaprete@google.com>
…dPlatform#9423) * [#16400] Fix and simplify InternalIpDiffSuppress function * Fixes as per PR comments * Adding new tests for different netmasks * fix test * Fixes to unit tests * Fixes per comments * Comments fixes --------- Co-authored-by: Luca Prete <lucaprete@google.com>
…dPlatform#9423) * [#16400] Fix and simplify InternalIpDiffSuppress function * Fixes as per PR comments * Adding new tests for different netmasks * fix test * Fixes to unit tests * Fixes per comments * Comments fixes --------- Co-authored-by: Luca Prete <lucaprete@google.com>
…dPlatform#9423) * [#16400] Fix and simplify InternalIpDiffSuppress function * Fixes as per PR comments * Adding new tests for different netmasks * fix test * Fixes to unit tests * Fixes per comments * Comments fixes --------- Co-authored-by: Luca Prete <lucaprete@google.com>
As per #16400 the PR
fixes the InternalIpDiffSuppress function, which currently generates a drift for net masked IPv6 addresses and non net-masked IPv6 addresses (i.e.
fd20:6c7:bb6a:f400:0:0:0:0/96
-->fd20:6c7:bb6a:f400::
)simplifies the InternalIpDiffSuppress function