Skip to content

Commit 2db582d

Browse files
GaOrtigaGabrielwinterhazel
authored and
dhslove
committed
Fix NPE during kubernetes cluster creation when network has rules with ports saved as null on DB (apache#9223)
Co-authored-by: Gabriel <[email protected]> Co-authored-by: Fabricio Duarte <[email protected]>
1 parent c1211bf commit 2db582d

File tree

6 files changed

+41
-17
lines changed

6 files changed

+41
-17
lines changed

‎engine/schema/src/main/java/com/cloud/network/dao/FirewallRulesDao.java‎

Lines changed: 3 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -43,7 +43,9 @@ public interface FirewallRulesDao extends GenericDao<FirewallRuleVO, Long> {
4343

4444
List<FirewallRuleVO> listStaticNatByVmId(long vmId);
4545

46-
List<FirewallRuleVO> listByIpPurposeAndProtocolAndNotRevoked(long ipAddressId, Integer startPort, Integer endPort, String protocol, FirewallRule.Purpose purpose);
46+
List<FirewallRuleVO> listByIpPurposePortsProtocolAndNotRevoked(long ipAddressId, Integer startPort, Integer endPort, String protocol, FirewallRule.Purpose purpose);
47+
48+
List<FirewallRuleVO> listByIpPurposeProtocolAndNotRevoked(long ipAddressId, FirewallRule.Purpose purpose, String protocol);
4749

4850
FirewallRuleVO findByRelatedId(long ruleId);
4951

‎engine/schema/src/main/java/com/cloud/network/dao/FirewallRulesDaoImpl.java‎

Lines changed: 19 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -263,8 +263,25 @@ public void saveDestinationCidrs(FirewallRuleVO firewallRule, List<String> cidrL
263263
}
264264

265265
@Override
266-
public List<FirewallRuleVO> listByIpPurposeAndProtocolAndNotRevoked(long ipAddressId, Integer startPort, Integer endPort, String protocol,
267-
FirewallRule.Purpose purpose) {
266+
public List<FirewallRuleVO> listByIpPurposeProtocolAndNotRevoked(long ipAddressId, Purpose purpose, String protocol) {
267+
SearchCriteria<FirewallRuleVO> sc = NotRevokedSearch.create();
268+
sc.setParameters("ipId", ipAddressId);
269+
sc.setParameters("state", State.Revoke);
270+
271+
if (purpose != null) {
272+
sc.setParameters("purpose", purpose);
273+
}
274+
275+
if (protocol != null) {
276+
sc.setParameters("protocol", protocol);
277+
}
278+
279+
return listBy(sc);
280+
}
281+
282+
@Override
283+
public List<FirewallRuleVO> listByIpPurposePortsProtocolAndNotRevoked(long ipAddressId, Integer startPort, Integer endPort, String protocol,
284+
FirewallRule.Purpose purpose) {
268285
SearchCriteria<FirewallRuleVO> sc = NotRevokedSearch.create();
269286
sc.setParameters("ipId", ipAddressId);
270287
sc.setParameters("state", State.Revoke);

‎plugins/integrations/kubernetes-service/src/main/java/com/cloud/kubernetes/cluster/KubernetesClusterManagerImpl.java‎

Lines changed: 4 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -74,6 +74,7 @@
7474
import org.apache.cloudstack.network.RoutedIpv4Manager;
7575
import org.apache.commons.codec.binary.Base64;
7676
import org.apache.commons.collections.CollectionUtils;
77+
import org.apache.commons.lang3.ObjectUtils;
7778
import org.apache.commons.lang3.StringUtils;
7879

7980
import com.cloud.api.ApiDBUtils;
@@ -382,10 +383,10 @@ public VMTemplateVO getKubernetesServiceTemplate(DataCenter dataCenter, Hypervis
382383
}
383384

384385
protected void validateIsolatedNetworkIpRules(long ipId, FirewallRule.Purpose purpose, Network network, int clusterTotalNodeCount) {
385-
List<FirewallRuleVO> rules = firewallRulesDao.listByIpAndPurposeAndNotRevoked(ipId, purpose);
386+
List<FirewallRuleVO> rules = firewallRulesDao.listByIpPurposeProtocolAndNotRevoked(ipId, purpose, NetUtils.TCP_PROTO);
386387
for (FirewallRuleVO rule : rules) {
387-
Integer startPort = rule.getSourcePortStart();
388-
Integer endPort = rule.getSourcePortEnd();
388+
int startPort = ObjectUtils.defaultIfNull(rule.getSourcePortStart(), 1);
389+
int endPort = ObjectUtils.defaultIfNull(rule.getSourcePortEnd(), NetUtils.PORT_RANGE_MAX);
389390
if (logger.isDebugEnabled()) {
390391
logger.debug("Validating rule with purpose: {} for network: {} with ports: {}-{}", purpose.toString(), network, startPort, endPort);
391392
}

‎plugins/integrations/kubernetes-service/src/main/java/com/cloud/kubernetes/cluster/actionworkers/KubernetesClusterResourceModifierActionWorker.java‎

Lines changed: 8 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -498,10 +498,12 @@ protected void provisionSshPortForwardingRules(IpAddress publicIp, Network netwo
498498

499499
protected FirewallRule removeApiFirewallRule(final IpAddress publicIp) {
500500
FirewallRule rule = null;
501-
List<FirewallRuleVO> firewallRules = firewallRulesDao.listByIpAndPurposeAndNotRevoked(publicIp.getId(), FirewallRule.Purpose.Firewall);
501+
List<FirewallRuleVO> firewallRules = firewallRulesDao.listByIpPurposeProtocolAndNotRevoked(publicIp.getId(), FirewallRule.Purpose.Firewall, NetUtils.TCP_PROTO);
502502
for (FirewallRuleVO firewallRule : firewallRules) {
503-
if (firewallRule.getSourcePortStart() == CLUSTER_API_PORT &&
504-
firewallRule.getSourcePortEnd() == CLUSTER_API_PORT) {
503+
Integer startPort = firewallRule.getSourcePortStart();
504+
Integer endPort = firewallRule.getSourcePortEnd();
505+
if (startPort != null && startPort == CLUSTER_API_PORT &&
506+
endPort != null && endPort == CLUSTER_API_PORT) {
505507
rule = firewallRule;
506508
firewallService.revokeIngressFwRule(firewallRule.getId(), true);
507509
logger.debug("The API firewall rule [%s] with the id [%s] was revoked",firewallRule.getName(),firewallRule.getId());
@@ -513,9 +515,10 @@ protected FirewallRule removeApiFirewallRule(final IpAddress publicIp) {
513515

514516
protected FirewallRule removeSshFirewallRule(final IpAddress publicIp) {
515517
FirewallRule rule = null;
516-
List<FirewallRuleVO> firewallRules = firewallRulesDao.listByIpAndPurposeAndNotRevoked(publicIp.getId(), FirewallRule.Purpose.Firewall);
518+
List<FirewallRuleVO> firewallRules = firewallRulesDao.listByIpPurposeProtocolAndNotRevoked(publicIp.getId(), FirewallRule.Purpose.Firewall, NetUtils.TCP_PROTO);
517519
for (FirewallRuleVO firewallRule : firewallRules) {
518-
if (firewallRule.getSourcePortStart() == CLUSTER_NODES_DEFAULT_START_SSH_PORT) {
520+
Integer startPort = firewallRule.getSourcePortStart();
521+
if (startPort != null && startPort == CLUSTER_NODES_DEFAULT_START_SSH_PORT) {
519522
rule = firewallRule;
520523
firewallService.revokeIngressFwRule(firewallRule.getId(), true);
521524
logger.debug("The SSH firewall rule [%s] with the id [%s] was revoked",firewallRule.getName(),firewallRule.getId());

‎plugins/integrations/kubernetes-service/src/test/java/com/cloud/kubernetes/cluster/KubernetesClusterManagerImplTest.java‎

Lines changed: 6 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -37,6 +37,7 @@
3737
import com.cloud.user.Account;
3838
import com.cloud.user.AccountManager;
3939
import com.cloud.user.User;
40+
import com.cloud.utils.net.NetUtils;
4041
import com.cloud.vm.VMInstanceVO;
4142
import com.cloud.vm.dao.VMInstanceDao;
4243
import org.apache.cloudstack.api.BaseCmd;
@@ -117,7 +118,7 @@ public void validateIsolatedNetworkIpRulesNoRules() {
117118
long ipId = 1L;
118119
FirewallRule.Purpose purpose = FirewallRule.Purpose.Firewall;
119120
Network network = Mockito.mock(Network.class);
120-
Mockito.when(firewallRulesDao.listByIpAndPurposeAndNotRevoked(ipId, purpose)).thenReturn(new ArrayList<>());
121+
Mockito.when(firewallRulesDao.listByIpPurposeProtocolAndNotRevoked(ipId, purpose, NetUtils.TCP_PROTO)).thenReturn(new ArrayList<>());
121122
kubernetesClusterManager.validateIsolatedNetworkIpRules(ipId, FirewallRule.Purpose.Firewall, network, 3);
122123
}
123124

@@ -131,7 +132,7 @@ public void validateIsolatedNetworkIpRulesNoConflictingRules() {
131132
long ipId = 1L;
132133
FirewallRule.Purpose purpose = FirewallRule.Purpose.Firewall;
133134
Network network = Mockito.mock(Network.class);
134-
Mockito.when(firewallRulesDao.listByIpAndPurposeAndNotRevoked(ipId, purpose)).thenReturn(List.of(createRule(80, 80), createRule(443, 443)));
135+
Mockito.when(firewallRulesDao.listByIpPurposeProtocolAndNotRevoked(ipId, purpose, NetUtils.TCP_PROTO)).thenReturn(List.of(createRule(80, 80), createRule(443, 443)));
135136
kubernetesClusterManager.validateIsolatedNetworkIpRules(ipId, FirewallRule.Purpose.Firewall, network, 3);
136137
}
137138

@@ -140,7 +141,7 @@ public void validateIsolatedNetworkIpRulesApiConflictingRules() {
140141
long ipId = 1L;
141142
FirewallRule.Purpose purpose = FirewallRule.Purpose.Firewall;
142143
Network network = Mockito.mock(Network.class);
143-
Mockito.when(firewallRulesDao.listByIpAndPurposeAndNotRevoked(ipId, purpose)).thenReturn(List.of(createRule(6440, 6445), createRule(443, 443)));
144+
Mockito.when(firewallRulesDao.listByIpPurposeProtocolAndNotRevoked(ipId, purpose, NetUtils.TCP_PROTO)).thenReturn(List.of(createRule(6440, 6445), createRule(443, 443)));
144145
kubernetesClusterManager.validateIsolatedNetworkIpRules(ipId, FirewallRule.Purpose.Firewall, network, 3);
145146
}
146147

@@ -149,7 +150,7 @@ public void validateIsolatedNetworkIpRulesSshConflictingRules() {
149150
long ipId = 1L;
150151
FirewallRule.Purpose purpose = FirewallRule.Purpose.Firewall;
151152
Network network = Mockito.mock(Network.class);
152-
Mockito.when(firewallRulesDao.listByIpAndPurposeAndNotRevoked(ipId, purpose)).thenReturn(List.of(createRule(2200, KubernetesClusterActionWorker.CLUSTER_NODES_DEFAULT_START_SSH_PORT), createRule(443, 443)));
153+
Mockito.when(firewallRulesDao.listByIpPurposeProtocolAndNotRevoked(ipId, purpose, NetUtils.TCP_PROTO)).thenReturn(List.of(createRule(2200, KubernetesClusterActionWorker.CLUSTER_NODES_DEFAULT_START_SSH_PORT), createRule(443, 443)));
153154
kubernetesClusterManager.validateIsolatedNetworkIpRules(ipId, FirewallRule.Purpose.Firewall, network, 3);
154155
}
155156

@@ -158,7 +159,7 @@ public void validateIsolatedNetworkIpRulesNearConflictingRules() {
158159
long ipId = 1L;
159160
FirewallRule.Purpose purpose = FirewallRule.Purpose.Firewall;
160161
Network network = Mockito.mock(Network.class);
161-
Mockito.when(firewallRulesDao.listByIpAndPurposeAndNotRevoked(ipId, purpose)).thenReturn(List.of(createRule(2220, 2221), createRule(2225, 2227), createRule(6440, 6442), createRule(6444, 6446)));
162+
Mockito.when(firewallRulesDao.listByIpPurposeProtocolAndNotRevoked(ipId, purpose, NetUtils.TCP_PROTO)).thenReturn(List.of(createRule(2220, 2221), createRule(2225, 2227), createRule(6440, 6442), createRule(6444, 6446)));
162163
kubernetesClusterManager.validateIsolatedNetworkIpRules(ipId, FirewallRule.Purpose.Firewall, network, 3);
163164
}
164165

‎server/src/main/java/com/cloud/network/firewall/FirewallManagerImpl.java‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1009,7 +1009,7 @@ public FirewallRule createRuleForAllCidrs(long ipAddrId, Account caller, Integer
10091009
Long relatedRuleId, long networkId) throws NetworkRuleConflictException {
10101010

10111011
// If firwallRule for this port range already exists, return it
1012-
List<FirewallRuleVO> rules = _firewallDao.listByIpPurposeAndProtocolAndNotRevoked(ipAddrId, startPort, endPort, protocol, Purpose.Firewall);
1012+
List<FirewallRuleVO> rules = _firewallDao.listByIpPurposePortsProtocolAndNotRevoked(ipAddrId, startPort, endPort, protocol, Purpose.Firewall);
10131013
if (!rules.isEmpty()) {
10141014
return rules.get(0);
10151015
}

0 commit comments

Comments
 (0)