From d7f658ef826b9cbda8680b353c8828400a71e3f2 Mon Sep 17 00:00:00 2001 From: Martin Schuppert Date: Wed, 8 Oct 2025 10:15:09 +0200 Subject: [PATCH] Mirror OpenShift Route admission errors to OpenStackControlPlane conditions Add route admission status checking to ensure that OpenStackControlPlane expose conditions (e.g., OpenStackControlPlaneExposeBarbicanReady) properly reflect errors from the underlying OpenShift Route objects. Previously, the expose conditions would be set based only on whether the route could be created, but would not reflect subsequent admission failures by the OpenShift router (e.g., hostname conflicts, invalid configurations). This could leave the condition in a misleading state where it appeared successful while the route was actually not admitted. Changes: - Add checkRouteAdmissionStatus() helper function to inspect route status.ingress[0].conditions for the "Admitted" condition - Update ensureRoute() to check route admission status after creation and update the condition accordingly - Add OpenStackControlPlaneExposeServiceReadyRouteAdmissionErrorMessage constant for clear error reporting - Set expose conditions to True only when routes are successfully admitted - Set expose conditions to False with detailed error messages when route admission fails This ensures that conditions like OpenStackControlPlaneExposeBarbicanReady, OpenStackControlPlaneExposeKeystoneAPIReady, etc. accurately reflect the actual state of the routes, making debugging easier for operators. The implementation handles edge cases gracefully: - Routes without ingress status yet (during initial creation) - Routes without admission conditions yet (still being processed) - Routes with failed admission (error is surfaced in the condition) - Routes successfully admitted (condition set to True) Note: RouteAdmitted (type "Admitted") is currently the only officially defined condition type in the OpenShift Route API (github.com/openshift/api). The implementation loops through all conditions to be future-proof for when additional condition types are added, but today it will only find the "Admitted" condition. Jira: https://issues.redhat.com/browse/OSPRH-8984 AssistedBy: cloude-4-sonnet Signed-off-by: Martin Schuppert --- apis/core/v1beta1/conditions.go | 3 + go.mod | 1 + pkg/openstack/common.go | 59 ++++ pkg/openstack/common_test.go | 574 ++++++++++++++++++++++++++++++++ 4 files changed, 637 insertions(+) create mode 100644 pkg/openstack/common_test.go diff --git a/apis/core/v1beta1/conditions.go b/apis/core/v1beta1/conditions.go index da0427c41..9d9e55e36 100644 --- a/apis/core/v1beta1/conditions.go +++ b/apis/core/v1beta1/conditions.go @@ -448,6 +448,9 @@ const ( // OpenStackControlPlaneExposeServiceReadyMessage OpenStackControlPlaneExposeServiceReadyMessage = "OpenStackControlPlane %s service exposed" + // OpenStackControlPlaneExposeServiceReadyRouteAdmissionErrorMessage + OpenStackControlPlaneExposeServiceReadyRouteAdmissionErrorMessage = "OpenStackControlPlane %s route %s admission failed: %s" + // OpenStackControlPlaneCAReadyInitMessage OpenStackControlPlaneCAReadyInitMessage = "OpenStackControlPlane CAs not started" diff --git a/go.mod b/go.mod index 823862c6d..01f396b4a 100644 --- a/go.mod +++ b/go.mod @@ -114,6 +114,7 @@ require ( golang.org/x/tools v0.37.0 // indirect gomodules.xyz/jsonpatch/v2 v2.5.0 // indirect google.golang.org/protobuf v1.36.7 // indirect + gopkg.in/evanphx/json-patch.v4 v4.12.0 // indirect gopkg.in/inf.v0 v0.9.1 // indirect k8s.io/apiextensions-apiserver v0.33.2 // indirect k8s.io/klog/v2 v2.130.1 // indirect diff --git a/pkg/openstack/common.go b/pkg/openstack/common.go index 59b86a459..4830d2539 100644 --- a/pkg/openstack/common.go +++ b/pkg/openstack/common.go @@ -516,6 +516,26 @@ func (ed *EndpointDetail) ensureRoute( return ctrlResult, nil } + // Check the route admission status and update condition accordingly + err = checkRouteAdmissionStatus(ctx, helper, ed.Name, ed.Namespace) + if err != nil { + instance.Status.Conditions.Set(condition.FalseCondition( + condType, + condition.ErrorReason, + condition.SeverityWarning, + corev1.OpenStackControlPlaneExposeServiceReadyRouteAdmissionErrorMessage, + owner.GetName(), + ed.Name, + err.Error())) + return ctrl.Result{}, err + } + + // Route is successfully created and admitted + instance.Status.Conditions.MarkTrue( + condType, + corev1.OpenStackControlPlaneExposeServiceReadyMessage, + owner.GetName()) + return ctrl.Result{}, nil } instance.Status.Conditions.Remove(condType) @@ -707,6 +727,45 @@ func (ed *EndpointDetail) CreateRoute( return ctrl.Result{}, nil } +// checkRouteAdmissionStatus checks the admission status of a route and returns an error if not admitted +func checkRouteAdmissionStatus( + ctx context.Context, + helper *helper.Helper, + routeName string, + namespace string, +) error { + serviceRoute := &routev1.Route{} + err := helper.GetClient().Get(ctx, types.NamespacedName{Name: routeName, Namespace: namespace}, serviceRoute) + if err != nil { + return err + } + + // Check if the route has ingress status + if len(serviceRoute.Status.Ingress) == 0 { + // Route exists but has no ingress status yet - this is normal during initial creation + // Return nil to allow reconciliation to continue + return nil + } + + // Check the admission status of the first ingress + for _, condition := range serviceRoute.Status.Ingress[0].Conditions { + // RouteAdmitted (value: "Admitted") is currently the only officially defined condition type in the OpenShift Route API + // https://github.com/openshift/api/blob/c9bef43e850983ce73f69a58b9da0bd02883c26a/route/v1/types.go#L384 + // RouteAdmitted means the route is able to service requests for the provided Host + if condition.Type == routev1.RouteAdmitted { + if condition.Status != k8s_corev1.ConditionTrue { + // Route admission failed - return error with the message + return fmt.Errorf("%s", condition.Message) + } + // Route is admitted successfully + return nil + } + } + + // No admission condition found yet - route is still being processed + return nil +} + // GetEndptCertSecret - func (e *Endpoints) GetEndptCertSecret(endpt service.Endpoint) *string { var endptTLSSecret *string diff --git a/pkg/openstack/common_test.go b/pkg/openstack/common_test.go new file mode 100644 index 000000000..8d4109dfc --- /dev/null +++ b/pkg/openstack/common_test.go @@ -0,0 +1,574 @@ +/* +Copyright 2024. + +Licensed under the Apache License, Version 2.0 (the "License"); +you may not use this file except in compliance with the License. +You may obtain a copy of the License at + + http://www.apache.org/licenses/LICENSE-2.0 + +Unless required by applicable law or agreed to in writing, software +distributed under the License is distributed on an "AS IS" BASIS, +WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +See the License for the specific language governing permissions and +limitations under the License. +*/ + +package openstack + +import ( + "context" + "testing" + + . "github.com/onsi/gomega" //revive:disable:dot-imports + + routev1 "github.com/openshift/api/route/v1" + "github.com/openstack-k8s-operators/lib-common/modules/common/helper" + corev1 "github.com/openstack-k8s-operators/openstack-operator/apis/core/v1beta1" + k8s_corev1 "k8s.io/api/core/v1" + metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" + "k8s.io/apimachinery/pkg/types" + "k8s.io/client-go/kubernetes/fake" + "k8s.io/client-go/kubernetes/scheme" + ctrl "sigs.k8s.io/controller-runtime" + "sigs.k8s.io/controller-runtime/pkg/client" + fakeclient "sigs.k8s.io/controller-runtime/pkg/client/fake" +) + +func init() { + // Register OpenShift route scheme + _ = routev1.AddToScheme(scheme.Scheme) + _ = corev1.AddToScheme(scheme.Scheme) +} + +// setupTestHelper creates a fake client and helper for testing +func setupTestHelper(objects ...client.Object) *helper.Helper { + s := scheme.Scheme + _ = routev1.AddToScheme(s) + _ = corev1.AddToScheme(s) + + fakeClient := fakeclient.NewClientBuilder(). + WithScheme(s). + WithObjects(objects...). + WithStatusSubresource(&routev1.Route{}). + Build() + + // Create a fake kubernetes clientset + fakeKubeClient := fake.NewSimpleClientset() + + // Create a mock OpenStackControlPlane object for the helper + mockObj := &corev1.OpenStackControlPlane{ + ObjectMeta: metav1.ObjectMeta{ + Name: "test-controlplane", + Namespace: "test-namespace", + }, + } + + h, _ := helper.NewHelper( + mockObj, + fakeClient, + fakeKubeClient, + s, + ctrl.Log.WithName("test"), + ) + return h +} + +func TestCheckRouteAdmissionStatus_RouteNotFound(t *testing.T) { + g := NewWithT(t) + ctx := context.Background() + h := setupTestHelper() + + // Test with non-existent route + err := checkRouteAdmissionStatus(ctx, h, "non-existent-route", "test-namespace") + g.Expect(err).To(HaveOccurred()) + g.Expect(err.Error()).To(ContainSubstring("not found")) +} + +func TestCheckRouteAdmissionStatus_NoIngressStatus(t *testing.T) { + g := NewWithT(t) + ctx := context.Background() + + // Create a route without ingress status + route := &routev1.Route{ + ObjectMeta: metav1.ObjectMeta{ + Name: "test-route", + Namespace: "test-namespace", + }, + Spec: routev1.RouteSpec{ + Host: "test.example.com", + }, + Status: routev1.RouteStatus{ + Ingress: []routev1.RouteIngress{}, + }, + } + + h := setupTestHelper(route) + + // Test with route that has no ingress status yet + err := checkRouteAdmissionStatus(ctx, h, "test-route", "test-namespace") + g.Expect(err).ToNot(HaveOccurred(), "Should return nil for routes without ingress status (normal during initial creation)") +} + +func TestCheckRouteAdmissionStatus_AdmittedTrue(t *testing.T) { + g := NewWithT(t) + ctx := context.Background() + + // Create a route with admitted status = true + route := &routev1.Route{ + ObjectMeta: metav1.ObjectMeta{ + Name: "test-route", + Namespace: "test-namespace", + }, + Spec: routev1.RouteSpec{ + Host: "test.example.com", + }, + Status: routev1.RouteStatus{ + Ingress: []routev1.RouteIngress{ + { + Host: "test.example.com", + Conditions: []routev1.RouteIngressCondition{ + { + Type: routev1.RouteAdmitted, + Status: k8s_corev1.ConditionTrue, + }, + }, + }, + }, + }, + } + + h := setupTestHelper(route) + + // Test with successfully admitted route + err := checkRouteAdmissionStatus(ctx, h, "test-route", "test-namespace") + g.Expect(err).ToNot(HaveOccurred(), "Should return nil for successfully admitted routes") +} + +func TestCheckRouteAdmissionStatus_AdmittedFalse(t *testing.T) { + g := NewWithT(t) + ctx := context.Background() + + errorMessage := "HostAlreadyClaimed: route host is already claimed by another route" + + // Create a route with admitted status = false + route := &routev1.Route{ + ObjectMeta: metav1.ObjectMeta{ + Name: "test-route", + Namespace: "test-namespace", + }, + Spec: routev1.RouteSpec{ + Host: "test.example.com", + }, + Status: routev1.RouteStatus{ + Ingress: []routev1.RouteIngress{ + { + Host: "test.example.com", + Conditions: []routev1.RouteIngressCondition{ + { + Type: routev1.RouteAdmitted, + Status: k8s_corev1.ConditionFalse, + Message: errorMessage, + }, + }, + }, + }, + }, + } + + h := setupTestHelper(route) + + // Test with route that failed admission + err := checkRouteAdmissionStatus(ctx, h, "test-route", "test-namespace") + g.Expect(err).To(HaveOccurred(), "Should return error for routes with failed admission") + g.Expect(err.Error()).To(Equal(errorMessage), "Error message should match the route's admission failure message") +} + +func TestCheckRouteAdmissionStatus_NoAdmissionCondition(t *testing.T) { + g := NewWithT(t) + ctx := context.Background() + + // Create a route with ingress status but no admission condition + route := &routev1.Route{ + ObjectMeta: metav1.ObjectMeta{ + Name: "test-route", + Namespace: "test-namespace", + }, + Spec: routev1.RouteSpec{ + Host: "test.example.com", + }, + Status: routev1.RouteStatus{ + Ingress: []routev1.RouteIngress{ + { + Host: "test.example.com", + Conditions: []routev1.RouteIngressCondition{ + { + Type: "SomeOtherCondition", + Status: k8s_corev1.ConditionTrue, + }, + }, + }, + }, + }, + } + + h := setupTestHelper(route) + + // Test with route that has no admission condition yet + err := checkRouteAdmissionStatus(ctx, h, "test-route", "test-namespace") + g.Expect(err).ToNot(HaveOccurred(), "Should return nil for routes without admission condition (still being processed)") +} + +func TestCheckRouteAdmissionStatus_MultipleConditions(t *testing.T) { + g := NewWithT(t) + ctx := context.Background() + + // Create a route with multiple conditions, including admitted = true + route := &routev1.Route{ + ObjectMeta: metav1.ObjectMeta{ + Name: "test-route", + Namespace: "test-namespace", + }, + Spec: routev1.RouteSpec{ + Host: "test.example.com", + }, + Status: routev1.RouteStatus{ + Ingress: []routev1.RouteIngress{ + { + Host: "test.example.com", + Conditions: []routev1.RouteIngressCondition{ + { + Type: "SomeOtherCondition", + Status: k8s_corev1.ConditionTrue, + }, + { + Type: routev1.RouteAdmitted, + Status: k8s_corev1.ConditionTrue, + }, + { + Type: "AnotherCondition", + Status: k8s_corev1.ConditionFalse, + }, + }, + }, + }, + }, + } + + h := setupTestHelper(route) + + // Test with route that has multiple conditions + err := checkRouteAdmissionStatus(ctx, h, "test-route", "test-namespace") + g.Expect(err).ToNot(HaveOccurred(), "Should return nil when admitted condition is true, regardless of other conditions") +} + +func TestCheckRouteAdmissionStatus_AdmittedFalseWithNoMessage(t *testing.T) { + g := NewWithT(t) + ctx := context.Background() + + // Create a route with admitted status = false but no error message + route := &routev1.Route{ + ObjectMeta: metav1.ObjectMeta{ + Name: "test-route", + Namespace: "test-namespace", + }, + Spec: routev1.RouteSpec{ + Host: "test.example.com", + }, + Status: routev1.RouteStatus{ + Ingress: []routev1.RouteIngress{ + { + Host: "test.example.com", + Conditions: []routev1.RouteIngressCondition{ + { + Type: routev1.RouteAdmitted, + Status: k8s_corev1.ConditionFalse, + Message: "", + }, + }, + }, + }, + }, + } + + h := setupTestHelper(route) + + // Test with route that failed admission but has no message + err := checkRouteAdmissionStatus(ctx, h, "test-route", "test-namespace") + g.Expect(err).To(HaveOccurred(), "Should return error for routes with failed admission") + g.Expect(err.Error()).To(Equal(""), "Error message should be empty when route has no failure message") +} + +func TestCheckRouteAdmissionStatus_DifferentFailureMessages(t *testing.T) { + testCases := []struct { + name string + errorMessage string + expectedError string + }{ + { + name: "HostAlreadyClaimed", + errorMessage: "HostAlreadyClaimed: route host is already claimed by another route", + expectedError: "HostAlreadyClaimed: route host is already claimed by another route", + }, + { + name: "RouteNotAdmitted", + errorMessage: "RouteNotAdmitted: host rejected due to validation errors", + expectedError: "RouteNotAdmitted: host rejected due to validation errors", + }, + { + name: "InvalidHost", + errorMessage: "InvalidHost: invalid characters in hostname", + expectedError: "InvalidHost: invalid characters in hostname", + }, + { + name: "NamespaceOwnershipError", + errorMessage: "NamespaceOwnershipError: namespace does not own the host", + expectedError: "NamespaceOwnershipError: namespace does not own the host", + }, + } + + for _, tc := range testCases { + t.Run(tc.name, func(t *testing.T) { + g := NewWithT(t) + ctx := context.Background() + + route := &routev1.Route{ + ObjectMeta: metav1.ObjectMeta{ + Name: "test-route", + Namespace: "test-namespace", + }, + Spec: routev1.RouteSpec{ + Host: "test.example.com", + }, + Status: routev1.RouteStatus{ + Ingress: []routev1.RouteIngress{ + { + Host: "test.example.com", + Conditions: []routev1.RouteIngressCondition{ + { + Type: routev1.RouteAdmitted, + Status: k8s_corev1.ConditionFalse, + Message: tc.errorMessage, + }, + }, + }, + }, + }, + } + + h := setupTestHelper(route) + + err := checkRouteAdmissionStatus(ctx, h, "test-route", "test-namespace") + g.Expect(err).To(HaveOccurred()) + g.Expect(err.Error()).To(Equal(tc.expectedError)) + }) + } +} + +func TestCheckRouteAdmissionStatus_MultipleIngressEntries(t *testing.T) { + g := NewWithT(t) + ctx := context.Background() + + // Create a route with multiple ingress entries (only first should be checked) + route := &routev1.Route{ + ObjectMeta: metav1.ObjectMeta{ + Name: "test-route", + Namespace: "test-namespace", + }, + Spec: routev1.RouteSpec{ + Host: "test.example.com", + }, + Status: routev1.RouteStatus{ + Ingress: []routev1.RouteIngress{ + { + Host: "test.example.com", + Conditions: []routev1.RouteIngressCondition{ + { + Type: routev1.RouteAdmitted, + Status: k8s_corev1.ConditionTrue, + }, + }, + }, + { + Host: "other.example.com", + Conditions: []routev1.RouteIngressCondition{ + { + Type: routev1.RouteAdmitted, + Status: k8s_corev1.ConditionFalse, + Message: "This should be ignored", + }, + }, + }, + }, + }, + } + + h := setupTestHelper(route) + + // Test that only the first ingress entry is checked + err := checkRouteAdmissionStatus(ctx, h, "test-route", "test-namespace") + g.Expect(err).ToNot(HaveOccurred(), "Should only check first ingress entry") +} + +func TestCheckRouteAdmissionStatus_RealWorldScenarios(t *testing.T) { + testCases := []struct { + name string + route *routev1.Route + expectedError bool + errorContains string + description string + }{ + { + name: "Fresh route - no status", + route: &routev1.Route{ + ObjectMeta: metav1.ObjectMeta{ + Name: "barbican-public", + Namespace: "openstack", + }, + Spec: routev1.RouteSpec{ + Host: "barbican.apps.example.com", + To: routev1.RouteTargetReference{ + Kind: "Service", + Name: "barbican-public", + }, + }, + Status: routev1.RouteStatus{}, + }, + expectedError: false, + description: "Newly created route without any status", + }, + { + name: "Route admitted successfully", + route: &routev1.Route{ + ObjectMeta: metav1.ObjectMeta{ + Name: "barbican-public", + Namespace: "openstack", + }, + Spec: routev1.RouteSpec{ + Host: "barbican.apps.example.com", + }, + Status: routev1.RouteStatus{ + Ingress: []routev1.RouteIngress{ + { + Host: "barbican.apps.example.com", + RouterName: "default", + Conditions: []routev1.RouteIngressCondition{ + { + Type: routev1.RouteAdmitted, + Status: k8s_corev1.ConditionTrue, + LastTransitionTime: &metav1.Time{}, + }, + }, + }, + }, + }, + }, + expectedError: false, + description: "Route successfully admitted by router", + }, + { + name: "Route with hostname conflict", + route: &routev1.Route{ + ObjectMeta: metav1.ObjectMeta{ + Name: "barbican-public", + Namespace: "openstack", + }, + Spec: routev1.RouteSpec{ + Host: "barbican.apps.example.com", + }, + Status: routev1.RouteStatus{ + Ingress: []routev1.RouteIngress{ + { + Host: "barbican.apps.example.com", + RouterName: "default", + Conditions: []routev1.RouteIngressCondition{ + { + Type: routev1.RouteAdmitted, + Status: k8s_corev1.ConditionFalse, + Reason: "HostAlreadyClaimed", + Message: "route host barbican.apps.example.com is already claimed by route keystone-public in namespace openstack", + LastTransitionTime: &metav1.Time{}, + }, + }, + }, + }, + }, + }, + expectedError: true, + errorContains: "route host barbican.apps.example.com is already claimed", + description: "Route hostname conflict with another route", + }, + } + + for _, tc := range testCases { + t.Run(tc.name, func(t *testing.T) { + g := NewWithT(t) + ctx := context.Background() + + h := setupTestHelper(tc.route) + + err := checkRouteAdmissionStatus( + ctx, + h, + tc.route.Name, + tc.route.Namespace, + ) + + if tc.expectedError { + g.Expect(err).To(HaveOccurred(), tc.description) + if tc.errorContains != "" { + g.Expect(err.Error()).To(ContainSubstring(tc.errorContains), tc.description) + } + } else { + g.Expect(err).ToNot(HaveOccurred(), tc.description) + } + }) + } +} + +// TestCheckRouteAdmissionStatus_UpdatedStatus tests that we properly handle route status updates +func TestCheckRouteAdmissionStatus_UpdatedStatus(t *testing.T) { + g := NewWithT(t) + ctx := context.Background() + + // Create initial route without status + route := &routev1.Route{ + ObjectMeta: metav1.ObjectMeta{ + Name: "test-route", + Namespace: "test-namespace", + }, + Spec: routev1.RouteSpec{ + Host: "test.example.com", + }, + Status: routev1.RouteStatus{}, + } + + h := setupTestHelper(route) + + // First check - no status + err := checkRouteAdmissionStatus(ctx, h, "test-route", "test-namespace") + g.Expect(err).ToNot(HaveOccurred()) + + // Update route status to admitted + updatedRoute := &routev1.Route{} + err = h.GetClient().Get(ctx, types.NamespacedName{Name: "test-route", Namespace: "test-namespace"}, updatedRoute) + g.Expect(err).ToNot(HaveOccurred()) + + updatedRoute.Status.Ingress = []routev1.RouteIngress{ + { + Host: "test.example.com", + Conditions: []routev1.RouteIngressCondition{ + { + Type: routev1.RouteAdmitted, + Status: k8s_corev1.ConditionTrue, + }, + }, + }, + } + err = h.GetClient().Status().Update(ctx, updatedRoute) + g.Expect(err).ToNot(HaveOccurred()) + + // Second check - should see admitted status + err = checkRouteAdmissionStatus(ctx, h, "test-route", "test-namespace") + g.Expect(err).ToNot(HaveOccurred()) +}