weizhouapache commented on PR #14027:
URL: https://github.com/apache/cloudstack/pull/14027#issuecomment-5574715869
> > > 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
--
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]
