From 485c5fe901d0fcc2e8c0f62c2973f5add0e688ee Mon Sep 17 00:00:00 2001 From: Jiri Podivin Date: Thu, 11 Apr 2024 11:36:37 +0200 Subject: [PATCH] Introducing spec level validation of dataplane/controlplane TLS consistency Verifies that TLS settings for nodeset are consistent with those of existing control plane, if there is one and only one. If there are multiple control planes the process will result in error, if there is no control plane in the namespace, the deployment will proceed. Tests are included Signed-off-by: Jiri Podivin --- .../openstackdataplanenodeset_types.go | 43 ++++++++ ...openstackdataplanedeployment_controller.go | 11 ++ go.mod | 2 +- tests/functional/base_test.go | 33 ++++++ ...tackdataplanedeployment_controller_test.go | 104 ++++++++++++++++++ tests/functional/suite_test.go | 2 +- 6 files changed, 193 insertions(+), 2 deletions(-) diff --git a/api/v1beta1/openstackdataplanenodeset_types.go b/api/v1beta1/openstackdataplanenodeset_types.go index 83e3a9bb5..2e22e411d 100644 --- a/api/v1beta1/openstackdataplanenodeset_types.go +++ b/api/v1beta1/openstackdataplanenodeset_types.go @@ -17,9 +17,11 @@ limitations under the License. package v1beta1 import ( + "context" "fmt" "golang.org/x/exp/slices" + "sigs.k8s.io/controller-runtime/pkg/client" infranetworkv1 "github.com/openstack-k8s-operators/infra-operator/apis/network/v1beta1" condition "github.com/openstack-k8s-operators/lib-common/modules/common/condition" @@ -290,3 +292,44 @@ func (r *OpenStackDataPlaneNodeSetSpec) duplicateNodeCheck(nodeSetList *OpenStac return } + +// Compare TLS settings of control plane and data plane +// if control plane name is specified attempt to retrieve it +// otherwise get any control plane in the namespace +func (r *OpenStackDataPlaneNodeSetSpec) ValidateTLS(namespace string, reconcilerClient client.Client, ctx context.Context) error { + var err error + + controlPlanes := openstackv1.OpenStackControlPlaneList{} + opts := client.ListOptions{ + Namespace: namespace, + } + + _ = reconcilerClient.List(ctx, &controlPlanes, &opts) + // Verify TLS status of control plane only if there is a single one + // report error if there are multiple, or proceed if there are none + if len(controlPlanes.Items) > 1 { + err = fmt.Errorf("multiple control planes found in the namespace %v", namespace) + } else if len(controlPlanes.Items) == 1 { + controlPlane := controlPlanes.Items[0] + err = r.TLSMatch(controlPlane) + } + + return err +} + +// Do TLS flags match in control plane ingress, pods and data plane +func (r *OpenStackDataPlaneNodeSetSpec) TLSMatch(controlPlane openstackv1.OpenStackControlPlane) *field.Error { + + if controlPlane.Spec.TLS.Ingress.Enabled != r.TLSEnabled || controlPlane.Spec.TLS.PodLevel.Enabled != r.TLSEnabled { + + return field.Forbidden( + field.NewPath("spec.tlsEnabled"), + fmt.Sprintf( + "TLS settings on Data Plane node set and Control Plane %s do not match, Node set: %t Control Plane Ingress: %t Control Plane PodLevel: %t", + controlPlane.Name, + r.TLSEnabled, + controlPlane.Spec.TLS.Ingress.Enabled, + controlPlane.Spec.TLS.PodLevel.Enabled)) + } + return nil +} diff --git a/controllers/openstackdataplanedeployment_controller.go b/controllers/openstackdataplanedeployment_controller.go index d9267cec2..603ab1662 100644 --- a/controllers/openstackdataplanedeployment_controller.go +++ b/controllers/openstackdataplanedeployment_controller.go @@ -183,6 +183,17 @@ func (r *OpenStackDataPlaneDeploymentReconciler) Reconcile(ctx context.Context, // Error reading the object - requeue the request. return ctrl.Result{}, err } + err = nodeSetInstance.Spec.ValidateTLS(instance.GetNamespace(), r.Client, ctx) + if err != nil { + Log.Info("nodeset %s TLS settings in conflict with control plane: %w", nodeSet, err) + instance.Status.Conditions.MarkFalse( + dataplanev1.SetupReadyCondition, + condition.ErrorReason, + condition.SeverityError, + dataplanev1.DataPlaneNodeSetErrorMessage, + err.Error()) + return ctrl.Result{}, err + } nodeSets.Items = append(nodeSets.Items, *nodeSetInstance) } diff --git a/go.mod b/go.mod index 617ef47d5..52f469420 100644 --- a/go.mod +++ b/go.mod @@ -28,6 +28,7 @@ require ( k8s.io/api v0.28.10 k8s.io/apimachinery v0.28.10 k8s.io/client-go v0.28.10 + k8s.io/utils v0.0.0-20240502163921-fe8a2dddb1d0 sigs.k8s.io/controller-runtime v0.16.6 ) @@ -113,7 +114,6 @@ require ( k8s.io/component-base v0.28.10 // indirect k8s.io/klog/v2 v2.120.1 // indirect k8s.io/kube-openapi v0.0.0-20240228011516-70dd3763d340 // indirect - k8s.io/utils v0.0.0-20240502163921-fe8a2dddb1d0 // indirect sigs.k8s.io/gateway-api v0.8.0 // indirect sigs.k8s.io/json v0.0.0-20221116044647-bc3834ca7abd // indirect sigs.k8s.io/structured-merge-diff/v4 v4.4.1 // indirect diff --git a/tests/functional/base_test.go b/tests/functional/base_test.go index 57996c4b1..c9450e1ee 100644 --- a/tests/functional/base_test.go +++ b/tests/functional/base_test.go @@ -9,6 +9,7 @@ import ( metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "k8s.io/apimachinery/pkg/apis/meta/v1/unstructured" "k8s.io/apimachinery/pkg/types" + "sigs.k8s.io/controller-runtime/pkg/client" dataplanev1 "github.com/openstack-k8s-operators/dataplane-operator/api/v1beta1" infrav1 "github.com/openstack-k8s-operators/infra-operator/apis/network/v1beta1" @@ -344,6 +345,38 @@ func DefaultDataplaneGlobalService(name types.NamespacedName) map[string]interfa } } +// Create simple OpenStackControlPlane +func CreateOpenStackControlPlane(name types.NamespacedName, tlsEnabled bool) client.Object { + + raw := map[string]interface{}{ + "apiVersion": "core.openstack.org/v1beta1", + "kind": "OpenStackControlPlane", + "metadata": map[string]interface{}{ + "name": name.Name, + "namespace": name.Namespace, + }, + "spec": map[string]interface{}{ + "secret": "osp-secret", + "storageClass": "local-storage", + "tls": map[string]interface{}{ + "ingress": map[string]interface{}{ + "enabled": tlsEnabled, + "ca": map[string]interface{}{ + "duration": "100h", + }, + "cert": map[string]interface{}{ + "duration": "10h", + }, + }, + "podLevel": map[string]interface{}{ + "enabled": tlsEnabled, + }, + }, + }, + } + return th.CreateUnstructured(raw) +} + // Get resources // Retrieve OpenStackDataPlaneDeployment and check for errors diff --git a/tests/functional/openstackdataplanedeployment_controller_test.go b/tests/functional/openstackdataplanedeployment_controller_test.go index 3cd18e2b3..c268caae9 100644 --- a/tests/functional/openstackdataplanedeployment_controller_test.go +++ b/tests/functional/openstackdataplanedeployment_controller_test.go @@ -16,6 +16,7 @@ import ( baremetalv1 "github.com/openstack-k8s-operators/openstack-baremetal-operator/api/v1beta1" corev1 "k8s.io/api/core/v1" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" + "k8s.io/utils/ptr" "k8s.io/apimachinery/pkg/types" ) @@ -36,6 +37,7 @@ var _ = Describe("Dataplane Deployment Test", func() { var dataplaneServiceName types.NamespacedName var dataplaneUpdateServiceName types.NamespacedName var dataplaneGlobalServiceName types.NamespacedName + var controlPlaneName types.NamespacedName BeforeEach(func() { dnsMasqName = types.NamespacedName{ @@ -719,4 +721,106 @@ var _ = Describe("Dataplane Deployment Test", func() { ) }) }) + + When("A user sets TLSEnabled to true with control plane TLS disabled", func() { + BeforeEach(func() { + controlPlaneName = types.NamespacedName{ + Name: "mock-control-plane", + Namespace: namespace, + } + CreateSSHSecret(dataplaneSSHSecretName) + DeferCleanup(th.DeleteInstance, th.CreateSecret(neutronOvnMetadataSecretName, map[string][]byte{ + "fake_keys": []byte("blih"), + })) + DeferCleanup(th.DeleteInstance, th.CreateSecret(novaNeutronMetadataSecretName, map[string][]byte{ + "fake_keys": []byte("blih"), + })) + DeferCleanup(th.DeleteInstance, th.CreateSecret(novaCellComputeConfigSecretName, map[string][]byte{ + "fake_keys": []byte("blih"), + })) + DeferCleanup(th.DeleteInstance, th.CreateSecret(novaMigrationSSHKey, map[string][]byte{ + "ssh-privatekey": []byte("fake-ssh-private-key"), + "ssh-publickey": []byte("fake-ssh-public-key"), + })) + DeferCleanup(th.DeleteInstance, th.CreateSecret(ceilometerConfigSecretName, map[string][]byte{ + "fake_keys": []byte("blih"), + })) + // DefaultDataPlanenodeSetSpec comes with two mock services, one marked for deployment on all nodesets + CreateDataplaneService(dataplaneServiceName, false) + CreateDataplaneService(dataplaneGlobalServiceName, true) + + DeferCleanup(th.DeleteService, dataplaneServiceName) + DeferCleanup(th.DeleteService, dataplaneGlobalServiceName) + DeferCleanup(th.DeleteInstance, CreateNetConfig(dataplaneNetConfigName, DefaultNetConfigSpec())) + DeferCleanup(th.DeleteInstance, CreateDNSMasq(dnsMasqName, DefaultDNSMasqSpec())) + SimulateDNSMasqComplete(dnsMasqName) + DeferCleanup(th.DeleteInstance, CreateDataplaneNodeSet(dataplaneNodeSetName, DefaultDataPlaneNodeSetSpec(dataplaneNodeSetName.Name))) + DeferCleanup(th.DeleteInstance, CreateDataplaneDeployment(dataplaneDeploymentName, DefaultDataPlaneDeploymentSpec())) + SimulateIPSetComplete(dataplaneNodeName) + SimulateDNSDataComplete(dataplaneNodeSetName) + + DeferCleanup(th.DeleteInstance, CreateOpenStackControlPlane(controlPlaneName, false)) + }) + + It("Should have Spec fields initialized", func() { + dataplaneDeploymentInstance := GetDataplaneDeployment(dataplaneDeploymentName) + expectedSpec := dataplanev1.OpenStackDataPlaneDeploymentSpec{ + NodeSets: []string{"edpm-compute-nodeset"}, + AnsibleTags: "", + AnsibleLimit: "", + AnsibleSkipTags: "", + DeploymentRequeueTime: 15, + ServicesOverride: nil, + BackoffLimit: ptr.To(int32(6)), + } + Expect(dataplaneDeploymentInstance.Spec).Should(Equal(expectedSpec)) + }) + + It("should have ready condiction set to false and input condition set to unknown", func() { + + nodeSet := dataplanev1.OpenStackDataPlaneNodeSet{} + baremetal := baremetalv1.OpenStackBaremetalSet{ + ObjectMeta: metav1.ObjectMeta{ + Name: nodeSet.Name, + Namespace: nodeSet.Namespace, + }, + } + // Create config map for OVN service + ovnConfigMapName := types.NamespacedName{ + Namespace: namespace, + Name: "ovncontroller-config", + } + mapData := map[string]interface{}{ + "ovsdb-config": "test-ovn-config", + } + th.CreateConfigMap(ovnConfigMapName, mapData) + + nodeSet = *GetDataplaneNodeSet(dataplaneNodeSetName) + + // Set baremetal provisioning conditions to True + Eventually(func(g Gomega) { + // OpenStackBaremetalSet has the same name as OpenStackDataPlaneNodeSet + g.Expect(th.K8sClient.Get(th.Ctx, dataplaneNodeSetName, &baremetal)).To(Succeed()) + baremetal.Status.Conditions.MarkTrue( + condition.ReadyCondition, + condition.ReadyMessage) + g.Expect(th.K8sClient.Status().Update(th.Ctx, &baremetal)).To(Succeed()) + + }, th.Timeout, th.Interval).Should(Succeed()) + + th.ExpectCondition( + dataplaneDeploymentName, + ConditionGetterFunc(DataplaneDeploymentConditionGetter), + condition.ReadyCondition, + corev1.ConditionFalse, + ) + th.ExpectCondition( + dataplaneDeploymentName, + ConditionGetterFunc(DataplaneDeploymentConditionGetter), + condition.InputReadyCondition, + corev1.ConditionUnknown, + ) + }) + + }) }) diff --git a/tests/functional/suite_test.go b/tests/functional/suite_test.go index ef62249b2..f6e45bc97 100644 --- a/tests/functional/suite_test.go +++ b/tests/functional/suite_test.go @@ -26,6 +26,7 @@ import ( "github.com/google/uuid" . "github.com/onsi/ginkgo/v2" //revive:disable:dot-imports . "github.com/onsi/gomega" //revive:disable:dot-imports + openstackv1 "github.com/openstack-k8s-operators/openstack-operator/apis/core/v1beta1" "k8s.io/client-go/kubernetes" "k8s.io/client-go/kubernetes/scheme" ctrl "sigs.k8s.io/controller-runtime" @@ -46,7 +47,6 @@ import ( infrav1 "github.com/openstack-k8s-operators/infra-operator/apis/network/v1beta1" aee "github.com/openstack-k8s-operators/openstack-ansibleee-operator/api/v1beta1" baremetalv1 "github.com/openstack-k8s-operators/openstack-baremetal-operator/api/v1beta1" - openstackv1 "github.com/openstack-k8s-operators/openstack-operator/apis/core/v1beta1" //revive:disable-next-line:dot-imports . "github.com/openstack-k8s-operators/lib-common/modules/common/test/helpers"