-
Notifications
You must be signed in to change notification settings - Fork 1.3k
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
The [opa eval] command is unable to access Rego policies located on other drives on windows #6922
Conversation
…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]>
The new test appears to be causing failures on non-Windows platforms ( |
✅ Deploy Preview for openpolicyagent ready!
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]>
b00c103
to
02dc039
Compare
@philipaconrad I got to know we are supporting these inputs.
Added the support for below
My comments:
|
There was a problem hiding this 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
…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]>
Signed-off-by: pckvcode <[email protected]>
There was a problem hiding this 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 { |
There was a problem hiding this comment.
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
?
There was a problem hiding this comment.
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", |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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
The current behavior is to interpret the 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. 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. |
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]>
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: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:
/a/b/c/example.rego
)file:///path/to/file.json
)C:\a\b\c\example.rego
)Documentation Reference:
According to the official documentation, the expected values for the
--data
flag in theopa eval
command are:/a/b/c/example.rego
)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:
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.)
[opa eval] command after fix. (Success: opa in D drive and Successfully load rego policies from C drive.)
Unit Testcase pass screenshot. Function tested
@boranx Can you please review this change and share your comments? Thanks in advance