[GitHub] [cloudstack] weizhouapache commented on a diff in pull request #6442: Possibility to choose the initial IP address on a isolated network or VPC

[GitBox <gi...@apache.org>]

weizhouapache commented on code in PR #6442:
URL: https://github.com/apache/cloudstack/pull/6442#discussion_r1082554632


##########
api/src/main/java/com/cloud/network/Network.java:
##########
@@ -268,6 +268,7 @@ public static class Capability {
         public static final Capability LoadBalancingSupportedIps = new Capability("LoadBalancingSupportedIps");
         public static final Capability AllowDnsSuffixModification = new Capability("AllowDnsSuffixModification");
         public static final Capability RedundantRouter = new Capability("RedundantRouter");
+        public static final Capability SelectSnatIpAllowed = new Capability("SelectSnatIpAllowed");

Review Comment:
   The capability name seems inconsistent with other capabilities.
   Maybe better to use AllowSpecificSourceNatIp or just SpecifySourceNatIp



##########
api/src/main/java/org/apache/cloudstack/api/ApiConstants.java:
##########
@@ -931,6 +931,7 @@ public class ApiConstants {
     public static final String LOGIN = "login";
     public static final String LOGOUT = "logout";
     public static final String LIST_IDPS = "listIdps";
+    public static final String IS_SELECTION_OF_STATIC_NAT_ALLOWED = "selectsnatipallowed";

Review Comment:
   I think it is Source Nat IP, not Static Nat Ip, right ?



##########
api/src/main/java/org/apache/cloudstack/api/response/NetworkOfferingResponse.java:
##########
@@ -143,6 +143,10 @@ public class NetworkOfferingResponse extends BaseResponseWithAnnotations {
     @Param(description = "the internet protocol of the network offering")
     private String internetProtocol;
 
+    @SerializedName(ApiConstants.IS_SELECTION_OF_STATIC_NAT_ALLOWED)

Review Comment:
   Better to use SourceNat instead of Snat



##########
engine/schema/src/main/java/com/cloud/offerings/NetworkOfferingVO.java:
##########
@@ -165,6 +165,17 @@ public String getDisplayText() {
     @Column(name="service_package_id")
     String servicePackageUuid = null;
 
+    @Column(name = "select_snat_address_allowed")

Review Comment:
   Can we use network offering details instead of a new column ?
   well, it means lots of changes maybe



##########
server/src/main/java/com/cloud/network/NetworkServiceImpl.java:
##########
@@ -1462,12 +1480,12 @@ public Network createGuestNetwork(CreateNetworkCmd cmd) throws InsufficientCapac
             }
         }
 
-        if (ntwkOff.getGuestType() != GuestType.Shared && (!StringUtils.isAllBlank(routerIp, routerIpv6))) {
-            throw new InvalidParameterValueException("Router IP can be specified only for Shared networks");
+        if (ntwkOff.getGuestType() == GuestType.L2 && (!StringUtils.isAllBlank(cmd.getRouterIPv4(), cmd.getRouterIPv6()))) {

Review Comment:
   Is there a validation for isolated networks source 
   nat support and capabilities here ?



##########
server/src/main/java/com/cloud/network/NetworkServiceImpl.java:
##########
@@ -1289,15 +1289,12 @@ private void checkSharedNetworkCidrOverlap(Long zoneId, long physicalNetworkId,
         }
     }
 
-    private void validateRouterIps(String routerIp, String routerIpv6, String startIp, String endIp, String gateway,
-                                   String netmask, String startIpv6, String endIpv6, String ip6Cidr) {
+    private void validateSharedRouterIPv4(String routerIp, String startIp, String endIp, String gateway, String netmask) {

Review Comment:
   SharedNetwork instead of only Shared?



-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: commits-unsubscribe@cloudstack.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org