Skip to content

ENG-719: Assume role for SES#6206

Merged
tvandort merged 2 commits intomainfrom
ENG-719
Jun 9, 2025
Merged

ENG-719: Assume role for SES#6206
tvandort merged 2 commits intomainfrom
ENG-719

Conversation

@tvandort
Copy link
Copy Markdown
Contributor

@tvandort tvandort commented Jun 8, 2025 •
edited by atlassian Bot
Loading

Closes ENG-719

Description Of Changes

Allow SES to assume roles.

Pre-Merge Checklist

  • Issue requirements met
  • All CI pipelines succeeded
  • CHANGELOG.md updated
    • Add a db-migration This indicates that a change includes a database migration label to the entry if your change includes a DB migration
    • Add a high-risk This issue suggests changes that have a high-probability of breaking existing code label to the entry if your change includes a high-risk change (i.e. potential for performance impact or unexpected regression) that should be flagged
    • Updates unreleased work already in Changelog, no new entry necessary
  • Followup issues:
    • Followup issues created
    • No followup issues
  • Database migrations:
    • Ensure that your downrev is up to date with the latest revision on main
    • Ensure that your downgrade() migration is correct and works
      • If a downgrade migration is not possible for this change, please call this out in the PR description!
    • No migrations
  • Documentation:
    • Documentation complete, PR opened in fidesdocs
    • Documentation issue created in fidesdocs
    • If there are any new client scopes created as part of the pull request, remember to update public-facing documentation that references our scope registry
    • No documentation updates required

@tvandort tvandort added do not merge Please don't merge yet, bad things will happen if you do dont build Tell the Github Actions CI not to run build (or dependent) steps. labels Jun 8, 2025
@vercel
Copy link
Copy Markdown
Contributor

vercel Bot commented Jun 8, 2025 •
edited
Loading

The latest updates on your projects. Learn more about Vercel for Git ↗︎

2 Skipped Deployments
Name Status Preview Comments Updated (UTC)
fides-plus-nightly ⬜️ Ignored (Inspect) Visit Preview Jun 9, 2025 6:36pm
fides-privacy-center ⬜️ Ignored (Inspect) Jun 9, 2025 6:36pm

@tvandort tvandort force-pushed the ENG-719 branch 2 times, most recently from ae47271 to f24124a Compare June 9, 2025 15:38
@codecov
Copy link
Copy Markdown

codecov Bot commented Jun 9, 2025 •
edited
Loading

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 87.23%. Comparing base (1b11d23) to head (33e464d).
Report is 1 commits behind head on main.

Additional details and impacted files 
@@           Coverage Diff           @@
##             main    #6206   +/-   ##
=======================================
  Coverage   87.23%   87.23%           
=======================================
  Files         427      427           
  Lines       26600    26601    +1     
  Branches     2910     2910           
=======================================
+ Hits        23204    23205    +1     
  Misses       2767     2767           
  Partials      629      629

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@tvandort tvandort removed the dont build Tell the Github Actions CI not to run build (or dependent) steps. label Jun 9, 2025
@tvandort tvandort marked this pull request as ready for review June 9, 2025 17:07
@tvandort tvandort force-pushed the ENG-719 branch 3 times, most recently from 750193f to 7ca53a4 Compare June 9, 2025 17:25
erosselli
erosselli approved these changes Jun 9, 2025
View reviewed changes
Copy link
Copy Markdown
Contributor

@erosselli erosselli left a comment

Choose a reason for hiding this comment

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

Don't forget the changelog entry. I know there's no time for adding tests but maybe we can ticket that separately (we have a tech debt epic) and at some point the eng team (read: likely me) might be able to squeeze it into a sprint

Comment thread src/fides/service/messaging/aws_ses_service.py
Comment on lines +90 to +93
assume_role_arn=CONFIG.credentials.get( # pylint: disable=no-member
"notifications", {}
).get("aws_ses_assume_role_arn")
or self.messaging_config_secrets.aws_assume_role_arn,
Copy link
Copy Markdown
Contributor

@erosselli erosselli Jun 9, 2025

Choose a reason for hiding this comment

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

this looks ok to me -- I'm just wondering what the order should be here , do we usually "prioritize" the env-set credentials? trying to think of what would follow the principle of least surprise for users configuring this integration

Copy link
Copy Markdown
Contributor Author

@tvandort tvandort Jun 9, 2025

Choose a reason for hiding this comment

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

I had a similar thought. It's... hard for me to say. For some reason I have this gut feeling that the most expected thing would be for the API to win over other methods.

But the API is less flexible in some cases bc it's a central point of truth. The reason I need to support this is for use cases where Fides is deployed to multiple regions, and this allows us to configure the environment variable granularly per region so it feels more specific in that sense.

Ultimately we chose to let the environment variable to have precedence before:

fides/src/fides/api/tasks/storage.py

Lines 139 to 141 in 7ca53a4

assume_role_arn=CONFIG.credentials.get( # pylint: disable=no-member
"storage", {}
).get("aws_s3_assume_role_arn"),

fides/src/fides/api/util/aws_util.py

Lines 53 to 54 in 7ca53a4

target_assume_role_arn = assume_role_arn or stored_assume_role_arn
if target_assume_role_arn:

So I want to keep doing that probably?

Copy link
Copy Markdown
Contributor

@erosselli erosselli Jun 9, 2025

Choose a reason for hiding this comment

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

ah that makes sense -- let's keep it consistent with the S3 configuration

@tvandort tvandort merged commit 0154330 into main Jun 9, 2025
40 checks passed
@tvandort tvandort deleted the ENG-719 branch June 9, 2025 19:18
@cypress
Copy link
Copy Markdown

cypress Bot commented Jun 9, 2025

fides    Run #12976

Run Properties:  status check passed Passed #12976  •  git commit 0154330039: ENG-719: Assume role for SES (#6206)
Project fides
Branch Review main
Run status status check passed Passed #12976
Run duration 00m 50s
Commit git commit 0154330039: ENG-719: Assume role for SES (#6206)
Committer Tom Van Dort
View all properties for this run ↗︎
Test results
Tests that failed  Failures 0
Tests that were flaky  Flaky 0
Tests that did not run due to a developer annotating a test with .skip  Pending 0
Tests that did not run due to a failure in a mocha hook  Skipped 0
Tests that passed  Passing 5
View all changes introduced in this branch ↗︎

erosselli pushed a commit that referenced this pull request Jun 12, 2025
@tvandort @erosselli
ENG-719: Assume role for SES (#6206)
0825ba9
erosselli pushed a commit that referenced this pull request Jun 12, 2025
@tvandort @erosselli
ENG-719: Assume role for SES (#6206)
976bfb0
erosselli pushed a commit that referenced this pull request Jun 16, 2025
@tvandort @erosselli
ENG-719: Assume role for SES (#6206)
4bffe33
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@erosselli erosselli erosselli approved these changes

Assignees

No one assigned

Labels

do not merge Please don't merge yet, bad things will happen if you do

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

2 participants

@tvandort @erosselli