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

Destination Snowflake: Verify hostname via regex in spec #24615

Merged
merged 5 commits into from
Mar 28, 2023

Conversation

edgao
Copy link
Contributor

@edgao edgao commented Mar 27, 2023

What

Revert the revert (#24405) of the hostname check, and:

  • add an optional https:// prefix
  • add an optional fifth subdomain

Relevant diff is in 9b51107

closes #23172; closes #24409

How

Fetched all our hosts:

select distinct actor.configuration ->> 'host' from connection
inner join jobs on jobs.scope :: uuid = connection.id
inner join actor on actor.id = connection.destination_id
where jobs.status = 'succeeded' and actor.actor_definition_id = '424892c4-daac-4491-b35d-c6688ba547ba'

And verified that the new regex matches all of them: (this query returns no results, i.e. all hostnames matched the regex)

select distinct actor.configuration ->> 'host' as host from connection
inner join jobs on jobs.scope :: uuid = connection.id
inner join actor on actor.id = connection.destination_id
where jobs.status = 'succeeded' and actor.actor_definition_id = '424892c4-daac-4491-b35d-c6688ba547ba'
and actor.configuration ->> 'host' !~ replace('^(http(s)?:\\/\\/)?([^./?#]+\\.)?([^./?#]+\\.)?([^./?#]+\\.)?([^./?#]+\\.snowflakecomputing\\.com)$', '\\', '\')
order by host

(the replace call is because postgres treats '\\' as literally two backslashes)

Also added/modified some tests based on Snowflake documentation.

Recommended reading order

  1. spec.json
  2. SnowflakeDestinationTest.java
  3. dockerfile, snowflake.md

🚨 User Impact 🚨

No breaking change expected. At the very least, this won't break any existing connections in our DB.

This is patch bump because it just reduces the time it takes for us to tell users that their hostname is invalid. Previously, that would fail in check; now the UI can validate it while the user types.

@octavia-squidington-iii octavia-squidington-iii added area/connectors Connector related issues area/documentation Improvements or additions to documentation connectors/destination/snowflake labels Mar 27, 2023
Copy link
Contributor Author

Choose a reason for hiding this comment

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

reviewers, please ignore this file - it's autogenerated and will be overwritten by /publish

Copy link
Contributor Author

Choose a reason for hiding this comment

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

ignore this file also.

@github-actions
Copy link
Contributor

github-actions bot commented Mar 27, 2023

Affected Connector Report

NOTE ⚠️ Changes in this PR affect the following connectors. Make sure to do the following as needed:

  • Run integration tests
  • Bump connector or module version
  • Add changelog
  • Publish the new version

✅ Sources (0)

Connector Version Changelog Publish
  • See "Actionable Items" below for how to resolve warnings and errors.

✅ Destinations (1)

Connector Version Changelog Publish
destination-snowflake 0.4.58
  • See "Actionable Items" below for how to resolve warnings and errors.

👀 Other Modules (1)

  • base-normalization

Actionable Items

(click to expand)

Category Status Actionable Item
Version
mismatch
The version of the connector is different from its normal variant. Please bump the version of the connector.

doc not found
The connector does not seem to have a documentation file. This can be normal (e.g. basic connector like source-jdbc is not published or documented). Please double-check to make sure that it is not a bug.
Changelog
doc not found
The connector does not seem to have a documentation file. This can be normal (e.g. basic connector like source-jdbc is not published or documented). Please double-check to make sure that it is not a bug.

changelog missing
There is no chnagelog for the current version of the connector. If you are the author of the current version, please add a changelog.
Publish
not in seed
The connector is not in the seed file (e.g. source_definitions.yaml), so its publication status cannot be checked. This can be normal (e.g. some connectors are cloud-specific, and only listed in the cloud seed file). Please double-check to make sure that it is not a bug.

diff seed version
The connector exists in the seed file, but the latest version is not listed there. This usually means that the latest version is not published. Please use the /publish command to publish the latest version.

@edgao edgao changed the title Edgao/snowflake hostname check Destination Snowflake: Verify hostname via regex in spec Mar 27, 2023
@edgao
Copy link
Contributor Author

edgao commented Mar 27, 2023

/test connector=connectors/destination-snowflake

🕑 connectors/destination-snowflake https://github.com/airbytehq/airbyte/actions/runs/4536880838
✅ connectors/destination-snowflake https://github.com/airbytehq/airbyte/actions/runs/4536880838
Python tests coverage:

Name                                                              Stmts   Miss  Cover
-------------------------------------------------------------------------------------
normalization/transform_config/__init__.py                            2      0   100%
normalization/transform_catalog/reserved_keywords.py                 15      0   100%
normalization/transform_catalog/__init__.py                           2      0   100%
normalization/destination_type.py                                    18      0   100%
normalization/__init__.py                                             4      0   100%
normalization/transform_catalog/destination_name_transformer.py     171     10    94%
normalization/transform_catalog/table_name_registry.py              174     34    80%
normalization/transform_config/transform.py                         195     48    75%
normalization/transform_catalog/utils.py                             51     14    73%
normalization/transform_catalog/dbt_macro.py                         22      7    68%
normalization/transform_catalog/catalog_processor.py                147     80    46%
normalization/transform_catalog/transform.py                         65     39    40%
normalization/transform_catalog/stream_processor.py                 595    400    33%
-------------------------------------------------------------------------------------
TOTAL                                                              1461    632    57%

Build Passed

Test summary info:

All Passed

@edgao edgao marked this pull request as ready for review March 27, 2023 21:44
@edgao edgao requested a review from a team as a code owner March 27, 2023 21:44
Copy link
Contributor

@ryankfu ryankfu left a comment

Choose a reason for hiding this comment

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

LGTM

@edgao
Copy link
Contributor Author

edgao commented Mar 27, 2023

/publish connector=connectors/destination-snowflake

🕑 Publishing the following connectors:
connectors/destination-snowflake
https://github.com/airbytehq/airbyte/actions/runs/4537209206


Connector Did it publish? Were definitions generated?
connectors/destination-snowflake

if you have connectors that successfully published but failed definition generation, follow step 4 here ▶️

@edgao edgao merged commit 25069a3 into master Mar 28, 2023
@edgao edgao deleted the edgao/snowflake_hostname_check branch March 28, 2023 15:12
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/connectors Connector related issues area/documentation Improvements or additions to documentation connectors/destination/snowflake
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Fix Snowflake host pattern validation Snowflake destination should verify hostname
3 participants