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.

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

fix: Make supporting classes of AwsCredentials serializable #1113

Merged
merged 5 commits into from
Feb 16, 2023

Conversation

jmahonin
Copy link
Contributor

@jmahonin jmahonin commented Dec 8, 2022

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:

  • Make sure to open an issue as a bug/issue before writing your code! That way we can discuss the change, evaluate designs, and agree on the general idea
  • Ensure the tests and linter pass
  • Code coverage does not decrease (if any source code was changed)
  • Appropriate docs were updated (if necessary)

Fixes #1112 ☕️

If you write sample code, please follow the samples format.

@jmahonin jmahonin requested a review from a team as a code owner December 8, 2022 16:05
@google-cla
Copy link

google-cla bot commented Dec 8, 2022

Thanks for your pull request! It looks like this may be your first contribution to a Google open source project. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

View this failed invocation of the CLA check for more information.

For the most up to date status, view the checks section at the bottom of the pull request.

@product-auto-label product-auto-label bot added the size: s Pull request size is small. label Dec 8, 2022
@jmahonin
Copy link
Contributor Author

jmahonin commented Dec 8, 2022

As with #1111 the CLA was signed yesterday for a workgroup jmahonin@decodable.co is a member of. Perhaps waiting for propagation delay.
CLA verification succeeded

@davidrabinowitz
Copy link

This can fix GoogleCloudDataproc/spark-bigquery-connector#845 as well. @jmahonin Do you need help with it?

@jmahonin
Copy link
Contributor Author

jmahonin commented Jan 6, 2023

This can fix GoogleCloudDataproc/spark-bigquery-connector#845 as well. @jmahonin Do you need help with it?

Just a review at this point!

I can confirm this PR fixes the same serialization issue I observed with Flink. I'm not in a position to test against Spark, but based on past experience, I likewise suspect this will fix it there as well.

@davidrabinowitz
Copy link

@jmahonin That's fine, I'll test it once released. Can you please rebase the PR so it can be merged?

@jmahonin
Copy link
Contributor Author

jmahonin commented Jan 6, 2023

Will do, expect a ping on Monday

@jmahonin jmahonin force-pushed the aws-credentials-serializable branch from 7bd9f53 to 04cf78a Compare January 9, 2023 14:37
@jmahonin
Copy link
Contributor Author

jmahonin commented Jan 9, 2023

@davidrabinowitz Rebased

Copy link
Member

@TimurSadykov TimurSadykov left a comment

Choose a reason for hiding this comment

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

Please add serialize unit tests for affected classes. You can leverage existing helper for it by extending the BaseSerializationTest

@product-auto-label product-auto-label bot added size: m Pull request size is medium. and removed size: s Pull request size is small. labels Jan 13, 2023
@jmahonin
Copy link
Contributor Author

Good call @TimurSadykov , added the tests and discovered ExternalAccountCredentials.ServiceAccountImpersonationOptions was not serializable. Added that as well.

There was already a serialization test on ImpersonatedCredentials, though they all use a mock transport factory so there isn't coverage on instantiating the factory in readObject (which is true of the existing serialization tests as well).

I didn't add a specific test for the deserialization of SystemEnvironmentProvider, it is similarly stubbed out with a TestEnvironmentProvider in the tests.

@jmahonin
Copy link
Contributor Author

I've updated the unit tests to ensure subclasses of ExternalAccountCredentials have similar serialize() tests.

However, the PluggableAuthCredentials uses a functional interface PluggableAuthHandler, which aren't easily serializable. Given the intent appears to be retrieving tokens using scripts / executables, it's not clear to me if this credential source should be serializable at all. Please advise.

Copy link
Member

@TimurSadykov TimurSadykov left a comment

Choose a reason for hiding this comment

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

LGTM

@jmahonin
Copy link
Contributor Author

jmahonin commented Feb 3, 2023

This fell off my radar for a bit. I see there's an approval and a request for changes re: subclasses having their own serialVersionUIDs. I'm happy to make the change, but want to confirm first given the PR's been approved.

@lsirac
Copy link
Contributor

lsirac commented Feb 14, 2023

jmahonin please make the change and we will merge the PR, LG otherwise

Discovered ServiceAccountImpersonationOptions needed be serializable as well.
Note that PluggableAuthCredentials uses a non-serializable ExecutableHandler.
It's not clear that this can/should be a serializable credential source.
@jmahonin
Copy link
Contributor Author

jmahonin please make the change and we will merge the PR, LG otherwise

This is done. I didn't include a serialVersionUID for PluggableAuthCredentials for the same reason described above here
#1113 (comment)

@lsirac lsirac merged commit 82bf871 into googleapis:main Feb 16, 2023
@TimurSadykov TimurSadykov added the kokoro:force-run Add this label to force Kokoro to re-run the tests. label Feb 17, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
kokoro:force-run Add this label to force Kokoro to re-run the tests. size: m Pull request size is medium.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

General: Various supporting classes when using AwsCredentials are not serializable
4 participants