From 4d04ae4b9fb2971d3c2f17c8d6699594ee58d2aa Mon Sep 17 00:00:00 2001 From: Francesco Pantano Date: Wed, 28 Aug 2024 14:51:02 +0200 Subject: [PATCH] Add network-attachment-definition support It might be required for the horizon Pods to be able to reach external services on a different network. For this reason this patch introduces the NetworkAttachment interface to the horizon API and implements the same logic already present in other operators. Jira: https://issues.redhat.com/browse/OSPRH-9285 Signed-off-by: Francesco Pantano --- api/bases/horizon.openstack.org_horizons.yaml | 17 +++ api/v1beta1/horizon_types.go | 8 + api/v1beta1/zz_generated.deepcopy.go | 20 +++ .../bases/horizon.openstack.org_horizons.yaml | 17 +++ config/rbac/role.yaml | 8 + controllers/horizon_controller.go | 138 +++++++++++++++++- go.mod | 2 +- main.go | 2 + pkg/horizon/deployment.go | 10 +- tests/functional/horizon_controller_test.go | 29 ++++ tests/functional/suite_test.go | 11 ++ 11 files changed, 255 insertions(+), 7 deletions(-) diff --git a/api/bases/horizon.openstack.org_horizons.yaml b/api/bases/horizon.openstack.org_horizons.yaml index b2a75e14..1167481a 100644 --- a/api/bases/horizon.openstack.org_horizons.yaml +++ b/api/bases/horizon.openstack.org_horizons.yaml @@ -16,6 +16,10 @@ spec: scope: Namespaced versions: - additionalPrinterColumns: + - description: NetworkAttachments + jsonPath: .status.networkAttachments + name: NetworkAttachments + type: string - description: Status jsonPath: .status.conditions[0].status name: Status @@ -1926,6 +1930,12 @@ spec: default: memcached description: Memcached instance name. type: string + networkAttachments: + description: NetworkAttachments is a list of NetworkAttachment resource + names to expose the services to the given network + items: + type: string + type: array nodeSelector: additionalProperties: type: string @@ -2233,6 +2243,13 @@ spec: type: string description: Map of hashes to track e.g. job status type: object + networkAttachments: + additionalProperties: + items: + type: string + type: array + description: NetworkAttachments status of the deployment pods + type: object observedGeneration: description: ObservedGeneration - the most recent generation observed for this service. If the observed generation is less than the spec diff --git a/api/v1beta1/horizon_types.go b/api/v1beta1/horizon_types.go index 65344c9b..7786601b 100644 --- a/api/v1beta1/horizon_types.go +++ b/api/v1beta1/horizon_types.go @@ -100,6 +100,10 @@ type HorizonSpecCore struct { // ExtraMounts containing conf files // +kubebuilder:default={} ExtraMounts []HorizonExtraVolMounts `json:"extraMounts,omitempty"` + + // +kubebuilder:validation:Optional + // NetworkAttachments is a list of NetworkAttachment resource names to expose the services to the given network + NetworkAttachments []string `json:"networkAttachments,omitempty"` } // HorizionOverrideSpec to override the generated manifest of several child resources. @@ -127,10 +131,14 @@ type HorizonStatus struct { // then the controller has not processed the latest changes injected by // the opentack-operator in the top-level CR (e.g. the ContainerImage) ObservedGeneration int64 `json:"observedGeneration,omitempty"` + + // NetworkAttachments status of the deployment pods + NetworkAttachments map[string][]string `json:"networkAttachments,omitempty"` } //+kubebuilder:object:root=true //+kubebuilder:subresource:status +//+kubebuilder:printcolumn:name="NetworkAttachments",type="string",JSONPath=".status.networkAttachments",description="NetworkAttachments" //+kubebuilder:printcolumn:name="Status",type="string",JSONPath=".status.conditions[0].status",description="Status" //+kubebuilder:printcolumn:name="Message",type="string",JSONPath=".status.conditions[0].message",description="Message" diff --git a/api/v1beta1/zz_generated.deepcopy.go b/api/v1beta1/zz_generated.deepcopy.go index 22af5d02..fcbce86c 100644 --- a/api/v1beta1/zz_generated.deepcopy.go +++ b/api/v1beta1/zz_generated.deepcopy.go @@ -192,6 +192,11 @@ func (in *HorizonSpecCore) DeepCopyInto(out *HorizonSpecCore) { (*in)[i].DeepCopyInto(&(*out)[i]) } } + if in.NetworkAttachments != nil { + in, out := &in.NetworkAttachments, &out.NetworkAttachments + *out = make([]string, len(*in)) + copy(*out, *in) + } } // DeepCopy is an autogenerated deepcopy function, copying the receiver, creating a new HorizonSpecCore. @@ -221,6 +226,21 @@ func (in *HorizonStatus) DeepCopyInto(out *HorizonStatus) { (*in)[i].DeepCopyInto(&(*out)[i]) } } + if in.NetworkAttachments != nil { + in, out := &in.NetworkAttachments, &out.NetworkAttachments + *out = make(map[string][]string, len(*in)) + for key, val := range *in { + var outVal []string + if val == nil { + (*out)[key] = nil + } else { + in, out := &val, &outVal + *out = make([]string, len(*in)) + copy(*out, *in) + } + (*out)[key] = outVal + } + } } // DeepCopy is an autogenerated deepcopy function, copying the receiver, creating a new HorizonStatus. diff --git a/config/crd/bases/horizon.openstack.org_horizons.yaml b/config/crd/bases/horizon.openstack.org_horizons.yaml index b2a75e14..1167481a 100644 --- a/config/crd/bases/horizon.openstack.org_horizons.yaml +++ b/config/crd/bases/horizon.openstack.org_horizons.yaml @@ -16,6 +16,10 @@ spec: scope: Namespaced versions: - additionalPrinterColumns: + - description: NetworkAttachments + jsonPath: .status.networkAttachments + name: NetworkAttachments + type: string - description: Status jsonPath: .status.conditions[0].status name: Status @@ -1926,6 +1930,12 @@ spec: default: memcached description: Memcached instance name. type: string + networkAttachments: + description: NetworkAttachments is a list of NetworkAttachment resource + names to expose the services to the given network + items: + type: string + type: array nodeSelector: additionalProperties: type: string @@ -2233,6 +2243,13 @@ spec: type: string description: Map of hashes to track e.g. job status type: object + networkAttachments: + additionalProperties: + items: + type: string + type: array + description: NetworkAttachments status of the deployment pods + type: object observedGeneration: description: ObservedGeneration - the most recent generation observed for this service. If the observed generation is less than the spec diff --git a/config/rbac/role.yaml b/config/rbac/role.yaml index 1a810922..ad466bec 100644 --- a/config/rbac/role.yaml +++ b/config/rbac/role.yaml @@ -103,6 +103,14 @@ rules: - get - patch - update +- apiGroups: + - k8s.cni.cncf.io + resources: + - network-attachment-definitions + verbs: + - get + - list + - watch - apiGroups: - keystone.openstack.org resources: diff --git a/controllers/horizon_controller.go b/controllers/horizon_controller.go index 0e51f089..a752811a 100644 --- a/controllers/horizon_controller.go +++ b/controllers/horizon_controller.go @@ -23,6 +23,7 @@ import ( "time" "github.com/go-logr/logr" + networkv1 "github.com/k8snetworkplumbingwg/network-attachment-definition-client/pkg/apis/k8s.cni.cncf.io/v1" horizonv1beta1 "github.com/openstack-k8s-operators/horizon-operator/api/v1beta1" horizon "github.com/openstack-k8s-operators/horizon-operator/pkg/horizon" memcachedv1 "github.com/openstack-k8s-operators/infra-operator/apis/memcached/v1beta1" @@ -35,17 +36,17 @@ import ( env "github.com/openstack-k8s-operators/lib-common/modules/common/env" helper "github.com/openstack-k8s-operators/lib-common/modules/common/helper" labels "github.com/openstack-k8s-operators/lib-common/modules/common/labels" + nad "github.com/openstack-k8s-operators/lib-common/modules/common/networkattachment" common_rbac "github.com/openstack-k8s-operators/lib-common/modules/common/rbac" oko_secret "github.com/openstack-k8s-operators/lib-common/modules/common/secret" "github.com/openstack-k8s-operators/lib-common/modules/common/service" "github.com/openstack-k8s-operators/lib-common/modules/common/tls" util "github.com/openstack-k8s-operators/lib-common/modules/common/util" - "k8s.io/apimachinery/pkg/fields" - appsv1 "k8s.io/api/apps/v1" corev1 "k8s.io/api/core/v1" rbacv1 "k8s.io/api/rbac/v1" k8s_errors "k8s.io/apimachinery/pkg/api/errors" + "k8s.io/apimachinery/pkg/fields" "k8s.io/apimachinery/pkg/runtime" "k8s.io/apimachinery/pkg/types" "k8s.io/apimachinery/pkg/util/rand" @@ -106,6 +107,8 @@ type HorizonReconciler struct { // service account permissions that are needed to grant permission to the above // +kubebuilder:rbac:groups="security.openshift.io",resourceNames=anyuid,resources=securitycontextconstraints,verbs=use // +kubebuilder:rbac:groups="",resources=pods,verbs=create;delete;get;list;patch;update;watch +// +kubebuilder:rbac:groups=k8s.cni.cncf.io,resources=network-attachment-definitions,verbs=get;list;watch +// service account, role, rolebinding // Reconcile is part of the main kubernetes reconciliation loop which aims to // move the current state of the cluster closer to the desired state. @@ -280,6 +283,35 @@ func (r *HorizonReconciler) SetupWithManager(mgr ctrl.Manager) error { return nil } + // Watch for changes to NADs + nadFn := func(ctx context.Context, o client.Object) []reconcile.Request { + result := []reconcile.Request{} + + // get all Horizon CRs + horizonList := &horizonv1beta1.HorizonList{} + listOpts := []client.ListOption{ + client.InNamespace(o.GetNamespace()), + } + if err := r.Client.List(context.Background(), horizonList, listOpts...); err != nil { + logger.Error(err, "Unable to retrieve Horizon CRs %w") + return nil + } + for _, cr := range horizonList.Items { + if util.StringInSlice(o.GetName(), cr.Spec.NetworkAttachments) { + name := client.ObjectKey{ + Namespace: cr.GetNamespace(), + Name: cr.GetName(), + } + logger.Info(fmt.Sprintf("NAD %s is used by Horizon CR %s", o.GetName(), cr.GetName())) + result = append(result, reconcile.Request{NamespacedName: name}) + } + } + if len(result) > 0 { + return result + } + return nil + } + return ctrl.NewControllerManagedBy(mgr). For(&horizonv1beta1.Horizon{}). Owns(&corev1.Service{}). @@ -291,6 +323,8 @@ func (r *HorizonReconciler) SetupWithManager(mgr ctrl.Manager) error { Owns(&rbacv1.RoleBinding{}). Watches(&memcachedv1.Memcached{}, handler.EnqueueRequestsFromMapFunc(memcachedFn)). + Watches(&networkv1.NetworkAttachmentDefinition{}, + handler.EnqueueRequestsFromMapFunc(nadFn)). Watches( &corev1.Secret{}, handler.EnqueueRequestsFromMapFunc(r.findObjectsForSrc), @@ -668,6 +702,22 @@ func (r *HorizonReconciler) reconcileNormal(ctx context.Context, instance *horiz return ctrlResult, err } + var serviceAnnotations map[string]string + // networks to attach to + serviceAnnotations, ctrlResult, err = r.ensureNAD(ctx, instance, instance.Spec.NetworkAttachments, helper) + if err != nil { + instance.Status.Conditions.MarkFalse( + condition.NetworkAttachmentsReadyCondition, + condition.ErrorReason, + condition.SeverityWarning, + condition.NetworkAttachmentsReadyErrorMessage, + err, + ) + return ctrlResult, err + } + + instance.Status.Conditions.MarkTrue(condition.NetworkAttachmentsReadyCondition, condition.NetworkAttachmentsReadyMessage) + // Handle service update ctrlResult, err = r.reconcileUpdate(ctx) if err != nil || (ctrlResult != ctrl.Result{}) { @@ -685,7 +735,7 @@ func (r *HorizonReconciler) reconcileNormal(ctx context.Context, instance *horiz // // Define a new Deployment object - deplDef, err := horizon.Deployment(instance, inputHash, serviceLabels) + deplDef, err := horizon.Deployment(instance, inputHash, serviceLabels, serviceAnnotations) if err != nil { Log.Error(err, "Deployment failed") instance.Status.Conditions.Set(condition.FalseCondition( @@ -721,6 +771,42 @@ func (r *HorizonReconciler) reconcileNormal(ctx context.Context, instance *horiz return ctrlResult, nil } instance.Status.ReadyCount = depl.GetDeployment().Status.ReadyReplicas + + networkReady := false + var networkAttachmentStatus map[string][]string + // verify if network attachment matches expectations + if *instance.Spec.Replicas > 0 { + networkReady, networkAttachmentStatus, err = nad.VerifyNetworkStatusFromAnnotation( + ctx, + helper, + instance.Spec.NetworkAttachments, + serviceLabels, + instance.Status.ReadyCount, + ) + if err != nil { + err = fmt.Errorf("verifying API NetworkAttachments (%s) %w", instance.Spec.NetworkAttachments, err) + instance.Status.Conditions.MarkFalse( + condition.NetworkAttachmentsReadyCondition, + condition.ErrorReason, + condition.SeverityWarning, + condition.NetworkAttachmentsReadyErrorMessage, + err) + return ctrl.Result{}, err + } + instance.Status.NetworkAttachments = networkAttachmentStatus + if networkReady { + instance.Status.Conditions.MarkTrue(condition.NetworkAttachmentsReadyCondition, condition.NetworkAttachmentsReadyMessage) + } else { + err := fmt.Errorf("not all pods have interfaces with ips as configured in NetworkAttachments: %s", instance.Spec.NetworkAttachments) + instance.Status.Conditions.Set(condition.FalseCondition( + condition.NetworkAttachmentsReadyCondition, + condition.ErrorReason, + condition.SeverityWarning, + condition.NetworkAttachmentsReadyErrorMessage, + err.Error())) + return ctrl.Result{}, err + } + } // Mark the Deployment as Ready only if the number of Replicas is equals // to the Deployed instances (ReadyCount), but mark it as True is Replicas // is zero. In addition, make sure the controller sees the last Generation @@ -748,7 +834,6 @@ func (r *HorizonReconciler) reconcileNormal(ctx context.Context, instance *horiz } // generateServiceConfigMaps - create configmaps which hold scripts and service configuration -// TODO add DefaultConfigOverwrite func (r *HorizonReconciler) generateServiceConfigMaps( ctx context.Context, instance *horizonv1beta1.Horizon, @@ -903,3 +988,48 @@ func configureHorizonRbac(ctx context.Context, helper *helper.Helper, instance * } return common_rbac.ReconcileRbac(ctx, helper, instance, rbacRules) } + +// ensureNAD - this function gets NADs based on the string[] passed as input +// and produces the required Annotation for the Horizon component +func (r *HorizonReconciler) ensureNAD( + ctx context.Context, + instance *horizonv1beta1.Horizon, + nAttach []string, + helper *helper.Helper, +) (map[string]string, ctrl.Result, error) { + + var serviceAnnotations map[string]string + var err error + // Iterate over the []networkattachment, get the corresponding NAD and create + // the required annotation + for _, netAtt := range nAttach { + _, err = nad.GetNADWithName(ctx, helper, netAtt, helper.GetBeforeObject().GetNamespace()) + if err != nil { + if k8s_errors.IsNotFound(err) { + helper.GetLogger().Info(fmt.Sprintf("network-attachment-definition %s not found", netAtt)) + + instance.Status.Conditions.Set(condition.FalseCondition( + condition.NetworkAttachmentsReadyCondition, + condition.RequestedReason, + condition.SeverityInfo, + condition.NetworkAttachmentsReadyWaitingMessage, + netAtt)) + return serviceAnnotations, ctrl.Result{RequeueAfter: time.Second * 10}, nil + } + instance.Status.Conditions.Set(condition.FalseCondition( + condition.NetworkAttachmentsReadyCondition, + condition.ErrorReason, + condition.SeverityWarning, + condition.NetworkAttachmentsReadyErrorMessage, + err.Error())) + return serviceAnnotations, ctrl.Result{RequeueAfter: time.Second * 10}, nil + } + } + // Create NetworkAnnotations + serviceAnnotations, err = nad.CreateNetworksAnnotation(helper.GetBeforeObject().GetNamespace(), nAttach) + if err != nil { + return serviceAnnotations, ctrl.Result{}, fmt.Errorf("failed create network annotation from %s: %w", + nAttach, err) + } + return serviceAnnotations, ctrl.Result{}, err +} diff --git a/go.mod b/go.mod index 04191d2c..55986ec5 100644 --- a/go.mod +++ b/go.mod @@ -7,6 +7,7 @@ replace github.com/openstack-k8s-operators/horizon-operator/api => ./api require ( github.com/go-logr/logr v1.4.2 github.com/google/uuid v1.6.0 + github.com/k8snetworkplumbingwg/network-attachment-definition-client v1.4.0 github.com/onsi/ginkgo/v2 v2.19.1 github.com/onsi/gomega v1.34.1 github.com/openstack-k8s-operators/horizon-operator/api v0.3.1-0.20240214134444-c675e5f69043 @@ -45,7 +46,6 @@ require ( github.com/imdario/mergo v0.3.16 // indirect github.com/josharian/intern v1.0.0 // indirect github.com/json-iterator/go v1.1.12 // indirect - github.com/k8snetworkplumbingwg/network-attachment-definition-client v1.4.0 // indirect github.com/mailru/easyjson v0.7.7 // indirect github.com/modern-go/concurrent v0.0.0-20180306012644-bacd9c7ef1dd // indirect github.com/modern-go/reflect2 v1.0.2 // indirect diff --git a/main.go b/main.go index 49fb682c..220e9476 100644 --- a/main.go +++ b/main.go @@ -38,6 +38,7 @@ import ( metricsserver "sigs.k8s.io/controller-runtime/pkg/metrics/server" + networkv1 "github.com/k8snetworkplumbingwg/network-attachment-definition-client/pkg/apis/k8s.cni.cncf.io/v1" memcachedv1 "github.com/openstack-k8s-operators/infra-operator/apis/memcached/v1beta1" keystonev1 "github.com/openstack-k8s-operators/keystone-operator/api/v1beta1" @@ -54,6 +55,7 @@ var ( func init() { utilruntime.Must(clientgoscheme.AddToScheme(scheme)) + utilruntime.Must(networkv1.AddToScheme(scheme)) utilruntime.Must(horizonv1beta1.AddToScheme(scheme)) utilruntime.Must(keystonev1.AddToScheme(scheme)) utilruntime.Must(memcachedv1.AddToScheme(scheme)) diff --git a/pkg/horizon/deployment.go b/pkg/horizon/deployment.go index 18366fd2..519399c1 100644 --- a/pkg/horizon/deployment.go +++ b/pkg/horizon/deployment.go @@ -33,7 +33,12 @@ const ( ) // Deployment creates the k8s deployment structure required to run Horizon -func Deployment(instance *horizonv1.Horizon, configHash string, labels map[string]string) (*appsv1.Deployment, error) { +func Deployment( + instance *horizonv1.Horizon, + configHash string, + labels map[string]string, + annotations map[string]string, +) (*appsv1.Deployment, error) { runAsUser := int64(0) args := []string{"-c", ServiceCommand} @@ -124,7 +129,8 @@ func Deployment(instance *horizonv1.Horizon, configHash string, labels map[strin Replicas: instance.Spec.Replicas, Template: corev1.PodTemplateSpec{ ObjectMeta: metav1.ObjectMeta{ - Labels: labels, + Annotations: annotations, + Labels: labels, }, Spec: corev1.PodSpec{ ServiceAccountName: instance.RbacResourceName(), diff --git a/tests/functional/horizon_controller_test.go b/tests/functional/horizon_controller_test.go index cc4fbc80..df2387b6 100644 --- a/tests/functional/horizon_controller_test.go +++ b/tests/functional/horizon_controller_test.go @@ -411,4 +411,33 @@ var _ = Describe("Horizon controller", func() { }, timeout, interval).Should(Succeed()) }) }) + When("Horizon CR instance is built with NAD", func() { + BeforeEach(func() { + nad := th.CreateNetworkAttachmentDefinition(types.NamespacedName{ + Namespace: namespace, + Name: "internalapi", + }) + DeferCleanup(th.DeleteInstance, nad) + rawSpec := map[string]interface{}{ + "secret": SecretName, + "networkAttachments": []string{"storage"}, + "memcachedInstance": "memcached", + } + DeferCleanup(th.DeleteInstance, CreateHorizon(horizonName, rawSpec)) + DeferCleanup( + k8sClient.Delete, ctx, CreateHorizonSecret(namespace, SecretName)) + DeferCleanup(infra.DeleteMemcached, infra.CreateMemcached(namespace, "memcached", memcachedSpec)) + infra.SimulateMemcachedReady(types.NamespacedName{ + Name: "memcached", + Namespace: namespace, + }) + keystoneAPI := keystone.CreateKeystoneAPI(namespace) + DeferCleanup(keystone.DeleteKeystoneAPI, keystoneAPI) + }) + It("Check the resulting endpoints of the generated sub-CRs", func() { + th.SimulateDeploymentReplicaReady(horizonName) + horizon := GetHorizon(horizonName) + Expect(horizon.Spec.NetworkAttachments).To(Equal([]string{"storage"})) + }) + }) }) diff --git a/tests/functional/suite_test.go b/tests/functional/suite_test.go index 1ddf1595..43bef70a 100644 --- a/tests/functional/suite_test.go +++ b/tests/functional/suite_test.go @@ -38,6 +38,7 @@ import ( "sigs.k8s.io/controller-runtime/pkg/log/zap" "sigs.k8s.io/controller-runtime/pkg/webhook" + networkv1 "github.com/k8snetworkplumbingwg/network-attachment-definition-client/pkg/apis/k8s.cni.cncf.io/v1" metricsserver "sigs.k8s.io/controller-runtime/pkg/metrics/server" horizonv1 "github.com/openstack-k8s-operators/horizon-operator/api/v1beta1" @@ -97,6 +98,9 @@ var _ = BeforeSuite(func() { memcachedCRDs, err := test.GetCRDDirFromModule( "github.com/openstack-k8s-operators/infra-operator/apis", "../../go.mod", "bases") Expect(err).ShouldNot(HaveOccurred()) + networkv1CRD, err := test.GetCRDDirFromModule( + "github.com/k8snetworkplumbingwg/network-attachment-definition-client", "../../go.mod", "artifacts/networks-crd.yaml") + Expect(err).ShouldNot(HaveOccurred()) By("bootstrapping test environment") testEnv = &envtest.Environment{ @@ -105,6 +109,11 @@ var _ = BeforeSuite(func() { keystoneCRDs, memcachedCRDs, }, + CRDInstallOptions: envtest.CRDInstallOptions{ + Paths: []string{ + networkv1CRD, + }, + }, ErrorIfCRDPathMissing: true, WebhookInstallOptions: envtest.WebhookInstallOptions{ Paths: []string{filepath.Join("..", "..", "config", "webhook")}, @@ -126,6 +135,8 @@ var _ = BeforeSuite(func() { Expect(err).NotTo(HaveOccurred()) err = memcachedv1.AddToScheme(scheme.Scheme) Expect(err).NotTo(HaveOccurred()) + err = networkv1.AddToScheme(scheme.Scheme) + Expect(err).NotTo(HaveOccurred()) //+kubebuilder:scaffold:scheme