Copilot commented on code in PR #12706:
URL: https://github.com/apache/cloudstack/pull/12706#discussion_r3225843519
##########
server/src/main/java/com/cloud/network/firewall/FirewallManagerImpl.java:
##########
@@ -198,25 +198,62 @@ public FirewallRule createEgressFirewallRule(FirewallRule
rule) throws NetworkRu
if (sourceCidrs != null && !sourceCidrs.isEmpty())
Collections.replaceAll(sourceCidrs, "0.0.0.0/0", network.getCidr());
- return createFirewallRule(null, caller, rule.getXid(),
rule.getSourcePortStart(), rule.getSourcePortEnd(), rule.getProtocol(),
sourceCidrs, rule.getDestinationCidrList(),
- rule.getIcmpCode(), rule.getIcmpType(), null, rule.getType(),
rule.getNetworkId(), rule.getTrafficType(), rule.isDisplay());
+ return createFirewallRuleForNonVPC(null, caller, rule.getXid(),
rule.getSourcePortStart(), rule.getSourcePortEnd(), rule.getProtocol(),
sourceCidrs,
+ rule.getDestinationCidrList(), rule.getIcmpCode(),
rule.getIcmpType(), null, rule.getType(), rule.getNetworkId(),
rule.getTrafficType(), rule.isDisplay());
}
@Override
@ActionEvent(eventType = EventTypes.EVENT_FIREWALL_OPEN, eventDescription
= "creating firewall rule", create = true)
public FirewallRule createIngressFirewallRule(FirewallRule rule) throws
NetworkRuleConflictException {
- Account caller = CallContext.current().getCallingAccount();
+ Account caller = CallContext.current().getCallingAccount();
Long sourceIpAddressId = rule.getSourceIpAddressId();
+ IPAddressVO sourceIp = getSourceIpForIngressRule(sourceIpAddressId);
+
Review Comment:
`createIngressFirewallRule` dereferences `sourceIp` without checking for
null. Since `getSourceIpForIngressRule` returns null when `sourceIpAddressId`
is null (or when the IP cannot be found), this can throw a NullPointerException
at `sourceIp.getVpcId()`. Please add validation/error handling for
missing/invalid source IP before branching on VPC vs isolated handling.
##########
server/src/main/java/com/cloud/network/firewall/FirewallManagerImpl.java:
##########
@@ -283,6 +320,158 @@ protected FirewallRule createFirewallRule(final Long
ipAddrId, Account caller, f
}
}
+ @DB
+ protected FirewallRule createFirewallRuleForVpc(final Long ipAddrId,
Account caller, final String xId, final Integer portStart, final Integer
portEnd, final String protocol,
+ final List<String>
sourceCidrList, final List<String> destCidrList, final Integer icmpCode, final
Integer icmpType,
+ final Long relatedRuleId,
final FirewallRuleType type, final Long vpcId,
+ final
FirewallRule.TrafficType trafficType, final Boolean forDisplay) throws
NetworkRuleConflictException {
+ IPAddressVO ipAddress = null;
+ try {
+ Long resolvedVpcId = vpcId;
+ if (ipAddrId != null) {
+ ipAddress = _ipAddressDao.acquireInLockTable(ipAddrId);
+ if (ipAddress == null) {
+ throw new InvalidParameterValueException("Unable to create
firewall rule; " + "couldn't locate IP address by id in the system");
+ }
+ resolvedVpcId = resolvedVpcId != null ? resolvedVpcId :
ipAddress.getVpcId();
+ }
+
+ if (resolvedVpcId == null) {
+ throw new InvalidParameterValueException("Unable to create VPC
firewall rule; couldn't locate VPC id");
+ }
+
+ validateFirewallRuleForVpc(caller, ipAddress, portStart, portEnd,
protocol, Purpose.Firewall, type, resolvedVpcId, trafficType);
+
+ if (!protocol.equalsIgnoreCase(NetUtils.ICMP_PROTO) && (icmpCode
!= null || icmpType != null)) {
+ throw new InvalidParameterValueException("Can specify icmpCode
and icmpType for ICMP protocol only");
+ }
+
+ if (protocol.equalsIgnoreCase(NetUtils.ICMP_PROTO) && (portStart
!= null || portEnd != null)) {
+ throw new InvalidParameterValueException("Can't specify
start/end port when protocol is ICMP");
+ }
+
+ Long accountId = null;
+ Long domainId = null;
+
+ if (ipAddress != null) {
+ accountId = ipAddress.getAllocatedToAccountId();
+ domainId = ipAddress.getAllocatedInDomainId();
+ } else {
+ Vpc vpc = _vpcMgr.getActiveVpc(resolvedVpcId);
+ if (vpc == null) {
+ throw new InvalidParameterValueException("Unable to create
VPC firewall rule; couldn't locate VPC by id=" + resolvedVpcId);
+ }
+ accountId = vpc.getAccountId();
+ domainId = vpc.getDomainId();
+ }
+
+ final Long accountIdFinal = accountId;
+ final Long domainIdFinal = domainId;
+ final Long resolvedNetworkIdFinal = null;
+ final Long resolvedVpcIdFinal = resolvedVpcId;
+ return
Transaction.execute((TransactionCallbackWithException<FirewallRuleVO,
NetworkRuleConflictException>) status -> {
+ FirewallRuleVO newRule = new FirewallRuleVO(xId, ipAddrId,
portStart, portEnd, protocol.toLowerCase(), resolvedNetworkIdFinal,
accountIdFinal, domainIdFinal, Purpose.Firewall,
+ sourceCidrList, destCidrList, icmpCode, icmpType,
relatedRuleId, trafficType);
+ newRule.setVpcId(resolvedVpcIdFinal);
+ newRule.setType(type);
Review Comment:
`createFirewallRuleForVpc` persists a `FirewallRuleVO` with `network_id` set
to null (`resolvedNetworkIdFinal = null`). The DB schema defines
`firewall_rules.network_id` as `NOT NULL` (see `setup/db/create-schema.sql`),
so this insert will fail at runtime. Either persist a valid networkId for VPC
firewall rules (e.g., derived from the source IP’s `sourceNetworkId`) or update
the schema/constraints accordingly (including upgrade paths).
##########
server/src/main/java/com/cloud/configuration/ConfigurationManagerImpl.java:
##########
@@ -7223,10 +7223,12 @@ public NetworkOffering createNetworkOffering(final
NetworkOfferingBaseCmd cmd) {
}
if (forVpc == null) {
- if (service == Service.SecurityGroup || service ==
Service.Firewall) {
+ if (service == Service.SecurityGroup) {
forVpc = false;
} else if (service == Service.NetworkACL) {
forVpc = true;
+ } else if (service == Service.Firewall) {
+ forVpc = true;
Review Comment:
When `forVpc` is not explicitly provided, the inference now sets `forVpc =
true` as soon as the service list contains `Service.Firewall`. Since Firewall
is also valid for non-VPC (isolated) offerings, this can misclassify offerings
and change API behavior for callers that omit `forVpc`. Consider inferring
`forVpc` only from VPC-specific services (e.g. `NetworkACL`) or requiring
`forVpc` to be explicitly set when the service mix is ambiguous.
--
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]
