fix: aws session creation is failing for s3 manager when roles are used #2799

koladilip · 2022-12-12T11:11:20Z

Description

Unify AWS session config creation process so that it is consistent across all AWS destinations. Earlier S3 has slightly different process to create SessionConfing so when updated the session config creation in aws utils to support RoleBasedAuth flag, it got missed for S3 manager as it was using different implementation so this unification should prevent such errors in the future.

Notion Ticket

[Warehouse AWS Role support] https://www.notion.so/rudderstacks/Warehouse-destination-config-for-Role-base-Authentication-control-plane-db9198dce2024723959c224e91a11ef4

Security

The code changed/added as part of this pull request won't create any security issues with how the software is being used.

sachin-chand-tc

LGTM.

codecov · 2022-12-12T11:45:03Z

Codecov Report

Base: 49.62% // Head: 46.92% // Decreases project coverage by -2.70% ⚠️

Coverage data is based on head (2bda02d) compared to base (e30fe25).
Patch coverage: 79.48% of modified lines in pull request are covered.

Additional details and impacted files

@@                Coverage Diff                @@
##           release/1.4.x    #2799      +/-   ##
=================================================
- Coverage          49.62%   46.92%   -2.71%     
=================================================
  Files                311      301      -10     
  Lines              52797    49161    -3636     
=================================================
- Hits               26199    23067    -3132     
+ Misses             25062    24628     -434     
+ Partials            1536     1466      -70

Impacted Files	Coverage Δ
warehouse/utils/utils.go	`70.05% <0.00%> (-3.38%)`	⬇️
services/filemanager/filemanager.go	`79.64% <50.00%> (+1.01%)`	⬆️
services/filemanager/s3manager.go	`78.21% <92.30%> (-0.56%)`	⬇️
utils/awsutils/session.go	`96.96% <100.00%> (+0.30%)`	⬆️
enterprise/config-env/noop.go	`0.00% <0.00%> (-100.00%)`	⬇️
testhelper/destination/redis.go	`0.00% <0.00%> (-32.73%)`	⬇️
router/throttler/throttler.go	`35.16% <0.00%> (-27.03%)`	⬇️
config/load.go	`72.43% <0.00%> (-25.51%)`	⬇️
regulation-worker/internal/client/client.go	`66.42% <0.00%> (-17.85%)`	⬇️
testhelper/health/checker.go	`71.42% <0.00%> (-11.91%)`	⬇️
... and 59 more

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report at Codecov.
📢 Do you have feedback about the report comment? Let us know in this issue.

atzoum · 2022-12-12T11:51:53Z

@koladilip what are we fixing and what issue did this cause? ref

atzoum · 2022-12-12T12:02:22Z

services/filemanager/s3manager_test.go

-func TestGetSessionConfigWithAccessKeys(t *testing.T) {
-	s3Manager := S3Manager{
-		Config: &S3Config{
-			Bucket:      "someBucket",
-			AccessKeyID: "someAccessKeyId",
-			AccessKey:   "someSecretAccessKey",
-			Region:      aws.String("someRegion"),
-		},
-	}
-	awsSessionConfig := s3Manager.getSessionConfig()
-	assert.NotNil(t, awsSessionConfig)
-	assert.Equal(t, s3Manager.Config.AccessKey, awsSessionConfig.AccessKey)
-	assert.Equal(t, s3Manager.Config.AccessKeyID, awsSessionConfig.AccessKeyID)
-}
-
-func TestGetSessionConfigWithIAMRole(t *testing.T) {
-	s3Manager := S3Manager{
-		Config: &S3Config{
-			Bucket:     "someBucket",
-			IAMRoleARN: "someIAMRole",
-			ExternalID: "someExternalID",
-			Region:     aws.String("someRegion"),
-		},
-	}
-	awsSessionConfig := s3Manager.getSessionConfig()
-	assert.NotNil(t, awsSessionConfig)
-	assert.Equal(t, s3Manager.Config.IAMRoleARN, awsSessionConfig.IAMRoleARN)
-	assert.Equal(t, s3Manager.Config.ExternalID, awsSessionConfig.ExternalID)
-}


Aren't these meaningful anymore? Are we introducing any other tests to verify that:

what we are fixing is actually fixed

no regression has been introduced in the meantime by these changes

Yes s3Manager.getSessionConfig() is removed so I have added few more tests to test these functionalities.

koladilip · 2022-12-12T12:32:36Z

what are we fixing and what issue did this cause? ref

@atzoum
Since S3 manager was using different implementation, when we refactored Session config creation in awsutils, we forgot to update the S3 manager and it introduced the bug so unifying all of them to use same implementation should fix the bug.

atzoum · 2022-12-12T12:33:58Z

what are we fixing and what issue did this cause? ref

@atzoum Since S3 manager was using different implementation, when we refactored Session config creation in awsutils, we forgot the S3 manager and it introduced the bug so unifying all of them to use same implementation should fix the bug.

What was the issue this bug caused? Can you please include it in the pull request's title?

koladilip · 2022-12-12T13:10:44Z

what are we fixing and what issue did this cause? ref

@atzoum Since S3 manager was using different implementation, when we refactored Session config creation in awsutils, we forgot the S3 manager and it introduced the bug so unifying all of them to use same implementation should fix the bug.

What was the issue this bug caused? Can you please include it in the pull request's title?

Updated the PR title, let me know if this explains the problem.

atzoum · 2022-12-12T13:16:19Z

what are we fixing and what issue did this cause? ref

@atzoum Since S3 manager was using different implementation, when we refactored Session config creation in awsutils, we forgot the S3 manager and it introduced the bug so unifying all of them to use same implementation should fix the bug.

What was the issue this bug caused? Can you please include it in the pull request's title?

Updated the PR title, let me know if this explains the problem.

From the title I understand that this is not really a bug, but a new feature. Did we ever support role based auth flag in s3 manager?

koladilip · 2022-12-12T13:19:48Z

what are we fixing and what issue did this cause? ref

@atzoum Since S3 manager was using different implementation, when we refactored Session config creation in awsutils, we forgot the S3 manager and it introduced the bug so unifying all of them to use same implementation should fix the bug.

What was the issue this bug caused? Can you please include it in the pull request's title?

Updated the PR title, let me know if this explains the problem.

From the title I understand that this is not really a bug, but a new feature. Did we ever support role based auth flag in s3 manager?

Actually this is bug as main aws session creator assumes that session config contain rolebased auth flag set properly but s3 manager didn't implement that functionality before so aws session creation is failing for s3 manage when roles are used.

atzoum · 2022-12-12T13:23:18Z

what are we fixing and what issue did this cause? ref

@atzoum Since S3 manager was using different implementation, when we refactored Session config creation in awsutils, we forgot the S3 manager and it introduced the bug so unifying all of them to use same implementation should fix the bug.

What was the issue this bug caused? Can you please include it in the pull request's title?

Updated the PR title, let me know if this explains the problem.

From the title I understand that this is not really a bug, but a new feature. Did we ever support role based auth flag in s3 manager?

Actually this is bug as main aws session creator assumes that session config contain rolebased auth flag set properly but s3 manager didn't implement that functionality before so aws session creation is failing for s3 manage when roles are used.

This should be the PR title then?
fix: aws session creation is failing for s3 manager when roles are used

koladilip · 2022-12-12T13:54:00Z

what are we fixing and what issue did this cause? ref

@atzoum Since S3 manager was using different implementation, when we refactored Session config creation in awsutils, we forgot the S3 manager and it introduced the bug so unifying all of them to use same implementation should fix the bug.

What was the issue this bug caused? Can you please include it in the pull request's title?

Updated the PR title, let me know if this explains the problem.

From the title I understand that this is not really a bug, but a new feature. Did we ever support role based auth flag in s3 manager?

Actually this is bug as main aws session creator assumes that session config contain rolebased auth flag set properly but s3 manager didn't implement that functionality before so aws session creation is failing for s3 manage when roles are used.

This should be the PR title then? fix: aws session creation is failing for s3 manager when roles are used

Updated.

sundernagesh18 · 2022-12-12T15:12:02Z

Tested the latest AWS FTR changes on the S3 datalake and the changes are working fine.

Redshift to use awsSession.Config.Credentials.Get() when roles are used to get temporary credentials instead of sts.GetSessionToken. Unify AWS session config creation process so that it is consistent across all AWS destinations. Earlier S3 has slightly different process to create SessionConfing so when updated the session config creation in aws utils to support RoleBasedAuth flag, it got missed for S3 manager as it was using different implementation so this unification should prevent such errors in the future.

koladilip requested review from krishna2020, sachin-chand-tc, chandumlg, achettyiitr and saikumarrs December 12, 2022 11:11

github-actions bot added server-team with tests labels Dec 12, 2022

koladilip changed the base branch from master to release/1.4.x December 12, 2022 11:11

koladilip force-pushed the fix.s3_manager_populate_role_based_auth_flag branch from cd33f92 to d98c944 Compare December 12, 2022 11:23

koladilip changed the title ~~fix: populate role base auth flag in s3 manager~~ fix: populate role based auth flag in s3 manager Dec 12, 2022

sachin-chand-tc approved these changes Dec 12, 2022

View reviewed changes

koladilip force-pushed the fix.s3_manager_populate_role_based_auth_flag branch from d98c944 to 9d5961c Compare December 12, 2022 11:33

atzoum reviewed Dec 12, 2022

View reviewed changes

koladilip force-pushed the fix.s3_manager_populate_role_based_auth_flag branch from 9d5961c to 56182c0 Compare December 12, 2022 12:14

koladilip changed the title ~~fix: populate role based auth flag in s3 manager~~ fix: unify session config creation for all aws destinations Dec 12, 2022

koladilip changed the title ~~fix: unify session config creation for all aws destinations~~ fix: use same utils to create session config for all aws destinations Dec 12, 2022

saikumarrs approved these changes Dec 12, 2022

View reviewed changes

koladilip force-pushed the fix.s3_manager_populate_role_based_auth_flag branch from 56182c0 to 3ff22a0 Compare December 12, 2022 13:02

achettyiitr approved these changes Dec 12, 2022

View reviewed changes

koladilip changed the title ~~fix: use same utils to create session config for all aws destinations~~ fix: add support for role based auth flag to s3 manager Dec 12, 2022

koladilip changed the title ~~fix: add support for role based auth flag to s3 manager~~ fix: aws session creation is failing for s3 manager when roles are used Dec 12, 2022

atzoum requested a review from BonapartePC December 12, 2022 14:36

koladilip force-pushed the fix.s3_manager_populate_role_based_auth_flag branch from 3ff22a0 to 2bda02d Compare December 13, 2022 13:32

github-actions bot added warehouse-team and removed server-team labels Dec 13, 2022

achettyiitr merged commit 1534d64 into release/1.4.x Dec 13, 2022

achettyiitr deleted the fix.s3_manager_populate_role_based_auth_flag branch December 13, 2022 16:17

rudder-server-bot mentioned this pull request Dec 13, 2022

chore: release 1.4.5 #2802

Merged

koladilip restored the fix.s3_manager_populate_role_based_auth_flag branch December 13, 2022 16:22

koladilip mentioned this pull request Dec 13, 2022

fix: aws session creation is failing for s3 manager when roles are used #2807

Closed

1 task

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix: aws session creation is failing for s3 manager when roles are used #2799

fix: aws session creation is failing for s3 manager when roles are used #2799

koladilip commented Dec 12, 2022 •

edited

Loading

sachin-chand-tc left a comment

codecov bot commented Dec 12, 2022 •

edited

Loading

atzoum commented Dec 12, 2022 •

edited by koladilip

Loading

atzoum Dec 12, 2022

koladilip Dec 12, 2022

koladilip commented Dec 12, 2022 •

edited

Loading

atzoum commented Dec 12, 2022

koladilip commented Dec 12, 2022 •

edited

Loading

atzoum commented Dec 12, 2022

koladilip commented Dec 12, 2022

atzoum commented Dec 12, 2022 •

edited by koladilip

Loading

koladilip commented Dec 12, 2022

sundernagesh18 commented Dec 12, 2022

fix: aws session creation is failing for s3 manager when roles are used #2799

fix: aws session creation is failing for s3 manager when roles are used #2799

Conversation

koladilip commented Dec 12, 2022 • edited Loading

Description

Notion Ticket

Security

sachin-chand-tc left a comment

Choose a reason for hiding this comment

codecov bot commented Dec 12, 2022 • edited Loading

Codecov Report

atzoum commented Dec 12, 2022 • edited by koladilip Loading

atzoum Dec 12, 2022

Choose a reason for hiding this comment

koladilip Dec 12, 2022

Choose a reason for hiding this comment

koladilip commented Dec 12, 2022 • edited Loading

atzoum commented Dec 12, 2022

koladilip commented Dec 12, 2022 • edited Loading

atzoum commented Dec 12, 2022

koladilip commented Dec 12, 2022

atzoum commented Dec 12, 2022 • edited by koladilip Loading

koladilip commented Dec 12, 2022

sundernagesh18 commented Dec 12, 2022

koladilip commented Dec 12, 2022 •

edited

Loading

codecov bot commented Dec 12, 2022 •

edited

Loading

atzoum commented Dec 12, 2022 •

edited by koladilip

Loading

koladilip commented Dec 12, 2022 •

edited

Loading

koladilip commented Dec 12, 2022 •

edited

Loading

atzoum commented Dec 12, 2022 •

edited by koladilip

Loading