Revert "Facilitate OPA decision correlation with business flows (#3041)"

This reverts commit 2a623a9. The code panics with: ``` fatal error: concurrent map writes goroutine 191993 [running]: github.com/zalando/skipper/filters/openpolicyagent.setDecisionIdInRequest(0x4004494720, {0x40067c6440?, 0x10?}) github.com/zalando/skipper/filters/openpolicyagent/evaluation.go:92 +0xe4 github.com/zalando/skipper/filters/openpolicyagent.(*OpenPolicyAgentInstance).Eval(0x40059ef400, {0x16b6bb0, 0x40044946c0}, 0x4004494720) github.com/zalando/skipper/filters/openpolicyagent/evaluation.go:27 +0x164 github.com/zalando/skipper/filters/openpolicyagent/opaauthorizerequest.(*opaAuthorizeRequestFilter).Request(0x4002234520, {0x16ccd90, 0x40091ca100}) github.com/zalando/skipper/filters/openpolicyagent/opaauthorizerequest/opaauthorizerequest.go:131 +0x1a4 ``` Signed-off-by: Alexander Yastrebov <[email protected]>
zalando · Jul 2, 2024 · 9c3255d · 9c3255d
1 parent d55bbf4
commit 9c3255d
Show file tree

Hide file tree

Showing 4 changed files with 15 additions and 106 deletions.
diff --git a/docs/tutorials/auth.md b/docs/tutorials/auth.md
@@ -466,15 +466,6 @@ The second argument is parsed as YAML, cannot be nested and values need to be st
 
 In Rego this can be used like this `input.attributes.contextExtensions["com.mycompany.myprop"] == "my value"`
 
-### Decision ID in Policies
-
-Each evaluation yields a distinct decision, identifiable by its unique decision ID.
-This decision ID can be located within the input at:
-
-`input.attributes.metadataContext.filterMetadata.open_policy_agent.decision_id`
-
-Typical use cases are either propagation of the decision ID to downstream systems or returning it as part of the response. As an example this can allow to trouble shoot deny requests by looking up details using the full decision in a control plane.
-
 ### Quick Start Rego Playground
 
 A quick way without setting up Backend APIs is to use the [Rego Playground](https://play.openpolicyagent.org/).

diff --git a/filters/openpolicyagent/evaluation.go b/filters/openpolicyagent/evaluation.go
@@ -3,7 +3,8 @@ package openpolicyagent
 import (
 	"context"
 	"fmt"
-	ext_authz_v3_core "github.com/envoyproxy/go-control-plane/envoy/config/core/v3"
+	"time"
+
 	ext_authz_v3 "github.com/envoyproxy/go-control-plane/envoy/service/auth/v3"
 	"github.com/open-policy-agent/opa-envoy-plugin/envoyauth"
 	"github.com/open-policy-agent/opa-envoy-plugin/opa/decisionlog"
@@ -12,8 +13,6 @@ import (
 	"github.com/open-policy-agent/opa/server"
 	"github.com/open-policy-agent/opa/topdown"
 	"github.com/opentracing/opentracing-go"
-	pbstruct "google.golang.org/protobuf/types/known/structpb"
-	"time"
 )
 
 func (opa *OpenPolicyAgentInstance) Eval(ctx context.Context, req *ext_authz_v3.CheckRequest) (*envoyauth.EvalResult, error) {
@@ -24,12 +23,6 @@ func (opa *OpenPolicyAgentInstance) Eval(ctx context.Context, req *ext_authz_v3.
 		return nil, err
 	}
 
-	err = setDecisionIdInRequest(req, decisionId)
-	if err != nil {
-		opa.Logger().WithFields(map[string]interface{}{"err": err}).Error("Unable to set decision ID in Request.")
-		return nil, err
-	}
-
 	result, stopeval, err := envoyauth.NewEvalResult(withDecisionID(decisionId))
 	if err != nil {
 		opa.Logger().WithFields(map[string]interface{}{"err": err}).Error("Unable to generate new result with decision ID.")
@@ -78,29 +71,6 @@ func (opa *OpenPolicyAgentInstance) Eval(ctx context.Context, req *ext_authz_v3.
 	return result, nil
 }
 
-func setDecisionIdInRequest(req *ext_authz_v3.CheckRequest, decisionId string) error {
-	if req.Attributes.MetadataContext == nil {
-		req.Attributes.MetadataContext = &ext_authz_v3_core.Metadata{
-			FilterMetadata: map[string]*pbstruct.Struct{},
-		}
-	}
-
-	filterMeta, err := formOpenPolicyAgentMetaDataObject(decisionId)
-	if err != nil {
-		return err
-	}
-	req.Attributes.MetadataContext.FilterMetadata["open_policy_agent"] = filterMeta
-	return nil
-}
-
-func formOpenPolicyAgentMetaDataObject(decisionId string) (*pbstruct.Struct, error) {
-
-	innerFields := make(map[string]interface{})
-	innerFields["decision_id"] = decisionId
-
-	return pbstruct.NewStruct(innerFields)
-}
-
 func (opa *OpenPolicyAgentInstance) logDecision(ctx context.Context, input interface{}, result *envoyauth.EvalResult, err error) error {
 	info := &server.Info{
 		Timestamp: time.Now(),

diff --git a/filters/openpolicyagent/opaauthorizerequest/opaauthorizerequest_test.go b/filters/openpolicyagent/opaauthorizerequest/opaauthorizerequest_test.go
@@ -255,21 +255,6 @@ func TestAuthorizeRequestFilter(t *testing.T) {
 			backendHeaders:  make(http.Header),
 			removeHeaders:   make(http.Header),
 		},
-		{
-			msg:             "Decision id in request header",
-			filterName:      "opaAuthorizeRequest",
-			bundleName:      "somebundle.tar.gz",
-			regoQuery:       "envoy/authz/allow_object_decision_id_in_header",
-			requestMethod:   "POST",
-			body:            `{ "target_id" : "123456" }`,
-			requestHeaders:  map[string][]string{"content-type": {"application/json"}},
-			requestPath:     "/allow/structured",
-			expectedStatus:  http.StatusOK,
-			expectedBody:    "Welcome!",
-			expectedHeaders: map[string][]string{"Decision-Id": {"some-random-decision-id-generated-during-evaluation"}},
-			backendHeaders:  make(http.Header),
-			removeHeaders:   make(http.Header),
-		},
 		{
 			msg:               "Invalid UTF-8 in Path",
 			filterName:        "opaAuthorizeRequest",
@@ -360,21 +345,8 @@ func TestAuthorizeRequestFilter(t *testing.T) {
 
 						allow_body {
 							input.parsed_body.target_id == "123456"
-						}	
-						
-						decision_id := input.attributes.metadataContext.filterMetadata.open_policy_agent.decision_id
-
-						allow_object_decision_id_in_header = response {
-						    input.parsed_path = ["allow", "structured"]
-						    decision_id 
-						    response := {
-						        "allowed": true,
-						        "response_headers_to_add": {
-						            "decision-id": decision_id
-						        }
-						    }
-						}
-						`,
+						}						
+					`,
 				}),
 			)
 
@@ -402,24 +374,10 @@ func TestAuthorizeRequestFilter(t *testing.T) {
 				}
 			}`, opaControlPlane.URL(), ti.regoQuery))
 
-			envoyMetaDataConfig := []byte(`{
-				"filter_metadata": {
-					"envoy.filters.http.header_to_metadata": {
-						"policy_type": "ingress"
-					}
-				}
-			}`)
-
-			opts := make([]func(*openpolicyagent.OpenPolicyAgentInstanceConfig) error, 0)
-			opts = append(opts,
-				openpolicyagent.WithConfigTemplate(config),
-				openpolicyagent.WithEnvoyMetadataBytes(envoyMetaDataConfig))
-
 			opaFactory := openpolicyagent.NewOpenPolicyAgentRegistry(openpolicyagent.WithTracer(&tracingtest.Tracer{}))
-			ftSpec := NewOpaAuthorizeRequestSpec(opaFactory, opts...)
-
+			ftSpec := NewOpaAuthorizeRequestSpec(opaFactory, openpolicyagent.WithConfigTemplate(config))
 			fr.Register(ftSpec)
-			ftSpec = NewOpaAuthorizeRequestWithBodySpec(opaFactory, opts...)
+			ftSpec = NewOpaAuthorizeRequestWithBodySpec(opaFactory, openpolicyagent.WithConfigTemplate(config))
 			fr.Register(ftSpec)
 
 			r := eskip.MustParse(fmt.Sprintf(`* -> %s("%s", "%s") %s -> "%s"`, ti.filterName, ti.bundleName, ti.contextExtensions, ti.extraeskip, clientServer.URL))
@@ -477,12 +435,8 @@ func isHeadersPresent(t *testing.T, expectedHeaders http.Header, headers http.He
 		if !headerFound {
 			return false
 		}
-		// since decision id is randomly generated we are just checking for not nil
-		if headerName == "Decision-Id" {
-			assert.NotNil(t, actualValues)
-		} else {
-			assert.ElementsMatch(t, expectedValues, actualValues)
-		}
+
+		assert.ElementsMatch(t, expectedValues, actualValues)
 	}
 	return true
 }

diff --git a/filters/openpolicyagent/openpolicyagent_test.go b/filters/openpolicyagent/openpolicyagent_test.go
@@ -29,7 +29,7 @@ import (
 	"github.com/zalando/skipper/routing"
 	"github.com/zalando/skipper/tracing/tracingtest"
 	"google.golang.org/protobuf/encoding/protojson"
-	pbstruct "google.golang.org/protobuf/types/known/structpb"
+	_struct "google.golang.org/protobuf/types/known/structpb"
 )
 
 type MockOpenPolicyAgentFilter struct {
@@ -63,22 +63,22 @@ func TestInterpolateTemplate(t *testing.T) {
 
 func TestLoadEnvoyMetadata(t *testing.T) {
 	cfg := &OpenPolicyAgentInstanceConfig{}
-	_ = WithEnvoyMetadataBytes([]byte(`
+	WithEnvoyMetadataBytes([]byte(`
 	{
 		"filter_metadata": {
 			"envoy.filters.http.header_to_metadata": {
 				"policy_type": "ingress"
-			},
+			}
 		}
 	}
 	`))(cfg)
 
 	expectedBytes, err := protojson.Marshal(&ext_authz_v3_core.Metadata{
-		FilterMetadata: map[string]*pbstruct.Struct{
+		FilterMetadata: map[string]*_struct.Struct{
 			"envoy.filters.http.header_to_metadata": {
-				Fields: map[string]*pbstruct.Value{
+				Fields: map[string]*_struct.Value{
 					"policy_type": {
-						Kind: &pbstruct.Value_StringValue{StringValue: "ingress"},
+						Kind: &_struct.Value_StringValue{StringValue: "ingress"},
 					},
 				},
 			},
@@ -411,13 +411,7 @@ func TestEval(t *testing.T) {
 	span := tracer.StartSpan("open-policy-agent")
 	ctx := opentracing.ContextWithSpan(context.Background(), span)
 
-	result, err := inst.Eval(ctx, &authv3.CheckRequest{
-		Attributes: &authv3.AttributeContext{
-			Request:           nil,
-			ContextExtensions: nil,
-			MetadataContext:   nil,
-		},
-	})
+	result, err := inst.Eval(ctx, &authv3.CheckRequest{})
 	assert.NoError(t, err)
 
 	allowed, err := result.IsAllowed()