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

Upgrade to OPA v0.70.0 #3374

Merged
merged 1 commit into from
Jan 15, 2025
Merged

Upgrade to OPA v0.70.0 #3374

merged 1 commit into from
Jan 15, 2025

Conversation

Pushpalanka
Copy link
Collaborator

@Pushpalanka Pushpalanka commented Jan 13, 2025

Make use of OPA v0.70.0 version, addressing the breaking change.

Breaking change context
The EvalContext interface is added with new CreatePreparedQueryOnce function and the signature of envoyauth.Eval(ctx, opa, inputValue, result) method used by Skipper were changed with in OPA v0.70.0 as a result of open-policy-agent/opa-envoy-plugin#604.
This has been done to support a cleaner optimization we look forward to use with PreparedQuery(transforming the Rego query to an intermediate state that is reused with different inputs, removing the repeated steps of parsing and compiling) option in the future.

The interface change is absorbed with no functional addition and just addressing successful compilation.

Notes:

  • Until the pre-evaluation based optimization is properly benchmarked, it is not supported to be enabled via configs. Only the breaking change for compilation is addressed.

  • Styra is already on this version.

@Pushpalanka Pushpalanka added the dependencies Pull requests that update a dependency file label Jan 13, 2025
@@ -74,6 +75,8 @@ type OpenPolicyAgentRegistry struct {
bodyReadBufferSize int64

tracer opentracing.Tracer

preevaluationOptimization bool
Copy link
Collaborator

Choose a reason for hiding this comment

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

This needs to be removed

@MustafaSaber
Copy link
Member

Can you add in the PR description a small comment about the breaking change and what is done to fix it? I understand if I read code I can figure that out but would be also great to figure from commit message

@Pushpalanka
Copy link
Collaborator Author

Pushpalanka commented Jan 13, 2025

Can you add in the PR description a small comment about the breaking change and what is done to fix it? I understand if I read code I can figure that out but would be also great to figure from commit message

Addressed. 👍

@MustafaSaber
Copy link
Member

👍

go.mod Outdated Show resolved Hide resolved
@mjungsbluth
Copy link
Collaborator

The changes related to the OPA use LGTM 👍 , Alexander's comment should still be addressed and there seems to be a merge conflict.

@Pushpalanka Pushpalanka force-pushed the opa-v0.70.0 branch 2 times, most recently from 789e4ad to eac57bd Compare January 14, 2025 12:55
@AlexanderYastrebov
Copy link
Member

AlexanderYastrebov commented Jan 14, 2025

@Pushpalanka Please squash it into a single commit with a proper title and description. The thing is that build pipeline automatically creates release notes out of it.

The EvalContext interface is added with new CreatePreparedQueryOnce function and the signature of envoyauth.Eval(ctx, opa, inputValue, result) method used by Skipper were changed with in OPA v0.70.0 as a result of open-policy-agent/opa-envoy-plugin#604.
The interface change is absorbed with no functional addition and just addressing successful compilation.

Signed-off-by: Pushpalanka Jayawardhana <pushpalanka.jayawardhana@zalando.de>
@Pushpalanka
Copy link
Collaborator Author

@Pushpalanka Please squash it into a single commit with a proper title and description. The thing is that build pipeline automatically creates release notes out of it.

Addressed 👍

@AlexanderYastrebov
Copy link
Member

👍

@AlexanderYastrebov
Copy link
Member

@Pushpalanka Thank you

@Pushpalanka
Copy link
Collaborator Author

👍

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
dependencies Pull requests that update a dependency file
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants