Skip to content

Implement subnet config validation (fixes #4552) #5364

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.

Sign up for GitHub

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

Merged
shin- merged 4 commits into from
Nov 17, 2017
Merged

Implement subnet config validation (fixes #4552) #5364

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
5691b82
Implement subnet config validation (fixes #4552)
DrewRomanyk Nov 9, 2017
68c636d
Fix subnet config test for windows
DrewRomanyk Nov 10, 2017
6c8184d
Add format to other v3 configs & remove unix dependency
DrewRomanyk Nov 11, 2017
badd4d7
Refactor subnet cidr validator & add new test
DrewRomanyk Nov 14, 2017
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion compose/config/config_schema_v3.0.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -294,7 +294,7 @@
"items": {
"type": "object",
"properties": {
"subnet": {"type": "string"}
"subnet": {"type": "string", "format": "subnet_ip_address"}
},
"additionalProperties": false
}
Expand Down
2 changes: 1 addition & 1 deletion compose/config/config_schema_v3.1.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -323,7 +323,7 @@
"items": {
"type": "object",
"properties": {
"subnet": {"type": "string"}
"subnet": {"type": "string", "format": "subnet_ip_address"}
},
"additionalProperties": false
}
Expand Down
2 changes: 1 addition & 1 deletion compose/config/config_schema_v3.2.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -368,7 +368,7 @@
"items": {
"type": "object",
"properties": {
"subnet": {"type": "string"}
"subnet": {"type": "string", "format": "subnet_ip_address"}
},
"additionalProperties": false
}
Expand Down
2 changes: 1 addition & 1 deletion compose/config/config_schema_v3.3.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -412,7 +412,7 @@
"items": {
"type": "object",
"properties": {
"subnet": {"type": "string"}
"subnet": {"type": "string", "format": "subnet_ip_address"}
},
"additionalProperties": false
}
Expand Down
2 changes: 1 addition & 1 deletion compose/config/config_schema_v3.4.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -420,7 +420,7 @@
"items": {
"type": "object",
"properties": {
"subnet": {"type": "string"}
"subnet": {"type": "string", "format": "subnet_ip_address"}
},
"additionalProperties": false
}
Expand Down
2 changes: 1 addition & 1 deletion compose/config/config_schema_v3.5.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -418,7 +418,7 @@
"items": {
"type": "object",
"properties": {
"subnet": {"type": "string"}
"subnet": {"type": "string", "format": "subnet_ip_address"}
},
"additionalProperties": false
}
Expand Down
37 changes: 36 additions & 1 deletion compose/config/validation.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -44,6 +44,31 @@
VALID_NAME_CHARS = '[a-zA-Z0-9\._\-]'
VALID_EXPOSE_FORMAT = r'^\d+(\-\d+)?(\/[a-zA-Z]+)?$'

VALID_IPV4_SEG = r'(\d{1,2}|1\d{2}|2[0-4]\d|25[0-5])'
VALID_IPV4_ADDR = "({IPV4_SEG}\.){{3}}{IPV4_SEG}".format(IPV4_SEG=VALID_IPV4_SEG)
VALID_REGEX_IPV4_CIDR = "^{IPV4_ADDR}/(\d|[1-2]\d|3[0-2])$".format(IPV4_ADDR=VALID_IPV4_ADDR)

VALID_IPV6_SEG = r'[0-9a-fA-F]{1,4}'
VALID_REGEX_IPV6_CIDR = "".join("""
^
(
(({IPV6_SEG}:){{7}}{IPV6_SEG})|
(({IPV6_SEG}:){{1,7}}:)|
(({IPV6_SEG}:){{1,6}}(:{IPV6_SEG}){{1,1}})|
(({IPV6_SEG}:){{1,5}}(:{IPV6_SEG}){{1,2}})|
(({IPV6_SEG}:){{1,4}}(:{IPV6_SEG}){{1,3}})|
(({IPV6_SEG}:){{1,3}}(:{IPV6_SEG}){{1,4}})|
(({IPV6_SEG}:){{1,2}}(:{IPV6_SEG}){{1,5}})|
(({IPV6_SEG}:){{1,1}}(:{IPV6_SEG}){{1,6}})|
(:((:{IPV6_SEG}){{1,7}}|:))|
(fe80:(:{IPV6_SEG}){{0,4}}%[0-9a-zA-Z]{{1,}})|
(::(ffff(:0{{1,4}}){{0,1}}:){{0,1}}{IPV4_ADDR})|
(({IPV6_SEG}:){{1,4}}:{IPV4_ADDR})
)
/(\d|[1-9]\d|1[0-1]\d|12[0-8])
$
""".format(IPV6_SEG=VALID_IPV6_SEG, IPV4_ADDR=VALID_IPV4_ADDR).split())


@FormatChecker.cls_checks(format="ports", raises=ValidationError)
def format_ports(instance):
Expand All @@ -64,6 +89,16 @@ def format_expose(instance):
return True


@FormatChecker.cls_checks("subnet_ip_address", raises=ValidationError)
def format_subnet_ip_address(instance):
if isinstance(instance, six.string_types):
Copy link

@shin- shin- Nov 13, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Author

@DrewRomanyk DrewRomanyk Nov 14, 2017 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link

@shin- shin- Nov 14, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Author

@DrewRomanyk DrewRomanyk Nov 15, 2017 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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!!

Copy link

@shin- shin- Nov 15, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link

@shin- shin- Nov 17, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

if not re.match(VALID_REGEX_IPV4_CIDR, instance) and \
not re.match(VALID_REGEX_IPV6_CIDR, instance):
raise ValidationError("should use the CIDR format")

return True


def match_named_volumes(service_dict, project_volumes):
service_volumes = service_dict.get('volumes', [])
for volume_spec in service_volumes:
Expand Down Expand Up @@ -391,7 +426,7 @@ def process_config_schema_errors(error):

def validate_against_config_schema(config_file):
schema = load_jsonschema(config_file)
format_checker = FormatChecker(["ports", "expose"])
format_checker = FormatChecker(["ports", "expose", "subnet_ip_address"])
validator = Draft4Validator(
schema,
resolver=RefResolver(get_resolver_path(), schema),
Expand Down
88 changes: 88 additions & 0 deletions tests/unit/config/config_test.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2846,6 +2846,94 @@ def check_config(self, cfg):
)


class SubnetTest(unittest.TestCase):
INVALID_SUBNET_TYPES = [
None,
False,
10,
]

INVALID_SUBNET_MAPPINGS = [
"",
"192.168.0.1/sdfsdfs",
"192.168.0.1/",
"192.168.0.1/33",
"192.168.0.1/01",
"192.168.0.1",
"fe80:0000:0000:0000:0204:61ff:fe9d:f156/sdfsdfs",
"fe80:0000:0000:0000:0204:61ff:fe9d:f156/",
"fe80:0000:0000:0000:0204:61ff:fe9d:f156/129",
"fe80:0000:0000:0000:0204:61ff:fe9d:f156/01",
"fe80:0000:0000:0000:0204:61ff:fe9d:f156",
"ge80:0000:0000:0000:0204:61ff:fe9d:f156/128",
Copy link

@shin- shin- Nov 13, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Add a test for more than 1 slash as well!

Copy link
Author

@DrewRomanyk DrewRomanyk Nov 14, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Will do, good idea!

"192.168.0.1/31/31",
]

VALID_SUBNET_MAPPINGS = [
"192.168.0.1/0",
"192.168.0.1/32",
"fe80:0000:0000:0000:0204:61ff:fe9d:f156/0",
"fe80:0000:0000:0000:0204:61ff:fe9d:f156/128",
"1:2:3:4:5:6:7:8/0",
"1::/0",
"1:2:3:4:5:6:7::/0",
"1::8/0",
"1:2:3:4:5:6::8/0",
"::/0",
"::8/0",
"::2:3:4:5:6:7:8/0",
"fe80::7:8%eth0/0",
"fe80::7:8%1/0",
"::255.255.255.255/0",
"::ffff:255.255.255.255/0",
"::ffff:0:255.255.255.255/0",
"2001:db8:3:4::192.0.2.33/0",
"64:ff9b::192.0.2.33/0",
]

def test_config_invalid_subnet_type_validation(self):
for invalid_subnet in self.INVALID_SUBNET_TYPES:
with pytest.raises(ConfigurationError) as exc:
self.check_config(invalid_subnet)

assert "contains an invalid type" in exc.value.msg

def test_config_invalid_subnet_format_validation(self):
for invalid_subnet in self.INVALID_SUBNET_MAPPINGS:
with pytest.raises(ConfigurationError) as exc:
self.check_config(invalid_subnet)

assert "should use the CIDR format" in exc.value.msg

def test_config_valid_subnet_format_validation(self):
for valid_subnet in self.VALID_SUBNET_MAPPINGS:
self.check_config(valid_subnet)

def check_config(self, subnet):
config.load(
build_config_details({
'version': '3.5',
'services': {
'web': {
'image': 'busybox'
}
},
'networks': {
'default': {
'ipam': {
'config': [
{
'subnet': subnet
}
],
'driver': 'default'
}
}
}
})
)


class InterpolationTest(unittest.TestCase):

@mock.patch.dict(os.environ)
Expand Down