-
Notifications
You must be signed in to change notification settings - Fork 4.1k
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
java CDK: build no longer downloads files from connector registry #34441
Conversation
The latest updates on your projects. Learn more about Vercel for Git ↗︎ 1 Ignored Deployment
|
3fdf276
to
89f99af
Compare
Coverage report for source-postgres
|
cad9d5b
to
8c129b9
Compare
80ad63c
to
92e6586
Compare
Confirmed with @alafanechere that the test logs will be scrubbed in github, but it's possible that the test logs end up in some other report, like a gradle scan, and remain unscrubbed. I'll investigate further. |
92e6586
to
a0bac97
Compare
a0bac97
to
4e1a328
Compare
@alafanechere could you review my last commit 4e1a328 please? |
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.
To what extent would it be harder / simpler to create this pattern within the containers under test instead of building the pattern in the Python logic?
I'm also wondering if we could declare this logic within the GradleTask
step.
Slightly out of scope:
Would it also be worth configuring the log scrubbing at the connector image level at runtime: when the config is parsed
@@ -326,3 +327,28 @@ def fail_if_missing_docker_hub_creds(ctx: click.Context) -> None: | |||
raise click.UsageError( | |||
"You need to be logged to DockerHub registry to run this command. Please set DOCKER_HUB_USERNAME and DOCKER_HUB_PASSWORD environment variables." | |||
) | |||
|
|||
|
|||
def java_log_scrub_pattern(secrets_to_mask: List[str]) -> str: |
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.
Could you please add a unit test for this? 🙏
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.
Yes! sorry it slipped my mind
Thanks for taking a look! I'll add the unit test.
Unfortunately, the scrubbing needs to be applied to all logs, including the test framework, and not just the logs emitted by the connector container. Also, not all secrets are equal: the scrubbing should only apply to those secrets downloaded by ci_credentials, we don't want to scrub passwords for testcontainer instances (which furthermore are often dumb strings like "password" or "test").
The integration test task class would be its proper place but for some reason, dagger secrets are defined by the PipelineContext, which isn't something I'm a fan of. This is why I moved the pattern generation logic to
Indeed. @stephane-airbyte and I are also thinking about how to properly scrub secrets in prod. It's a bit more involved in that case. |
4e1a328
to
b96fa1b
Compare
I added a unit test and checked again that the scrubbing works (it does). |
b96fa1b
to
d6c9f7a
Compare
/publish-java-cdk
|
This reverts commit 7ebd202.
These files were part of the CDK build and formed a circular dependency: their content is updated based on the connector metadata when a connector is published. Incidentally, removing these also allows removing the dependencies on micronaut.
This PR is best reviewed commit-by-commit:
DestinationAcceptanceTest
, which is the only downstream dependency, to rely on the connector'smetadata.yaml
file instead of the connector metadata json blob.specs_secrets_mask.yaml
file which was used by a custom log4j plugin to scrub secrets from the logs. The log config in this repo is now only used for testing and since tests are run via airbyte-ci the secrets will already be scrubbed by dagger.