Copilot commented on code in PR #815: URL: https://github.com/apache/solr-operator/pull/815#discussion_r2726795855
########## tests/e2e/solrcloud_gateway_test.go: ########## @@ -0,0 +1,281 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one or more + * contributor license agreements. See the NOTICE file distributed with + * this work for additional information regarding copyright ownership. + * The ASF licenses this file to You 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 e2e + +import ( + "context" + solrv1beta1 "github.com/apache/solr-operator/api/v1beta1" + "github.com/apache/solr-operator/controllers/util" + . "github.com/onsi/ginkgo/v2" + . "github.com/onsi/gomega" +) + +var _ = FDescribe("E2E - SolrCloud - Gateway API", func() { + var ( + solrCloud *solrv1beta1.SolrCloud + gatewayNamespace = "default" + gatewayName = "test-gateway" + ) + + BeforeEach(func() { + solrCloud = generateBaseSolrCloud(1) + solrCloud.Spec.SolrAddressability = solrv1beta1.SolrAddressabilityOptions{ + External: &solrv1beta1.ExternalAddressability{ + Method: solrv1beta1.Gateway, + UseExternalAddress: true, + DomainName: testDomain, + Gateway: &solrv1beta1.SolrGatewayOptions{ + ParentRefs: []solrv1beta1.GatewayParentReference{ + { + Name: gatewayName, + Namespace: &gatewayNamespace, + }, + }, + }, + }, + } + }) + + JustBeforeEach(func(ctx context.Context) { + By("creating the SolrCloud") + Expect(k8sClient.Create(ctx, solrCloud)).To(Succeed()) + + DeferCleanup(func(ctx context.Context) { + cleanupTest(ctx, solrCloud) + }) + + By("Waiting for the SolrCloud to come up healthy") + solrCloud = expectSolrCloudToBeReady(ctx, solrCloud) + + By("creating a first Solr Collection") + createAndQueryCollection(ctx, solrCloud, "basic", 1, 1) + }) + + FContext("Can Remove HTTPRoutes and Services when changing addressability", func() { + + FIt("Can adapt to changing needs", func(ctx context.Context) { Review Comment: The test uses `FIt` (focused test marker) which should not be committed. This causes only the focused tests to run, skipping all other tests. Please change all instances of `FIt` to `It` before merging. ```suggestion It("Can adapt to changing needs", func(ctx context.Context) { ``` ########## controllers/solrcloud_controller.go: ########## @@ -436,6 +441,223 @@ func (r *SolrCloudReconciler) Reconcile(ctx context.Context, req ctrl.Request) ( } } + // Reconcile Gateway API HTTPRoutes + if extAddressabilityOpts != nil && extAddressabilityOpts.Method == solrv1beta1.Gateway { + // Validate that gateway config is provided + if extAddressabilityOpts.Gateway == nil || len(extAddressabilityOpts.Gateway.ParentRefs) == 0 { + return requeueOrNot, fmt.Errorf("gateway.parentRefs is required when using method=Gateway") + } + + // Reconcile Common HTTPRoute (if not hidden) + if !extAddressabilityOpts.HideCommon { + commonHTTPRoute := util.GenerateCommonHTTPRoute(instance, solrNodeNames) + commonHTTPRouteLogger := logger.WithValues("httproute", commonHTTPRoute.Name) + foundCommonHTTPRoute := &gatewayv1.HTTPRoute{} + err = r.Get(ctx, types.NamespacedName{Name: commonHTTPRoute.Name, Namespace: commonHTTPRoute.Namespace}, foundCommonHTTPRoute) + if err != nil && errors.IsNotFound(err) { + commonHTTPRouteLogger.Info("Creating common HTTPRoute") + if err = controllerutil.SetControllerReference(instance, commonHTTPRoute, r.Scheme); err == nil { + err = r.Create(ctx, commonHTTPRoute) + } + } else if err == nil { + var needsUpdate bool + needsUpdate, err = util.OvertakeControllerRef(instance, foundCommonHTTPRoute, r.Scheme) + needsUpdate = util.CopyHTTPRouteFields(commonHTTPRoute, foundCommonHTTPRoute, commonHTTPRouteLogger) || needsUpdate + + if needsUpdate && err == nil { + commonHTTPRouteLogger.Info("Updating common HTTPRoute") + err = r.Update(ctx, foundCommonHTTPRoute) + } + } + if err != nil { + return requeueOrNot, err + } + } else { + // Delete common HTTPRoute if it exists but should be hidden + foundCommonHTTPRoute := &gatewayv1.HTTPRoute{} + err := r.Get(ctx, types.NamespacedName{Name: instance.CommonHTTPRouteName(), Namespace: instance.GetNamespace()}, foundCommonHTTPRoute) + if err == nil { + logger.Info("Deleting common HTTPRoute (hideCommon=true)") + err = r.Delete(ctx, foundCommonHTTPRoute) + if err != nil && !errors.IsNotFound(err) { + return requeueOrNot, err + } + } + } + + // Reconcile Node HTTPRoutes (if not hidden) + if !extAddressabilityOpts.HideNodes { + for _, nodeName := range solrNodeNames { + nodeHTTPRoute := util.GenerateNodeHTTPRoute(instance, nodeName) + nodeHTTPRouteLogger := logger.WithValues("httproute", nodeHTTPRoute.Name) + foundNodeHTTPRoute := &gatewayv1.HTTPRoute{} + err = r.Get(ctx, types.NamespacedName{Name: nodeHTTPRoute.Name, Namespace: nodeHTTPRoute.Namespace}, foundNodeHTTPRoute) + if err != nil && errors.IsNotFound(err) { + nodeHTTPRouteLogger.Info("Creating node HTTPRoute") + if err = controllerutil.SetControllerReference(instance, nodeHTTPRoute, r.Scheme); err == nil { + err = r.Create(ctx, nodeHTTPRoute) + } + } else if err == nil { + var needsUpdate bool + needsUpdate, err = util.OvertakeControllerRef(instance, foundNodeHTTPRoute, r.Scheme) + needsUpdate = util.CopyHTTPRouteFields(nodeHTTPRoute, foundNodeHTTPRoute, nodeHTTPRouteLogger) || needsUpdate + + if needsUpdate && err == nil { + nodeHTTPRouteLogger.Info("Updating node HTTPRoute") + err = r.Update(ctx, foundNodeHTTPRoute) + } + } + if err != nil { + return requeueOrNot, err + } + } + } + + // Cleanup orphaned node HTTPRoutes (when scaling down) + httpRouteList := &gatewayv1.HTTPRouteList{} + labelSelector := labels.SelectorFromSet(instance.SharedLabels()) + listOps := &client.ListOptions{ + Namespace: instance.Namespace, + LabelSelector: labelSelector, + } + err = r.List(ctx, httpRouteList, listOps) + if err == nil { + for _, httpRoute := range httpRouteList.Items { + // Skip the common HTTPRoute + if httpRoute.Name == instance.CommonHTTPRouteName() { + continue + } + // Check if this node still exists + nodeExists := false + for _, nodeName := range solrNodeNames { + if httpRoute.Name == instance.NodeHTTPRouteName(nodeName) { + nodeExists = true + break + } + } + // Delete orphaned HTTPRoute + if !nodeExists { + logger.Info("Deleting orphaned node HTTPRoute", "httproute", httpRoute.Name) + err = r.Delete(ctx, &httpRoute) + if err != nil && !errors.IsNotFound(err) { + return requeueOrNot, err + } + } + } + } + + // Reconcile BackendTLSPolicy resources if configured + if extAddressabilityOpts.HasBackendTLSPolicy() { + // Reconcile Common BackendTLSPolicy (if not hidden) + if !extAddressabilityOpts.HideCommon { + commonPolicy := util.GenerateCommonBackendTLSPolicy(instance) + if commonPolicy != nil { + commonPolicyLogger := logger.WithValues("backendtlspolicy", commonPolicy.Name) + foundCommonPolicy := &gatewayv1.BackendTLSPolicy{} + err = r.Get(ctx, types.NamespacedName{Name: commonPolicy.Name, Namespace: commonPolicy.Namespace}, foundCommonPolicy) + + if err != nil && errors.IsNotFound(err) { + commonPolicyLogger.Info("Creating BackendTLSPolicy") + if err = controllerutil.SetControllerReference(instance, commonPolicy, r.Scheme); err == nil { + err = r.Create(ctx, commonPolicy) + } + } else if err == nil { + var needsUpdate bool + needsUpdate, err = util.OvertakeControllerRef(instance, foundCommonPolicy, r.Scheme) + if err == nil && util.CopyBackendTLSPolicyFields(commonPolicy, foundCommonPolicy, commonPolicyLogger) { + needsUpdate = true + } + if needsUpdate { + commonPolicyLogger.Info("Updating BackendTLSPolicy") + err = r.Update(ctx, foundCommonPolicy) + } + } + if err != nil { + return requeueOrNot, err + } + } + } + + // Reconcile Node BackendTLSPolicies (if not hidden) + if !extAddressabilityOpts.HideNodes { + for _, nodeName := range solrNodeNames { + nodePolicy := util.GenerateNodeBackendTLSPolicy(instance, nodeName) + if nodePolicy != nil { + nodePolicyLogger := logger.WithValues("backendtlspolicy", nodePolicy.Name) + foundNodePolicy := &gatewayv1.BackendTLSPolicy{} + err = r.Get(ctx, types.NamespacedName{Name: nodePolicy.Name, Namespace: nodePolicy.Namespace}, foundNodePolicy) + + if err != nil && errors.IsNotFound(err) { + nodePolicyLogger.Info("Creating BackendTLSPolicy") + if err = controllerutil.SetControllerReference(instance, nodePolicy, r.Scheme); err == nil { + err = r.Create(ctx, nodePolicy) + } + } else if err == nil { + var needsUpdate bool + needsUpdate, err = util.OvertakeControllerRef(instance, foundNodePolicy, r.Scheme) + if err == nil && util.CopyBackendTLSPolicyFields(nodePolicy, foundNodePolicy, nodePolicyLogger) { + needsUpdate = true + } + if needsUpdate { + nodePolicyLogger.Info("Updating BackendTLSPolicy") + err = r.Update(ctx, foundNodePolicy) + } + } + + if err != nil { + return requeueOrNot, err + } + } + } + } + + // Cleanup orphaned node BackendTLSPolicies (when scaling down) + backendTLSPolicyList := &gatewayv1.BackendTLSPolicyList{} + err = r.List(ctx, backendTLSPolicyList, listOps) + if err == nil { + for _, policy := range backendTLSPolicyList.Items { + // Skip the common BackendTLSPolicy + if policy.Name == instance.CommonBackendTLSPolicyName() { + continue + } + // Check if this node still exists + nodeExists := false + for _, nodeName := range solrNodeNames { + if policy.Name == instance.NodeBackendTLSPolicyName(nodeName) { + nodeExists = true + break + } + } + if !nodeExists { + logger.Info("Deleting orphaned BackendTLSPolicy", "backendtlspolicy", policy.Name) + err = r.Delete(ctx, &policy) + if err != nil && !errors.IsNotFound(err) { + return requeueOrNot, err + } + } + } + } + } + } else { + // If not using Gateway method, clean up any existing HTTPRoutes + httpRouteList := &gatewayv1.HTTPRouteList{} + labelSelector := labels.SelectorFromSet(instance.SharedLabels()) + listOps := &client.ListOptions{ + Namespace: instance.Namespace, + LabelSelector: labelSelector, + } + err := r.List(ctx, httpRouteList, listOps) + if err == nil && len(httpRouteList.Items) > 0 { + logger.Info("Cleaning up HTTPRoutes (method changed from Gateway)") + for _, httpRoute := range httpRouteList.Items { + err = r.Delete(ctx, &httpRoute) + if err != nil && !errors.IsNotFound(err) { + return requeueOrNot, err + } + } + } Review Comment: Missing cleanup logic for BackendTLSPolicy resources when not using Gateway method or when BackendTLSPolicy is disabled. The code cleans up HTTPRoutes when not using Gateway method (lines 642-659), but there's no corresponding cleanup for BackendTLSPolicy resources that may exist from a previous configuration. Consider adding cleanup logic similar to the HTTPRoute cleanup to delete any existing BackendTLSPolicy resources when either: 1. The method is changed from Gateway to another method 2. BackendTLSPolicy configuration is removed while still using Gateway method This could lead to orphaned BackendTLSPolicy resources that are not automatically cleaned up. ```suggestion } // Also clean up any existing BackendTLSPolicies that may have been created when using Gateway backendTLSPolicyList := &gatewayv1.BackendTLSPolicyList{} err = r.List(ctx, backendTLSPolicyList, listOps) if err == nil && len(backendTLSPolicyList.Items) > 0 { logger.Info("Cleaning up BackendTLSPolicies (method changed from Gateway)") for _, backendTLSPolicy := range backendTLSPolicyList.Items { err = r.Delete(ctx, &backendTLSPolicy) if err != nil && !errors.IsNotFound(err) { return requeueOrNot, err } } } ``` ########## tests/e2e/solrcloud_gateway_test.go: ########## @@ -0,0 +1,281 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one or more + * contributor license agreements. See the NOTICE file distributed with + * this work for additional information regarding copyright ownership. + * The ASF licenses this file to You 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 e2e + +import ( + "context" + solrv1beta1 "github.com/apache/solr-operator/api/v1beta1" + "github.com/apache/solr-operator/controllers/util" + . "github.com/onsi/ginkgo/v2" + . "github.com/onsi/gomega" +) + +var _ = FDescribe("E2E - SolrCloud - Gateway API", func() { Review Comment: The test suite uses `FDescribe` and `FIt` (focused test markers) which should not be committed to the main branch. These markers cause only the focused tests to run, skipping all other tests in the test suite. This can lead to other tests being accidentally ignored in CI/CD pipelines. Please change `FDescribe` to `Describe` and all `FIt` to `It` before merging. ```suggestion var _ = Describe("E2E - SolrCloud - Gateway API", func() { ``` ########## tests/e2e/solrcloud_gateway_test.go: ########## @@ -0,0 +1,281 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one or more + * contributor license agreements. See the NOTICE file distributed with + * this work for additional information regarding copyright ownership. + * The ASF licenses this file to You 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 e2e + +import ( + "context" + solrv1beta1 "github.com/apache/solr-operator/api/v1beta1" + "github.com/apache/solr-operator/controllers/util" + . "github.com/onsi/ginkgo/v2" + . "github.com/onsi/gomega" +) + +var _ = FDescribe("E2E - SolrCloud - Gateway API", func() { + var ( + solrCloud *solrv1beta1.SolrCloud + gatewayNamespace = "default" + gatewayName = "test-gateway" + ) + + BeforeEach(func() { + solrCloud = generateBaseSolrCloud(1) + solrCloud.Spec.SolrAddressability = solrv1beta1.SolrAddressabilityOptions{ + External: &solrv1beta1.ExternalAddressability{ + Method: solrv1beta1.Gateway, + UseExternalAddress: true, + DomainName: testDomain, + Gateway: &solrv1beta1.SolrGatewayOptions{ + ParentRefs: []solrv1beta1.GatewayParentReference{ + { + Name: gatewayName, + Namespace: &gatewayNamespace, + }, + }, + }, + }, + } + }) + + JustBeforeEach(func(ctx context.Context) { + By("creating the SolrCloud") + Expect(k8sClient.Create(ctx, solrCloud)).To(Succeed()) + + DeferCleanup(func(ctx context.Context) { + cleanupTest(ctx, solrCloud) + }) + + By("Waiting for the SolrCloud to come up healthy") + solrCloud = expectSolrCloudToBeReady(ctx, solrCloud) + + By("creating a first Solr Collection") + createAndQueryCollection(ctx, solrCloud, "basic", 1, 1) + }) + + FContext("Can Remove HTTPRoutes and Services when changing addressability", func() { + + FIt("Can adapt to changing needs", func(ctx context.Context) { + By("testing the Solr StatefulSet") + statefulSet := expectStatefulSet(ctx, solrCloud, solrCloud.StatefulSetName()) + // Pod Annotations test + Expect(statefulSet.Spec.Template.Annotations).To(HaveKeyWithValue(util.ServiceTypeAnnotation, util.PerNodeServiceType), "Since external address is used for advertising, the perNode service should be specified in the pod annotations.") + + By("testing the Solr Common Service") + expectService(ctx, solrCloud, solrCloud.CommonServiceName(), statefulSet.Spec.Selector.MatchLabels, false) + + By("ensuring the Solr Headless Service does not exist") + expectNoService(ctx, solrCloud, solrCloud.HeadlessServiceName(), "Headless service shouldn't exist, but it does.") + + By("making sure the individual Solr Node Services exist and route correctly") + nodeNames := solrCloud.GetAllSolrPodNames() + Expect(nodeNames).To(HaveLen(1), "SolrCloud has incorrect number of nodeNames.") + for _, nodeName := range nodeNames { + expectService(ctx, solrCloud, nodeName, util.MergeLabelsOrAnnotations(statefulSet.Spec.Selector.MatchLabels, map[string]string{"statefulset.kubernetes.io/pod-name": nodeName}), false) + } + + By("making sure Common HTTPRoute was created correctly") + expectHTTPRoute(ctx, solrCloud, solrCloud.CommonHTTPRouteName()) + + By("making sure Node HTTPRoutes were created correctly") + for _, nodeName := range nodeNames { + expectHTTPRoute(ctx, solrCloud, solrCloud.NodeHTTPRouteName(nodeName)) + } + + By("Turning off node external addressability and making sure the node services are deleted") + expectSolrCloudWithChecks(ctx, solrCloud, func(g Gomega, found *solrv1beta1.SolrCloud) { + found.Spec.SolrAddressability.External.HideNodes = true + found.Spec.SolrAddressability.External.UseExternalAddress = false + g.Expect(k8sClient.Update(ctx, found)).To(Succeed(), "Couldn't update the solrCloud to not advertise the nodes externally.") + }) + + // Since node external addressability is off, but common external addressability is on, the common HTTPRoute should exist, but the node HTTPRoutes should not + expectHTTPRoute(ctx, solrCloud, solrCloud.CommonHTTPRouteName()) + + expectService(ctx, solrCloud, solrCloud.HeadlessServiceName(), statefulSet.Spec.Selector.MatchLabels, true) + + for _, nodeName := range nodeNames { + eventuallyExpectNoHTTPRoute(ctx, solrCloud, solrCloud.NodeHTTPRouteName(nodeName)) + } + pod := expectPodNow(ctx, solrCloud, solrCloud.GetSolrPodName(0)) + Expect(pod.Annotations).To(HaveKeyWithValue(util.ServiceTypeAnnotation, util.HeadlessServiceType)) + + By("Turning off common external addressability and making sure the HTTPRoutes are deleted") + expectSolrCloudWithChecks(ctx, solrCloud, func(g Gomega, found *solrv1beta1.SolrCloud) { + found.Spec.SolrAddressability.External = nil + g.Expect(k8sClient.Update(ctx, found)).To(Succeed(), "Couldn't update the solrCloud to remove external addressability") + }) + eventuallyExpectNoHTTPRoute(ctx, solrCloud, solrCloud.CommonHTTPRouteName()) + + By("Turning back on common external addressability and making sure the headless service is deleted") + expectSolrCloudWithChecks(ctx, solrCloud, func(g Gomega, found *solrv1beta1.SolrCloud) { + found.Spec.SolrAddressability = solrv1beta1.SolrAddressabilityOptions{ + External: &solrv1beta1.ExternalAddressability{ + Method: solrv1beta1.Gateway, + UseExternalAddress: true, + DomainName: testDomain, + Gateway: &solrv1beta1.SolrGatewayOptions{ + ParentRefs: []solrv1beta1.GatewayParentReference{ + { + Name: gatewayName, + Namespace: &gatewayNamespace, + }, + }, + }, + }, + } + g.Expect(k8sClient.Update(ctx, found)).To(Succeed(), "Couldn't update the solrCloud to add external addressability") + }) + expectHTTPRoute(ctx, solrCloud, solrCloud.CommonHTTPRouteName()) + + By("testing the Solr Common Service") + expectService(ctx, solrCloud, solrCloud.CommonServiceName(), statefulSet.Spec.Selector.MatchLabels, false) + + By("ensuring the Solr Headless Service does not exist") + expectNoService(ctx, solrCloud, solrCloud.HeadlessServiceName(), "Headless service shouldn't exist, but it does.") + }) + }) + + FContext("BackendTLSPolicy Management", func() { + var ( + caCertConfigMapName = "solr-ca-cert" + ) + + FIt("Creates and manages BackendTLSPolicy resources", func(ctx context.Context) { + By("verifying BackendTLSPolicy resources do not exist initially") + expectNoBackendTLSPolicy(ctx, solrCloud, solrCloud.CommonBackendTLSPolicyName()) + nodeNames := solrCloud.GetAllSolrPodNames() + for _, nodeName := range nodeNames { + expectNoBackendTLSPolicy(ctx, solrCloud, solrCloud.NodeBackendTLSPolicyName(nodeName)) + } + + By("enabling BackendTLSPolicy with CA certificate reference") + expectSolrCloudWithChecks(ctx, solrCloud, func(g Gomega, found *solrv1beta1.SolrCloud) { + found.Spec.SolrAddressability.External.Gateway.BackendTLSPolicy = &solrv1beta1.SolrBackendTLSPolicy{ + CACertificateRefs: []solrv1beta1.GatewayCertificateReference{ + { + Name: caCertConfigMapName, + }, + }, + } + g.Expect(k8sClient.Update(ctx, found)).To(Succeed(), "Couldn't update the solrCloud to add BackendTLSPolicy") + }) + + By("verifying Common BackendTLSPolicy was created correctly") + commonPolicy := expectBackendTLSPolicyWithChecks(ctx, solrCloud, solrCloud.CommonBackendTLSPolicyName(), func(g Gomega, policy *gatewayv1.BackendTLSPolicy) { + g.Expect(policy.Spec.TargetRefs).To(HaveLen(1), "BackendTLSPolicy should have one target ref") + g.Expect(string(policy.Spec.TargetRefs[0].Name)).To(Equal(solrCloud.CommonServiceName()), "BackendTLSPolicy should target common service") + g.Expect(string(policy.Spec.TargetRefs[0].Kind)).To(Equal("Service"), "BackendTLSPolicy should target Service kind") + g.Expect(string(policy.Spec.Validation.Hostname)).To(Equal(solrCloud.CommonServiceName()), "BackendTLSPolicy hostname should match service name") + g.Expect(policy.Spec.Validation.CACertificateRefs).To(HaveLen(1), "BackendTLSPolicy should have one CA cert ref") + g.Expect(string(policy.Spec.Validation.CACertificateRefs[0].Name)).To(Equal(caCertConfigMapName), "CA cert ref name should match") + g.Expect(string(policy.Spec.Validation.CACertificateRefs[0].Kind)).To(Equal("ConfigMap"), "CA cert ref kind should default to ConfigMap") + g.Expect(string(policy.Spec.Validation.CACertificateRefs[0].Group)).To(Equal(""), "CA cert ref group should be empty (core API)") + }) + Expect(commonPolicy).ToNot(BeNil()) + + By("verifying Node BackendTLSPolicy resources were created correctly") + for _, nodeName := range nodeNames { + nodePolicy := expectBackendTLSPolicyWithChecks(ctx, solrCloud, solrCloud.NodeBackendTLSPolicyName(nodeName), func(g Gomega, policy *gatewayv1.BackendTLSPolicy) { + g.Expect(policy.Spec.TargetRefs).To(HaveLen(1), "BackendTLSPolicy should have one target ref") + g.Expect(string(policy.Spec.TargetRefs[0].Name)).To(Equal(nodeName), "BackendTLSPolicy should target node service") + g.Expect(string(policy.Spec.Validation.Hostname)).To(Equal(nodeName), "BackendTLSPolicy hostname should match node service name") + g.Expect(policy.Spec.Validation.CACertificateRefs).To(HaveLen(1), "BackendTLSPolicy should have one CA cert ref") + }) + Expect(nodePolicy).ToNot(BeNil()) + } + + By("updating BackendTLSPolicy to use wellKnownCACertificates") + expectSolrCloudWithChecks(ctx, solrCloud, func(g Gomega, found *solrv1beta1.SolrCloud) { + wellKnown := "System" + found.Spec.SolrAddressability.External.Gateway.BackendTLSPolicy = &solrv1beta1.SolrBackendTLSPolicy{ + WellKnownCACertificates: &wellKnown, + } + g.Expect(k8sClient.Update(ctx, found)).To(Succeed(), "Couldn't update the solrCloud BackendTLSPolicy to use wellKnownCACertificates") + }) + + By("verifying BackendTLSPolicy was updated to use wellKnownCACertificates") + expectBackendTLSPolicyWithChecks(ctx, solrCloud, solrCloud.CommonBackendTLSPolicyName(), func(g Gomega, policy *gatewayv1.BackendTLSPolicy) { + g.Expect(policy.Spec.Validation.CACertificateRefs).To(BeNil(), "CACertificateRefs should be nil when using wellKnownCACertificates") + g.Expect(policy.Spec.Validation.WellKnownCACertificates).ToNot(BeNil(), "WellKnownCACertificates should be set") + g.Expect(string(*policy.Spec.Validation.WellKnownCACertificates)).To(Equal("System"), "WellKnownCACertificates should be 'System'") + }) + + By("disabling node external addressability and verifying node BackendTLSPolicy resources are deleted") + expectSolrCloudWithChecks(ctx, solrCloud, func(g Gomega, found *solrv1beta1.SolrCloud) { + found.Spec.SolrAddressability.External.HideNodes = true + g.Expect(k8sClient.Update(ctx, found)).To(Succeed(), "Couldn't update the solrCloud to hide nodes") + }) + + By("verifying node BackendTLSPolicy resources were deleted") + for _, nodeName := range nodeNames { + eventuallyExpectNoBackendTLSPolicy(ctx, solrCloud, solrCloud.NodeBackendTLSPolicyName(nodeName)) + } + + By("verifying common BackendTLSPolicy still exists") + expectBackendTLSPolicy(ctx, solrCloud, solrCloud.CommonBackendTLSPolicyName()) + + By("removing BackendTLSPolicy configuration") + expectSolrCloudWithChecks(ctx, solrCloud, func(g Gomega, found *solrv1beta1.SolrCloud) { + found.Spec.SolrAddressability.External.Gateway.BackendTLSPolicy = nil + g.Expect(k8sClient.Update(ctx, found)).To(Succeed(), "Couldn't update the solrCloud to remove BackendTLSPolicy") + }) + + By("verifying all BackendTLSPolicy resources were deleted") + eventuallyExpectNoBackendTLSPolicy(ctx, solrCloud, solrCloud.CommonBackendTLSPolicyName()) + }) + + FIt("Cleans up BackendTLSPolicy when changing from Gateway method", func(ctx context.Context) { Review Comment: The test uses `FIt` (focused test marker) which should not be committed. Please change all instances of `FIt` to `It` before merging. ########## tests/e2e/solrcloud_gateway_test.go: ########## @@ -0,0 +1,281 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one or more + * contributor license agreements. See the NOTICE file distributed with + * this work for additional information regarding copyright ownership. + * The ASF licenses this file to You 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 e2e + +import ( + "context" + solrv1beta1 "github.com/apache/solr-operator/api/v1beta1" + "github.com/apache/solr-operator/controllers/util" + . "github.com/onsi/ginkgo/v2" + . "github.com/onsi/gomega" +) + +var _ = FDescribe("E2E - SolrCloud - Gateway API", func() { + var ( + solrCloud *solrv1beta1.SolrCloud + gatewayNamespace = "default" + gatewayName = "test-gateway" + ) + + BeforeEach(func() { + solrCloud = generateBaseSolrCloud(1) + solrCloud.Spec.SolrAddressability = solrv1beta1.SolrAddressabilityOptions{ + External: &solrv1beta1.ExternalAddressability{ + Method: solrv1beta1.Gateway, + UseExternalAddress: true, + DomainName: testDomain, + Gateway: &solrv1beta1.SolrGatewayOptions{ + ParentRefs: []solrv1beta1.GatewayParentReference{ + { + Name: gatewayName, + Namespace: &gatewayNamespace, + }, + }, + }, + }, + } + }) + + JustBeforeEach(func(ctx context.Context) { + By("creating the SolrCloud") + Expect(k8sClient.Create(ctx, solrCloud)).To(Succeed()) + + DeferCleanup(func(ctx context.Context) { + cleanupTest(ctx, solrCloud) + }) + + By("Waiting for the SolrCloud to come up healthy") + solrCloud = expectSolrCloudToBeReady(ctx, solrCloud) + + By("creating a first Solr Collection") + createAndQueryCollection(ctx, solrCloud, "basic", 1, 1) + }) + + FContext("Can Remove HTTPRoutes and Services when changing addressability", func() { Review Comment: The test context uses `FContext` (focused context marker) which should not be committed. This causes only the focused contexts to run, skipping all other test contexts. Please change `FContext` to `Context` before merging. ########## tests/e2e/solrcloud_gateway_test.go: ########## @@ -0,0 +1,281 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one or more + * contributor license agreements. See the NOTICE file distributed with + * this work for additional information regarding copyright ownership. + * The ASF licenses this file to You 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 e2e + +import ( + "context" + solrv1beta1 "github.com/apache/solr-operator/api/v1beta1" + "github.com/apache/solr-operator/controllers/util" + . "github.com/onsi/ginkgo/v2" + . "github.com/onsi/gomega" +) + +var _ = FDescribe("E2E - SolrCloud - Gateway API", func() { + var ( + solrCloud *solrv1beta1.SolrCloud + gatewayNamespace = "default" + gatewayName = "test-gateway" + ) + + BeforeEach(func() { + solrCloud = generateBaseSolrCloud(1) + solrCloud.Spec.SolrAddressability = solrv1beta1.SolrAddressabilityOptions{ + External: &solrv1beta1.ExternalAddressability{ + Method: solrv1beta1.Gateway, + UseExternalAddress: true, + DomainName: testDomain, + Gateway: &solrv1beta1.SolrGatewayOptions{ + ParentRefs: []solrv1beta1.GatewayParentReference{ + { + Name: gatewayName, + Namespace: &gatewayNamespace, + }, + }, + }, + }, + } + }) + + JustBeforeEach(func(ctx context.Context) { + By("creating the SolrCloud") + Expect(k8sClient.Create(ctx, solrCloud)).To(Succeed()) + + DeferCleanup(func(ctx context.Context) { + cleanupTest(ctx, solrCloud) + }) + + By("Waiting for the SolrCloud to come up healthy") + solrCloud = expectSolrCloudToBeReady(ctx, solrCloud) + + By("creating a first Solr Collection") + createAndQueryCollection(ctx, solrCloud, "basic", 1, 1) + }) + + FContext("Can Remove HTTPRoutes and Services when changing addressability", func() { + + FIt("Can adapt to changing needs", func(ctx context.Context) { + By("testing the Solr StatefulSet") + statefulSet := expectStatefulSet(ctx, solrCloud, solrCloud.StatefulSetName()) + // Pod Annotations test + Expect(statefulSet.Spec.Template.Annotations).To(HaveKeyWithValue(util.ServiceTypeAnnotation, util.PerNodeServiceType), "Since external address is used for advertising, the perNode service should be specified in the pod annotations.") + + By("testing the Solr Common Service") + expectService(ctx, solrCloud, solrCloud.CommonServiceName(), statefulSet.Spec.Selector.MatchLabels, false) + + By("ensuring the Solr Headless Service does not exist") + expectNoService(ctx, solrCloud, solrCloud.HeadlessServiceName(), "Headless service shouldn't exist, but it does.") + + By("making sure the individual Solr Node Services exist and route correctly") + nodeNames := solrCloud.GetAllSolrPodNames() + Expect(nodeNames).To(HaveLen(1), "SolrCloud has incorrect number of nodeNames.") + for _, nodeName := range nodeNames { + expectService(ctx, solrCloud, nodeName, util.MergeLabelsOrAnnotations(statefulSet.Spec.Selector.MatchLabels, map[string]string{"statefulset.kubernetes.io/pod-name": nodeName}), false) + } + + By("making sure Common HTTPRoute was created correctly") + expectHTTPRoute(ctx, solrCloud, solrCloud.CommonHTTPRouteName()) + + By("making sure Node HTTPRoutes were created correctly") + for _, nodeName := range nodeNames { + expectHTTPRoute(ctx, solrCloud, solrCloud.NodeHTTPRouteName(nodeName)) + } + + By("Turning off node external addressability and making sure the node services are deleted") + expectSolrCloudWithChecks(ctx, solrCloud, func(g Gomega, found *solrv1beta1.SolrCloud) { + found.Spec.SolrAddressability.External.HideNodes = true + found.Spec.SolrAddressability.External.UseExternalAddress = false + g.Expect(k8sClient.Update(ctx, found)).To(Succeed(), "Couldn't update the solrCloud to not advertise the nodes externally.") + }) + + // Since node external addressability is off, but common external addressability is on, the common HTTPRoute should exist, but the node HTTPRoutes should not + expectHTTPRoute(ctx, solrCloud, solrCloud.CommonHTTPRouteName()) + + expectService(ctx, solrCloud, solrCloud.HeadlessServiceName(), statefulSet.Spec.Selector.MatchLabels, true) + + for _, nodeName := range nodeNames { + eventuallyExpectNoHTTPRoute(ctx, solrCloud, solrCloud.NodeHTTPRouteName(nodeName)) + } + pod := expectPodNow(ctx, solrCloud, solrCloud.GetSolrPodName(0)) + Expect(pod.Annotations).To(HaveKeyWithValue(util.ServiceTypeAnnotation, util.HeadlessServiceType)) + + By("Turning off common external addressability and making sure the HTTPRoutes are deleted") + expectSolrCloudWithChecks(ctx, solrCloud, func(g Gomega, found *solrv1beta1.SolrCloud) { + found.Spec.SolrAddressability.External = nil + g.Expect(k8sClient.Update(ctx, found)).To(Succeed(), "Couldn't update the solrCloud to remove external addressability") + }) + eventuallyExpectNoHTTPRoute(ctx, solrCloud, solrCloud.CommonHTTPRouteName()) + + By("Turning back on common external addressability and making sure the headless service is deleted") + expectSolrCloudWithChecks(ctx, solrCloud, func(g Gomega, found *solrv1beta1.SolrCloud) { + found.Spec.SolrAddressability = solrv1beta1.SolrAddressabilityOptions{ + External: &solrv1beta1.ExternalAddressability{ + Method: solrv1beta1.Gateway, + UseExternalAddress: true, + DomainName: testDomain, + Gateway: &solrv1beta1.SolrGatewayOptions{ + ParentRefs: []solrv1beta1.GatewayParentReference{ + { + Name: gatewayName, + Namespace: &gatewayNamespace, + }, + }, + }, + }, + } + g.Expect(k8sClient.Update(ctx, found)).To(Succeed(), "Couldn't update the solrCloud to add external addressability") + }) + expectHTTPRoute(ctx, solrCloud, solrCloud.CommonHTTPRouteName()) + + By("testing the Solr Common Service") + expectService(ctx, solrCloud, solrCloud.CommonServiceName(), statefulSet.Spec.Selector.MatchLabels, false) + + By("ensuring the Solr Headless Service does not exist") + expectNoService(ctx, solrCloud, solrCloud.HeadlessServiceName(), "Headless service shouldn't exist, but it does.") + }) + }) + + FContext("BackendTLSPolicy Management", func() { + var ( + caCertConfigMapName = "solr-ca-cert" + ) + + FIt("Creates and manages BackendTLSPolicy resources", func(ctx context.Context) { Review Comment: The test uses `FIt` (focused test marker) which should not be committed. Please change all instances of `FIt` to `It` before merging. ########## tests/e2e/solrcloud_gateway_test.go: ########## @@ -0,0 +1,281 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one or more + * contributor license agreements. See the NOTICE file distributed with + * this work for additional information regarding copyright ownership. + * The ASF licenses this file to You 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 e2e + +import ( + "context" + solrv1beta1 "github.com/apache/solr-operator/api/v1beta1" + "github.com/apache/solr-operator/controllers/util" + . "github.com/onsi/ginkgo/v2" + . "github.com/onsi/gomega" +) + +var _ = FDescribe("E2E - SolrCloud - Gateway API", func() { + var ( + solrCloud *solrv1beta1.SolrCloud + gatewayNamespace = "default" + gatewayName = "test-gateway" + ) + + BeforeEach(func() { + solrCloud = generateBaseSolrCloud(1) + solrCloud.Spec.SolrAddressability = solrv1beta1.SolrAddressabilityOptions{ + External: &solrv1beta1.ExternalAddressability{ + Method: solrv1beta1.Gateway, + UseExternalAddress: true, + DomainName: testDomain, + Gateway: &solrv1beta1.SolrGatewayOptions{ + ParentRefs: []solrv1beta1.GatewayParentReference{ + { + Name: gatewayName, + Namespace: &gatewayNamespace, + }, + }, + }, + }, + } + }) + + JustBeforeEach(func(ctx context.Context) { + By("creating the SolrCloud") + Expect(k8sClient.Create(ctx, solrCloud)).To(Succeed()) + + DeferCleanup(func(ctx context.Context) { + cleanupTest(ctx, solrCloud) + }) + + By("Waiting for the SolrCloud to come up healthy") + solrCloud = expectSolrCloudToBeReady(ctx, solrCloud) + + By("creating a first Solr Collection") + createAndQueryCollection(ctx, solrCloud, "basic", 1, 1) + }) + + FContext("Can Remove HTTPRoutes and Services when changing addressability", func() { + + FIt("Can adapt to changing needs", func(ctx context.Context) { + By("testing the Solr StatefulSet") + statefulSet := expectStatefulSet(ctx, solrCloud, solrCloud.StatefulSetName()) + // Pod Annotations test + Expect(statefulSet.Spec.Template.Annotations).To(HaveKeyWithValue(util.ServiceTypeAnnotation, util.PerNodeServiceType), "Since external address is used for advertising, the perNode service should be specified in the pod annotations.") + + By("testing the Solr Common Service") + expectService(ctx, solrCloud, solrCloud.CommonServiceName(), statefulSet.Spec.Selector.MatchLabels, false) + + By("ensuring the Solr Headless Service does not exist") + expectNoService(ctx, solrCloud, solrCloud.HeadlessServiceName(), "Headless service shouldn't exist, but it does.") + + By("making sure the individual Solr Node Services exist and route correctly") + nodeNames := solrCloud.GetAllSolrPodNames() + Expect(nodeNames).To(HaveLen(1), "SolrCloud has incorrect number of nodeNames.") + for _, nodeName := range nodeNames { + expectService(ctx, solrCloud, nodeName, util.MergeLabelsOrAnnotations(statefulSet.Spec.Selector.MatchLabels, map[string]string{"statefulset.kubernetes.io/pod-name": nodeName}), false) + } + + By("making sure Common HTTPRoute was created correctly") + expectHTTPRoute(ctx, solrCloud, solrCloud.CommonHTTPRouteName()) + + By("making sure Node HTTPRoutes were created correctly") + for _, nodeName := range nodeNames { + expectHTTPRoute(ctx, solrCloud, solrCloud.NodeHTTPRouteName(nodeName)) + } + + By("Turning off node external addressability and making sure the node services are deleted") + expectSolrCloudWithChecks(ctx, solrCloud, func(g Gomega, found *solrv1beta1.SolrCloud) { + found.Spec.SolrAddressability.External.HideNodes = true + found.Spec.SolrAddressability.External.UseExternalAddress = false + g.Expect(k8sClient.Update(ctx, found)).To(Succeed(), "Couldn't update the solrCloud to not advertise the nodes externally.") + }) + + // Since node external addressability is off, but common external addressability is on, the common HTTPRoute should exist, but the node HTTPRoutes should not + expectHTTPRoute(ctx, solrCloud, solrCloud.CommonHTTPRouteName()) + + expectService(ctx, solrCloud, solrCloud.HeadlessServiceName(), statefulSet.Spec.Selector.MatchLabels, true) + + for _, nodeName := range nodeNames { + eventuallyExpectNoHTTPRoute(ctx, solrCloud, solrCloud.NodeHTTPRouteName(nodeName)) + } + pod := expectPodNow(ctx, solrCloud, solrCloud.GetSolrPodName(0)) + Expect(pod.Annotations).To(HaveKeyWithValue(util.ServiceTypeAnnotation, util.HeadlessServiceType)) + + By("Turning off common external addressability and making sure the HTTPRoutes are deleted") + expectSolrCloudWithChecks(ctx, solrCloud, func(g Gomega, found *solrv1beta1.SolrCloud) { + found.Spec.SolrAddressability.External = nil + g.Expect(k8sClient.Update(ctx, found)).To(Succeed(), "Couldn't update the solrCloud to remove external addressability") + }) + eventuallyExpectNoHTTPRoute(ctx, solrCloud, solrCloud.CommonHTTPRouteName()) + + By("Turning back on common external addressability and making sure the headless service is deleted") + expectSolrCloudWithChecks(ctx, solrCloud, func(g Gomega, found *solrv1beta1.SolrCloud) { + found.Spec.SolrAddressability = solrv1beta1.SolrAddressabilityOptions{ + External: &solrv1beta1.ExternalAddressability{ + Method: solrv1beta1.Gateway, + UseExternalAddress: true, + DomainName: testDomain, + Gateway: &solrv1beta1.SolrGatewayOptions{ + ParentRefs: []solrv1beta1.GatewayParentReference{ + { + Name: gatewayName, + Namespace: &gatewayNamespace, + }, + }, + }, + }, + } + g.Expect(k8sClient.Update(ctx, found)).To(Succeed(), "Couldn't update the solrCloud to add external addressability") + }) + expectHTTPRoute(ctx, solrCloud, solrCloud.CommonHTTPRouteName()) + + By("testing the Solr Common Service") + expectService(ctx, solrCloud, solrCloud.CommonServiceName(), statefulSet.Spec.Selector.MatchLabels, false) + + By("ensuring the Solr Headless Service does not exist") + expectNoService(ctx, solrCloud, solrCloud.HeadlessServiceName(), "Headless service shouldn't exist, but it does.") + }) + }) + + FContext("BackendTLSPolicy Management", func() { Review Comment: The test context uses `FContext` (focused context marker) which should not be committed. Please change `FContext` to `Context` before merging. ########## controllers/util/gateway_util_backendtls.go: ########## @@ -0,0 +1,192 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one or more + * contributor license agreements. See the NOTICE file distributed with + * this work for additional information regarding copyright ownership. + * The ASF licenses this file to You 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 util + +import ( + solr "github.com/apache/solr-operator/api/v1beta1" + "github.com/go-logr/logr" + metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" + gatewayv1 "sigs.k8s.io/gateway-api/apis/v1" +) + +// GenerateCommonBackendTLSPolicy creates a BackendTLSPolicy for the common Solr service +func GenerateCommonBackendTLSPolicy(solrCloud *solr.SolrCloud) *gatewayv1.BackendTLSPolicy { + if solrCloud.Spec.SolrAddressability.External.Gateway == nil || + solrCloud.Spec.SolrAddressability.External.Gateway.BackendTLSPolicy == nil { + return nil + } + + // Get the full FQDN for hostname validation + domainName := solrCloud.Spec.SolrAddressability.External.DomainName + hostname := solrCloud.ExternalCommonUrl(domainName, false) Review Comment: The hostname validation uses the full URL returned by `ExternalCommonUrl` and `ExternalNodeUrl`, but these functions include the hostname without the protocol scheme. However, the comment on line 34 says "Get the full FQDN for hostname validation", which is accurate. The `PreciseHostname` type expects just the hostname, which is what's being provided. No issue here, but the variable naming could be clearer (e.g., `fqdn` instead of `hostname` to distinguish from the field name). ########## api/v1beta1/solrcloud_types.go: ########## @@ -626,6 +638,90 @@ type SolrIngressTLSTermination struct { TLSSecret string `json:"tlsSecret,omitempty"` } +// SolrGatewayOptions defines how a SolrCloud should be exposed via Kubernetes Gateway API +type SolrGatewayOptions struct { + // ParentRefs specifies the Gateway(s) to attach HTTPRoutes to. + // This is required when using method=Gateway. + // + // The referenced Gateway must already exist and be managed by your platform team. + // The Solr Operator only manages the HTTPRoute resources. + // + // +kubebuilder:validation:MinItems=1 + ParentRefs []GatewayParentReference `json:"parentRefs"` + + // Annotations to add to HTTPRoute resources + // +optional + Annotations map[string]string `json:"annotations,omitempty"` + + // Labels to add to HTTPRoute resources + // +optional + Labels map[string]string `json:"labels,omitempty"` + + // BackendTLSPolicy defines TLS configuration for backend connections from Gateway to Solr pods. + // + // This is used when Solr pods are running with TLS enabled (spec.solrTLS) and the Gateway + // needs to establish secure connections to the backend services. + // + // The Solr Operator will create BackendTLSPolicy resources for each HTTPRoute. + // + // +optional + BackendTLSPolicy *SolrBackendTLSPolicy `json:"backendTLSPolicy,omitempty"` +} + +// GatewayParentReference identifies a parent Gateway resource to attach HTTPRoutes to +type GatewayParentReference struct { + // Name of the Gateway resource + Name string `json:"name"` + + // Namespace of the Gateway resource. + // If not specified, defaults to the HTTPRoute's namespace. + // +optional + Namespace *string `json:"namespace,omitempty"` + + // SectionName refers to a specific listener on the Gateway. + // For example, "https" or "http". + // +optional + SectionName *string `json:"sectionName,omitempty"` +} + +// SolrBackendTLSPolicy defines backend TLS configuration for Gateway API +// +kubebuilder:validation:MaxProperties=1 +type SolrBackendTLSPolicy struct { + // CACertificateRefs contains one or more references to Kubernetes objects that contain + // TLS certificates of the Certificate Authorities that can be used as a trust anchor + // to validate the certificates presented by the backend. + // + // +optional + // +kubebuilder:validation:MaxItems=8 + CACertificateRefs []GatewayCertificateReference `json:"caCertificateRefs,omitempty"` + + // WellKnownCACertificates specifies whether system CA certificates may be used in the + // TLS handshake between the gateway and backend pod. + // + // If WellKnownCACertificates is unspecified or empty (""), then CACertificateRefs must be + // specified with at least one entry for a valid configuration. + // + // Only one of CACertificateRefs or WellKnownCACertificates may be specified, not both. + // + // +optional + WellKnownCACertificates *string `json:"wellKnownCACertificates,omitempty"` +} Review Comment: The `SolrBackendTLSPolicy` type has a validation constraint `+kubebuilder:validation:MaxProperties=1` which ensures only one of `CACertificateRefs` or `WellKnownCACertificates` can be set. However, both fields are optional, so it's valid to have zero properties set (neither field configured). This means the BackendTLSPolicy generation functions could create a BackendTLSPolicy with neither `CACertificateRefs` nor `WellKnownCACertificates` set, which would be invalid according to Gateway API specifications. The `HasBackendTLSPolicy()` function (line 1590) does check that at least one is configured before returning true, which prevents creation of invalid policies. However, the validation comment on line 688 could be clearer about this constraint. Consider adding a comment explaining that at least one field must be set for a valid BackendTLSPolicy configuration. ########## controllers/solrcloud_controller.go: ########## @@ -436,6 +441,223 @@ func (r *SolrCloudReconciler) Reconcile(ctx context.Context, req ctrl.Request) ( } } + // Reconcile Gateway API HTTPRoutes + if extAddressabilityOpts != nil && extAddressabilityOpts.Method == solrv1beta1.Gateway { + // Validate that gateway config is provided + if extAddressabilityOpts.Gateway == nil || len(extAddressabilityOpts.Gateway.ParentRefs) == 0 { + return requeueOrNot, fmt.Errorf("gateway.parentRefs is required when using method=Gateway") + } + + // Reconcile Common HTTPRoute (if not hidden) + if !extAddressabilityOpts.HideCommon { + commonHTTPRoute := util.GenerateCommonHTTPRoute(instance, solrNodeNames) + commonHTTPRouteLogger := logger.WithValues("httproute", commonHTTPRoute.Name) + foundCommonHTTPRoute := &gatewayv1.HTTPRoute{} + err = r.Get(ctx, types.NamespacedName{Name: commonHTTPRoute.Name, Namespace: commonHTTPRoute.Namespace}, foundCommonHTTPRoute) + if err != nil && errors.IsNotFound(err) { + commonHTTPRouteLogger.Info("Creating common HTTPRoute") + if err = controllerutil.SetControllerReference(instance, commonHTTPRoute, r.Scheme); err == nil { + err = r.Create(ctx, commonHTTPRoute) + } + } else if err == nil { + var needsUpdate bool + needsUpdate, err = util.OvertakeControllerRef(instance, foundCommonHTTPRoute, r.Scheme) + needsUpdate = util.CopyHTTPRouteFields(commonHTTPRoute, foundCommonHTTPRoute, commonHTTPRouteLogger) || needsUpdate + + if needsUpdate && err == nil { + commonHTTPRouteLogger.Info("Updating common HTTPRoute") + err = r.Update(ctx, foundCommonHTTPRoute) + } + } + if err != nil { + return requeueOrNot, err + } + } else { + // Delete common HTTPRoute if it exists but should be hidden + foundCommonHTTPRoute := &gatewayv1.HTTPRoute{} + err := r.Get(ctx, types.NamespacedName{Name: instance.CommonHTTPRouteName(), Namespace: instance.GetNamespace()}, foundCommonHTTPRoute) + if err == nil { + logger.Info("Deleting common HTTPRoute (hideCommon=true)") + err = r.Delete(ctx, foundCommonHTTPRoute) + if err != nil && !errors.IsNotFound(err) { + return requeueOrNot, err + } + } + } + + // Reconcile Node HTTPRoutes (if not hidden) + if !extAddressabilityOpts.HideNodes { + for _, nodeName := range solrNodeNames { + nodeHTTPRoute := util.GenerateNodeHTTPRoute(instance, nodeName) + nodeHTTPRouteLogger := logger.WithValues("httproute", nodeHTTPRoute.Name) + foundNodeHTTPRoute := &gatewayv1.HTTPRoute{} + err = r.Get(ctx, types.NamespacedName{Name: nodeHTTPRoute.Name, Namespace: nodeHTTPRoute.Namespace}, foundNodeHTTPRoute) + if err != nil && errors.IsNotFound(err) { + nodeHTTPRouteLogger.Info("Creating node HTTPRoute") + if err = controllerutil.SetControllerReference(instance, nodeHTTPRoute, r.Scheme); err == nil { + err = r.Create(ctx, nodeHTTPRoute) + } + } else if err == nil { + var needsUpdate bool + needsUpdate, err = util.OvertakeControllerRef(instance, foundNodeHTTPRoute, r.Scheme) + needsUpdate = util.CopyHTTPRouteFields(nodeHTTPRoute, foundNodeHTTPRoute, nodeHTTPRouteLogger) || needsUpdate + + if needsUpdate && err == nil { + nodeHTTPRouteLogger.Info("Updating node HTTPRoute") + err = r.Update(ctx, foundNodeHTTPRoute) + } + } + if err != nil { + return requeueOrNot, err + } + } + } + + // Cleanup orphaned node HTTPRoutes (when scaling down) + httpRouteList := &gatewayv1.HTTPRouteList{} + labelSelector := labels.SelectorFromSet(instance.SharedLabels()) + listOps := &client.ListOptions{ + Namespace: instance.Namespace, + LabelSelector: labelSelector, + } + err = r.List(ctx, httpRouteList, listOps) + if err == nil { + for _, httpRoute := range httpRouteList.Items { + // Skip the common HTTPRoute + if httpRoute.Name == instance.CommonHTTPRouteName() { + continue + } + // Check if this node still exists + nodeExists := false + for _, nodeName := range solrNodeNames { + if httpRoute.Name == instance.NodeHTTPRouteName(nodeName) { + nodeExists = true + break + } + } + // Delete orphaned HTTPRoute + if !nodeExists { + logger.Info("Deleting orphaned node HTTPRoute", "httproute", httpRoute.Name) + err = r.Delete(ctx, &httpRoute) + if err != nil && !errors.IsNotFound(err) { + return requeueOrNot, err + } + } + } + } + + // Reconcile BackendTLSPolicy resources if configured + if extAddressabilityOpts.HasBackendTLSPolicy() { + // Reconcile Common BackendTLSPolicy (if not hidden) + if !extAddressabilityOpts.HideCommon { + commonPolicy := util.GenerateCommonBackendTLSPolicy(instance) + if commonPolicy != nil { + commonPolicyLogger := logger.WithValues("backendtlspolicy", commonPolicy.Name) + foundCommonPolicy := &gatewayv1.BackendTLSPolicy{} + err = r.Get(ctx, types.NamespacedName{Name: commonPolicy.Name, Namespace: commonPolicy.Namespace}, foundCommonPolicy) + + if err != nil && errors.IsNotFound(err) { + commonPolicyLogger.Info("Creating BackendTLSPolicy") + if err = controllerutil.SetControllerReference(instance, commonPolicy, r.Scheme); err == nil { + err = r.Create(ctx, commonPolicy) + } + } else if err == nil { + var needsUpdate bool + needsUpdate, err = util.OvertakeControllerRef(instance, foundCommonPolicy, r.Scheme) + if err == nil && util.CopyBackendTLSPolicyFields(commonPolicy, foundCommonPolicy, commonPolicyLogger) { + needsUpdate = true + } + if needsUpdate { + commonPolicyLogger.Info("Updating BackendTLSPolicy") + err = r.Update(ctx, foundCommonPolicy) + } + } + if err != nil { + return requeueOrNot, err + } + } + } + + // Reconcile Node BackendTLSPolicies (if not hidden) + if !extAddressabilityOpts.HideNodes { + for _, nodeName := range solrNodeNames { + nodePolicy := util.GenerateNodeBackendTLSPolicy(instance, nodeName) + if nodePolicy != nil { + nodePolicyLogger := logger.WithValues("backendtlspolicy", nodePolicy.Name) + foundNodePolicy := &gatewayv1.BackendTLSPolicy{} + err = r.Get(ctx, types.NamespacedName{Name: nodePolicy.Name, Namespace: nodePolicy.Namespace}, foundNodePolicy) + + if err != nil && errors.IsNotFound(err) { + nodePolicyLogger.Info("Creating BackendTLSPolicy") + if err = controllerutil.SetControllerReference(instance, nodePolicy, r.Scheme); err == nil { + err = r.Create(ctx, nodePolicy) + } + } else if err == nil { + var needsUpdate bool + needsUpdate, err = util.OvertakeControllerRef(instance, foundNodePolicy, r.Scheme) + if err == nil && util.CopyBackendTLSPolicyFields(nodePolicy, foundNodePolicy, nodePolicyLogger) { + needsUpdate = true + } + if needsUpdate { + nodePolicyLogger.Info("Updating BackendTLSPolicy") + err = r.Update(ctx, foundNodePolicy) + } + } + + if err != nil { + return requeueOrNot, err + } + } + } + } + + // Cleanup orphaned node BackendTLSPolicies (when scaling down) + backendTLSPolicyList := &gatewayv1.BackendTLSPolicyList{} + err = r.List(ctx, backendTLSPolicyList, listOps) + if err == nil { + for _, policy := range backendTLSPolicyList.Items { + // Skip the common BackendTLSPolicy + if policy.Name == instance.CommonBackendTLSPolicyName() { + continue + } + // Check if this node still exists + nodeExists := false + for _, nodeName := range solrNodeNames { + if policy.Name == instance.NodeBackendTLSPolicyName(nodeName) { + nodeExists = true + break + } + } + if !nodeExists { + logger.Info("Deleting orphaned BackendTLSPolicy", "backendtlspolicy", policy.Name) + err = r.Delete(ctx, &policy) + if err != nil && !errors.IsNotFound(err) { + return requeueOrNot, err + } + } + } + } + } Review Comment: Missing cleanup logic for BackendTLSPolicy resources when: 1. The configuration changes from `hideCommon: false` to `hideCommon: true` - the common BackendTLSPolicy will not be deleted 2. The configuration changes from `hideNodes: false` to `hideNodes: true` - the node BackendTLSPolicy resources will not be deleted 3. The BackendTLSPolicy configuration is removed entirely (but method remains Gateway) The current code only creates/updates BackendTLSPolicy resources when configured and not hidden (lines 550-640), and only deletes orphaned node policies during scale-down (lines 614-639). However, there's no explicit deletion when these resources should be removed due to configuration changes. Consider adding cleanup logic similar to HTTPRoutes (lines 476-486) that explicitly deletes the common BackendTLSPolicy when `hideCommon` is true, and deletes node BackendTLSPolicy resources when `hideNodes` is true. Also add cleanup when `HasBackendTLSPolicy()` returns false but resources still exist. -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected] --------------------------------------------------------------------- To unsubscribe, e-mail: [email protected] For additional commands, e-mail: [email protected]
