-
Notifications
You must be signed in to change notification settings - Fork 1.1k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Add checks if IP range is passed if Shared Network #10168
base: 4.19
Are you sure you want to change the base?
Conversation
@Pearl1594 in the expected result in #10114 it says
So that is not true? instead it is
? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
code looks good if we accept that deployment fails.
Initially when I reported the issue, I thought the issue was with specifyVlan - as broadcast URI ends up being null. However, on further investigation the reason for it was IP address not being passed. With missing inputs it's not possible to have a successful deployment. |
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## 4.19 #10168 +/- ##
=============================================
- Coverage 15.12% 4.30% -10.83%
=============================================
Files 5408 366 -5042
Lines 473954 29570 -444384
Branches 57810 5180 -52630
=============================================
- Hits 71709 1272 -70437
+ Misses 394231 28154 -366077
+ Partials 8014 144 -7870
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
if (!ntwkOff.isSpecifyIpRanges()) { | ||
throw new CloudRuntimeException("Specify IP Ranged should be true for Shared Networks"); | ||
} | ||
if (ipv4 && Objects.isNull(startIP)) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
what about endIP and endIPv6 ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The definition for endIP against the API parameter is :the ending IP address in the network IP" + " range. If not specified, will be defaulted to startIP"
So I presumed that'll be handled internally
However, I had one query - if IPv6 address is passed is it necessary to pass ipv4?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
it should not anymore, but there might be some remnance from old ipv4 only days, @Pearl1594 . Why do you think so?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Coz I was wondering if then these check should be in an if-else block.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
in the ipv6 case we have to deal with SLAAC. I don't think we should considder this in this fix. If at all, it is a separate issue.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There seems to be an import for java.util.Objects missing, @Pearl1594 .
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
clgtm
Description
Fixes: #10114
This PR adds necessary check for IP range for shared Networks.
Types of changes
Feature/Enhancement Scale or Bug Severity
Feature/Enhancement Scale
Bug Severity
Screenshots (if appropriate):
How Has This Been Tested?
How did you try to break this feature and the system with this change?