address review comments

kvalliyurnatt · kvalliyurnatt · commit a5ce7edd81ef · 2025-03-06T15:39:10.000-05:00
diff --git a/pkg/apis/agent/v1alpha1/validations.go b/pkg/apis/agent/v1alpha1/validations.go
@@ -62,7 +62,7 @@ func checkSupportedVersion(a *Agent) field.ErrorList {
 }
 
 func checkIfVersionDeprecated(a *Agent) (string, field.ErrorList) {
-	return commonv1.CheckDeprecatedStackVersion(a.Spec.Version, version.DeprecatedVersions)
+	return commonv1.CheckDeprecatedStackVersion(a.Spec.Version)
 }
 
 func checkAtMostOneDeploymentOption(a *Agent) field.ErrorList {
diff --git a/pkg/apis/agent/v1alpha1/validations_test.go b/pkg/apis/agent/v1alpha1/validations_test.go
@@ -177,7 +177,7 @@ func Test_checkIfVersionDeprecated(t *testing.T) {
 		{
 			name:    "deprecated",
 			version: "7.4.0",
-			want:    "Version 7.4.0 is EOL and will be removed in a future release of the ECK operator",
+			want:    "Version 7.4.0 is EOL and support for it will be removed in a future release of the ECK operator",
 		},
 	}
 
diff --git a/pkg/apis/agent/v1alpha1/webhook.go b/pkg/apis/agent/v1alpha1/webhook.go
@@ -77,6 +77,16 @@ func (a *Agent) WebhookPath() string {
 func (a *Agent) validate(old *Agent) (admission.Warnings, error) {
 	warnings := a.warnings()
 	var errors field.ErrorList
+
+	// depreciation check
+	depreciationWarnings, depreciationErrors := checkIfVersionDeprecated(a)
+	if depreciationErrors != nil {
+		errors = append(errors, depreciationErrors...)
+	}
+	if depreciationWarnings != "" {
+		warnings = append(warnings, depreciationWarnings)
+	}
+
 	if old != nil {
 		for _, uc := range updateChecks {
 			if err := uc(old, a); err != nil {
@@ -95,15 +105,6 @@ func (a *Agent) validate(old *Agent) (admission.Warnings, error) {
 		}
 	}
 
-	// depreciation check
-	depreciationWarnings, depreciationErrors := checkIfVersionDeprecated(a)
-	if depreciationErrors != nil {
-		errors = append(errors, depreciationErrors...)
-	}
-	if depreciationWarnings != "" {
-		warnings = append(warnings, depreciationWarnings)
-	}
-
 	if len(errors) > 0 {
 		return warnings, apierrors.NewInvalid(groupKind, a.Name, errors)
 	}
diff --git a/pkg/apis/apm/v1/webhook.go b/pkg/apis/apm/v1/webhook.go
@@ -128,7 +128,7 @@ func checkSupportedVersion(as *ApmServer) field.ErrorList {
 }
 
 func checkIfVersionDeprecated(as *ApmServer) (string, field.ErrorList) {
-	return commonv1.CheckDeprecatedStackVersion(as.Spec.Version, version.DeprecatedVersions)
+	return commonv1.CheckDeprecatedStackVersion(as.Spec.Version)
 }
 
 func checkNoDowngrade(prev, curr *ApmServer) field.ErrorList {
diff --git a/pkg/apis/apm/v1/webhook_test.go b/pkg/apis/apm/v1/webhook_test.go
@@ -122,8 +122,8 @@ func TestWebhook(t *testing.T) {
 				apm.Spec.Version = "7.4.0"
 				return serialize(t, apm)
 			},
-			Check: test.ValidationWebhookSuceededWithWarnings(
-				`Version 7.4.0 is EOL and will be removed in a future release of the ECK operator`,
+			Check: test.ValidationWebhookSucceededWithWarnings(
+				`Version 7.4.0 is EOL and support for it will be removed in a future release of the ECK operator`,
 			),
 		},
 		{
diff --git a/pkg/apis/apm/v1beta1/webhook.go b/pkg/apis/apm/v1beta1/webhook.go
@@ -122,7 +122,7 @@ func checkSupportedVersion(as *ApmServer) field.ErrorList {
 }
 
 func checkIfVersionDeprecated(as *ApmServer) (string, field.ErrorList) {
-	return commonv1.CheckDeprecatedStackVersion(as.Spec.Version, version.DeprecatedVersions)
+	return commonv1.CheckDeprecatedStackVersion(as.Spec.Version)
 }
 
 func checkNoDowngrade(prev, curr *ApmServer) field.ErrorList {
diff --git a/pkg/apis/apm/v1beta1/webhook_test.go b/pkg/apis/apm/v1beta1/webhook_test.go
@@ -144,8 +144,8 @@ func TestWebhook(t *testing.T) {
 				apm.Spec.Version = "7.4.0"
 				return serialize(t, apm)
 			},
-			Check: test.ValidationWebhookSuceededWithWarnings(
-				`Version 7.4.0 is EOL and will be removed in a future release of the ECK operator`,
+			Check: test.ValidationWebhookSucceededWithWarnings(
+				`Version 7.4.0 is EOL and support for it will be removed in a future release of the ECK operator`,
 			),
 		},
 		{
diff --git a/pkg/apis/beat/v1beta1/validations.go b/pkg/apis/beat/v1beta1/validations.go
@@ -48,7 +48,7 @@ func checkSupportedVersion(b *Beat) field.ErrorList {
 }
 
 func checkIfVersionDeprecated(b *Beat) (string, field.ErrorList) {
-	return commonv1.CheckDeprecatedStackVersion(b.Spec.Version, version.DeprecatedVersions)
+	return commonv1.CheckDeprecatedStackVersion(b.Spec.Version)
 }
 
 func checkAtMostOneDeploymentOption(b *Beat) field.ErrorList {
diff --git a/pkg/apis/beat/v1beta1/validations_test.go b/pkg/apis/beat/v1beta1/validations_test.go
@@ -375,7 +375,7 @@ func Test_checkIfVersionDeprecated(t *testing.T) {
 					Version: "7.10.0",
 				},
 			},
-			want: "Version 7.10.0 is EOL and will be removed in a future release of the ECK operator",
+			want: "Version 7.10.0 is EOL and support for it will be removed in a future release of the ECK operator",
 		},
 	}
 	for _, tt := range tests {
diff --git a/pkg/apis/common/v1/validations.go b/pkg/apis/common/v1/validations.go
@@ -77,14 +77,14 @@ func CheckSupportedStackVersion(ver string, supported version.MinMaxVersion) fie
 }
 
 // CheckDeprecatedStackVersion checks that the given version is not deprecated.
-func CheckDeprecatedStackVersion(ver string, deprecatedVersion version.MinMaxVersion) (string, field.ErrorList) {
+func CheckDeprecatedStackVersion(ver string) (string, field.ErrorList) {
 	v, err := ParseVersion(ver)
 	if err != nil {
 		return "", err
 	}
 
-	if v.Major >= deprecatedVersion.Min.Major && v.Major <= deprecatedVersion.Max.Major && v.Minor >= deprecatedVersion.Min.Minor && v.Minor <= deprecatedVersion.Max.Minor {
-		return fmt.Sprintf("Version %s is EOL and will be removed in a future release of the ECK operator", ver), nil
+	if v.GTE(version.DeprecatedVersions.Min) && v.LT(version.DeprecatedVersions.Max) {
+		return fmt.Sprintf("Version %s is EOL and support for it will be removed in a future release of the ECK operator", ver), nil
 	}
 
 	return "", nil
diff --git a/pkg/apis/enterprisesearch/v1/webhook.go b/pkg/apis/enterprisesearch/v1/webhook.go
@@ -123,7 +123,7 @@ func checkSupportedVersion(ent *EnterpriseSearch) field.ErrorList {
 }
 
 func checkIfVersionDeprecated(ent *EnterpriseSearch) (string, field.ErrorList) {
-	return commonv1.CheckDeprecatedStackVersion(ent.Spec.Version, version.DeprecatedVersions)
+	return commonv1.CheckDeprecatedStackVersion(ent.Spec.Version)
 }
 
 func checkNoDowngrade(prev, curr *EnterpriseSearch) field.ErrorList {
diff --git a/pkg/apis/enterprisesearch/v1/webhook_test.go b/pkg/apis/enterprisesearch/v1/webhook_test.go
@@ -95,8 +95,8 @@ func TestWebhook(t *testing.T) {
 				ent.Spec.Version = "7.10.0"
 				return serialize(t, ent)
 			},
-			Check: test.ValidationWebhookSuceededWithWarnings(
-				`Version 7.10.0 is EOL and will be removed in a future release of the ECK operator`,
+			Check: test.ValidationWebhookSucceededWithWarnings(
+				`Version 7.10.0 is EOL and support for it will be removed in a future release of the ECK operator`,
 			),
 		},
 		{
diff --git a/pkg/apis/enterprisesearch/v1beta1/webhook.go b/pkg/apis/enterprisesearch/v1beta1/webhook.go
@@ -122,7 +122,7 @@ func checkSupportedVersion(ent *EnterpriseSearch) field.ErrorList {
 }
 
 func checkIfVersionDeprecated(ent *EnterpriseSearch) (string, field.ErrorList) {
-	return commonv1.CheckDeprecatedStackVersion(ent.Spec.Version, version.DeprecatedVersions)
+	return commonv1.CheckDeprecatedStackVersion(ent.Spec.Version)
 }
 
 func checkNoDowngrade(prev, curr *EnterpriseSearch) field.ErrorList {
diff --git a/pkg/apis/enterprisesearch/v1beta1/webhook_test.go b/pkg/apis/enterprisesearch/v1beta1/webhook_test.go
@@ -108,8 +108,8 @@ func TestWebhook(t *testing.T) {
 				ent.Spec.Version = "7.10.0"
 				return serialize(t, ent)
 			},
-			Check: test.ValidationWebhookSuceededWithWarnings(
-				`Version 7.10.0 is EOL and will be removed in a future release of the ECK operator`,
+			Check: test.ValidationWebhookSucceededWithWarnings(
+				`Version 7.10.0 is EOL and support for it will be removed in a future release of the ECK operator`,
 			),
 		},
 		{
diff --git a/pkg/apis/kibana/v1/webhook.go b/pkg/apis/kibana/v1/webhook.go
@@ -125,7 +125,7 @@ func checkSupportedVersion(k *Kibana) field.ErrorList {
 }
 
 func checkIfVersionDeprecated(k *Kibana) (string, field.ErrorList) {
-	return commonv1.CheckDeprecatedStackVersion(k.Spec.Version, version.DeprecatedVersions)
+	return commonv1.CheckDeprecatedStackVersion(k.Spec.Version)
 }
 
 func checkNoDowngrade(prev, curr *Kibana) field.ErrorList {
diff --git a/pkg/apis/kibana/v1/webhook_test.go b/pkg/apis/kibana/v1/webhook_test.go
@@ -82,8 +82,8 @@ func TestWebhook(t *testing.T) {
 				k.Spec.Version = "7.10.0"
 				return serialize(t, k)
 			},
-			Check: test.ValidationWebhookSuceededWithWarnings(
-				`Version 7.10.0 is EOL and will be removed in a future release of the ECK operator`,
+			Check: test.ValidationWebhookSucceededWithWarnings(
+				`Version 7.10.0 is EOL and support for it will be removed in a future release of the ECK operator`,
 			),
 		},
 		{
diff --git a/pkg/apis/kibana/v1beta1/webhook.go b/pkg/apis/kibana/v1beta1/webhook.go
@@ -121,7 +121,7 @@ func checkSupportedVersion(k *Kibana) field.ErrorList {
 }
 
 func checkIfVersionDeprecated(k *Kibana) (string, field.ErrorList) {
-	return commonv1.CheckDeprecatedStackVersion(k.Spec.Version, version.DeprecatedVersions)
+	return commonv1.CheckDeprecatedStackVersion(k.Spec.Version)
 }
 
 func checkNoDowngrade(prev, curr *Kibana) field.ErrorList {
diff --git a/pkg/apis/kibana/v1beta1/webhook_test.go b/pkg/apis/kibana/v1beta1/webhook_test.go
@@ -82,8 +82,8 @@ func TestWebhook(t *testing.T) {
 				k.Spec.Version = "7.10.0"
 				return serialize(t, k)
 			},
-			Check: test.ValidationWebhookSuceededWithWarnings(
-				`Version 7.10.0 is EOL and will be removed in a future release of the ECK operator`,
+			Check: test.ValidationWebhookSucceededWithWarnings(
+				`Version 7.10.0 is EOL and support for it will be removed in a future release of the ECK operator`,
 			),
 		},
 		{
diff --git a/pkg/apis/maps/v1alpha1/webhook.go b/pkg/apis/maps/v1alpha1/webhook.go
@@ -103,7 +103,7 @@ func checkSupportedVersion(ems *ElasticMapsServer) field.ErrorList {
 }
 
 func checkIfVersionDeprecated(ems *ElasticMapsServer) (string, field.ErrorList) {
-	return commonv1.CheckDeprecatedStackVersion(ems.Spec.Version, version.DeprecatedVersions)
+	return commonv1.CheckDeprecatedStackVersion(ems.Spec.Version)
 }
 
 func checkAssociation(ems *ElasticMapsServer) field.ErrorList {
diff --git a/pkg/apis/maps/v1alpha1/webhook_test.go b/pkg/apis/maps/v1alpha1/webhook_test.go
@@ -82,8 +82,8 @@ func TestWebhook(t *testing.T) {
 				m.Spec.Version = "7.14.0"
 				return serialize(t, m)
 			},
-			Check: test.ValidationWebhookSuceededWithWarnings(
-				`Version 7.14.0 is EOL and will be removed in a future release of the ECK operator`,
+			Check: test.ValidationWebhookSucceededWithWarnings(
+				`Version 7.14.0 is EOL and support for it will be removed in a future release of the ECK operator`,
 			),
 		},
 		{
diff --git a/pkg/controller/common/version/version.go b/pkg/controller/common/version/version.go
@@ -24,7 +24,7 @@ var GlobalMinStackVersion Version
 // supported Stack versions. See https://www.elastic.co/support/matrix#matrix_compatibility
 var (
 	DeprecatedVersions = MinMaxVersion{Min: From(7, 0, 0), Max: From(7, 16, 0)}
-	// Some of the versions below might be EOL but we do not want to block users from usign ECK with those versions
+	// Some of the versions below might be EOL but we do not want to block users from using ECK with those versions yet.
 	// We will add a warning when users try to create a resource with an EOL version.
 	SupportedAPMServerVersions        = MinMaxVersion{Min: From(7, 0, 0), Max: From(9, 99, 99)}
 	SupportedEnterpriseSearchVersions = MinMaxVersion{Min: From(7, 7, 0), Max: From(8, 99, 99)}
diff --git a/pkg/controller/elasticsearch/validation/validations.go b/pkg/controller/elasticsearch/validation/validations.go
@@ -107,7 +107,7 @@ func check(es esv1.Elasticsearch, validations []validation) (string, field.Error
 	}
 
 	// check if Elasticsearch version is deprecated
-	warnings, deprecatedErrors := checkDeprecatedVersion(&es)
+	warnings, deprecatedErrors := commonv1.CheckDeprecatedStackVersion(es.Spec.Version)
 	if len(deprecatedErrors) > 0 {
 		errs = append(errs, deprecatedErrors...)
 	}
@@ -141,10 +141,6 @@ func supportedVersion(es esv1.Elasticsearch) field.ErrorList {
 	return field.ErrorList{field.Invalid(field.NewPath("spec").Child("version"), es.Spec.Version, unsupportedVersionMsg)}
 }
 
-func checkDeprecatedVersion(es *esv1.Elasticsearch) (string, field.ErrorList) {
-	return commonv1.CheckDeprecatedStackVersion(es.Spec.Version, version.DeprecatedVersions)
-}
-
 func supportsRemoteClusterUsingAPIKey(es esv1.Elasticsearch) field.ErrorList {
 	ver, err := version.Parse(es.Spec.Version)
 	if err != nil {
diff --git a/pkg/controller/elasticsearch/validation/webhook_test.go b/pkg/controller/elasticsearch/validation/webhook_test.go
@@ -161,7 +161,7 @@ func Test_validatingWebhook_Handle(t *testing.T) {
 					}},
 				},
 			},
-			want: admission.Allowed("Version 7.9.0 is EOL and will be removed in a future release of the ECK operator"),
+			want: admission.Allowed("Version 7.9.0 is EOL and support for it will be removed in a future release of the ECK operator"),
 		},
 	}
 	for _, tt := range tests {
diff --git a/pkg/utils/test/webhook.go b/pkg/utils/test/webhook.go
@@ -74,7 +74,7 @@ func ValidationWebhookFailed(causeRegexes ...string) func(*testing.T, *admission
 	}
 }
 
-func ValidationWebhookSuceededWithWarnings(warningsRegexes ...string) func(*testing.T, *admissionv1beta1.AdmissionResponse) {
+func ValidationWebhookSucceededWithWarnings(warningsRegexes ...string) func(*testing.T, *admissionv1beta1.AdmissionResponse) {
 	return func(t *testing.T, response *admissionv1beta1.AdmissionResponse) {
 		t.Helper()
 		require.True(t, response.Allowed, "Request denied: %s", response.Result.Reason)
@@ -89,7 +89,7 @@ func ValidationWebhookSuceededWithWarnings(warningsRegexes ...string) func(*test
 					break
 				}
 			}
-			require.True(t, found, "[%s] is not present in cause list", wr)
+			require.True(t, found, "[%s] is not present in warning list", wr)
 		}
 	}
 }

Original file line number	Diff line number	Diff line change
`@@ -62,7 +62,7 @@ func checkSupportedVersion(a *Agent) field.ErrorList {`
`62`	`62`	`}`
`63`	`63`
`64`	`64`	`func checkIfVersionDeprecated(a *Agent) (string, field.ErrorList) {`
`65`		`- return commonv1.CheckDeprecatedStackVersion(a.Spec.Version, version.DeprecatedVersions)`
	`65`	`+ return commonv1.CheckDeprecatedStackVersion(a.Spec.Version)`
`66`	`66`	`}`
`67`	`67`
`68`	`68`	`func checkAtMostOneDeploymentOption(a *Agent) field.ErrorList {`
Original file line number	Diff line number	Diff line change
`@@ -177,7 +177,7 @@ func Test_checkIfVersionDeprecated(t *testing.T) {`
`177`	`177`	`{`
`178`	`178`	`name: "deprecated",`
`179`	`179`	`version: "7.4.0",`
`180`		`- want: "Version 7.4.0 is EOL and will be removed in a future release of the ECK operator",`
	`180`	`+ want: "Version 7.4.0 is EOL and support for it will be removed in a future release of the ECK operator",`
`181`	`181`	`},`
`182`	`182`	`}`
`183`	`183`
Original file line number	Diff line number	Diff line change
`@@ -128,7 +128,7 @@ func checkSupportedVersion(as *ApmServer) field.ErrorList {`
`128`	`128`	`}`
`129`	`129`
`130`	`130`	`func checkIfVersionDeprecated(as *ApmServer) (string, field.ErrorList) {`
`131`		`- return commonv1.CheckDeprecatedStackVersion(as.Spec.Version, version.DeprecatedVersions)`
	`131`	`+ return commonv1.CheckDeprecatedStackVersion(as.Spec.Version)`
`132`	`132`	`}`
`133`	`133`
`134`	`134`	`func checkNoDowngrade(prev, curr *ApmServer) field.ErrorList {`
Original file line number	Diff line number	Diff line change
`@@ -122,7 +122,7 @@ func checkSupportedVersion(as *ApmServer) field.ErrorList {`
`122`	`122`	`}`
`123`	`123`
`124`	`124`	`func checkIfVersionDeprecated(as *ApmServer) (string, field.ErrorList) {`
`125`		`- return commonv1.CheckDeprecatedStackVersion(as.Spec.Version, version.DeprecatedVersions)`
	`125`	`+ return commonv1.CheckDeprecatedStackVersion(as.Spec.Version)`
`126`	`126`	`}`
`127`	`127`
`128`	`128`	`func checkNoDowngrade(prev, curr *ApmServer) field.ErrorList {`
Original file line number	Diff line number	Diff line change
`@@ -48,7 +48,7 @@ func checkSupportedVersion(b *Beat) field.ErrorList {`
`48`	`48`	`}`
`49`	`49`
`50`	`50`	`func checkIfVersionDeprecated(b *Beat) (string, field.ErrorList) {`
`51`		`- return commonv1.CheckDeprecatedStackVersion(b.Spec.Version, version.DeprecatedVersions)`
	`51`	`+ return commonv1.CheckDeprecatedStackVersion(b.Spec.Version)`
`52`	`52`	`}`
`53`	`53`
`54`	`54`	`func checkAtMostOneDeploymentOption(b *Beat) field.ErrorList {`
Original file line number	Diff line number	Diff line change
`@@ -375,7 +375,7 @@ func Test_checkIfVersionDeprecated(t *testing.T) {`
`375`	`375`	`Version: "7.10.0",`
`376`	`376`	`},`
`377`	`377`	`},`
`378`		`- want: "Version 7.10.0 is EOL and will be removed in a future release of the ECK operator",`
	`378`	`+ want: "Version 7.10.0 is EOL and support for it will be removed in a future release of the ECK operator",`
`379`	`379`	`},`
`380`	`380`	`}`
`381`	`381`	`for _, tt := range tests {`
Original file line number	Diff line number	Diff line change
`@@ -77,14 +77,14 @@ func CheckSupportedStackVersion(ver string, supported version.MinMaxVersion) fie`
`77`	`77`	`}`
`78`	`78`
`79`	`79`	`// CheckDeprecatedStackVersion checks that the given version is not deprecated.`
`80`		`-func CheckDeprecatedStackVersion(ver string, deprecatedVersion version.MinMaxVersion) (string, field.ErrorList) {`
	`80`	`+func CheckDeprecatedStackVersion(ver string) (string, field.ErrorList) {`
`81`	`81`	`v, err := ParseVersion(ver)`
`82`	`82`	`if err != nil {`
`83`	`83`	`return "", err`
`84`	`84`	`}`
`85`	`85`
`86`		`- if v.Major >= deprecatedVersion.Min.Major && v.Major <= deprecatedVersion.Max.Major && v.Minor >= deprecatedVersion.Min.Minor && v.Minor <= deprecatedVersion.Max.Minor {`
`87`		`- return fmt.Sprintf("Version %s is EOL and will be removed in a future release of the ECK operator", ver), nil`
	`86`	`+ if v.GTE(version.DeprecatedVersions.Min) && v.LT(version.DeprecatedVersions.Max) {`
	`87`	`+ return fmt.Sprintf("Version %s is EOL and support for it will be removed in a future release of the ECK operator", ver), nil`
`88`	`88`	`}`
`89`	`89`
`90`	`90`	`return "", nil`