server: null-check public IP before the DNS/Source NAT guard in create

[nagaboinaramgopal]

Description

Description

createPublicLoadBalancerRule guards a special case for the DNS port before it
has resolved the public IP:

if (srcPortStart == DNS_PORT && ipVO.isSourceNat()) {

When the caller does not pass an explicit IP (ipAddrId is null), ipVO is null
at this point, so creating a load balancer rule on the DNS port throws a
NullPointerException instead of the normal validation error. Fixed by
null-checking ipVO before calling isSourceNat(), so the DNS/Source NAT branch
is skipped when there is no IP and the flow reaches the intended parameter
validation.

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 creates a public load balancer rule on the DNS port with
no explicit IP and asserts it fails with a parameter validation error instead of
a NullPointerException. Also built the standard packages and deployed on a KVM
advanced zone.

[DaanHoogland]

clgtm

[github-actions]

This pull request has merge conflicts. Dear author, please fix the conflicts and sync your branch with the base branch.

[weizhouapache]

code lgtm

[weizhouapache]

please ignore my previous approval

• We have never encountered an NPE caused by this line of code.

• I suggest throwing an exception if the ipVO object is null.

cc @DaanHoogland

[DaanHoogland]

please ignore my previous approval

• We have never encountered an NPE caused by this line of code.
• I suggest throwing an exception if the ipVO object is null.

cc @DaanHoogland

@nagaboinaramgopal , can you agree with @weizhouapache , or do you have a scenario where you want to skip checking the network services and continue?

[nagaboinaramgopal]

please ignore my previous approval

• We have never encountered an NPE caused by this line of code.
• I suggest throwing an exception if the ipVO object is null.

cc @DaanHoogland

@nagaboinaramgopal , can you agree with @weizhouapache , or do you have a scenario where you want to skip checking the network services and continue?

Thanks @weizhouapache @DaanHoogland There is a concrete scenario for skipping and continuing: an elastic LB rule, or any call without an ipAddrId, on port 53.

ipVO is only looked up when ipAddrId != null (a few lines above), so for elastic LB it is null at this line, and the system IP is assigned just below in the off.isElasticLb() && ipVO == null block. On the current line ipVO.isSourceNat() therefore throws a raw NPE before we ever reach that assignment.

On throwing instead of guarding: the null case is already handled loudly a few lines down, at if (ipVO == null) throw new InvalidParameterValueException("Unable to create load balance rule; can't find/allocate source IP"). So a genuinely unresolvable IP already gives the caller a clear error rather than an NPE. Throwing at this earlier line would instead break the elastic LB path, where ipVO is expected to be null here and is resolved immediately after.

That is why the change only guards the port 53 source-NAT check with ipVO != null and leaves the existing validation below to handle the null case. If you would rather it read more explicitly, an alternative is to move this port 53 check to just after that ipVO == null validation, so it always runs on a resolved IP and needs no guard. Happy to go either way.

[weizhouapache]

please ignore my previous approval

• We have never encountered an NPE caused by this line of code.
• I suggest throwing an exception if the ipVO object is null.

cc @DaanHoogland

@nagaboinaramgopal , can you agree with @weizhouapache , or do you have a scenario where you want to skip checking the network services and continue?

Thanks @weizhouapache @DaanHoogland There is a concrete scenario for skipping and continuing: an elastic LB rule, or any call without an ipAddrId, on port 53.

ipVO is only looked up when ipAddrId != null (a few lines above), so for elastic LB it is null at this line, and the system IP is assigned just below in the off.isElasticLb() && ipVO == null block. On the current line ipVO.isSourceNat() therefore throws a raw NPE before we ever reach that assignment.

On throwing instead of guarding: the null case is already handled loudly a few lines down, at if (ipVO == null) throw new InvalidParameterValueException("Unable to create load balance rule; can't find/allocate source IP"). So a genuinely unresolvable IP already gives the caller a clear error rather than an NPE. Throwing at this earlier line would instead break the elastic LB path, where ipVO is expected to be null here and is resolved immediately after.

That is why the change only guards the port 53 source-NAT check with ipVO != null and leaves the existing validation below to handle the null case. If you would rather it read more explicitly, an alternative is to move this port 53 check to just after that ipVO == null validation, so it always runs on a resolved IP and needs no guard. Happy to go either way.

@nagaboinaramgopal
I never tested elastic lb. If so, maybe it is better to prevent NPE only, instead of prevent null ipVO.
looks ok to me . cc @DaanHoogland

[weizhouapache]

@blueorangutan package

[blueorangutan]

@weizhouapache 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.

[nagaboinaramgopal]

please ignore my previous approval

• We have never encountered an NPE caused by this line of code.
• I suggest throwing an exception if the ipVO object is null.

cc @DaanHoogland

@nagaboinaramgopal , can you agree with @weizhouapache , or do you have a scenario where you want to skip checking the network services and continue?

Thanks @weizhouapache @DaanHoogland There is a concrete scenario for skipping and continuing: an elastic LB rule, or any call without an ipAddrId, on port 53.
ipVO is only looked up when ipAddrId != null (a few lines above), so for elastic LB it is null at this line, and the system IP is assigned just below in the off.isElasticLb() && ipVO == null block. On the current line ipVO.isSourceNat() therefore throws a raw NPE before we ever reach that assignment.
On throwing instead of guarding: the null case is already handled loudly a few lines down, at if (ipVO == null) throw new InvalidParameterValueException("Unable to create load balance rule; can't find/allocate source IP"). So a genuinely unresolvable IP already gives the caller a clear error rather than an NPE. Throwing at this earlier line would instead break the elastic LB path, where ipVO is expected to be null here and is resolved immediately after.
That is why the change only guards the port 53 source-NAT check with ipVO != null and leaves the existing validation below to handle the null case. If you would rather it read more explicitly, an alternative is to move this port 53 check to just after that ipVO == null validation, so it always runs on a resolved IP and needs no guard. Happy to go either way.

@nagaboinaramgopal I never tested elastic lb. If so, maybe it is better to prevent NPE only, instead of prevent null ipVO. looks ok to me . cc @DaanHoogland

Thanks @weizhouapache . That is exactly what the guard does here: it only prevents the NPE on this line and leaves the existing ipVO == null validation below to reject a genuinely unresolvable IP, so no behaviour change for the null case, just no raw NPE. Appreciate the review.

[blueorangutan]

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

[DaanHoogland]

@blueorangutan test

[blueorangutan]

@DaanHoogland a [SL] Trillian-Jenkins test job (ol8 mgmt + kvm-ol8) has been kicked to run smoke tests

[codecov]

Codecov Report

❌ Patch coverage is 0% with 1 line in your changes missing coverage. Please review.
✅ Project coverage is 16.35%. Comparing base (3a79799) to head (bac2cd6).
⚠️ Report is 1 commits behind head on 4.20.

Files with missing lines	Patch %	Lines
...loud/network/lb/LoadBalancingRulesManagerImpl.java	0.00%	0 Missing and 1 partial ⚠️

Additional details and impacted files

@@            Coverage Diff            @@
##               4.20   #14027   +/-   ##
=========================================
  Coverage     16.34%   16.35%           
+ Complexity    13576    13575    -1     
=========================================
  Files          5669     5669           
  Lines        501405   501405           
  Branches      60907    60907           
=========================================
+ Hits          81975    81981    +6     
+ Misses       410246   410234   -12     
- Partials       9184     9190    +6     

Flag	Coverage Δ
uitests	4.14% <ø> (ø)
unittests	17.21% <0.00%> (+<0.01%)	⬆️

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]

[SF] Trillian Build Failed (tid-16925)