Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Allow customize service via service.Override and handle endpoint protocol #332

Merged
merged 9 commits into from
Sep 19, 2023
2 changes: 2 additions & 0 deletions modules/certmanager/go.mod
Original file line number Diff line number Diff line change
Expand Up @@ -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
2 changes: 0 additions & 2 deletions modules/certmanager/go.sum
Original file line number Diff line number Diff line change
Expand Up @@ -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=
Expand Down
41 changes: 30 additions & 11 deletions modules/common/endpoint/endpoint.go
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,7 @@ package endpoint

import (
"context"
"fmt"
"net/url"
"strings"
"time"
Expand Down Expand Up @@ -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
Expand All @@ -73,12 +82,14 @@ 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(
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ack, we still have this here but after the whole chain is merged we can do some cleanup.

ctx context.Context,
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)
Expand All @@ -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 {
Expand All @@ -105,7 +117,7 @@ func ExposeEndpoints(
}

// Create the service
svc := service.NewService(
svc, err := service.NewService(
service.MetalLBService(&service.MetalLBServiceDetails{
Name: endpointName,
Namespace: h.GetBeforeObject().GetNamespace(),
Expand All @@ -117,12 +129,15 @@ func ExposeEndpoints(
Protocol: protocol,
},
}),
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, ",")
Expand All @@ -144,11 +159,11 @@ func ExposeEndpoints(
}
// create service - end

hostname = svc.GetServiceHostnamePort()
hostname, port = svc.GetServiceHostnamePort()
} else {

// Create the service
svc := service.NewService(
svc, err := service.NewService(
service.GenericService(&service.GenericServiceDetails{
Name: endpointName,
Namespace: h.GetBeforeObject().GetNamespace(),
Expand All @@ -159,9 +174,13 @@ func ExposeEndpoints(
Port: data.Port,
Protocol: corev1.ProtocolTCP,
}}),
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
Expand All @@ -170,10 +189,10 @@ func ExposeEndpoints(
}
// create service - end

hostname = svc.GetServiceHostnamePort()
hostname, port = 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(
Expand All @@ -184,7 +203,6 @@ func ExposeEndpoints(
ServiceName: endpointName,
TargetPortName: endpointName,
}),
exportLabels,
timeout,
data.RouteOverride,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@stuggi quick question here: when a GenericRoute is created, in case tls is used we're terminating the ssl on the Route, w/o re-encrypting the second segment (from the Route to the Service).
Is my understanding correct?
This would be a good behavior to keep and provide parameters via RouteOverride to change the behavior to re-encrypt.
For instance Glance atm doesn't support tls, and in TripleO we used to deploy an httpd in front of GlanceAPI to properly handle tls.
This is something I'm going to work on in the next few weeks, but wanted to check that we actually have the ability to terminate the connection on the Route until that part is done.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yes, for TLS-E we terminate at the route and re-encrypt to the service pods. Before you start working on this, we already do initial work for TLS-E in a generic way for all services. So don't start before checking with us because its probably duplicate work.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yes, I'll wait to see the first patch series landed, and when doing tls we can sync up and discuss about the Glance use case. We might need to have an additional sidecar container w/ httpd or make it default like it is today in Manila/Cinder.

)
Expand Down Expand Up @@ -216,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
}
Expand Down
18 changes: 17 additions & 1 deletion modules/common/route/route.go
Original file line number Diff line number Diff line change
Expand Up @@ -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) {
Expand Down Expand Up @@ -89,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{
Expand Down Expand Up @@ -141,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 {
Expand Down
10 changes: 7 additions & 3 deletions modules/common/route/types.go
Original file line number Diff line number Diff line change
Expand Up @@ -22,16 +22,20 @@ import (
"time"

routev1 "github.com/openshift/api/route/v1"
metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
)

// Route -
// +kubebuilder:object:generate:=false
bogdando marked this conversation as resolved.
Show resolved Hide resolved
type Route struct {
route *routev1.Route
timeout time.Duration
hostname string
route *routev1.Route
timeout time.Duration
hostname string
OwnerReferences []metav1.Object
}

// GenericRouteDetails -
// +kubebuilder:object:generate:=false
type GenericRouteDetails struct {
Name string
Namespace string
Expand Down
42 changes: 0 additions & 42 deletions modules/common/route/zz_generated.deepcopy.go

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

Loading