feat: support query_parameters_to_set in response #663
Conversation
sedovmik
force-pushed
the
feat/query-set
branch
2 times, most recently
from
517ba53 to
d4ddfb9
Compare
internal/internal_test.go
Outdated
|
|
||
| default allow = true | ||
|
|
||
| query_parameters_to_set := [ |
There was a problem hiding this comment.
Great contribution!
Could you consider the following API:
query_parameters_to_set := {
"foo": "value1",
"bar": "value2",
}
It leads to less iterations and less memory allocations.
There was a problem hiding this comment.
Thank you for the comment! I agree that the format could be simplified. I intended to keep it aligned with Envoy's config.core.v3.QueryParameter for implementation simplicity, but your suggestion is indeed more ergonomic.
Regarding multi-value parameters - In HTTP, it's possible to have multiple values for the same query parameter (e.g., ?foo=value1&foo=value2). To properly support this, I could add support for both value types (string and array of strings):
query_parameters_to_set := {
"foo": "value1", # single value
"bar": ["value1", "value2"] # multiple values
}What are your thoughts? I'd be happy to update the implementation accordingly.
There was a problem hiding this comment.
I fully support both value types.
Add support for query_parameters_to_set field in response to allow setting/overwriting query parameters before sending upstream. Similar to existing query_parameters_to_remove support. Signed-off-by: Mikhail Sedov <[email protected]>
sedovmik
force-pushed
the
feat/query-set
branch
from
d4ddfb9 to
ed4bd47
Compare
Change query_parameters_to_set to use a map format. The format
supports both single values and arrays for setting multiple
query parameters with the same key.
Example usage in OPA policies:
```rego
package envoy.authz
default allow = true
query_parameters_to_set = {
"user_id": "123",
"tenant": "acme"
}
query_parameters_to_set = {
"role": ["admin", "user"],
"scope": ["read", "write"]
}
query_parameters_to_set = {
"user_id": "123",
"role": ["admin", "user"]
}
result["allowed"] = allow
result["query_parameters_to_set"] = query_parameters_to_set
```
Signed-off-by: Mikhail Sedov <[email protected]>
sedovmik
force-pushed
the
feat/query-set
branch
from
ed4bd47 to
4e99048
Compare
| return nil, fmt.Errorf("type assertion error, expected query_parameters_to_set to be a map but got '%T'", val) | ||
| } | ||
|
|
||
| var result []*ext_core_v3.QueryParameter |
There was a problem hiding this comment.
Could you preallocate the capacity to improve the performance?
| var result []*ext_core_v3.QueryParameter | |
| result := make([]*ext_core_v3.QueryParameter, 0, len(params)) |
| Value: v, | ||
| }) | ||
| case []interface{}: | ||
| for _, item := range v { |
There was a problem hiding this comment.
Here you also could grow the slice using slices.Grow:
| for _, item := range v { | |
| result = slices.Grow(result, len(v)) | |
| for _, item := range v { |
|
Please add this feature to the official OPA documentation (like open-policy-agent/opa#7241) |
Add support for query_parameters_to_set field in response to allow setting/overwriting query parameters before sending upstream. Similar to existing query_parameters_to_remove support.
Example usage in OPA policies: