Skip to content

Conversation

@sedovmik
Copy link
Contributor

@sedovmik sedovmik commented Feb 6, 2025

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:

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

@sedovmik sedovmik force-pushed the feat/query-set branch 2 times, most recently from 517ba53 to d4ddfb9 Compare February 6, 2025 00:24
@ashutosh-narkar ashutosh-narkar requested a review from tjons February 6, 2025 22:05
default allow = true
query_parameters_to_set := [
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I fully support both value types.

@sedovmik sedovmik force-pushed the feat/query-set branch 2 times, most recently from ed4bd47 to 4e99048 Compare February 13, 2025 13:28
@regeda
Copy link
Contributor

regeda commented Feb 19, 2025

Please add this feature to the official OPA documentation (like open-policy-agent/opa#7241)

@sedovmik
Copy link
Contributor Author

sedovmik commented Mar 2, 2025

@regeda Thank you for the review. The documentation PR is available here: open-policy-agent/opa#7405

@sedovmik sedovmik force-pushed the feat/query-set branch 2 times, most recently from 5ff5386 to a33fbe3 Compare March 6, 2025 07:07
@sedovmik
Copy link
Contributor Author

sedovmik commented Mar 7, 2025

@tjons @ashutosh-narkar

I rework the change with all @regeda's suggestions, including the docs update in the main repo. Everything is ready for another look when you get a chance.

@ashutosh-narkar
Copy link
Member

including the docs update in the main repo.

Thanks for the contribution. You'll need to update this page. Is that what you're referring to?

@sedovmik
Copy link
Contributor Author

sedovmik commented Mar 7, 2025

You'll need to update this page.

Yes, PR for documentation is ready: open-policy-agent/opa#7405

@ashutosh-narkar
Copy link
Member

Can you please squash your commits and we can get this in. Thanks!

Add support for query_parameters_to_set field in response to allow
setting/overwriting query parameters before sending upstream.

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

```

Co-authored-by: Anthony Regeda <[email protected]>
Signed-off-by: Mikhail Sedov <[email protected]>
@ashutosh-narkar ashutosh-narkar merged commit 25f3884 into open-policy-agent:main Mar 7, 2025
8 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants