Conversation
sauravzg
force-pushed
the
feat/header-mutations
branch
5 times, most recently
from
328631a to
86d68ac
Compare
This commit introduces configuration objects for the external authorization (ExtAuthz) filter and the gRPC service it uses. These classes provide a structured, immutable representation of the configuration defined in the xDS protobuf messages. The main new classes are: - `ExtAuthzConfig`: Represents the configuration for the `ExtAuthz` filter, including settings for the gRPC service, header mutation rules, and other filter behaviors. - `GrpcServiceConfig`: Represents the configuration for a gRPC service, including the target URI, credentials, and other settings. - `HeaderMutationRulesConfig`: Represents the configuration for header mutation rules. This commit also includes parsers to create these configuration objects from the corresponding protobuf messages, as well as unit tests for the new classes.
xds/src/main/java/io/grpc/xds/internal/headermutations/HeaderMutator.java
Outdated
Show resolved
Hide resolved
sauravzg
force-pushed
the
feat/header-mutations
branch
from
86d68ac to
759130e
Compare
|
@kannanjgithub PTAL, if the implementation mutation meets the spec for ext proc. You need to TAL at the last two commits only. |
… the updated requirements
sauravzg
force-pushed
the
feat/header-mutations
branch
from
759130e to
4bfd5e6
Compare
sauravzg
changed the title
This commit introduces a library for handling header mutations as specified by the xDS protocol. This library provides the core functionality for modifying request and response headers based on a set of rules. The main components of this library are: - `HeaderMutator`: Applies header mutations to `Metadata` objects. - `HeaderMutationFilter`: Filters header mutations based on a set of configurable rules, such as disallowing mutations of system headers. - `HeaderMutations`: A value class that represents the set of mutations to be applied to request and response headers. - `HeaderMutationDisallowedException`: An exception that is thrown when a disallowed header mutation is attempted. This commit also includes comprehensive unit tests for the new library.
sauravzg
force-pushed
the
feat/header-mutations
branch
from
4bfd5e6 to
6961989
Compare
kannanjgithub
left a comment
kannanjgithub
left a comment
There was a problem hiding this comment.
Sending comments for headermutations.
|
|
||
| /** Represents mutations for response headers. */ | ||
| @AutoValue | ||
| public abstract static class ResponseHeaderMutations { |
There was a problem hiding this comment.
What is the need to have disallow header removals for response mutations, as the gRFC doesn't say response headers can't be removed. It looks one class to handle both request and response headers mutations would do.
|
|
||
| /** | ||
| * Represents a header option to be appended or mutated as part of xDS configuration. | ||
| * Avoids direct dependency on Envoy's proto objects. |
There was a problem hiding this comment.
Having essentially mirror protos here and in other places seems unnecessary to me. We have the dependency on envoy protos anyway in GrpcServiceConfigParser that cannot be avoided. What is the problem in directly depending on envoy protos everywhere? If it is helpful to have structurally different protos I can see the need from a utility perspective (we do that for xds resource protos like CdsUpdate) , but not from trying to avoid usage perspective.
|
|
||
| default: | ||
| // Should be unreachable unless there's a proto schema mismatch. | ||
| logger.warning("Unknown HeaderAppendAction: " + action); |
There was a problem hiding this comment.
As the mutations happen per-rpc we shouldn't be logging these warnings.
|
|
||
|
|
||
|
|
||
| public HeaderMutationFilter(Optional<HeaderMutationRulesConfig> mutationRules) { // NOPMD |
There was a problem hiding this comment.
Remove this comment?
| if (rules.allowExpression().isPresent()) { | ||
| return rules.allowExpression().get().matcher(lowerCaseHeaderName).matches(); | ||
| } | ||
| return !rules.disallowAll(); |
There was a problem hiding this comment.
Do this check at the beginning,
if (rules.disallowAll()) {
return false;
}
kannanjgithub
left a comment
kannanjgithub
left a comment
There was a problem hiding this comment.
Reviewed rest of the PR.
2 participants
This PR sits on top of #12493, so only the last commit + any fixups need to be reviewed.
This commit introduces a library for handling header mutations as specified by the xDS protocol. This library provides the core functionality for modifying request and response headers based on a set of rules.
The main components of this library are:
HeaderMutator: Applies header mutations toMetadataobjects.HeaderMutationFilter: Filters header mutations based on a set of configurable rules, such as disallowing mutations of system headers.HeaderMutations: A value class that represents the set of mutations to be applied to request and response headers.HeaderMutationDisallowedException: An exception that is thrown when a disallowed header mutation is attempted.This commit also includes comprehensive unit tests for the new library.