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

Support config sub-values for allowedHosts #21950

Merged
merged 3 commits into from
Jan 27, 2023

Conversation

evantahler
Copy link
Contributor

@evantahler evantahler commented Jan 26, 2023

Following #21676 and closes #21913, this PR adds support for allowedHosts that come from sub-keys in the connector's configuration.

For example, configs for postgres sources which use SSH tunnels might look like:

{
  "ssl": false,
  "host": "real-server-ip.com",
  "port": 5432,
  "schemas": [ "public" ],
  "database": "datawarehouse",
  "password": { "_secret": "..." },
  "ssl_mode": { "mode": "disable" },
  "username": "db-username",
  "tunnel_method": {
    "tunnel_host": "1.2.3.4",
    "tunnel_port": 22,
    "tunnel_user": "ssh-username",
    "tunnel_method": "SSH_PASSWORD_AUTH",
    "tunnel_user_password": { "_secret": "..." }
  },
}

Which means allowedHosts should support both host and tunnel_method.tunnel_host.

@octavia-squidington-iv octavia-squidington-iv added area/platform issues related to the platform area/worker Related to worker labels Jan 26, 2023
@evantahler evantahler requested review from a team, benmoriceau, git-phu and davinchia January 26, 2023 23:58
@evantahler evantahler marked this pull request as ready for review January 26, 2023 23:58
@evantahler evantahler temporarily deployed to more-secrets January 26, 2023 23:59 — with GitHub Actions Inactive
@evantahler evantahler temporarily deployed to more-secrets January 26, 2023 23:59 — with GitHub Actions Inactive
Comment on lines 1349 to +1352
allowedHosts:
hosts:
- "${host}"
- "${tunnel_method.tunnel_host}"
Copy link
Contributor Author

Choose a reason for hiding this comment

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

cc @bleonard / @prateekmukhedkar - this should be the complete set of needs for source-postgres!

Copy link
Contributor

Choose a reason for hiding this comment

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

@evantahler I am working on adding allowedHosts to the remaining database sources(link). Reading through this PR, it appears that ${tunnel_method.tunnel_host} is only required for those sources that support connection via SSH tunnels. (for example as of now we don't support connecting via ssh for source-redshift). And for all such sources I will only include ${host} sub-value under allowedHosts. Does that sound correct?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yep!

@github-actions
Copy link
Contributor

github-actions bot commented Jan 27, 2023

Platform Test Results

   234 files  ±0     234 suites  ±0   24m 58s ⏱️ + 1m 37s
1 617 tests +2  1 606 ✔️ +2  11 💤 ±0  0 ±0 
1 637 runs  +2  1 626 ✔️ +2  11 💤 ±0  0 ±0 

Results for commit 147ac5d. ± Comparison against base commit bb0ce26.

♻️ This comment has been updated with latest results.

@evantahler evantahler temporarily deployed to more-secrets January 27, 2023 00:30 — with GitHub Actions Inactive
@evantahler evantahler temporarily deployed to more-secrets January 27, 2023 00:30 — with GitHub Actions Inactive
@evantahler evantahler temporarily deployed to more-secrets January 27, 2023 00:34 — with GitHub Actions Inactive
@evantahler evantahler temporarily deployed to more-secrets January 27, 2023 00:34 — with GitHub Actions Inactive
@github-actions
Copy link
Contributor

github-actions bot commented Jan 27, 2023

Airbyte Code Coverage

File Coverage [95.21%] 🍏
ConfigReplacer.java 95.21% 🍏
Total Project Coverage 24.02%

Copy link
Contributor

@git-phu git-phu left a comment

Choose a reason for hiding this comment

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

Ah ok, makes sense!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/platform issues related to the platform area/worker Related to worker
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Consider DB proxies for AllowedHosts
4 participants