adestis-mf opened a new pull request, #14681:
URL: https://github.com/apache/cloudstack/pull/14681
### Description
This PR fixes adding and removing public IPs of a VPC tier on the VPC
virtual router when the VPC
offering provides the Firewall service for public IPs (#12706).
`IpAddressManagerImpl.applyIpAssociations` passes
`VpcVirtualRouterElement.applyIps` the union of
the services of the public IPs of a tier. Once a public IP of the tier has a
firewall rule,
`Firewall` is part of that union. `applyIps` associates the IPs only when
`canHandle(tier, service)` is true for every service, and the tier's network
offering does not
list the Firewall service, because #12706 puts it on the VPC offering.
`applyIps` then skips the
association and returns `true`:
- a new public IP with port forwarding or load balancing rules in that tier
never reaches the
router, also not on a network restart;
- a public IP whose last rule is removed stays configured on the router.
`applyIps` now checks the Firewall service on the VPC when the tier does not
provide it
(`canHandle(Vpc, Firewall)`, the same check as in `applyFWRulesInVPC`).
Other services and
`canHandle(Network, Service)` are unchanged, so
`testCanHandleFirewallUsesVpcCapability` still
holds. A debug line is logged when the association is skipped, so the case
is no longer silent.
What changes: in a VPC whose offering provides the Firewall service through
the VPC virtual
router, public IPs of a tier are added to and removed from the router the
same way as in a VPC
without the Firewall service. The IPs passed to the element are the same as
before. IPs that have
only firewall rules and the source NAT IP of the VPC are not associated with
a tier and are still
not passed. VPC offerings whose Firewall provider is not the VPC virtual
router behave as before.
This targets `main`: #12706 is not in `4.22`, and there is no `4.23` branch.
The commit also
applies cleanly on top of the 4.23.0.0 tag.
Fixes: #14146
Fixes: #14669
### 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 `VpcVirtualRouterElementTest`:
- `testApplyIpsAssociatesTierIpsWhenTheVpcProvidesFirewall`: the tier does
not provide Firewall,
the VPC does → the IPs are associated.
- `testApplyIpsSkipsAssociationWhenNeitherTierNorVpcProvidesFirewall`:
neither provides it → no
association, no router lookup.
- `testCanHandleIpServiceUsesTheVpcOnlyForFirewall`: other services never
consult the VPC.
- `testCanHandleIpServiceKeepsTierFirewall`: when the tier provides
Firewall, the VPC is not
consulted.
With the previous check (`canHandle(network, service)`) put back into
`applyIps`,
`testApplyIpsAssociatesTierIpsWhenTheVpcProvidesFirewall` fails.
```
mvn -pl server -am test -Dtest=VpcVirtualRouterElementTest \
-Dsurefire.failIfNoSpecifiedTests=false -DfailIfNoTests=false
```
`VpcVirtualRouterElementTest`: 8 tests, 0 failures. The tests in
`com.cloud.network.element`,
`com.cloud.network.router`, `com.cloud.network.vpc`, `com.cloud.network.lb`,
`com.cloud.network.firewall`, `IpAddressManager*Test` and
`NetworkModel*Test` also pass (352
tests, 0 failures, 4 skipped). Checkstyle reports no violations.
The behaviour without this change was reproduced on 4.23.0.0; steps and
results are in #14146.
The patched build has not been run on a live system.
#### How did you try to break this feature and the system with this change?
I went through the cases in which the changed check decides differently:
- public IPs with only firewall rules and the VPC source NAT IP are not
associated with a tier,
so they are not passed to `applyIps`, as before;
- static NAT passes only its own IPs; the change only matters there when the
static NAT IP itself
has a firewall rule;
- redundant VPC routers are handled by the same loop over the routers of the
tier as without
firewall rules;
- other services, and VPC offerings with another Firewall provider, keep the
previous result.
--
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]
