Skip to content
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.

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

Replace cerberus with jsonschema for configuration validation #3638

Merged
merged 2 commits into from Sep 4, 2022
Merged

Replace cerberus with jsonschema for configuration validation #3638

merged 2 commits into from Sep 4, 2022

Conversation

zhan9san
Copy link
Contributor

@zhan9san zhan9san commented Aug 19, 2022

Fixes: #3637 Remove cerberus, use jsonschema instead.

@ssbarnea
Could you kindly review this PR?

I tried to make this PR smaller, but it seems not.

Feel free to leave your comments.

Schema Definition

As the definition of schemas between cerberus and jsonschema are different, all schemas are redefined and consolidated into one src/molecule/schemas/molecule.json.

ansible-lint will sync the schemas from https://github.com/ansible/schemas, but for molecule, I think there is no need to sync it from another repo. So src/molecule/schemas/molecule.json makes the maintenance convenient.

JSON Schema

ansible-lint: "http://json-schema.org/draft-07/schema",

https://json-schema.org/draft/2020-12/schema is used in this PR.

This new draft supports Unevaluated Properties, and it has more power than additionalProperties.
So it is used in MoleculeDependencyModel schema.

_preflight

_preflight and related test functions are deleted as well, because I think it is redundant.

Validation Errors

For simplicity, only the first error would be showed. It is not user-friendly.
But currently, the PR is too big to review, so I suggest we can optimize it later to show all errors via iter_errors, if needed.

Unit test

Some unit tests are deleted for simplicity. We can add more in another PR, if needed.

@zhan9san zhan9san requested review from a team as code owners August 19, 2022 02:44
@zhan9san zhan9san requested review from Shaps, sky-joker, ganeshrn, cidrblock and shatakshiiii and removed request for a team August 19, 2022 02:44
@ssbarnea
Copy link
Member

@zhan9san Thanks for working on this, that is indeed a very helpful change. I did not look into details but it makes sense to remove lots of tests that no longer apply to use of json-schema.

I do totally agree that we should move some of the schemas inside the projects themselves. In fact I was planning to do the same for ansible-lint and ansible-navigator, as these are quite simple. There is no more authoritative source for managing a schema than the project using it.

There is still one areas that worries me, is the use of a newer draft spec and basically because these schemas are using by vscode-yaml extension, which does not support the latest drafts. We must be sure that we do not start to use features that are not yet supported by it or we will break it.

@ssbarnea
Copy link
Member

If someone can take a look on this change it would be awesome. What is the impact on current plugins? Are they going to break and will need to be fixed?

@ssbarnea ssbarnea changed the title Use jsonschema validate Replace cerberus with jsonschema for configuration validation Sep 4, 2022
@ssbarnea ssbarnea merged commit 14b2b2f into ansible:main Sep 4, 2022
@ssbarnea ssbarnea added bug and removed enhancement labels Oct 16, 2022
@jsf9k jsf9k mentioned this pull request Oct 18, 2022
@elara-leitstellentechnik

What is the impact on current plugins? Are they going to break and will need to be fixed?

This breaks molecule-vmware in several places:
The default driver template puts a bunch of configuration into the driver section (see Readme.md), which leads to this validation error:

CRITICAL Failed to validate /home/fake/code/molecule-test/test/molecule/default/molecule.yml

["Additional properties are not allowed ('datacenter', 'dns_servers', 'esxi_hostname', 'folder', 'instance_os_type', 'validate_certs', 'vcenter_hostname', 'vcenter_password', 'vcenter_username', 'vm_password', 'vm_username' were unexpected)"]

This seems easily fixable by moving the config to the driver.options section. Is this the intended way to handle this usecase?
However the enum with valid drivers finally breaks the vmware plugin:

CRITICAL Failed to validate /home/fake/code/ansible-config/molecule/default/molecule.yml

["'vmware' is not one of ['azure', 'ec2', 'delegated', 'docker', 'containers', 'openstack', 'podman', 'vagrant', 'digitalocean', 'gce', 'libvirt', 'lxd']"]

Having this enum seems like a really bad idea to me, since it will be a showstopper for every new driver being developed. I would therefore suggest, either making this only a warning, or remove it altogether.

@apatard
Copy link
Contributor

apatard commented Oct 25, 2022

What is the impact on current plugins? Are they going to break and will need to be fixed?

This breaks molecule-vmware in several places: The default driver template puts a bunch of configuration into the driver section (see Readme.md), which leads to this validation error:

CRITICAL Failed to validate /home/fake/code/molecule-test/test/molecule/default/molecule.yml

["Additional properties are not allowed ('datacenter', 'dns_servers', 'esxi_hostname', 'folder', 'instance_os_type', 'validate_certs', 'vcenter_hostname', 'vcenter_password', 'vcenter_username', 'vm_password', 'vm_username' were unexpected)"]

This seems easily fixable by moving the config to the driver.options section. Is this the intended way to handle this usecase? However the enum with valid drivers finally breaks the vmware plugin:

CRITICAL Failed to validate /home/fake/code/ansible-config/molecule/default/molecule.yml

["'vmware' is not one of ['azure', 'ec2', 'delegated', 'docker', 'containers', 'openstack', 'podman', 'vagrant', 'digitalocean', 'gce', 'libvirt', 'lxd']"]

Having this enum seems like a really bad idea to me, since it will be a showstopper for every new driver being developed. I would therefore suggest, either making this only a warning, or remove it altogether.

Please have a look at #3689, which, I guess, is the same problem.

@gardar
Copy link
Contributor

gardar commented Dec 1, 2022

This seems to break delegated drivers, how can we extend the schema?

@zhan9san
Copy link
Contributor Author

zhan9san commented Dec 1, 2022

Have you tried the latest version?

The delegated driver is in the allowed list.

https://github.com/ansible-community/molecule/blob/210e4b5a062e988894a4b39a2a6160a648098a6c/src/molecule/data/molecule.json#L85

@gardar
Copy link
Contributor

gardar commented Dec 1, 2022

Have you tried the latest version?

The delegated driver is in the allowed list.

https://github.com/ansible-community/molecule/blob/210e4b5a062e988894a4b39a2a6160a648098a6c/src/molecule/data/molecule.json#L85

Yep I'm on the latest release, v4.0.3.
My delegated driver schema looks something like this:

---
driver:
  name: delegated
  options:
    managed: true
  foreman_fqdn: https://my.foreman.server
  foreman_validate_certs: false
  foreman_ssh_user: root
  foreman_ssh_port: 22
  foreman_ssh_job_template_name: "Ansible Roles - Single Role"
  foreman_ssh_job_template_input:
    - key: "ansible_role"
      value: "example.role"

The Error I get is:
["Additional properties are not allowed ('foreman_fqdn', 'foreman_validate_certs', 'foreman_ssh_user', 'foreman_ssh_port', 'foreman_ssh_job_template_name', 'foreman_ssh_job_template_input' were unexpected)"]

@zhan9san
Copy link
Contributor Author

zhan9san commented Dec 2, 2022

@gardar

Could you help review #3765 ?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Remove cerberus dependency from molecule
6 participants