-
Notifications
You must be signed in to change notification settings - Fork 4.2k
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
Create a secure-only MongoDb destination #6945
Conversation
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.
LGTM
@Override | ||
public ConnectorSpecification modifySpec(ConnectorSpecification originalSpec) throws Exception { | ||
final ConnectorSpecification spec = Jsons.clone(originalSpec); | ||
((ObjectNode) spec.getConnectionSpecification().get("properties").get("instance_type").get("oneOf").get(0).get("properties")).remove("tls"); |
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.
Can you add a comment here why the tls
property is removed? Something like "remove the boolean TLS option to enforce TLS connection".
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.
In addition, it seems that simply removing this property is not enough. In the original destination code:
instanceConfig.get(TLS).asBoolean()
If the tls
property is undefined, the above statement will throw null pointer exception. Can you also update that to do a null
check, and default its value to true
?
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.
Can you add a comment here why the
tls
property is removed? Something like "remove the boolean TLS option to enforce TLS connection".
Thanks, added.
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.
In addition, it seems that simply removing this property is not enough. In the original destination code:
instanceConfig.get(TLS).asBoolean()If the
tls
property is undefined, the above statement will throw null pointer exception. Can you also update that to do anull
check, and default its value totrue
?
Thanks for noticing it, one commit with this change and test was lost, sorry for that, everything is in place now.
} | ||
} | ||
} | ||
} |
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.
This expected_spec
file is defined, but never used. Can you add a unit test / acceptance test for this secure only connector?
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.
Added
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.
@irynakruk, thank you for the updates.
Please make sure to follow and check off the Pre-merge Checklist
to make sure the connector is properly published.
/test connector=connectors/destination-mongodb
|
/test connector=connectors/destination-mongodb-strict-encrypt
|
/test connector=connectors/destination-mongodb-strict-encrypt
|
@tuliren thanks for review, I've pushed nitpick commit and small fixes for tests and dependencies. Also, I've bumped version for |
/publish connector=connectors/destination-mongodb
|
/publish connector=connectors/destination-mongodb-strict-encrypt
|
/publish connector=connectors/destination-mongodb-strict-encrypt
|
* Added mongodb destination strict encrypt
What
We want to create secure-only versions of connectors that can be used in the Airbyte cloud. The idea is that these connectors inherently prevent certain insecure connections such as connecting to a database over the public internet without encryption.
How
Created a new connector destination-mongodb-strict-encrypt based on the current connector, modified the connector's spec to hide any options which allow the user to disable TLS change the connector to enable TLS by default if the TLS option is not specified.
Recommended reading order
MongodbDestinationStrictEncrypt.java
tests
Pre-merge Checklist
Community member or Airbyter
airbyte_secret
./gradlew :airbyte-integrations:connectors:<name>:integrationTest
.README.md
bootstrap.md
. See description and examplesdocs/SUMMARY.md
docs/integrations/<source or destination>/<name>.md
including changelog. See changelog exampledocs/integrations/README.md
airbyte-integrations/builds.md
Airbyter
If this is a community PR, the Airbyte engineer reviewing this PR is responsible for the below items.
/test connector=connectors/<name>
command is passing./publish
command described here