-
Notifications
You must be signed in to change notification settings - Fork 11
iam: add conformance tests for IAM Identity Management APIs for GCP #186
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
iam: add conformance tests for IAM Identity Management APIs for GCP #186
Conversation
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #186 +/- ##
=========================================
Coverage 83.21% 83.21%
Complexity 91 91
=========================================
Files 150 150
Lines 8190 8190
Branches 950 950
=========================================
Hits 6815 6815
Misses 925 925
Partials 450 450
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
7693624 to
741577f
Compare
3fc467e to
99f415d
Compare
|
799509a to
149ac5a
Compare
| * | ||
| * <p><b>Usage:</b> | ||
| * <ul> | ||
| * <li>Record mode: {@code mvn test -Drecord} - Saves responses to {@code src/test/resources/recordings/iam/}</li> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We don't need this instruction in comment here. We can remove it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ack
| * @param endpoint The gRPC endpoint to connect to | ||
| * @return A configured TransportChannelProvider for gRPC | ||
| */ | ||
| public static TransportChannelProvider getGrpcTransportChannelProvider(int port, String endpoint) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I didn't find any usage of this method in this PR. Where we need this getGrpcTransportChannelProvider?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's not needed anymore. I'll remove it in follow up commit.
| AbstractIam iam = harness.createIamDriver(true); | ||
| IamClient iamClient = new IamClient(iam); | ||
|
|
||
| String identityId = iamClient.createIdentity( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
After the Identity creation, do we have any clean up step for these test created resources?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good catch. CleanUp is not implemented. I'll address it in follow up commits.
bc4175b to
d549cb3
Compare
d549cb3 to
f12dcbd
Compare
| * <p>NOTE: GCP IAM Admin API is gRPC-only. Unlike other conformance tests that use | ||
| * HTTP/HTTPS proxy-based WireMock recording, this test uses: | ||
| * <ul> | ||
| * <li><b>RECORD mode:</b> Wraps IAMClient to intercept and save responses as JSON files</li> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
that's exactly what happens for other conformance tests, which part is unlike other conf tests?
| private static final Logger logger = LoggerFactory.getLogger(GcpIamIT.class); | ||
| private static WireMockServer grpcWireMockServer; | ||
| private static WireMockGrpcService mockIamService; | ||
| private static final String GRPC_DIR = "src/test/resources/grpc"; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
it's okay to use the same path for recording and let the wiremock framework take care of creating directories. I see now we have to create directories and all manually.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
scratch this please, I see it's a setup for grpc here https://github.com/wiremock/wiremock-grpc-extension
| private static final String TRUSTED_PRINCIPAL = "[email protected]"; | ||
|
|
||
| @Override | ||
| public AbstractIam createIamDriver(boolean useValidCredentials) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think useValidCredentials pattern is taken from blobstore, we aren't used it here, I think we should remove it, it doesn't add much value.
| ProjectsSettings.Builder projectsSettingsBuilder = ProjectsSettings.newBuilder() | ||
| .setTransportChannelProvider(channelProvider); | ||
| try { | ||
| if (isRecordMode && useValidCredentials) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ditto
| if (isRecordMode && useValidCredentials) { | ||
| // RECORD mode: Use gRPC interceptor to capture and save responses | ||
| logger.info("--- RECORD MODE: Connecting to Real GCP with Recording ---"); | ||
| Files.createDirectories(Paths.get(RECORDINGS_DIR)); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's create the directory manually as one time operation and assume that it exists, this won't work in bazel fork where build works in temp location.
(Also I believe the recordings directory should be taken care of wiremock for creation.)
| return; | ||
| } | ||
| // 1. Prepare directories and clean up old descriptor files | ||
| Files.createDirectories(Paths.get(GRPC_DIR)); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ditto, let's create it manually as one time
| /** | ||
| * Loads all stub files from the recordings directory. | ||
| */ | ||
| private static void loadAllStubs() throws IOException { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
don't wiremock load the recordings automatically for gRPC?
| Message responseMessage = parseResponseForMethod(methodNameFromJson, responseJson); | ||
|
|
||
| if (responseMessage == null) { | ||
| logger.error(" ERROR: Unknown method type, skipping: {}", methodNameFromJson); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: remove tab in log
| .willReturn(message(responseMessage)) | ||
| ); | ||
|
|
||
| logger.info(" ✓ Successfully registered stub for: {}", methodNameFromJson); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: remove genAI special symbols
Summary
GCP IAM Admin API Conformance Testing Strategy
Specific challenges encountered with the GCP IAM Admin Java Client that necessitated a different testing approach compared to the standard REST-based conformance tests.
1. Client Transport Limitations (gRPC vs. REST)
Unlike other GCP client libraries we use, the
google-iam-adminlibrary does not natively expose an HTTP/JSON (REST) transport option in itsStubSettings. It forces the use of gRPC, which requires HTTP/2. This prevented us from using our standard HTTP-based WireMock recording setup.2. WireMock gRPC Extension Limitations
While we utilized the
wiremock-grpc-extensionto handle the HTTP/2 connection and protocol framing, it currently lacks full parity with the standard WireMock HTTP recorder:UNIMPLEMENTEDerrors when using standard auto-loaded mappings.3. Implementation of Custom Record/Replay Pattern
To bridge this gap, we implemented a manual Record/Replay workflow:
JsonFormatand saved them to a dedicatedrecorded_responsesdirectory (avoiding conflicts with WireMock's auto-scanner).