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

The [opa eval] command is unable to access Rego policies located on other drives on windows #6922

Conversation

pckvcode
Copy link

@pckvcode pckvcode commented Aug 12, 2024

The [opa eval] command is unable to access Rego policies located on other drives in Windows. For more details, see #6910 and open-policy-agent/conftest#979.

OPA should be capable of accessing Rego policies from specified paths including drives (e.g., c:\a\b\c.rego).

The code has been updated to allow OPA to load Rego policies from paths with drives. Now, OPA can load Rego policies from paths(e.g., /a/b/c/example.rego), path with URLs(e.g., file:///path/to/file.json), and path with drives(e.g., C:\a\b\c\example.rego).

Fixes #6910

Why the changes in this PR are needed?

The opa eval command encountered an issue where it could not load Rego policies from different drives on Windows. The error message was:

CreateFile \\Users\\pck\\AppData\\Local\\Temp\\temp_dir_12342318055089\\example.rego: The system cannot find the path specified

It fixes: #6910 and open-policy-agent/conftest#979

What are the changes in this PR?

Code Update:

The code has been modified to allow OPA to load Rego policies from paths that include drive letters. Now, OPA supports loading Rego policies from:

  • Existing Inputs:
    • Paths (e.g., /a/b/c/example.rego)
    • Paths with URLs (e.g., file:///path/to/file.json)
    • Paths with drive letters and URLs (e.g., "C:file:///C:/a/b/c")
  • New Input:
    • Paths with drive letters (e.g., C:\a\b\c\example.rego)

Documentation Reference:

According to the official documentation, the expected values for the --data flag in the opa eval command are:

  1. A simple path (e.g., /a/b/c/example.rego)
  2. A path with a URL (e.g., file:///path/to/file.json)

Ideally, specifying a path like C:\a\b\c\example.rego should work, but it was failing. The code was modified to support paths with drive letters by ensuring that the prefix is only separated when its length is greater than 1.

Notes to assist PR review:

Steps to Reproduce the issue:

  1. Place the OPA binary on the D drive.
  2. Store the Rego policies on the C drive.
  3. Attempt to evaluate the policies using the opa eval command with the policies located on the C drive. The above error will occur. For more details, refer to this GitHub issue.

Further comments:

[opa eval] command before fix. (Failed: opa in D drive and Failed to load rego policies from C drive.)

Screenshot 2024-08-11 at 9 10 42 PM_Edited

[opa eval] command after fix. (Success: opa in D drive and Successfully load rego policies from C drive.)

Screenshot 2024-08-12 at 1 50 03 PM

Unit Testcase pass screenshot. Function tested

Screenshot 2024-08-12 at 1 50 28 PM

@boranx Can you please review this change and share your comments? Thanks in advance

…ther drives in Windows. For more details, see open-policy-agent#6910 and open-policy-agent/conftest#979.

OPA should be capable of accessing Rego policies from specified paths including drives (e.g., c:\a\b\c.rego).

The code has been updated to allow OPA to load Rego policies from paths with drives. Now, OPA can load Rego policies from paths, URLs, and drives.

Fixes open-policy-agent#6910

Signed-off-by: pckvcode <[email protected]>
@philipaconrad
Copy link
Contributor

The new test appears to be causing failures on non-Windows platforms (linux, darwin). We may need to either split the test off for Windows, or find some other way to OS-gate it.

Copy link

netlify bot commented Aug 21, 2024

Deploy Preview for openpolicyagent ready!

Name Link
🔨 Latest commit 9601e7e
🔍 Latest deploy log https://app.netlify.com/sites/openpolicyagent/deploys/66c6d425d695500008ad8b6a
😎 Deploy Preview https://deploy-preview-6922--openpolicyagent.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

…nput sources:

Existing Inputs:
1. Absolute file paths (e.g., /a/b/c/example.rego)
2. Paths with URLs (e.g., file:///path/to/file.json)
3. Paths with drive letters and URLs (e.g., "C:file:///C:/a/b/c")

New Inputs:
1. Paths with drive letters (e.g., C:\a\b\c\example.rego)

This requirement ensures that Opa can handle a variety of input formats, including local file paths, remote URLs, and even mixed formats that include both drive letters and URLs.

open-policy-agent#6910
Signed-off-by: pckvcode <[email protected]>
@pckvcode pckvcode force-pushed the access-rego-policies-from-other-windows-drives branch from b00c103 to 02dc039 Compare August 21, 2024 10:48
@pckvcode
Copy link
Author

@philipaconrad
I fixed it now. Can you please approve the workflow and review the changes?

I got to know we are supporting these inputs.

  • Paths (e.g., /a/b/c/example.rego)
  • Paths with URLs (e.g., file:///path/to/file.json)
  • Paths with drive letters and URLs (e.g., "C:file:///C:/a/b/c")

Added the support for below

  • Paths with drive letters (e.g., C:\a\b\c\example.rego)

My comments:

Copy link
Member

@boranx boranx left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

IMO, intuitively, I'd say we can support the following

Paths (e.g., /a/b/c/example.rego)
Paths with URLs (e.g., file:///path/to/file.json)

but I'm not sure if Paths with drive letters and URLs (e.g., "C:file:///C:/a/b/c") is a valid format, as I haven't seen such thing before 🤔

My 2 cents is to add a unit test for each one of those cases, as supported vs not supported

@pckvcode
Copy link
Author

I tested with the latest OPA Windows AMD64 binary. Here are the results:

curl -O https://github.com/open-policy-agent/opa/releases/download/v0.67.1/opa_windows_amd64.exe

Note:

  • In all these cases, I kept the binary on the Windows D drive.

Succeeded for these paths:

  • Paths with URLs (e.g., file:///path/to/file.json)
  • Paths with drive letters and URLs (e.g., "C:file:///C:/a/b/c")
  • Standard paths (e.g., /a/b/c/example.rego)
Commands used:

.\opa_windows_amd64.exe eval --input "C:\Users\pck\Punith\TestFiles\input.json" --data "file:///C:/Users/pck/Punith/TestFiles/rego_policies/example.rego" "data.example.authz.allow"
.\opa_windows_amd64.exe eval --input "C:\Users\pck\Punith\TestFiles\input.json" --data "C:file:///C:/Users/pck/Punith/TestFiles/rego_policies/example.rego" "data.example.authz.allow"
# able to access from D drive since the binary in D drive
.\opa_windows_amd64.exe eval --input "C:\Users\pck\Punith\TestFiles\input.json" --data "D:/punith/opa-bug/TestFiles/rego_policies/example.rego" "data.example.authz.allow"
Screenshot 2024-08-23 at 1 52 16 PM Screenshot 2024-08-23 at 1 52 45 PM Screenshot 2024-08-23 at 1 54 15 PM

Failed for this path:

  • Paths with drive letters (e.g., C:\a\b\c\example.rego)
# Fails since the binary is in D drive and loading policies from C drive
.\opa_windows_amd64.exe eval --input "C:\Users\pck\Punith\TestFiles\input.json" --data "C:\Users\pck\Punith\TestFiles\rego_policies\example.rego" "data.example.authz.allow"
Screenshot 2024-08-23 at 1 54 48 PM

I then tested with my code changes, and it worked for the following paths:

  • Paths with URLs (e.g., file:///path/to/file.json)
  • Paths with drive letters and URLs (e.g., "C:file:///C:/a/b/c")
  • Standard paths (e.g., /a/b/c/example.rego)
  • Paths with drive letters (e.g., C:\a\b\c\example.rego)

Commands used:

.\opa-new.exe eval --input "C:\Users\pck\Punith\TestFiles\input.json" --data "file:///C:/Users/pck/Punith/TestFiles/rego_policies/example.rego" "data.example.authz.allow"
.\opa-new.exe eval --input "C:\Users\pck\Punith\TestFiles\input.json" --data "C:file:///C:/Users/pck/Punith/TestFiles/rego_policies/example.rego" "data.example.authz.allow"
.\opa-new.exe eval --input "D:/punith/opa-bug/TestFiles/rego_policies/example.rego" "data.example.authz.allow"
.\opa-new.exe eval --input "C:\Users\pck\Punith\TestFiles\input.json" --data "C:\Users\pck\Punith\TestFiles\rego_policies\example.rego" "data.example.authz.allow"
Screenshot 2024-08-23 at 1 55 12 PM Screenshot 2024-08-23 at 1 55 38 PM Screenshot 2024-08-23 at 1 56 02 PM Screenshot 2024-08-23 at 1 56 39 PM

I also added a unit test case to verify loading files from paths with drive letters (e.g., C:\a\b\c\example.rego).

…clude a Windows drive letter (e.g., C:\a\b\c\example.rego). When tests are run on Windows, temporary directories are created on the C drive (e.g., C:\Users\<username>\AppData\Local\Temp).

Fixes: open-policy-agent#6910

Signed-off-by: pckvcode <[email protected]>
Copy link
Contributor

@johanfylling johanfylling left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just a few questions.

I don't have a windows machine to test on, but I would expect the path file:///c:/path/to/my/file/data.json to work. Have you tested this format?

@@ -582,7 +582,9 @@ func SplitPrefix(path string) ([]string, string) {
}
parts := strings.SplitN(path, ":", 2)
if len(parts) == 2 && len(parts[0]) > 0 {
return strings.Split(parts[0], "."), parts[1]
if strings.HasPrefix(parts[1], "file:///") || len(parts[0]) > 1 {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why do we put significance on the path containing file:/// or the document prefix being more than one chars long for us to return those parts rather than the given path?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

To add some context: I believe the prefix part is the prefix for which the contents of that file will be found under the data document during evaluation.

E.g.: given the following data file data.json:

{
  "foo": 42
}

and we run:

$ opa eval -fpretty -d file:///path/to/my/file/data.json 'data = x'

we expect the output:

+------------+
|     x      |
+------------+
| {"foo":42} |
+------------+

But if we add the a: prefix to the file path, we expect this prefix to be added to the data document:

opa eval -fpretty -d a:file:///path/to/my/file/data.json 'data = x'
+------------------+
|        x         |
+------------------+
| {"a":{"foo":42}} |
+------------------+

@@ -879,6 +889,15 @@ func TestSplitPrefix(t *testing.T) {
wantParts: []string{"x", "y"},
wantPath: "file:///c:/a/b/c",
},
{
input: "c:/a/b/c",
wantPath: "c:/a/b/c",
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This doesn't seem compatible with how data prefixes work in OPA, as we'd expect c to become the data prefix.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is it advisable to always use a file URL(ex: file:///c:/path/to/my/file/data.json instead of c:/path/to/my/file/data.json), especially on Windows?
How does the system behave if we provide the path as c:/path/to/my/file/data.json? In this case, should c be interpreted as a prefix or as the drive letter?

In this image, we see that using c:/path/to/my/file/data.json has failed
Screenshot 2024-08-27 at 6 25 33 PM

@pckvcode
Copy link
Author

Just a few questions.

I don't have a windows machine to test on, but I would expect the path file:///c:/path/to/my/file/data.json to work. Have you tested this format?

I tried using the path file:///c:/path/to/my/file/data.json, and it worked.
However, when I used c:/path/to/my/file/data.json, it failed.
Screenshot 2024-08-27 at 6 21 16 PM

Screenshot 2024-08-27 at 6 30 39 PM

Is it advisable to always use a file URL, especially on Windows?
How does the system behave if we provide the path as c:/path/to/my/file/data.json? In this case, should c be interpreted as a prefix or as the drive letter?

@johanfylling
Copy link
Contributor

The current behavior is to interpret the c: in c:/path/to/my/file/data.json as the data prefix c. Modifying this behavior would be a breaking change, and given that there are no recommendations/requirements for the number of characters in a prefix, I don't think this is something we can safely change.

Given the verbosity of file-urls and that Windows is the only supported operating system that uses the drive letter syntax, I don't think we can say the usage of file-urls are a general recommendation; but definitely a requirement for specifying absolute paths on Windows.
The prefix:path syntax is an unfortunate overlap. If not already clarified in the docs, I think it warrants an mention.

Thank you for bringing this to our attention @pckvcode 😃! I'm closing this PR, but if you still want to contribute to the project with your findings, you are more than welcome to open a new PR with updates to the docs.

pckvcode pushed a commit to pckvcode/conftest-pck that referenced this pull request Sep 3, 2024
    Conftest encounters errors on Windows when loading file paths that include drive letters (e.g., `C:/path/to/data.yaml`).
    Even when using a file URL (e.g., `file:///C:/path/to/data.yaml`), we still face issues.
    With these code changes, Conftest can now successfully load files using a file URL (e.g., `file:///C:/path/to/data.yaml`).
    We opted for file URLs instead of paths with drive letters (e.g., `C:/path/to/data.yaml`) because OPA does not support file paths with drive letters. For more details, see [this issue comment](open-policy-agent/opa#6922 (comment)).

    Resolves: open-policy-agent#979

Signed-off-by: Punith C K <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Not able to access rego policies from other drive in windows
4 participants