saltstack / salt

Software to automate the management and configuration of any infrastructure or application at scale. Get access to the Salt software package repository here:
https://repo.saltproject.io/
Apache License 2.0
14.15k stars 5.47k forks source link

[TEST FAILURE] test_schema fails with jsonschema >= 4.0.0 #63000

Open frebib opened 1 year ago

frebib commented 1 year ago

Salt 3005.1 running tests on Debian Bookworm with jsonschema 4.7.2-3: https://packages.debian.org/bookworm/python3-jsonschema

tests/unit/utils/test_schema.py:2100 (ConfigTestCase.test_not_config_validation)
self = <tests.unit.utils.test_schema.ConfigTestCase testMethod=test_not_config_validation>
    @skipIf(HAS_JSONSCHEMA is False, "The 'jsonschema' library is missing")
    def test_not_config_validation(self):
        class TestConf(schema.Schema):
            item = schema.ArrayItem(
                title="Hungry",
                description="Are you hungry?",
                items=schema.NotItem(item=schema.BooleanItem()),
            )
        try:
            jsonschema.validate({"item": ["no"]}, TestConf.serialize())
        except jsonschema.exceptions.ValidationError as exc:
            self.fail("ValidationError raised: {}".format(exc))
        try:
            jsonschema.validate({"item": ["yes"]}, TestConf.serialize())
        except jsonschema.exceptions.ValidationError as exc:
            self.fail("ValidationError raised: {}".format(exc))
        with self.assertRaises(jsonschema.exceptions.ValidationError) as excinfo:
            jsonschema.validate({"item": [True]}, TestConf.serialize())
>       self.assertIn("is not allowed for", excinfo.exception.message)
E       AssertionError: 'is not allowed for' not found in "True should not be valid under {'type': 'boolean'}"
tests/unit/utils/test_schema.py:2122: AssertionError

seems to be caused by https://github.com/python-jsonschema/jsonschema/commit/641e9b8cf898928b4e4257e1e5cda243fc1f1aa7

Also this failure that I can't find the cause of:

tests/unit/utils/test_schema.py:874 (ConfigTestCase.test_hostname_config_validation)
self = <tests.unit.utils.test_schema.ConfigTestCase testMethod=test_hostname_config_validation>
    @skipIf(HAS_JSONSCHEMA is False, "The 'jsonschema' library is missing")
    def test_hostname_config_validation(self):
        class TestConf(schema.Schema):
            item = schema.HostnameItem(title="Item", description="Item description")
        try:
            jsonschema.validate(
                {"item": "localhost"},
                TestConf.serialize(),
                format_checker=jsonschema.FormatChecker(),
            )
        except jsonschema.exceptions.ValidationError as exc:
            self.fail("ValidationError raised: {}".format(exc))
>       with self.assertRaises(jsonschema.exceptions.ValidationError) as excinfo:
E       AssertionError: ValidationError not raised
tests/unit/utils/test_schema.py:889: AssertionError
frebib commented 1 year ago

The latter test succeeds with older jsonschema:

>>> JSONSCHEMA_VERSION
LooseVersion ('3.2.0')
>>> class TestConf(schema.Schema):
...     item = schema.HostnameItem(title="Item", description="Item description")
>>> jsonschema.validate(
...     {"item": "3"},
...     TestConf.serialize(),
...     format_checker=jsonschema.FormatChecker(),
... )
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
  File "/usr/lib/python3/dist-packages/jsonschema/validators.py", line 934, in validate
    raise error
jsonschema.exceptions.ValidationError: '3' is not a 'hostname'

Failed validating 'format' in schema['properties']['item']:
    {'description': 'Item description',
     'format': 'hostname',
     'title': 'Item',
     'type': 'string'}

On instance['item']:
    '3'

and fails (validates when it shouldn't) with newer jsonschema:

>>> HAS_JSONSCHEMA
True
>>> JSONSCHEMA_VERSION
LooseVersion ('4.7.2')
>>> class TestConf(schema.Schema):
...     item = schema.HostnameItem(title="Item", description="Item description")
...
>>> jsonschema.validate(
...     {"item": "3"},
...     TestConf.serialize(),
...     format_checker=jsonschema.FormatChecker(),
... )
>>>
frebib commented 1 year ago

I thought it could possibly be this, but that seems to reckon 3 is a bad hostname too https://github.com/python-jsonschema/jsonschema/commit/3b5f927e981576567018146a574e82bcc3a1947a#diff-cdc6b1a8a01e77eecdfa0d2559447e4ca46a014edaab1f604db31db783fed8bcR222

>>> from fqdn import FQDN
>>> FQDN("3").is_valid
False
dukelion commented 1 year ago

"localhost" is not a valid FQDN either:

>>> FQDN("localhost").is_valid
False
tj90241 commented 10 months ago

The hostname test actually looks like a Debian packaging bug of sorts.

The new jsonschema packaged in bookworm has:

with suppress(ImportError):
    from fqdn import FQDN

    @_checks_drafts(
        draft3="host-name",
        draft4="hostname",
        draft6="hostname",
        draft7="hostname",
        draft201909="hostname",
        draft202012="hostname",
    )
    def is_host_name(instance: object) -> bool:
        if not isinstance(instance, str):
            return True
        return FQDN(instance).is_valid

yet:

tyler@cacd0ca2615c:/tmp$ python3
Python 3.11.2 (main, Mar 13 2023, 12:18:29) [GCC 12.2.0] on linux
Type "help", "copyright", "credits" or "license" for more information.
>>> from fqdn import FQDN
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
ModuleNotFoundError: No module named 'fqdn'

The ImportError suppression is hiding the problem.

Hacking around the problem by YOLO-ing the fqdn module into a Debian container:

Step 13/13 : RUN pip install --break-system-packages fqdn` 
...
C_ALL=C.UTF-8 NO_INTERNET=1 python3 -m pytest -ra ./tests/unit/utils/test_schema.py
 ...
FAILED tests/unit/utils/test_schema.py::ConfigTestCase::test_hostname_config_validation - AssertionError: ValidationError raised: 'localhost' is not a 'hostname'

The next problem being that fqdn.FQDN no longer considers localhost a valid FQDN... because it is not:

tyler@95b49c4cf07d:/var/lib/sandbox/salt-build/salt-3006.4+dfsg$ python3
Python 3.11.2 (main, Mar 13 2023, 12:18:29) [GCC 12.2.0] on linux
Type "help", "copyright", "credits" or "license" for more information.
>>> from fqdn import FQDN
>>> FQDN('localhost').is_valid
False
>>> FQDN('localhostz').is_valid
False
>>> FQDN('localhost.this.is.a.fqdn').is_valid
True

jsonschema kind of changed its behavior in that it does not really check for a hostname per se, but rather a FQDN now...?

So, really two issues here!

tj90241 commented 10 months ago

There's a third change needed for jsonschema>=4.0.0 as a result of this as well: https://github.com/python-jsonschema/jsonschema/commit/eb633b216bad1acb1a7739279c5ab83bdd63d7a1

tj90241 commented 10 months ago

And then a fourth issue that looks like a bug in the unit tests: tests/unit/utils/test_schema.py::ConfigTestCase::test_optional_requirements_config_validation

Has:

        with self.assertRaises(jsonschema.exceptions.ValidationError) as excinfo:
            jsonschema.validate(
                {"personal_access_token": "foo"}, Requirements.serialize()
            )
        if JSONSCHEMA_VERSION >= Version("3.0.0"):
            self.assertIn(
                "'ssh_key_file' is a required property", excinfo.exception.message
            )
        else:
            self.assertIn(
                "is not valid under any of the given schemas", excinfo.exception.message
            )

yet ssh_key_file is not specified as a requirement in the schema -- only personal_access_token is in this context.