weizhouapache commented on code in PR #10174:
URL: https://github.com/apache/cloudstack/pull/10174#discussion_r1909320377
##########
services/secondary-storage/controller/src/main/java/org/apache/cloudstack/secondarystorage/SecondaryStorageManagerImpl.java:
##########
@@ -402,11 +402,17 @@ private List<String> getAllowedInternalSiteCidrs() {
}
String[] cidrs = _allowedInternalSites.split(",");
for (String cidr : cidrs) {
- if (NetUtils.isValidIp4Cidr(cidr) || NetUtils.isValidIp4(cidr) ||
!cidr.startsWith("0.0.0.0")) {
- if (NetUtils.getCleanIp4Cidr(cidr).equals(cidr)) {
+ if (NetUtils.isValidIp4Cidr(cidr)) {
Review Comment:
this looks fine
##########
services/secondary-storage/controller/src/main/java/org/apache/cloudstack/secondarystorage/SecondaryStorageManagerImpl.java:
##########
@@ -402,11 +402,17 @@ private List<String> getAllowedInternalSiteCidrs() {
}
String[] cidrs = _allowedInternalSites.split(",");
for (String cidr : cidrs) {
- if (NetUtils.isValidIp4Cidr(cidr) || NetUtils.isValidIp4(cidr) ||
!cidr.startsWith("0.0.0.0")) {
- if (NetUtils.getCleanIp4Cidr(cidr).equals(cidr)) {
+ if (NetUtils.isValidIp4Cidr(cidr)) {
+ if (! NetUtils.getCleanIp4Cidr(cidr).equals(cidr)) {
s_logger.warn(String.format("Invalid CIDR %s in %s", cidr,
SecStorageAllowedInternalDownloadSites.key()));
}
- allowedCidrs.add(cidr);
+ allowedCidrs.add(NetUtils.getCleanIp4Cidr(cidr));
+ } else if (NetUtils.isValidIp4(cidr)) {
+ String newCidr = cidr + "/32";
Review Comment:
this is good.
alternatively we can pass `cidr`.
```
allowedCidrs.add(cidr);
```
the results are same
```
ip route add <ip> via <gw>
ip route add <ip>/32 via <gw>
```
##########
services/secondary-storage/controller/src/main/java/org/apache/cloudstack/secondarystorage/SecondaryStorageManagerImpl.java:
##########
@@ -402,11 +402,17 @@ private List<String> getAllowedInternalSiteCidrs() {
}
String[] cidrs = _allowedInternalSites.split(",");
for (String cidr : cidrs) {
- if (NetUtils.isValidIp4Cidr(cidr) || NetUtils.isValidIp4(cidr) ||
!cidr.startsWith("0.0.0.0")) {
- if (NetUtils.getCleanIp4Cidr(cidr).equals(cidr)) {
+ if (NetUtils.isValidIp4Cidr(cidr)) {
+ if (! NetUtils.getCleanIp4Cidr(cidr).equals(cidr)) {
s_logger.warn(String.format("Invalid CIDR %s in %s", cidr,
SecStorageAllowedInternalDownloadSites.key()));
}
- allowedCidrs.add(cidr);
+ allowedCidrs.add(NetUtils.getCleanIp4Cidr(cidr));
+ } else if (NetUtils.isValidIp4(cidr)) {
+ String newCidr = cidr + "/32";
+ s_logger.warn(String.format("Ip address is not a valid CIDR;
%s using %s/32", cidr, newCidr));
+ allowedCidrs.add(newCidr);
+ } else if (!cidr.startsWith("0.0.0.0")) {
+ allowedCidrs.add(NetUtils.getCleanIp4Cidr(cidr));
Review Comment:
this is what I was confused
I have no idea what is the correct check
maybe it should be `if (cidr.startsWith("0.0.0.0"))` , so that 0.0.0.0/0 is
accepted (which is same as `ip route add default via xxx`) ?
the checks need be consistent with
https://github.com/apache/cloudstack/blob/fadb39ece73775e3aadbe715ca428b24e256af34/server/src/main/java/com/cloud/configuration/ConfigurationManagerImpl.java#L1355-L1362
--
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]
