🎉 Core: Support for IRSA when logging on EKS and passing role to Dest/Source Pods #19010

gilandose · 2022-11-06T23:48:36Z

What

DefaultAWSCredentialsProviderChain into the logging route for S3 this allows logs on EKS or EC2 to use standard more secure role assumption methods.

Specifically this unlocks IRSA by removing passing of access keys, and adding STS dependency to allow WebIdentityTokenProvider to be used. Note this would all allow a GKE cluster to interface with AWS using IRSA

Additionally allow the airbyte-admin ServiceAccount to be passed to connectors to allow the IRSA role to be assumed here. Note it may be preferable to assign a different service account for the dest/source pod eventually this could be a connector specific SA with restrictive permissions

resolves #10570
resolves #5942

extends excellent work done in: #10682 credit: @adamschmidt

How

For AWS access keys can automatically be read from the DefaultAWSCredentialsProviderChain, these were being passed around too eagerly and also set into system properties for logging. To change this I've removed some of the explicit passing, and allow the access keys to be passed around transparently when set

🚨 User Impact 🚨

No user impact using ACCESS_KEYS works as before

Pre-merge Checklist

Expand the relevant checklist and delete the others.

Updating a connector

Community member or Airbyter

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
- Changelog updated in docs/integrations/<source or destination>/<name>.md including changelog. See changelog example
PR name follows PR naming conventions

Airbyter

If this is a community PR, the Airbyte engineer reviewing this PR is responsible for the below items.

Create a non-forked branch based on this PR and test the below items on it
Build is successful
If new credentials are required for use in CI, add them to GSM. Instructions.
/test connector=connectors/<name> command is passing
New Connector version released on Dockerhub and connector version bumped by running the /publish command described here

Tests

Unit

Task :airbyte-config:config-models:test

ConfigSchemaTest > testFile() PASSED

ConfigSchemaTest > testPrepareKnownSchemas() PASSED

DataTypeEnumTest > testConversionFromJsonSchemaPrimitiveToDataType() PASSED

EnvConfigsTest > testAirbyteVersion() PASSED

EnvConfigsTest > testGetDatabaseUrl() PASSED

EnvConfigsTest > testSharedJobEnvMapRetrieval() PASSED

EnvConfigsTest > testJobKubeNodeSelectors() PASSED

EnvConfigsTest > testWorkspaceRoot() PASSED

EnvConfigsTest > ensureGetEnvBehavior() PASSED

EnvConfigsTest > testLocalRoot() PASSED

EnvConfigsTest > testDiscoverKubeNodeSelectors() PASSED

EnvConfigsTest > testPublishMetrics() PASSED

EnvConfigsTest > testConfigRoot() PASSED

EnvConfigsTest > testGetDatabasePassword() PASSED

EnvConfigsTest > testSpecKubeNodeSelectors() PASSED

EnvConfigsTest > testAirbyteRole() PASSED

EnvConfigsTest > testCheckKubeNodeSelectors() PASSED

EnvConfigsTest > testDeploymentMode() PASSED

EnvConfigsTest > testTrackingStrategy() PASSED

EnvConfigsTest > testGetWorkspaceDockerMount() PASSED

EnvConfigsTest > testDockerNetwork() PASSED

EnvConfigsTest > testGetLocalDockerMount() PASSED

EnvConfigsTest > testRemoteConnectorCatalogUrl() PASSED

EnvConfigsTest > testGetDatabaseUser() PASSED

EnvConfigsTest > Should parse constant tags PASSED

EnvConfigsTest > testSplitKVPairsFromEnvString() PASSED

EnvConfigsTest > testworkerKubeTolerations() PASSED

EnvConfigsTest > testErrorReportingStrategy() PASSED

EnvConfigsTest > testAllJobEnvMapRetrieval() PASSED

EnvConfigsTest > CheckJobResourceSettings > checkJobCpuRequest should take precedent if set PASSED

EnvConfigsTest > CheckJobResourceSettings > checkJobMemoryLimit should take precedent if set PASSED

EnvConfigsTest > CheckJobResourceSettings > checkJobMemoryRequest should take precedent if set PASSED

EnvConfigsTest > CheckJobResourceSettings > should default to JobMainMemoryRequest if not set PASSED

EnvConfigsTest > CheckJobResourceSettings > should default to JobMainCpuRequest if not set PASSED

EnvConfigsTest > CheckJobResourceSettings > checkJobCpuLimit should take precedent if set PASSED

EnvConfigsTest > CheckJobResourceSettings > should default to JobMainCpuLimit if not set PASSED

EnvConfigsTest > CheckJobResourceSettings > should default to JobMainMemoryLimit if not set PASSED

CloudLogsClientTest > createCloudLogClientTestAws() PASSED

CloudLogsClientTest > createCloudLogClientTestGcs() PASSED

CloudLogsClientTest > createCloudLogClientTestMinio() PASSED

LogClientSingletonTest > testGetJobLogFileNullPath() PASSED

LogClientSingletonTest > testGetJobLogFileEmptyPath() PASSED

LogClientSingletonTest > testGetJobLogFileK8s() PASSED

ScheduleHelpersTest > testGetSecondsInUnit() PASSED

ScheduleHelpersTest > testAllOfTimeUnitEnumValues() PASSED

StateMessageHelperTest > testEmpty() PASSED

StateMessageHelperTest > testDuplicatedGlobalState() PASSED

StateMessageHelperTest > testLegacyInNewFormat() PASSED

StateMessageHelperTest > testLegacyStateConversion() PASSED

StateMessageHelperTest > testEmptyList() PASSED

StateMessageHelperTest > testStreamForceLegacy() PASSED

StateMessageHelperTest > testInvalidMixedState() PASSED

StateMessageHelperTest > testGlobalStateConversion() PASSED

StateMessageHelperTest > testGlobal() PASSED

StateMessageHelperTest > testLegacy() PASSED

StateMessageHelperTest > testGlobalForceLegacy() PASSED

StateMessageHelperTest > testLegacyInList() PASSED

StateMessageHelperTest > testStream() PASSED

StateMessageHelperTest > testStreamStateConversion() PASSED

YamlListToStandardDefinitionsTest > verifyAndConvertToModelList > should error out on bad data PASSED

YamlListToStandardDefinitionsTest > verifyAndConvertToModelList > should error out on duplicate name PASSED

YamlListToStandardDefinitionsTest > verifyAndConvertToModelList > should correctly read yaml file PASSED

YamlListToStandardDefinitionsTest > verifyAndConvertToModelList > should error out on duplicate id PASSED

YamlListToStandardDefinitionsTest > verifyAndConvertToModelList > should error out on empty file PASSED

YamlListToStandardDefinitionsTest > vertifyAndConvertToJsonNode > should error out on bad data PASSED

YamlListToStandardDefinitionsTest > vertifyAndConvertToJsonNode > should error out on duplicate name PASSED

YamlListToStandardDefinitionsTest > vertifyAndConvertToJsonNode > should correctly read yaml file PASSED

YamlListToStandardDefinitionsTest > vertifyAndConvertToJsonNode > should error out on duplicate id PASSED

YamlListToStandardDefinitionsTest > vertifyAndConvertToJsonNode > should error out on empty file PASSED

CloudLogsClientTest > testGcs() PASSED

CloudLogsClientTest > testGcsMissingBucket() PASSED

DefaultS3ClientFactoryTest > testS3() PASSED

DefaultS3ClientFactoryTest > testS3RegionNotSet() PASSED

MinioS3ClientFactoryTest > testMinio() PASSED

MinioS3ClientFactoryTest > testMinioEndpointMissing() PASSED

Integration

Put your integration tests output here.

Acceptance

Put your acceptance tests output here.

CLAassistant · 2022-11-06T23:48:41Z

All committers have signed the CLA.

gilandose · 2022-11-07T00:12:04Z

might be of interest @alafanechere as you reviewed #10682

marcosmarxm · 2022-11-22T20:24:54Z

Hello 👋, first thank you for this amazing contribution.

We really appreciate the effort you've made to improve the project.
We ask you patience for the code review. Last month our team was focused on Hacktoberfest event and that probably left some PR without the proper feedback. And this week, due to the Thanksgiving US Holiday, most our team is out of office with their families. Another important piece of information why code won't be merge this week is: as a safety measure the core team has decided to freeze merging code to main branch to keep the release stable. Next week we'll return to you with the proper code review and update the status of your contribution.

If you have any questions feel free to send me a message in Slack!
Thanks!

nhorton · 2022-12-11T22:46:27Z

Any update on this? We are pretty blocked waiting on this.

benmoriceau · 2022-12-12T19:03:09Z

airbyte-workers/src/main/java/io/airbyte/workers/config/ProcessFactoryBeanFactory.java

@@ -122,10 +126,12 @@ public ProcessFactory discoverDockerProcessFactory(
  public ProcessFactory discoverKubernetesProcessFactory(
                                                         @Named("discoverWorkerConfigs") final WorkerConfigs workerConfigs,
                                                         @Value("${airbyte.worker.job.kube.namespace}") final String kubernetesNamespace,
+                                                         @Value("${airbyte.worker.job.kube.service.account") final String kubernetesServiceAccount,


This value is missing from the file application.yaml in the resource folder of the airbyte-worker module. This will cause some issue while being created.

benmoriceau · 2022-12-12T19:09:19Z

airbyte-commons-worker/src/main/java/io/airbyte/workers/process/KubePodProcess.java

-      podBuilder = podBuilder.withServiceAccount("airbyte-admin").withAutomountServiceAccountToken(true);
-    }
+        .withNewSpec()
+        .withServiceAccount(isOrchestrator ? "airbyte-admin" : serviceAccount)


Should we add a dedicated variable for the orchestrator SA and well as the one added in order to have something like:
.withServiceAccount(isOrchestrator ? orchestratorServiceAccount : serviceAccount). If we go that way, we need to make sure that the default value is "airbyte-admin".

mjstel · 2022-12-16T13:11:15Z

charts/airbyte-worker/templates/deployment.yaml

@@ -119,6 +119,8 @@ spec:
          valueFrom:
            fieldRef:
              fieldPath: metadata.namespace
+        - name: JOB_KUBE_NAMESPACE


This is probably a typo and should be JOB_KUBE_SERVICE_ACCOUNT right?

Suggested change

- name: JOB_KUBE_NAMESPACE

- name: JOB_KUBE_SERVICE_ACCOUNT

This PR has been migrated, @mjstel if you can please re-add your suggestion to the new PR so your authorship is preserved: https://github.com/airbytehq/airbyte-platform/pull/177/files#r1120705574

kube/resources/worker.yaml

marcosmarxm · 2022-12-20T19:51:30Z

Hello 👋:skin-tone-2: and thank you for your contribution!

Airbyte has instituted a code freeze between 19 and 30 December, to make sure there are no disruptions during the holidays.
Because of this, reviewing and merging your contribution may take longer than usual.
We apologize for the delay, but we want everyone to have a quiet and happy holiday.

If you have any questions or need further clarification, please don't hesitate to ping via Slack.

Santhin · 2023-01-30T13:45:04Z

Hey, any update on this?

gilandose · 2023-02-20T13:37:32Z

Hey, any update on this?

will take a look at the review comments

wizardofdevops · 2023-02-27T22:44:40Z

+1 running into this issue with helm chart!

sh4sh · 2023-02-28T20:13:06Z

Hello @gilandose, as you may have noticed we've just finished a pretty big overhaul of our code repositories. Most of this pull request is now located in a new repo airbytehq/airbyte-platform.

I've gone ahead and cherry picked your changes into a new PR: airbytehq/airbyte-platform#177

I was not able to base the PR from your fork as I don't have write permissions, so I created a new branch migration/19010 in airbyte-platform for this purpose. You're more than welcome to make future changes to this work, however you will need to fork and submit a new PR due to repo permissions - if you'd like to do so, please tag me and/or reference this PR so that nothing gets lost. Either way, full authorship will be preserved in the commit history.

I've also copied over the comments/suggestions on this PR, so hopefully we can continue and merge this exciting new feature.

sh4sh · 2023-02-28T21:12:36Z

I've added changes that are included in this pr here: #23594

Alternatively, we can merge from airbytehq/airbyte master and use this PR -- however I don't have write access so I can't push changes. So, we have a new PR in the meantime 😄

gilandose and others added 6 commits November 4, 2022 00:59

allow other types of logging IAM permissioning

8eae2bb

update import

319536d

fixed method

59c1a1b

update to dependencies

3b83418

fix dep

5559164

resolving pod launches and server logging

b8be760

github-actions bot added area/connectors Connector related issues area/documentation Improvements or additions to documentation labels Nov 6, 2022

octavia-squidington-iv added bounty community connectors/destination/s3 labels Nov 6, 2022

gilandose and others added 2 commits November 7, 2022 00:01

formating

a128e7e

Merge branch 'master' into fix-irsa-issues

a6fbae7

gilandose changed the title ~~🎉 Support for IRSA when logging on EKS and passing role to Dest/Source Pods~~ 🎉 Core: Support for IRSA when logging on EKS and passing role to Dest/Source Pods Nov 7, 2022

adjust to build using defaults instead of airbyte-admin

4e438dc

github-actions bot added area/platform issues related to the platform area/worker Related to worker labels Nov 9, 2022

update charts and kustomize

5ba8888

github-actions bot added the kubernetes label Nov 9, 2022

sajarin added internal and removed bounty labels Dec 5, 2022

sajarin requested a review from supertopher December 5, 2022 17:36

supertopher requested review from alovew, benmoriceau and colesnodgrass December 12, 2022 18:57

benmoriceau reviewed Dec 12, 2022

View reviewed changes

mjstel reviewed Dec 16, 2022

View reviewed changes

kube/resources/worker.yaml Show resolved Hide resolved

sajarin requested a review from benmoriceau February 2, 2023 14:09

sh4sh mentioned this pull request Feb 28, 2023

🎉 Core: Support for IRSA when logging on EKS and passing role to Dest/Source Pods airbytehq/airbyte-platform#177

Closed

14 tasks

sh4sh mentioned this pull request Feb 28, 2023

Docs and deps for #19010 #23594

Closed

sh4sh closed this Feb 28, 2023

don-code mentioned this pull request May 27, 2023

✨Destination S3: Support IRSA for AWS Authentication airbytehq/airbyte-platform#244

Closed

2 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

🎉 Core: Support for IRSA when logging on EKS and passing role to Dest/Source Pods #19010

🎉 Core: Support for IRSA when logging on EKS and passing role to Dest/Source Pods #19010

gilandose commented Nov 6, 2022 •

edited

CLAassistant commented Nov 6, 2022 •

edited

gilandose commented Nov 7, 2022

marcosmarxm commented Nov 22, 2022

nhorton commented Dec 11, 2022

benmoriceau Dec 12, 2022

benmoriceau Dec 12, 2022

mjstel Dec 16, 2022 •

edited

sh4sh Feb 28, 2023

marcosmarxm commented Dec 20, 2022

Santhin commented Jan 30, 2023

gilandose commented Feb 20, 2023

wizardofdevops commented Feb 27, 2023

sh4sh commented Feb 28, 2023 •

edited

sh4sh commented Feb 28, 2023 •

edited

🎉 Core: Support for IRSA when logging on EKS and passing role to Dest/Source Pods #19010

🎉 Core: Support for IRSA when logging on EKS and passing role to Dest/Source Pods #19010

Conversation

gilandose commented Nov 6, 2022 • edited

What

How

Recommended reading order

🚨 User Impact 🚨

Pre-merge Checklist

Community member or Airbyter

Airbyter

Tests

CLAassistant commented Nov 6, 2022 • edited

gilandose commented Nov 7, 2022

marcosmarxm commented Nov 22, 2022

nhorton commented Dec 11, 2022

benmoriceau Dec 12, 2022

Choose a reason for hiding this comment

benmoriceau Dec 12, 2022

Choose a reason for hiding this comment

mjstel Dec 16, 2022 • edited

Choose a reason for hiding this comment

sh4sh Feb 28, 2023

Choose a reason for hiding this comment

marcosmarxm commented Dec 20, 2022

Santhin commented Jan 30, 2023

gilandose commented Feb 20, 2023

wizardofdevops commented Feb 27, 2023

sh4sh commented Feb 28, 2023 • edited

sh4sh commented Feb 28, 2023 • edited

gilandose commented Nov 6, 2022 •

edited

CLAassistant commented Nov 6, 2022 •

edited

mjstel Dec 16, 2022 •

edited

sh4sh commented Feb 28, 2023 •

edited

sh4sh commented Feb 28, 2023 •

edited