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

format package unexpectedly mutating AST nodes #2439

Closed
tsandall opened this issue May 27, 2020 · 0 comments · Fixed by #2440
Closed

format package unexpectedly mutating AST nodes #2439

tsandall opened this issue May 27, 2020 · 0 comments · Fixed by #2440
Labels

Comments

@tsandall
Copy link
Member

tsandall commented May 27, 2020

The format.Ast function may mutate AST nodes to un-mangle wildcard variables. The problem is that this mutation/un-mangling is not applied consistently across multiple calls to format.Ast if the the inputs to format.Ast contain pointers to the same term strurcts. This problem can be reproduced by running the new opa build command with optimization enabled and multiple entrypoints specified:

x.rego

package test

p = x {
	input.b = x
}

q = y {
	input.a = y
}

run the build:

opa build x.rego -O=1 -e test/p -e test/-q -o bundle.tar.gz

inspect the output:

torin:~$ tar xzvf bundle.tar.gz
tar: Removing leading '/' from member names
x data.json
x optimized/test.1.rego
x optimized/test.rego
x .manifest
torin:~$ cat optimized/test.rego
package test

p = __wildcard0__ {
	input.b = _
}
torin:~$ cat optimized/test.1.rego
package test

q = __wildcard0__ {
	input.a = __wildcard0__
}

We run into this with opa build because the compiler only generates a single result term for all entrypoint PE runs--e.g., the compiler outputs rules like this:

p = $result { input.b = $result }

and

q = $result { input.a = $result }

Under the hood, the $result variable in the rule heads point to the same memory. The $result variable in the bodies point to different memory due to rewriting that happens on build. When the format package runs on one of the modules the $result variables are un-mangled to __wildcard0__ consistently--however, when the $result in the head is un-mangled, the term is mutated in place--this causes the term in the other module to get updated. When the format package runs on the other module, it only sees one occurrence of $result so it doesn't do any unmangling.

@tsandall tsandall added the bug label May 27, 2020
tsandall added a commit to tsandall/opa that referenced this issue May 29, 2020
As part of this change, also update the format package to unmangle the
variables slightly differently--just remove the wildcard prefix
instead of translating the variable names. This makes it easier to
tell where the variables came from in the first place and is a bit
less complicated.

Fixes open-policy-agent#2439

Signed-off-by: Torin Sandall <torinsandall@gmail.com>
patrick-east pushed a commit that referenced this issue May 29, 2020
As part of this change, also update the format package to unmangle the
variables slightly differently--just remove the wildcard prefix
instead of translating the variable names. This makes it easier to
tell where the variables came from in the first place and is a bit
less complicated.

Fixes #2439

Signed-off-by: Torin Sandall <torinsandall@gmail.com>
patrick-east pushed a commit to patrick-east/opa that referenced this issue Jun 1, 2020
As part of this change, also update the format package to unmangle the
variables slightly differently--just remove the wildcard prefix
instead of translating the variable names. This makes it easier to
tell where the variables came from in the first place and is a bit
less complicated.

Fixes open-policy-agent#2439

Signed-off-by: Torin Sandall <torinsandall@gmail.com>
(cherry picked from commit fe18f11)
This was referenced Jun 1, 2020
patrick-east pushed a commit that referenced this issue Jun 1, 2020
As part of this change, also update the format package to unmangle the
variables slightly differently--just remove the wildcard prefix
instead of translating the variable names. This makes it easier to
tell where the variables came from in the first place and is a bit
less complicated.

Fixes #2439

Signed-off-by: Torin Sandall <torinsandall@gmail.com>
(cherry picked from commit fe18f11)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
Archived in project
Development

Successfully merging a pull request may close this issue.

1 participant