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

Move Driver JSONSchema Validation to Drivers. #3689

Closed
dmlb2000 opened this issue Oct 19, 2022 · 18 comments
Closed

Move Driver JSONSchema Validation to Drivers. #3689

dmlb2000 opened this issue Oct 19, 2022 · 18 comments
Labels

Comments

@dmlb2000
Copy link
Contributor

Issue Type

  • Bug report

Molecule and Ansible details

ansible [core 2.13.5]
  config file = None
  configured module search path = ['/home/dmlb2000/.ansible/plugins/modules', '/usr/share/ansible/plugins/modules']
  ansible python module location = /home/dmlb2000/.virtualenvs/ansible-azure/lib/python3.10/site-packages/ansible
  ansible collection location = /home/dmlb2000/.ansible/collections:/usr/share/ansible/collections
  executable location = /home/dmlb2000/.virtualenvs/ansible-azure/bin/ansible
  python version = 3.10.7 (main, Sep  7 2022, 00:00:00) [GCC 12.2.1 20220819 (Red Hat 12.2.1-1)]
  jinja version = 3.0.3
  libyaml = True
molecule 4.0.1 using python 3.10
    ansible:2.13.5
    azurevm:0.3.1.dev4 from molecule_azurevm
    delegated:4.0.1 from molecule
    podman:2.0.3 from molecule_podman requiring collections: containers.podman>=1.7.0 ansible.posix>=1.3.0

Molecule installation method (one of):

  • pip

Ansible installation method (one of):

  • pip

Detail any linters or test runners used:

Desired Behavior

Internal or private drivers are useful for companies and organizations to test against their internal services and configurations. These drivers often embed internal names and services to access credentials that will never be available for open source usage. However, these drivers are very useful internally standardizing testing environments.

Actual Behaviour

The JSONSchema validation is insufficient to include JSONSchema provided by private or internal molecule drivers and in its current (4.0.2) implementation breaks the usage of these drivers.

@dmlb2000 dmlb2000 added the bug label Oct 19, 2022
@dmlb2000
Copy link
Contributor Author

My biggest issue with this is that a security fix/patch fix update broke the usage of internal or private drivers.

@zhan9san
Copy link
Contributor

@apatard
Copy link
Contributor

apatard commented Oct 21, 2022

Instead of disabling it, I wonder if we can extend it.

fwiw, I've been wondering for quite some time if it was possible to have a schema for each driver and to supply it when checking the molecule schema. What bugs me is that what's valid for a driver may not be valid for an other, so I fear trying to support each driver in the same schema may make valid an invalid molecule file.

Moreover, if each driver provides its own schema in its repo, it may make easier to extend it. Extending it in molecule means having the driver PR reviewed and accepted but can't be merged since the CI will fail due to schema error, so molecule schema would need to be merged first. Obviously, merging the schema change first, may lead to issues on its own, since the driver PR review may lead to more schema change...

The issue here is a little bit different but I guess that properly solving this schema handling issue will solve this issue too.

Unfortunately, -ENOTIME here. So, I don't even know if it's technically possible.

@dmlb2000
Copy link
Contributor Author

Okay, similarly with me on the -ENOTIME...

Here's my suggestions...

  1. Remove the enum of the driver name in MoleculeDriverModel, it's just a string any string.
  2. Add another method to https://github.com/ansible-community/molecule/blob/main/src/molecule/driver/base.py called validate() that takes the options key in the MoleculeDriverModel and validates it.
  3. The base class implementation should just accept the options as an object and that's it.
  4. In https://github.com/ansible-community/molecule/blob/main/src/molecule/model/schema_v3.py import the Driver and call the new validate() method.
  5. Remove MoleculeDriverOptionsModel from the JSONSchema and move that to the drivers themselves.
  6. Create validate() method in https://github.com/ansible-community/molecule/blob/main/src/molecule/driver/delegated.py as example for how to validate the driver options.

@meffie
Copy link

meffie commented Oct 24, 2022

This is a critical bug and completely breaks our use of molecule which relies on a driver plugin. Is there a workaround (other than downgrading?)

meffie added a commit to meffie/molecule-proxmox that referenced this issue Oct 24, 2022
molecule 4.0.2 change the schema validation and broke third party
plugins.  Pin the max version we support until the schema validation
is fixed.

See ansible/molecule#3689
@ssbarnea
Copy link
Member

This can be fixed by allowing other driver values and not setting additional properties but someone needs to patch the schema to allow this (without altering the one for known drivers).

@meffie
Copy link

meffie commented Oct 24, 2022

Thank you ssbarnea. By 'patch the schema', do you mean something along the line of the recent commit
3ff57d4 Fix schema (#3687) ?

@zhan9san
Copy link
Contributor

Okay, similarly with me on the -ENOTIME...

Here's my suggestions...

  1. Remove the enum of the driver name in MoleculeDriverModel, it's just a string any string.
  2. Add another method to https://github.com/ansible-community/molecule/blob/main/src/molecule/driver/base.py called validate() that takes the options key in the MoleculeDriverModel and validates it.
  3. The base class implementation should just accept the options as an object and that's it.
  4. In https://github.com/ansible-community/molecule/blob/main/src/molecule/model/schema_v3.py import the Driver and call the new validate() method.
  5. Remove MoleculeDriverOptionsModel from the JSONSchema and move that to the drivers themselves.
  6. Create validate() method in https://github.com/ansible-community/molecule/blob/main/src/molecule/driver/delegated.py as example for how to validate the driver options.

Thanks @dmlb2000
Sounds great.
I'll have a try.

@ssbarnea
Copy link
Member

I will close this ticket because is misleading as we have no plans to remove/disable schema validation. Yep, we could have bugs with schemas, but these can be sorted by altering them, not by disabling it.

@dmlb2000 dmlb2000 changed the title Disable JSONSchema Validation Move Driver JSONSchema Validation to Drivers. Oct 27, 2022
@dmlb2000
Copy link
Contributor Author

dmlb2000 commented Oct 27, 2022

@ssbarnea I reject your reality and substitute my own...

Not sure what you mean by misleading, reading the body of the comments and discussion would present that there is a legitimate issue that needs to be resolved. Feel free to offer a better title if you like.

@ssbarnea ssbarnea reopened this Oct 28, 2022
@ssbarnea
Copy link
Member

Step one would to make the driver settings to allow arbitrary options.

Second stage will be to make drivers return their settings and use these to build a consolidated schema for them. The schema should still be inside molecule project and should contain data from all actively maintained drivers, so users can user auto-complete when editing files. That step will tame a longer time implement.

@dmlb2000
Copy link
Contributor Author

@ssbarnea I've been cogitating on your last couple of sentences and am coming up confused.

Do you have some example of allowing users to auto-complete (I assume the molecule.yml file) using the schema? Something in VSCode allows this?

I have some concerns with centralizing the schema to allow IDEs to have a complete understanding of allowed values in the molecule.yml file. I feel like this might be trading user experience for developer experience, as it might make developing drivers for molecule more difficult. Can you please elaborate on your longer term goals you mentioned? Or if you wrote them down somewhere already I'm happy to read up on it.

@gciavarrini
Copy link
Contributor

gciavarrini commented Dec 29, 2022

As explained in this issue and in #3783, using a custom driver is a valuable feature.
Since this PR is still under development, and It seems inactive from late October, I'd like to suggest a simpler solution to quickly have the possibility to use custom drivers.

For this purpose, I created a PR #3795 with the hope of receiving feedback.

My PR patches the molecule.json schema to allow also driver names that start with molecule or custom prefixes (i.e., custom-driver-name). IMO the use of prefixes, in addition to the current enum, still guarantees control over user input by excluding unintentional typos.

Any thoughts?

@NiceRath
Copy link

NiceRath commented Feb 6, 2023

Greetings.

Input from my side => might not 100% correlate with this ticket:

I'm running into the same 'Additional properties are not allowed' validation error whenever I use YAML-anchors to keep duplicate configuration to a minimum.

The shared configuration needs to be defined somewhere and that validation make this very frustrating.

Practical use-case:

references:
  docker:
    all: &docker_all
      purge_networks: true
      image: 'debian:11-slim'
      dockerfile: '../../../../molecule/Dockerfiles/deb11_systemd.j2'
      build_image: yes
      tmpfs: ['/tmp', '/run', '/run/lock']
      privileged: true
      command: '/sbin/init'
      docker_host: 'tcp://test-docker:2375'

platforms:
  - name: "test-1"
    <<: *docker_all
  - name: "test-2"
    <<: *docker_all

- Rath

@isuftin
Copy link

isuftin commented Feb 16, 2023

To pile on a bit, the validation is also getting in the way of being able to deploy an EC2 instance via Molecule that contains a volume definition.

ansible-community/molecule-plugins#167

The Molecule EC2 driver plugin uses https://docs.ansible.com/ansible/latest/collections/amazon/aws/ec2_instance_module.html

The driver passes in content from molecule.yml platform section's "volume" parameter and this should be a list of dict. The Molecule validator mandates it be a list of string. Therefore there's no way to launch an EC2 instance with anything but the default volume configuration.

@apatard
Copy link
Contributor

apatard commented Feb 17, 2023

@isuftin imho it's a little bit off-topic for this bug. I've not looked closely at ansible-community/molecule-plugins#167 but from what I understand, it's a bug in the schema. This possibly means that even if it's moved to the driver, someone will still have to open a PR to fix this issue.

@nejch
Copy link

nejch commented Mar 29, 2023

I'd second @NiceRath's idea of using "hidden" keys for references (#3689 (comment)) that don't get validated.

Our use case is similar, defining many ec2 instances with very similar config that would benefit from YAML anchors.

FWIW, GitLab CI allows defining "hidden jobs" prefixed by . (e.g. .references) for this https://docs.gitlab.com/ee/ci/yaml/yaml_optimization.html#anchors.

@ssbarnea
Copy link
Member

Addressed by #3765 - if that does not look enough, please make a PR.

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

No branches or pull requests

9 participants