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

Facilitate OPA decision correlation with business flows #3041

Open
wants to merge 14 commits into
base: master
Choose a base branch
from
Open

Facilitate OPA decision correlation with business flows #3041

wants to merge 14 commits into from
+106 −15

Conversation

JanardhanSharma
Copy link

@JanardhanSharma JanardhanSharma commented Apr 25, 2024 •
edited

Context

Problem

Currently there is no recommended way of correlating OPA decisions with execution flows. This leaves the OPA integrators to take different approaches which could carry security risks and break as the platform evolves.

Correlation is required by two main use cases,

  • Troubleshooting - If a particular flow fails, engineers are enabled to isolate the related OPA decisions and see if it impacted the failure.
  • Fine tune policies - To understand the impact of policy in relation to business events analysis the decision logs published to Data Lake.

Solution

Improve the Skipper filter opaAuthorizeRequest to inject decisionID as an input to the policy. Add a new attribute to the filterMetadata called open_policy_agent and under that add the decision_id.

{
   "input":{
      "attributes":{
         "metadataContext":{
            "filterMetadata":{
               "open_policy_agent":{
                  "decision_id":"xxx.xx.xxx.xxx"
               }
            }
         },
         "request":{
         }
      }
   }
}

Similar solution for Envoy exists: open-policy-agent/opa#6519
Ref: dynamic meta-data

Changes

Decision id is added to the request object and is available to be used in the policies.

@szuecs
Copy link
Member

szuecs commented Apr 26, 2024 •
edited

@JanardhanSharma please sign off your commits use -s, that's required for DCO.
The issue link was non public. Please copy the description to the public such that it's self contained.
Thanks!

@JanardhanSharma JanardhanSharma force-pushed the facilitate-OPA-decision-correlation-with-business-flows branch from d3e8c4c to 43f11eb Compare April 26, 2024 11:58
@szuecs szuecs added the minor no risk changes, for example new filters label Apr 26, 2024
Janardhan Sharma and others added 8 commits April 26, 2024 19:04
Facilitate OPA decision correlation with business flows
84ad4a0 
Signed-off-by: Janardhan Sharma <janardhan.sharma@zalando.de>
@JanardhanSharma
review comment
e5786f6 
Signed-off-by: Janardhan Sharma <janardhan.sharma@zalando.de>
@JanardhanSharma
review comment - import variable renamed
db3b2ea 
Signed-off-by: Janardhan Sharma <janardhan.sharma@zalando.de>
@mjungsbluth
OPA: Add tracing for outbound http calls (zalando#3034)
059e727 
Signed-off-by: Magnus Jungsbluth <magnus.jungsbluth@zalando.de>
Signed-off-by: Janardhan Sharma <janardhan.sharma@zalando.de>
@AlexanderYastrebov
Specify tracing span kind on creation (zalando#3039)
bb88e1f 
OpenTelemetry-OpenTracing bridge span kind can not be changed after creation,
see open-telemetry/opentelemetry-go#3953

The workaround is to specify span kind on creation which works for
both Open Tracing and Open Telemetry bridge spans.

Note that this change removes non-standard "shedder" kind from spans created by `admissionControl` filter.
Use operation name "admission_control" to query its spans instead if needed.

For zalando#2104

Signed-off-by: Alexander Yastrebov <alexander.yastrebov@zalando.de>
Signed-off-by: Janardhan Sharma <janardhan.sharma@zalando.de>
@AlexanderYastrebov
config: refactor test defaultConfig (zalando#3042)
bd738f9 
Make `defaultConfig` return configuration equal to one
created from default flags and modified by a helper function
for defining expected test case values.

Signed-off-by: Alexander Yastrebov <alexander.yastrebov@zalando.de>
Signed-off-by: Janardhan Sharma <janardhan.sharma@zalando.de>
@JanardhanSharma
Fix static analysis error
727c27f 
Signed-off-by: Janardhan Sharma <janardhan.sharma@zalando.de>
@JanardhanSharma
fix static analysis error. Signed-off-by: Janardhan Sharma <janardhan…
372e51e 
….sharma@outlook.com>

Signed-off-by: Janardhan Sharma <janardhan.sharma@zalando.de>
@JanardhanSharma JanardhanSharma force-pushed the facilitate-OPA-decision-correlation-with-business-flows branch from 65a1998 to 372e51e Compare April 26, 2024 17:07
@szuecs
Copy link
Member

szuecs commented Apr 29, 2024 

 --- FAIL: TestEval (0.10s)
panic: runtime error: invalid memory address or nil pointer dereference [recovered]
	panic: runtime error: invalid memory address or nil pointer dereference
[signal SIGSEGV: segmentation violation code=0x1 addr=0x0 pc=0x22b9568]

goroutine 257 [running]:
testing.tRunner.func1.2({0x2486180, 0x34b9810})
	/opt/hostedtoolcache/go/1.22.2/x64/src/testing/testing.go:1631 +0x3f7
testing.tRunner.func1()
	/opt/hostedtoolcache/go/1.22.2/x64/src/testing/testing.go:1634 +0x6b6
panic({0x2486180?, 0x34b9810?})
	/opt/hostedtoolcache/go/1.22.2/x64/src/runtime/panic.go:770 +0x132
github.com/zalando/skipper/filters/openpolicyagent.metaDataContextDoesNotExist(...)
	/home/runner/work/skipper/skipper/filters/openpolicyagent/evaluation.go:81
github.com/zalando/skipper/filters/openpolicyagent.setDecisionIdInRequest(0xc0006a7800, {0xc000047d80, 0x20})
	/home/runner/work/skipper/skipper/filters/openpolicyagent/evaluation.go:73 +0x68
github.com/zalando/skipper/filters/openpolicyagent.(*OpenPolicyAgentInstance).Eval(0xc00035ef00, {0x29db2d0, 0xc0006a77d0}, 0xc0006a7800)
	/home/runner/work/skipper/skipper/filters/openpolicyagent/evaluation.go:27 +0x2ac
github.com/zalando/skipper/filters/openpolicyagent.TestEval(0xc00071f380)
	/home/runner/work/skipper/skipper/filters/openpolicyagent/openpolicyagent_test.go:424 +0x3d5
testing.tRunner(0xc00071f380, 0x2764af8)
	/opt/hostedtoolcache/go/1.22.2/x64/src/testing/testing.go:1689 +0x21f
created by testing.(*T).Run in goroutine 1
	/opt/hostedtoolcache/go/1.22.2/x64/src/testing/testing.go:1742 +0x826

test faoilure fix and rename the import variable
eda16ca 
Signed-off-by: Janardhan Sharma <janardhan.sharma@zalando.de>
@JanardhanSharma
Copy link
Author

@szuecs Could you may be add @mjungsbluth as reviewer as well, please. He can have a quick look as well.

@JanardhanSharma
Copy link
Author 

 --- FAIL: TestEval (0.10s)
panic: runtime error: invalid memory address or nil pointer dereference [recovered]
	panic: runtime error: invalid memory address or nil pointer dereference
[signal SIGSEGV: segmentation violation code=0x1 addr=0x0 pc=0x22b9568]

goroutine 257 [running]:
testing.tRunner.func1.2({0x2486180, 0x34b9810})
	/opt/hostedtoolcache/go/1.22.2/x64/src/testing/testing.go:1631 +0x3f7
testing.tRunner.func1()
	/opt/hostedtoolcache/go/1.22.2/x64/src/testing/testing.go:1634 +0x6b6
panic({0x2486180?, 0x34b9810?})
	/opt/hostedtoolcache/go/1.22.2/x64/src/runtime/panic.go:770 +0x132
github.com/zalando/skipper/filters/openpolicyagent.metaDataContextDoesNotExist(...)
	/home/runner/work/skipper/skipper/filters/openpolicyagent/evaluation.go:81
github.com/zalando/skipper/filters/openpolicyagent.setDecisionIdInRequest(0xc0006a7800, {0xc000047d80, 0x20})
	/home/runner/work/skipper/skipper/filters/openpolicyagent/evaluation.go:73 +0x68
github.com/zalando/skipper/filters/openpolicyagent.(*OpenPolicyAgentInstance).Eval(0xc00035ef00, {0x29db2d0, 0xc0006a77d0}, 0xc0006a7800)
	/home/runner/work/skipper/skipper/filters/openpolicyagent/evaluation.go:27 +0x2ac
github.com/zalando/skipper/filters/openpolicyagent.TestEval(0xc00071f380)
	/home/runner/work/skipper/skipper/filters/openpolicyagent/openpolicyagent_test.go:424 +0x3d5
testing.tRunner(0xc00071f380, 0x2764af8)
	/opt/hostedtoolcache/go/1.22.2/x64/src/testing/testing.go:1689 +0x21f
created by testing.(*T).Run in goroutine 1
	/opt/hostedtoolcache/go/1.22.2/x64/src/testing/testing.go:1742 +0x826

fixed

mjungsbluth
mjungsbluth reviewed May 2, 2024
View reviewed changes
docs/tutorials/auth.md Outdated Show resolved Hide resolved
docs/tutorials/auth.md Outdated Show resolved Hide resolved
filters/openpolicyagent/evaluation.go Outdated Show resolved Hide resolved
filters/openpolicyagent/evaluation.go Outdated Show resolved Hide resolved
filters/openpolicyagent/evaluation.go Outdated Show resolved Hide resolved
filters/openpolicyagent/openpolicyagent_test.go Outdated Show resolved Hide resolved
review comments - refactored code and updated documentation
d2b0b31 
Signed-off-by: Janardhan Sharma <janardhan.sharma@zalando.de>
@szuecs
Copy link
Member

szuecs commented May 6, 2024 

taticcheck -checks "all,-ST1000,-ST1003,-ST1012,-ST1020,-ST1021" ./...
Error: filters/openpolicyagent/openpolicyagent_test.go:18:2: package "github.com/envoyproxy/go-control-plane/envoy/service/auth/v3" is being imported more than once (ST1019)
Error: 	filters/openpolicyagent/openpolicyagent_test.go:19:2: other import of "github.com/envoyproxy/go-control-plane/envoy/service/auth/v3"
make: *** [Makefile:138: staticcheck] Error 1
Error: Process completed with exit code 2.

duplicate import fixed
3c12e4b 
Signed-off-by: Janardhan Sharma <janardhan.sharma@zalando.de>
handled error
aef72d0 
Signed-off-by: Janardhan Sharma <janardhan.sharma@zalando.de>
review comments
3307de3 
Signed-off-by: Janardhan Sharma <janardhan.sharma@zalando.de>
@szuecs
Copy link
Member

szuecs commented May 14, 2024

👍

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Pushpalanka Pushpalanka Pushpalanka left review comments

@mjungsbluth mjungsbluth Awaiting requested review from mjungsbluth

@szuecs szuecs Awaiting requested review from szuecs

Assignees
No one assigned
Labels
minor no risk changes, for example new filters
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
5 participants
@JanardhanSharma @szuecs @Pushpalanka @mjungsbluth @AlexanderYastrebov