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

ssh tunnel for postgres normalization #5818

Closed
wants to merge 77 commits into from
Closed

ssh tunnel for postgres normalization #5818

wants to merge 77 commits into from

Conversation

Phlair
Copy link
Contributor

@Phlair Phlair commented Sep 2, 2021 •
edited
Loading

What

Adds support for ssh tunnelling during normalization (& custom dbt transforms if non-Kube) for postgres destination.

Custom dbt transforms don't work on Kube as per this issue.

The core additions in this PR will enable other destinations to use ssh for normalization too provided ssh support/tests have been added as in Charles' PR. There is just a small update required per destination in airbyte-integrations/bases/base-normalization/normalization/transform_config/transform.py.

How

Added a bash script sshtunneling.sh that can read ssh tunnel options from the config file and if ssh tunneling is on, open an ssh tunnel using standard ssh procedure (and subsequently close it). Utilising a control-socket so we can target close the specific ssh session.

For key authorization, the key gets written to a tmp file and used in standard ssh command.

For password authorization, I've used sshpass to allow passing password non-interactively.

Notes

I'm aware that the implementation in airbyte-workers for custom dbt transforms is less than ideal. This is mainly due to the fact it runs on a user provided docker image rather than one we've defined. Specifically:

  • Adding ssh script location to airbyte-workers gradle sourceSets to include in resources so we stay DRY.
  • Requiring the user to ensure jq (and sshpass if using password auth) are installed on their provided docker image, albeit with a janky attempt to install these if they aren't present.
    This can definitely be improved (TODO: add link to issue for improving this here)
  • Choosing our port to use in normalization transform.py and then writing this to a file in order to ensure we pick this up and use the same port in the sshtunneling script

New normalization will need to be published before the postgres tests can pass (I think)

Recommended reading order

  1. sshtunneling.sh (core script for opening/closing ssh tunnel)
  2. entrypoint.sh
  3. dbt_transformation_entrypoint.sh
  4. transform.py (normalization)
  5. test_transform_config.py
  6. everything else

Pre-merge Checklist

  • Test/Publish base-normalization

marcosmarxm and others added 30 commits August 21, 2021 23:57
@marcosmarxm
oracle normalization
6d531f9
@marcosmarxm
correct dbt_project function for oracle
078be20
@marcosmarxm
unit tests
b1f4d81
@marcosmarxm
run format
36959b1
@marcosmarxm
Merge branch 'master' into marcosmarxm/normalization-oracle
c22a678
@marcosmarxm
correct ephemeral tests
1c23695
@marcosmarxm
add gradle dependency for oracle destination
f797107
@marcosmarxm
run int tests
545be65
@marcosmarxm
add oracle in settings.gradle for normalization run[
22c903e
@cgardens
autogenerated finals
91db2cd
@cgardens
init
8aaadaa
@cgardens
remove dynamic generation of data
04c09fa
@cgardens
styling
f1bfd1b
@cgardens
clean up
b5aa5f4
@cgardens
clean
800f16c
@cgardens
opinionated capitalization
c78df0b
@cgardens
more opinionated caps
b1943d5
@cgardens
sometimes you have to roll the hard six
8cb4988
@cgardens
victory
df19286
@cgardens
add secrets to ci
e2f30b8
@cgardens
fix those tests
0755c53
@cgardens
clean
6cd9311
@cgardens
wip
817f1fa
@marcosmarxm
use default airbyte columns
53e92c6
@marcosmarxm
format
b529041
@marcosmarxm
test all destinatoin ephemeral
202af8f
@marcosmarxm
correct unit test
5924283
@marcosmarxm
correct unit test
a697133
@marcosmarxm
destination docs update
48a71b0
@marcosmarxm
correct mypy
3b4723d
@Phlair
Copy link
Contributor Author

Phlair commented Sep 7, 2021 •
edited by github-actions bot
Loading

/test connector=bases/base-normalization

🕑 bases/base-normalization https://github.com/airbytehq/airbyte/actions/runs/1209981379
❌ bases/base-normalization https://github.com/airbytehq/airbyte/actions/runs/1209981379

@jrhizor jrhizor temporarily deployed to more-secrets September 7, 2021 15:16 Inactive
@Phlair Phlair mentioned this pull request Sep 7, 2021
Closed
@Phlair
Copy link
Contributor Author

Phlair commented Sep 7, 2021 •
edited by github-actions bot
Loading

/test connector=bases/base-normalization

🕑 bases/base-normalization https://github.com/airbytehq/airbyte/actions/runs/1210540958
✅ bases/base-normalization https://github.com/airbytehq/airbyte/actions/runs/1210540958

@jrhizor jrhizor temporarily deployed to more-secrets September 7, 2021 18:22 Inactive
@Phlair
Merge branch 'marcosmarxm/normalization-oracle' of https://github.com…
1a98048 
…/airbytehq/airbyte into george/new-pg-ssh-norm

# Conflicts:
#	airbyte-integrations/bases/base-normalization/normalization/transform_config/transform.py
@Phlair
added my changes back into transform.py
8be8147
@Phlair
bump version
3cd895e
@github-actions github-actions bot added area/api Related to the api area/platform issues related to the platform area/documentation Improvements or additions to documentation area/frontend labels Sep 7, 2021
@Phlair Phlair removed area/documentation Improvements or additions to documentation area/frontend area/api Related to the api area/platform issues related to the platform labels Sep 7, 2021
@Phlair
Copy link
Contributor Author

Phlair commented Sep 7, 2021 •
edited by github-actions bot
Loading

/test connector=bases/base-normalization

🕑 bases/base-normalization https://github.com/airbytehq/airbyte/actions/runs/1210663941
✅ bases/base-normalization https://github.com/airbytehq/airbyte/actions/runs/1210663941

@jrhizor jrhizor temporarily deployed to more-secrets September 7, 2021 19:05 Inactive
@cgardens cgardens mentioned this pull request Sep 7, 2021
Merged
@cgardens
Copy link
Contributor

cgardens commented Sep 7, 2021

@Phlair - not sure what happened in this PR. I think merging in marcos branch broke the diff. Moved to a separate PR #5897 and cleaned it up and merged that into our base branch. Closing this now as it is duplicate.

@cgardens cgardens closed this Sep 7, 2021
@Phlair
Copy link
Contributor Author

Phlair commented Sep 7, 2021

@cgardens good shout. You may have done this elsewhere but normalisation needs to be published first and then the postgres tests should work 👍

@Phlair Phlair deleted the george/new-pg-ssh-norm branch September 9, 2021 10:57
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ChristopheDuong ChristopheDuong ChristopheDuong approved these changes

@cgardens cgardens cgardens approved these changes

@sherifnada sherifnada Awaiting requested review from sherifnada

Assignees

@Phlair Phlair

Labels
area/connectors Connector related issues area/worker Related to worker normalization
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Normalization: Support SSH tunneling for postgres destination
5 participants
@Phlair @cgardens @ChristopheDuong @jrhizor @marcosmarxm