ESD-1648: Tighten validation and interactive-prompt gaps#535
Open
Phil-Browne wants to merge 17 commits into
Open
ESD-1648: Tighten validation and interactive-prompt gaps#535Phil-Browne wants to merge 17 commits into
Phil-Browne wants to merge 17 commits into
Conversation
Close six gaps where interactive prompts and validators diverged from the flag/JSON paths: - Validate vNIC VLAN input during interactive MVE ordering - Reject empty required credentials in VXC partner prompts (AWS, Azure, IBM) and stop silently defaulting an unparseable Azure peering VLAN to 0; validate the parsed VLAN range too - Count runes, not bytes, in port/MVE/AWS name-length validators - Validate MCR prefix-filter entries as CIDRs with address-family consistency checks - Reject a float value of exactly 2^63 in GetIntFromInterface - Allow removing a tag with an empty existing value during interactive tag editing, and handle a newly entered empty-value key deterministically
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #535 +/- ##
==========================================
+ Coverage 78.57% 79.02% +0.45%
==========================================
Files 192 193 +1
Lines 18522 18630 +108
==========================================
+ Hits 14553 14723 +170
+ Misses 2904 2849 -55
+ Partials 1065 1058 -7
🚀 New features to boost your workflow:
|
Codecov flagged three partial hits: ValidatePortRequest, ValidateLAGPortRequest, and ValidateBuyMVERequest never exercised the name-length-exceeded branch, only ValidatePortName and ValidateMVERequest did.
Contributor
There was a problem hiding this comment.
Pull request overview
Tightens input validation and aligns interactive prompt behavior with the flag/JSON and validator paths across multiple Megaport CLI resources (VXC partners, MVE ordering, MCR prefix filters, and tag editing), with additional test coverage for the new validation behavior.
Changes:
- Switched several name-length checks to count Unicode runes (not bytes) and added multibyte-focused tests.
- Added stricter interactive prompt validation for partner credentials/VLANs and improved tag-editing behavior when values are empty.
- Strengthened MCR prefix-filter entry validation (CIDR + address-family consistency) and tightened numeric conversion overflow handling.
Reviewed changes
Copilot reviewed 16 out of 16 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| internal/validation/vxc.go | Counts runes for AWS/IBM name-length validation. |
| internal/validation/vxc_test.go | Adds multibyte AWS connection-name boundary tests. |
| internal/validation/port.go | Counts runes for port-name validation. |
| internal/validation/port_test.go | Adds multibyte port-name boundary tests. |
| internal/validation/mve.go | Counts runes for MVE-name validation. |
| internal/validation/mve_test.go | Adds multibyte MVE-name boundary tests. |
| internal/validation/mcr.go | Validates prefix-filter entries as CIDRs and enforces address-family consistency. |
| internal/validation/mcr_test.go | Adds CIDR/address-family test cases for prefix-filter updates. |
| internal/validation/conversion.go | Tightens float64→int conversion overflow rejection. |
| internal/validation/conversion_test.go | Adds regression tests for the 2^63 float64 rounding edge case. |
| internal/utils/prompts.go | Makes tag removal work even when existing tag values are empty; adds clearer messaging. |
| internal/utils/prompts_test.go | Adds coverage for empty-value tag removal and empty-value new keys. |
| internal/commands/vxc/vxc_prompts_test.go | Adds tests ensuring partner prompts reject empty required credentials and invalid VLAN input. |
| internal/commands/vxc/vxc_prompts_partner.go | Enforces required partner credentials in interactive prompts; validates Azure peering VLAN parsing/range. |
| internal/commands/mve/mve_prompts.go | Validates interactive vNIC VLAN input with the shared VLAN validator. |
| internal/commands/mve/mve_prompts_test.go | Adds out-of-range VLAN test coverage for interactive vNIC prompting. |
Comparing against float64(math.MaxInt) directly wrongly rejects the true max int on platforms where MaxInt fits exactly in a float64 (it only rounds up to 2^63 when int is 64 bits). Add 1 to get the correct exclusive upper bound on either width, and make the boundary tests derive from math.MaxInt instead of a hardcoded 2^63 so they stay in range regardless of platform int width. Also update the ValidatePrefixFilterListRequest / ValidateUpdatePrefixFilterList doc comments to mention the CIDR and address-family checks they now perform.
ValidateUpdatePrefixFilterList dispatched into the shared entry validator without checking AddressFamily was present and valid, unlike the create path. An empty or invalid value would silently fall into the IPv6 branch.
…th native - Azure peering VLAN prompt now uses validation.ParseInt instead of a raw strconv.Atoi wrap, so a bad value gets the same friendly 'invalid VLAN' wording as other numeric prompts instead of a leaked Atoi error string. - The WASM interactive tag prompt still used the old value == "" && tags[key] != "" condition, so it refused to remove a tag with an empty current value and stayed silent on a key that didn't exist. Mirrored the native prompts.go fix: check key existence explicitly and report when there's nothing to remove.
ValidateAWSPartnerConfig allowed an empty ConnectionName and only checked max length when non-empty, while the interactive prompt path already treats connection name as required. Since JSON and flag-built configs both funnel through this shared validator, the gap let those paths bypass a requirement the prompt enforces. Update the fixtures across three test functions that constructed VXCPartnerConfigAWS without a connection name.
ValidateVXCPartnerConfig treated any *VXCPartnerConfigTransit as always valid, so a nil pointer or a wrong ConnectType would pass validation and only fail later against the API. Extracted a ValidateTransitPartnerConfig function that checks both, matching the nil and connect-type checks the other partner validators already do.
…ries ValidateAWSPartnerConfig and ValidateIBMPartnerConfig could panic on a typed-nil pointer reaching the dispatch switch, unlike their Azure, vRouter, and Transit siblings which already nil-check. Also guard validatePrefixFilterEntries against a nil entry in the list, which would otherwise panic instead of returning a validation error.
Comment on lines
146
to
148
| if entry.Action != "permit" && entry.Action != "deny" { | ||
| return NewValidationError("entry action", entry.Action, "must be permit or deny") | ||
| } |
Comment on lines
+27
to
+29
| {"float64 above int range rejected", float64(math.MaxInt) * 2, 0, false}, | ||
| {"float64 just above max int rejected", justAboveMaxInt, 0, false}, | ||
| {"float64 just below max int accepted", nearMaxInt, int(nearMaxInt), true}, |
Google and Oracle partner config validators lacked the nil-guard that AWS, IBM, Azure, vRouter, and Transit already have, so a nil pointer would panic instead of returning a validation error. The IBM interactive prompt also required a customer ASN unconditionally, even though the prompt text and validator both treat it as optional when the opposite end is an MCR.
The IBM partner config name is optional in the API (it defaults to "MEGAPORT" server-side), and the SDK and Terraform provider both treat it that way. The validator was requiring it, which broke JSON and flag inputs that omitted the name, and the interactive prompt required it too. Length and character-set checks still apply when a name is given.
- validate interactive MCR prefix filter list creation before returning - require pairing key / virtual circuit ID in Google and Oracle prompts - enforce MVE name length on update requests - validate prefix filter entry GE/LE bounds per address family and GE <= LE
This was referenced Jul 11, 2026
The GetIntFromInterface int-width bound fix and the interactive tag-editing empty-value fix are orthogonal to the prompt/validator parity work here, so they were pulled into their own PRs: - GetIntFromInterface overflow bound -> #538 - interactive tag-edit empty-value handling -> #539 Restores those six files to the main baseline; this PR now stays focused on partner-config and prefix-filter validation parity.
Collaborator
|
This makes the AWS connection name required on all input paths: |
The AWS partner config name is optional in the API (both AwsRequest and AwshcRequest schemas omit it from required fields and default it to "MEGAPORT" server-side), same as IBM. An earlier commit in this PR made it required in the shared validator and the interactive prompt, which blocked previously-valid AWS VXC orders via flags and JSON. Length check still applies when a name is given.
Contributor
Author
|
Confirmed against the OpenAPI spec: Fixed in a4f570a: |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Closes gaps where interactive prompts and validators diverged from the flag/JSON input paths.
Two orthogonal fixes originally in this PR were split out to keep the scope focused:
GetIntFromInterfaceint-width overflow bound -> fix(validation): correct int-width bound in GetIntFromInterface #538