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.

Sign up for GitHub

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

Add support to configure multiple service ports #316

Open
wants to merge 3 commits into
base: main
Choose a base branch
from
Open

Add support to configure multiple service ports #316

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
db0979d
Add support to configure multiple service ports
shivamerla Feb 7, 2025
ce452e2
Set targetports to defaults for each microservice when they are not c…
shivamerla Feb 12, 2025
bc7188a
Further cleanup
shivamerla Feb 13, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
57 changes: 57 additions & 0 deletions api/apps/v1alpha1/common_types.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -21,6 +21,7 @@ import (
autoscalingv2 "k8s.io/api/autoscaling/v2"
corev1 "k8s.io/api/core/v1"
networkingv1 "k8s.io/api/networking/v1"
"k8s.io/apimachinery/pkg/util/intstr"
)

// Expose defines attributes to expose the service
Expand Down Expand Up @@ -115,3 +116,59 @@ type CertConfig struct {
// MountPath is the path where the certificates should be mounted in the container.
MountPath string `json:"mountPath"`
}

// selectNamedPort returns the first occurrence of a given named port, or an empty string if not found.
func selectNamedPort(serviceSpec Service, portNames ...string) string {
for _, name := range portNames {
for _, port := range serviceSpec.Ports {
if port.Name == name {
return name
}
}
}
return ""
}

// getProbePort determines the appropriate port for probes based on the service spec.
func getProbePort(serviceSpec Service) intstr.IntOrString {
Copy link
Collaborator

Choose a reason for hiding this comment

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

we can probably use rendertypes.ServiceParams as input here and simplify this to always prefer "api" namedPort from the ServiceParams.Ports list

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Each resource is rendered independently during reconciliation. That will add a dependency that we always have to render serviceParams before setting up deployment or servicemonitor.

switch len(serviceSpec.Ports) {
case 1:
port := serviceSpec.Ports[0]
if port.Name != "" {
return intstr.FromString(port.Name)
}
return intstr.FromInt(int(port.Port))
case 0:
// Default to "api" as the operator always adds a default named port with 8000
return intstr.FromString("api")
default:
// Multiple ports: Prefer "api"
if portName := selectNamedPort(serviceSpec, "api"); portName != "" {
shivamerla marked this conversation as resolved.
Show resolved Hide resolved
return intstr.FromString(portName)
}
// Default when multiple ports exist
return intstr.FromString("api")
}
}
Copy link
Collaborator

@varunrsekar varunrsekar Feb 12, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

case where length is 1 and default case where length > 1 should have similar behavior:

  • Prefer "api"
  • Otherwise, pick first port
    Isn't this the case? Defaulting to "api" when other options exist doesnt seem right to me.
Suggested change
switch len(serviceSpec.Ports) {
case 1:
port := serviceSpec.Ports[0]
if port.Name != "" {
return intstr.FromString(port.Name)
}
return intstr.FromInt(int(port.Port))
case 0:
// Default to "api" as the operator always adds a default named port with 8000
return intstr.FromString("api")
default:
// Multiple ports: Prefer "api"
if portName := selectNamedPort(serviceSpec, "api"); portName != "" {
return intstr.FromString(portName)
}
// Default when multiple ports exist
return intstr.FromString("api")
}
}
switch len(serviceSpec.Ports) {
case 0:
// Default to "api" as the operator always adds a default named port with 8000
return intstr.FromString("api")
default:
// Multiple ports: Prefer "api"
if portName := selectNamedPort(serviceSpec, "api"); portName != "" {
return intstr.FromString(portName)
}
// Default when multiple ports exist
port := serviceSpec.Ports[0]
if port.Name != "" {
return intstr.FromString(port.Name)
}
return intstr.FromInt(int(port.Port))
}
}

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

We just can't assume to use the first port when multiple ports are given. For e.g. in case of Customizer, if "api" and "internal" ports are used with different names, we can't assume "internal" ports for liveness/startup probes etc. Hence i was always enforcing that a named port of "api" should be specified in that case. Probably better things is to validate this during reconciliation and error out when required named ports are not provided.


// getMetricsPort determines the appropriate port for metrics based on the service spec.
func getMetricsPort(serviceSpec Service) intstr.IntOrString {
switch len(serviceSpec.Ports) {
case 1:
port := serviceSpec.Ports[0]
if port.Name != "" {
return intstr.FromString(port.Name)
}
return intstr.FromInt(int(port.Port))
case 0:
// Default to "api" as the operator always adds a default named port with 8000
return intstr.FromString("api")
default:
// Multiple ports: Prefer "metrics", fallback to "api"
if portName := selectNamedPort(serviceSpec, "metrics", "api"); portName != "" {
return intstr.FromString(portName)
}
// Default when multiple ports exist
return intstr.FromString("metrics")
Copy link
Collaborator

Choose a reason for hiding this comment

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

similarly here: Shouldn't we be getting the first port from the list if "metrics" and "api" dont exist?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

same logic as above, in documentation we have to specify that named ports for "api" or "metrics" should be provided when they are served differently by the underlying service. Randomly assigning would not work.

}
}
72 changes: 72 additions & 0 deletions api/apps/v1alpha1/common_types_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,72 @@
/*
Copyright 2025.

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 v1alpha1

import (
"testing"

corev1 "k8s.io/api/core/v1"
"k8s.io/apimachinery/pkg/util/intstr"
)

// assertEqual checks if the actual result matches the expected result.
func assertEqual(t *testing.T, actual, expected intstr.IntOrString) {
if actual != expected {
t.Errorf("Got %v, expected %v", actual, expected)
}
}

func TestGetProbePort(t *testing.T) {
tests := []struct {
name string
serviceSpec Service
expected intstr.IntOrString
}{
{"Single named port", Service{Ports: []corev1.ServicePort{{Name: "api", Port: 8080}}}, intstr.FromString("api")},
{"Single unnamed port", Service{Ports: []corev1.ServicePort{{Port: 9090}}}, intstr.FromInt(9090)},
{"Multiple ports - prefers 'api'", Service{Ports: []corev1.ServicePort{{Name: "metrics", Port: 9090}, {Name: "api", Port: 8080}}}, intstr.FromString("api")},
{"No ports - uses legacy Port field", Service{Port: 7070}, intstr.FromString("api")},
{"No ports at all - defaults to 'api'", Service{}, intstr.FromString("api")},
}

for _, tt := range tests {
t.Run(tt.name, func(t *testing.T) {
assertEqual(t, getProbePort(tt.serviceSpec), tt.expected)
})
}
}

func TestGetMetricsPort(t *testing.T) {
tests := []struct {
name string
serviceSpec Service
expected intstr.IntOrString
}{
{"Single named port", Service{Ports: []corev1.ServicePort{{Name: "metrics", Port: 8081}}}, intstr.FromString("metrics")},
{"Single unnamed port", Service{Ports: []corev1.ServicePort{{Port: 8181}}}, intstr.FromInt(8181)},
Copy link
Collaborator

Choose a reason for hiding this comment

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

trying to understand the logic.. in this case, why it shouldn't produce api like the test case in L63?

Copy link
Collaborator Author

@shivamerla shivamerla Feb 13, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

when say user provides input as below:

  expose:
    service:
      type: ClusterIP
      ports:
        - port: 8000
           protocol: TCP

then, in the deployment spec, we don't add any named ports like "api". Hence we use the port number that is given to us. we will add a default named port ("api: 8000" ) when no ports are given in the config at all.

{"Multiple ports - prefers 'metrics'", Service{Ports: []corev1.ServicePort{{Name: "api", Port: 8080}, {Name: "metrics", Port: 9090}}}, intstr.FromString("metrics")},
{"Multiple ports - no 'metrics', uses 'api'", Service{Ports: []corev1.ServicePort{{Name: "grpc", Port: 5050}, {Name: "api", Port: 8080}}}, intstr.FromString("api")},
{"No ports - uses legacy Port field", Service{Port: 6060}, intstr.FromString("api")},
{"No ports at all - defaults to 'metrics'", Service{}, intstr.FromString("api")},
}

for _, tt := range tests {
t.Run(tt.name, func(t *testing.T) {
assertEqual(t, getMetricsPort(tt.serviceSpec), tt.expected)
})
}
}
31 changes: 15 additions & 16 deletions api/apps/v1alpha1/nemo_customizer_types.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -436,10 +436,7 @@ func (n *NemoCustomizer) GetDefaultStartupProbe() *corev1.Probe {
ProbeHandler: corev1.ProbeHandler{
HTTPGet: &corev1.HTTPGetAction{
Path: "/v1/health/ready",
Port: intstr.IntOrString{
Type: intstr.Type(1),
StrVal: "api",
},
Port: getProbePort(n.Spec.Expose.Service),
},
},
}
Expand All @@ -466,11 +463,7 @@ func (n *NemoCustomizer) GetDefaultLivenessProbe() *corev1.Probe {
ProbeHandler: corev1.ProbeHandler{
HTTPGet: &corev1.HTTPGetAction{
Path: "/v1/health/live",
Port: intstr.IntOrString{
Type: intstr.Type(1),
StrVal: "api",
},
Scheme: "HTTP",
Port: getProbePort(n.Spec.Expose.Service),
},
},
}
Expand All @@ -496,10 +489,7 @@ func (n *NemoCustomizer) GetDefaultReadinessProbe() *corev1.Probe {
ProbeHandler: corev1.ProbeHandler{
HTTPGet: &corev1.HTTPGetAction{
Path: "/v1/health/ready",
Port: intstr.IntOrString{
Type: intstr.Type(1),
StrVal: "api",
},
Port: getProbePort(n.Spec.Expose.Service),
},
},
}
Expand Down Expand Up @@ -752,8 +742,8 @@ func (n *NemoCustomizer) GetServiceParams() *rendertypes.ServiceParams {
// Use corev1.ServicePort instead of deprecated params.Port
params.Ports = []corev1.ServicePort{{
Name: "api",
Port: 8000,
TargetPort: intstr.FromInt(8000),
Port: n.GetServicePort(),
TargetPort: intstr.FromInt32(n.GetServicePort()),
Protocol: corev1.ProtocolTCP,
}}
}
Expand Down Expand Up @@ -892,11 +882,20 @@ func (n *NemoCustomizer) GetServiceMonitorParams() *rendertypes.ServiceMonitorPa
params.Labels = svcLabels
params.Annotations = n.GetServiceMonitorAnnotations()

// Determine the appropriate port for monitoring
metricsPort := getMetricsPort(n.Spec.Expose.Service)

// Set Service Monitor spec
smSpec := monitoringv1.ServiceMonitorSpec{
NamespaceSelector: monitoringv1.NamespaceSelector{MatchNames: []string{n.Namespace}},
Selector: metav1.LabelSelector{MatchLabels: n.GetServiceLabels()},
Endpoints: []monitoringv1.Endpoint{{Port: "service-port", ScrapeTimeout: serviceMonitor.ScrapeTimeout, Interval: serviceMonitor.Interval}},
Endpoints: []monitoringv1.Endpoint{
{
Port: metricsPort.StrVal,
ScrapeTimeout: serviceMonitor.ScrapeTimeout,
Interval: serviceMonitor.Interval,
},
},
}
params.SMSpec = smSpec
return params
Expand Down
65 changes: 50 additions & 15 deletions api/apps/v1alpha1/nemo_datastore_types.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -592,10 +592,7 @@ func (n *NemoDatastore) GetDefaultLivenessProbe() *corev1.Probe {
ProbeHandler: corev1.ProbeHandler{
HTTPGet: &corev1.HTTPGetAction{
Path: "/v1/health/live",
Port: intstr.IntOrString{
Type: intstr.Type(0),
IntVal: n.Spec.Expose.Service.Port,
},
Port: getProbePort(n.Spec.Expose.Service),
Copy link
Collaborator

Choose a reason for hiding this comment

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

This implies that .spec.expose.service is required, but it is not currently.
Maybe we should make it a required field or add a default value?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

yes, we can make this as a required field, earlier there was just single port param and default for that, so it was always present.

},
},
}
Expand All @@ -622,10 +619,7 @@ func (n *NemoDatastore) GetDefaultReadinessProbe() *corev1.Probe {
ProbeHandler: corev1.ProbeHandler{
HTTPGet: &corev1.HTTPGetAction{
Path: "/v1/health/ready",
Port: intstr.IntOrString{
Type: intstr.Type(0),
IntVal: n.Spec.Expose.Service.Port,
},
Port: getProbePort(n.Spec.Expose.Service),
},
},
}
Expand All @@ -649,10 +643,7 @@ func (n *NemoDatastore) GetDefaultStartupProbe() *corev1.Probe {
ProbeHandler: corev1.ProbeHandler{
HTTPGet: &corev1.HTTPGetAction{
Path: "/v1/health/ready",
Port: intstr.IntOrString{
Type: intstr.Type(0),
IntVal: n.Spec.Expose.Service.Port,
},
Port: getProbePort(n.Spec.Expose.Service),
shivamerla marked this conversation as resolved.
Show resolved Hide resolved
},
},
}
Expand Down Expand Up @@ -806,6 +797,11 @@ func (n *NemoDatastore) IsServiceMonitorEnabled() bool {
return n.Spec.Metrics.Enabled != nil && *n.Spec.Metrics.Enabled
}

// GetServicePorts returns the service ports for the NemoDatastore deployment
func (n *NemoDatastore) GetServicePorts() []corev1.ServicePort {
return n.Spec.Expose.Service.Ports
}

// GetServicePort returns the service port for the NemoDatastore deployment
func (n *NemoDatastore) GetServicePort() int32 {
return n.Spec.Expose.Service.Port
Expand Down Expand Up @@ -893,6 +889,26 @@ func (n *NemoDatastore) GetDeploymentParams() *rendertypes.DeploymentParams {

// Set runtime class
params.RuntimeClassName = n.GetRuntimeClass()

// Extract ports from spec and update rendering params
if len(n.GetServicePorts()) > 0 {
var containerPorts []corev1.ContainerPort
for _, svcPort := range n.GetServicePorts() {
containerPorts = append(containerPorts, corev1.ContainerPort{
Name: svcPort.Name,
Protocol: svcPort.Protocol,
ContainerPort: svcPort.Port,
})
}
params.Ports = containerPorts
} else {
params.Ports = []corev1.ContainerPort{{
Name: "api",
Protocol: corev1.ProtocolTCP,
ContainerPort: n.GetServicePort(),
}}
}

return params
}

Expand Down Expand Up @@ -959,8 +975,18 @@ func (n *NemoDatastore) GetServiceParams() *rendertypes.ServiceParams {
params.Type = n.GetServiceType()

// Set service ports
params.Port = n.GetServicePort()
params.PortName = "service-port"
servicePorts := n.GetServicePorts()
if len(servicePorts) != 0 {
params.Ports = servicePorts
} else {
// Use corev1.ServicePort instead of deprecated params.Port
params.Ports = []corev1.ServicePort{{
Name: "api",
Port: n.GetServicePort(),
TargetPort: intstr.FromInt32(n.GetServicePort()),
Protocol: corev1.ProtocolTCP,
}}
}
return params
}

Expand Down Expand Up @@ -1063,11 +1089,20 @@ func (n *NemoDatastore) GetServiceMonitorParams() *rendertypes.ServiceMonitorPar
params.Labels = svcLabels
params.Annotations = n.GetServiceMonitorAnnotations()

// Determine the appropriate port for monitoring
metricsPort := getMetricsPort(n.Spec.Expose.Service)

// Set Service Monitor spec
smSpec := monitoringv1.ServiceMonitorSpec{
NamespaceSelector: monitoringv1.NamespaceSelector{MatchNames: []string{n.Namespace}},
Selector: metav1.LabelSelector{MatchLabels: n.GetServiceLabels()},
Endpoints: []monitoringv1.Endpoint{{Port: "service-port", ScrapeTimeout: serviceMonitor.ScrapeTimeout, Interval: serviceMonitor.Interval}},
Endpoints: []monitoringv1.Endpoint{
{
Port: metricsPort.StrVal,
ScrapeTimeout: serviceMonitor.ScrapeTimeout,
Interval: serviceMonitor.Interval,
},
},
}
params.SMSpec = smSpec
return params
Expand Down
Loading