Support Firewall for public IPs in VPC#12706
Conversation
boring-cyborg
Bot
added
component:kubernetes
component:networking
component:orchestration
component:UI
component:virtual-router
Python
|
@blueorangutan package |
|
@harikrishna-patnala a [SL] Jenkins job has been kicked to build packages. It will be bundled with no SystemVM templates. I'll keep you posted as I make progress. |
|
Packaging result [SF]: ✔️ el8 ✔️ el9 ✔️ el10 ✔️ debian ✔️ suse15. SL-JID 16942 |
There was a problem hiding this comment.
Pull request overview
This PR adds support for firewall rules on public IPs in VPC networks, which were previously only available for non-VPC isolated networks. This is a major feature addition that enables VPC users to configure ingress/egress firewall rules on their public IP addresses, similar to the functionality available in traditional isolated networks.
Changes:
- Enabled Firewall service for VPC offerings and networks by removing it from the list of non-supported services
- Added Firewall capability to VpcVirtualRouterElement by removing the code that previously excluded it
- Updated UI to show firewall tabs for VPC public IPs in various states (source NAT, static NAT, port forwarding, load balancing)
- Implemented iptables firewall chain logic in VPC virtual routers to support per-public-IP firewall rules
- Added database migration to retrofit existing VPC offerings, VPC networks, and VPCs with Firewall service
Reviewed changes
Copilot reviewed 9 out of 11 changed files in this pull request and generated 6 comments.
Show a summary per file
| File | Description |
|---|---|
| ui/src/views/offering/AddVpcOffering.vue | Adds Firewall service with VpcVirtualRouter provider to the list of services when creating VPC offerings |
| ui/src/views/network/PublicIpResource.vue | Updates tab visibility logic to show firewall tab for VPC IPs in various states (source NAT, static NAT, etc.) |
| systemvm/debian/opt/cloud/bin/cs/CsAddress.py | Implements PREROUTING firewall chain jump and firewall chain with default DROP policy for public IPs in VPC routers |
| systemvm/debian/opt/cloud/bin/configure.py | Modifies ACL processing to use AclIP (per-IP firewall) instead of AclDevice for VPC firewall rules |
| server/src/main/java/org/apache/cloudstack/network/RoutedIpv4ManagerImpl.java | Removes VPCVirtualRouter check from isVirtualRouterGateway method for Network parameter |
| server/src/main/java/com/cloud/network/vpc/VpcManagerImpl.java | Removes Service.Firewall from the list of non-supported VPC services |
| server/src/main/java/com/cloud/network/element/VpcVirtualRouterElement.java | Re-enables Firewall capability for VPC Virtual Router by removing the code that excluded it |
| server/src/main/java/com/cloud/configuration/ConfigurationManagerImpl.java | Updates network offering validation to no longer automatically set forVpc=false when Firewall service is included |
| plugins/integrations/kubernetes-service/src/main/java/com/cloud/kubernetes/cluster/KubernetesClusterManagerImpl.java | Updates Kubernetes VPC network offering creation to include Firewall service with VPCVirtualRouter provider |
| engine/schema/src/main/java/com/cloud/upgrade/dao/Upgrade42210to42300.java | Adds database migration to insert Firewall service mappings for existing VPC offerings, networks, and VPCs |
| engine/orchestration/src/main/java/org/apache/cloudstack/engine/orchestration/NetworkOrchestrator.java | Adds Firewall service to default VPC offering providers |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| public boolean isVirtualRouterGateway(Network network) { | ||
| return isRoutedNetwork(network) | ||
| && (networkServiceMapDao.canProviderSupportServiceInNetwork(network.getId(), Service.Gateway, Provider.VirtualRouter)) | ||
| || networkServiceMapDao.canProviderSupportServiceInNetwork(network.getId(), Service.Gateway, Provider.VPCVirtualRouter); | ||
| && (networkServiceMapDao.canProviderSupportServiceInNetwork(network.getId(), Service.Gateway, Provider.VirtualRouter)); |
There was a problem hiding this comment.
The removal of the VPCVirtualRouter check from the isVirtualRouterGateway(Network network) method creates an inconsistency with the overloaded isVirtualRouterGateway(NetworkOffering networkOffering) method at lines 996-999, which still checks for VPCVirtualRouter. This inconsistency could lead to different behavior depending on which method is called. Additionally, there's a potential operator precedence issue in lines 997-999 where the && operator may not bind as intended with the || operator without proper parentheses.
|
[SF] Trillian Build Failed (tid-15532) |
|
@blueorangutan package |
|
@harikrishna-patnala a [SL] Jenkins job has been kicked to build packages. It will be bundled with no SystemVM templates. I'll keep you posted as I make progress. |
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #12706 +/- ##
==========================================
Coverage 18.08% 18.09%
- Complexity 16706 16736 +30
==========================================
Files 6037 6037
Lines 542437 542827 +390
Branches 66420 66486 +66
==========================================
+ Hits 98088 98206 +118
- Misses 433333 433557 +224
- Partials 11016 11064 +48
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 10 out of 12 changed files in this pull request and generated 5 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
@blueorangutan package |
|
@harikrishna-patnala a [SL] Jenkins job has been kicked to build packages. It will be bundled with no SystemVM templates. I'll keep you posted as I make progress. |
|
Packaging result [SF]: ✔️ el8 ✔️ el9 ✔️ el10 ✔️ debian ✔️ suse15. SL-JID 16957 |
|
@blueorangutan package |
|
@harikrishna-patnala a [SL] Jenkins job has been kicked to build packages. It will be bundled with no SystemVM templates. I'll keep you posted as I make progress. |
|
Packaging result [SF]: ✔️ el8 ✔️ el9 ✔️ el10 ✔️ debian ✔️ suse15. SL-JID 16959 |
|
@blueorangutan package |
|
@harikrishna-patnala a [SL] Jenkins job has been kicked to build packages. It will be bundled with no SystemVM templates. I'll keep you posted as I make progress. |
|
Packaging result [SF]: ✔️ el8 ✔️ el9 ✔️ el10 ✔️ debian ✔️ suse15. SL-JID 16983 |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 11 out of 13 changed files in this pull request and generated 7 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| // VPC IPs with static nat have firewall (only if associatednetworkid present) | ||
| if (this.resource.isstaticnat) { | ||
| if (this.resource.virtualmachinetype === 'DomainRouter') { | ||
| this.tabs = this.defaultTabs.concat(this.$route.meta.tabs.filter(tab => tab.name === 'vpn')) | ||
| let tabs = this.defaultTabs.concat(this.$route.meta.tabs.filter(tab => tab.name === 'vpn')) | ||
| if (this.resource.associatednetworkid) { | ||
| tabs = this.defaultTabs.concat(this.$route.meta.tabs.filter(tab => ['vpn', 'firewall'].includes(tab.name))) | ||
| } | ||
| this.tabs = tabs |
There was a problem hiding this comment.
The updated comment says “VPC IPs with static nat have firewall…”, but in the virtualmachinetype === 'DomainRouter' branch the code still only adds the vpn tab unless associatednetworkid is present. Either adjust the comment to match the actual behavior, or update the tab logic to reflect the intended UX.
| while (networksRs.next()) { | ||
| long networkId = networksRs.getLong(1); | ||
| try (PreparedStatement insertService = conn.prepareStatement( | ||
| "INSERT INGORE INTO `cloud`.`ntwk_service_map` " + |
There was a problem hiding this comment.
The SQL statement string has a typo: INSERT INGORE should be INSERT IGNORE. As written, this migration will fail at runtime when updating ntwk_service_map.
| "INSERT INGORE INTO `cloud`.`ntwk_service_map` " + | |
| "INSERT IGNORE INTO `cloud`.`ntwk_service_map` " + |
|
|
||
| for (String filePath : filesPathUnderViewsDirectory) { | ||
| LOGGER.debug("Executing PROCEDURE script [{}].", filePath); | ||
| LOGGER.debug(String.format("Executing PROCEDURE script [%s].", filePath)); |
There was a problem hiding this comment.
Avoid String.format inside LOGGER.debug(...) here: it eagerly allocates even when debug logging is disabled and bypasses SLF4J parameterized logging. Prefer LOGGER.debug("Executing PROCEDURE script [{}].", filePath) (and similarly after normalizing filePath if you want the logged value to match what is executed).
| LOGGER.debug(String.format("Executing PROCEDURE script [%s].", filePath)); | |
| LOGGER.debug("Executing PROCEDURE script [{}].", filePath); |
| filePath = filePath.substring(1); | ||
| } | ||
|
|
||
| InputStream viewScript = Thread.currentThread().getContextClassLoader().getResourceAsStream(filePath); |
There was a problem hiding this comment.
getResourceAsStream(filePath) can return null; runScript(conn, viewScript) will then throw a NullPointerException (not caught in runScript). Add a null check and throw a CloudRuntimeException that includes the resolved filePath to make failures actionable.
| InputStream viewScript = Thread.currentThread().getContextClassLoader().getResourceAsStream(filePath); | |
| InputStream viewScript = Thread.currentThread().getContextClassLoader().getResourceAsStream(filePath); | |
| if (viewScript == null) { | |
| String message = String.format("Unable to find PROCEDURE script resource [%s] on the classpath.", filePath); | |
| LOGGER.error(message); | |
| throw new CloudRuntimeException(message); | |
| } |
| LOGGER.debug(String.format("Executing VIEW script [%s].", filePath)); | ||
| if (filePath.startsWith("/")) { | ||
| filePath = filePath.substring(1); | ||
| } | ||
|
|
There was a problem hiding this comment.
Same as above: String.format eagerly builds the message even when debug logging is disabled. Prefer SLF4J parameterized logging (LOGGER.debug("Executing VIEW script [{}].", filePath)) and consider logging the normalized path if that’s what is executed.
| LOGGER.debug(String.format("Executing VIEW script [%s].", filePath)); | |
| if (filePath.startsWith("/")) { | |
| filePath = filePath.substring(1); | |
| } | |
| if (filePath.startsWith("/")) { | |
| filePath = filePath.substring(1); | |
| } | |
| LOGGER.debug("Executing VIEW script [{}].", filePath); |
| filePath = filePath.substring(1); | ||
| } | ||
|
|
||
| InputStream viewScript = Thread.currentThread().getContextClassLoader().getResourceAsStream(filePath); |
There was a problem hiding this comment.
Same null-safety issue for view scripts: if getResourceAsStream(filePath) returns null, runScript will NPE. Add an explicit null check and fail with a clear exception that includes the script path.
| InputStream viewScript = Thread.currentThread().getContextClassLoader().getResourceAsStream(filePath); | |
| InputStream viewScript = Thread.currentThread().getContextClassLoader().getResourceAsStream(filePath); | |
| if (viewScript == null) { | |
| String message = String.format("VIEW script resource [%s] could not be found on the classpath.", filePath); | |
| LOGGER.error(message); | |
| throw new CloudRuntimeException(message); | |
| } |
| services.push({ | ||
| name: 'Firewall', | ||
| provider: [ | ||
| { name: 'VpcVirtualRouter' } | ||
| ] | ||
| }) |
There was a problem hiding this comment.
Firewall is only added to the services list for the non-NSX/non-Netris path. Since the backend changes add Firewall support to VPC offerings (including NSX/Netris VPC offerings via VpcVirtualRouter), the UI should also include the Firewall service in the NSX and Netris branches; otherwise those offerings can’t be configured with Firewall from the UI.
vishesh92
left a comment
vishesh92
left a comment
There was a problem hiding this comment.
I had some doubts about a few changes. Left some comments.
| continue | ||
| if self.config.is_vpc(): | ||
| continue | ||
| if self.config.is_vpc() and not ("purpose" in self.dbag[item] and self.dbag[item]["purpose"] == "Firewall"): |
There was a problem hiding this comment.
I am not exactly sure about this change.
There was a problem hiding this comment.
firewall case is skipped here and handled in next else case
|
This pull request has merge conflicts. Dear author, please fix the conflicts and sync your branch with the base branch. |
|
[SF] Trillian Build Failed (tid-16045) |
harikrishna-patnala
force-pushed
the
FWinVPCpublicIP
branch
from
1022c61 to
8fec093
Compare
| Network network = ApiDBUtils.findNetworkById(fwRule.getNetworkId()); | ||
| response.setNetworkId(network.getUuid()); | ||
| Long networkId = fwRule.getNetworkId(); | ||
| if (networkId != null) { |
There was a problem hiding this comment.
Do we also need to add VPC id here?
There was a problem hiding this comment.
makes sense, I will add it
| Account caller = CallContext.current().getCallingAccount(); | ||
| Long sourceIpAddressId = rule.getSourceIpAddressId(); | ||
| IPAddressVO sourceIp = getSourceIpForIngressRule(sourceIpAddressId); | ||
|
|
| 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); |
| } else if (service == Service.Firewall) { | ||
| forVpc = true; |
vishesh92
left a comment
vishesh92
left a comment
There was a problem hiding this comment.
clgtm. just a few things which needs to be checked.
Description
This PR adds support for firewall rules on public IPs in VPC networks.
Doc PR apache/cloudstack-documentation#633
Types of changes
Feature/Enhancement Scale or Bug Severity
Feature/Enhancement Scale
Bug Severity
Screenshots (if appropriate):
How Has This Been Tested?
How did you try to break this feature and the system with this change?