Use maven plugin to generate Client-Side CAB proto code. #1639
Conversation
Change-Id: I8154ea3c3c3126a1cf47e563b9b9e2f6b85c7f1a
cab-token-generator/pom.xml
Outdated
| <version>0.6.1</version> | ||
| <configuration> | ||
| <protoSourceRoot>${project.basedir}/proto</protoSourceRoot> | ||
| <protocArtifact>com.google.protobuf:protoc:${project.protobuf.version}:exe:${os.detected.classifier}</protocArtifact> |
There was a problem hiding this comment.
Can we define a new maven property for the protoc version (i.e. something like project.protoc.version)? I would like to separate the Protoc and Protobuf-Java runtime to give us flexibility to move the runtime forward.
| // Copyright 2023 Google LLC | ||
| // | ||
| // Licensed under the Apache License, Version 2.0 (the "License"); | ||
| // you may not use this file except in compliance with the License. | ||
| // You may obtain a copy of the License at | ||
| // | ||
| // http://www.apache.org/licenses/LICENSE-2.0 | ||
| // | ||
| // Unless required by applicable law or agreed to in writing, software | ||
| // distributed under the License is distributed on an "AS IS" BASIS, | ||
| // WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. | ||
| // See the License for the specific language governing permissions and | ||
| // limitations under the License. |
There was a problem hiding this comment.
To be safe, can the header be updated to match https://github.com/googleapis/google-auth-library-java/blob/main/java.header with the updated year? Same for the other proto files below?
|
|
||
| syntax = "proto3"; | ||
|
|
||
| package cel.expr; |
There was a problem hiding this comment.
General question regarding checked.proto and syntax.proto: Where do these proto files come from? Is the SoT from the Cel repo?
If so, how do we plan to maintain that this is updated or in sync? Or are these just a subset of those existing protos and your team will maintain compatibility? Would these be updated frequently?
There was a problem hiding this comment.
This is a snapshot from cel-spec.
I dumped these proto files here because the maven protobuf plugin does not support fetching remote dependencies if they are not in a maven package. The expr proto is very stable: https://github.com/google/cel-spec/commits/master/proto/cel/expr/checked.proto, https://github.com/google/cel-spec/commits/master/proto/cel/expr/syntax.proto. These will be maintained along with the client-side CAB proto.
That being said, I plan to talk to CEL team on if they are interested to publish the proto files along with the cel-java maven package so we don't need to keep a snapshot here.
There was a problem hiding this comment.
Thanks! I pulled the generated proto gen code down locally and I have another concern.
I see that the packaged jar would have these Protoc Gen code and it would have the classes that customers can use:
i.e.
// Protobuf Java Version: 3.25.5
package dev.cel.expr;
/**
* <pre>
* A CEL expression which has been successfully type checked.
* </pre>
*
* Protobuf type {@code cel.expr.CheckedExpr}
*/
public final class CheckedExpr extends
com.google.protobuf.GeneratedMessageV3 implements
If a customer already has usages of CheckExpr from dev.cel.expr would this conflict. If this is a snapshot of the existing protos, is it possible to change the package name to have CAB prefix to distinguish them?
There was a problem hiding this comment.
Thanks for pointing out. That's indeed a problem. The output jar file will also include classes from dev.cel.*.
What I wanted is to add the dumped proto files into the -I option of the protoc command, so only the client-side-cab proto will be compiled and the generated code would reference dev.cel.* classes (like the current generated code in the repo).
The maven protobuf plugin is not very flexible with this and the only solution seems to be putting the cel-spec proto files in a dependency of the cab-token-generator package.
I think there are a few options:
- Ask CEL team to publish the proto files in a maven package. They can either be in the main dev.cel.cel artifact or another artifact that only contains the proto files. This probably won't make it to the 2/3 release.
- Dump the cel-spec proto files in another package next to cab-token-generator. I already tried this and it works as expected (https://github.com/huangjiahua/google-auth-library-java/tree/proto2/cab-token-generator-proto-dependency).
- Keep the generated code in the release for now and figure out a better solution.
Adding a prefix to the cel protos probably is not a good solution because the factory class actually uses the dev.cel.expr.Expr class from the dev.cel:
Please let me know WDYT. Thanks!
There was a problem hiding this comment.
- Ask CEL team to publish the proto files in a maven package. They can either be in the main dev.cel.cel artifact or another artifact that only contains the proto files. This probably won't make it to the 2/3 release.
Just to clarify so I understand this option. If the proto files are published to a new maven package and then are referenced during the Maven Protobuf Plugin step, they wouldn't be generated and put in the output jar file because the dev cel protos don't exist in the /protos sub folder, right?
If this is the case, is it possible to move the cel protos live outside of the existing /proto subfolder folder?
- Dump the cel-spec proto files in another package next to cab-token-generator.
Thanks, I agree that this is an option but I would want to explore all the alternative options that we have that don't create a whole new maven module.
- Keep the generated code in the release for now and figure out a better solution.
I think I'm fine with for now while we explore the alternative options.
Adding a prefix to the cel protos probably is not a good solution because the factory class actually uses the dev.cel.expr.Expr class from the dev.cel
Yeah that makes sense. I think one option we have is potentially to manually convert the prefix'd Expr class to dev.cel Expr class. Not ideal and not something that I think we would want to pursue unless there aren't better options.
There was a problem hiding this comment.
Just to clarify so I understand this option. If the proto files are published to a new maven package and then are referenced during the Maven Protobuf Plugin step, they wouldn't be generated and put in the output jar file because the dev cel protos don't exist in the /protos sub folder, right?
Yes. The cel proto files will become the -I flag in the protoc command that it assumes the generated code already exists (in some other maven dependencies).
If this is the case, is it possible to move the cel protos live outside of the existing /proto subfolder folder?
Do you mean not adding a new dependency but simply move cel protos outside the /proto folder? That won't work with the protobuf maven plugin. If we simply move them out protoc cannot find them and the compilation will fail. I wish we could add the -I option using the plugin but it does not have that feature.
There was a problem hiding this comment.
Ok, let's put a pause on this auto-generating Protoc in Auth then and get some guidance from Cel (about potentially having the protos published to maven central). IIRC, you mentioned that the cel and cab protos don't change much and I think it's fine to have them just live here for now.
Change-Id: I2efb2fa0472e3af01a3955126cd27e37450ee95a
Change-Id: I87a5e073f16eac13ac74940bceca64fab21cee64
Use the
protobuf-maven-pluginto automatically generate the protobuf Java code and remove the generated code.The version of protoc is determined by the
project.protobuf.versionand it's currently 3.25.5.Change-Id: I8154ea3c3c3126a1cf47e563b9b9e2f6b85c7f1a
Thank you for opening a Pull Request! Before submitting your PR, there are a few things you can do to make sure it goes smoothly:
Fixes #<issue_number_goes_here> ☕️
If you write sample code, please follow the samples format.