-
Notifications
You must be signed in to change notification settings - Fork 375
Eopa decision_log plugin with open policy agent #3739
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
base: master
Are you sure you want to change the base?
Conversation
Deployment ChecklistThis change falls under the deployment policy. 💁 Since Nov 10th, we are in the RED deployment zone. This means all changes released to production must adhere to the following requirements:
👉 Regardless of which boxes you click in this comment, merge/deployment will not be blocked. |
162084c to
46f3c03
Compare
Use commits from cw2025 such that the labeler works during code freeze ```` % git cherry-pick e23559c [gh-action/labeler c4dc064] automate: pr label cw2025 for convenience (zalando#3724) Date: Mon Nov 10 22:28:21 2025 +0100 1 file changed, 7 insertions(+) create mode 100644 .github/labeler.yml % git cherry-pick 5192763 [gh-action/labeler fdba813] fix: yaml indentation in labeler.yml (zalando#3732) Date: Fri Nov 14 19:54:55 2025 +0100 1 file changed, 6 insertions(+), 2 deletions(-) ```` --------- Signed-off-by: Sandor Szücs <[email protected]> Signed-off-by: nuwandi-wickramasinghe_zse <[email protected]>
Signed-off-by: nuwandi-wickramasinghe_zse <[email protected]>
Signed-off-by: nuwandi-wickramasinghe_zse <[email protected]>
Signed-off-by: nuwandi-wickramasinghe_zse <[email protected]>
Signed-off-by: nuwandi-wickramasinghe_zse <[email protected]>
follow up on zalando#3737 Signed-off-by: Mustafa Abdelrahman <[email protected]> Signed-off-by: nuwandi-wickramasinghe_zse <[email protected]>
Signed-off-by: nuwandi-wickramasinghe_zse <[email protected]>
Signed-off-by: nuwandi-wickramasinghe_zse <[email protected]>
# Conflicts: # .github/labeler.yml # .github/workflows/labeler.yaml
Pushpalanka
left a comment
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.
Added few thoughts.
filters/openpolicyagent/opaauthorizerequest/opaauthorizerequest_test.go
Outdated
Show resolved
Hide resolved
filters/openpolicyagent/opaserveresponse/opaserveresponse_test.go
Outdated
Show resolved
Hide resolved
filters/openpolicyagent/opaauthorizerequest/opaauthorizerequest_test.go
Outdated
Show resolved
Hide resolved
| "github.com/open-policy-agent/opa/v1/plugins" | ||
| ) | ||
|
|
||
| func All() map[string]plugins.Factory { |
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 really only cherry picks one plugin from eopa, when I look at
https://github.com/open-policy-agent/eopa/blob/7b4e2ef7a83a452ccbb24c697867dd49ffd3d88e/pkg/sdk/sdk.go#L22
We are leaving out quite a few things, so the flag announces sth that is a bit overselling what is actually done (enabling S3 uploads)
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.
Will make the flag more specific.
The initial idea was to enable extra plugins which are defined here https://github.com/open-policy-agent/eopa/blob/main/pkg/plugins/all.go#L17 with the flag. Hence the name enable-enterprise-open-policy-agent-plugins. But we have our early implementation of the envoy plugin and the data plugin had a conflict with our body parse filter.
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.
If you look at the link I shared there is more than the plugins but also some hooks.
If you‘re cherrypicking just the s3 plugin you might as well just add it always. It will not be active without configuration.
what specifically is the „conflict“ with the body parsing?
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.
Yes, the PR completely focuses on S3 decision log functionality even though there are more features provided with eopa. Currently working on a POC to see how we can switch to S3 decision logging and how to roll this out in Zalando as this will be time critical.
So we can either skip the flag or check on introducing all additional eopa features with a flag?
The new data plugin package "github.com/open-policy-agent/eopa/pkg/plugins/data" changed the unmarshalling behavior of the of the json body, resulting in a broken json body causing 500 response code. This test fails if the new package is imported in the project. The error thrown from request body parsing differs with the new data package. Could not figure out why without spending more time on it. AFAIU with or without the new data package, the same unmarshalling logic from the same version of open-policy-agent/opa/v1/util should be used. I will create an issue to track this.
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.
I removed the flag as it is a harmless plugin with no impact unless we change the configuration.
Created https://github.com/zalando-infosec/corporate-iam-issues/issues/1395 to enable all eopa capabilities
Signed-off-by: nuwandi-wickramasinghe_zse <[email protected]>
| var bootConfig map[string]any | ||
| err = util.Unmarshal(configBytes, &bootConfig) | ||
| if err != nil { | ||
| return nil, err | ||
| } |
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 part is not exactly related to enabling the eopa_dl plugin.
It seems unless we add discovery.BootConfig function during the discovery initialization, the decision_log or plugins configuration provided in the opaconfig.yaml is not effective.
As we are planning to work with both bootstrapped and discovered configuration (Possible from opa v0.64.0 onwards) we will need this.
If I did not miss anything, This configuration we have for the clusters is not currently doing anything and will start to make an impact once this change is merged.
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.
so regardless the decision log config will be overwritten by Styra DAS correct?
So to test it in integration we would need to change the discovery config?
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.
With the current filter implementation, yes.
This code change takes away the requirement to change the discovery config, by passing the bootstrap config into the discovery initialization (at line 703)
Signed-off-by: nuwandi-wickramasinghe_zse <[email protected]>
# Conflicts: # go.mod # go.sum
Update EOPA dependency to the latest commit from the decision_logs branch (a55b2111d4d7c0c832cbd4b22d42dfe2b38e3e4a) which includes the latest decision logs functionality and improvements. 🤖 Generated with Claude Code Co-Authored-By: Claude <[email protected]> Signed-off-by: nuwandi-wickramasinghe_zse <[email protected]>
Enterprise OPA or eopa (now donated to the OPA community) provides an improved behavior of decision log export with a plugin.
This PR introduces eopa_dl plugin into the open policy agent instance and makes sure the new plugin works with the existing opaconfig and discovery config. To use the new decision login feature, we need modifications in the configuration which is outside of this repo.
With the current opa and discovery configs, there will not be any functional changes.