Copilot commented on code in PR #12706:
URL: https://github.com/apache/cloudstack/pull/12706#discussion_r2871853767
##########
engine/schema/src/main/java/com/cloud/upgrade/dao/Upgrade42210to42300.java:
##########
@@ -42,4 +50,126 @@ public InputStream[] getPrepareScripts() {
return new InputStream[] {script};
}
+
+ @Override
+ public void performDataMigration(Connection conn) {
+ updateNetworkDefaultOfferingsForVPCWithFirewallService(conn);
+ updateVpcOfferingsWithFirewallService(conn);
+ }
+
+ private void
updateNetworkDefaultOfferingsForVPCWithFirewallService(Connection conn) {
+ logger.debug("Updating default Network offerings for VPC to add
Firewall service with VpcVirtualRouter provider");
+
+ final List<String> defaultVpcOfferingUniqueNames = Arrays.asList(
+ NetworkOffering.DefaultIsolatedNetworkOfferingForVpcNetworks,
+
NetworkOffering.DefaultIsolatedNetworkOfferingForVpcNetworksNoLB,
+
NetworkOffering.DefaultIsolatedNetworkOfferingForVpcNetworksWithInternalLB,
+ NetworkOffering.DEFAULT_NAT_NSX_OFFERING_FOR_VPC,
+ NetworkOffering.DEFAULT_ROUTED_NSX_OFFERING_FOR_VPC,
+ NetworkOffering.DEFAULT_NAT_NSX_OFFERING_FOR_VPC_WITH_ILB,
+ NetworkOffering.DEFAULT_ROUTED_NETRIS_OFFERING_FOR_VPC,
+ NetworkOffering.DEFAULT_NAT_NETRIS_OFFERING_FOR_VPC
+ );
+
+ try {
+ for (String uniqueName : defaultVpcOfferingUniqueNames) {
+ try (PreparedStatement pstmt = conn.prepareStatement("SELECT
id FROM `cloud`.`network_offerings` WHERE unique_name = ?")) {
+ pstmt.setString(1, uniqueName);
+ try (ResultSet rs = pstmt.executeQuery()) {
+ if (!rs.next()) {
+ continue;
+ }
+ long offeringId = rs.getLong(1);
+ // Insert into ntwk_offering_service_map
+ try (PreparedStatement insertOfferingPstmt =
conn.prepareStatement(
+ "INSERT IGNORE INTO
`cloud`.`ntwk_offering_service_map` " +
+ "(network_offering_id, service,
provider, created) " +
+ "VALUES (?, 'Firewall',
'VpcVirtualRouter', now())")) {
+ insertOfferingPstmt.setLong(1, offeringId);
+ insertOfferingPstmt.executeUpdate();
+ }
+ // Update existing networks (ntwk_service_map)
+ try (PreparedStatement selectNetworksPstmt =
conn.prepareStatement(
+ "SELECT id FROM `cloud`.`networks` WHERE
network_offering_id = ?")) {
+ selectNetworksPstmt.setLong(1, offeringId);
+ try (ResultSet networksRs =
selectNetworksPstmt.executeQuery()) {
+ while (networksRs.next()) {
+ long networkId = networksRs.getLong(1);
+ try (PreparedStatement insertService =
conn.prepareStatement(
+ "INSERT INGORE INTO
`cloud`.`ntwk_service_map` " +
Review 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`.
```suggestion
"INSERT IGNORE INTO
`cloud`.`ntwk_service_map` " +
```
##########
ui/src/views/network/PublicIpResource.vue:
##########
@@ -135,22 +135,39 @@ export default {
return
}
if (this.resource && this.resource.vpcid) {
- // VPC IPs with source nat have only VPN
+ // VPC IPs with source nat have VPN and Firewall (firewall only if
associatednetworkid present)
if (this.resource.issourcenat) {
- 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
return
}
- // VPC IPs with static nat have nothing
+ // 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
Review 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.
##########
engine/schema/src/main/java/com/cloud/upgrade/DatabaseUpgradeChecker.java:
##########
@@ -327,7 +327,10 @@ protected void executeProcedureScripts() {
Connection conn = txn.getConnection();
for (String filePath : filesPathUnderViewsDirectory) {
- LOGGER.debug("Executing PROCEDURE script [{}].", filePath);
+ LOGGER.debug(String.format("Executing PROCEDURE script [%s].",
filePath));
+ if (filePath.startsWith("/")) {
+ filePath = filePath.substring(1);
+ }
InputStream viewScript =
Thread.currentThread().getContextClassLoader().getResourceAsStream(filePath);
Review 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.
```suggestion
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);
}
```
##########
engine/schema/src/main/java/com/cloud/upgrade/DatabaseUpgradeChecker.java:
##########
@@ -439,7 +442,10 @@ protected void executeViewScripts() {
Connection conn = txn.getConnection();
for (String filePath : filesPathUnderViewsDirectory) {
- LOGGER.debug("Executing VIEW script [{}].", filePath);
+ LOGGER.debug(String.format("Executing VIEW script [%s].",
filePath));
+ if (filePath.startsWith("/")) {
+ filePath = filePath.substring(1);
+ }
Review 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.
```suggestion
if (filePath.startsWith("/")) {
filePath = filePath.substring(1);
}
LOGGER.debug("Executing VIEW script [{}].", filePath);
```
##########
engine/schema/src/main/java/com/cloud/upgrade/DatabaseUpgradeChecker.java:
##########
@@ -439,7 +442,10 @@ protected void executeViewScripts() {
Connection conn = txn.getConnection();
for (String filePath : filesPathUnderViewsDirectory) {
- LOGGER.debug("Executing VIEW script [{}].", filePath);
+ LOGGER.debug(String.format("Executing VIEW script [%s].",
filePath));
+ if (filePath.startsWith("/")) {
+ filePath = filePath.substring(1);
+ }
InputStream viewScript =
Thread.currentThread().getContextClassLoader().getResourceAsStream(filePath);
Review 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.
```suggestion
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);
}
```
##########
engine/schema/src/main/java/com/cloud/upgrade/DatabaseUpgradeChecker.java:
##########
@@ -327,7 +327,10 @@ protected void executeProcedureScripts() {
Connection conn = txn.getConnection();
for (String filePath : filesPathUnderViewsDirectory) {
- LOGGER.debug("Executing PROCEDURE script [{}].", filePath);
+ LOGGER.debug(String.format("Executing PROCEDURE script [%s].",
filePath));
Review 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).
```suggestion
LOGGER.debug("Executing PROCEDURE script [{}].", filePath);
```
##########
ui/src/views/offering/AddVpcOffering.vue:
##########
@@ -525,6 +525,12 @@ export default {
{ name: 'ConfigDrive' }
]
})
+ services.push({
+ name: 'Firewall',
+ provider: [
+ { name: 'VpcVirtualRouter' }
+ ]
+ })
Review 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.
--
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]
