goal86sg opened a new pull request, #13961:
URL: https://github.com/apache/cloudstack/pull/13961
## Description
Fixes #13761
A VPC tier without an attached network ACL is a valid, supported state
(`aclid` is optional on `createNetwork`; CloudStack stopped assigning a
default-deny ACL unconditionally in CLOUDSTACK-2809). However, four CKS
lifecycle sites compared the nullable `Long` returned by
`Network.getNetworkACLId()` against the primitive `long` constants
`NetworkACL.DEFAULT_ALLOW` / `NetworkACL.DEFAULT_DENY`, auto-unboxing it and
throwing `NullPointerException` when the tier had no ACL attached. This broke
CKS cluster create/start/delete on a legitimate VPC tier configuration and left
clusters stuck in `Starting`.
### Root cause
`NetworkACL.DEFAULT_ALLOW` (=2) and `NetworkACL.DEFAULT_DENY` (=1) are
primitive `long` constants (`api/.../vpc/NetworkACL.java`). So an expression
like `network.getNetworkACLId() == NetworkACL.DEFAULT_ALLOW` auto-unboxes the
nullable `Long` and throws `NullPointerException: Cannot invoke
"java.lang.Long.longValue()"` when the tier has no ACL attached.
### Fix
Make the four comparisons null-safe with `Objects.equals` (value comparison,
no unboxing):
| # | Class | Method | Before | After |
|---|-------|--------|--------|-------|
| 1 | `KubernetesClusterManagerImpl` | `validateVpcTier` | `== DEFAULT_DENY`
| `Objects.equals(..., DEFAULT_DENY)` — null is a valid state, not rejected |
| 2 | `KubernetesClusterResourceModifierActionWorker` |
`createVpcTierAclRules` | `== DEFAULT_ALLOW` (early return) |
`Objects.equals(..., DEFAULT_ALLOW)` — null falls through to provisioning |
| 3 | `KubernetesClusterResourceModifierActionWorker` |
`removeVpcTierAclRules` | `== DEFAULT_ALLOW` (early return) | `null \|\|
Objects.equals(..., DEFAULT_ALLOW)` — no ACL ⇒ no-op on delete |
| 4 | `KubernetesClusterStartWorker` | `setupKubernetesEtcdNetworkRules` |
`!= DEFAULT_ALLOW` | `!Objects.equals(..., DEFAULT_ALLOW)` — null falls through
to provisioning |
This lets CKS reach the existing `NetworkACLService` auto-create path
(`NetworkACLServiceImpl.createAclListIfNeeded`), which creates and attaches a
custom ACL when a rule is added with `networkid` and no `aclid` — exactly the
behavior the issue expects. The downstream paths are already null-safe
(`NetworkACLItemDaoImpl.listByACL(null)` returns an empty list), so only the
four comparisons needed changing.
## Types of changes
- [x] Bug fix (non-breaking change which fixes an issue)
## How Has This Been Tested?
- New regression unit tests, each reproducing the NPE on `main` before this
change and passing after:
- `KubernetesClusterManagerImplTest#testValidateVpcTierNullAclId` — null
ACL is accepted (no NPE / no rejection).
-
`KubernetesClusterResourceModifierActionWorkerTest#removeVpcTierAclRulesNullAclIdIsNoOp`
— delete with no ACL is a no-op (no NPE).
- Ran the `kubernetes-service` plugin unit-test module locally (JDK 17): all
tests pass — `KubernetesClusterManagerImplTest` 48/48 and
`KubernetesClusterResourceModifierActionWorkerTest` 8/8 — including the two new
regression tests.
- The two remaining nullable-ACL paths (`createVpcTierAclRules`,
`setupKubernetesEtcdNetworkRules`) use the identical null-safe `Objects.equals`
pattern and are exercised by the reporter's regression suite referenced in
#13761; the existing CKS Marvin/integration tests cover the broader
create/start/delete lifecycle.
## Checklist
- [x] I have read the CONTRIBUTING document.
- [x] My code follows the code style of this project.
- [x] I have signed off my commits (DCO).
- [x] I have added tests that prove my fix is effective.
--
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]
