Skip to content

server: null-check requested IP in assignSourceNatPublicIpAddress - #14025

Open
nagaboinaramgopal wants to merge 1 commit into
apache:4.20from
nagaboinaramgopal:fix/sourcenat-ip-npe
Open

server: null-check requested IP in assignSourceNatPublicIpAddress#14025
nagaboinaramgopal wants to merge 1 commit into
apache:4.20from
nagaboinaramgopal:fix/sourcenat-ip-npe

Conversation

@nagaboinaramgopal

Copy link
Copy Markdown

Description

assignSourceNatPublicIpAddress looks up the requested IP with
findByIpAndDcId and then reads its state:

IPAddressVO ipAddressVO = _ipAddressDao.findByIpAndDcId(dcId, requestedIp);
if (ipAddressVO.getState() != State.Free) {

findByIpAndDcId returns null when the requested IP is not a known public IP in
the zone, so this throws a NullPointerException instead of a meaningful error.
Fixed by null-checking the result first, so the method throws the intended
InsufficientAddressCapacityException when the IP cannot be found.

Types of changes

  • Bug fix (non-breaking change which fixes an issue)

Feature/Enhancement Scale or Bug Severity

Bug Severity

  • Minor

How Has This Been Tested?

Added a unit test that requests a source NAT IP not present in the zone and
asserts an InsufficientAddressCapacityException is thrown instead of an NPE.
Also built the standard packages and deployed on a KVM advanced zone.

@DaanHoogland DaanHoogland left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

clgtm

assignSourceNatPublicIpAddress looked up the requested IP with
_ipAddressDao.findByIpAndDcId() and immediately checked ipAddressVO.getState().
findByIpAndDcId returns null when the requested IP is not a known public IP in
the zone, so an invalid or unknown requestedIp produced a NullPointerException
instead of a meaningful capacity error.

Treat a null lookup result like a non-free address and throw
InsufficientAddressCapacityException.

Adds a regression test with an unknown requested IP (NullPointerException
before the fix).
@nagaboinaramgopal
nagaboinaramgopal changed the base branch from main to 4.20 September 3, 2026 17:52
@DaanHoogland

Copy link
Copy Markdown
Contributor

@blueorangutan package

@blueorangutan

Copy link
Copy Markdown

@DaanHoogland a [SL] Jenkins job has been kicked to build packages. It will be bundled with KVM, XenServer and VMware SystemVM templates. I'll keep you posted as I make progress.

@codecov

codecov Bot commented Sep 3, 2026

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 19.78%. Comparing base (2cd8c5e) to head (87d961b).

Additional details and impacted files
@@             Coverage Diff              @@
##               4.20   #14025      +/-   ##
============================================
+ Coverage     16.34%   19.78%   +3.44%     
- Complexity    13574    19997    +6423     
============================================
  Files          5669     6371     +702     
  Lines        501368   575899   +74531     
  Branches      60903    70495    +9592     
============================================
+ Hits          81964   113962   +31998     
- Misses       410219   449508   +39289     
- Partials       9185    12429    +3244     
Flag Coverage Δ
uitests 3.53% <ø> (-0.62%) ⬇️
unittests 21.06% <ø> (+3.85%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@blueorangutan

Copy link
Copy Markdown

Packaging result [SF]: ✔️ el8 ✔️ el9 ✔️ el10 ✔️ debian ✔️ suse15. SL-JID 19092

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

Status: Backlog

Development

Successfully merging this pull request may close these issues.

4 participants