yannh / kubeconform

A FAST Kubernetes manifests validator, with support for Custom Resources!
Apache License 2.0
2.27k stars 123 forks source link

openapi2jsonschema: Allow writting to subdirectories #276

Open peschmae opened 4 months ago

peschmae commented 4 months ago

This pull requests extends the openapi2jsonschema.py allowing to write to a subdirectory of the current working directory.

The filename templated from FILENAME_FORMAT is verified to be a child of the working directory, and if the subdirectory doesn't exist yet, it's created.

yannh commented 4 months ago

Hi @peschmae ! I'm not sure about adding this, It is opinionated (can only create subdirectories in the current working directory) and also can not create subdirectories of subdirectories. Would it not be easy to create the repository just before running this script if needed?

peschmae commented 4 months ago

Hi @yannh Thanks for the feedback.

I'm not sure about adding this, It is opinionated (can only create subdirectories in the current working directory) and also can not create subdirectories of subdirectories.

The current implementation, doesn't allow writting to any subdirectory, even if they already exists (due to the usage of os.path.basename), so even if we use something like FILENAME_FORMAT=existing/directory/{group}-{version} the files will always end up in the current working directory. This was why I wanted to make it possible to write to a subdirectory at all. The reason I opted to only allow subdirectories in the current working directory, is that the FILENAME_FORMAT is user input, and would need some sanitzing to ensure no OS files can be overwritten with this script. If only the current working directory is permitted, this will already limit the abuse potential quite a bit. (this is similar to what the current implementation is trying to achieve with os.path.basename )

I then came across #197 which tried to write the files into the directory structure used for the CRD Catalog (FILENAME_FORMAT= {fullgroup}/{kind}_{version}) which doesn't work either. To also make this possible, I've added the code to create subdirectories. Since it uses parents=true it will create the full directory structure necessary. (even nested subdirectories eg FILENAME_FORMAT=this/doesnt/exist/yet/{group}-{version})

Would it not be easy to create the repository just before running this script if needed?

While it's possible to create a single subdirectory before running the script, the approach for FILENAME_FORMAT= {fullgroup}/{kind}_{version} would be a lot more complicated, as it would require to create all group directories previously, which entails handling the CRDs/schemas to extract the information. In the openapi2jsonschema all the relevant information is already present, and requires minimal changes.

mhutter commented 1 month ago

Hi @yannh and @peschmae,

I ran into this as well recently (also while preparing stuff for CRDs-Catalog).

My pain point was that I had to process a CRD definition that contained many different Groups, so manually creating them, AND then having to move all the generated files around would certainly be possible with a couple of lines of bash script, but certainly not worth my time :-) So I went for a simple 2-line change:

@@ -102,7 +102,9 @@ def write_schema_file(schema, filename):
     schemaJSON = json.dumps(schema, indent=2)

     # Dealing with user input here..
-    filename = os.path.basename(filename)
+    dirname = os.path.dirname(filename)
+    os.makedirs(dirname, exist_ok=True)
+
     f = open(filename, "w")
     print(schemaJSON, file=f)
     f.close()

I then ran the script like this:

FILENAME_FORMAT='{fullgroup}/{kind}_{version}' ./openapi2jsonschema.py path/to/crds.yaml

Yes of course if you set FILENAME_FORMAT to some system directory AND you run the script with root permissions, then you're f...ed. I don't entirely see the point of "some sanitzing to ensure no OS files can be overwritten with this script" here. After all, a local user will execute this script, not some random untrusted user.

taylor-knapp commented 2 weeks ago

@yannh seconding I manually updated the script locally to dynamically create directories based on the fullgroup.

I think it's worth allowing users to choose to add a subdirectory, even if there are some drawbacks. The alternative is now it 'fails' silently and just drops the directories.