adestis-mf opened a new pull request, #14683:
URL: https://github.com/apache/cloudstack/pull/14683
### Description
This PR fixes scaling a CKS cluster on an isolated network that has external
nodes (VMs added with `addNodesToKubernetesCluster`).
When the cluster is scaled, `scaleKubernetesClusterIsolatedNetworkRules`
rebuilds the SSH rules on the source NAT IP: it revokes the firewall rule of
the SSH port range (`removeSshFirewallRule`), removes the port forwarding rules
in that range and calls `setupKubernetesClusterIsolatedNetworkRules`. That
creates the range for the control and worker nodes that CKS deploys, 2222 to
2222+n-1 for n such nodes, and after it a single-port firewall rule and a port
forwarding rule for each external node. The existing rules of the external
nodes were not removed first:
- Scale-up: the new range overlaps the firewall rule of the first external
node. `FirewallManagerImpl.detectRulesConflict` rejects it, because the ports
overlap and protocol and source CIDR (`0.0.0.0/0`) are the same, and the scale
fails with "unable to update network rules", caused by "Failed to provision
firewall rules for SSH access" (#12633). By then the SSH firewall rule and the
SSH port forwarding rules of the other nodes are removed, and they are not
recreated.
- Scale-down: the external node gets new rules on its new port, and its old
firewall and port forwarding rules stay on the old port.
Changes:
- `removeExternalNodeSshRules` runs before the rules are rebuilt. For each
external node it revokes the port forwarding rules on the cluster's public IP
from a port of the node SSH port range (2222 up to the SSH ports of the etcd
nodes, `cloud.kubernetes.etcd.node.start.port`) to port 22, each together with
the firewall rule for that port. The existing code then recreates them after
the new range. The firewall rule is revoked first, so that if a revoke fails, a
retry still finds it through the port forwarding rule. Both failures are
reported as `ManagementServerException`, which the scale worker already handles.
- On scale-down the node VMs are now passed to the rebuild in the same order
as on scale-up: the nodes deployed by CKS first, then the external nodes
(`getNodeVmIdsForSshRules`). That is the order in which
`addFirewallRulesForNodes` and `provisionSshPortForwardingRules` assign the SSH
ports. Until now scale-down passed the VM map order. Once a scale-up has added
a node after an external node, and a node before the external node in the VM
map is then removed (by `nodeids`), that order put the external node before the
remaining new node, and the port forwarding rules no longer matched the
firewall rules. A later scale then failed on the same conflict.
As a result the SSH port of an external node moves with the range on every
scale, as the existing code intends, and its old port is closed.
Clusters without external nodes get the same rules as before. The VPC tier
path is only affected by the order of the node VMs on scale-down: with external
nodes, their port forwarding rules now get the ports after the other nodes, as
on scale-up. Shared networks are not affected.
Fixes: #12633
#14673 describes the same problem and is a duplicate of #12633.
This targets `4.22`, where the bug exists (#12633 is on the 4.22.3
milestone). The commit merges forward to `main` without conflicts.
Scaling with `nodeids` that include an external node, which is what the node
list in the UI does (#12641), failed with the same error: the range stays the
same, and the recreated rule of the external node conflicts with its existing
rule. Scaling never removes external nodes (`scaleDownKubernetesClusterSize`
filters them out), but the node count was still lowered by the number of
`nodeids`. With the conflict fixed, such a request would succeed without
removing anything, so `scaleKubernetesCluster` now rejects external nodes in
`nodeids` up front (`validateKubernetesClusterScaleNodesToRemove`); they are
removed with `removeNodesFromKubernetesCluster`. The UI still offers the delete
action for external nodes in the node list, which now gets this error; changing
the UI to use `removeNodesFromKubernetesCluster` there (#12641) is left for a
separate change.
#14670 (open, against `main`) adds the same SSH firewall rules on VPC tiers
whose VPC has the Firewall service and rebuilds them the same way on scale.
Once this change is merged forward, I'll call the new method in that path as
well.
### Types of changes
- [ ] Breaking change (fix or feature that would cause existing
functionality to change)
- [ ] New feature (non-breaking change which adds functionality)
- [x] Bug fix (non-breaking change which fixes an issue)
- [ ] Enhancement (improves an existing feature and functionality)
- [ ] Cleanup (Code refactoring and cleanup, that may add test cases)
- [ ] Build/CI
- [ ] Test (unit or integration test code)
### Feature/Enhancement Scale or Bug Severity
#### Feature/Enhancement Scale
- [ ] Major
- [ ] Minor
#### Bug Severity
- [ ] BLOCKER
- [ ] Critical
- [x] Major
- [ ] Minor
- [ ] Trivial
### Screenshots (if appropriate):
### How Has This Been Tested?
New unit tests in `KubernetesClusterScaleWorkerTest` and
`KubernetesClusterManagerImplTest`. Three of them run
`scaleKubernetesClusterIsolatedNetworkRules` with the actual
`removeSshFirewallRule`, `removePortForwardingRules`,
`addFirewallRulesForNodes` and `provisionSshPortForwardingRules`. The rules are
kept in lists; a new firewall rule that overlaps an existing one is rejected,
which models what `detectRulesConflict` does for these rules.
- Scale-up with two external nodes and an etcd node: 2222-2224 for the nodes
deployed by CKS, 2225 and 2226 for the external nodes, each port forwarded to
its node; the etcd rule is not touched. The rules of the external nodes are
listed before the range rule, so `removeSshFirewallRule` would pick one of them
if they were not removed first.
- Scale-up, then scale-down removing a worker that comes before the external
node in the VM map, then scale-up again: after each step one rule per port and
each port forwarded to the right node, no rule left on an old port.
- Scale-up without external nodes: the same rules as before, and no port
forwarding rule is looked up or revoked by VM.
- `getNodeVmIdsForSshRules`: the external nodes come last, etcd nodes are
left out.
- `removeExternalNodeSshRules` revokes an old SSH rule of an external node
as well, and keeps its other rules: to another private port, from a port below
2222 or in the etcd range, and on another public IP.
- A failed port forwarding revoke is reported as
`ManagementServerException`, and a second call removes what is left. A firewall
revoke that returns false is reported, and the port forwarding rule stays.
- `KubernetesClusterManagerImplTest`:
`validateKubernetesClusterScaleNodesToRemove` accepts nodes deployed by CKS and
rejects a list with an external node.
Without the new call in `scaleKubernetesClusterIsolatedNetworkRules`, the
two scale tests with external nodes fail with "Failed to provision firewall
rules for SSH access": the new range 2222-2224 overlaps the old rule of an
external node on 2224. When the scale-down passes the node VMs in VM map order,
as before, it gives the external node port 2223 and the worker port 2224, while
the single-port firewall rule on 2224 is the one created for the external node;
the following scale-up then fails with the same error. If the external node
rules are removed after `removeSshFirewallRule` instead of before, the scale-up
test fails as well.
```
mvn -pl plugins/integrations/kubernetes-service -am test
-Dtest='**/kubernetes/**/*Test' \
-Dsurefire.failIfNoSpecifiedTests=false -DfailIfNoTests=false
```
Result with this change on top of `4.22` (b56bbc2293), JDK 11 / Maven 3.10:
BUILD SUCCESS, 87 tests in the kubernetes-service module, 0 failures, 0 errors;
checkstyle 0 violations. The commit also applies on top of `main` (1881dc6654):
137 tests, 0 failures, 0 errors; checkstyle 0 violations.
Live, on 4.23.0.0 without this change (KVM, isolated network created by CKS,
v1.35.4, 1 control node, 1 worker and an external node from the CKS-ready
Ubuntu 22.04 template; the code paths involved are the same as on `4.22`):
- Scale-up to `size=3` failed with "unable to update network rules": the new
range 2222-2224 conflicted with the rule of the external node on 2224.
Afterwards only the rules of the external node and 6443 were left, and the
cluster was back in `Running`.
- Doing by hand what this change does, deleting the firewall and port
forwarding rule of the external node, and scaling up again gave 2222-2224 for
the control and worker nodes and 2225 for the external node. Each port reached
the expected node, and all nodes were `Ready`.
- Removing the first worker by `nodeids` then forwarded 2223 to the external
node and 2224 to the new worker, and left the old rules of the external node on
2225. The next scale-up failed again: `removeSshFirewallRule` revoked the old
rule of the external node on 2225 instead of the range rule, and the new range
conflicted with the old one.
The patched build itself has not been deployed to a live management server.
#### How did you try to break this feature and the system with this change?
- Cluster without external nodes: nothing is revoked by the new method, and
the rules are rebuilt as before.
- Only rules that match what CKS creates for an external node are revoked: a
port forwarding rule of the external node on the cluster's public IP from a
port of the node SSH port range to port 22, and the firewall rule for exactly
that port. `KubernetesClusterRemoveWorker` finds the SSH port of a node in a
similar way. A rule that a user created with exactly this pattern for an
external node is revoked too; one from a port outside that range, to another
port or on another IP is kept.
- A failed revoke stops the scale through the existing failure handling
instead of leaving the cluster in `Scaling`, and a retry finds the remaining
rules.
- `removeSshFirewallRule` is unchanged; #14165 changes it, and this PR does
not touch those lines.
- Any client that passes an external node in `nodeids` to
`scaleKubernetesCluster`, such as the delete action in the UI's node list, now
gets an error. The request fails with "External nodes can not be removed by
scaling a cluster, use removeNodesFromKubernetesCluster", before any change to
the cluster. Before, the same request failed later on isolated networks, after
the SSH port forwarding rules of the other nodes had been removed, or, on VPC
tiers, lowered the node count without removing the node (both from reading the
code). Requests with only nodes deployed by CKS are validated as before.
- `scaleKubernetesClusterIsolatedNetworkRules` is now `protected` for the
unit tests.
--
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]
