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

CDK: Improve filter_secrets skip empty secret #15684

Merged
merged 3 commits into from
Aug 16, 2022
Merged

CDK: Improve filter_secrets skip empty secret #15684

merged 3 commits into from
Aug 16, 2022

Conversation

grubberr
Copy link
Contributor

@grubberr grubberr commented Aug 16, 2022 •
edited
Loading

Signed-off-by: Sergey Chvalyuk grubberr@gmail.com

What

source-bing-ads has empty secret value on purpose:

$ cat secrets/config.json |  jq .client_secret
""

it produce "strange" log output like this:

{"type": "LOG", "log": {"level": "INFO", "message": "****C****h****e****c****k**** ****s****u****c****c****e****e****d****e****d****"}}

How

Don't replace empty string with 4 starts

@grubberr
Improve filter_secrets skip empty string
b24376b 
Signed-off-by: Sergey Chvalyuk <grubberr@gmail.com>
@grubberr grubberr requested a review from a team as a code owner August 16, 2022 07:30
@github-actions github-actions bot added the CDK Connector Development Kit label Aug 16, 2022
@grubberr grubberr self-assigned this Aug 16, 2022
sherifnada
sherifnada reviewed Aug 16, 2022
View reviewed changes
airbyte-cdk/python/airbyte_cdk/utils/airbyte_secrets_utils.py
@@ -67,5 +67,6 @@ def filter_secrets(string: str) -> str:
# TODO this should perform a maximal match for each secret. if "x" and "xk" are both secret values, and this method is called twice on
# the input "xk", then depending on call order it might only obfuscate "*k". This is a bug.
for secret in __SECRETS_FROM_CONFIG:
string = string.replace(str(secret), "****")
if secret:
Copy link
Contributor

Choose a reason for hiding this comment

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

could you unit test this behavior please?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

ok I will add now

@grubberr
test_secret_filtering updated
ff5911a 
Signed-off-by: Sergey Chvalyuk <grubberr@gmail.com>
@grubberr
update changelog message
484fce4 
Signed-off-by: Sergey Chvalyuk <grubberr@gmail.com>
@grubberr
Copy link
Contributor Author

grubberr commented Aug 16, 2022 •
edited by github-actions bot
Loading

/publish-cdk dry-run=true

🕑 https://github.com/airbytehq/airbyte/actions/runs/2869191103
✅ https://github.com/airbytehq/airbyte/actions/runs/2869191103

@grubberr
Copy link
Contributor Author

grubberr commented Aug 16, 2022 •
edited by github-actions bot
Loading

/publish-cdk dry-run=false

🕑 https://github.com/airbytehq/airbyte/actions/runs/2869371088
✅ https://github.com/airbytehq/airbyte/actions/runs/2869371088

@grubberr grubberr merged commit 4e6cb05 into master Aug 16, 2022
@grubberr grubberr deleted the grubberr/airbyte_cdk_filter_secrets_empty_string branch August 16, 2022 16:28
@grubberr grubberr changed the title CDK: Improve filter_secrets skip empty string CDK: Improve filter_secrets skip empty secret Aug 16, 2022
@octavia-squidington-iii octavia-squidington-iii mentioned this pull request Aug 19, 2022
Closed
rodireich pushed a commit that referenced this pull request Aug 20, 2022
@grubberr @rodireich
CDK: Improve filter_secrets skip empty string (#15684)
8258c4e 
* Improve `filter_secrets` skip empty string

Signed-off-by: Sergey Chvalyuk <grubberr@gmail.com>
@evantahler evantahler mentioned this pull request Aug 22, 2022
Merged
@edgao
Copy link
Contributor

edgao commented Aug 25, 2022

what's the process for getting this update into source-bing-ads? Do I just need to publish a new version?

@grubberr
Copy link
Contributor Author 

user@hrk1-ldl-A02942:~$ docker run -ti --rm --entrypoint /bin/bash airbyte/source-bing-ads:0.1.10 
root@c15119154049:/airbyte/integration_code# pip3 freeze | grep -i cdk
airbyte-cdk==0.1.74

@edgao yes we just need to re-release source-bing-ads to catch up new version of airbye_cdk==0.1.75

@edgao edgao mentioned this pull request Aug 25, 2022
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@sherifnada sherifnada sherifnada approved these changes

@arsenlosenko arsenlosenko arsenlosenko approved these changes

@bazarnov bazarnov bazarnov approved these changes

@Phlair Phlair Awaiting requested review from Phlair

Assignees

@grubberr grubberr

Labels
CDK Connector Development Kit
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
5 participants
@grubberr @edgao @sherifnada @arsenlosenko @bazarnov