feat(xds): Add configuration objects for ExtAuthz and GrpcService #12492
Conversation
sauravzg
force-pushed
the
feat/authz-config
branch
5 times, most recently
from
8ffe2d8 to
6738492
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.
sauravzg
force-pushed
the
feat/authz-config
branch
from
6738492 to
a02a2a9
Compare
|
|
||
| private static Pattern parseRegex(String regex, String fieldName) throws ExtAuthzParseException { | ||
| try { | ||
| return Pattern.compile(regex); |
There was a problem hiding this comment.
Use re2j, like in io.grpc.xds.internal.Matchers
| return Matchers.FractionMatcher.create(proto.getNumerator(), denominator); | ||
| } | ||
|
|
||
| private static HeaderMutationRulesConfig parseHeaderMutationRules(HeaderMutationRules proto) |
There was a problem hiding this comment.
This is not specific to ext_authz (it is part of ext_proc as well), so I'd expect it to be shared.
| * @return An {@link ExtAuthzConfig} instance. | ||
| * @throws ExtAuthzParseException if the proto is invalid or contains unsupported features. | ||
| */ | ||
| public static ExtAuthzConfig fromProto(ExtAuthz extAuthzProto) throws ExtAuthzParseException { |
There was a problem hiding this comment.
The point of having our own config object is to insulate things from the xDS protos. That was originally because we needed to support xDS v2 and v3, but the code structure still stands. That is to say, converting to the config object would ordinarily be in a separate class; probably the ExtAuthzFilter class in this case.
Right now the PR series is awkward, because nothing (even considering #12496) actually uses this parsing code.
| public abstract class ExtAuthzConfig { | ||
|
|
||
| /** Creates a new builder for creating {@link ExtAuthzConfig} instances. */ | ||
| public static Builder builder() { |
There was a problem hiding this comment.
We typically name it newBuilder()
| public static ExtAuthzConfig fromProto(ExtAuthz extAuthzProto) throws ExtAuthzParseException { | ||
| if (!extAuthzProto.hasGrpcService()) { | ||
| throw new ExtAuthzParseException( | ||
| "unsupported ExtAuthz service type: only grpc_service is " + "supported"); |
| } catch (InvalidProtocolBufferException e) { | ||
| // TODO(sauravzg): Add unit tests when we have a solution for TLS creds. | ||
| // This code is as of writing unreachable because all channel credential message | ||
| // types except TLS are empty messages. |
There was a problem hiding this comment.
I see a unpack(Xdscredentials.class) up there. That doesn't look empty.
You don't actually need something non-empty to trigger this. You can actually cause this exception parsing google.protobuf.Empty. But I understand you're really saying that you aren't decoding the any if you won't look at any fields inside of the message.
| private static CallCredentials callCredsFromProto(Any cred) throws GrpcServiceParseException { | ||
| try { | ||
| AccessTokenCredentials accessToken = cred.unpack(AccessTokenCredentials.class); | ||
| // TODO(sauravzg): Verify if the current behavior is per spec.The `AccessTokenCredentials` |
There was a problem hiding this comment.
The MAX_VALUE date approach is actually correct; we should use this token forever (until xds gives us a new version). However, this code is not observing "Note that the token will not be sent on the wire unless the connection has security level PRIVACY_AND_INTEGRITY." So right now it'd be better to throw than half-implement this.
|
|
||
| public abstract Optional<Duration> timeout(); | ||
|
|
||
| public abstract Optional<Metadata> initialMetadata(); |
There was a problem hiding this comment.
Metadata is mutable. We'd normally use a unmodifiable/immutable Map for these config objects.
| * channel. This is a stub implementation for channel creation until the GrpcService trusted server | ||
| * implementation is completely implemented. | ||
| */ | ||
| public final class InsecureGrpcChannelFactory implements GrpcServiceConfigChannelFactory { |
There was a problem hiding this comment.
This doesn't seem to be used. I was trying to figure out what GrpcServiceConfigChannelFactory was for, and the answer is it isn't for anything.
|
|
||
|
|
||
| /** | ||
| * A Java representation of the {@link io.envoyproxy.envoy.config.core.v3.GrpcService} proto, |
There was a problem hiding this comment.
These classes should stand on their own, without the proto. These are allowed to be different than the proto, and saying it is just the proto or "defined in" the proto defeats part of the point. The proto is converted to these classes and then the proto should be forgotten. Yes, this needs to support anything the proto supports, but the point isn't to just be a proto-clone.
2 participants
This PR sits on top of #12491 , so only the last commit + any fixups need to be reviewed.
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 subset of the configuration defined in the xDS protobuf messages.
The main new classes are:
ExtAuthzConfig: Represents the configuration for theExtAuthzfilter, 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 only bothers to deal with creating config objects from grpc/proposal#510 and doesn't handle the rest of the parts about creating a secure channel. This instead opts to create an interface with an insecure implementation to unblock further development
The relevant sections of the spec are
This commit also includes parsers to create these configuration objects from the corresponding protobuf messages, as well as unit tests for the new classes.
Chain of dependent PRs