open-policy-agent / opa

Open Policy Agent (OPA) is an open source, general-purpose policy engine.
https://www.openpolicyagent.org
Apache License 2.0
9.52k stars 1.32k forks source link

OPA support for persisted bundles with names using special characters in Windows #6915

Closed alvarogomez93 closed 2 weeks ago

alvarogomez93 commented 1 month ago

What is the underlying problem you're trying to solve?

If a bundle uses a special character like such as ? in bundle name in the bundles config, Windows will fail to write the folder that persists the bundle since it uses the bundle name as a basis for the folder name.

For example, if a bundle is called: foo?context=bar, then the directory: foo?context=bar cannot be created to place the bundle.tar.gz inside.

An example config is:

"bundles": {
    "foo": {
...
        "resource": "/bundles/foo",
    },
    "foo?type=context": {
...
        "resource": "/bundles/foo?type=context",
    }
}

Describe the ideal solution

Allow setting a different folder name per bundle in the OPA config file.

Describe a "Good Enough" solution

Strip the special characters of the bundle name before creating the folder.

anderseknert commented 1 month ago

@alvarogomez93 do you know if two different bundles can exist where the only difference is the query component? If not it would seem like a good enough solution to just strip that out. But if foo and foo?context=data could mean two different bundles altogether, that wouldn't work.

alvarogomez93 commented 1 month ago

@anderseknert in our case I believe the only difference in the name is the ?context part when we use separate data/policy bundles

charlieegan3 commented 1 month ago

For clarity, here is an example: (thanks @alvarogomez93 for the pointers)

"bundles": {
    "foo": {
        "persist": true,
        "polling": {
            "max_delay_seconds": 15,
            "min_delay_seconds": 10
        },
        "resource": "/bundles/foo"
    },
    "foo?type=context": {
        "persist": true,
        "polling": {
            "max_delay_seconds": 15,
            "min_delay_seconds": 10
        },
        "resource": "/bundles/foo?type=context"
    }
}
anderseknert commented 1 month ago

Thanks! That means we'll just have to decode it into something that is considered valid for a filename on all systems.

ashutosh-narkar commented 1 month ago

Adding a new config that specifies the relative persistence path and defaults to the bundle name seems like a good choice.

anderseknert commented 1 month ago

That would still leave us with a broken default in the case reported, and would put the onus on the user to change the configuration to work around the issue... that they may not even be aware of. So I'd go with just making a simple exception for this case, where we just escape (or transform) values that aren't valid for filesystem paths in any given OS we support.

ashutosh-narkar commented 1 month ago

Sure. Sorry I meant let's fix this. And the configurable path option seems like good functionality to include.