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

airbyte worker should be configured with the correct secrets manager based on env vars configured #20997

Closed
wants to merge 2 commits into from
Closed

airbyte worker should be configured with the correct secrets manager based on env vars configured #20997

wants to merge 2 commits into from

Conversation

mohitreddy1996
Copy link
Contributor

@mohitreddy1996 mohitreddy1996 commented Jan 3, 2023

What

Fixes the bug in the Airbyte worker, where the persistent secret state manager is not initialized correctly or consistently with Airbyte server.

Support for AWS secret manager was added in - #19690.

If environment variables are set to use AWS secret manager, Airbyte server creates the secrets correctly, but the worker crashes with -

airbyte-09454b13-worker-77557fd7f8-r7vd5 airbyte-worker-container java.lang.RuntimeException: That secret was not found in the store! Coordinate: airbyte_workspace_00000000-0000-0000-0000-000000000000_secret_3c6975e3-b6d9-4a9f-a787-febd00621327_v1
airbyte-09454b13-worker-77557fd7f8-r7vd5 airbyte-worker-container 	at io.airbyte.config.persistence.split_secrets.SecretsHelpers.getOrThrowSecretValue(SecretsHelpers.java:267) ~[io.airbyte.airbyte-config-config-persistence-0.40.26.jar:?]
airbyte-09454b13-worker-77557fd7f8-r7vd5 airbyte-worker-container 	at io.airbyte.config.persistence.split_secrets.SecretsHelpers.combineConfig(SecretsHelpers.java:119) ~[io.airbyte.airbyte-config-config-persistence-0.40.26.jar:?]
airbyte-09454b13-worker-77557fd7f8-r7vd5 airbyte-worker-container 	at io.airbyte.config.persistence.split_secrets.SecretsHelpers.lambda$combineConfig$1(SecretsHelpers.java:132) ~[io.airbyte.airbyte-config-config-persistence-0.40.26.jar:?]
airbyte-09454b13-worker-77557fd7f8-r7vd5 airbyte-worker-container 	at java.util.Iterator.forEachRemaining(Iterator.java:133) ~[?:?]
airbyte-09454b13-worker-77557fd7f8-r7vd5 airbyte-worker-container 	at io.airbyte.config.persistence.split_secrets.SecretsHelpers.combineConfig(SecretsHelpers.java:123) ~[io.airbyte.airbyte-config-config-persistence-0.40.26.jar:?]
airbyte-09454b13-worker-77557fd7f8-r7vd5 airbyte-worker-container 	at io.airbyte.config.persistence.split_secrets.SecretsHelpers.lambda$combineConfig$1(SecretsHelpers.java:132) ~[io.airbyte.airbyte-config-config-persistence-0.40.26.jar:?]
airbyte-09454b13-worker-77557fd7f8-r7vd5 airbyte-worker-container 	at java.util.Iterator.forEachRemaining(Iterator.java:133) ~[?:?]
airbyte-09454b13-worker-77557fd7f8-r7vd5 airbyte-worker-container 	at io.airbyte.config.persistence.split_secrets.SecretsHelpers.combineConfig(SecretsHelpers.java:123) ~[io.airbyte.airbyte-config-config-persistence-0.40.26.jar:?]
airbyte-09454b13-worker-77557fd7f8-r7vd5 airbyte-worker-container 	at io.airbyte.config.persistence.split_secrets.RealSecretsHydrator.hydrate(RealSecretsHydrator.java:22) ~[io.airbyte.airbyte-config-config-persistence-0.40.26.jar:?]
airbyte-09454b13-worker-77557fd7f8-r7vd5 airbyte-worker-container 	at io.airbyte.workers.temporal.check.connection.CheckConnectionActivityImpl.runWithJobOutput(CheckConnectionActivityImpl.java:91) ~[io.airbyte-airbyte-workers-0.40.26.jar:?]
airbyte-09454b13-worker-77557fd7f8-r7vd5 airbyte-worker-container 	at jdk.internal.reflect.DirectMethodHandleAccessor.invoke(DirectMethodHandleAccessor.java:104) ~[?:?]
airbyte-09454b13-worker-77557fd7f8-r7vd5 airbyte-worker-container 	at java.lang.reflect.Method.invoke(Method.java:578) ~[?:?]
airbyte-09454b13-worker-77557fd7f8-r7vd5 airbyte-worker-container 	at io.temporal.internal.activity.RootActivityInboundCallsInterceptor$POJOActivityInboundCallsInterceptor.executeActivity(RootActivityInboundCallsInterceptor.java:64) ~[temporal-sdk-1.17.0.jar:?]
airbyte-09454b13-worker-77557fd7f8-r7vd5 airbyte-worker-container 	at io.temporal.internal.activity.RootActivityInboundCallsInterceptor.execute(RootActivityInboundCallsInterceptor.java:43) ~[temporal-sdk-1.17.0.jar:?]
airbyte-09454b13-worker-77557fd7f8-r7vd5 airbyte-worker-container 	at io.temporal.internal.activity.ActivityTaskExecutors$BaseActivityTaskExecutor.execute(ActivityTaskExecutors.java:95) ~[temporal-sdk-1.17.0.jar:?]

How

Describe the solution

  • Create the secret manager singleton based on the configuration micronaut configuration derived from env vars

Recommended reading order

  1. application.yml
  2. SecretPersistenceBeanFactory.java

🚨 User Impact 🚨

Are there any breaking changes? What is the end result perceived by the user? If yes, please merge this PR with the 🚨🚨 emoji so changelog authors can further highlight this if needed.

NO.

Pre-merge Checklist

Expand the relevant checklist and delete the others.

Community member or Airbyter

  • Community member? Grant edit access to maintainers (instructions)
  • Secrets in the connector's spec are annotated with airbyte_secret
  • Unit & integration tests added and passing. Community members, please provide proof of success locally e.g: screenshot or copy-paste unit, integration, and acceptance test output. To run acceptance tests for a Python connector, follow instructions in the README. For java connectors run ./gradlew :airbyte-integrations:connectors:<name>:integrationTest.
  • Code reviews completed
  • Documentation updated
    • Connector's README.md
    • Connector's bootstrap.md. See description and examples
    • docs/integrations/<source or destination>/<name>.md including changelog. See changelog example
    • docs/integrations/README.md
    • airbyte-integrations/builds.md
  • PR name follows PR naming conventions

Tests

Unit

File change does not seem to have an unit test or maybe I am wrong. Could you point me to the right file?

Integration

File change does not seem to have an integration test or maybe I am wrong. Could you point me to the right file?

Acceptance

Put your acceptance tests output here.

@octavia-squidington-iv octavia-squidington-iv added area/platform issues related to the platform area/worker Related to worker community labels Jan 3, 2023
@mohitreddy1996 mohitreddy1996 changed the title airbyte worker should be configured with the correct secrets manager … airbyte worker should be configured with the correct secrets manager based on env vars configured Jan 3, 2023
@mohitreddy1996 mohitreddy1996 marked this pull request as ready for review January 3, 2023 23:43
@mohitreddy1996
Copy link
Contributor Author

tagging - @pmossman and @mauricioalarcon who were original PR's reviewer and author respectively.

Would love your feedback and approvals if the change makes sense.

@pmossman pmossman temporarily deployed to more-secrets January 4, 2023 21:26 — with GitHub Actions Inactive
@pmossman pmossman temporarily deployed to more-secrets January 4, 2023 21:26 — with GitHub Actions Inactive
@pmossman
Copy link
Contributor

@mohitreddy1996 apologies for not following up here sooner - it looks like a fix for this issue was merged into master this morning as of #21073. Can you confirm and close this PR if it does indeed fix the issue?

@mohitreddy1996
Copy link
Contributor Author

@pmossman yeah the PR looks good! Thank you! :)

@mohitreddy1996 mohitreddy1996 deleted the mohit/fix-worker-configuration-aws-secret-manager branch January 21, 2023 00:13
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 community
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants