-
Notifications
You must be signed in to change notification settings - Fork 9.2k
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
Relocate Valid4ByteAsn function to common lib, add to CloudWAN Network Policy Doc Data Source #27305
Conversation
… Network Policy Doc DS
Community NoteVoting for Prioritization
For Submitters
|
…alization in validation 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.
Welcome @schuylr 👋
It looks like this is your first Pull Request submission to the Terraform AWS Provider! If you haven’t already done so please make sure you have checked out our CONTRIBUTOR guide and FAQ to make sure your contribution is adhering to best practice and has all the necessary elements in place for a successful approval.
Also take a look at our FAQ which details how we prioritize Pull Requests for inclusion.
Thanks again, and welcome to the community! 😃
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
Since this PR adds a plan-time validation for the user, it should have a changeling entry. Something like "Add plan-time validation for |
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 for the PR, @schuylr. Moving the valid4ByteASN
function looks good. I'm concerned that the change from an int
to string
for the asn
value in the JSON document might not be accepted by the API calls where it's used
I was double-checking this yesterday as I was adding the changelog comment. CloudWAN is indeed looking for an integer/number format in the JSON, so this type change is not accepted by AWS APIs. In discussion with @GlennChia yesterday, I think the best approach will be to modify the |
New tests:
|
internal/verify/validate.go
Outdated
@@ -20,6 +20,27 @@ var accountIDRegexp = regexp.MustCompile(`^(aws|aws-managed|\d{12})$`) | |||
var partitionRegexp = regexp.MustCompile(`^aws(-[a-z]+)*$`) | |||
var regionRegexp = regexp.MustCompile(`^[a-z]{2}(-[a-z]+)+-\d$`) | |||
|
|||
func Valid4ByteASN(v interface{}, k string) (ws []string, errors []error) { | |||
stringValue := strconv.Itoa(v.(int)) |
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.
@gdavison Can you see any concerns with the 32-bit BSD build on this?
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.
ASN numbers are 32-bit unsigned values, so this will fail for any values between 2147483647
and 4294967295
. Basically, for any resources using terraform-plugin-sdk
(and therefore a TypeInt
) we have to accept a TypeString
and parse it into an int64
. Technically, we could use a uint32
, but the AWS API takes an int64
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, we can't use an int
to contain uint32
values on a 32-bit platform. Instead we can use an int64
, since that's what the AWS API takes
internal/verify/validate.go
Outdated
@@ -20,6 +20,27 @@ var accountIDRegexp = regexp.MustCompile(`^(aws|aws-managed|\d{12})$`) | |||
var partitionRegexp = regexp.MustCompile(`^aws(-[a-z]+)*$`) | |||
var regionRegexp = regexp.MustCompile(`^[a-z]{2}(-[a-z]+)+-\d$`) | |||
|
|||
func Valid4ByteASN(v interface{}, k string) (ws []string, errors []error) { | |||
stringValue := strconv.Itoa(v.(int)) |
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.
ASN numbers are 32-bit unsigned values, so this will fail for any values between 2147483647
and 4294967295
. Basically, for any resources using terraform-plugin-sdk
(and therefore a TypeInt
) we have to accept a TypeString
and parse it into an int64
. Technically, we could use a uint32
, but the AWS API takes an int64
@@ -160,8 +160,9 @@ func DataSourceCoreNetworkPolicyDocument() *schema.Resource { | |||
ValidateFunc: verify.ValidRegionName, | |||
}, | |||
"asn": { | |||
Type: schema.TypeInt, | |||
Optional: true, | |||
Type: schema.TypeInt, |
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.
This should be changed to a schema.TypeString
, which will be handled internally as an int64
. We can take advantage of the Terraform parser which will allow some string values, such as numbers and boolean values to omit the quotes
…iguration.edge_locations.asn' -> 'TypeString'.
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 🚀.
% make testacc TESTARGS='-run=TestAccNetworkManagerCoreNetworkPolicyDocumentDataSource_' PKG=networkmanager ACCTEST_PARALLELISM=2
==> Checking that code complies with gofmt requirements...
TF_ACC=1 go test ./internal/service/networkmanager/... -v -count 1 -parallel 2 -run=TestAccNetworkManagerCoreNetworkPolicyDocumentDataSource_ -timeout 180m
=== RUN TestAccNetworkManagerCoreNetworkPolicyDocumentDataSource_basic
=== PAUSE TestAccNetworkManagerCoreNetworkPolicyDocumentDataSource_basic
=== CONT TestAccNetworkManagerCoreNetworkPolicyDocumentDataSource_basic
--- PASS: TestAccNetworkManagerCoreNetworkPolicyDocumentDataSource_basic (92.99s)
PASS
ok github.com/hashicorp/terraform-provider-aws/internal/service/networkmanager 99.341s
% make testacc TESTARGS='-run=TestAccSiteVPNCustomerGateway_4ByteASN\|TestAccTransitGateway_serial/ConnectPeer/BgpAsn' PKG=ec2 ACCTEST_PARALLELISM=2
==> Checking that code complies with gofmt requirements...
TF_ACC=1 go test ./internal/service/ec2/... -v -count 1 -parallel 2 -run=TestAccSiteVPNCustomerGateway_4ByteASN\|TestAccTransitGateway_serial/ConnectPeer/BgpAsn -timeout 180m
=== RUN TestAccTransitGateway_serial
=== RUN TestAccTransitGateway_serial/ConnectPeer
=== RUN TestAccTransitGateway_serial/ConnectPeer/BgpAsn
--- PASS: TestAccTransitGateway_serial (594.69s)
--- PASS: TestAccTransitGateway_serial/ConnectPeer (594.69s)
--- PASS: TestAccTransitGateway_serial/ConnectPeer/BgpAsn (594.68s)
=== RUN TestAccSiteVPNCustomerGateway_4ByteASN
=== PAUSE TestAccSiteVPNCustomerGateway_4ByteASN
=== CONT TestAccSiteVPNCustomerGateway_4ByteASN
--- PASS: TestAccSiteVPNCustomerGateway_4ByteASN (33.02s)
PASS
ok github.com/hashicorp/terraform-provider-aws/internal/service/ec2 632.081s
@schuylr Thanks for the contribution 🎉 👏. |
Thanks for your edits, learned a bunch!! |
This functionality has been released in v4.38.0 of the Terraform AWS Provider. Please see the Terraform documentation on provider versioning or reach out if you need any assistance upgrading. For further feature requests or bug reports with this functionality, please create a new GitHub issue following the template. Thank you! |
I'm going to lock this pull request because it has been closed for 30 days ⏳. This helps our maintainers find and focus on the active issues. |
Description
This PR restores the validation for a 4-byte ASN on CloudWAN (Network Manager) Core Network Policy Document data source under Edge Locations. The validation was removed due to build failures on 32-bit systems, but can be restored by re-using the
valid4ByteAsn
function from theec2
service provider as a common validation function.This PR also modifies the
ec2
service provider to use the relocated common validation function (tests below).When
terraform-plugin-framework
becomes GA, the function can be modified to leverageInt64
as a new schema type.No changelog entry created per these instructions
Relations
Closes #24902
References
https://github.com/hashicorp/terraform-provider-aws/pull/24890/files#r877631861
#14030
#1888
Output from Acceptance Testing