rootnomad commented on code in PR #14165:
URL: https://github.com/apache/cloudstack/pull/14165#discussion_r4284707784
##########
plugins/integrations/kubernetes-service/src/main/java/com/cloud/kubernetes/cluster/actionworkers/KubernetesClusterResourceModifierActionWorker.java:
##########
@@ -544,16 +544,23 @@ protected FirewallRule removeApiFirewallRule(final
IpAddress publicIp) {
}
protected FirewallRule removeSshFirewallRule(final IpAddress publicIp,
final long networkId) {
- FirewallRule rule = null;
+ FirewallRuleVO rule = null;
List<FirewallRuleVO> firewallRules =
firewallRulesDao.listByIpPurposeProtocolAndNotRevoked(publicIp.getId(),
FirewallRule.Purpose.Firewall, NetUtils.TCP_PROTO);
for (FirewallRuleVO firewallRule : firewallRules) {
- PortForwardingRuleVO pfRule =
portForwardingRulesDao.findByNetworkAndPorts(networkId,
firewallRule.getSourcePortStart(), firewallRule.getSourcePortEnd());
- if (Objects.equals(firewallRule.getSourcePortStart(),
CLUSTER_NODES_DEFAULT_START_SSH_PORT) || (Objects.nonNull(pfRule) &&
pfRule.getDestinationPortStart() == DEFAULT_SSH_PORT) ) {
+ if (Objects.equals(firewallRule.getSourcePortStart(),
CLUSTER_NODES_DEFAULT_START_SSH_PORT)) {
rule = firewallRule;
- firewallService.revokeIngressFwRule(firewallRule.getId(),
true);
- logger.debug("The SSH firewall rule {} with the id {} was
revoked", firewallRule.getName(), firewallRule.getId());
break;
}
+ if (rule == null) {
+ PortForwardingRuleVO pfRule =
portForwardingRulesDao.findByNetworkAndPorts(networkId,
firewallRule.getSourcePortStart(), firewallRule.getSourcePortEnd());
+ if (Objects.nonNull(pfRule) &&
pfRule.getDestinationPortStart() == DEFAULT_SSH_PORT) {
Review Comment:
Good catch. You are right that with the 2222 range rule gone, the fallback
matches the first rule forwarding to 22, and the etcd rules at 50000+ do, so it
could grab one and the scale path would then wipe 2222..50000. Since the caller
already bounds its cleanup safely on a null return, I capped the fallback below
KubernetesEtcdNodeStartPort so it never crosses into the etcd range, with a
test for the no-node-rule case. Pushed in 75edb36.
##########
plugins/integrations/kubernetes-service/src/main/java/com/cloud/kubernetes/cluster/actionworkers/KubernetesClusterResourceModifierActionWorker.java:
##########
@@ -544,16 +544,23 @@ protected FirewallRule removeApiFirewallRule(final
IpAddress publicIp) {
}
protected FirewallRule removeSshFirewallRule(final IpAddress publicIp,
final long networkId) {
- FirewallRule rule = null;
+ FirewallRuleVO rule = null;
List<FirewallRuleVO> firewallRules =
firewallRulesDao.listByIpPurposeProtocolAndNotRevoked(publicIp.getId(),
FirewallRule.Purpose.Firewall, NetUtils.TCP_PROTO);
for (FirewallRuleVO firewallRule : firewallRules) {
- PortForwardingRuleVO pfRule =
portForwardingRulesDao.findByNetworkAndPorts(networkId,
firewallRule.getSourcePortStart(), firewallRule.getSourcePortEnd());
- if (Objects.equals(firewallRule.getSourcePortStart(),
CLUSTER_NODES_DEFAULT_START_SSH_PORT) || (Objects.nonNull(pfRule) &&
pfRule.getDestinationPortStart() == DEFAULT_SSH_PORT) ) {
+ if (Objects.equals(firewallRule.getSourcePortStart(),
CLUSTER_NODES_DEFAULT_START_SSH_PORT)) {
rule = firewallRule;
- firewallService.revokeIngressFwRule(firewallRule.getId(),
true);
- logger.debug("The SSH firewall rule {} with the id {} was
revoked", firewallRule.getName(), firewallRule.getId());
break;
}
+ if (rule == null) {
+ PortForwardingRuleVO pfRule =
portForwardingRulesDao.findByNetworkAndPorts(networkId,
firewallRule.getSourcePortStart(), firewallRule.getSourcePortEnd());
+ if (Objects.nonNull(pfRule) &&
pfRule.getDestinationPortStart() == DEFAULT_SSH_PORT) {
+ rule = firewallRule;
+ }
+ }
+ }
+ if (rule != null) {
Review Comment:
Agreed, the etcd firewall rules at 50000+ were not being revoked on destroy
and would linger in a network the user keeps, clashing with the next etcd
cluster on those ports. Fixed in the same push: the destroy path now revokes
the etcd ingress rules alongside the node and API rules, with a test. 75edb36.
--
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]
