Copilot commented on code in PR #12622:
URL: https://github.com/apache/cloudstack/pull/12622#discussion_r3021176325
##########
engine/schema/src/main/resources/META-INF/db/schema-42200to42210.sql:
##########
@@ -34,4 +34,13 @@ UPDATE `cloud`.`alert` SET type = 34 WHERE name =
'ALERT.VR.PRIVATE.IFACE.MTU';
-- Update configuration 'kvm.ssh.to.agent' description and is_dynamic fields
UPDATE `cloud`.`configuration` SET description = 'True if the management
server will restart the agent service via SSH into the KVM hosts after or
during maintenance operations', is_dynamic = 1 WHERE name = 'kvm.ssh.to.agent';
+-- Sanitize legacy network-level addressing fields for Public networks
+UPDATE `cloud`.`networks`
+SET `broadcast_uri` = NULL,
+ `gateway` = NULL,
+ `cidr` = NULL,
+ `ip6_gateway` = NULL,
+ `ip6_cidr` = NULL
+WHERE `traffic_type` = 'Public';
Review Comment:
This migration nulls `cidr`/`gateway` (and `broadcast_uri`) for all
`traffic_type='Public'` networks. With the current `NetworkVO.equals()`
implementation, two Public networks with `cidr == null` compare as equal, while
`NetworkVO.hashCode()` is based on `id`; this violates the equals/hashCode
contract and can break HashSet/Map behavior (and may become more likely after
this UPDATE makes `cidr` null everywhere). Consider either (a) updating
`NetworkVO.equals()`/`hashCode()` to be consistent (and to handle
comma-separated CIDRs as described in the PR), or (b) narrowing this UPDATE to
only sanitize rows that actually contain legacy comma-separated values so you
don’t increase the number of `cidr == null` Public networks.
```suggestion
WHERE `traffic_type` = 'Public`
AND (
`cidr` LIKE '%,%'
OR `ip6_cidr` LIKE '%,%'
OR `gateway` LIKE '%,%'
OR `ip6_gateway` LIKE '%,%'
);
```
##########
server/src/main/java/com/cloud/configuration/ConfigurationManagerImpl.java:
##########
@@ -6507,11 +6507,14 @@ private boolean
deleteAndPublishVlanAndPublicIpRange(final long userId, final lo
final boolean ipv4 = deletedVlan.getVlanGateway() != null;
final boolean ipv6 = deletedVlan.getIp6Gateway() != null;
final long networkId = deletedVlan.getNetworkId();
+ final NetworkVO networkVO = _networkDao.findById(networkId);
- if (ipv4) {
- removeCidrAndGatewayForIpv4(networkId, deletedVlan);
- } else if (ipv6) {
- removeCidrAndGatewayForIpv6(networkId, deletedVlan);
+ if (networkVO != null && networkVO.getTrafficType() !=
TrafficType.Public) {
+ if (ipv4) {
+ removeCidrAndGatewayForIpv4(networkId, deletedVlan);
Review Comment:
`networkVO` is fetched here to check traffic type, but
`removeCidrAndGatewayForIpv4/Ipv6` immediately re-fetch the same `NetworkVO`
again. To avoid the extra DB hit (and keep the code easier to follow), consider
passing the already-fetched `NetworkVO` into the remove* helpers or moving the
traffic-type guard into those helpers.
--
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]
