Sanitize NSVCA fields on YAML parser #3346

pedro-psb · 2023-12-01T20:27:33Z

This PR adds support for reading the literal value (non-casted value) of an unquoted number from a YAML file in the parse_modular function.

This is a customization of the YAML spec, but is the expected behavior for this context (see #3285)

Change yaml.safe_load to yaml.load(..., Loader=yaml.BaseLoader)
Add unit test to parse_modular for the specific issue
Small refactors

pedro-psb · 2023-12-01T21:00:41Z

@dralley The test is failing with the same permission issue I was having locally, so I guess I won't open an issue about that on oci-env.

I'm not sure how to approach this exactly, but I can do some exploration work next week.

usr/local/lib/python3.8/site-packages/pulp_rpm/app/modulemd.py:192: in parse_modular
    for module in split_modulemd_file(file):
usr/local/lib/python3.8/site-packages/pulp_rpm/app/modulemd.py:71: in split_modulemd_file
    with tempfile.TemporaryDirectory(dir=".") as tf:
usr/lib64/python3.8/tempfile.py:780: in __init__
    self.name = mkdtemp(suffix, prefix, dir)
_ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ 

suffix = '', prefix = 'tmp', dir = '.'

    def mkdtemp(suffix=None, prefix=None, dir=None):
        """User-callable function to create and return a unique temporary
        directory.  The return value is the pathname of the directory.
    
        Arguments are as for mkstemp, except that the 'text' argument is
        not accepted.
    
        The directory is readable, writable, and searchable only by the
        creating user.
    
        Caller is responsible for deleting the directory when done with it.
        """
    
        prefix, suffix, dir, output_type = _sanitize_params(prefix, suffix, dir)
    
        names = _get_candidate_names()
        if output_type is bytes:
            names = map(_os.fsencode, names)
    
        for seq in range(TMP_MAX):
            name = next(names)
            file = _os.path.join(dir, prefix + name + suffix)
            _sys.audit("tempfile.mkdtemp", file)
            try:
>               _os.mkdir(file, 0o700)
E               PermissionError: [Errno 13] Permission denied: './tmpu6rk65ns'

pulp_rpm/tests/unit/test_modulemd.py

pulp_rpm/app/tasks/synchronizing.py

pulp_rpm/tests/unit/test_modulemd.py

dralley · 2023-12-04T04:53:52Z

Have you verified that the tests fail without this change?

pedro-psb · 2023-12-04T11:58:09Z

Have you verified that the tests fail without this change?

Yes. The failure was the one we expected

pulp_rpm/tests/unit/test_modulemd.py

dralley · 2023-12-04T18:25:57Z

Ok, so there is an exception:

Almost everything is text fields, but we do have a boolean or two, and some JSON that need to be stored as such. The JSON might be OK currently, but it looks like the booleans are not.

The CI errors are referring to this field: https://github.com/pulp/pulp_rpm/blob/main/pulp_rpm/app/models/modulemd.py#L30

So we need to find a way to make sure floats are parsed as strings, but not booleans and such.

pedro-psb · 2023-12-04T21:03:40Z

So we need to find a way to make sure floats are parsed as strings, but not booleans and such.

I was thinking about doing a simple bool casting, but still, other fields inside those Json could end up messed up.

I'll go back to the more conservative approach of the custom Loader, which provides per-field control over not casting it. Then I'll add only the NSVCA fields as fields which should "bypass YAML" (thus, always be str), as we know there are no type problems with that. From our previous discussion on matrix, I believe this is safe (I can further look into that).

Wdyt?

dralley · 2023-12-05T16:28:19Z

pulp_rpm/app/modulemd.py

+    """Custom Loader that preserve unquoted float in specific fields"""
+
+    # Field to preserve (will bypass yaml casting)
+    PRESERVED_FIELDS = ("name", "stream", "version", "context", "arch")


I believe this ought to handle the stream field on ModulemdDefaults and ModulemdObsoletes as well, but we ought to make sure it's tested

https://github.com/pulp/pulp_rpm/blob/main/pulp_rpm/app/models/modulemd.py#L101

We'll talk about this a bit more on a call

dralley · 2023-12-07T18:33:01Z

pulp_rpm/tests/unit/test_modulemd.py

@@ -90,8 +89,7 @@ def test_parse_modular_preserves_literal_unquoted_values_3285(tmp_path):
    with open(file_name, "w") as file:
        file.write(sample_file_data)

-    mock_obj = SimpleNamespace(path=str(file_name))
-    all, defaults, obsoletes = parse_modular(mock_obj)
+    all, defaults, obsoletes = parse_modular(str(file_name))


You should be able to get rid of the str() around file_name, I'm not sure why that was here before?

The str is there because I was using a Path object before. Will remove it

dralley · 2023-12-07T18:43:21Z

pulp_rpm/app/modulemd.py

+
+
+class ModularYamlLoader(yaml.SafeLoader):
+    """Custom Loader that preserve unquoted float in specific fields"""


Can we leave a bit more detail, including the issue number where this was originally encountered, and perhaps point to the libmodulemd equivalent code

Also, where this code in particular came from

pulp_rpm/tests/unit/test_modulemd.py

dralley · 2023-12-07T19:05:37Z

pulp_rpm/tests/unit/test_modulemd.py

+    # parse_modular changes the structure and key names for obsoletes
+    assert modulemd_obsoletes["module_name"] == "perl"
+    assert modulemd_obsoletes["module_stream"] == "5.30"
+    assert modulemd_obsoletes["obsoleted_by_module_name"] == "5.40"


Shouldn't this be obsoleted_by_module_stream?

edit: oh look, a bug

the actual metadata is unaffected since we use raw yaml snippets. Our other tests are primarily interested in this, and so it wasn't caught. Only the metadata Pulp itself keeps for reference is wrong

This is a pretty minor bug, all things considered, because module obsoletes are basically unused and even less often would this metadata in question be referenced. However we should probably fix it anyways.

This should be

assert modulemd_obsoletes["obsoleted_by_module_name"] == "perl" assert modulemd_obsoletes["obsoleted_by_module_stream"] == "5.40"

Let's also fill these tests out with a few more fields

I found "obsoleted_by_module_name" odd, but just thought, "it is what it is".
Will fix the constant name and test more fields

👍

Nice job. You can trust your instincts, though! Just because it works in a certain way currently, doesn't mean it's right :)

pedro-psb · 2023-12-11T13:52:44Z

When adding more asserts, I've found that modulemd_defaults does not use modified. I've left it as is because it does not seem useful to add new things if that's not obviously wrong (as the module_name thing).

pedro-psb · 2023-12-11T14:49:39Z

The failure is due to the fix in obsoletes constant name, I'm taking a look into the test (will add those changes to another commit)

dralley · 2023-12-11T14:52:26Z

Most likely, the constant in the test is wrong. What I did was take the output from the initial implementation, check it by eye to ensure it was correct, and then commit that. But it's possible I missed something, in which case the constant would be incorrect.

- Replace yaml.safe_load by YAML loader that prevents unquoted numbers in NSVCA fields to loose trailing/heading zeros. - Add unit test to `parse_modular` for the specific issue closes pulp#3285

- Pass only necesary data to parse_modular (the path string) - Improve some docstrings and simple types [noissue]

dralley · 2023-12-11T21:20:45Z

pulp_rpm/tests/unit/test_modulemd.py

+        modulemd_obsoletes["message"]
+        == "Module stream perl:5.30 is no longer supported. Please switch to perl:5.32"
+    )
+    assert modulemd_obsoletes["obsoleted_by_module_stream"] == "5.40"


add obsoleted_by_module_name also

dralley

Nice work! This is good to go with one last small addition to the test

OBSOLETE_BY_STREAM attr was named as "obsoleted_by_module_name" instead of "obsoleted_by_name_stream". Fix tests accordingly. [noissue]

github-actions bot added the multi-commit Added when a PR consists of more than one commit label Dec 1, 2023

pedro-psb changed the title ~~Fix: Sanitize NSVCA fields on YAML parser~~ Sanitize NSVCA fields on YAML parser Dec 1, 2023

dkliban reviewed Dec 2, 2023

View reviewed changes

pulp_rpm/tests/unit/test_modulemd.py Outdated Show resolved Hide resolved

dralley reviewed Dec 4, 2023

View reviewed changes

pulp_rpm/app/tasks/synchronizing.py Show resolved Hide resolved

dralley reviewed Dec 4, 2023

View reviewed changes

pulp_rpm/tests/unit/test_modulemd.py Outdated Show resolved Hide resolved

pedro-psb force-pushed the fix/sanitize-nsvca-fields branch 3 times, most recently from 5f0c091 to d34ad7e Compare December 4, 2023 16:12

dralley reviewed Dec 4, 2023

View reviewed changes

pulp_rpm/tests/unit/test_modulemd.py Show resolved Hide resolved

dralley reviewed Dec 5, 2023

View reviewed changes

pedro-psb force-pushed the fix/sanitize-nsvca-fields branch 3 times, most recently from 69cfd7b to d1f7fe6 Compare December 7, 2023 17:27

dralley reviewed Dec 7, 2023

View reviewed changes

pulp_rpm/tests/unit/test_modulemd.py Show resolved Hide resolved

dralley reviewed Dec 7, 2023

View reviewed changes

pedro-psb force-pushed the fix/sanitize-nsvca-fields branch 2 times, most recently from 3caeb02 to a9d5d95 Compare December 11, 2023 13:40

pedro-psb force-pushed the fix/sanitize-nsvca-fields branch 2 times, most recently from 85b925b to 3b40687 Compare December 11, 2023 18:16

Sanitize NSVCA values on modulemd YAML files

9664a8d

- Replace yaml.safe_load by YAML loader that prevents unquoted numbers in NSVCA fields to loose trailing/heading zeros. - Add unit test to `parse_modular` for the specific issue closes pulp#3285

Refactor parse_modular, docstrings and small type annotations

de1fee7

- Pass only necesary data to parse_modular (the path string) - Improve some docstrings and simple types [noissue]

pedro-psb force-pushed the fix/sanitize-nsvca-fields branch from 3b40687 to 4739428 Compare December 11, 2023 18:32

dralley reviewed Dec 11, 2023

View reviewed changes

dralley approved these changes Dec 11, 2023

View reviewed changes

Fix wrong name for OBSOLETE_BY_STREAM attr

f3d5e8e

OBSOLETE_BY_STREAM attr was named as "obsoleted_by_module_name" instead of "obsoleted_by_name_stream". Fix tests accordingly. [noissue]

pedro-psb force-pushed the fix/sanitize-nsvca-fields branch from 4739428 to f3d5e8e Compare December 11, 2023 22:00

dralley merged commit 37b11cb into pulp:main Dec 12, 2023
16 checks passed

pedro-psb deleted the fix/sanitize-nsvca-fields branch December 12, 2023 00:36

dralley mentioned this pull request Mar 5, 2024

"00000000" modular context chomped to "0" #3454

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Sanitize NSVCA fields on YAML parser #3346

Sanitize NSVCA fields on YAML parser #3346

pedro-psb commented Dec 1, 2023

pedro-psb commented Dec 1, 2023

dralley commented Dec 4, 2023

pedro-psb commented Dec 4, 2023

dralley commented Dec 4, 2023

pedro-psb commented Dec 4, 2023

dralley Dec 5, 2023

dralley Dec 7, 2023 •

edited

Loading

pedro-psb Dec 7, 2023

dralley Dec 7, 2023

dralley Dec 7, 2023

pedro-psb Dec 7, 2023

dralley Dec 7, 2023 •

edited

Loading

dralley Dec 7, 2023 •

edited

Loading

pedro-psb Dec 7, 2023

dralley Dec 7, 2023

pedro-psb commented Dec 11, 2023

pedro-psb commented Dec 11, 2023 •

edited

Loading

dralley commented Dec 11, 2023

dralley Dec 11, 2023

dralley left a comment •

edited

Loading



		class ModularYamlLoader(yaml.SafeLoader):
		"""Custom Loader that preserve unquoted float in specific fields"""

Sanitize NSVCA fields on YAML parser #3346

Sanitize NSVCA fields on YAML parser #3346

Conversation

pedro-psb commented Dec 1, 2023

pedro-psb commented Dec 1, 2023

dralley commented Dec 4, 2023

pedro-psb commented Dec 4, 2023

dralley commented Dec 4, 2023

pedro-psb commented Dec 4, 2023

Choose a reason for hiding this comment

dralley Dec 7, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dralley Dec 7, 2023 • edited Loading

Choose a reason for hiding this comment

dralley Dec 7, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pedro-psb commented Dec 11, 2023

pedro-psb commented Dec 11, 2023 • edited Loading

dralley commented Dec 11, 2023

Choose a reason for hiding this comment

dralley left a comment • edited Loading

Choose a reason for hiding this comment

dralley Dec 7, 2023 •

edited

Loading

dralley Dec 7, 2023 •

edited

Loading

dralley Dec 7, 2023 •

edited

Loading

pedro-psb commented Dec 11, 2023 •

edited

Loading

dralley left a comment •

edited

Loading