improvement(offering): make network mode mandatory when creating vpc …#13622
improvement(offering): make network mode mandatory when creating vpc …#13622Chinmay048 wants to merge 3 commits into
Conversation
…and network offerings (apache#13607)
There was a problem hiding this comment.
Pull request overview
This PR aims to eliminate ambiguous VPC/Network Offering configurations by making networkmode (ROUTED vs NATTED) mandatory at both the UI form-validation layer and the Java API parameter layer.
Changes:
- Added Ant Design Vue form rules to require
networkmodeselection in the VPC Offering and Network Offering creation UIs. - Marked
networkmodeasrequired = truein the Java API command parameter annotations for VPC and Network offering creation flows.
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 4 comments.
| File | Description |
|---|---|
| ui/src/views/offering/AddVpcOffering.vue | Adds a required validation rule for networkmode in the VPC offering creation form. |
| ui/src/views/offering/AddNetworkOffering.vue | Adds a required validation rule for networkmode in the network offering creation form. |
| api/src/main/java/org/apache/cloudstack/api/command/admin/vpc/CreateVPCOfferingCmd.java | Sets networkmode as required in the API command parameter definition for VPC offering creation (currently impacts clone as well via inheritance). |
| api/src/main/java/org/apache/cloudstack/api/command/admin/network/NetworkOfferingBaseCmd.java | Sets networkmode as required in the shared base command for network offering create/clone (currently impacts clone as well via inheritance). |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| // ADD THIS LINE: | ||
| networkmode: [{ required: true, message: this.$t('message.error.select') }], |
There was a problem hiding this comment.
this makes sense as well @Chinmay048 , can you apply?
| // ADD THIS LINE: | ||
| networkmode: [{ required: true, message: this.$t('message.error.select') }], |
| @Parameter(name = ApiConstants.NETWORK_MODE, type = CommandType.STRING, required = true, description = "the network mode for the VPC offering") | ||
| private String networkMode; |
| @Parameter(name = ApiConstants.NETWORK_MODE, type = CommandType.STRING, required = true, description = "the network mode for the network offering") | ||
| private String networkMode; |
DaanHoogland
left a comment
DaanHoogland
left a comment
There was a problem hiding this comment.
why remove the possible values from the descriptions of the networkMode parameters?
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #13622 +/- ##
============================================
+ Coverage 19.50% 19.64% +0.14%
- Complexity 19444 19789 +345
============================================
Files 6303 6368 +65
Lines 569286 575107 +5821
Branches 69791 70370 +579
============================================
+ Hits 111040 112996 +1956
- Misses 446093 449826 +3733
- Partials 12153 12285 +132
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
|
@DaanHoogland Thanks for the guidance! I've added the required unit tests to cover the mandatory networkMode validation, verifying that the proper exception is thrown when the parameter is null or empty. Additionally, I have restored and properly scoped the API parameter descriptions for both the VPC and Network offerings that were unintentionally altered during the required flag update. Everything is compiling cleanly and passing unit tests on my end please let me know if this is ready for a merge! |
| description = "Indicates the mode with which the network will operate. Valid option: NATTED or ROUTED", | ||
| since = "4.20.0") | ||
| private String networkMode; | ||
| @Parameter(name = ApiConstants.NETWORK_MODE, type = CommandType.STRING, required = true, description = "the network mode for the network offering") |
There was a problem hiding this comment.
here you should also put back the possible values and in addition to that the since parameter. as well as in CreateVPCOffering.
| @Parameter(name = ApiConstants.INTERNET_PROTOCOL, | ||
| type = CommandType.STRING, | ||
| description = "The internet protocol of network offering. Options are IPv4 and dualstack. Default is IPv4. dualstack will create a network offering that supports both IPv4 and IPv6", | ||
| since = "4.17.0") | ||
| private String internetProtocol; | ||
|
|
||
| @Parameter(name = ApiConstants.SUPPORTED_SERVICES, | ||
| type = CommandType.LIST, | ||
| collectionType = CommandType.STRING, | ||
| description = "Services supported by the network offering") | ||
| private List<String> supportedServices; | ||
|
|
||
| @Parameter(name = ApiConstants.SERVICE_PROVIDER_LIST, | ||
| type = CommandType.MAP, | ||
| description = "Provider to service mapping. If not specified, the provider for the service will be mapped to the default provider on the physical network") | ||
| private Map serviceProviderList; | ||
|
|
||
| @Parameter(name = ApiConstants.SERVICE_CAPABILITY_LIST, type = CommandType.MAP, description = "Desired service capabilities as part of network offering") | ||
| private Map serviceCapabilitystList; | ||
|
|
||
| @Parameter(name = ApiConstants.SPECIFY_IP_RANGES, | ||
| type = CommandType.BOOLEAN, | ||
| description = "True if network offering supports specifying ip ranges; defaulted to false if not specified") | ||
| private Boolean specifyIpRanges; | ||
|
|
||
| @Parameter(name = ApiConstants.IS_PERSISTENT, | ||
| type = CommandType.BOOLEAN, | ||
| description = "True if network offering supports persistent networks; defaulted to false if not specified") | ||
| private Boolean isPersistent; | ||
|
|
||
| @Parameter(name = ApiConstants.FOR_VPC, | ||
| type = CommandType.BOOLEAN, | ||
| description = "True if network offering is meant to be used for VPC, false otherwise.") | ||
| private Boolean forVpc; | ||
|
|
||
| @Parameter(name = ApiConstants.FOR_NSX, | ||
| type = CommandType.BOOLEAN, | ||
| description = "true if network offering is meant to be used for NSX, false otherwise.", | ||
| since = "4.20.0") | ||
| private Boolean forNsx; | ||
|
|
||
| @Parameter(name = ApiConstants.NSX_SUPPORT_LB, | ||
| type = CommandType.BOOLEAN, | ||
| description = "True if network offering for NSX network offering supports Load balancer service.", | ||
| since = "4.20.0") | ||
| private Boolean nsxSupportsLbService; | ||
|
|
||
| @Parameter(name = ApiConstants.NSX_SUPPORTS_INTERNAL_LB, | ||
| type = CommandType.BOOLEAN, | ||
| description = "True if network offering for NSX network offering supports Internal Load balancer service.", | ||
| since = "4.20.0") | ||
| private Boolean nsxSupportsInternalLbService; | ||
|
|
||
| @Parameter( | ||
| name = ApiConstants.NETWORK_MODE, | ||
| type = CommandType.STRING, | ||
| required = true, | ||
| description = "the network mode of the network offering, possible values are NATTED and ROUTED" | ||
| ) | ||
| private String networkMode; | ||
|
|
||
| @Parameter(name = ApiConstants.FOR_TUNGSTEN, | ||
| type = CommandType.BOOLEAN, | ||
| description = "True if network offering is meant to be used for Tungsten-Fabric, false otherwise.") | ||
| private Boolean forTungsten; | ||
|
|
||
| @Parameter(name = ApiConstants.DETAILS, type = CommandType.MAP, since = "4.2.0", description = "Network offering details in key/value pairs." | ||
| + " Supported keys are internallbprovider/publiclbprovider with service provider as a value, and" | ||
| + " promiscuousmode/macaddresschanges/forgedtransmits with true/false as value to accept/reject the security settings if available for a nic/portgroup") | ||
| protected Map details; | ||
|
|
||
| @Parameter(name = ApiConstants.EGRESS_DEFAULT_POLICY, | ||
| type = CommandType.BOOLEAN, | ||
| description = "True if guest network default egress policy is allow; false if default egress policy is deny") | ||
| private Boolean egressDefaultPolicy; | ||
|
|
||
| @Parameter(name = ApiConstants.KEEPALIVE_ENABLED, | ||
| type = CommandType.BOOLEAN, | ||
| required = false, | ||
| description = "If true keepalive will be turned on in the loadbalancer. At the time of writing this has only an effect on haproxy; the mode http and httpclose options are unset in the haproxy conf file.") | ||
| private Boolean keepAliveEnabled; | ||
|
|
||
| @Parameter(name = ApiConstants.MAX_CONNECTIONS, | ||
| type = CommandType.INTEGER, | ||
| description = "Maximum number of concurrent connections supported by the Network offering") | ||
| private Integer maxConnections; | ||
|
|
||
| @Parameter(name = ApiConstants.DOMAIN_ID, | ||
| type = CommandType.LIST, | ||
| collectionType = CommandType.UUID, | ||
| entityType = DomainResponse.class, | ||
| description = "The ID of the containing domain(s), null for public offerings") | ||
| private List<Long> domainIds; | ||
|
|
||
| @Parameter(name = ApiConstants.ZONE_ID, | ||
| type = CommandType.LIST, | ||
| collectionType = CommandType.UUID, | ||
| entityType = ZoneResponse.class, | ||
| description = "The ID of the containing zone(s), null for public offerings", | ||
| since = "4.13") | ||
| private List<Long> zoneIds; | ||
|
|
||
| @Parameter(name = ApiConstants.ENABLE, | ||
| type = CommandType.BOOLEAN, | ||
| description = "Set to true if the offering is to be enabled during creation. Default is false", | ||
| since = "4.16") | ||
| private Boolean enable; | ||
|
|
||
| @Parameter(name = ApiConstants.SPECIFY_AS_NUMBER, type = CommandType.BOOLEAN, since = "4.20.0", | ||
| description = "true if network offering supports choosing AS number") | ||
| private Boolean specifyAsNumber; | ||
|
|
||
| @Parameter(name = ApiConstants.ROUTING_MODE, | ||
| type = CommandType.STRING, | ||
| since = "4.20.0", | ||
| description = "the routing mode for the network offering. Supported types are: Static or Dynamic.") | ||
| private String routingMode; | ||
|
|
||
|
|
There was a problem hiding this comment.
Next make sure all annotations in have a since parameter in ``CreateNetworkOffering` containing "4.24.0” , the expected version to add this PR to.
There was a problem hiding this comment.
Actually, having a second look at this change I am not so sure about it anymore.
CC/can you have a look @weizhouapache @vishesh92 ?
There was a problem hiding this comment.
Do not make API changes !!!
There was a problem hiding this comment.
@weizhouapache @DaanHoogland Understood! I realize that adding required = true to existing @ Parameter annotations breaks backward compatibility for external scripts and API clients.
I will immediately revert the required = true changes in both CreateVPCOfferingCmd and CreateNetworkOfferingCmd so the external API contract remains untouched. To enforce the required network mode behavior safely, I will move the validation and exception handling strictly into the backend service/manager layer instead. I'll push the updated approach shortly!
Description
This PR resolves an issue where the
networkmodeparameter (ROUTED vs NATTED) was treated as optional during the creation of VPC and Network Offerings, leading to ambiguous network configurations.Full-Stack Boundary Enforcement:
required: true) inAddVpcOffering.vueandAddNetworkOffering.vueto give administrators immediate visual feedback and prevent form submission if a network mode is not selected.required = trueto the@Parameterannotation inCreateVPCOfferingCmd.javaandNetworkOfferingBaseCmd.java(which governs creation and cloning operations without impacting updates).Types of changes
Checklist: