Skip to content

Update Flyte to Go 1.24 #6603

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.

Sign up for GitHub

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

Jump to bottom
Open
wants to merge 4 commits into
base: master
Choose a base branch
from
Open

Update Flyte to Go 1.24 #6603

wants to merge 4 commits into from

Conversation

Sovietaced
Copy link
Member

@Sovietaced Sovietaced commented Aug 25, 2025 •
edited
Loading

Why are the changes needed?

New versions of go include new features, performance improvements, and security fixes.

What changes were proposed in this pull request?

Updates Flyte to use Go 1.24. Changes can be summarized as follows:

  1. Go 1.24 introduces additional checks with go vet which get upset when constants are passed to methods designed to take string formats. Most of the code changes address go vet errors.
  2. Updates mockery to a version that supports Go 1.24. As such generated code has minor changes
  3. They changed the behavior such that seeding rand is a no-op. I decided to refactor CSRF tokens with increased entropy (10 bytes -> 32 bytes) and decided to use a more secure random. Also, I eliminated seedings the CSRF token based on the wall clock timestamp so the values are less predictable for security. This is a change in behavior, but one that should be transparent to the user.

How was this patch tested?

Not tested yet but I can land this in Stack AV's fork.

Check all the applicable boxes

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

Related PRs

Docs link

Summary by Bito

This pull request updates the Flyte project to Go version 1.24, enhancing performance, security, and compatibility. Key changes include addressing go vet errors, updating the mockery dependency, and refactoring CSRF token generation for improved security. Overall, these modifications ensure the project adheres to the latest Go standards while maintaining functionality.

@flyte-bot
Copy link
Collaborator

Bito Automatic Review Skipped - Draft PR

Bito didn't auto-review because this pull request is in draft status.
No action is needed if you didn't intend for the agent to review it. Otherwise, to manually trigger a review, type /review in a comment and save.
You can change draft PR review settings here, or contact your Bito workspace admin at [email protected].

@codecov Codecov
Copy link

codecov bot commented Aug 25, 2025 •
edited
Loading

Codecov Report

❌ Patch coverage is 43.58974% with 44 lines in your changes missing coverage. Please review.
✅ Project coverage is 56.00%. Comparing base (5f26461) to head (64a1c06).

Files with missing lines Patch % Lines
flyteadmin/pkg/errors/errors.go 50.00% 6 Missing ⚠️
...ytepropeller/pkg/controller/nodes/array/handler.go 25.00% 6 Missing ⚠️
flyteadmin/auth/cookie.go 58.33% 3 Missing and 2 partials ⚠️
flyteadmin/auth/handlers.go 16.66% 4 Missing and 1 partial ⚠️
flytestdlib/errors/error.go 0.00% 5 Missing ⚠️
datacatalog/pkg/manager/impl/validators/errors.go 0.00% 3 Missing ⚠️
...yteadmin/pkg/repositories/transformers/workflow.go 0.00% 2 Missing ⚠️
...teplugins/go/tasks/plugins/array/k8s/management.go 33.33% 2 Missing ⚠️
datacatalog/pkg/repositories/errors/postgres.go 0.00% 1 Missing ⚠️
flyteadmin/pkg/manager/impl/launch_plan_manager.go 0.00% 1 Missing ⚠️
... and 8 more
Additional details and impacted files 
@@            Coverage Diff             @@
##           master    #6603      +/-   ##
==========================================
- Coverage   58.59%   56.00%   -2.59%     
==========================================
  Files         929      929              
  Lines       70851    70856       +5     
==========================================
- Hits        41512    39680    -1832     
- Misses      26193    28141    +1948     
+ Partials     3146     3035     -111
Flag Coverage Δ
unittests-datacatalog 52.23% <0.00%> (-6.80%) ⬇️
unittests-flyteadmin 51.75% <45.94%> (-4.39%) ⬇️
unittests-flytecopilot 39.56% <ø> (ø)
unittests-flytectl 63.39% <80.00%> (-1.26%) ⬇️
unittests-flyteidl 73.21% <0.00%> (-2.91%) ⬇️
unittests-flyteplugins 59.35% <33.33%> (-1.75%) ⬇️
unittests-flytepropeller 53.21% <30.76%> (-1.85%) ⬇️
unittests-flytestdlib 62.45% <0.00%> (-0.65%) ⬇️

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.

@flyte-bot
Copy link
Collaborator

Bito Automatic Review Skipped - Draft PR

Bito didn't auto-review because this pull request is in draft status.
No action is needed if you didn't intend for the agent to review it. Otherwise, to manually trigger a review, type /review in a comment and save.
You can change draft PR review settings here, or contact your Bito workspace admin at [email protected].

1 similar comment
@flyte-bot
Copy link
Collaborator

Bito Automatic Review Skipped - Draft PR

Bito didn't auto-review because this pull request is in draft status.
No action is needed if you didn't intend for the agent to review it. Otherwise, to manually trigger a review, type /review in a comment and save.
You can change draft PR review settings here, or contact your Bito workspace admin at [email protected].

@Sovietaced Sovietaced changed the title wip Update Flyte to Go 1.24 Aug 26, 2025
@Sovietaced Sovietaced added housekeeping Issues that help maintain flyte and keep it tech-debt free changed For changes in existing functionality labels Aug 26, 2025
@Sovietaced Sovietaced marked this pull request as ready for review August 26, 2025 03:06
Sovietaced
Sovietaced commented Aug 26, 2025
View reviewed changes
flytestdlib/errors/error.go
}

// Creates a new error using an error code and a message.
func Error(errorCode ErrorCode, msg string) error {
Copy link
Member Author

Choose a reason for hiding this comment

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

Adds a non-format variant to satisfy go vet

Sovietaced
Sovietaced commented Aug 30, 2025
View reviewed changes
flyteadmin/auth/cookie.go
return "", errors.Wrapf(ErrSecureCookie, err, "Error reading secure cookie %s", cookie.Name)
}

func NewCsrfToken(seed int64) string {
Copy link
Member Author

Choose a reason for hiding this comment

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

Removed support for seeding since its apparently unwise to seed the token based on wall clock time, as that makes it potentially predictable for the user.

EngHabu
EngHabu previously approved these changes Aug 30, 2025
View reviewed changes
Copy link
Contributor

@EngHabu EngHabu left a comment

Choose a reason for hiding this comment

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

Changes look good.. will merge once you confirm testing in live service

@Sovietaced
Update Flyte to Go 1.24
bff30da 
Signed-off-by: Jason Parraga <[email protected]>
@Sovietaced
lint fix
0dff174 
Signed-off-by: Jason Parraga <[email protected]>
@Sovietaced
Add back unit test
49578fb 
Signed-off-by: Jason Parraga <[email protected]>
@Sovietaced
make generate
64a1c06 
Signed-off-by: Jason Parraga <[email protected]>
@flyte-bot
Copy link
Collaborator

Bito Automatic Review Failed - Technical Failure

Bito encountered technical difficulties while generating code feedback . To retry, type /review in a comment and save. If the issue persists, contact [email protected] and provide the following details:

Agent Run ID: cb1a2f98-8225-4f27-b0f3-d52831d1f0df

@ddl-ebrown
Copy link
Contributor

Thanks @Sovietaced !

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@EngHabu EngHabu EngHabu left review comments

At least 1 approving review is required to merge this pull request.

Assignees
No one assigned
Labels
changed For changes in existing functionality housekeeping Issues that help maintain flyte and keep it tech-debt free
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@Sovietaced @flyte-bot @ddl-ebrown @EngHabu