Adding support for IAP (identity-aware proxy) in the JDBC driver.#27
Adding support for IAP (identity-aware proxy) in the JDBC driver.#27MitchellGale wants to merge 3 commits intolookerfrom
Conversation
Summary of ChangesHello, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request introduces support for connecting to Looker instances through an Identity-Aware Proxy (IAP) in the JDBC driver. It achieves this by adding necessary Google-related dependencies, defining new connection properties for IAP client ID and service account email, and modifying the HTTP request handling to include IAP tokens for proxy authorization. Highlights
🧠 New Feature in Public Preview: You can now enable Memory to help Gemini Code Assist learn from your team's feedback. This makes future code reviews more consistent and personalized to your project's style. Click here to enable Memory in your admin console. Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for GitHub and other Google products, sign up here. Footnotes
|
There was a problem hiding this comment.
Code Review
This pull request adds support for Identity-Aware Proxy (IAP) in the JDBC driver, which is a valuable enhancement. The changes introduce new dependencies, connection properties, and logic to handle IAP authentication. The implementation is mostly solid, but I've identified a few areas for improvement. Specifically, there are some redundant dependencies in the Gradle build file that should be cleaned up, a duplicated block of code that needs to be removed, and an empty test file that should be populated with tests for the new functionality.
core/src/main/java/org/apache/calcite/avatica/remote/looker/LookerRemoteMeta.java
Outdated
Show resolved
Hide resolved
core/src/test/java/org/apache/calcite/avatica/remote/looker/LookerSdkFactoryTest.java
Show resolved
Hide resolved
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request introduces support for Identity-Aware Proxy (IAP) in the JDBC driver, allowing connections to IAP-enabled Looker instances. Key changes include adding new connection properties for IAP client ID and service account email, updating the LookerRemoteMeta to include the Proxy-Authorization header, and modifying the LookerSdkFactory to pass IAP-related properties to the Looker SDK. A new test case has also been added to verify the SDK's initialization with IAP properties. Overall, the changes are well-aligned with the stated objective. However, there is a potential issue in how connection properties are handled in LookerSdkFactory that could lead to unintended behavior.
core/src/main/java/org/apache/calcite/avatica/remote/looker/LookerSdkFactory.java
Outdated
Show resolved
Hide resolved
bf0d455 to
cfb85db
Compare
olivrlee
left a comment
There was a problem hiding this comment.
Overall looks good, could you address the CI build errors? Looks like it failed to compile `Execution failed for task ':core:compileJava'
Thanks! I need to provide the new Kotlin SDK build after this PR is merged/released looker-open-source/sdk-codegen#1672 I have a dependency there so this won't be able to be able to pass/be merged until after that. But testing locally with a local build it worked as expected. |
Adding support for
iap_client_idandiap_service_account_emailstring fields in JDBC connections to allow connection through an Identity-Aware Proxy (IAP) enabled Looker instance through the Kotlin SDK.