From e89336dc42eb0a4f4c3a9861f5390b9519c5ab5b Mon Sep 17 00:00:00 2001 From: Martin Schuppert Date: Mon, 24 Jul 2023 17:51:26 +0200 Subject: [PATCH 1/9] Allow customize service via service.Override Adds an OverridSpec to the Service which allows to customize metadata.Annotations, metadata.Labels and spec of a service. The override values get merged into the object definition created by the operator. This is part of moving the external connection config into the openstack-operator and allows to override the default service created by an operatior with a MetalLB LoadBalancer service spec. Jira: OSP-26690 --- modules/certmanager/go.mod | 2 + modules/certmanager/go.sum | 2 - modules/common/endpoint/endpoint.go | 19 +- modules/common/service/service.go | 122 ++++++++++--- modules/common/service/types.go | 160 +++++++++++++++- .../common/service/zz_generated.deepcopy.go | 141 ++++++++++++++ .../common/test/functional/service_test.go | 172 ++++++++++++++++++ modules/openstack/go.mod | 1 - modules/openstack/go.sum | 2 - modules/openstack/openstack.go | 8 +- modules/test/go.mod | 4 +- modules/test/go.sum | 2 - 12 files changed, 593 insertions(+), 42 deletions(-) create mode 100644 modules/common/service/zz_generated.deepcopy.go create mode 100644 modules/common/test/functional/service_test.go diff --git a/modules/certmanager/go.mod b/modules/certmanager/go.mod index d5cabce3..d0b8b636 100644 --- a/modules/certmanager/go.mod +++ b/modules/certmanager/go.mod @@ -90,6 +90,8 @@ replace github.com/openstack-k8s-operators/lib-common/modules/common => ../commo replace github.com/openstack-k8s-operators/lib-common/modules/test => ../test +replace github.com/openstack-k8s-operators/lib-common/modules/openstack => ../openstack + // mschuppert: map to latest commit from release-4.13 tag // must consistent within modules and service operators replace github.com/openshift/api => github.com/openshift/api v0.0.0-20230414143018-3367bc7e6ac7 diff --git a/modules/certmanager/go.sum b/modules/certmanager/go.sum index 2fdeea13..b337a108 100644 --- a/modules/certmanager/go.sum +++ b/modules/certmanager/go.sum @@ -240,8 +240,6 @@ github.com/openstack-k8s-operators/infra-operator/apis v0.1.1-0.20230914145253-1 github.com/openstack-k8s-operators/infra-operator/apis v0.1.1-0.20230914145253-116f307c7875/go.mod h1:NgrvT3CKMu6fE8Nt1H79qHx11L3I7Bb2eItniM7c9ow= github.com/openstack-k8s-operators/keystone-operator/api v0.1.1-0.20230914163026-da9aa9de960a h1:MFYwi2Xk9r3OMPToCSbvqYVNrm7P+aFzGDN0eVNpgu8= github.com/openstack-k8s-operators/keystone-operator/api v0.1.1-0.20230914163026-da9aa9de960a/go.mod h1:nxrbUOIGMJ1h2pNlawhURdt/XJ95dW2wZGedmOVo2aw= -github.com/openstack-k8s-operators/lib-common/modules/openstack v0.1.1-0.20230913075424-2680ce4b6ad2 h1:4L5DRfSnomBfyRwCfAzqQwk0+osnIbyJ1VvKBy+tzyY= -github.com/openstack-k8s-operators/lib-common/modules/openstack v0.1.1-0.20230913075424-2680ce4b6ad2/go.mod h1:NZ6weu5xOAkNPTqg1luC22DO7ZbyqiilRkvrFfhjFm0= github.com/openstack-k8s-operators/mariadb-operator/api v0.1.1-0.20230913081601-9e4fc8aadad5 h1:dQcSQuXfgzgOhc4v+zD0jE6WWhn6FHr5nALOjJBPxyI= github.com/openstack-k8s-operators/mariadb-operator/api v0.1.1-0.20230913081601-9e4fc8aadad5/go.mod h1:mJyhm/YiQZaYhLvOuLng/ITpwx8HvsYVht+VotS1Ed8= github.com/pkg/errors v0.8.0/go.mod h1:bwawxfHBFNV+L2hUp1rHADufV3IMtnDRdf1r5NINEl0= diff --git a/modules/common/endpoint/endpoint.go b/modules/common/endpoint/endpoint.go index 91c74da1..6497d00a 100644 --- a/modules/common/endpoint/endpoint.go +++ b/modules/common/endpoint/endpoint.go @@ -78,7 +78,7 @@ func ExposeEndpoints( h *helper.Helper, serviceName string, endpointSelector map[string]string, - endpoints map[Endpoint]Data, + endpoints map[service.Endpoint]Data, timeout time.Duration, ) (map[string]string, ctrl.Result, error) { endpointMap := make(map[string]string) @@ -105,7 +105,7 @@ func ExposeEndpoints( } // Create the service - svc := service.NewService( + svc, err := service.NewService( service.MetalLBService(&service.MetalLBServiceDetails{ Name: endpointName, Namespace: h.GetBeforeObject().GetNamespace(), @@ -119,10 +119,14 @@ func ExposeEndpoints( }), exportLabels, timeout, + &service.OverrideSpec{}, ) + if err != nil { + return endpointMap, ctrl.Result{}, err + } annotations := map[string]string{ service.MetalLBAddressPoolAnnotation: data.MetalLB.IPAddressPool, - AnnotationHostnameKey: svc.GetServiceHostname(), // add annotation to register service name in dnsmasq + service.AnnotationHostnameKey: svc.GetServiceHostname(), // add annotation to register service name in dnsmasq } if len(data.MetalLB.LoadBalancerIPs) > 0 { annotations[service.MetalLBLoadBalancerIPs] = strings.Join(data.MetalLB.LoadBalancerIPs, ",") @@ -148,7 +152,7 @@ func ExposeEndpoints( } else { // Create the service - svc := service.NewService( + svc, err := service.NewService( service.GenericService(&service.GenericServiceDetails{ Name: endpointName, Namespace: h.GetBeforeObject().GetNamespace(), @@ -161,7 +165,12 @@ func ExposeEndpoints( }}), exportLabels, 5, + &service.OverrideSpec{}, ) + if err != nil { + return endpointMap, ctrl.Result{}, err + } + ctrlResult, err := svc.CreateOrPatch(ctx, h) if err != nil { return endpointMap, ctrlResult, err @@ -173,7 +182,7 @@ func ExposeEndpoints( hostname = svc.GetServiceHostnamePort() // Create the route if it is public endpoint - if endpointType == EndpointPublic { + if endpointType == service.EndpointPublic { // Create the route // TODO TLS route, err := route.NewRoute( diff --git a/modules/common/service/service.go b/modules/common/service/service.go index c17cc863..ba92e79b 100644 --- a/modules/common/service/service.go +++ b/modules/common/service/service.go @@ -18,6 +18,7 @@ package service import ( "context" + "encoding/json" "fmt" "time" @@ -26,6 +27,7 @@ import ( metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "k8s.io/apimachinery/pkg/labels" "k8s.io/apimachinery/pkg/types" + "k8s.io/apimachinery/pkg/util/strategicpatch" "sigs.k8s.io/controller-runtime/pkg/client" "sigs.k8s.io/controller-runtime/pkg/controller/controllerutil" @@ -39,12 +41,50 @@ func NewService( service *corev1.Service, labels map[string]string, timeout time.Duration, -) *Service { - return &Service{ + override *OverrideSpec, +) (*Service, error) { + svc := &Service{ service: service, serviceHostname: fmt.Sprintf("%s.%s.svc", service.Name, service.GetNamespace()), timeout: timeout, } + + // patch service with possible overrides of Labels, Annotations and Spec + if override != nil { + if override.EmbeddedLabelsAnnotations != nil { + if override.Labels != nil { + svc.service.Labels = util.MergeStringMaps(override.Labels, service.Labels) + } + if override.Annotations != nil { + svc.service.Annotations = util.MergeStringMaps(override.Annotations, service.Annotations) + } + } + if override.Spec != nil { + originalSpec, err := json.Marshal(service.Spec) + if err != nil { + return svc, fmt.Errorf("error marshalling Service Spec: %w", err) + } + + patch, err := json.Marshal(override.Spec) + if err != nil { + return svc, fmt.Errorf("error marshalling Service Spec override: %w", err) + } + + patchedJSON, err := strategicpatch.StrategicMergePatch(originalSpec, patch, corev1.ServiceSpec{}) + if err != nil { + return svc, fmt.Errorf("error patching Service Spec: %w", err) + } + + patchedSpec := corev1.ServiceSpec{} + err = json.Unmarshal(patchedJSON, &patchedSpec) + if err != nil { + return svc, fmt.Errorf("error unmarshalling patched Service Spec: %w", err) + } + svc.service.Spec = patchedSpec + } + } + + return svc, nil } // GetClusterIPs - returns the cluster IPs of the created service @@ -72,6 +112,27 @@ func (s *Service) GetServiceHostnamePort() string { return fmt.Sprintf("%s:%d", s.GetServiceHostname(), GetServicesPortDetails(s.service, s.service.Name).Port) } +// GetLabels - returns labels of the service +func (s *Service) GetLabels() map[string]string { + return s.service.Labels +} + +// GetAnnotations - returns annotations of the service +func (s *Service) GetAnnotations() map[string]string { + return s.service.Annotations +} + +// GetSpec - returns the spec of the service +func (s *Service) GetSpec() *corev1.ServiceSpec { + spec := s.service.Spec + return &spec +} + +// GetServiceType - returns type of the service spec +func (s *Service) GetServiceType() corev1.ServiceType { + return s.service.Spec.Type +} + // AddAnnotation - Adds annotation and merges it with the current set func (s *Service) AddAnnotation(anno map[string]string) { s.service.Annotations = util.MergeStringMaps(s.service.Annotations, anno) @@ -79,6 +140,19 @@ func (s *Service) AddAnnotation(anno map[string]string) { // GenericService func func GenericService(svcInfo *GenericServiceDetails) *corev1.Service { + ports := svcInfo.Ports + if len(svcInfo.Ports) == 0 { + ports = []corev1.ServicePort{ + { + Name: svcInfo.Port.Name, + Port: svcInfo.Port.Port, + // corev1.ProtocolTCP/ corev1.ProtocolUDP/ corev1.ProtocolSCTP + // - https://pkg.go.dev/k8s.io/api@v0.23.6/core/v1#Protocol + Protocol: svcInfo.Port.Protocol, + }, + } + + } return &corev1.Service{ ObjectMeta: metav1.ObjectMeta{ Name: svcInfo.Name, @@ -86,23 +160,31 @@ func GenericService(svcInfo *GenericServiceDetails) *corev1.Service { Labels: svcInfo.Labels, }, Spec: corev1.ServiceSpec{ - Selector: svcInfo.Selector, - Ports: []corev1.ServicePort{ - { - Name: svcInfo.Port.Name, - Port: svcInfo.Port.Port, - // corev1.ProtocolTCP/ corev1.ProtocolUDP/ corev1.ProtocolSCTP - // - https://pkg.go.dev/k8s.io/api@v0.23.6/core/v1#Protocol - Protocol: svcInfo.Port.Protocol, - }, - }, + Selector: svcInfo.Selector, + Ports: ports, ClusterIP: svcInfo.ClusterIP, + Type: corev1.ServiceTypeClusterIP, }, } } // MetalLBService func +// NOTE: (mschuppert) deprecated, can be removed when external endpoint creation moved to openstack-operator func MetalLBService(svcInfo *MetalLBServiceDetails) *corev1.Service { + ports := svcInfo.Ports + if len(svcInfo.Ports) == 0 { + ports = []corev1.ServicePort{ + { + Name: svcInfo.Port.Name, + Port: svcInfo.Port.Port, + // corev1.ProtocolTCP/ corev1.ProtocolUDP/ corev1.ProtocolSCTP + // - https://pkg.go.dev/k8s.io/api@v0.23.6/core/v1#Protocol + Protocol: svcInfo.Port.Protocol, + }, + } + + } + return &corev1.Service{ ObjectMeta: metav1.ObjectMeta{ Name: svcInfo.Name, @@ -112,16 +194,8 @@ func MetalLBService(svcInfo *MetalLBServiceDetails) *corev1.Service { }, Spec: corev1.ServiceSpec{ Selector: svcInfo.Selector, - Ports: []corev1.ServicePort{ - { - Name: svcInfo.Port.Name, - Port: svcInfo.Port.Port, - // corev1.ProtocolTCP/ corev1.ProtocolUDP/ corev1.ProtocolSCTP - // - https://pkg.go.dev/k8s.io/api@v0.23.6/core/v1#Protocol - Protocol: svcInfo.Port.Protocol, - }, - }, - Type: corev1.ServiceTypeLoadBalancer, + Ports: ports, + Type: corev1.ServiceTypeLoadBalancer, }, } } @@ -139,8 +213,8 @@ func (s *Service) CreateOrPatch( } op, err := controllerutil.CreateOrPatch(ctx, h.GetClient(), service, func() error { - service.Labels = util.MergeStringMaps(service.Labels, s.service.Labels) - service.Annotations = util.MergeStringMaps(service.Annotations, s.service.Annotations) + service.Labels = s.service.Labels + service.Annotations = s.service.Annotations service.Spec = s.service.Spec err := controllerutil.SetControllerReference(h.GetBeforeObject(), service, h.GetScheme()) diff --git a/modules/common/service/types.go b/modules/common/service/types.go index e1968ace..fca3e659 100644 --- a/modules/common/service/types.go +++ b/modules/common/service/types.go @@ -14,6 +14,8 @@ See the License for the specific language governing permissions and limitations under the License. */ +// +kubebuilder:object:generate:=true + package service import ( @@ -22,7 +24,25 @@ import ( corev1 "k8s.io/api/core/v1" ) +// Endpoint - typedef to enumerate Endpoint verbs +// NOTE: (mschuppert) have to duplicate this for now. Can not have +// circular dep back to endpoint pkg also can not move it at this +// point as we have circular dep in test module to keystone. +type Endpoint string + +const ( + // EndpointAdmin - admin endpoint + EndpointAdmin Endpoint = "admin" + // EndpointInternal - internal endpoint + EndpointInternal Endpoint = "internal" + // EndpointPublic - public endpoint + EndpointPublic Endpoint = "public" + // AnnotationHostnameKey - + AnnotationHostnameKey = "dnsmasq.network.openstack.org/hostname" +) + // Service - +// +kubebuilder:object:generate:=false type Service struct { service *corev1.Service timeout time.Duration @@ -33,16 +53,20 @@ type Service struct { } // GenericServiceDetails - +// +kubebuilder:object:generate:=false type GenericServiceDetails struct { Name string Namespace string Labels map[string]string Selector map[string]string + // deprecated, use Ports Port GenericServicePort + Ports []corev1.ServicePort ClusterIP string } // GenericServicePort - +// +kubebuilder:object:generate:=false type GenericServicePort struct { Name string Port int32 @@ -50,13 +74,16 @@ type GenericServicePort struct { } // MetalLBServiceDetails - +// +kubebuilder:object:generate:=false type MetalLBServiceDetails struct { Name string Namespace string Annotations map[string]string Labels map[string]string Selector map[string]string - Port GenericServicePort + // deprecated, use Ports + Port GenericServicePort + Ports []corev1.ServicePort } const ( @@ -67,3 +94,134 @@ const ( // MetalLBLoadBalancerIPs - MetalLBLoadBalancerIPs = "metallb.universe.tf/loadBalancerIPs" ) + +// OverrideSpec - service override configuration for the Service created to serve traffic to the cluster. +// Allows for the manifest of the created Service to be overwritten with custom configuration. +type OverrideSpec struct { + *EmbeddedLabelsAnnotations `json:"metadata,omitempty" protobuf:"bytes,1,opt,name=metadata"` + Spec *OverrideServiceSpec `json:"spec,omitempty" protobuf:"bytes,2,opt,name=spec"` +} + +// RoutedOverrideSpec - a routed service override configuration for the Service created to serve traffic +// to the cluster. Allows for the manifest of the created Service to be overwritten with custom configuration. +type RoutedOverrideSpec struct { + OverrideSpec `json:",inline"` + EndpointURL *string `json:"endpointURL,omitempty"` +} + +// EmbeddedLabelsAnnotations is an embedded subset of the fields included in k8s.io/apimachinery/pkg/apis/meta/v1.ObjectMeta. +// Only labels and annotations are included. +type EmbeddedLabelsAnnotations struct { + // Map of string keys and values that can be used to organize and categorize + // (scope and select) objects. May match selectors of replication controllers + // and services. + // More info: http://kubernetes.io/docs/user-guide/labels + // +optional + Labels map[string]string `json:"labels,omitempty" protobuf:"bytes,11,rep,name=labels"` + + // Annotations is an unstructured key value map stored with a resource that may be + // set by external tools to store and retrieve arbitrary metadata. They are not + // queryable and should be preserved when modifying objects. + // More info: http://kubernetes.io/docs/user-guide/annotations + // +optional + Annotations map[string]string `json:"annotations,omitempty" protobuf:"bytes,12,rep,name=annotations"` +} + +// OverrideServiceSpec is a subset of the fields included in https://pkg.go.dev/k8s.io/api@v0.26.6/core/v1#ServiceSpec +// Limited to Type, SessionAffinity, LoadBalancerSourceRanges, ExternalName, ExternalTrafficPolicy, SessionAffinityConfig, +// IPFamilyPolicy, LoadBalancerClass and InternalTrafficPolicy +type OverrideServiceSpec struct { + // type determines how the Service is exposed. Defaults to ClusterIP. Valid + // options are ExternalName, ClusterIP, NodePort, and LoadBalancer. + // "ClusterIP" allocates a cluster-internal IP address for load-balancing + // to endpoints. Endpoints are determined by the selector or if that is not + // specified, by manual construction of an Endpoints object or + // EndpointSlice objects. If clusterIP is "None", no virtual IP is + // allocated and the endpoints are published as a set of endpoints rather + // than a virtual IP. + // "NodePort" builds on ClusterIP and allocates a port on every node which + // routes to the same endpoints as the clusterIP. + // "LoadBalancer" builds on NodePort and creates an external load-balancer + // (if supported in the current cloud) which routes to the same endpoints + // as the clusterIP. + // "ExternalName" aliases this service to the specified externalName. + // Several other fields do not apply to ExternalName services. + // More info: https://kubernetes.io/docs/concepts/services-networking/service/#publishing-services-service-types + // +optional + Type corev1.ServiceType `json:"type,omitempty" protobuf:"bytes,4,opt,name=type,casttype=ServiceType"` + + // Supports "ClientIP" and "None". Used to maintain session affinity. + // Enable client IP based session affinity. + // Must be ClientIP or None. + // Defaults to None. + // More info: https://kubernetes.io/docs/concepts/services-networking/service/#virtual-ips-and-service-proxies + // +optional + SessionAffinity corev1.ServiceAffinity `json:"sessionAffinity,omitempty" protobuf:"bytes,7,opt,name=sessionAffinity,casttype=ServiceAffinity"` + + // If specified and supported by the platform, this will restrict traffic through the cloud-provider + // load-balancer will be restricted to the specified client IPs. This field will be ignored if the + // cloud-provider does not support the feature." + // More info: https://kubernetes.io/docs/tasks/access-application-cluster/create-external-load-balancer/ + // +optional + LoadBalancerSourceRanges []string `json:"loadBalancerSourceRanges,omitempty" protobuf:"bytes,9,opt,name=loadBalancerSourceRanges"` + + // externalName is the external reference that discovery mechanisms will + // return as an alias for this service (e.g. a DNS CNAME record). No + // proxying will be involved. Must be a lowercase RFC-1123 hostname + // (https://tools.ietf.org/html/rfc1123) and requires `type` to be "ExternalName". + // +optional + ExternalName string `json:"externalName,omitempty" protobuf:"bytes,10,opt,name=externalName"` + + // externalTrafficPolicy describes how nodes distribute service traffic they + // receive on one of the Service's "externally-facing" addresses (NodePorts, + // ExternalIPs, and LoadBalancer IPs). If set to "Local", the proxy will configure + // the service in a way that assumes that external load balancers will take care + // of balancing the service traffic between nodes, and so each node will deliver + // traffic only to the node-local endpoints of the service, without masquerading + // the client source IP. (Traffic mistakenly sent to a node with no endpoints will + // be dropped.) The default value, "Cluster", uses the standard behavior of + // routing to all endpoints evenly (possibly modified by topology and other + // features). Note that traffic sent to an External IP or LoadBalancer IP from + // within the cluster will always get "Cluster" semantics, but clients sending to + // a NodePort from within the cluster may need to take traffic policy into account + // when picking a node. + // +optional + ExternalTrafficPolicy corev1.ServiceExternalTrafficPolicyType `json:"externalTrafficPolicy,omitempty" protobuf:"bytes,11,opt,name=externalTrafficPolicy"` + + // sessionAffinityConfig contains the configurations of session affinity. + // +optional + SessionAffinityConfig *corev1.SessionAffinityConfig `json:"sessionAffinityConfig,omitempty" protobuf:"bytes,14,opt,name=sessionAffinityConfig"` + + // IPFamilyPolicy represents the dual-stack-ness requested or required by + // this Service. If there is no value provided, then this field will be set + // to SingleStack. Services can be "SingleStack" (a single IP family), + // "PreferDualStack" (two IP families on dual-stack configured clusters or + // a single IP family on single-stack clusters), or "RequireDualStack" + // (two IP families on dual-stack configured clusters, otherwise fail). The + // ipFamilies and clusterIPs fields depend on the value of this field. This + // field will be wiped when updating a service to type ExternalName. + // +optional + IPFamilyPolicy *corev1.IPFamilyPolicy `json:"ipFamilyPolicy,omitempty" protobuf:"bytes,17,opt,name=ipFamilyPolicy,casttype=IPFamilyPolicy"` + + // loadBalancerClass is the class of the load balancer implementation this Service belongs to. + // If specified, the value of this field must be a label-style identifier, with an optional prefix, + // e.g. "internal-vip" or "example.com/internal-vip". Unprefixed names are reserved for end-users. + // This field can only be set when the Service type is 'LoadBalancer'. If not set, the default load + // balancer implementation is used, today this is typically done through the cloud provider integration, + // but should apply for any default implementation. If set, it is assumed that a load balancer + // implementation is watching for Services with a matching class. Any default load balancer + // implementation (e.g. cloud providers) should ignore Services that set this field. + // This field can only be set when creating or updating a Service to type 'LoadBalancer'. + // Once set, it can not be changed. This field will be wiped when a service is updated to a non 'LoadBalancer' type. + // +optional + LoadBalancerClass *string `json:"loadBalancerClass,omitempty" protobuf:"bytes,21,opt,name=loadBalancerClass"` + + // InternalTrafficPolicy describes how nodes distribute service traffic they + // receive on the ClusterIP. If set to "Local", the proxy will assume that pods + // only want to talk to endpoints of the service on the same node as the pod, + // dropping the traffic if there are no local endpoints. The default value, + // "Cluster", uses the standard behavior of routing to all endpoints evenly + // (possibly modified by topology and other features). + // +optional + InternalTrafficPolicy *corev1.ServiceInternalTrafficPolicyType `json:"internalTrafficPolicy,omitempty" protobuf:"bytes,22,opt,name=internalTrafficPolicy"` +} diff --git a/modules/common/service/zz_generated.deepcopy.go b/modules/common/service/zz_generated.deepcopy.go new file mode 100644 index 00000000..a0e4de0c --- /dev/null +++ b/modules/common/service/zz_generated.deepcopy.go @@ -0,0 +1,141 @@ +//go:build !ignore_autogenerated +// +build !ignore_autogenerated + +/* + + +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. +*/ + +// Code generated by controller-gen. DO NOT EDIT. + +package service + +import ( + "k8s.io/api/core/v1" +) + +// DeepCopyInto is an autogenerated deepcopy function, copying the receiver, writing into out. in must be non-nil. +func (in *EmbeddedLabelsAnnotations) DeepCopyInto(out *EmbeddedLabelsAnnotations) { + *out = *in + if in.Labels != nil { + in, out := &in.Labels, &out.Labels + *out = make(map[string]string, len(*in)) + for key, val := range *in { + (*out)[key] = val + } + } + if in.Annotations != nil { + in, out := &in.Annotations, &out.Annotations + *out = make(map[string]string, len(*in)) + for key, val := range *in { + (*out)[key] = val + } + } +} + +// DeepCopy is an autogenerated deepcopy function, copying the receiver, creating a new EmbeddedLabelsAnnotations. +func (in *EmbeddedLabelsAnnotations) DeepCopy() *EmbeddedLabelsAnnotations { + if in == nil { + return nil + } + out := new(EmbeddedLabelsAnnotations) + in.DeepCopyInto(out) + return out +} + +// DeepCopyInto is an autogenerated deepcopy function, copying the receiver, writing into out. in must be non-nil. +func (in *OverrideServiceSpec) DeepCopyInto(out *OverrideServiceSpec) { + *out = *in + if in.LoadBalancerSourceRanges != nil { + in, out := &in.LoadBalancerSourceRanges, &out.LoadBalancerSourceRanges + *out = make([]string, len(*in)) + copy(*out, *in) + } + if in.SessionAffinityConfig != nil { + in, out := &in.SessionAffinityConfig, &out.SessionAffinityConfig + *out = new(v1.SessionAffinityConfig) + (*in).DeepCopyInto(*out) + } + if in.IPFamilyPolicy != nil { + in, out := &in.IPFamilyPolicy, &out.IPFamilyPolicy + *out = new(v1.IPFamilyPolicy) + **out = **in + } + if in.LoadBalancerClass != nil { + in, out := &in.LoadBalancerClass, &out.LoadBalancerClass + *out = new(string) + **out = **in + } + if in.InternalTrafficPolicy != nil { + in, out := &in.InternalTrafficPolicy, &out.InternalTrafficPolicy + *out = new(v1.ServiceInternalTrafficPolicyType) + **out = **in + } +} + +// DeepCopy is an autogenerated deepcopy function, copying the receiver, creating a new OverrideServiceSpec. +func (in *OverrideServiceSpec) DeepCopy() *OverrideServiceSpec { + if in == nil { + return nil + } + out := new(OverrideServiceSpec) + in.DeepCopyInto(out) + return out +} + +// DeepCopyInto is an autogenerated deepcopy function, copying the receiver, writing into out. in must be non-nil. +func (in *OverrideSpec) DeepCopyInto(out *OverrideSpec) { + *out = *in + if in.EmbeddedLabelsAnnotations != nil { + in, out := &in.EmbeddedLabelsAnnotations, &out.EmbeddedLabelsAnnotations + *out = new(EmbeddedLabelsAnnotations) + (*in).DeepCopyInto(*out) + } + if in.Spec != nil { + in, out := &in.Spec, &out.Spec + *out = new(OverrideServiceSpec) + (*in).DeepCopyInto(*out) + } +} + +// DeepCopy is an autogenerated deepcopy function, copying the receiver, creating a new OverrideSpec. +func (in *OverrideSpec) DeepCopy() *OverrideSpec { + if in == nil { + return nil + } + out := new(OverrideSpec) + in.DeepCopyInto(out) + return out +} + +// DeepCopyInto is an autogenerated deepcopy function, copying the receiver, writing into out. in must be non-nil. +func (in *RoutedOverrideSpec) DeepCopyInto(out *RoutedOverrideSpec) { + *out = *in + in.OverrideSpec.DeepCopyInto(&out.OverrideSpec) + if in.EndpointURL != nil { + in, out := &in.EndpointURL, &out.EndpointURL + *out = new(string) + **out = **in + } +} + +// DeepCopy is an autogenerated deepcopy function, copying the receiver, creating a new RoutedOverrideSpec. +func (in *RoutedOverrideSpec) DeepCopy() *RoutedOverrideSpec { + if in == nil { + return nil + } + out := new(RoutedOverrideSpec) + in.DeepCopyInto(out) + return out +} diff --git a/modules/common/test/functional/service_test.go b/modules/common/test/functional/service_test.go new file mode 100644 index 00000000..adb3a080 --- /dev/null +++ b/modules/common/test/functional/service_test.go @@ -0,0 +1,172 @@ +/* +Copyright 2023 Red Hat + +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 functional + +import ( + "github.com/google/uuid" + . "github.com/onsi/ginkgo/v2" + . "github.com/onsi/gomega" + "github.com/openstack-k8s-operators/lib-common/modules/common/service" + "k8s.io/apimachinery/pkg/types" + "k8s.io/apimachinery/pkg/util/intstr" + + corev1 "k8s.io/api/core/v1" + metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" +) + +func getExampleService(namespace string) *corev1.Service { + return &corev1.Service{ + ObjectMeta: metav1.ObjectMeta{ + Name: "test-svc", + Namespace: namespace, + Labels: map[string]string{ + "label": "a", + "replace": "a", + }, + Annotations: map[string]string{ + "anno": "a", + "replace": "a", + }, + }, + Spec: corev1.ServiceSpec{ + Type: corev1.ServiceTypeClusterIP, + Ports: []corev1.ServicePort{ + { + Name: "test-port", + Port: int32(80), + Protocol: corev1.ProtocolTCP, + TargetPort: intstr.FromInt(8080), + }, + }, + Selector: map[string]string{ + "internal": "true", + "service": "foo", + }, + }, + } +} + +var _ = Describe("service package", func() { + var namespace string + + BeforeEach(func() { + // NOTE(gibi): We need to create a unique namespace for each test run + // as namespaces cannot be deleted in a locally running envtest. See + // https://book.kubebuilder.io/reference/envtest.html#namespace-usage-limitation + namespace = uuid.New().String() + th.CreateNamespace(namespace) + // We still request the delete of the Namespace to properly cleanup if + // we run the test in an existing cluster. + DeferCleanup(th.DeleteNamespace, namespace) + + }) + + It("creates service with defaults", func() { + s, err := service.NewService( + getExampleService(namespace), + map[string]string{}, + timeout, + &service.OverrideSpec{}, + ) + Expect(err).ShouldNot(HaveOccurred()) + + _, err = s.CreateOrPatch(ctx, h) + Expect(err).ShouldNot(HaveOccurred()) + svc := th.AssertServiceExists(types.NamespacedName{Namespace: namespace, Name: "test-svc"}) + Expect(svc.Annotations["anno"]).To(Equal("a")) + Expect(svc.Annotations["replace"]).To(Equal("a")) + Expect(svc.Labels["label"]).To(Equal("a")) + Expect(svc.Labels["replace"]).To(Equal("a")) + Expect(svc.Spec.Type).To(Equal(corev1.ServiceTypeClusterIP)) + Expect(svc.Spec.Ports[0].Name).To(Equal("test-port")) + Expect(svc.Spec.Ports[0].Port).To(Equal(int32(80))) + Expect(svc.Spec.Ports[0].Protocol).To(Equal(corev1.ProtocolTCP)) + Expect(svc.Spec.Ports[0].TargetPort.IntVal).To(Equal(int32(8080))) + }) + + It("merges labels to the service", func() { + s, err := service.NewService( + getExampleService(namespace), + map[string]string{}, + timeout, + &service.OverrideSpec{ + EmbeddedLabelsAnnotations: &service.EmbeddedLabelsAnnotations{ + Labels: map[string]string{ + "foo": "b", + "replace": "b", + }, + }, + }, + ) + Expect(err).ShouldNot(HaveOccurred()) + + _, err = s.CreateOrPatch(ctx, h) + Expect(err).ShouldNot(HaveOccurred()) + rv1 := th.AssertServiceExists(types.NamespacedName{Namespace: namespace, Name: "test-svc"}) + // non overridden label exists + Expect(rv1.Labels["label"]).To(Equal("a")) + // adds new label + Expect(rv1.Labels["foo"]).To(Equal("b")) + // override replaces existing label + Expect(rv1.Labels["replace"]).To(Equal("b")) + }) + + It("merges annotations to the service", func() { + s, err := service.NewService( + getExampleService(namespace), + map[string]string{}, + timeout, + &service.OverrideSpec{ + EmbeddedLabelsAnnotations: &service.EmbeddedLabelsAnnotations{ + Annotations: map[string]string{ + "foo": "b", + "replace": "b", + }, + }, + }, + ) + Expect(err).ShouldNot(HaveOccurred()) + + _, err = s.CreateOrPatch(ctx, h) + Expect(err).ShouldNot(HaveOccurred()) + rv1 := th.AssertServiceExists(types.NamespacedName{Namespace: namespace, Name: "test-svc"}) + // non overridden annotation exists + Expect(rv1.Annotations["anno"]).To(Equal("a")) + // adds new annotation + Expect(rv1.Annotations["foo"]).To(Equal("b")) + // override replaces existing annotation + Expect(rv1.Annotations["replace"]).To(Equal("b")) + }) + + It("overrides spec.Type to LoadBalancer", func() { + s, err := service.NewService( + getExampleService(namespace), + map[string]string{}, + timeout, + &service.OverrideSpec{ + Spec: &service.OverrideServiceSpec{ + Type: corev1.ServiceTypeLoadBalancer, + }, + }, + ) + Expect(err).ShouldNot(HaveOccurred()) + + _, err = s.CreateOrPatch(ctx, h) + Expect(err).ShouldNot(HaveOccurred()) + svc := th.AssertServiceExists(types.NamespacedName{Namespace: namespace, Name: "test-svc"}) + Expect(svc.Spec.Type).To(Equal(corev1.ServiceTypeLoadBalancer)) + }) +}) diff --git a/modules/openstack/go.mod b/modules/openstack/go.mod index 1d9c6bfd..01761a09 100644 --- a/modules/openstack/go.mod +++ b/modules/openstack/go.mod @@ -33,7 +33,6 @@ require ( github.com/modern-go/concurrent v0.0.0-20180306012644-bacd9c7ef1dd // indirect github.com/modern-go/reflect2 v1.0.2 // indirect github.com/munnerz/goautoneg v0.0.0-20191010083416-a7dc8b61c822 // indirect - github.com/openshift/api v3.9.0+incompatible // indirect github.com/pkg/errors v0.9.1 // indirect github.com/prometheus/client_golang v1.14.0 // indirect github.com/prometheus/client_model v0.3.0 // indirect diff --git a/modules/openstack/go.sum b/modules/openstack/go.sum index 5fbf173c..866e70b7 100644 --- a/modules/openstack/go.sum +++ b/modules/openstack/go.sum @@ -222,8 +222,6 @@ github.com/mwitkow/go-conntrack v0.0.0-20161129095857-cc309e4a2223/go.mod h1:qRW github.com/mwitkow/go-conntrack v0.0.0-20190716064945-2f068394615f/go.mod h1:qRWi+5nqEBWmkhHvq77mSJWrCKwh8bxhgT7d/eI7P4U= github.com/onsi/ginkgo/v2 v2.12.0 h1:UIVDowFPwpg6yMUpPjGkYvf06K3RAiJXUhCxEwQVHRI= github.com/onsi/gomega v1.27.10 h1:naR28SdDFlqrG6kScpT8VWpu1xWY5nJRCF3XaYyBjhI= -github.com/openshift/api v0.0.0-20230414143018-3367bc7e6ac7 h1:rncLxJBpFGqBztyxCMwNRnMjhhIDOWHJowi6q8G6koI= -github.com/openshift/api v0.0.0-20230414143018-3367bc7e6ac7/go.mod h1:ctXNyWanKEjGj8sss1KjjHQ3ENKFm33FFnS5BKaIPh4= github.com/pkg/errors v0.8.0/go.mod h1:bwawxfHBFNV+L2hUp1rHADufV3IMtnDRdf1r5NINEl0= github.com/pkg/errors v0.8.1/go.mod h1:bwawxfHBFNV+L2hUp1rHADufV3IMtnDRdf1r5NINEl0= github.com/pkg/errors v0.9.1 h1:FEBLx1zS214owpjy7qsBeixbURkuhQAwrK5UwLGTwt4= diff --git a/modules/openstack/openstack.go b/modules/openstack/openstack.go index 56ac2ade..b392f6ed 100644 --- a/modules/openstack/openstack.go +++ b/modules/openstack/openstack.go @@ -22,7 +22,7 @@ import ( "github.com/go-logr/logr" gophercloud "github.com/gophercloud/gophercloud" "github.com/gophercloud/gophercloud/openstack" - endpoint "github.com/openstack-k8s-operators/lib-common/modules/common/endpoint" + service "github.com/openstack-k8s-operators/lib-common/modules/common/service" ) // OpenStack - @@ -99,11 +99,11 @@ func GetAvailability( endpointInterface string, ) (gophercloud.Availability, error) { var availability gophercloud.Availability - if endpointInterface == string(endpoint.EndpointAdmin) { + if endpointInterface == string(service.EndpointAdmin) { availability = gophercloud.AvailabilityAdmin - } else if endpointInterface == string(endpoint.EndpointInternal) { + } else if endpointInterface == string(service.EndpointInternal) { availability = gophercloud.AvailabilityInternal - } else if endpointInterface == string(endpoint.EndpointPublic) { + } else if endpointInterface == string(service.EndpointPublic) { availability = gophercloud.AvailabilityPublic } else { return availability, fmt.Errorf("endpoint interface %s not known", endpointInterface) diff --git a/modules/test/go.mod b/modules/test/go.mod index 8067f32c..a1a06546 100644 --- a/modules/test/go.mod +++ b/modules/test/go.mod @@ -10,7 +10,7 @@ require ( github.com/onsi/gomega v1.27.10 github.com/openstack-k8s-operators/infra-operator/apis v0.1.1-0.20230914145253-116f307c7875 github.com/openstack-k8s-operators/keystone-operator/api v0.1.1-0.20230914163026-da9aa9de960a - github.com/openstack-k8s-operators/lib-common/modules/common v0.1.1-0.20230824094610-976b18ca2875 + github.com/openstack-k8s-operators/lib-common/modules/common v0.1.1-0.20230913075424-2680ce4b6ad2 github.com/openstack-k8s-operators/mariadb-operator/api v0.1.1-0.20230913081601-9e4fc8aadad5 golang.org/x/exp v0.0.0-20230905200255-921286631fa9 golang.org/x/mod v0.12.0 @@ -81,6 +81,8 @@ require ( replace github.com/openstack-k8s-operators/lib-common/modules/common => ../common +replace github.com/openstack-k8s-operators/lib-common/modules/openstack => ../openstack + // mschuppert: map to latest commit from release-4.13 tag // must consistent within modules and service operators replace github.com/openshift/api => github.com/openshift/api v0.0.0-20230414143018-3367bc7e6ac7 diff --git a/modules/test/go.sum b/modules/test/go.sum index 3ccde443..1435ca3a 100644 --- a/modules/test/go.sum +++ b/modules/test/go.sum @@ -234,8 +234,6 @@ github.com/openstack-k8s-operators/infra-operator/apis v0.1.1-0.20230914145253-1 github.com/openstack-k8s-operators/infra-operator/apis v0.1.1-0.20230914145253-116f307c7875/go.mod h1:NgrvT3CKMu6fE8Nt1H79qHx11L3I7Bb2eItniM7c9ow= github.com/openstack-k8s-operators/keystone-operator/api v0.1.1-0.20230914163026-da9aa9de960a h1:MFYwi2Xk9r3OMPToCSbvqYVNrm7P+aFzGDN0eVNpgu8= github.com/openstack-k8s-operators/keystone-operator/api v0.1.1-0.20230914163026-da9aa9de960a/go.mod h1:nxrbUOIGMJ1h2pNlawhURdt/XJ95dW2wZGedmOVo2aw= -github.com/openstack-k8s-operators/lib-common/modules/openstack v0.1.1-0.20230824094610-976b18ca2875 h1:aUlwELsLYWQ3FL+/nRG/1uGVNW86c3MhtLrHNVDd57k= -github.com/openstack-k8s-operators/lib-common/modules/openstack v0.1.1-0.20230824094610-976b18ca2875/go.mod h1:Vng+vqdTJUuZ+AEzSAaU0I7bn3qwYMMFEUHHhiH0440= github.com/openstack-k8s-operators/mariadb-operator/api v0.1.1-0.20230913081601-9e4fc8aadad5 h1:dQcSQuXfgzgOhc4v+zD0jE6WWhn6FHr5nALOjJBPxyI= github.com/openstack-k8s-operators/mariadb-operator/api v0.1.1-0.20230913081601-9e4fc8aadad5/go.mod h1:mJyhm/YiQZaYhLvOuLng/ITpwx8HvsYVht+VotS1Ed8= github.com/pkg/errors v0.8.0/go.mod h1:bwawxfHBFNV+L2hUp1rHADufV3IMtnDRdf1r5NINEl0= From 0453ee80d80425aa3d74c56a656f77fd58a4aa76 Mon Sep 17 00:00:00 2001 From: Martin Schuppert Date: Mon, 31 Jul 2023 08:33:13 +0200 Subject: [PATCH 2/9] remove not used label parameter in NewService and NewRoute --- modules/common/endpoint/endpoint.go | 3 --- modules/common/route/route.go | 1 - modules/common/service/service.go | 1 - modules/common/test/functional/route_test.go | 10 ---------- modules/common/test/functional/service_test.go | 4 ---- 5 files changed, 19 deletions(-) diff --git a/modules/common/endpoint/endpoint.go b/modules/common/endpoint/endpoint.go index 6497d00a..454df0ad 100644 --- a/modules/common/endpoint/endpoint.go +++ b/modules/common/endpoint/endpoint.go @@ -117,7 +117,6 @@ func ExposeEndpoints( Protocol: protocol, }, }), - exportLabels, timeout, &service.OverrideSpec{}, ) @@ -163,7 +162,6 @@ func ExposeEndpoints( Port: data.Port, Protocol: corev1.ProtocolTCP, }}), - exportLabels, 5, &service.OverrideSpec{}, ) @@ -193,7 +191,6 @@ func ExposeEndpoints( ServiceName: endpointName, TargetPortName: endpointName, }), - exportLabels, timeout, data.RouteOverride, ) diff --git a/modules/common/route/route.go b/modules/common/route/route.go index c6ba5d12..347f0dff 100644 --- a/modules/common/route/route.go +++ b/modules/common/route/route.go @@ -37,7 +37,6 @@ import ( // NewRoute returns an initialized Route. func NewRoute( route *routev1.Route, - labels map[string]string, timeout time.Duration, override *OverrideSpec, ) (*Route, error) { diff --git a/modules/common/service/service.go b/modules/common/service/service.go index ba92e79b..ca4a14fd 100644 --- a/modules/common/service/service.go +++ b/modules/common/service/service.go @@ -39,7 +39,6 @@ import ( // NewService returns an initialized Service. func NewService( service *corev1.Service, - labels map[string]string, timeout time.Duration, override *OverrideSpec, ) (*Service, error) { diff --git a/modules/common/test/functional/route_test.go b/modules/common/test/functional/route_test.go index 8a2fd921..7020d661 100644 --- a/modules/common/test/functional/route_test.go +++ b/modules/common/test/functional/route_test.go @@ -74,7 +74,6 @@ var _ = Describe("route package", func() { It("creates route with defaults", func() { r, err := route.NewRoute( getExampleRoute(namespace), - map[string]string{}, timeout, &route.OverrideSpec{}, ) @@ -97,7 +96,6 @@ var _ = Describe("route package", func() { It("merges labels to the route", func() { r, err := route.NewRoute( getExampleRoute(namespace), - map[string]string{}, timeout, &route.OverrideSpec{ EmbeddedLabelsAnnotations: &route.EmbeddedLabelsAnnotations{ @@ -124,7 +122,6 @@ var _ = Describe("route package", func() { It("merges annotations to the route", func() { r, err := route.NewRoute( getExampleRoute(namespace), - map[string]string{}, timeout, &route.OverrideSpec{ EmbeddedLabelsAnnotations: &route.EmbeddedLabelsAnnotations{ @@ -151,7 +148,6 @@ var _ = Describe("route package", func() { It("overrides spec.host if specified", func() { r, err := route.NewRoute( getExampleRoute(namespace), - map[string]string{}, timeout, &route.OverrideSpec{ Spec: &route.Spec{ @@ -170,7 +166,6 @@ var _ = Describe("route package", func() { It("overrides spec.subdomain if specified", func() { r, err := route.NewRoute( getExampleRoute(namespace), - map[string]string{}, timeout, &route.OverrideSpec{ Spec: &route.Spec{ @@ -189,7 +184,6 @@ var _ = Describe("route package", func() { It("overrides spec.path if specified", func() { r, err := route.NewRoute( getExampleRoute(namespace), - map[string]string{}, timeout, &route.OverrideSpec{ Spec: &route.Spec{ @@ -208,7 +202,6 @@ var _ = Describe("route package", func() { It("overrides spec.to if specified", func() { r, err := route.NewRoute( getExampleRoute(namespace), - map[string]string{}, timeout, &route.OverrideSpec{ Spec: &route.Spec{ @@ -232,7 +225,6 @@ var _ = Describe("route package", func() { It("overrides spec.alternateBackends if specified", func() { r, err := route.NewRoute( getExampleRoute(namespace), - map[string]string{}, timeout, &route.OverrideSpec{ Spec: &route.Spec{ @@ -258,7 +250,6 @@ var _ = Describe("route package", func() { It("overrides spec.port if specified", func() { r, err := route.NewRoute( getExampleRoute(namespace), - map[string]string{}, timeout, &route.OverrideSpec{ Spec: &route.Spec{ @@ -279,7 +270,6 @@ var _ = Describe("route package", func() { It("overrides spec.tls if specified", func() { r, err := route.NewRoute( getExampleRoute(namespace), - map[string]string{}, timeout, &route.OverrideSpec{ Spec: &route.Spec{ diff --git a/modules/common/test/functional/service_test.go b/modules/common/test/functional/service_test.go index adb3a080..adb9b1dd 100644 --- a/modules/common/test/functional/service_test.go +++ b/modules/common/test/functional/service_test.go @@ -77,7 +77,6 @@ var _ = Describe("service package", func() { It("creates service with defaults", func() { s, err := service.NewService( getExampleService(namespace), - map[string]string{}, timeout, &service.OverrideSpec{}, ) @@ -100,7 +99,6 @@ var _ = Describe("service package", func() { It("merges labels to the service", func() { s, err := service.NewService( getExampleService(namespace), - map[string]string{}, timeout, &service.OverrideSpec{ EmbeddedLabelsAnnotations: &service.EmbeddedLabelsAnnotations{ @@ -127,7 +125,6 @@ var _ = Describe("service package", func() { It("merges annotations to the service", func() { s, err := service.NewService( getExampleService(namespace), - map[string]string{}, timeout, &service.OverrideSpec{ EmbeddedLabelsAnnotations: &service.EmbeddedLabelsAnnotations{ @@ -154,7 +151,6 @@ var _ = Describe("service package", func() { It("overrides spec.Type to LoadBalancer", func() { s, err := service.NewService( getExampleService(namespace), - map[string]string{}, timeout, &service.OverrideSpec{ Spec: &service.OverrideServiceSpec{ From 4c3c5e518aebaab85d0c9a86989f53d82c7b2cc3 Mon Sep 17 00:00:00 2001 From: Martin Schuppert Date: Wed, 26 Jul 2023 18:19:40 +0200 Subject: [PATCH 3/9] [TLS] handle endpoint protocol Adds Protocol to endpoint Data struct with funcs to get the protocol based on the type. If the type is nil, http is considered to match the current behavior. Also * adds Method to return the api endpoint URL and fix GetServiceHostnamePort * if service port is nil GetServiceHostnamePort return just hostname Jira: OSP-26845 --- modules/common/endpoint/endpoint.go | 19 ++++- modules/common/service/service.go | 59 +++++++++++++- modules/common/service/types.go | 9 +++ .../common/test/functional/service_test.go | 80 +++++++++++++++++-- 4 files changed, 153 insertions(+), 14 deletions(-) diff --git a/modules/common/endpoint/endpoint.go b/modules/common/endpoint/endpoint.go index 454df0ad..4f513346 100644 --- a/modules/common/endpoint/endpoint.go +++ b/modules/common/endpoint/endpoint.go @@ -18,6 +18,7 @@ package endpoint import ( "context" + "fmt" "net/url" "strings" "time" @@ -50,13 +51,21 @@ type Data struct { Port int32 // An optional path suffix to append to route hostname when forming Keystone endpoint URLs Path string + // protocol of the endpoint (http/https/none) + Protocol *service.Protocol // details for metallb service generation + // NOTE: (mschuppert) deprecated, can be removed when external endpoint creation moved to openstack-operator + // and ExposeEndpoints() can be removed MetalLB *MetalLBData // possible overrides for Route + // NOTE: (mschuppert) deprecated, can be removed when external endpoint creation moved to openstack-operator + // and ExposeEndpoints() can be removed RouteOverride *route.OverrideSpec } // MetalLBData - information specific to creating the MetalLB service +// NOTE: (mschuppert) deprecated, can be removed when external endpoint creation moved to openstack-operator +// and ExposeEndpoints() can be removed type MetalLBData struct { // Name of the metallb IpAddressPool IPAddressPool string @@ -73,6 +82,8 @@ type MetalLBData struct { } // ExposeEndpoints - creates services, routes and returns a map of created openstack endpoint +// NOTE: (mschuppert) deprecated, can be removed when external endpoint creation moved to openstack-operator +// and ExposeEndpoints() can be removed func ExposeEndpoints( ctx context.Context, h *helper.Helper, @@ -95,6 +106,7 @@ func ExposeEndpoints( // Create metallb service if specified, otherwise create a route var hostname string + var port string if data.MetalLB != nil { var protocol corev1.Protocol if data.MetalLB.Protocol != nil { @@ -147,7 +159,7 @@ func ExposeEndpoints( } // create service - end - hostname = svc.GetServiceHostnamePort() + hostname, port = svc.GetServiceHostnamePort() } else { // Create the service @@ -177,7 +189,7 @@ func ExposeEndpoints( } // create service - end - hostname = svc.GetServiceHostnamePort() + hostname, port = svc.GetServiceHostnamePort() // Create the route if it is public endpoint if endpointType == service.EndpointPublic { @@ -222,7 +234,8 @@ func ExposeEndpoints( // Do not include data.Path in parsing check because %(project_id)s // is invalid without being encoded, but they should not be encoded in the actual endpoint - apiEndpoint, err := url.Parse(protocol + hostname) + endptURL := fmt.Sprintf("%s://%s:%s", protocol, hostname, port) + apiEndpoint, err := url.Parse(endptURL) if err != nil { return endpointMap, ctrl.Result{}, err } diff --git a/modules/common/service/service.go b/modules/common/service/service.go index ca4a14fd..9605b227 100644 --- a/modules/common/service/service.go +++ b/modules/common/service/service.go @@ -20,6 +20,8 @@ import ( "context" "encoding/json" "fmt" + "net/url" + "strconv" "time" corev1 "k8s.io/api/core/v1" @@ -106,9 +108,16 @@ func (s *Service) GetServiceHostname() string { return s.serviceHostname } -// GetServiceHostnamePort - returns the service hostname with port -func (s *Service) GetServiceHostnamePort() string { - return fmt.Sprintf("%s:%d", s.GetServiceHostname(), GetServicesPortDetails(s.service, s.service.Name).Port) +// GetServiceHostnamePort - returns the service hostname with port if service port +// is not nil, otherwise returns GetServiceHostname() +func (s *Service) GetServiceHostnamePort() (string, string) { + servicePort := GetServicesPortDetails(s.service, s.service.Name) + if servicePort != nil { + return s.GetServiceHostname(), + strconv.FormatInt(int64(servicePort.Port), 10) + } + + return s.GetServiceHostname(), "" } // GetLabels - returns labels of the service @@ -137,6 +146,38 @@ func (s *Service) AddAnnotation(anno map[string]string) { s.service.Annotations = util.MergeStringMaps(s.service.Annotations, anno) } +// GetAPIEndpoint - returns the API endpoint URL for the service to register in keystone. +func (s *Service) GetAPIEndpoint(svcOverride *OverrideSpec, protocol *Protocol, path string) (string, error) { + var apiEndpoint *url.URL + var err error + if svcOverride != nil && svcOverride.EndpointURL != nil { + apiEndpoint, err = url.Parse(*svcOverride.EndpointURL) + if err != nil { + return "", err + } + } else { + hostname, port := s.GetServiceHostnamePort() + + var endptURL string + if protocol != nil && + ((*protocol == ProtocolHTTP && port == "80") || + (*protocol == ProtocolHTTPS && port == "443")) { + endptURL = fmt.Sprintf("%s%s", EndptProtocol(protocol), hostname) + } else { + endptURL = fmt.Sprintf("%s%s:%s", EndptProtocol(protocol), hostname, port) + } + + // Do not include the path in parsing check because %(project_id)s + // is invalid without being encoded, but they should not be encoded in the actual endpoint + apiEndpoint, err = url.Parse(endptURL) + if err != nil { + return "", err + } + } + + return apiEndpoint.String() + path, nil +} + // GenericService func func GenericService(svcInfo *GenericServiceDetails) *corev1.Service { ports := svcInfo.Ports @@ -353,3 +394,15 @@ func GetServicesPortDetails( return nil } + +// EndptProtocol returns the protocol for the endpoint if proto is nil http is considered +func EndptProtocol(proto *Protocol) string { + if proto == nil { + return string(ProtocolHTTP) + "://" + } + if *proto == ProtocolNone { + return "" + } + + return string(*proto) + "://" +} diff --git a/modules/common/service/types.go b/modules/common/service/types.go index fca3e659..a33b7b67 100644 --- a/modules/common/service/types.go +++ b/modules/common/service/types.go @@ -30,6 +30,9 @@ import ( // point as we have circular dep in test module to keystone. type Endpoint string +// Protocol of the endpoint (http/https) +type Protocol string + const ( // EndpointAdmin - admin endpoint EndpointAdmin Endpoint = "admin" @@ -39,6 +42,12 @@ const ( EndpointPublic Endpoint = "public" // AnnotationHostnameKey - AnnotationHostnameKey = "dnsmasq.network.openstack.org/hostname" + // ProtocolHTTP - + ProtocolHTTP Protocol = "http" + // ProtocolHTTPS - + ProtocolHTTPS Protocol = "https" + // ProtocolNone - + ProtocolNone Protocol = "" ) // Service - diff --git a/modules/common/test/functional/service_test.go b/modules/common/test/functional/service_test.go index adb9b1dd..1c18f9a1 100644 --- a/modules/common/test/functional/service_test.go +++ b/modules/common/test/functional/service_test.go @@ -16,6 +16,8 @@ limitations under the License. package functional import ( + "fmt" + "github.com/google/uuid" . "github.com/onsi/ginkgo/v2" . "github.com/onsi/gomega" @@ -27,7 +29,7 @@ import ( metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" ) -func getExampleService(namespace string) *corev1.Service { +func getExampleService(namespace string, port int32) *corev1.Service { return &corev1.Service{ ObjectMeta: metav1.ObjectMeta{ Name: "test-svc", @@ -45,8 +47,8 @@ func getExampleService(namespace string) *corev1.Service { Type: corev1.ServiceTypeClusterIP, Ports: []corev1.ServicePort{ { - Name: "test-port", - Port: int32(80), + Name: "test-svc", + Port: port, Protocol: corev1.ProtocolTCP, TargetPort: intstr.FromInt(8080), }, @@ -76,12 +78,15 @@ var _ = Describe("service package", func() { It("creates service with defaults", func() { s, err := service.NewService( - getExampleService(namespace), + getExampleService(namespace, int32(80)), timeout, &service.OverrideSpec{}, ) Expect(err).ShouldNot(HaveOccurred()) + // AddAnnotations() + s.AddAnnotation(map[string]string{"add": "bar"}) + _, err = s.CreateOrPatch(ctx, h) Expect(err).ShouldNot(HaveOccurred()) svc := th.AssertServiceExists(types.NamespacedName{Namespace: namespace, Name: "test-svc"}) @@ -90,15 +95,59 @@ var _ = Describe("service package", func() { Expect(svc.Labels["label"]).To(Equal("a")) Expect(svc.Labels["replace"]).To(Equal("a")) Expect(svc.Spec.Type).To(Equal(corev1.ServiceTypeClusterIP)) - Expect(svc.Spec.Ports[0].Name).To(Equal("test-port")) + Expect(svc.Spec.Ports[0].Name).To(Equal("test-svc")) Expect(svc.Spec.Ports[0].Port).To(Equal(int32(80))) Expect(svc.Spec.Ports[0].Protocol).To(Equal(corev1.ProtocolTCP)) Expect(svc.Spec.Ports[0].TargetPort.IntVal).To(Equal(int32(8080))) + + // Test Getters + Expect(s.GetAnnotations()).To(Equal(map[string]string{ + "anno": "a", + "replace": "a", + "add": "bar", + })) + Expect(s.GetLabels()).To(Equal(map[string]string{ + "label": "a", + "replace": "a", + })) + Expect(s.GetServiceType()).To(Equal(corev1.ServiceTypeClusterIP)) + Expect(s.GetServiceHostname()).To(Equal(fmt.Sprintf("test-svc.%s.svc", namespace))) + hostname, port := s.GetServiceHostnamePort() + Expect(hostname).To(Equal(fmt.Sprintf("test-svc.%s.svc", namespace))) + Expect(port).To(Equal("80")) + // HTTP endpoint with no port + endpointURL, err := s.GetAPIEndpoint(nil, service.PtrProtocol(service.ProtocolHTTP), "") + Expect(err).ShouldNot(HaveOccurred()) + Expect(endpointURL).To(Equal(fmt.Sprintf("http://test-svc.%s.svc", namespace))) + + // GetServiceWithName() + svc, err = service.GetServiceWithName(th.Ctx, h, "test-svc", namespace) + Expect(err).ShouldNot(HaveOccurred()) + + // GetServicesListWithLabel() + svcList, err := service.GetServicesListWithLabel(th.Ctx, h, namespace, map[string]string{ + "label": "a", + "replace": "a", + }) + Expect(err).ShouldNot(HaveOccurred()) + Expect(svcList.Items).Should(HaveLen(1)) + + // GetServicesPortDetails() + servicePortDetails := service.GetServicesPortDetails(svc, "test-svc") + Expect(servicePortDetails).NotTo(BeNil()) + Expect(servicePortDetails.Port).To(Equal(int32(80))) + Expect(servicePortDetails.TargetPort.IntVal).To(Equal(int32(8080))) + + // Delete method + err = s.Delete(ctx, h) + Expect(err).ShouldNot(HaveOccurred()) + th.AssertServiceDoesNotExist(types.NamespacedName{Namespace: namespace, Name: "test-svc"}) + }) It("merges labels to the service", func() { s, err := service.NewService( - getExampleService(namespace), + getExampleService(namespace, int32(5000)), timeout, &service.OverrideSpec{ EmbeddedLabelsAnnotations: &service.EmbeddedLabelsAnnotations{ @@ -120,11 +169,16 @@ var _ = Describe("service package", func() { Expect(rv1.Labels["foo"]).To(Equal("b")) // override replaces existing label Expect(rv1.Labels["replace"]).To(Equal("b")) + + // HTTPS endpoint with port + endpointURL, err := s.GetAPIEndpoint(nil, service.PtrProtocol(service.ProtocolHTTPS), "") + Expect(err).ShouldNot(HaveOccurred()) + Expect(endpointURL).To(Equal(fmt.Sprintf("https://test-svc.%s.svc:5000", namespace))) }) It("merges annotations to the service", func() { s, err := service.NewService( - getExampleService(namespace), + getExampleService(namespace, int32(443)), timeout, &service.OverrideSpec{ EmbeddedLabelsAnnotations: &service.EmbeddedLabelsAnnotations{ @@ -146,11 +200,16 @@ var _ = Describe("service package", func() { Expect(rv1.Annotations["foo"]).To(Equal("b")) // override replaces existing annotation Expect(rv1.Annotations["replace"]).To(Equal("b")) + + // HTTPS endpoint with no port + endpointURL, err := s.GetAPIEndpoint(nil, service.PtrProtocol(service.ProtocolHTTPS), "") + Expect(err).ShouldNot(HaveOccurred()) + Expect(endpointURL).To(Equal(fmt.Sprintf("https://test-svc.%s.svc", namespace))) }) It("overrides spec.Type to LoadBalancer", func() { s, err := service.NewService( - getExampleService(namespace), + getExampleService(namespace, int32(80)), timeout, &service.OverrideSpec{ Spec: &service.OverrideServiceSpec{ @@ -164,5 +223,10 @@ var _ = Describe("service package", func() { Expect(err).ShouldNot(HaveOccurred()) svc := th.AssertServiceExists(types.NamespacedName{Namespace: namespace, Name: "test-svc"}) Expect(svc.Spec.Type).To(Equal(corev1.ServiceTypeLoadBalancer)) + + // NONE endpoint with port + endpointURL, err := s.GetAPIEndpoint(nil, service.PtrProtocol(service.ProtocolNone), "") + Expect(err).ShouldNot(HaveOccurred()) + Expect(endpointURL).To(Equal(fmt.Sprintf("test-svc.%s.svc:80", namespace))) }) }) From 5479d078974f07b836824f9e760f2eeec5c66503 Mon Sep 17 00:00:00 2001 From: Martin Schuppert Date: Thu, 3 Aug 2023 12:16:49 +0200 Subject: [PATCH 4/9] [route] add method to return route obj --- modules/common/route/route.go | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/modules/common/route/route.go b/modules/common/route/route.go index 347f0dff..f5ab5d17 100644 --- a/modules/common/route/route.go +++ b/modules/common/route/route.go @@ -88,6 +88,11 @@ func (r *Route) GetHostname() string { return r.hostname } +// GetRoute - returns the route object +func (r *Route) GetRoute() *routev1.Route { + return r.route +} + // GenericRoute func func GenericRoute(routeInfo *GenericRouteDetails) *routev1.Route { serviceRef := routev1.RouteTargetReference{ From c76eb7bb6b3ed5c7c2932b4e22f08967f3ac18ab Mon Sep 17 00:00:00 2001 From: Martin Schuppert Date: Wed, 9 Aug 2023 16:15:02 +0200 Subject: [PATCH 5/9] Add ToOverrideServiceSpec() method Adds methog to convert corev1.ServiceSpec to OverrideServiceSpec Also adds unit tests for service pkg --- modules/common/service/service.go | 26 +- modules/common/service/service_test.go | 426 ++++++++++++++++++ modules/common/service/types.go | 1 + .../common/test/functional/service_test.go | 9 +- 4 files changed, 455 insertions(+), 7 deletions(-) create mode 100644 modules/common/service/service_test.go diff --git a/modules/common/service/service.go b/modules/common/service/service.go index 9605b227..dbb54952 100644 --- a/modules/common/service/service.go +++ b/modules/common/service/service.go @@ -147,11 +147,11 @@ func (s *Service) AddAnnotation(anno map[string]string) { } // GetAPIEndpoint - returns the API endpoint URL for the service to register in keystone. -func (s *Service) GetAPIEndpoint(svcOverride *OverrideSpec, protocol *Protocol, path string) (string, error) { +func (s *Service) GetAPIEndpoint(endpointURL *string, protocol *Protocol, path string) (string, error) { var apiEndpoint *url.URL var err error - if svcOverride != nil && svcOverride.EndpointURL != nil { - apiEndpoint, err = url.Parse(*svcOverride.EndpointURL) + if endpointURL != nil { + apiEndpoint, err = url.Parse(*endpointURL) if err != nil { return "", err } @@ -178,6 +178,26 @@ func (s *Service) GetAPIEndpoint(svcOverride *OverrideSpec, protocol *Protocol, return apiEndpoint.String() + path, nil } +// ToOverrideServiceSpec - convert corev1.ServiceSpec to OverrideServiceSpec +func (s *Service) ToOverrideServiceSpec() (*OverrideServiceSpec, error) { + overrideServiceSpec := &OverrideServiceSpec{} + + serviceSpec := s.GetSpec() + if serviceSpec != nil { + serviceSpecBytes, err := json.Marshal(serviceSpec) + if err != nil { + return nil, fmt.Errorf("error marshalling Service Spec: %w", err) + } + + err = json.Unmarshal(serviceSpecBytes, overrideServiceSpec) + if err != nil { + return nil, fmt.Errorf("error unmarshalling service OverrideSpec: %w", err) + } + } + + return overrideServiceSpec, nil +} + // GenericService func func GenericService(svcInfo *GenericServiceDetails) *corev1.Service { ports := svcInfo.Ports diff --git a/modules/common/service/service_test.go b/modules/common/service/service_test.go new file mode 100644 index 00000000..6e68c736 --- /dev/null +++ b/modules/common/service/service_test.go @@ -0,0 +1,426 @@ +/* +Copyright 2022 Red Hat + +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. +*/ + +// +kubebuilder:object:generate:=true + +package service + +import ( + "testing" + "time" + + . "github.com/onsi/gomega" + corev1 "k8s.io/api/core/v1" + metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" + "k8s.io/apimachinery/pkg/util/intstr" + "k8s.io/utils/ptr" +) + +var ( + svcClusterIP = corev1.Service{ + ObjectMeta: metav1.ObjectMeta{ + Name: "foo", + Namespace: "namespace", + Labels: map[string]string{ + "foo": "bar", + }, + }, + Spec: corev1.ServiceSpec{ + Ports: []corev1.ServicePort{}, + Type: corev1.ServiceTypeClusterIP, + }, + } + svcLoadBalancer = corev1.Service{ + ObjectMeta: metav1.ObjectMeta{ + Name: "foo", + Namespace: "namespace", + Labels: map[string]string{ + "foo": "bar", + }}, + Spec: corev1.ServiceSpec{ + Ports: []corev1.ServicePort{}, + Type: corev1.ServiceTypeLoadBalancer, + }, + } + portHTTP = []corev1.ServicePort{ + { + Name: "foo", + Protocol: corev1.ProtocolTCP, + AppProtocol: nil, + Port: int32(80), + TargetPort: intstr.FromInt(0), + NodePort: 0, + }, + } + portHTTPS = []corev1.ServicePort{ + { + Name: "foo", + Protocol: corev1.ProtocolTCP, + AppProtocol: nil, + Port: int32(443), + TargetPort: intstr.FromInt(0), + NodePort: 0, + }, + } + portCustom = []corev1.ServicePort{ + { + Name: "foo", + Protocol: corev1.ProtocolTCP, + AppProtocol: nil, + Port: int32(8080), + TargetPort: intstr.FromInt(0), + NodePort: 0, + }, + } + timeout = time.Duration(5) * time.Second + override = OverrideSpec{ + Spec: &OverrideServiceSpec{ + Type: corev1.ServiceTypeLoadBalancer, + }, + } + overrideServiceSpecClusterIP = OverrideServiceSpec{ + Type: corev1.ServiceTypeClusterIP, + } + overrideServiceSpecLoadBalancer = OverrideServiceSpec{ + Type: corev1.ServiceTypeLoadBalancer, + } +) + +func TestGenericService(t *testing.T) { + tests := []struct { + name string + service GenericServiceDetails + want corev1.Service + }{ + { + name: "Service with port, no labels, selector", + service: GenericServiceDetails{ + Name: "foo", + Namespace: "namespace", + Labels: map[string]string{}, + Selector: map[string]string{}, + Ports: []corev1.ServicePort{ + { + Name: "port", + Port: int32(80), + Protocol: corev1.ProtocolTCP, + }, + }, + }, + want: corev1.Service{ + ObjectMeta: metav1.ObjectMeta{ + Name: "foo", + Namespace: "namespace", + Labels: map[string]string{}, + }, + Spec: corev1.ServiceSpec{ + Ports: []corev1.ServicePort{ + { + Name: "port", + Protocol: corev1.ProtocolTCP, + AppProtocol: nil, + Port: int32(80), + TargetPort: intstr.FromInt(0), + NodePort: 0, + }, + }, + Selector: map[string]string{}, + Type: corev1.ServiceTypeClusterIP, + }, + }, + }, + { + name: "Service with port, labels, selector", + service: GenericServiceDetails{ + Name: "foo", + Namespace: "namespace", + Labels: map[string]string{ + "foo": "bar", + }, + Selector: map[string]string{ + "foo": "bar", + }, + Ports: []corev1.ServicePort{ + { + Name: "port", + Port: int32(80), + Protocol: corev1.ProtocolTCP, + }, + }, + }, + want: corev1.Service{ + ObjectMeta: metav1.ObjectMeta{ + Name: "foo", + Namespace: "namespace", + Labels: map[string]string{ + "foo": "bar", + }, + }, + Spec: corev1.ServiceSpec{ + Ports: []corev1.ServicePort{ + { + Name: "port", + Protocol: corev1.ProtocolTCP, + AppProtocol: nil, + Port: int32(80), + TargetPort: intstr.FromInt(0), + NodePort: 0, + }, + }, + Selector: map[string]string{ + "foo": "bar", + }, + Type: corev1.ServiceTypeClusterIP, + }, + }, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + g := NewWithT(t) + + service := GenericService(&tt.service) + + g.Expect(*service).To(Equal(tt.want)) + }) + } +} + +func getServiceWithPort(svc corev1.Service, ports []corev1.ServicePort) *corev1.Service { + svc.Spec.Ports = ports + + return &svc +} + +func TestNewService(t *testing.T) { + tests := []struct { + name string + service *corev1.Service + override OverrideSpec + want Service + wantPort string + wantOverrideServiceSpec OverrideServiceSpec + }{ + { + name: "HTTP ClusterIP service no override", + service: getServiceWithPort(svcClusterIP, portHTTP), + override: OverrideSpec{}, + want: Service{ + service: getServiceWithPort(svcClusterIP, portHTTP), + timeout: timeout, + serviceHostname: "foo.namespace.svc", + }, + wantPort: "80", + wantOverrideServiceSpec: overrideServiceSpecClusterIP, + }, + { + name: "HTTPS ClusterIP service no override", + service: getServiceWithPort(svcClusterIP, portHTTPS), + override: OverrideSpec{}, + want: Service{ + service: getServiceWithPort(svcClusterIP, portHTTPS), + timeout: timeout, + serviceHostname: "foo.namespace.svc", + }, + wantPort: "443", + wantOverrideServiceSpec: overrideServiceSpecClusterIP, + }, + { + name: "None ClusterIP service no override", + service: getServiceWithPort(svcClusterIP, portCustom), + override: OverrideSpec{}, + want: Service{ + service: getServiceWithPort(svcClusterIP, portCustom), + timeout: timeout, + serviceHostname: "foo.namespace.svc", + }, + wantPort: "8080", + wantOverrideServiceSpec: overrideServiceSpecClusterIP, + }, + { + name: "HTTP ClusterIP service override service Type to LoadBalancer", + service: getServiceWithPort(svcClusterIP, portHTTP), + override: override, + want: Service{ + service: getServiceWithPort(svcLoadBalancer, portHTTP), + timeout: timeout, + serviceHostname: "foo.namespace.svc", + }, + wantPort: "80", + wantOverrideServiceSpec: overrideServiceSpecLoadBalancer, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + g := NewWithT(t) + + service, err := NewService(tt.service, timeout, &tt.override) + g.Expect(err).ToNot(HaveOccurred()) + // timeout + g.Expect(service.timeout).To(Equal(timeout)) + // GetServiceType + g.Expect(service.GetServiceType()).To(Equal(tt.want.service.Spec.Type)) + // GetLabels + g.Expect(service.GetLabels()).To(Equal(map[string]string{ + "foo": "bar", + })) + // AddAnnotation + service.AddAnnotation(map[string]string{"foo": "bar"}) + // GetAnnotations + g.Expect(service.GetAnnotations()).To(Equal(map[string]string{"foo": "bar"})) + // GetServiceHostname + g.Expect(service.GetServiceHostname()).To(Equal(tt.want.serviceHostname)) + // GetServiceHostnamePort + hostname, port := service.GetServiceHostnamePort() + g.Expect(hostname).To(Equal(tt.want.serviceHostname)) + g.Expect(port).To(Equal(tt.wantPort)) + // GetSpec + g.Expect(*service.GetSpec()).To(Equal(tt.want.service.Spec)) + // ToOverrideServiceSpec + dd, err := service.ToOverrideServiceSpec() + g.Expect(err).ToNot(HaveOccurred()) + g.Expect(dd).ToNot(BeNil()) + g.Expect(*dd).To(Equal(tt.wantOverrideServiceSpec)) + }) + } +} + +func TestGetAPIEndpoint(t *testing.T) { + tests := []struct { + name string + service *corev1.Service + endpointURL *string + proto Protocol + port string + path string + want string + }{ + { + name: "HTTP ClusterIP service default port 80, no override", + service: getServiceWithPort(svcClusterIP, portHTTP), + endpointURL: nil, + proto: ProtocolHTTP, + path: "", + want: "http://foo.namespace.svc", + }, + { + name: "HTTP ClusterIP service non default 8080 port, no override", + service: getServiceWithPort(svcClusterIP, portCustom), + endpointURL: nil, + proto: ProtocolHTTP, + path: "/path", + want: "http://foo.namespace.svc:8080/path", + }, + { + name: "HTTPS ClusterIP service default 443 port, no override", + service: getServiceWithPort(svcClusterIP, portHTTPS), + endpointURL: nil, + proto: ProtocolHTTPS, + path: "/path", + want: "https://foo.namespace.svc/path", + }, + { + name: "HTTPS ClusterIP service non default 8080 port, no override", + service: getServiceWithPort(svcClusterIP, portCustom), + endpointURL: nil, + proto: ProtocolHTTPS, + path: "/path", + want: "https://foo.namespace.svc:8080/path", + }, + { + name: "None ClusterIP service port 80 no override", + service: getServiceWithPort(svcClusterIP, portHTTP), + endpointURL: nil, + proto: ProtocolNone, + path: "/path", + want: "foo.namespace.svc:80/path", + }, + { + name: "None ClusterIP service port 8080 override", + service: getServiceWithPort(svcClusterIP, portCustom), + endpointURL: nil, + proto: ProtocolNone, + path: "/path", + want: "foo.namespace.svc:8080/path", + }, + { + name: "Override EndpointURL with path", + service: getServiceWithPort(svcClusterIP, portCustom), + endpointURL: ptr.To("http://this.url"), + proto: ProtocolNone, + path: "/path", + want: "http://this.url/path", + }, + { + name: "Override EndpointURL no path", + service: getServiceWithPort(svcClusterIP, portCustom), + endpointURL: ptr.To("http://this.url"), + proto: ProtocolNone, + path: "", + want: "http://this.url", + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + g := NewWithT(t) + + service, err := NewService(tt.service, timeout, nil) + g.Expect(err).ToNot(HaveOccurred()) + url, err := service.GetAPIEndpoint(tt.endpointURL, ptr.To(tt.proto), tt.path) + g.Expect(err).ToNot(HaveOccurred()) + g.Expect(url).To(Equal(tt.want)) + }) + } +} + +func TestToOverrideServiceSpec(t *testing.T) { + tests := []struct { + name string + service *corev1.Service + override OverrideSpec + want OverrideServiceSpec + }{ + { + name: "No override", + service: getServiceWithPort(svcClusterIP, portHTTP), + override: OverrideSpec{}, + want: overrideServiceSpecClusterIP, + }, + { + name: "HTTP ClusterIP service override service Type to LoadBalancer", + service: getServiceWithPort(svcClusterIP, portHTTP), + override: override, + want: overrideServiceSpecLoadBalancer, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + g := NewWithT(t) + + service, err := NewService(tt.service, timeout, &tt.override) + g.Expect(err).ToNot(HaveOccurred()) + // ToOverrideServiceSpec + ovrrdServiceSpec, err := service.ToOverrideServiceSpec() + g.Expect(err).ToNot(HaveOccurred()) + g.Expect(ovrrdServiceSpec).ToNot(BeNil()) + g.Expect(*ovrrdServiceSpec).To(Equal(tt.want)) + }) + } +} diff --git a/modules/common/service/types.go b/modules/common/service/types.go index a33b7b67..abd497c8 100644 --- a/modules/common/service/types.go +++ b/modules/common/service/types.go @@ -76,6 +76,7 @@ type GenericServiceDetails struct { // GenericServicePort - // +kubebuilder:object:generate:=false +// NOTE: (mschuppert) deprecated, can be removed when service operators moved to Ports type GenericServicePort struct { Name string Port int32 diff --git a/modules/common/test/functional/service_test.go b/modules/common/test/functional/service_test.go index 1c18f9a1..6b847602 100644 --- a/modules/common/test/functional/service_test.go +++ b/modules/common/test/functional/service_test.go @@ -24,6 +24,7 @@ import ( "github.com/openstack-k8s-operators/lib-common/modules/common/service" "k8s.io/apimachinery/pkg/types" "k8s.io/apimachinery/pkg/util/intstr" + "k8s.io/utils/ptr" corev1 "k8s.io/api/core/v1" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" @@ -116,7 +117,7 @@ var _ = Describe("service package", func() { Expect(hostname).To(Equal(fmt.Sprintf("test-svc.%s.svc", namespace))) Expect(port).To(Equal("80")) // HTTP endpoint with no port - endpointURL, err := s.GetAPIEndpoint(nil, service.PtrProtocol(service.ProtocolHTTP), "") + endpointURL, err := s.GetAPIEndpoint(nil, ptr.To(service.ProtocolHTTP), "") Expect(err).ShouldNot(HaveOccurred()) Expect(endpointURL).To(Equal(fmt.Sprintf("http://test-svc.%s.svc", namespace))) @@ -171,7 +172,7 @@ var _ = Describe("service package", func() { Expect(rv1.Labels["replace"]).To(Equal("b")) // HTTPS endpoint with port - endpointURL, err := s.GetAPIEndpoint(nil, service.PtrProtocol(service.ProtocolHTTPS), "") + endpointURL, err := s.GetAPIEndpoint(nil, ptr.To(service.ProtocolHTTPS), "") Expect(err).ShouldNot(HaveOccurred()) Expect(endpointURL).To(Equal(fmt.Sprintf("https://test-svc.%s.svc:5000", namespace))) }) @@ -202,7 +203,7 @@ var _ = Describe("service package", func() { Expect(rv1.Annotations["replace"]).To(Equal("b")) // HTTPS endpoint with no port - endpointURL, err := s.GetAPIEndpoint(nil, service.PtrProtocol(service.ProtocolHTTPS), "") + endpointURL, err := s.GetAPIEndpoint(nil, ptr.To(service.ProtocolHTTPS), "") Expect(err).ShouldNot(HaveOccurred()) Expect(endpointURL).To(Equal(fmt.Sprintf("https://test-svc.%s.svc", namespace))) }) @@ -225,7 +226,7 @@ var _ = Describe("service package", func() { Expect(svc.Spec.Type).To(Equal(corev1.ServiceTypeLoadBalancer)) // NONE endpoint with port - endpointURL, err := s.GetAPIEndpoint(nil, service.PtrProtocol(service.ProtocolNone), "") + endpointURL, err := s.GetAPIEndpoint(nil, ptr.To(service.ProtocolNone), "") Expect(err).ShouldNot(HaveOccurred()) Expect(endpointURL).To(Equal(fmt.Sprintf("test-svc.%s.svc:80", namespace))) }) From 0058795c31b2c6debf0f7ba9be24ee792f2e8b0d Mon Sep 17 00:00:00 2001 From: Martin Schuppert Date: Tue, 5 Sep 2023 09:13:39 +0200 Subject: [PATCH 6/9] [route] allow setting additional OwnerRefs Add an object to the ownerRef list of the route prevents the route from being deleted before the service is deleted. Otherwise this can result in cleanup issues which require the endpoint to be reachable. --- modules/common/route/route.go | 12 ++++++++++++ modules/common/route/types.go | 8 +++++--- modules/common/route/zz_generated.deepcopy.go | 7 +++++++ 3 files changed, 24 insertions(+), 3 deletions(-) diff --git a/modules/common/route/route.go b/modules/common/route/route.go index f5ab5d17..e0b75734 100644 --- a/modules/common/route/route.go +++ b/modules/common/route/route.go @@ -145,6 +145,18 @@ func (r *Route) CreateOrPatch( return err } + // Add the service CR to the ownerRef list of the route to prevent the route being deleted + // before the service is deleted. Otherwise this can result cleanup issues which require + // the endpoint to be reachable. + // If ALL objects in the list have been deleted, this object will be garbage collected. + // https://github.com/kubernetes/apimachinery/blob/15d95c0b2af3f4fcf46dce24105e5fbb9379af5a/pkg/apis/meta/v1/types.go#L240-L247 + for _, owner := range r.OwnerReferences { + err := controllerutil.SetOwnerReference(owner, route, h.GetScheme()) + if err != nil { + return err + } + } + return nil }) if err != nil { diff --git a/modules/common/route/types.go b/modules/common/route/types.go index 5f0b4568..46f1f08a 100644 --- a/modules/common/route/types.go +++ b/modules/common/route/types.go @@ -22,13 +22,15 @@ import ( "time" routev1 "github.com/openshift/api/route/v1" + metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" ) // Route - type Route struct { - route *routev1.Route - timeout time.Duration - hostname string + route *routev1.Route + timeout time.Duration + hostname string + OwnerReferences []metav1.Object } // GenericRouteDetails - diff --git a/modules/common/route/zz_generated.deepcopy.go b/modules/common/route/zz_generated.deepcopy.go index b60d3ce1..bc090eca 100644 --- a/modules/common/route/zz_generated.deepcopy.go +++ b/modules/common/route/zz_generated.deepcopy.go @@ -23,6 +23,7 @@ package route import ( "github.com/openshift/api/route/v1" + metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" ) // DeepCopyInto is an autogenerated deepcopy function, copying the receiver, writing into out. in must be non-nil. @@ -109,6 +110,12 @@ func (in *Route) DeepCopyInto(out *Route) { *out = new(v1.Route) (*in).DeepCopyInto(*out) } + if in.OwnerReferences != nil { + in, out := &in.OwnerReferences, &out.OwnerReferences + *out = make([]metav1.Object, len(*in)) + for i := range *in { + } + } } // DeepCopy is an autogenerated deepcopy function, copying the receiver, creating a new Route. From 559d36b8aa8a375bf4343bbee04a1fc48ee2f862 Mon Sep 17 00:00:00 2001 From: Martin Schuppert Date: Tue, 5 Sep 2023 09:49:23 +0200 Subject: [PATCH 7/9] [route] ignore non spec types for controller-gen --- modules/common/route/types.go | 2 + modules/common/route/zz_generated.deepcopy.go | 49 ------------------- 2 files changed, 2 insertions(+), 49 deletions(-) diff --git a/modules/common/route/types.go b/modules/common/route/types.go index 46f1f08a..d7f48d5c 100644 --- a/modules/common/route/types.go +++ b/modules/common/route/types.go @@ -26,6 +26,7 @@ import ( ) // Route - +// +kubebuilder:object:generate:=false type Route struct { route *routev1.Route timeout time.Duration @@ -34,6 +35,7 @@ type Route struct { } // GenericRouteDetails - +// +kubebuilder:object:generate:=false type GenericRouteDetails struct { Name string Namespace string diff --git a/modules/common/route/zz_generated.deepcopy.go b/modules/common/route/zz_generated.deepcopy.go index bc090eca..74f38c0b 100644 --- a/modules/common/route/zz_generated.deepcopy.go +++ b/modules/common/route/zz_generated.deepcopy.go @@ -23,7 +23,6 @@ package route import ( "github.com/openshift/api/route/v1" - metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" ) // DeepCopyInto is an autogenerated deepcopy function, copying the receiver, writing into out. in must be non-nil. @@ -55,28 +54,6 @@ func (in *EmbeddedLabelsAnnotations) DeepCopy() *EmbeddedLabelsAnnotations { return out } -// DeepCopyInto is an autogenerated deepcopy function, copying the receiver, writing into out. in must be non-nil. -func (in *GenericRouteDetails) DeepCopyInto(out *GenericRouteDetails) { - *out = *in - if in.Labels != nil { - in, out := &in.Labels, &out.Labels - *out = make(map[string]string, len(*in)) - for key, val := range *in { - (*out)[key] = val - } - } -} - -// DeepCopy is an autogenerated deepcopy function, copying the receiver, creating a new GenericRouteDetails. -func (in *GenericRouteDetails) DeepCopy() *GenericRouteDetails { - if in == nil { - return nil - } - out := new(GenericRouteDetails) - in.DeepCopyInto(out) - return out -} - // DeepCopyInto is an autogenerated deepcopy function, copying the receiver, writing into out. in must be non-nil. func (in *OverrideSpec) DeepCopyInto(out *OverrideSpec) { *out = *in @@ -102,32 +79,6 @@ func (in *OverrideSpec) DeepCopy() *OverrideSpec { return out } -// DeepCopyInto is an autogenerated deepcopy function, copying the receiver, writing into out. in must be non-nil. -func (in *Route) DeepCopyInto(out *Route) { - *out = *in - if in.route != nil { - in, out := &in.route, &out.route - *out = new(v1.Route) - (*in).DeepCopyInto(*out) - } - if in.OwnerReferences != nil { - in, out := &in.OwnerReferences, &out.OwnerReferences - *out = make([]metav1.Object, len(*in)) - for i := range *in { - } - } -} - -// DeepCopy is an autogenerated deepcopy function, copying the receiver, creating a new Route. -func (in *Route) DeepCopy() *Route { - if in == nil { - return nil - } - out := new(Route) - in.DeepCopyInto(out) - return out -} - // DeepCopyInto is an autogenerated deepcopy function, copying the receiver, writing into out. in must be non-nil. func (in *Spec) DeepCopyInto(out *Spec) { *out = *in From fb36ba6336e2ae96d3e70c0c0b5fc1e5b892ee6d Mon Sep 17 00:00:00 2001 From: Martin Schuppert Date: Tue, 5 Sep 2023 09:31:47 +0200 Subject: [PATCH 8/9] [svc] add endpoint annotation key const Annotation which can be set on a service to reflect for which endpoint type the service is (public/internal) --- modules/common/service/types.go | 2 ++ 1 file changed, 2 insertions(+) diff --git a/modules/common/service/types.go b/modules/common/service/types.go index abd497c8..66425662 100644 --- a/modules/common/service/types.go +++ b/modules/common/service/types.go @@ -40,6 +40,8 @@ const ( EndpointInternal Endpoint = "internal" // EndpointPublic - public endpoint EndpointPublic Endpoint = "public" + // AnnotationEndpointKey - + AnnotationEndpointKey = "endpoint" // AnnotationHostnameKey - AnnotationHostnameKey = "dnsmasq.network.openstack.org/hostname" // ProtocolHTTP - From 37d5500006d3575ad387858c13d2d6125779e727 Mon Sep 17 00:00:00 2001 From: Martin Schuppert Date: Tue, 5 Sep 2023 09:34:55 +0200 Subject: [PATCH 9/9] [svc] Add annotations for ingress control Adds AnnotationIngressCreateKey. Setting this annotation on the service will control the openstack-operator to create a route. --- modules/common/service/types.go | 2 ++ 1 file changed, 2 insertions(+) diff --git a/modules/common/service/types.go b/modules/common/service/types.go index 66425662..b7b6aeb0 100644 --- a/modules/common/service/types.go +++ b/modules/common/service/types.go @@ -40,6 +40,8 @@ const ( EndpointInternal Endpoint = "internal" // EndpointPublic - public endpoint EndpointPublic Endpoint = "public" + // AnnotationIngressCreateKey - + AnnotationIngressCreateKey = "core.openstack.org/ingress_create" // AnnotationEndpointKey - AnnotationEndpointKey = "endpoint" // AnnotationHostnameKey -