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

Updated SigV4 signing library in Gremlin connection for Neptune connector #1698

Merged
merged 5 commits into from
May 9, 2024

Conversation

xiazcy
Copy link
Contributor

@xiazcy xiazcy commented Jan 12, 2024

Issue #, if available:
#1574

Description of changes:
The SigV4 library used for IAM in Gremlin connection in Neptune connector is a deprecated library, this PR updates it to use the new maintained library for signing. The deprecated library is only recommended to be used with 3.4.x Gremlin drivers, and may work when using 3.5.x and earlier 3.6.x driver versions, but will no longer work with 3.7.x.

Also downgraded the Gremlin driver version used by the connector to within the supported range for Neptune (currently 3.6.5 is the max).

I was able to reproduce the error on 3.7.1 gremlin console, trying to connect to IAM enabled cluster using the old SigV4 signing library:
image

I have tested the fix in this PR by deploying a customer connector and querying an IAM enabled cluster, and was successfull:
image
Screenshot 2024-01-19 at 5 26 13 PM

By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.

@xiazcy xiazcy marked this pull request as ready for review January 20, 2024 01:18
@xiazcy
Copy link
Contributor Author

xiazcy commented Jan 29, 2024

This PR also provides a fix to the custom schema writer that allows a workaround for issue #1655, where naming clashes can be avoided by creating a view from custom queries.

value.value = Float.parseFloat(fieldValue.toString());
value.isSet = 1;
}
else {
else if (fieldValue instanceof ArrayList) {
ArrayList<Object> objValues = (ArrayList) fieldValue;
if (objValues != null && objValues.get(0) != null && !(objValues.get(0).toString().trim().isEmpty())) {
value.value = Float.parseFloat(objValues.get(0).toString());
Copy link
Contributor

Choose a reason for hiding this comment

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

Shouldn't we parse the list of value instead? Why are we parsing only the first element in list

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We can only return zero or one value for a column. Custom query may return a single value wrapped inside a list/set, so we needed to access the element. In corner cases multiple values are returned, we are also only returning the first value based on this constraint.


if (fieldValue != null) {
if (fieldValue.getClass().equals(String.class)) {
value.value = fieldValue.toString();
value.isSet = 1;
}
else {
else if (fieldValue instanceof ArrayList) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Do we need to apply this condition else if (fieldValue instanceof ArrayList) to rest of types? For example DATEMILLI

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, I did miss these. I'll add them.

@triggan
Copy link

triggan commented May 1, 2024

@chngpe - Gentle ping to see if we can get this reviewed and merged soon. This is blocking a few customers as the current version of the connector does not work with an IAM-enabled Neptune cluster.

@xiazcy
Copy link
Contributor Author

xiazcy commented May 7, 2024

Hi @chngpe, is there an ETA for merging this PR? Or is there anything else I should update? Thanks!

@AbdulR3hman AbdulR3hman merged commit e080a7c into awslabs:master May 9, 2024
6 checks passed
@AbdulR3hman
Copy link
Contributor

@xiazcy done; thank you for your contribution.

github-actions bot pushed a commit that referenced this pull request May 14, 2024
  - Revert "build(deps): bump com.google.cloud:google-cloud-bigquery" (#1971)
  - build(deps): bump com.google.cloud:google-cloud-bigquery from 2.37.0 to 2.40.1 (#1962)
  - build(deps): bump com.google.cloud:google-cloud-bigquery
  - build(deps): bump com.google.api.grpc:grpc-google-cloud-bigquerystorage-v1 from 2.47.0 to 3.5.1 (#1961)
  - build(deps): bump com.google.api.grpc:grpc-google-cloud-bigquerystorage-v1
  - build(deps): bump gremlinDriverVersion from 3.6.5 to 3.7.2 (#1967)
  - build(deps): bump gremlinDriverVersion from 3.6.5 to 3.7.2
  - build(deps): bump com.google.cloud:google-cloud-resourcemanager from 1.44.0 to 1.45.0 (#1968)
  - build(deps): bump com.google.cloud:google-cloud-resourcemanager
  - build(deps): bump org.elasticsearch.client:elasticsearch-rest-client from 8.13.3 to 8.13.4 (#1970)
  - build(deps): bump org.elasticsearch.client:elasticsearch-rest-client
  - build(deps): bump com.amazon.redshift:redshift-jdbc42 from 2.1.0.26 to 2.1.0.27 (#1969)
  - build(deps): bump com.amazon.redshift:redshift-jdbc42
  - build(deps): bump software.amazon.awssdk:bom from 2.25.45 to 2.25.50 (#1965)
  - build(deps): bump software.amazon.awssdk:bom from 2.25.45 to 2.25.50
  - build(deps): bump com.google.cloud:google-cloud-storage from 2.37.0 to 2.38.0 (#1966)
  - build(deps): bump com.google.cloud:google-cloud-storage
  - build(deps): bump aws-sdk.version from 1.12.715 to 1.12.720 (#1964)
  - build(deps): bump aws-sdk.version from 1.12.715 to 1.12.720
  - Enable map of complex types support (#1917)
  - unifying vertica connector as jdbc connector (#1918)
  - Feature/oracle fips (#1946)
  - Updated SigV4 signing library in Gremlin connection for Neptune connector (#1698)
github-actions bot pushed a commit that referenced this pull request May 17, 2024
  - Snowflake split issue (#1963)
  - Com.google.cloud google cloud bigquery version update (#1975)
  - Fix ValidationException for Timestream connector (#1888)
  - build(deps): bump com.amazon.redshift:redshift-jdbc42 from 2.1.0.27 to 2.1.0.28 in /athena-redshift (#1974)
  - build(deps): bump com.amazon.redshift:redshift-jdbc42
  - Updated prepare_dev_env.sh script (#1972)
  - Revert "build(deps): bump com.google.cloud:google-cloud-bigquery" (#1971)
  - build(deps): bump com.google.cloud:google-cloud-bigquery from 2.37.0 to 2.40.1 (#1962)
  - build(deps): bump com.google.cloud:google-cloud-bigquery
  - build(deps): bump com.google.api.grpc:grpc-google-cloud-bigquerystorage-v1 from 2.47.0 to 3.5.1 (#1961)
  - build(deps): bump com.google.api.grpc:grpc-google-cloud-bigquerystorage-v1
  - build(deps): bump gremlinDriverVersion from 3.6.5 to 3.7.2 (#1967)
  - build(deps): bump gremlinDriverVersion from 3.6.5 to 3.7.2
  - build(deps): bump com.google.cloud:google-cloud-resourcemanager from 1.44.0 to 1.45.0 (#1968)
  - build(deps): bump com.google.cloud:google-cloud-resourcemanager
  - build(deps): bump org.elasticsearch.client:elasticsearch-rest-client from 8.13.3 to 8.13.4 (#1970)
  - build(deps): bump org.elasticsearch.client:elasticsearch-rest-client
  - build(deps): bump com.amazon.redshift:redshift-jdbc42 from 2.1.0.26 to 2.1.0.27 (#1969)
  - build(deps): bump com.amazon.redshift:redshift-jdbc42
  - build(deps): bump software.amazon.awssdk:bom from 2.25.45 to 2.25.50 (#1965)
  - build(deps): bump software.amazon.awssdk:bom from 2.25.45 to 2.25.50
  - build(deps): bump com.google.cloud:google-cloud-storage from 2.37.0 to 2.38.0 (#1966)
  - build(deps): bump com.google.cloud:google-cloud-storage
  - build(deps): bump aws-sdk.version from 1.12.715 to 1.12.720 (#1964)
  - build(deps): bump aws-sdk.version from 1.12.715 to 1.12.720
  - Enable map of complex types support (#1917)
  - unifying vertica connector as jdbc connector (#1918)
  - Feature/oracle fips (#1946)
  - Updated SigV4 signing library in Gremlin connection for Neptune connector (#1698)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants