-
Notifications
You must be signed in to change notification settings - Fork 61
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
Single IP addresses in Device model specified with standard formats instead of patterns #237
Single IP addresses in Device model specified with standard formats instead of patterns #237
Conversation
* IPv6 address in device changed to SingleIpv6Adress. IPv6 address in ApplicationServer is not changed * Patterns for single IPv4 and IPv6 adresses, used in device model, substituted for the standards ipv4 and ipv6 formats defined in JSON-schema
See my comment within the issue. |
MegaLinter is complaining about trailing spaces again |
Trailing space
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.
LGTM, thanks @jlurien for the update
@jlurien @eric-murray I thought that this was just a change to the device and not the application server. We have use cases for supporting subnets for the application server, including 0.0.0.0/0 for all ipv4 traffic from a specific device and similarly for ipv6. |
The modified application server schema still allows for the specification of subnets, including /0 subnets |
I forgot to remark that I have kept the pattern for ApplicationServerIpv4Address. I think that one is quite safe, but as it is the only one with pattern it may appear as inconsistent with the other definitions. I have doubts here. |
My vote would go for eliminating the pattern as in BTW: I will finalize the release candidate PR #236 under the assumption that we are closing this PR here. At latest in our call on Friday that should get a go. |
@RandyLevensalor you are right that this is an extension of this PR, but I also see that we need to do it for consistency (getting rid of the the complex allOf pattern for IPv6 addresses). With the separate definition of the IP addresses for application server we are exactly allowing here address ranges (including /0) while having a more precise definition on the device object (which is reused within other APIs). Does that address your concern sufficiently? |
For ApplicationServerIpv6Address, pattern is removed. But there is a pattern also in ApplicationServerIpv4Address. |
My "vote" was for removing the pattern also for ApplicationServerIpv4Address. But it don't harm therefore I'm also fine with keeping it, at least for now. |
@RandyLevensalor: can you comment/approve if your use case are still covered with the answers above, especially with the one from Eric? If yes we would be good to go here. |
I think this is coherent approach, so implementations do not have to apply different logic per IP address version. I will remove it as well. |
@jlurien |
Removed pattern from ApplicationServerIpv4Address
I've just removed it. It should be complete now. |
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.
@jlurien Thanks for updating the application server addresses.
Counting also Eric's previous approval in, we are done. I merge now to be able to prepare the RC for our call today. |
What type of PR is this?
What this PR does / why we need it:
Which issue(s) this PR fixes:
Fixes #230
Special notes for reviewers:
Changelog input
Additional documentation