Re-introduce GetResponseHTTPHeaders method #667

Pushpalanka · 2025-02-11T20:09:11Z

Context:
Re-introduce the removed method from #628 using separate transform functions following this comment from @mjungsbluth.

Both signatures will be available to use as required,

func (result *EvalResult) GetResponseEnvoyHeaderValueOptions() ([]*ext_core_v3.HeaderValueOption, error) {
func (result *EvalResult) GetResponseHTTPHeaders() (http.Header, error) {

Signed-off-by: Pushpalanka Jayawardhana <[email protected]>

mjungsbluth · 2025-02-12T14:22:02Z

envoyauth/response.go

+
+	takeResponseHeaders := func(headers map[string]interface{}) error {
+		for key, value := range headers {
+			switch values := value.(type) {


This seems to be a structural duplicate to the transformation for the envoy HttpOption. Do you want to unify this more?

Yes indeed. Addressed.

What I meant is that this switch statement in the for loop is essentially the same as in makeEnvoyHeaderValueOptionsFromHeadersMap so you could model this with a transformer + collector function and then the same for loop and structural swich could be used...

Thanks for clarifying. I hope it is there now.

… methods Signed-off-by: Pushpalanka Jayawardhana <[email protected]>

Signed-off-by: Pushpalanka Jayawardhana <[email protected]>

mjungsbluth · 2025-02-18T09:31:38Z

envoyauth/response.go

 		case []string:
-			hvo = slices.Grow(hvo, len(val))


Removing this might yield a small performance hit as the array will need to grow for each sub-element in the Envoy case.

uses a pre allocation function to reduce the performance overhead when it's done along with append. Signed-off-by: Pushpalanka Jayawardhana <[email protected]>

mjungsbluth · 2025-02-19T06:32:59Z

envoyauth/response.go

 	if *result == nil {
-		*result = make(http.Header)
+		*result = make(http.Header, additional)


I think this grows the map itself and not the values for a specific key.
Maybe that is also not easily possiblr

Yes, that's true. Will ignore the additional value, but keep for the function signature to match.

Signed-off-by: Pushpalanka Jayawardhana <[email protected]>

mjungsbluth · 2025-02-19T10:22:24Z

envoyauth/response_test.go

@@ -455,6 +455,127 @@ func TestGetResponseHeaders(t *testing.T) {
 	if len(result) != 2 {
 		t.Fatalf("Expected two header but got %v", len(result))
 	}
+	//


Commented code to be removed?

Mistake, will uncomment it. Introduced it to valid the case of []interface{} for header values.

mjungsbluth · 2025-02-19T10:28:18Z

LGTM in general!

regeda · 2025-02-19T10:32:02Z

envoyauth/response.go

+
+	switch v := val.(type) {
+	case map[string]interface{}:
+		return []map[string]interface{}{v}, nil


Please, avoid additional allocations. We have a highload service based on OPA where each allocation drastically impacts the performance.

regeda · 2025-02-19T10:33:48Z

envoyauth/response.go

+	return getHeadersWithTransformation(result.Decision, fieldName, preallocateForHTTPHeaders, collectHTTPHeaders)
+}
+
+func getHeadersWithTransformation[T any](


IMHO, a new approach looks a bit complicated and not easy to follow the logic.

Could you make just a function which converts the result from getHeadersFromDecision into http.Header?

Suggested change

func getHeadersWithTransformation[T any](

func (result *EvalResult) getHTTPHeadersFromDecision(fieldName string) (http.Header, error) {

opts, err := result.getHeadersFromDecision(fieldName)

if err != nil {

return nil, err

}

header := make(http.Header, len(opts))

for _, opt = range opts {

header.Add(opt.GetHeader().GetKey(), opt.GetHeader().GetValue())

}

return header, nil

}

It's is an effort to balance complexity and performance for both cases of returning HeaderValueOption and http.Header.
This suggested approach introduces an unnecessary intermediate conversion to ext_core_v3.HeaderValueOption for http.Header returning case affecting performance in that scenario.
This concern directs us towards the implemented approach which keep both the cases optimized for highload services.

Could you introduce the benchmark of getHeadersFromDecision before and after your refactoring?

Moreover, you already made an intermediate conversion using extractHeadersFromDecision function.

@regeda I respect your requirements, but it's probably better to follow up with PRs of your own to improve performance where needed rather than making requests for that in others contributions.

I'm kind of responsible for this breaking change.

I would help to set it back. PTAL #668

That's fair! Let's have this merged and follow up with additions based off of this.

Also, if you're interested in reducing allocations, you'll find a lot to like in OPA v1.2.0 🙂

Signed-off-by: Pushpalanka Jayawardhana <[email protected]>

regeda · 2025-02-19T12:30:13Z

envoyauth/response.go

+	return getHeadersWithTransformation(result.Decision, fieldName, preallocateForHTTPHeaders, collectHTTPHeaders)
+}
+
+func getHeadersWithTransformation[T any](


Moreover, you already made an intermediate conversion using extractHeadersFromDecision function.

srenatus

Thanks! LGTM 🎈

srenatus · 2025-02-19T12:54:26Z

envoyauth/response.go

@@ -4,9 +4,6 @@ import (
 	"context"
 	"encoding/json"
 	"fmt"
-	"net/http"
-	"slices"


I kind of appreciated the blocks here -- stdlib vs third-party. but that's not a blocker.

Pushpalanka added 3 commits February 11, 2025 21:02

Re-introduce GetResponseHTTPHeaders method

cc945d3

Signed-off-by: Pushpalanka Jayawardhana <[email protected]>

Align the error message to existed value

6beba6e

Signed-off-by: Pushpalanka Jayawardhana <[email protected]>

Add test

513c371

Signed-off-by: Pushpalanka Jayawardhana <[email protected]>

Pushpalanka marked this pull request as ready for review February 12, 2025 08:45

Merge branch 'main' into gethttpheaders

187f9ad

mjungsbluth reviewed Feb 12, 2025

View reviewed changes

Further unify transformHeadersToEnvoy and transformToHTTPHeaderFormat…

0b39891

… methods Signed-off-by: Pushpalanka Jayawardhana <[email protected]>

Pushpalanka requested a review from mjungsbluth February 13, 2025 18:42

Address review comments

0d64276

Signed-off-by: Pushpalanka Jayawardhana <[email protected]>

Pushpalanka force-pushed the gethttpheaders branch from 9ef00b5 to 9951c4e Compare February 17, 2025 21:53

Reduce cognitive complexity of transformation function

af9dbb4

Signed-off-by: Pushpalanka Jayawardhana <[email protected]>

Pushpalanka force-pushed the gethttpheaders branch from 9951c4e to af9dbb4 Compare February 17, 2025 22:01

mjungsbluth reviewed Feb 18, 2025

View reviewed changes

Keep slice.grow benefit in performance

27017cd

uses a pre allocation function to reduce the performance overhead when it's done along with append. Signed-off-by: Pushpalanka Jayawardhana <[email protected]>

mjungsbluth reviewed Feb 19, 2025

View reviewed changes

Remove incorrect size allocation for http.Header

a36e69e

Signed-off-by: Pushpalanka Jayawardhana <[email protected]>

mjungsbluth reviewed Feb 19, 2025

View reviewed changes

regeda suggested changes Feb 19, 2025

View reviewed changes

Introduce test for the case []interface{} of header values.

f0fdb84

Signed-off-by: Pushpalanka Jayawardhana <[email protected]>

regeda suggested changes Feb 19, 2025

View reviewed changes

anderseknert approved these changes Feb 19, 2025

View reviewed changes

srenatus approved these changes Feb 19, 2025

View reviewed changes

anderseknert merged commit a41ef12 into open-policy-agent:main Feb 19, 2025
8 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Re-introduce GetResponseHTTPHeaders method #667

Re-introduce GetResponseHTTPHeaders method #667

Pushpalanka commented Feb 11, 2025

mjungsbluth Feb 12, 2025

Pushpalanka Feb 13, 2025

mjungsbluth Feb 14, 2025

Pushpalanka Feb 17, 2025

mjungsbluth Feb 18, 2025

mjungsbluth Feb 19, 2025

Pushpalanka Feb 19, 2025

mjungsbluth Feb 19, 2025

Pushpalanka Feb 19, 2025

mjungsbluth commented Feb 19, 2025

regeda Feb 19, 2025

regeda Feb 19, 2025 •

edited

Loading

Pushpalanka Feb 19, 2025

regeda Feb 19, 2025

regeda Feb 19, 2025

anderseknert Feb 19, 2025

regeda Feb 19, 2025

anderseknert Feb 19, 2025

anderseknert Feb 19, 2025

regeda Feb 19, 2025

srenatus left a comment

srenatus Feb 19, 2025

-func getHeadersWithTransformation[T any](
+func (result *EvalResult) getHTTPHeadersFromDecision(fieldName string) (http.Header, error) {
+	opts, err := result.getHeadersFromDecision(fieldName)
+	if err != nil {
+		return nil, err
+	}
+	header := make(http.Header, len(opts))
+	for _, opt = range opts {
+		header.Add(opt.GetHeader().GetKey(), opt.GetHeader().GetValue())
+	}
+	return header, nil
+}

Re-introduce GetResponseHTTPHeaders method #667

Re-introduce GetResponseHTTPHeaders method #667

Conversation

Pushpalanka commented Feb 11, 2025

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mjungsbluth commented Feb 19, 2025

Choose a reason for hiding this comment

regeda Feb 19, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

srenatus left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

regeda Feb 19, 2025 •

edited

Loading