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: [email protected]

For queries about this service, please contact Infrastructure at:
[email protected]

Reply via email to