Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Fix NPE during kubernetes cluster creation when network has rules with ports saved as null on DB #9223

Open
wants to merge 5 commits into
base: main
Choose a base branch
from
Open

Fix NPE during kubernetes cluster creation when network has rules with ports saved as null on DB #9223

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
328cc95
fix NPE during kubernetes cluster creation
Jun 7, 2024
b8470ec
fix delete
Jun 12, 2024
738c53b
skip other protocols
Jun 12, 2024
921190d
use NetUtils.TCP_PROTO instead
Jun 12, 2024
b3be45e
fix tests
Jun 13, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 3 additions & 1 deletion engine/schema/src/main/java/com/cloud/network/dao/FirewallRulesDao.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -43,7 +43,9 @@ public interface FirewallRulesDao extends GenericDao<FirewallRuleVO, Long> {

List<FirewallRuleVO> listStaticNatByVmId(long vmId);

List<FirewallRuleVO> listByIpPurposeAndProtocolAndNotRevoked(long ipAddressId, Integer startPort, Integer endPort, String protocol, FirewallRule.Purpose purpose);
List<FirewallRuleVO> listByIpPurposePortsProtocolAndNotRevoked(long ipAddressId, Integer startPort, Integer endPort, String protocol, FirewallRule.Purpose purpose);

List<FirewallRuleVO> listByIpPurposeProtocolAndNotRevoked(long ipAddressId, FirewallRule.Purpose purpose, String protocol);

FirewallRuleVO findByRelatedId(long ruleId);

Expand Down
21 changes: 19 additions & 2 deletions engine/schema/src/main/java/com/cloud/network/dao/FirewallRulesDaoImpl.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -266,8 +266,25 @@
}

@Override
public List<FirewallRuleVO> listByIpPurposeAndProtocolAndNotRevoked(long ipAddressId, Integer startPort, Integer endPort, String protocol,
FirewallRule.Purpose purpose) {
public List<FirewallRuleVO> listByIpPurposeProtocolAndNotRevoked(long ipAddressId, Purpose purpose, String protocol) {
SearchCriteria<FirewallRuleVO> sc = NotRevokedSearch.create();
sc.setParameters("ipId", ipAddressId);
sc.setParameters("state", State.Revoke);

Check warning on line 272 in engine/schema/src/main/java/com/cloud/network/dao/FirewallRulesDaoImpl.java

View check run for this annotation

Codecov / codecov/patch

engine/schema/src/main/java/com/cloud/network/dao/FirewallRulesDaoImpl.java#L269-L272 

Added lines #L269 - L272 were not covered by tests

if (purpose != null) {
sc.setParameters("purpose", purpose);

Check warning on line 275 in engine/schema/src/main/java/com/cloud/network/dao/FirewallRulesDaoImpl.java

View check run for this annotation

Codecov / codecov/patch

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

Added line #L275 was not covered by tests
}

if (protocol != null) {
sc.setParameters("protocol", protocol);

Check warning on line 279 in engine/schema/src/main/java/com/cloud/network/dao/FirewallRulesDaoImpl.java

View check run for this annotation

Codecov / codecov/patch

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

Added line #L279 was not covered by tests
}

return listBy(sc);
}

Check warning on line 283 in engine/schema/src/main/java/com/cloud/network/dao/FirewallRulesDaoImpl.java

View check run for this annotation

Codecov / codecov/patch

engine/schema/src/main/java/com/cloud/network/dao/FirewallRulesDaoImpl.java#L282-L283 

Added lines #L282 - L283 were not covered by tests

@Override
public List<FirewallRuleVO> listByIpPurposePortsProtocolAndNotRevoked(long ipAddressId, Integer startPort, Integer endPort, String protocol,
FirewallRule.Purpose purpose) {

Check warning on line 287 in engine/schema/src/main/java/com/cloud/network/dao/FirewallRulesDaoImpl.java

View check run for this annotation

Codecov / codecov/patch

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

Added line #L287 was not covered by tests
SearchCriteria<FirewallRuleVO> sc = NotRevokedSearch.create();
sc.setParameters("ipId", ipAddressId);
sc.setParameters("state", State.Revoke);
Expand Down
7 changes: 4 additions & 3 deletions ...etes-service/src/main/java/com/cloud/kubernetes/cluster/KubernetesClusterManagerImpl.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -74,6 +74,7 @@
import org.apache.cloudstack.network.RoutedIpv4Manager;
import org.apache.commons.codec.binary.Base64;
import org.apache.commons.collections.CollectionUtils;
import org.apache.commons.lang3.ObjectUtils;
import org.apache.commons.lang3.StringUtils;

import com.cloud.api.ApiDBUtils;
Expand Down Expand Up @@ -382,10 +383,10 @@ public VMTemplateVO getKubernetesServiceTemplate(DataCenter dataCenter, Hypervis
}

protected void validateIsolatedNetworkIpRules(long ipId, FirewallRule.Purpose purpose, Network network, int clusterTotalNodeCount) {
List<FirewallRuleVO> rules = firewallRulesDao.listByIpAndPurposeAndNotRevoked(ipId, purpose);
List<FirewallRuleVO> rules = firewallRulesDao.listByIpPurposeProtocolAndNotRevoked(ipId, purpose, NetUtils.TCP_PROTO);
for (FirewallRuleVO rule : rules) {
Integer startPort = rule.getSourcePortStart();
Integer endPort = rule.getSourcePortEnd();
int startPort = ObjectUtils.defaultIfNull(rule.getSourcePortStart(), 1);
int endPort = ObjectUtils.defaultIfNull(rule.getSourcePortEnd(), KubernetesClusterActionWorker.MAX_PORT);
weizhouapache marked this conversation as resolved.
Show resolved Hide resolved
if (logger.isDebugEnabled()) {
logger.debug(String.format("Validating rule with purpose: %s for network: %s with ports: %d-%d", purpose.toString(), network.getUuid(), startPort, endPort));
}
Expand Down
1 change: 1 addition & 0 deletions ...c/main/java/com/cloud/kubernetes/cluster/actionworkers/KubernetesClusterActionWorker.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -106,6 +106,7 @@ public class KubernetesClusterActionWorker {
public static final int DEFAULT_SSH_PORT = 22;
public static final int CLUSTER_NODES_DEFAULT_START_SSH_PORT = 2222;
public static final int CLUSTER_NODES_DEFAULT_SSH_PORT_SG = DEFAULT_SSH_PORT;
public static final int MAX_PORT = 65535;
DaanHoogland marked this conversation as resolved.
Show resolved Hide resolved

public static final String CKS_CLUSTER_SECURITY_GROUP_NAME = "CKSSecurityGroup";
public static final String CKS_SECURITY_GROUP_DESCRIPTION = "Security group for CKS nodes";
Expand Down
13 changes: 8 additions & 5 deletions ...cloud/kubernetes/cluster/actionworkers/KubernetesClusterResourceModifierActionWorker.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -498,10 +498,12 @@

protected FirewallRule removeApiFirewallRule(final IpAddress publicIp) {
FirewallRule rule = null;
List<FirewallRuleVO> firewallRules = firewallRulesDao.listByIpAndPurposeAndNotRevoked(publicIp.getId(), FirewallRule.Purpose.Firewall);
List<FirewallRuleVO> firewallRules = firewallRulesDao.listByIpPurposeProtocolAndNotRevoked(publicIp.getId(), FirewallRule.Purpose.Firewall, NetUtils.TCP_PROTO);

Check warning on line 501 in plugins/integrations/kubernetes-service/src/main/java/com/cloud/kubernetes/cluster/actionworkers/KubernetesClusterResourceModifierActionWorker.java

View check run for this annotation

Codecov / codecov/patch

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

Added line #L501 was not covered by tests
for (FirewallRuleVO firewallRule : firewallRules) {
if (firewallRule.getSourcePortStart() == CLUSTER_API_PORT &&
firewallRule.getSourcePortEnd() == CLUSTER_API_PORT) {
Integer startPort = firewallRule.getSourcePortStart();
Integer endPort = firewallRule.getSourcePortEnd();

Check warning on line 504 in plugins/integrations/kubernetes-service/src/main/java/com/cloud/kubernetes/cluster/actionworkers/KubernetesClusterResourceModifierActionWorker.java

View check run for this annotation

Codecov / codecov/patch

plugins/integrations/kubernetes-service/src/main/java/com/cloud/kubernetes/cluster/actionworkers/KubernetesClusterResourceModifierActionWorker.java#L503-L504 

Added lines #L503 - L504 were not covered by tests
if (startPort != null && startPort == CLUSTER_API_PORT &&
endPort != null && endPort == CLUSTER_API_PORT) {
rule = firewallRule;
firewallService.revokeIngressFwRule(firewallRule.getId(), true);
logger.debug("The API firewall rule [%s] with the id [%s] was revoked",firewallRule.getName(),firewallRule.getId());
Expand All @@ -513,9 +515,10 @@

protected FirewallRule removeSshFirewallRule(final IpAddress publicIp) {
FirewallRule rule = null;
List<FirewallRuleVO> firewallRules = firewallRulesDao.listByIpAndPurposeAndNotRevoked(publicIp.getId(), FirewallRule.Purpose.Firewall);
List<FirewallRuleVO> firewallRules = firewallRulesDao.listByIpPurposeProtocolAndNotRevoked(publicIp.getId(), FirewallRule.Purpose.Firewall, NetUtils.TCP_PROTO);

Check warning on line 518 in plugins/integrations/kubernetes-service/src/main/java/com/cloud/kubernetes/cluster/actionworkers/KubernetesClusterResourceModifierActionWorker.java

View check run for this annotation

Codecov / codecov/patch

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

Added line #L518 was not covered by tests
for (FirewallRuleVO firewallRule : firewallRules) {
if (firewallRule.getSourcePortStart() == CLUSTER_NODES_DEFAULT_START_SSH_PORT) {
Integer startPort = firewallRule.getSourcePortStart();

Check warning on line 520 in plugins/integrations/kubernetes-service/src/main/java/com/cloud/kubernetes/cluster/actionworkers/KubernetesClusterResourceModifierActionWorker.java

View check run for this annotation

Codecov / codecov/patch

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

Added line #L520 was not covered by tests
if (startPort != null && startPort == CLUSTER_NODES_DEFAULT_START_SSH_PORT) {
rule = firewallRule;
firewallService.revokeIngressFwRule(firewallRule.getId(), true);
logger.debug("The SSH firewall rule [%s] with the id [%s] was revoked",firewallRule.getName(),firewallRule.getId());
Expand Down
11 changes: 6 additions & 5 deletions ...-service/src/test/java/com/cloud/kubernetes/cluster/KubernetesClusterManagerImplTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -37,6 +37,7 @@
import com.cloud.user.Account;
import com.cloud.user.AccountManager;
import com.cloud.user.User;
import com.cloud.utils.net.NetUtils;
import com.cloud.vm.VMInstanceVO;
import com.cloud.vm.dao.VMInstanceDao;
import org.apache.cloudstack.api.BaseCmd;
Expand Down Expand Up @@ -117,7 +118,7 @@ public void validateIsolatedNetworkIpRulesNoRules() {
long ipId = 1L;
FirewallRule.Purpose purpose = FirewallRule.Purpose.Firewall;
Network network = Mockito.mock(Network.class);
Mockito.when(firewallRulesDao.listByIpAndPurposeAndNotRevoked(ipId, purpose)).thenReturn(new ArrayList<>());
Mockito.when(firewallRulesDao.listByIpPurposeProtocolAndNotRevoked(ipId, purpose, NetUtils.TCP_PROTO)).thenReturn(new ArrayList<>());
kubernetesClusterManager.validateIsolatedNetworkIpRules(ipId, FirewallRule.Purpose.Firewall, network, 3);
}

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

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

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

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

Expand Down
2 changes: 1 addition & 1 deletion server/src/main/java/com/cloud/network/firewall/FirewallManagerImpl.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -968,7 +968,7 @@
Long relatedRuleId, long networkId) throws NetworkRuleConflictException {

// If firwallRule for this port range already exists, return it
List<FirewallRuleVO> rules = _firewallDao.listByIpPurposeAndProtocolAndNotRevoked(ipAddrId, startPort, endPort, protocol, Purpose.Firewall);
List<FirewallRuleVO> rules = _firewallDao.listByIpPurposePortsProtocolAndNotRevoked(ipAddrId, startPort, endPort, protocol, Purpose.Firewall);

Check warning on line 971 in server/src/main/java/com/cloud/network/firewall/FirewallManagerImpl.java

View check run for this annotation

Codecov / codecov/patch

server/src/main/java/com/cloud/network/firewall/FirewallManagerImpl.java#L971 

Added line #L971 was not covered by tests
if (!rules.isEmpty()) {
return rules.get(0);
}
Expand Down
Loading