Skip to content

Conversation

ppeerttu
Copy link
Contributor

@ppeerttu ppeerttu commented Apr 1, 2025

Tracking issue

Why are the changes needed?

There is no feasible way to manage Flyte database secret rotation from external secret source when installing Flyte through Helm charts in AWS EKS (I doubt it's possible in other clouds either). This is a common approach taken with AWS RDS, where AWS manages the database secret rotation automatically through secrets manager. While external secrets operator is able to update Kubernetes native secrets based on changes in an external source (e.g. AWS Secrets Manager), it doesn't restart pods mounting those secrets, leaving the rotation incomplete in this case.

What changes were proposed in this pull request?

Add ability to annotate Kubernetes deployments created by the flyte-core Helm chart. This makes it possible to use external operators, such as Reloader, to rotate the pods using the secrets.

How was this patch tested?

Labels

  • added: Ability to configure Helm chart deployment annotations

Setup process

Screenshots

Check all the applicable boxes

  • I updated the documentation accordingly (auto-updated)
  • All new and existing tests passed.
  • All commits are signed-off.

Related PRs

Docs link

Summary by Bito

This PR adds configurable deployment annotations to Helm charts, enabling integration with external secret management tools like Reloader. It implements conditional annotation blocks across deployment templates based on configuration values, updates values.yaml with annotation guidance, and fixes secret rotation in Docker manifests for improved security.

Unit tests added: False

Estimated effort to review (1-5, lower is better): 2

Copy link

welcome bot commented Apr 1, 2025

Thank you for opening this pull request! 🙌

These tips will help get your PR across the finish line:

  • Most of the repos have a PR template; if not, fill it out to the best of your knowledge.
  • Sign off your commits (Reference: DCO Guide).

@flyte-bot
Copy link
Collaborator

flyte-bot commented Apr 1, 2025

Code Review Agent Run Status

  • Limitations and other issues: ❌ Failure - Bito Automatic Review Skipped - Draft PR

    Bito didn't auto-review because this pull request is in draft status.
    To trigger review, mark the PR as ready or type /review in the comment and save.
    You can change draft PR review settings here, or contact the agent instance creator at [email protected].

@ppeerttu ppeerttu force-pushed the feature/configure-deployment-annotations branch from caf3043 to 9d71fb7 Compare April 1, 2025 10:14
@ppeerttu ppeerttu marked this pull request as ready for review April 1, 2025 11:13
@flyte-bot
Copy link
Collaborator

flyte-bot commented Apr 1, 2025

Code Review Agent Run #3cb024

Actionable Suggestions - 0
Review Details
  • Files reviewed - 11 · Commit Range: 9d71fb7..9d71fb7
    • charts/flyte-core/templates/admin/deployment.yaml
    • charts/flyte-core/templates/clusterresourcesync/deployment.yaml
    • charts/flyte-core/templates/console/deployment.yaml
    • charts/flyte-core/templates/datacatalog/deployment.yaml
    • charts/flyte-core/templates/flytescheduler/deployment.yaml
    • charts/flyte-core/templates/propeller/deployment.yaml
    • charts/flyte-core/templates/propeller/webhook.yaml
    • charts/flyte-core/values.yaml
    • docker/sandbox-bundled/manifests/complete-agent.yaml
    • docker/sandbox-bundled/manifests/complete.yaml
    • docker/sandbox-bundled/manifests/dev.yaml
  • Files skipped - 1
    • charts/flyte-core/README.md - Reason: Filter setting
  • Tools
    • Whispers (Secret Scanner) - ✔︎ Successful
    • Detect-secrets (Secret Scanner) - ✔︎ Successful

Bito Usage Guide

Commands

Type the following command in the pull request comment and save the comment.

  • /review - Manually triggers a full AI review.

Refer to the documentation for additional commands.

Configuration

This repository uses code_review_bito You can customize the agent settings here or contact your Bito workspace admin at [email protected].

Documentation & Help

AI Code Review powered by Bito Logo

@flyte-bot
Copy link
Collaborator

flyte-bot commented Apr 1, 2025

Changelist by Bito

This pull request implements the following key changes.

Key Change Files Impacted
Feature Improvement - Deployment Annotations Configurations

deployment.yaml - Added conditional annotations block based on configuration values to support external secret operators.

deployment.yaml - Inserted annotations condition checking to allow dynamic annotation configuration.

deployment.yaml - Enabled conditional annotations insertion for deployment, improving integration with pod restart tools.

deployment.yaml - Implemented annotations block conditioned on configuration to support external annotation inputs.

deployment.yaml - Introduced annotations handling to enable external secret rotation mechanisms.

deployment.yaml - Added annotations insertion logic to improve deployment configurability.

webhook.yaml - Integrated conditional annotations block in the webhook deployment template.

values.yaml - Updated with commented annotations placeholders for Flyte deployments, standardizing configuration across services.

Bug Fix - Secret updates in docker manifests

complete-connector.yaml - Updated haSharedSecret and checksum/secret values for correct secret rotation handling.

complete.yaml - Revised secret values to fix inconsistencies in secret management.

dev.yaml - Modified secret values and checksums to align with enhanced security practices.

Copy link

codecov bot commented Apr 6, 2025

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 58.49%. Comparing base (cb56df3) to head (340b844).
Report is 2 commits behind head on master.

Additional details and impacted files
@@           Coverage Diff           @@
##           master    #6385   +/-   ##
=======================================
  Coverage   58.49%   58.49%           
=======================================
  Files         940      940           
  Lines       71566    71566           
=======================================
  Hits        41860    41860           
  Misses      26525    26525           
  Partials     3181     3181           
Flag Coverage Δ
unittests-datacatalog 59.03% <ø> (ø)
unittests-flyteadmin 56.27% <ø> (ø)
unittests-flytecopilot 30.99% <ø> (ø)
unittests-flytectl 64.72% <ø> (ø)
unittests-flyteidl 76.12% <ø> (ø)
unittests-flyteplugins 60.95% <ø> (ø)
unittests-flytepropeller 54.79% <ø> (ø)
unittests-flytestdlib 64.04% <ø> (ø)

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ 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.

Sovietaced
Sovietaced previously approved these changes Apr 17, 2025
Copy link
Member

@Sovietaced Sovietaced left a comment

Choose a reason for hiding this comment

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

This looks reasonable to me.

@Sovietaced
Copy link
Member

@ppeerttu can you regenerate the helm stuff to resolve the merge conflicts?

@ppeerttu
Copy link
Contributor Author

@ppeerttu can you regenerate the helm stuff to resolve the merge conflicts?

@Sovietaced will do as soon as I'm back at the office, probably next Thursday!

@ppeerttu ppeerttu force-pushed the feature/configure-deployment-annotations branch from 9d71fb7 to 340b844 Compare April 23, 2025 06:31
@ppeerttu ppeerttu requested a review from Sovietaced April 23, 2025 06:32
@flyte-bot
Copy link
Collaborator

flyte-bot commented Apr 25, 2025

Code Review Agent Run #419294

Actionable Suggestions - 0
Additional Suggestions - 4
  • charts/flyte-core/templates/datacatalog/deployment.yaml - 1
    • Redundant nested structure for annotations rendering · Line 8-13
  • charts/flyte-core/templates/clusterresourcesync/deployment.yaml - 1
    • Redundant nesting of annotations variable check · Line 8-13
  • charts/flyte-core/templates/console/deployment.yaml - 1
    • Redundant conditional checks for annotations block · Line 8-13
  • charts/flyte-core/templates/admin/deployment.yaml - 1
    • Redundant nested template logic for annotations · Line 8-13
Review Details
  • Files reviewed - 11 · Commit Range: 340b844..340b844
    • charts/flyte-core/templates/admin/deployment.yaml
    • charts/flyte-core/templates/clusterresourcesync/deployment.yaml
    • charts/flyte-core/templates/console/deployment.yaml
    • charts/flyte-core/templates/datacatalog/deployment.yaml
    • charts/flyte-core/templates/flytescheduler/deployment.yaml
    • charts/flyte-core/templates/propeller/deployment.yaml
    • charts/flyte-core/templates/propeller/webhook.yaml
    • charts/flyte-core/values.yaml
    • docker/sandbox-bundled/manifests/complete-connector.yaml
    • docker/sandbox-bundled/manifests/complete.yaml
    • docker/sandbox-bundled/manifests/dev.yaml
  • Files skipped - 1
    • charts/flyte-core/README.md - Reason: Filter setting
  • Tools
    • Whispers (Secret Scanner) - ✔︎ Successful
    • Detect-secrets (Secret Scanner) - ✔︎ Successful

Bito Usage Guide

Commands

Type the following command in the pull request comment and save the comment.

  • /review - Manually triggers a full AI review.

Refer to the documentation for additional commands.

Configuration

This repository uses code_review_bito You can customize the agent settings here or contact your Bito workspace admin at [email protected].

Documentation & Help

AI Code Review powered by Bito Logo

@Sovietaced
Copy link
Member

@ppeerttu I don't know why the doc build is failing, maybe try rebasing?

@ppeerttu
Copy link
Contributor Author

@Sovietaced to me it looks like master is broken - I think the latest merged PR somehow managed to break the docs build. I'll see if I can find time to debug the build locally and come up with a fix.

@Sovietaced
Copy link
Member

@Sovietaced to me it looks like master is broken - I think the latest merged PR somehow managed to break the docs build. I'll see if I can find time to debug the build locally and come up with a fix.

I made a PR lately and it had no issues building: #6419

Which is why I thought it's maybe not up to date and rebasing on main would help.

@Sovietaced Sovietaced added the added Merged changes that add new functionality label Apr 29, 2025
@Sovietaced
Copy link
Member

Worked with the maintainers to remove the build step. Should be able to merge now.

@Sovietaced Sovietaced merged commit 3a50800 into flyteorg:master Apr 29, 2025
54 of 55 checks passed
Copy link

welcome bot commented Apr 29, 2025

Congrats on merging your first pull request! 🎉

@ppeerttu ppeerttu deleted the feature/configure-deployment-annotations branch April 30, 2025 05:02
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

added Merged changes that add new functionality

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants