Implement subnet config validation (fixes #4552)

[DrewRomanyk]

Add enhancement to config validation for subnet validation. This is my first Docker Compose PR, so please let me know if I missed anything!

ASSUMPTIONS:

• Only apply this to version 3.5
• Valid/Invalid as shown in the tests are correct for all subnets used by Docker Compose
• OS has support for socket.inet_pton() (As implemented by http://python-jsonschema.readthedocs.io/en/latest/validate/#validating-formats)

EDIT:
It seems that I am failing the Windows py27, as it appears to not support socket.inet_pton(). Should I make the solution not have to support socket.inet_pton(), but rather a very long regex to support all notations of ipv6 such as (https://www.regexpal.com/93988)?

[shin-]

Hi Drew!

I think you're off to a good start, but the test you added still seems to be failing. You can run the test suite locally first using the tox command (you might need to install it with pip install tox). If you're debugging and only want to run a few tests at a time, you can use pytest directly as well.

On the design end:

• Validation should be added to all versions that support the subnet configuration, i.e. 3.0 - 3.5
• You indeed can not rely on UNIX-only functions/libraries. A regex is a sensible solution here.

HTH!

[DrewRomanyk]

Hi Joffrey!

Thanks for the feedback and for the advice! I did test using the script/test/default method, however it passed those tests on my mac.

As for the design bits, I:

• added the validation for all the v3 config versions
• removed that UNIX dependency.

However, I found the regex for the IPV6 would be very huge, and decided to use the format string option to make it more readable, if however, you would prefer just one huge regex string, I can easily do that! Also for the subnet config, is CIDR required?

[shin-]

Almost there! There's a couple issues with the validation and some nitpicking that I'd like you to address, but other than that, solid PR.

[shin-]

I don't think we need a regexp for this part - a simple int conversion should suffice.

[DrewRomanyk]

Would it be okay if I would just include the CIDR prefix check within the regex pattern then, as all CIDRs would require that prefix?

[shin-]

That would work too.

[shin-]

Conventionally, the entire string is the CIDR format. The part after the slash is usually called the prefix. In our case, I think it's fine to say "should use the CIDR format" for this error message.

[shin-]

In the same vein, you might want to update the variable and constant names to match.

[shin-]

It's unlikely this will ever be fed something that isn't a string, but if it does, this will break when you try to split() a few lines later.
I'd remove the type check altogether.

[DrewRomanyk]

Actually, it seems that this check is common for the format checker methods in order for it to go to the type checker. Otherwise, in the old code, it will fail at the '/' check or in the new code where I just use regex, at the re.match code.

So would you prefer me to:

• Translate TypeError to ValidationError by try/except
• Add TypeError to the annotation for raises
• Keep the check which is in line with the pattern used by the Docker Compose expose format checker, and also ipv4/ipv6 format checkers

[shin-]

The other 2 format_* methods can take integers as well, which is why they check the type. Since we enforce type: string on subnet entries, it will exit before ever hitting this method if the type is invalid:

$ cat docker-compose.yml 
version: '3.5'
services:
  web:
    image: busybox
networks:
  default:
    ipam:
      config:
        - 'subnet': 1234
      driver: 'default'
$ docker-compose config
ERROR: The Compose file './docker-compose.yml' is invalid because:
networks.default.ipam.config.subnet contains an invalid type, it should be a string

[DrewRomanyk]

Hey Joffrey!

If you are using the new format code with those commands then maybe our environments are different, otherwise, it appears that at least on my end it is running the format checker before running the type checker. When I remove the isinstance check and run those two same commands with the same docker compose file:

$ cat docker-compose.yaml 
version: '3.5'
services:
  web:
    image: busybox
networks:
  default:
    ipam:
      config:
        - 'subnet': 1234
      driver: 'default'
$ docker-compose config
...
...
...
compose/config/validation.py", line 94, in format_subnet_ip_address
    if not re.match(VALID_REGEX_IPV4_CIDR, instance) and \
  File "/System/Library/Frameworks/Python.framework/Versions/2.7/lib/python2.7/re.py", line 141, in match
    return _compile(pattern, flags).match(string)
TypeError: expected string or buffer

And when I modify the 3.5 config json schema file, to where subnet requires a number type, the type checker still doesn't complain when I give it a boolean. And when I run the type:number/format:subnet with a valid subnet, it fails at the type checker, but goes through the format checker (I tested by a simple print statement). Could our Json schema libraries be out of sync then?

So then, in this case, could it be:

• Are our environments not in sync? Aka: I messed something up with my environment to where Format is running before Type check?
• I messed up setting the Json schema up somewhere?
• Is the solution with isinstance safer?

If I go inside the jsonschema._format.py code, here is a few examples of what I've seen, which they all seem to have that isinstance check even for string only types such as emails

@_checks_drafts("email")
def is_email(instance):
    if not isinstance(instance, str_types):
        return True
    return "@" in instance
@_checks_drafts(draft3="ip-address", draft4="ipv4")
def is_ipv4(instance):
    if not isinstance(instance, str_types):
        return True
    if not _ipv4_re.match(instance):
        return False
    return all(0 <= int(component) <= 255 for component in instance.split("."))
@_checks_drafts("regex", raises=re.error)
def is_regex(instance):
    if not isinstance(instance, str_types):
        return True
    return re.compile(instance)
@_checks_drafts(draft3="date", raises=ValueError)
def is_date(instance):
    if not isinstance(instance, str_types):
        return True
    return datetime.datetime.strptime(instance, "%Y-%m-%d")
@_checks_drafts(draft3="time", raises=ValueError)
def is_time(instance):
    if not isinstance(instance, str_types):
        return True
    return datetime.datetime.strptime(instance, "%H:%M:%S")

Thank you again for all the assistance/feedback Joffrey, I really appreciate it!!

[shin-]

Interesting, I'll check out your branch and take a closer look. I'll let you know.

[shin-]

Yup, you were right, my bad. I expected the type constraint to be enforced before format validation.

[shin-]

>>> format_subnet_ip_address('a/b/c')
ValueError: too many values to unpack
>>>

Since we never want more than 2 parts, we should do instance.split('/', 1) here.

[shin-]

Add a test for more than 1 slash as well!

[DrewRomanyk]

Will do, good idea!

[shin-]

LGTM, thank you!