Operating System: All
Platform: All

Hi Anton,

I have a few queries w.r.t this bug:

1) Which test scripts are we referring to here?

2) Probable reason of not assigning the same IP that is passed via northbound and hard coding/installing 10.0.0.0/24 every time. Wants to understand if problem is with OF plugin or if its at switch side.

3 What is the possible set of valid values or how will we classify values as valid in the tests.

3) How can I replicate the Bug? Specific steps if any.

5) Switch used (OVS/Mininet) for replication.

(In reply to Shreshtha Joshi from comment #1)
> Hi Anton,
>
> I have a few queries w.r.t this bug:
>
> 1) Which test scripts are we referring to here?
>
> 2) Probable reason of not assigning the same IP that is passed via
> northbound and hard coding/installing 10.0.0.0/24 every time. Wants to
> understand if problem is with OF plugin or if its at switch side.

Wrong example.

Try passing 10.0.0.1/24 - something which controller allows due to deficiencies in its yangtools backend.

You get back from the switch 10.0.0.0/24 which fails to match because the comparator also does not canonicalize prefixes as required by the yang RFC. Once again - deficiency is in yangtools, but as long as yangtools remains deficient the plugin will have to have workarounds for this.

>
> 3 What is the possible set of valid values or how will we classify values as
> valid in the tests.
>
> 3) How can I replicate the Bug? Specific steps if any.

See above.

>
> 5) Switch used (OVS/Mininet) for replication.

Pretty much any switch you like. The results will be the same.

What is the need to maintain operational state equals to configuration state.

While writing the flow when I pass <ipv4-destination>10.0.10.2/24</ipv4-destination> the response from the switch which is in operational gives me <ipv4-destination>10.0.10.0/24</ipv4-destination>, which seems logical to me as the switch will return the entire subnet mask(network range) instead of a particular ipv4-destination.

I am sorry but I fail to understand how will openFlow plugin will raise an invalid argument exception. Also how can you classify this ipv4-destination as valid/invalid.

(In reply to Shreshtha Joshi from comment #3)
> What is the need to maintain operational state equals to configuration state.
>
> While writing the flow when I pass
> <ipv4-destination>10.0.10.2/24</ipv4-destination> the response from the
> switch which is in operational gives me
> <ipv4-destination>10.0.10.0/24</ipv4-destination>, which seems logical to me
> as the switch will return the entire subnet mask(network range) instead of a
> particular ipv4-destination.

Correct.

>
> I am sorry but I fail to understand how will openFlow plugin will raise an
> invalid argument exception. Also how can you classify this ipv4-destination
> as valid/invalid.

The first ipv4-destination is invalid. So says the yang RFC. However it is allowed by controller core and this is what will be in the config state.

The switch however will accept it AND normalize it internally. The result will be exactly that - that the operational state will differ from config state. You are also correct that this is logical.

However this is a bug because:

Any flow expiration events regarding the flow will not be matched. Statistics will not be matched. And so on. The result will be buggy behaviour.

Two options here:

1. Pile up a lot of extra code to deal with incorrect addresses/netmasks (see statistics manager) as an example.

2. Perform additional validation on input to compensate that the current yangtools are incapable of complying to the Yang RFC. Raise invalid argument exception if an invalid (as per RFC) argument is given.

This is trivial and can be added to the MatchConverter logic. I apologize for not doing it myself, I am buried under internal work. I can do it later this month.

Assigned to: shreshtha.joshi@tcs.com

Thanks.

Now its clear. Just one more thing, I looked into Yang RFC 6021 but I did not find why the first ipv4 address mentioned above in the example is invalid.

If you can please point me to that, I can start with the resolution for this bug.

I believe you want the northbound itself too send the subnet/network range as ipv4 destination instead of a specific IP. Please correct me if my understanding is incorrect.

If the above understanding hold true, raising the invalid argument will not be the correct approach and northbound itself should block the IP with invalid argument exception and should not let that IP to pass through at the first place. Doing the same as you have already mentioned needs changes in md-sal and yangtools.

Please suggest which approach to follow through.

Thanks

Northbound cannot, because northbound is incapable to.

We reviewed this on both the controller call and the openflow call and the decision was (there is a bug to that too) to do validation in openflow and other plugins which need it.

Going by the comments here,I got that, from a lot of values that are passed by user to match builder,we might want to start with the IP's to be canonical first.And if not,raise an exception.(Please correct me if wrong.)

Note: As per yang RFC 6021, the IP4 address should be in dotted-quad notation, so "/24" is not expected, hence <ipv4-destination>10.0.10.2/24</ipv4-destination> is not a vaid IP and hence is my logic below.
Interestingly, for IPV6 address the pattern mentioned in RFC passes the "/XX" input(tried on a standalone pgm), though its description says that it allows only full,mixed, shortened, and shortened-mixed notation.

Now for the solution suggested previously here, I have identified the change location in OF plugin code and IP4 pattern validation as per Yang RFC 6021.Please find psuedo code as given below. Let me know if I can proceed with same.

The change can be made in :MatchConvertorImpl.java for the convert method as follows:
(/org/opendaylight/openflowplugin/openflow/md/core/sal/convertor/match)

@Override
public List<MatchEntry> convert(Match match, BigInteger datapathid) {
...
Line 521 : if (layer3Match != null) {
if (layer3Match instanceof Ipv4Match) {
...
Line 574 : if (ipv4Match.getIpv4Destination() != null) {
Ipv4Prefix ipv4Prefix = ipv4Match.getIpv4Destination();
// ------Added For OPNFLWPLUG-412---above is usually a CIDR notation IP--
String ipv4AddressStr = ipv4Prefix.getValue();
String ipv4PatternString = "(([0-9]|[1-9][0-9]|1[0-9][0-9]|2[0-4][0-9]|25[0-5]).)

([0-9]|[1-9][0-9]|1[0-9][0-9]|2[0-4][0-9]|25 [0-5])(%[p

p

]+)?";
Pattern patternNew = Pattern.compile(ipv4PatternString);
Matcher matcher = patternNew.matcher(ipv4AddressStr);
if (!matcher.matches())

System.out.println("......ipv4AddressStr=" + ipv4AddressStr+" is avalid Input for MD-sal and yangtools.");
...
}
//similar changes for ipv6PatternString can be done
}
//where in the "ipv4PatternString" is the pattern of IP address as per Yang RFC 6021.

Have a look at the RFC comments, IPv4, IPv6 adress + prefix are special as their canonical form and restrictions cannot be expressed as regexp or range.

Example: 10.0.0.2/24 is an INVALID prefix according to the RFC and if you give it to a switch it will return back 10.0.0.0/24

IMHO the only way to make it work in a guaranteed manner is to:

1. Raise an exception on non-canonical input
2. Do some special handling for /32 in v4 and /128 in v6 maybe also raising an exception for some of the use cases.

Thanks for your reply Mate, I agree with your suggestion (1.raise an exception when invalid.2.special handling for /32 or /128 --> Perfect)

Only concern I have now is :
I am referring to,RFC 6021, Page 18 -

typedef ipv4-prefix {
type string {
pattern
'(([0-9]|[1-9][0-9]|1[0-9][0-9]|2[0-4][0-9]|25[0-5])\.)

'
+ '([0-9]|[1-9][0-9]|1[0-9][0-9]|2[0-4][0-9]|25[0-5])' <--***Look here, validating the last byte i.e. 2, of your sample IP e.g 10.0.0.2
+ '/(([0-9])|([1-2][0-9])|(3[0-2]))';
}

As per above patterns specified( ip addres + prefix),the very first pattern [0-9], passes single value 2 (of ip:10.0.0.2) also.
Hence the example 10.0.0.2 will pass.

I believe there is no check, for range of digits, that can be said to be Invalid or even Valid.
Hence, I am still not sure, How did you manage to conclude that the IP : 10.0.0.2/24 is INVALID.

Could you please mention, the RFC number, with exact section, you are referring to, so that we could be on the same page?

Thanks.

Have a look the portion of the same RFC on canonical form. It is explained there.

We had a discussion on this on the list and on yangdoctors mailing list.

The REGEXP is NOT the ultimate authority on validity. It is ONLY one of the constraints. The other constraint IS the canonical representation which for these types is mentioned in the comments pointing to the exact RFC for each of them.

Rashmi,

Are you working on this? Any updates?

Abhijit

Hi Abhijit,

Post last comment,I found

"The canonical format of an IPv4 prefix has all bits of
the IPv4 address set to zero that are not part of the
IPv4 prefix.";

As per above statement in RFC, following are examples of valid/invalid IP address prefixs.

//few Invalid IP's
Address =10.0.10.2/24; 10.0.10.0/20 ;10.10.17.0/20 ; 10.10.50.0/20

//Few valid IP's
Address ="10.0.10.0/24"; "10.0.0.0/20" ; "10.10.16.0/20"; "10.10.48.0/20"

Patch having above implementaion is done locally.
Please let me know I can submit same.

-Rashmi.

Working on the patch as suggested, in following gerrit link :

https://git.opendaylight.org/gerrit/#/c/29801/

Rashmi

Patch modified after review, is now available on same gerrit link.

Patch was made some time ago, but it's outdated now.

In the meantime custom match comparator was added (in OPNFLWPLUG-722) which solved the problem of matching flows (config vs. operational) in the case when switch "fixes" IP address according to mask - e.g. configured: 10.0.0.1/24 - installed and returned: 10.0.0.0/24. However this fixing is present in OVS up to version 2.3 only. More recent versions of OVS (2.4 and 2.5) simply reject installation these flows with: Device reported error type BADMATCH code BADWILDCARDS.

So at this I'm not sure if we still want to add some pre-validation or if we want the user to take responsibility for correct input.

Outdated, no response few months. Closing.