xds: Add configuration objects for ExtAuthz, GrpcService and Bootstrap changes for GrpcService#12492
xds: Add configuration objects for ExtAuthz, GrpcService and Bootstrap changes for GrpcService#12492sauravzg wants to merge 3 commits intogrpc:masterfrom
Conversation
sauravzg
force-pushed
the
feat/authz-config
branch
6 times, most recently
from
6738492 to
a02a2a9
Compare
xds/src/main/java/io/grpc/xds/internal/extauthz/ExtAuthzConfig.java
Outdated
Show resolved
Hide resolved
xds/src/main/java/io/grpc/xds/internal/extauthz/ExtAuthzConfig.java
Outdated
Show resolved
Hide resolved
| * @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.
There was a problem hiding this comment.
I believe the objects are now independent from proto . Some artifacts of documentation may linger despite my attempts to fix wherever applicable.
The parsing is now moved into a separate class.
There is still not code using it until the very likely the final PR in the chain , where we construct the actual filter.
But it serves as an implementation of the business logic for translating xds config to java representations.
I would continue to prefer implementations in a bottom up manner, where we define interfaces and implementation, evaluate them in isolation and then evaluate their use at callsites if possible.
| 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()
There was a problem hiding this comment.
go/java-practices/builders#api seems to prefer builder . I see Builder being commonly used in the code base as well.
Is there a particular reason to name it newBuilder ?
Asking because of laziness . I fixed one place and realized I have zillion more to fix , so reverting one change would probably be easier :).
Happy to change if you have strong opinions on this .
xds/src/main/java/io/grpc/xds/internal/extauthz/ExtAuthzConfig.java
Outdated
Show resolved
Hide resolved
xds/src/main/java/io/grpc/xds/internal/grpcservice/GrpcServiceConfig.java
Outdated
Show resolved
Hide resolved
| 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.
There was a problem hiding this comment.
Done. Implemented a SecurityAwareWrapper which currently fails the RPC if it doesn't meet the privacy requirements.
Keeping this open , since I am unsure if we should fail the RPC or just make it a no-op
xds/src/main/java/io/grpc/xds/internal/grpcservice/GrpcServiceConfig.java
Outdated
Show resolved
Hide resolved
xds/src/main/java/io/grpc/xds/internal/grpcservice/InsecureGrpcChannelFactory.java
Outdated
Show resolved
Hide resolved
xds/src/main/java/io/grpc/xds/internal/grpcservice/GrpcServiceConfig.java
Outdated
Show resolved
Hide resolved
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.
Fixup: Address comments from grpc#12492 for non PR deps Fixup: CallCreds changes to use CompositeCreds Fixup: 12493 : Remove redundant CheckParamsBuilder Fixup: 12493 : Move the CertificateProvider to CheckRequestBuilder and create utils class Fixup: 12492 Separate out config parsing yolo
sauravzg
force-pushed
the
feat/authz-config
branch
from
a02a2a9 to
47f2a85
Compare
|
@ejona86 PTAL. I've addressed most of the comments and have updated the PR description. Apart from addresing the comments, this PR now also contains the remaining implementation of GrpcService which wasn't in scope last quarter (essentially bootstrap changes and some bootstrap abstractions). |
sauravzg
force-pushed
the
feat/authz-config
branch
from
47f2a85 to
5654c64
Compare
… the updated requirements
sauravzg
changed the title
2 participants
This commit introduces configuration objects for the external authorization (ExtAuthz) filter and the gRPC service and corresponding translations from XDS proto and Bootstrap. These classes provide a structured, immutable representation of the subset of the configuration defined in the xDS protobuf messages.
This PR should mostly now (hopefully ) be compliant with grpc/proposal#510 but without
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.ChannelCredsConfigand friends: To allow comparison between credential configuration , to allow caching based on creds which'll be needed in followup PRs for authz and proc.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.