ext_authz: added support for the append_action in gRPC ExtAuthz

[agrawroh]

Description

This PR adds support for the append_action enum in gRPC ExtAuthz OkHttpResponse.headers for upstream request header mutations.

Today we only support the deprecated append. After this change, APPEND_IF_EXISTS_OR_ADD, ADD_IF_ABSENT, OVERWRITE_IF_EXISTS, and OVERWRITE_IF_EXISTS_OR_ADD actions would also be supported and will provide parity with response_headers_to_add handling.

Fix #42857

---

Commit Message: ext_authz: added support for the append_action in gRPC ExtAuthz
Additional Description: Added support for the append_action enum in gRPC ExtAuthz OkHttpResponse.headers for upstream request header mutations.
Risk Level: Low
Testing: Added Unit + Integration Tests
Docs Changes: Added
Release Notes: Added

[repokitteh-read-only]

As a reminder, PRs marked as draft will not be automatically assigned reviewers,
or be handled by maintainer-oncall triage.

Please mark your PR as ready when you want it to be reviewed!

🐱

Caused by: #42878 was opened by agrawroh.

see: more, trace.

[wbpcode]

Thanks for this contribution. TBH, I think this require a full cleanup/refactoring. See, this PR bring some behavior change anyway, I think we can do more directly.

I think we shouldn't split the repeated config.core.v3.HeaderValueOption to four list because it will change the order of operation in the list and result in unexpected result. We should take the header mutation filter or the route level response_headers_to_add as example, process every entry in the previous order.

/wait

[wbpcode]

Thanks so much for the update. It's much better and I added some more comments.

/wait

[krinkinmu]

@wbpcode thanks for the feedback - I will use that option as fallback if we don't find anything better.

FWIW, I think we should consider option 5 - while changing proto field to optional is not generally backward compatible, I couldn't think of anything wrong with that in this particular case, so if we can convince ourselves that it's a safe option, I think it gives us the best result (e.g., Envoy community does not need to support two different behaviors and users are not affected).

I do also understand the position that making a change to the proto message, especially if such a change isn't generally backward compatible, seems riskier even if we don't see anything obviously wrong with the change.

[wbpcode]

@wbpcode thanks for the feedback - I will use that option as fallback if we don't find anything better.

FWIW, I think we should consider option 5 - while changing proto field to optional is not generally backward compatible, I couldn't think of anything wrong with that in this particular case, so if we can convince ourselves that it's a safe option, I think it gives us the best result (e.g., Envoy community does not need to support two different behaviors and users are not affected).

I do also understand the position that making a change to the proto message, especially if such a change isn't generally backward compatible, seems riskier even if we don't see anything obviously wrong with the change.

Thank you so much if you can help. And about the API change, I didn't get how to make the enum as optional? Could you elaborate more? I think it's conflict of two fields' default values?

[ggreenway]

I'd strongly prefer option 4 over option 3. I don't think it's ok to break old ext_authz servers.

[krinkinmu]

Thank you so much if you can help. And about the API change, I didn't get how to make the enum as optional? Could you elaborate more? I think it's conflict of two fields' default values?

Ah, sorry, let me show what I'm suggesting here:

message HeaderValueOption {
  enum HeaderAppendAction {
    APPEND_IF_EXISTS_OR_ADD = 0;
    ADD_IF_ABSENT = 1;
    OVERWRITE_IF_EXISTS_OR_ADD = 2;
    OVERWRITE_IF_EXISTS = 3;
  }

  HeaderValue header = 1;
  google.protobuf.BoolValue append = 2;

-  HeaderAppendAction append_action = 3;
+  optional HeaderAppendAction append_action = 3;

  bool keep_empty_value = 4;
}

In the diff above I suggest to make the append_action field inside the HeaderValueOption message optional. This will tell protobuffer to explicitly track presence for the append_action field value, so we can tell the difference between this value not being set at all and set to a default value.

With that proto change done, we can then change getAppendAction function in source/extensions/filters/common/ext_authz/ext_authz_grpc_impl.cc like this:

AppendActionResult getAppendAction(envoy::config::core::v3::HeaderValueOption& header,
                                   bool& saw_invalid_append_actions) {
  if (header.has_append()) {
    return {header.append().value() ? HeaderValueOption::APPEND_IF_EXISTS_OR_ADD
                                    : HeaderValueOption::OVERWRITE_IF_EXISTS_OR_ADD,
            true};
  }

+  if (!header.has_append_action()) {
+    return {HeaderValueOption::OVERWRITE_IF_EXISTS_OR_ADD, true};
+  }

  if (!envoy::config::core::v3::HeaderValueOption::HeaderAppendAction_IsValid(
          header.append_action())) {
    saw_invalid_append_actions = true;
    return {HeaderValueOption::OVERWRITE_IF_EXISTS_OR_ADD, false};
  }

  return {header.append_action(), false};
}

And the logic behind it is that in this function we can now detect the situation when neither append nor append_action is set and treat it the same way as when append is set to false. So the old code, that does not know about append_action will get the same behavior they had before, but the new code that wants to use append_action will have to build with the new proto message and set append_action explicitly instead of assuming that the default behavior is APPEND_IF_EXISTS_OR_ADD.

Another way to rephrase this behavior is that when neither append nor append_action is set, default for the append takes precedence over the default for append_action.

It's still a bit weird behavior, but it's helps to avoid breaking the old code, while at the same time giving the new code an option to use append_action if they want.

[tyxia]

A while back ext_proc was trying to do the similar thing（though that pr wasn't finished)

Here is my suggestion at that time #29862 (comment)

I haven't closely looked at the code here to see if they are exactly same issue but I just post my previous comment above for your reference.

[krinkinmu]

A while back ext_proc was trying to do the similar thing.

Here is my suggestion at that time #29862 (comment)

I haven't closely looked at the code here to see if they are exactly same issue but I just post my previous comment above for your reference.

Treating APPEND_IF_EXISTS_OR_ADD as special and requiring append to be set as well in this case works as well - that's another option to consider.

That being said, if we are considering options with slightly weird behaviors, I prefer optional append_action to requiring append to be set for APPEND_IF_EXISTS_OR_ADD - in both cases we'd need to document the corner cases, but I think the behavior with optional append_action is slightly easier to describe and understand, but that's not a hard preference - I'm ok with both approaches though.

As for the option to add UNSPECIFIED value to the enum, then I don't think it works anymore - renaming enum values is not backward compatible.

[ldb]

I was surprised by this issue, too (see #43310), but the fix is extremely straightforward from a user's perspective by explicitly setting OVERWRITE_IF_EXISTS_OR_ADD.

v1.37 hasn't been out for long, would simply marking this retroactively as a breaking change not be an option also?

[krinkinmu]

I dug a bit through the history of the logic before this PR and basically for response header mutations, we've been doing the logic implemented in this PR for quite some time, so the behavior was always quite inconsistent. The change is still breaking, but it makes the behavior consistent compared to what we had before.

On top of that support for append_action to the response headers was added in a backward incompatible way, just like in this PR.

v1.37 hasn't been out for long, would simply marking this retroactively as a breaking change not be an option also?

@ldb we can stick with the new behavior and leave it as-is, and indeed when this change causes a visible breakage it's not too bad and could be fixed relatively easily, the concern would be with situations when this change breaks things silently - it's hard to tell what the impact of this could be.

[wbpcode]

cc @krinkinmu get it, thanks for the detailed explanation. But I think it's hard to accept this API break change. cc @adisuissa @markdroth . What I am worrying is that change will effect our final encoding bytes which may break compatibility between control plane and data plane.

[krinkinmu]

cc @krinkinmu get it, thanks for the detailed explanation. But I think it's hard to accept this API break change. cc @adisuissa @markdroth . What I am worrying is that change will effect our final encoding bytes which may break compatibility between control plane and data plane.

FWIW, changing a enum field to optional does not affect wire encoding of the field we add optional to - whether a enum field is marked as optional or not, it will always be encoded as int32 - that's part of the protobuf spec. Field presense tracking happens in the generated code without changing wire encoding of the fields.

It does affect what fields will get actually encoded and what will not though. Specifically, if the code without optional modifier serializes a message into wire format - it will not serialize enums with default values. So it would look to a receiving side as if the value has never been set. However, I argue, that it's ok in our case and does not break any existing code - because existing code does not check whether the field was explicitly set or not, and existing well behaved external auth servers don't send append_action field.

I'm fine if we wanted to reject this option, but if we do that, it probably should not be on the basis of wire format incopatibility only. There are other caveats with this, like inconsistent API behavior (e.g., for response headers mutations we get one behavior and for request header mutations we get another behavior).

And if the inconsistent API is a deal breaker here, then we are basically saying that we do want to break the existing behavior here, so the plan should be to add a feature flag to be able to enable the old behavior and advirtise the change as breaking.

Side note, I checked and I don't think that this PR made it into 1.37 release and it wasn't backported, so unless things change, this new behaviour will go into 1.38 - that's the release where we'd need to advertise this as a breaking change.

[yanjunxiang-google]

Yeah, adding append_action support is a breaking change. For example, if the ext_proc server does not explicitly encode either "append" or "append_action", the old Envoy code(without this PR) takes the action as "append = false", which is equivalent to : "append_action = OVERWRITE_IF_EXISTS_OR_ADD". However with this PR, Envoy takes the action as "append_action = APPEND_IF_EXISTS_OR_ADD", which have different meaning.

We can not find a good solution yet, that's the reason we hold off supporting it in ext_proc.

One hacky solution is to have another new field in the API to tell "append_action" is encoded or not. If that new field is default or false, then Envoy takes "append" field. This is hacky. Another approach is to ask all the ext_authz/ext_proc servers out there always explicitly encode "append" to "false" or "true", and never relying on Envoy default behavior----- this may not be practical.

[ggreenway]

If this isn't yet in any release, is a behavior breaking change, and we don't have any clear path forward, should we revert this for now until we know what the path forward is? Ideally it would be consistent with what ext_proc will/would do.

[wbpcode]

cc @krinkinmu if it doesn't break the encoding, I am fine to that. But note, this may still violated our API policy and need to involve other @envoyproxy/api-shepherds in. Let's post this to slack channel.

[wbpcode]

If this isn't yet in any release, is a behavior breaking change, and we don't have any clear path forward, should we revert this for now until we know what the path forward is? Ideally it would be consistent with what ext_proc will/would do.

I will not block this if we get agreement on this. May also need @agrawroh to take a check to this.

[agrawroh]

@wbpcode @ggreenway I think it's fine to revert the change. This PR also did a lot of other refactoring to ExtAuthZ (given the prior code was bit of a mess). Do we prefer having a follow-up to revert just the append_action change?

[krinkinmu]

@wbpcode @ggreenway I think it's fine to revert the change. This PR also did a lot of other refactoring to ExtAuthZ (given the prior code was bit of a mess). Do we prefer having a follow-up to revert just the append_action change?

FWIW, I don't think there any other known issues with this PR, so reverting just append_action sounds fine.

cc @krinkinmu if it doesn't break the encoding, I am fine to that. But note, this may still violated our API policy and need to involve other @envoyproxy/api-shepherds in. Let's post this to slack channel.

I'll start a discussion in slack on the best way to proceed. It seems like we are going to revert append_action support for now, so probably makes sense to start a discussion on what would be the best path forward to implement append_action support again.

[adisuissa]

cc @krinkinmu if it doesn't break the encoding, I am fine to that. But note, this may still violated our API policy and need to involve other @envoyproxy/api-shepherds in. Let's post this to slack channel.

+1 on reverting the PR as this is a breaking change that will cause more harm if it goes into an Envoy release.
Better to have more dedicated PRs next time.

[agrawroh]

@adisuissa Okay, let's revert this for now and I can do a follow-up for the cleanup without the append_action change.

[yanavlasov]

/gemini review

[gemini-code-assist]

Code Review

This pull request introduces a significant and valuable refactoring by unifying header mutation logic for ext_authz. The introduction of the HeaderMutation struct and the consolidation of different header modification vectors into request_header_mutations, response_header_mutations, and local_response_header_mutations greatly improves code clarity and maintainability. The changes also include performance improvements by using std::move to avoid unnecessary copies.

However, I've identified a regression in how headers are handled for local replies in Denied and Error scenarios. The new implementation breaks support for multiple headers with the same name (e.g., Set-Cookie), which was previously supported. I've provided a detailed comment on this issue.

_{Note: Security Review did not run due to the size of the PR.}

[gemini-code-assist]

The new implementation for applying header mutations to local replies introduces a behavior change that breaks support for multiple headers with the same name (e.g., Set-Cookie).

Previously, for Denied and Error responses, headers to be "set" were handled by first removing all existing headers with the same key, and then using addCopy for each header from the authorization response. This allowed multiple headers with the same name to be added.

The new implementation uses applyHeaderMutation, which for the OVERWRITE_IF_EXISTS_OR_ADD action (what append: false maps to), calls headers.setCopy(). setCopy removes all existing headers and adds a single new one. When multiple mutations for the same header with this action are processed in a loop, only the last one will remain.

This is a regression for use cases like returning multiple Set-Cookie headers in a 401 Unauthorized response. This is confirmed by the changes in test/extensions/filters/http/ext_authz/ext_authz_test.cc:OverrideEncodingHeaders, where the test was modified to use APPEND_IF_EXISTS_OR_ADD to work around this issue.

To fix this, the logic within this lambda should be adjusted to replicate the old behavior for OVERWRITE_IF_EXISTS_OR_ADD mutations when preparing a local reply. This could involve separating mutations by action type, removing all keys for OVERWRITE_IF_EXISTS_OR_ADD mutations first, and then using addCopy to apply them, similar to the previous implementation.