Skip to content
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.

Sign up for GitHub

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

Jump to bottom

feat: enable setting ipType connection option for r2dbc drivers #937

Merged
merged 16 commits into from
Jan 23, 2023
Merged

feat: enable setting ipType connection option for r2dbc drivers #937

merged 16 commits into from
Jan 23, 2023

Conversation

shubha-rajan
Copy link
Contributor

@shubha-rajan shubha-rajan commented Aug 3, 2022 •
edited
Loading

Currently there is no way to use the R2DBC connectors and specify IP Type, which is a glaring feature gap. This PR adds support for setting IP type when configuring an R2DBC connection factory and adds tests to make sure the right IP address is being used.

Lots of testing boilerplate here, but the relevant files to look at are:

  • CoreSocketFactory.java
  • GcpConnectionFactoryProvider.java
  • CloudSqlConnectionFactory.java
  • GcpConnectionFactoryProviderMysqlTest.java (for the test)

Also added GcpConnectionFactoryProviderTest.java which is mostly just providing a stub for the CoreSocketFactory which can be used in tests that extend the class

@shubha-rajan shubha-rajan force-pushed the ipTypes-r2dbc branch 3 times, most recently from d4c6581 to 4d4a259 Compare August 4, 2022 17:40
@shubha-rajan shubha-rajan changed the title feat: (WIP) enable setting ipType connection option for r2dbc drivers feat: enable setting ipType connection option for r2dbc drivers Dec 14, 2022
@shubha-rajan shubha-rajan marked this pull request as ready for review December 14, 2022 03:25
@shubha-rajan shubha-rajan requested a review from a team December 14, 2022 03:25
@shubha-rajan shubha-rajan force-pushed the ipTypes-r2dbc branch 3 times, most recently from d5263a3 to 0b322d1 Compare January 20, 2023 14:42
@enocom enocom self-requested a review January 20, 2023 15:00
enocom
enocom reviewed Jan 20, 2023
View reviewed changes
core/src/main/java/com/google/cloud/sql/core/CoreSocketFactory.java Outdated
* Returns preferred ip address that can be used to establish Cloud SQL connection.
*/
public static String getHostIp(String csqlInstanceName, String ipTypes)
throws IOException {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: can we move this up to the signature line?

core/src/main/java/com/google/cloud/sql/core/CoreSocketFactory.java Outdated
*/
public static String getHostIp(String csqlInstanceName) throws IOException {
public static String getHostIp(String csqlInstanceName)
throws IOException {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: move this up a line

r2dbc/core/pom.xml Outdated
<dependency>
<groupId>junit</groupId>
<artifactId>junit</artifactId>
<version>4.13.1</version>
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

4.13.2

pom.xml Outdated
@@ -273,9 +273,9 @@
com.google.guava:guava

Global test dependencies unused in r2dbc core (no tests currently):
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we update this comment since we have tests now?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In fact, the first junit package could probably be deleted.

r2dbc/core/src/test/java/com/google/cloud/sql/core/GcpConnectionFactoryProviderTest.java Outdated
public class GcpConnectionFactoryProviderTest {

static final String PUBLIC_IP = "127.0.0.1";
static final String PRIVATE_IP = "127.0.0.2";
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nit: 10.0.0.1 is a more private-like IP address.

r2dbc/core/pom.xml
</dependency>
<dependency>
<groupId>org.bouncycastle</groupId>
<artifactId>bcpkix-jdk15on</artifactId>
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What does this dependency do?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is necessary to create the fake ephemeral cert that the coreSocketFactoryStub uses.

pom.xml Outdated
@@ -273,9 +273,9 @@
com.google.guava:guava

Global test dependencies unused in r2dbc core (no tests currently):
junit:junit,com.google.truth:truth
junit:junit,com.google.truth:truth, org.bouncycastle:bcpkix-jdk15on
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I suppose this plugin can't see that you are using this dep in test, can it?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yeah the plugin throws errors that it's an unused dependency, but we have to include it in the test suites that subclass GcpConnectionFactoryProviderTest or else we get ClassNotFound errors

r2dbc/mysql/src/test/java/com/google/cloud/sql/core/GcpConnectionFactoryProviderMysqlTest.java

@Test
public void setsCorrectOptionsForDriverHostAndPortPrivate() {
try (MockedStatic<CoreSocketFactory> mockSocketFactory = Mockito.mockStatic(
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We we pull this duplicated setup into a shared function, passing in the assertions maybe?

@enocom
Copy link
Member

enocom commented Jan 23, 2023

@shubha-rajan can we split this PR up? It's pretty big. A few ideas:

  • fixing tcpConnectonFactory could be one PR
  • the bulk of the change plus one test could be another PR
  • the remaining tests for the other engines could be another PR (or multiple if necessary)

@shubha-rajan shubha-rajan requested a review from enocom January 23, 2023 19:48
@shubha-rajan
Copy link
Contributor Author

@shubha-rajan can we split this PR up? It's pretty big. A few ideas:

  • fixing tcpConnectonFactory could be one PR
  • the bulk of the change plus one test could be another PR
  • the remaining tests for the other engines could be another PR (or multiple if necessary)

Sure

@shubha-rajan
Copy link
Contributor Author

moved fixing tcpConnectionFactory to #1130 and the tests for Postgres and SQL Server to #1131

enocom
enocom reviewed Jan 23, 2023
View reviewed changes
r2dbc/mysql/src/test/java/com/google/cloud/sql/core/GcpConnectionFactoryProviderMysqlTest.java Outdated
mockSocketFactory.when(() -> CoreSocketFactory.getSslData(fakeInstanceName))
.thenReturn(coreSocketFactoryStub.getCloudSqlInstance(fakeInstanceName).getSslData());

mockSocketFactory.when(() -> CoreSocketFactory.getHostIp(fakeInstanceName, "PRIVATE"))
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How does this line differ from the line below?

@shubha-rajan shubha-rajan merged commit 559952f into main Jan 23, 2023
@shubha-rajan shubha-rajan deleted the ipTypes-r2dbc branch January 23, 2023 22:51
@infra-db-release-bot infra-db-release-bot mentioned this pull request Jan 23, 2023
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@enocom enocom enocom approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@shubha-rajan @enocom