feat: add validation for timeoutInSeconds for MonitorConfig in TrafficManagerProfile

[britaniar]

What type of PR is this?
/kind feature
/kind api-change

What this PR does / why we need it:
Uses CEL to add kubebuilder API validation for field timeoutInSeconds within MonitorConfig in TrafficManagerProfile.

Which issue(s) this PR fixes:

Fixes #

Requirements:

• run make reviewable for basic local test
• Read and followed fleet-networking's Code of conduct.
• uses conventional commit messages
• includes documentation
• adds unit tests

How has this code been tested

API validation integration test

Special notes for your reviewer

If the IntervalInSeconds is set to 30 seconds (default), then you can set the Timeout value between 5 and 10 seconds.
If the IntervalInSeconds is set to 10 seconds, then you can set the Timeout value between 5 and 9 seconds.
As noted here

[kaito-pr-agent]

Title

(Describe updated until commit 877a71a)

Add validation for timeoutInSeconds in MonitorConfig of TrafficManagerProfile

---

Description

• Added CEL validation for timeoutInSeconds in MonitorConfig of TrafficManagerProfile.

• Updated integration tests to validate timeoutInSeconds constraints.

• Expanded API validation tests to cover various scenarios for timeoutInSeconds.

• Updated CRD schema to include validation rules for timeoutInSeconds.

---

Changes walkthrough 📝

	Relevant files
Enhancement	trafficmanagerprofile_types.go Add CEL validation for timeoutInSeconds                                    api/v1beta1/trafficmanagerprofile_types.go Added CEL validation rules for timeoutInSeconds based on intervalInSeconds. +2/-0
Tests	controller_integration_test.go Update integration tests for timeoutInSeconds validation  pkg/controllers/hub/trafficmanagerprofile/controller_integration_test.go Updated tests to ensure timeoutInSeconds validation works as expected. +12/-6    api_validation_integration_test.go Expand API validation tests for timeoutInSeconds                  test/apis/v1beta1/api_validation_integration_test.go Expanded API validation tests to cover different timeoutInSeconds scenarios. +236/-0
Configuration changes	networking.fleet.azure.com_trafficmanagerprofiles.yaml Update CRD schema for timeoutInSeconds validation                config/crd/bases/networking.fleet.azure.com_trafficmanagerprofiles.yaml Updated CRD schema to include validation rules for timeoutInSeconds. +11/-0

---

Need help?

Type /help how to ... in the comments thread for any questions about PR-Agent usage.

Check out the documentation for more information.

[kaito-pr-agent]

PR Reviewer Guide 🔍

(Review updated until commit 877a71a)

Here are some key observations to aid the review process:

⏱️ Estimated effort to review: 3 🔵🔵🔵⚪⚪

🧪 PR contains tests

🔒 No security concerns identified

⚡ No major issues detected

[kaito-pr-agent]

PR Code Suggestions ✨

Latest suggestions up to 877a71a

Explore these optional code suggestions:

Category	Suggestion	Impact
Possible issue	Validate timeoutInSeconds based on intervalInSeconds Ensure that the validation rules are correctly applied to both intervalInSeconds and timeoutInSeconds. api/v1beta1/trafficmanagerprofile_types.go [60-61] +// +kubebuilder:validation:XValidation:rule="has(self.intervalInSeconds) && self.intervalInSeconds == 30 ? (!has(self.timeoutInSeconds) || (self.timeoutInSeconds >= 5 && self.timeoutInSeconds <= 10)) : true",message="timeoutInSeconds must be between 5 and 10 when intervalInSeconds is 30" +// +kubebuilder:validation:XValidation:rule="has(self.intervalInSeconds) && self.intervalInSeconds == 10 ? (!has(self.timeoutInSeconds) || (self.timeoutInSeconds >= 5 && self.timeoutInSeconds <= 9)) : true",message="timeoutInSeconds must be between 5 and 9 when intervalInSeconds is 10" - Suggestion importance[1-10]: 8 __ Why: The suggestion ensures that the validation rules are correctly applied to both intervalInSeconds and timeoutInSeconds, improving the robustness of the code.	Medium
General	Test successful update of timeoutInSeconds Add a test case to verify that updating timeoutInSeconds within the allowed range does not fail. pkg/controllers/hub/trafficmanagerprofile/controller_integration_test.go [157-162] -It("Update the trafficManagerProfile spec and should fail", func() { +It("Update the trafficManagerProfile spec and should succeed", func() { Expect(k8sClient.Get(ctx, types.NamespacedName{Namespace: testNamespace, Name: name}, profile)).Should(Succeed(), "failed to get the trafficManagerProfile") profile.Spec.MonitorConfig.IntervalInSeconds = ptr.To[int64](10) - profile.Spec.MonitorConfig.TimeoutInSeconds = ptr.To[int64](10) - Expect(k8sClient.Update(ctx, profile)).ShouldNot(Succeed(), "failed to update the trafficManagerProfile") + profile.Spec.MonitorConfig.TimeoutInSeconds = ptr.To[int64](7) + Expect(k8sClient.Update(ctx, profile)).Should(Succeed(), "failed to update the trafficManagerProfile") }) Suggestion importance[1-10]: 5 __ Why: While adding a test case to verify successful updates is useful, the current suggestion proposes changing the expected outcome from failure to success, which contradicts the existing test intent. A more appropriate suggestion would be to add a test case for successful updates.	Low
	Improve error message clarity Simplify the error message to focus on the specific validation failure. test/apis/v1beta1/api_validation_integration_test.go [183-188] It("should deny creating API with timeoutInSeconds < 5 when intervalInSeconds is 30", func() { profile := &fleetnetv1beta1.TrafficManagerProfile{ ObjectMeta: objectMetaWithNameValid, Spec: fleetnetv1beta1.TrafficManagerProfileSpec{ MonitorConfig: &fleetnetv1beta1.MonitorConfig{ IntervalInSeconds: ptr.To(int64(30)), TimeoutInSeconds: ptr.To(int64(4)), }, ResourceGroup: "test-resource-group", }, } By("expecting denial of CREATE API with invalid timeoutInSeconds") var err = hubClient.Create(ctx, profile) Expect(errors.As(err, &statusErr)).To(BeTrue(), fmt.Sprintf("Create API call produced error %s. Error type wanted is %s.", reflect.TypeOf(err), reflect.TypeOf(&k8serrors.StatusError{}))) Expect(statusErr.Status().Message).Should(ContainSubstring("spec.monitorConfig.timeoutInSeconds in body should be greater than or equal to 5")) - Expect(statusErr.Status().Message).Should(ContainSubstring("timeoutInSeconds must be between 5 and 10 when intervalInSeconds is 30")) }) Suggestion importance[1-10]: 3 __ Why: Simplifying the error message to focus on the specific validation failure could make the error messages clearer for developers, but the current suggestion does not actually modify the error message content, so it has limited impact.	Low

---

Previous suggestions

Suggestions up to commit f99d868

Category	Suggestion	Impact
Possible issue	Validate timeoutInSeconds based on intervalInSeconds Ensure that the timeoutInSeconds field is validated correctly based on the intervalInSeconds. api/v1beta1/trafficmanagerprofile_types.go [58-61] +// +kubebuilder:validation:XValidation:rule="has(self.intervalInSeconds) && self.intervalInSeconds == 30 ? (!has(self.timeoutInSeconds) || (self.timeoutInSeconds >= 5 && self.timeoutInSeconds <= 10)) : true",message="timeoutInSeconds must be between 5 and 10 when intervalInSeconds is 30" +// +kubebuilder:validation:XValidation:rule="has(self.intervalInSeconds) && self.intervalInSeconds == 10 ? (!has(self.timeoutInSeconds) || (self.timeoutInSeconds >= 5 && self.timeoutInSeconds <= 9)) : true",message="timeoutInSeconds must be between 5 and 9 when intervalInSeconds is 10" - Suggestion importance[1-10]: 8 __ Why: The suggestion ensures that the timeoutInSeconds field is validated correctly based on the intervalInSeconds, which is crucial for maintaining data integrity and preventing invalid configurations.	Medium
General	Test successful update of timeoutInSeconds Add a test case to verify that updating timeoutInSeconds within the allowed range does not fail. pkg/controllers/hub/trafficmanagerprofile/controller_integration_test.go [157-162] -It("Update the trafficManagerProfile spec and should fail", func() { +It("Update the trafficManagerProfile spec and should succeed", func() { Expect(k8sClient.Get(ctx, types.NamespacedName{Namespace: testNamespace, Name: name}, profile)).Should(Succeed(), "failed to get the trafficManagerProfile") profile.Spec.MonitorConfig.IntervalInSeconds = ptr.To[int64](10) - profile.Spec.MonitorConfig.TimeoutInSeconds = ptr.To[int64](10) - Expect(k8sClient.Update(ctx, profile)).ShouldNot(Succeed(), "failed to update the trafficManagerProfile") + profile.Spec.MonitorConfig.TimeoutInSeconds = ptr.To[int64](8) + Expect(k8sClient.Update(ctx, profile)).Should(Succeed(), "failed to update the trafficManagerProfile") }) Suggestion importance[1-10]: 6 __ Why: The suggestion adds a test case to verify that updating timeoutInSeconds within the allowed range does not fail, improving the test coverage and ensuring that the implementation behaves as expected under normal conditions.	Low

Suggestions up to commit 685a78c

Category	Suggestion	Impact
General	Add validation rule for when intervalInSeconds is not defined Ensure that the validation rules cover all possible scenarios for intervalInSeconds. api/v1beta1/trafficmanagerprofile_types.go [60-61] // +kubebuilder:validation:XValidation:rule="has(self.intervalInSeconds) && self.intervalInSeconds == 30 ? (!has(self.timeoutInSeconds) || (self.timeoutInSeconds >= 5 && self.timeoutInSeconds <= 10)) : true",message="timeoutInSeconds must be between 5 and 10 when intervalInSeconds is 30" // +kubebuilder:validation:XValidation:rule="has(self.intervalInSeconds) && self.intervalInSeconds == 10 ? (!has(self.timeoutInSeconds) || (self.timeoutInSeconds >= 5 && self.timeoutInSeconds <= 9)) : true",message="timeoutInSeconds must be between 5 and 9 when intervalInSeconds is 10" +// +kubebuilder:validation:XValidation:rule="!has(self.intervalInSeconds) || (has(self.timeoutInSeconds) && (self.timeoutInSeconds >= 5 && self.timeoutInSeconds <= 10))",message="timeoutInSeconds must be between 5 and 10 when intervalInSeconds is defined" Suggestion importance[1-10]: 8 __ Why: The existing validation rules only cover scenarios where intervalInSeconds is defined. Adding a rule for when intervalInSeconds is not defined ensures comprehensive validation.	Medium

Suggestions up to commit b662d8e

Category	Suggestion	Impact
General	Add validation rule for when intervalInSeconds is not defined Ensure that the validation rules cover all possible scenarios for intervalInSeconds. api/v1beta1/trafficmanagerprofile_types.go [58-61] // +kubebuilder:validation:XValidation:rule="has(self.intervalInSeconds) && self.intervalInSeconds == 30 ? (!has(self.timeoutInSeconds) || (self.timeoutInSeconds >= 5 && self.timeoutInSeconds <= 10)) : true",message="timeoutInSeconds must be between 5 and 10 when intervalInSeconds is 30" // +kubebuilder:validation:XValidation:rule="has(self.intervalInSeconds) && self.intervalInSeconds == 10 ? (!has(self.timeoutInSeconds) || (self.timeoutInSeconds >= 5 && self.timeoutInSeconds <= 9)) : true",message="timeoutInSeconds must be between 5 and 9 when intervalInSeconds is 10" +// +kubebuilder:validation:XValidation:rule="!has(self.intervalInSeconds) || (has(self.timeoutInSeconds) && (self.timeoutInSeconds >= 5 && self.timeoutInSeconds <= 10))",message="timeoutInSeconds must be between 5 and 10 when intervalInSeconds is defined" Suggestion importance[1-10]: 8 __ Why: The existing validation rules do not account for the scenario where intervalInSeconds is not defined. Adding this rule ensures comprehensive validation coverage.	Medium

Suggestions up to commit a0606c5

Category	Suggestion	Impact
General	Add validation rule for intervalInSeconds not equal to 30 Ensure that the validation rules cover all possible scenarios for intervalInSeconds and timeoutInSeconds. api/v1beta1/trafficmanagerprofile_types.go [58-60] // +kubebuilder:validation:XValidation:rule="!has(self.intervalInSeconds) || (has(self.intervalInSeconds) && self.intervalInSeconds == 30) ? (!has(self.timeoutInSeconds) || (self.timeoutInSeconds >= 5 && self.timeoutInSeconds <= 10)) : true",message="timeoutInSeconds must be between 5 and 10 when intervalInSeconds is 30" // +kubebuilder:validation:XValidation:rule="has(self.intervalInSeconds) && self.intervalInSeconds == 10 ? (!has(self.timeoutInSeconds) || (self.timeoutInSeconds >= 5 && self.timeoutInSeconds <= 9)) : true",message="timeoutInSeconds must be between 5 and 9 when intervalInSeconds is 10" +// +kubebuilder:validation:XValidation:rule="!has(self.intervalInSeconds) || (has(self.intervalInSeconds) && self.intervalInSeconds != 30) ? (!has(self.timeoutInSeconds) || (self.timeoutInSeconds >= 5 && self.timeoutInSeconds <= 9)) : true",message="timeoutInSeconds must be between 5 and 9 when intervalInSeconds is not 30" Suggestion importance[1-10]: 7 __ Why: The existing validation rules cover scenarios when intervalInSeconds is 30 or 10. However, there is no rule for other values of intervalInSeconds. Adding a rule for intervalInSeconds not equal to 30 ensures comprehensive validation.	Medium

[codecov]

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 80.28%. Comparing base (1960b9d) to head (877a71a).
Report is 2 commits behind head on main.

Additional details and impacted files

@@            Coverage Diff             @@
##             main     #292      +/-   ##
==========================================
+ Coverage   80.14%   80.28%   +0.14%     
==========================================
  Files          29       29              
  Lines        4065     4099      +34     
==========================================
+ Hits         3258     3291      +33     
- Misses        642      643       +1     
  Partials      165      165              

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