From 2d9c35b06f06a642e526c2f56437dfff55d88f35 Mon Sep 17 00:00:00 2001 From: Prafulla Mahindrakar Date: Wed, 18 Sep 2024 08:39:38 -0700 Subject: [PATCH] Add semver check for dev and beta version (#5757) * Add semver check for dev and beta version Signed-off-by: pmahindrakar-oss * add unsupported version tests Signed-off-by: pmahindrakar-oss * passing context and refactored to table driven utests Signed-off-by: pmahindrakar-oss --------- Signed-off-by: pmahindrakar-oss --- .../pkg/controller/config/config.go | 26 ++- .../pkg/controller/config/config_test.go | 158 +++++++++++++----- .../pkg/controller/nodes/common/utils.go | 2 +- 3 files changed, 134 insertions(+), 52 deletions(-) diff --git a/flytepropeller/pkg/controller/config/config.go b/flytepropeller/pkg/controller/config/config.go index f045058e95..f9b46a7037 100644 --- a/flytepropeller/pkg/controller/config/config.go +++ b/flytepropeller/pkg/controller/config/config.go @@ -36,6 +36,7 @@ package config import ( "context" "fmt" + "regexp" "time" "github.com/Masterminds/semver" @@ -133,6 +134,9 @@ var ( MaxSizeInMBForOffloading: 1000, // 1 GB is the default size before failing fast. }, } + + // This regex is used to sanitize semver versions passed to IsSupportedSDK checks for literal offloading feature. + sanitizeProtoRegex = regexp.MustCompile(`v?(\d+\.\d+\.\d+)`) ) // Config that uses the flytestdlib Config module to generate commandline and load config files. This configuration is @@ -187,18 +191,24 @@ type LiteralOffloadingConfig struct { } // IsSupportedSDKVersion returns true if the provided SDK and version are supported by the literal offloading config. -func (l LiteralOffloadingConfig) IsSupportedSDKVersion(sdk string, versionString string) bool { +func (l LiteralOffloadingConfig) IsSupportedSDKVersion(ctx context.Context, sdk string, versionString string) bool { + regexMatches := sanitizeProtoRegex.FindStringSubmatch(versionString) + if len(regexMatches) > 1 { + logger.Infof(ctx, "original: %s, semVer: %s", versionString, regexMatches[1]) + } else { + logger.Warnf(ctx, "no match found for: %s", versionString) + return false + } + version, err := semver.NewVersion(regexMatches[1]) + if err != nil { + logger.Warnf(ctx, "Failed to parse version %s", versionString) + return false + } if leastSupportedVersion, ok := l.SupportedSDKVersions[sdk]; ok { c, err := semver.NewConstraint(fmt.Sprintf(">= %s", leastSupportedVersion)) if err != nil { // This should never happen - logger.Warnf(context.TODO(), "Failed to parse version constraint %s", leastSupportedVersion) - return false - } - version, err := semver.NewVersion(versionString) - if err != nil { - // This should never happen - logger.Warnf(context.TODO(), "Failed to parse version %s", versionString) + logger.Warnf(ctx, "Failed to parse version constraint %s", leastSupportedVersion) return false } return c.Check(version) diff --git a/flytepropeller/pkg/controller/config/config_test.go b/flytepropeller/pkg/controller/config/config_test.go index afc9ed2fea..507643a569 100644 --- a/flytepropeller/pkg/controller/config/config_test.go +++ b/flytepropeller/pkg/controller/config/config_test.go @@ -1,54 +1,126 @@ package config import ( + "context" "testing" "github.com/stretchr/testify/assert" ) func TestIsSupportedSDKVersion(t *testing.T) { - t.Run("supported version", func(t *testing.T) { - config := LiteralOffloadingConfig{ - SupportedSDKVersions: map[string]string{ - "flytekit": "0.16.0", - }, - } - assert.True(t, config.IsSupportedSDKVersion("flytekit", "0.16.0")) - }) - - t.Run("unsupported version", func(t *testing.T) { - config := LiteralOffloadingConfig{ - SupportedSDKVersions: map[string]string{ - "flytekit": "0.16.0", - }, - } - assert.False(t, config.IsSupportedSDKVersion("flytekit", "0.15.0")) - }) - - t.Run("unsupported SDK", func(t *testing.T) { - config := LiteralOffloadingConfig{ - SupportedSDKVersions: map[string]string{ - "flytekit": "0.16.0", - }, - } - assert.False(t, config.IsSupportedSDKVersion("unknown", "0.16.0")) - }) - - t.Run("invalid version", func(t *testing.T) { - config := LiteralOffloadingConfig{ - SupportedSDKVersions: map[string]string{ - "flytekit": "0.16.0", - }, - } - assert.False(t, config.IsSupportedSDKVersion("flytekit", "invalid")) - }) + ctx := context.Background() + tests := []struct { + name string + config LiteralOffloadingConfig + sdk string + version string + expectedResult bool + }{ + { + name: "supported version", + config: LiteralOffloadingConfig{ + SupportedSDKVersions: map[string]string{ + "flytekit": "0.16.0", + }, + }, + sdk: "flytekit", + version: "0.16.0", + expectedResult: true, + }, + { + name: "unsupported version", + config: LiteralOffloadingConfig{ + SupportedSDKVersions: map[string]string{ + "flytekit": "0.16.0", + }, + }, + sdk: "flytekit", + version: "0.15.0", + expectedResult: false, + }, + { + name: "unsupported SDK", + config: LiteralOffloadingConfig{ + SupportedSDKVersions: map[string]string{ + "flytekit": "0.16.0", + }, + }, + sdk: "unknown", + version: "0.16.0", + expectedResult: false, + }, + { + name: "invalid version", + config: LiteralOffloadingConfig{ + SupportedSDKVersions: map[string]string{ + "flytekit": "0.16.0", + }, + }, + sdk: "flytekit", + version: "invalid", + expectedResult: false, + }, + { + name: "invalid constraint", + config: LiteralOffloadingConfig{ + SupportedSDKVersions: map[string]string{ + "flytekit": "invalid", + }, + }, + sdk: "flytekit", + version: "0.16.0", + expectedResult: false, + }, + { + name: "supported dev version", + config: LiteralOffloadingConfig{ + SupportedSDKVersions: map[string]string{ + "flytekit": "1.13.4", + }, + }, + sdk: "flytekit", + version: "1.13.4.dev12+g990b450ea.d20240917", + expectedResult: true, + }, + { + name: "supported beta version", + config: LiteralOffloadingConfig{ + SupportedSDKVersions: map[string]string{ + "flytekit": "1.13.4", + }, + }, + sdk: "flytekit", + version: "v1.13.6b0", + expectedResult: true, + }, + { + name: "unsupported dev version", + config: LiteralOffloadingConfig{ + SupportedSDKVersions: map[string]string{ + "flytekit": "1.13.4", + }, + }, + sdk: "flytekit", + version: "1.13.3.dev12+g990b450ea.d20240917", + expectedResult: false, + }, + { + name: "unsupported beta version", + config: LiteralOffloadingConfig{ + SupportedSDKVersions: map[string]string{ + "flytekit": "1.13.4", + }, + }, + sdk: "flytekit", + version: "v1.13.3b0", + expectedResult: false, + }, + } - t.Run("invalid constraint", func(t *testing.T) { - config := LiteralOffloadingConfig{ - SupportedSDKVersions: map[string]string{ - "flytekit": "invalid", - }, - } - assert.False(t, config.IsSupportedSDKVersion("flytekit", "0.16.0")) - }) + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + result := tt.config.IsSupportedSDKVersion(ctx, tt.sdk, tt.version) + assert.Equal(t, tt.expectedResult, result) + }) + } } diff --git a/flytepropeller/pkg/controller/nodes/common/utils.go b/flytepropeller/pkg/controller/nodes/common/utils.go index 89bb0afe2e..b02d830fe9 100644 --- a/flytepropeller/pkg/controller/nodes/common/utils.go +++ b/flytepropeller/pkg/controller/nodes/common/utils.go @@ -147,7 +147,7 @@ func CheckOffloadingCompat(ctx context.Context, nCtx interfaces.NodeExecutionCon return &phaseInfo } runtimeData := taskNode.CoreTask().GetMetadata().GetRuntime() - if !literalOffloadingConfig.IsSupportedSDKVersion(runtimeData.GetType().String(), runtimeData.GetVersion()) { + if !literalOffloadingConfig.IsSupportedSDKVersion(ctx, runtimeData.GetType().String(), runtimeData.GetVersion()) { if !literalOffloadingConfig.Enabled { errMsg := fmt.Sprintf("task [%s] is trying to consume offloaded literals but feature is not enabled", taskID) logger.Errorf(ctx, errMsg)