camaraproject / QualityOnDemand

Repository to describe, develop, document and test the QualityOnDemand API family
https://wiki.camaraproject.org/x/zwOeAQ
Apache License 2.0
37 stars 60 forks source link

Single IP addresses in Device model specified with standard formats instead of patterns #237

Closed jlurien closed 7 months ago

jlurien commented 7 months ago

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

* Single IP addresses in Device model specified with standard formats instead of patterns

Additional documentation

hdamker commented 7 months ago

See my comment within the issue.

eric-murray commented 7 months ago

MegaLinter is complaining about trailing spaces again

RandyLevensalor commented 7 months ago

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

eric-murray commented 7 months ago

@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

jlurien commented 7 months ago

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.

hdamker commented 7 months ago

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 ApplicationServerIpv6Address and stay with string and the description.

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.

hdamker commented 7 months ago

@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

@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?

jlurien commented 7 months ago

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 ApplicationServerIpv6Address and stay with string and the description.

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.

For ApplicationServerIpv6Address, pattern is removed. But there is a pattern also in ApplicationServerIpv4Address.

hdamker commented 7 months ago

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 ApplicationServerIpv6Address and stay with string and the description. 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.

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.

hdamker commented 7 months ago

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

jlurien commented 7 months ago

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 ApplicationServerIpv6Address and stay with string and the description. 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.

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.

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.

eric-murray commented 7 months ago

@jlurien Are you still planning to commit additional changes to remove the pattern for IPv4 addresses?

jlurien commented 7 months ago

@jlurien Are you still planning to commit additional changes to remove the pattern for IPv4 addresses?

I've just removed it. It should be complete now.

hdamker commented 7 months ago

Counting also Eric's previous approval in, we are done. I merge now to be able to prepare the RC for our call today.