Skip to content

Commit

Permalink
feat(routing): support multi port services (#76)
Browse files Browse the repository at this point in the history 
The selected Service might have multiple ports to different types of
apis or protocols. In the previous version, we only exported the first
port under the name ServiceName+ServiceNamespace.

With this change we include all ports on the service and export them as
ServiceName+ServicePortName+SerivceNamespace

The PublicServiceName is changed to include the ServicePortName which
means we now also loop over each ServicePort in the exportedService.

Also moved the Host creation/concat into functions of the TemplateData
object which means that they are constructed in one location and reused
between templates and metadata updates in the api.

---------

Co-authored-by: Bartosz Majsak <[email protected]>
  • Loading branch information
@aslakknutsen @bartoszmajsak
aslakknutsen and bartoszmajsak authored Sep 4, 2024
1 parent a0078c1 commit 5915fa2
Show file tree
Hide file tree
Showing 6 changed files with 190 additions and 128 deletions.
209 changes: 122 additions & 87 deletions controllers/routingctrl/controller_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -126,7 +126,8 @@ var _ = Describe("Platform routing setup for the component", test.EnvTest(), fun
annotations.RoutingAddressesPublic("").Key(),
), "public services are not expected to be defined in this mode")

externalAddressesAnnotation := annotations.RoutingAddressesExternal(fmt.Sprintf("%s-%s.%s", svc.Name, svc.Namespace, domain))
externalAddressesAnnotation := annotations.RoutingAddressesExternal(
fmt.Sprintf("%[1]s-http-%[2]s.%[3]s"+";"+"%[1]s-grpc-%[2]s.%[3]s", svc.Name, svc.Namespace, domain))

g.Expect(updatedComponent.GetAnnotations()).To(HaveKeyWithValue(
externalAddressesAnnotation.Key(), externalAddressesAnnotation.Value(),
Expand Down Expand Up @@ -190,7 +191,8 @@ var _ = Describe("Platform routing setup for the component", test.EnvTest(), fun
), "public services are not expected to be defined in this mode")

publicAddressAnnotation := annotations.RoutingAddressesPublic(
fmt.Sprintf("%[1]s-%[2]s.%[3]s;%[1]s-%[2]s.%[3]s.svc;%[1]s-%[2]s.%[3]s.svc.cluster.local",
fmt.Sprintf("%[1]s-http-%[2]s.%[3]s;%[1]s-http-%[2]s.%[3]s.svc;%[1]s-http-%[2]s.%[3]s.svc.cluster.local;"+
"%[1]s-grpc-%[2]s.%[3]s;%[1]s-grpc-%[2]s.%[3]s.svc;%[1]s-grpc-%[2]s.%[3]s.svc.cluster.local",
svc.Name, svc.Namespace, routingConfiguration.GatewayNamespace),
)

Expand Down Expand Up @@ -236,10 +238,12 @@ var _ = Describe("Platform routing setup for the component", test.EnvTest(), fun
return errGet
}

externalAddressAnnotation := annotations.RoutingAddressesExternal(fmt.Sprintf("%s-%s.%s", svc.Name, svc.Namespace, domain))
externalAddressAnnotation := annotations.RoutingAddressesExternal(
fmt.Sprintf("%[1]s-http-%[2]s.%[3]s"+";"+"%[1]s-grpc-%[2]s.%[3]s", svc.Name, svc.Namespace, domain))

publicAddrAnnotation := annotations.RoutingAddressesPublic(
fmt.Sprintf("%[1]s-%[2]s.%[3]s;%[1]s-%[2]s.%[3]s.svc;%[1]s-%[2]s.%[3]s.svc.cluster.local",
fmt.Sprintf("%[1]s-http-%[2]s.%[3]s;%[1]s-http-%[2]s.%[3]s.svc;%[1]s-http-%[2]s.%[3]s.svc.cluster.local;"+
"%[1]s-grpc-%[2]s.%[3]s;%[1]s-grpc-%[2]s.%[3]s.svc;%[1]s-grpc-%[2]s.%[3]s.svc.cluster.local",
svc.Name, svc.Namespace, routingConfiguration.GatewayNamespace,
),
)
Expand Down Expand Up @@ -387,7 +391,8 @@ var _ = Describe("Platform routing setup for the component", test.EnvTest(), fun
), "public services are not expected to be defined in this mode")

publicAddressAnnotation := annotations.RoutingAddressesPublic(
fmt.Sprintf("%[1]s-%[2]s.%[3]s;%[1]s-%[2]s.%[3]s.svc;%[1]s-%[2]s.%[3]s.svc.cluster.local",
fmt.Sprintf("%[1]s-http-%[2]s.%[3]s;%[1]s-http-%[2]s.%[3]s.svc;%[1]s-http-%[2]s.%[3]s.svc.cluster.local;"+
"%[1]s-grpc-%[2]s.%[3]s;%[1]s-grpc-%[2]s.%[3]s.svc;%[1]s-grpc-%[2]s.%[3]s.svc.cluster.local",
svc.Name, svc.Namespace, routingConfiguration.GatewayNamespace),
)

Expand Down Expand Up @@ -539,131 +544,156 @@ func hasNoAddressAnnotations(component *unstructured.Unstructured) func(g Gomega
}
}

func routeExistsFor(exportedSvc *corev1.Service) func(g Gomega, ctx context.Context) error {
func routeExistsFor(exposedSvc *corev1.Service) func(g Gomega, ctx context.Context) error {
return func(g Gomega, ctx context.Context) error {
svcRoute := &openshiftroutev1.Route{}
if errGet := envTest.Get(ctx, types.NamespacedName{
Name: exportedSvc.Name + "-" + exportedSvc.Namespace + "-route",
Namespace: routingConfiguration.GatewayNamespace,
}, svcRoute); errGet != nil {
return errGet
g.Expect(exposedSvc.Spec.Ports).ToNot(BeEmpty())

for _, exposedPort := range exposedSvc.Spec.Ports {
svcRoute := &openshiftroutev1.Route{}
if errGet := envTest.Get(ctx, types.NamespacedName{
Name: exposedSvc.Name + "-" + exposedPort.Name + "-" + exposedSvc.Namespace + "-route",
Namespace: routingConfiguration.GatewayNamespace,
}, svcRoute); errGet != nil {
return errGet
}

g.Expect(svcRoute).To(BeAttachedToService(routingConfiguration.IngressService))
g.Expect(svcRoute).To(HaveHost(exposedSvc.Name + "-" + exposedPort.Name + "-" + exposedSvc.Namespace + "." + domain))
}

g.Expect(svcRoute).To(BeAttachedToService(routingConfiguration.IngressService))
g.Expect(svcRoute).To(HaveHost(exportedSvc.Name + "-" + exportedSvc.Namespace + "." + domain))

return nil
}
}

func publicSvcExistsFor(exposedSvc *corev1.Service) func(g Gomega, ctx context.Context) error {
return func(g Gomega, ctx context.Context) error {
publicSvc := &corev1.Service{}
if errGet := envTest.Get(ctx, types.NamespacedName{
Name: exposedSvc.Name + "-" + exposedSvc.Namespace,
Namespace: routingConfiguration.GatewayNamespace,
}, publicSvc); errGet != nil {
return errGet
g.Expect(exposedSvc.Spec.Ports).ToNot(BeEmpty())

for _, exposedPort := range exposedSvc.Spec.Ports {
publicSvc := &corev1.Service{}
if errGet := envTest.Get(ctx, types.NamespacedName{
Name: exposedSvc.Name + "-" + exposedPort.Name + "-" + exposedSvc.Namespace,
Namespace: routingConfiguration.GatewayNamespace,
}, publicSvc); errGet != nil {
return errGet
}

g.Expect(publicSvc.GetAnnotations()).To(
HaveKeyWithValue(
"service.beta.openshift.io/serving-cert-secret-name",
exposedSvc.Name+"-"+exposedPort.Name+"-"+exposedSvc.Namespace+"-certs",
),
)

g.Expect(publicSvc.Spec.Selector).To(
HaveKeyWithValue(routingConfiguration.IngressSelectorLabel, routingConfiguration.IngressSelectorValue),
)
}

g.Expect(publicSvc.GetAnnotations()).To(
HaveKeyWithValue(
"service.beta.openshift.io/serving-cert-secret-name",
exposedSvc.Name+"-"+exposedSvc.Namespace+"-certs",
),
)

g.Expect(publicSvc.Spec.Selector).To(
HaveKeyWithValue(routingConfiguration.IngressSelectorLabel, routingConfiguration.IngressSelectorValue),
)

return nil
}
}

func publicGatewayExistsFor(exposedSvc *corev1.Service) func(g Gomega, ctx context.Context) error {
return func(g Gomega, ctx context.Context) error {
publicGateway := &v1beta1.Gateway{}
if errGet := envTest.Get(ctx, types.NamespacedName{
Name: exposedSvc.Name + "-" + exposedSvc.Namespace,
Namespace: routingConfiguration.GatewayNamespace,
}, publicGateway); errGet != nil {
return errGet
g.Expect(exposedSvc.Spec.Ports).ToNot(BeEmpty())

for _, exposedPort := range exposedSvc.Spec.Ports {
publicGateway := &v1beta1.Gateway{}
if errGet := envTest.Get(ctx, types.NamespacedName{
Name: exposedSvc.Name + "-" + exposedPort.Name + "-" + exposedSvc.Namespace,
Namespace: routingConfiguration.GatewayNamespace,
}, publicGateway); errGet != nil {
return errGet
}

g.Expect(publicGateway.Spec.GetSelector()).To(HaveKeyWithValue(routingConfiguration.IngressSelectorLabel, routingConfiguration.IngressSelectorValue))
// limitation: only checks first element of []*Server slice
g.Expect(publicGateway).To(
HaveHosts(
exposedSvc.Name+"-"+exposedPort.Name+"-"+exposedSvc.Namespace+"."+routingConfiguration.GatewayNamespace,
exposedSvc.Name+"-"+exposedPort.Name+"-"+exposedSvc.Namespace+"."+routingConfiguration.GatewayNamespace+".svc",
exposedSvc.Name+"-"+exposedPort.Name+"-"+exposedSvc.Namespace+"."+routingConfiguration.GatewayNamespace+".svc.cluster.local",
),
)
}

g.Expect(publicGateway.Spec.GetSelector()).To(HaveKeyWithValue(routingConfiguration.IngressSelectorLabel, routingConfiguration.IngressSelectorValue))
// limitation: only checks first element of []*Server slice
g.Expect(publicGateway).To(
HaveHosts(
exposedSvc.Name+"-"+exposedSvc.Namespace+"."+routingConfiguration.GatewayNamespace,
exposedSvc.Name+"-"+exposedSvc.Namespace+"."+routingConfiguration.GatewayNamespace+".svc",
exposedSvc.Name+"-"+exposedSvc.Namespace+"."+routingConfiguration.GatewayNamespace+".svc.cluster.local",
),
)

return nil
}
}

func publicVirtualSvcExistsFor(exposedSvc *corev1.Service) func(g Gomega, ctx context.Context) error {
return func(g Gomega, ctx context.Context) error {
publicVS := &v1beta1.VirtualService{}
if errGet := envTest.Get(ctx, types.NamespacedName{
Name: exposedSvc.Name + "-" + exposedSvc.Namespace,
Namespace: routingConfiguration.GatewayNamespace,
}, publicVS); errGet != nil {
return errGet
}

g.Expect(publicVS).To(
HaveHosts(
exposedSvc.Name+"-"+exposedSvc.Namespace+"."+routingConfiguration.GatewayNamespace,
exposedSvc.Name+"-"+exposedSvc.Namespace+"."+routingConfiguration.GatewayNamespace+".svc",
exposedSvc.Name+"-"+exposedSvc.Namespace+"."+routingConfiguration.GatewayNamespace+".svc.cluster.local",
),
)
g.Expect(publicVS).To(BeAttachedToGateways("mesh", exposedSvc.Name+"-"+exposedSvc.Namespace))
g.Expect(publicVS).To(RouteToHost(exposedSvc.Name+"."+exposedSvc.Namespace+".svc.cluster.local", 8000))
g.Expect(exposedSvc.Spec.Ports).ToNot(BeEmpty())

for _, exposedPort := range exposedSvc.Spec.Ports {
if errGet := envTest.Get(ctx, types.NamespacedName{
Name: exposedSvc.Name + "-" + exposedPort.Name + "-" + exposedSvc.Namespace,
Namespace: routingConfiguration.GatewayNamespace,
}, publicVS); errGet != nil {
return errGet
}

g.Expect(publicVS).To(
HaveHosts(
exposedSvc.Name+"-"+exposedPort.Name+"-"+exposedSvc.Namespace+"."+routingConfiguration.GatewayNamespace,
exposedSvc.Name+"-"+exposedPort.Name+"-"+exposedSvc.Namespace+"."+routingConfiguration.GatewayNamespace+".svc",
exposedSvc.Name+"-"+exposedPort.Name+"-"+exposedSvc.Namespace+"."+routingConfiguration.GatewayNamespace+".svc.cluster.local",
),
)
g.Expect(publicVS).To(BeAttachedToGateways("mesh", exposedSvc.Name+"-"+exposedPort.Name+"-"+exposedSvc.Namespace))
g.Expect(publicVS).To(RouteToHost(exposedSvc.Name+"."+exposedSvc.Namespace+".svc.cluster.local", uint32(exposedPort.TargetPort.IntVal)))
}

return nil
}
}

func destinationRuleExistsFor(exposedSvc *corev1.Service) func(g Gomega, ctx context.Context) error {
return func(g Gomega, ctx context.Context) error {
destinationRule := &v1beta1.DestinationRule{}
if errGet := envTest.Get(ctx, types.NamespacedName{
Name: exposedSvc.Name + "-" + exposedSvc.Namespace,
Namespace: routingConfiguration.GatewayNamespace,
}, destinationRule); errGet != nil {
return errGet
g.Expect(exposedSvc.Spec.Ports).ToNot(BeEmpty())

for _, exposedPort := range exposedSvc.Spec.Ports {
destinationRule := &v1beta1.DestinationRule{}
if errGet := envTest.Get(ctx, types.NamespacedName{
Name: exposedSvc.Name + "-" + exposedPort.Name + "-" + exposedSvc.Namespace,
Namespace: routingConfiguration.GatewayNamespace,
}, destinationRule); errGet != nil {
return errGet
}

g.Expect(destinationRule).To(
HaveHost(
exposedSvc.Name + "-" + exposedPort.Name + "-" + exposedSvc.Namespace + "." + routingConfiguration.GatewayNamespace + ".svc.cluster.local",
),
)
g.Expect(destinationRule.Spec.GetTrafficPolicy().GetTls().GetMode()).To(Equal(istionetworkingv1beta1.ClientTLSSettings_DISABLE))
}

g.Expect(destinationRule).To(
HaveHost(
exposedSvc.Name + "-" + exposedSvc.Namespace + "." + routingConfiguration.GatewayNamespace + ".svc.cluster.local",
),
)
g.Expect(destinationRule.Spec.GetTrafficPolicy().GetTls().GetMode()).To(Equal(istionetworkingv1beta1.ClientTLSSettings_DISABLE))

return nil
}
}

func ingressVirtualServiceExistsFor(exportedSvc *corev1.Service) func(g Gomega, ctx context.Context) error {
func ingressVirtualServiceExistsFor(exposedSvc *corev1.Service) func(g Gomega, ctx context.Context) error {
return func(g Gomega, ctx context.Context) error {
routerVS := &v1beta1.VirtualService{}
if errGet := envTest.Get(ctx, types.NamespacedName{
Name: exportedSvc.Name + "-" + exportedSvc.Namespace + "-ingress",
Namespace: routingConfiguration.GatewayNamespace,
}, routerVS); errGet != nil {
return errGet
g.Expect(exposedSvc.Spec.Ports).ToNot(BeEmpty())

for _, exposedPort := range exposedSvc.Spec.Ports {
routerVS := &v1beta1.VirtualService{}
if errGet := envTest.Get(ctx, types.NamespacedName{
Name: exposedSvc.Name + "-" + exposedPort.Name + "-" + exposedSvc.Namespace + "-ingress",
Namespace: routingConfiguration.GatewayNamespace,
}, routerVS); errGet != nil {
return errGet
}

g.Expect(routerVS).To(HaveHost(exposedSvc.Name + "-" + exposedPort.Name + "-" + exposedSvc.Namespace + "." + domain))
g.Expect(routerVS).To(BeAttachedToGateways(routingConfiguration.IngressService))
g.Expect(routerVS).To(RouteToHost(exposedSvc.Name+"."+exposedSvc.Namespace+".svc.cluster.local", uint32(exposedPort.TargetPort.IntValue())))
}

g.Expect(routerVS).To(HaveHost(exportedSvc.Name + "-" + exportedSvc.Namespace + "." + domain))
g.Expect(routerVS).To(BeAttachedToGateways(routingConfiguration.IngressService))
g.Expect(routerVS).To(RouteToHost(exportedSvc.Name+"."+exportedSvc.Namespace+".svc.cluster.local", 8000))

return nil
}
}
Expand Down Expand Up @@ -692,6 +722,11 @@ func simpleSvcDeployment(ctx context.Context, nsName, svcName string) (*appsv1.D
Port: 8080,
TargetPort: intstr.FromInt32(8000),
},
{
Name: "grpc",
Port: 9080,
TargetPort: intstr.FromInt32(9000),
},
},
},
}
Expand Down
55 changes: 31 additions & 24 deletions controllers/routingctrl/reconcile_resources.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@ import (
"context"
"errors"
"fmt"
"strings"

"github.com/opendatahub-io/odh-platform/pkg/cluster"
"github.com/opendatahub-io/odh-platform/pkg/config"
Expand Down Expand Up @@ -34,7 +35,7 @@ func (r *Controller) createRoutingResources(ctx context.Context, target *unstruc

renderedSelectors, errLables := config.ResolveSelectors(r.component.ServiceSelector, target)
if errLables != nil {
return fmt.Errorf("could not render labels for ServiceSelector %v. Error %w", r.component.ServiceSelector, errLables)
return fmt.Errorf("could not render labels for ServiceSelector %v: %w", r.component.ServiceSelector, errLables)
}

exportedServices, errSvcGet := getExportedServices(ctx, r.Client, renderedSelectors, target)
Expand Down Expand Up @@ -67,45 +68,51 @@ func (r *Controller) createRoutingResources(ctx context.Context, target *unstruc
func (r *Controller) exportService(ctx context.Context, target *unstructured.Unstructured, exportedSvc *corev1.Service, domain string) error {
exportModes := r.extractExportModes(target)

templateData := routing.NewExposedServiceConfig(exportedSvc, r.config, domain)
externalHosts := []string{}
publicHosts := []string{}

// To establish ownership for watched component
ownershipLabels := append(labels.AsOwner(target), labels.AppManagedBy("odh-routing-controller"))

for _, exportMode := range exportModes {
resources, err := r.templateLoader.Load(templateData, exportMode)
if err != nil {
return fmt.Errorf("could not load templates for type %s: %w", exportMode, err)
}
for _, exportedSvcPort := range exportedSvc.Spec.Ports {
templateData := routing.NewExposedServiceConfig(exportedSvc, exportedSvcPort, r.config, domain)

for _, exportMode := range exportModes {
resources, err := r.templateLoader.Load(templateData, exportMode)
if err != nil {
return fmt.Errorf("could not load templates for type %s: %w", exportMode, err)
}

ownershipLabels = append(ownershipLabels, labels.ExportType(exportMode))
if errApply := unstruct.Apply(ctx, r.Client, resources, ownershipLabels...); errApply != nil {
return fmt.Errorf("could not apply routing resources for type %s: %w", exportMode, errApply)
ownershipLabels = append(ownershipLabels, labels.ExportType(exportMode))
if errApply := unstruct.Apply(ctx, r.Client, resources, ownershipLabels...); errApply != nil {
return fmt.Errorf("could not apply routing resources for type %s: %w", exportMode, errApply)
}

switch exportMode {
case routing.ExternalRoute:
externalHosts = append(externalHosts, templateData.ExternalHost())
case routing.PublicRoute:
publicHosts = append(publicHosts, templateData.PublicHosts()...)
}
}
}

return r.propagateHostsToWatchedCR(target, templateData)
return r.propagateHostsToWatchedCR(target, publicHosts, externalHosts)
}

func (r *Controller) propagateHostsToWatchedCR(target *unstructured.Unstructured, data *routing.ExposedServiceConfig) error {
exportModes := r.extractExportModes(target)

func (r *Controller) propagateHostsToWatchedCR(target *unstructured.Unstructured, publicHosts, externalHosts []string) error {
// Remove all existing routing addresses
metaOptions := []metadata.Option{
annotations.Remove(annotations.RoutingAddressesExternal("")),
annotations.Remove(annotations.RoutingAddressesPublic("")),
}

// TODO(mvp): put the logic of creating host names into a single place
for _, exportMode := range exportModes {
switch exportMode {
case routing.ExternalRoute:
externalAddress := annotations.RoutingAddressesExternal(fmt.Sprintf("%s-%s.%s", data.ServiceName, data.ServiceNamespace, data.Domain))
metaOptions = append(metaOptions, externalAddress)
case routing.PublicRoute:
publicAddresses := annotations.RoutingAddressesPublic(fmt.Sprintf("%[1]s.%[2]s;%[1]s.%[2]s.svc;%[1]s.%[2]s.svc.cluster.local", data.PublicServiceName, data.GatewayNamespace))
metaOptions = append(metaOptions, publicAddresses)
}
if len(publicHosts) > 0 {
metaOptions = append(metaOptions, annotations.RoutingAddressesPublic(strings.Join(publicHosts, ";")))
}

if len(externalHosts) > 0 {
metaOptions = append(metaOptions, annotations.RoutingAddressesExternal(strings.Join(externalHosts, ";")))
}

metadata.ApplyMetaOptions(target, metaOptions...)
Expand Down
Loading

0 comments on commit 5915fa2

Please sign in to comment.