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.

Sign up for GitHub

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

Jump to bottom

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

Closed
wants to merge 5 commits into from
Closed

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

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
3a9a702
The [opa eval] command is unable to access Rego policies located on o…
pckvcode Aug 12, 2024
02dc039
Opa should be capable of accessing Rego policies from the following i…
pckvcode Aug 21, 2024
9601e7e
Merge branch 'main' into access-rego-policies-from-other-windows-drives
pckvcode Aug 22, 2024
b2f071f
Adding unit tests to support loading Rego policies from paths that in…
pckvcode Aug 23, 2024
035070d
Fixing lint in loader_test.go
pckvcode Aug 23, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 3 additions & 1 deletion loader/loader.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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}} |
+------------------+

return strings.Split(parts[0], "."), parts[1]
}
}
return nil, path
}
Expand Down
29 changes: 24 additions & 5 deletions loader/loader_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,7 @@ import (
"path"
"path/filepath"
"reflect"
"runtime"
"sort"
"strings"
"testing"
Expand Down Expand Up @@ -808,15 +809,19 @@ func TestLoadFileURL(t *testing.T) {
files := map[string]string{
"/a/a/1.json": `1`, // this will load as a directory (e.g., file://a/a)
"b.json": `{"b": 2}`, // this will load as a normal file
"c.json": `3`, // this will loas as rooted file
"c.json": `3`, // this will load as rooted file
"d.json": `{"d": 4}`, // this will load as a normal file without the prefix file:///
}
test.WithTempFS(files, func(rootDir string) {

paths := mustListPaths(rootDir, false)[1:]
sort.Strings(paths)

for i := range paths {
paths[i] = "file://" + paths[i]
fileURLPrefix := "file://"
if IsWindows() {
fileURLPrefix = "file:///"
}
for i := range paths[:3] {
paths[i] = fileURLPrefix + paths[i]
}

paths[2] = "c:" + paths[2]
Expand All @@ -826,13 +831,18 @@ func TestLoadFileURL(t *testing.T) {
t.Fatal(err)
}

exp := parseJSON(`{"a": 1, "b": 2, "c": 3}`)
exp := parseJSON(`{"a": 1, "b": 2, "c": 3, "d":4}`)
if !reflect.DeepEqual(exp, result.Documents) {
t.Fatalf("Expected %v but got %v", exp, result.Documents)
}
})
}

// IsWindows: checks if the user's OS is Windows
func IsWindows() bool {
return runtime.GOOS == "windows"
}

func TestUnsupportedURLScheme(t *testing.T) {
_, err := NewFileLoader().All([]string{"http://openpolicyagent.org"})
if err == nil || !strings.Contains(err.Error(), "unsupported URL scheme: http://openpolicyagent.org") {
Expand Down Expand Up @@ -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

},
{
input: "c:file:///c:/a/b/c",
wantParts: []string{"c"},
wantPath: "file:///c:/a/b/c",
},
}

for _, tc := range tests {
Expand Down