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

feat/add support for hostPort in container #2763

Merged
merged 1 commit into from
Apr 9, 2024
Merged

feat/add support for hostPort in container #2763

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
16 changes: 16 additions & 0 deletions .chloggen/2763 - add hostPort support.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,16 @@
# One of 'breaking', 'deprecation', 'new_component', 'enhancement', 'bug_fix'
change_type: enhancement

# The name of the component, or a single word describing the area of concern, (e.g. collector, target allocator, auto-instrumentation, opamp, github action)
component: operator

# A brief description of the change. Surround your text with quotes ("") if it needs to start with a backtick (`).
note: "Add support for adding/extending otc-collector container ports."

# One or more tracking issues related to the change
issues: [2763]

# (Optional) One or more lines of additional information to render under the primary note.
# These lines will be padded with 2 spaces and then inserted directly into the document.
# Use pipe (|) for multiline entries.
subtext:
76 changes: 47 additions & 29 deletions apis/v1alpha1/collector_webhook_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -475,15 +475,19 @@ func TestOTELColValidatingWebhook(t *testing.T) {
thrift_http:
endpoint: 0.0.0.0:15268
`,
Ports: []v1.ServicePort{
Ports: []PortsSpec{
{
Name: "port1",
Port: 5555,
ServicePort: v1.ServicePort{
Name: "port1",
Port: 5555,
},
},
{
Name: "port2",
Port: 5554,
Protocol: v1.ProtocolUDP,
ServicePort: v1.ServicePort{
Name: "port2",
Port: 5554,
Protocol: v1.ProtocolUDP,
},
},
},
Autoscaler: &AutoscalerSpec{
Expand Down Expand Up @@ -533,15 +537,19 @@ func TestOTELColValidatingWebhook(t *testing.T) {
thrift_http:
endpoint: 0.0.0.0:15268
`,
Ports: []v1.ServicePort{
Ports: []PortsSpec{
{
Name: "port1",
Port: 5555,
ServicePort: v1.ServicePort{
Name: "port1",
Port: 5555,
},
},
{
Name: "port2",
Port: 5554,
Protocol: v1.ProtocolUDP,
ServicePort: v1.ServicePort{
Name: "port2",
Port: 5554,
Protocol: v1.ProtocolUDP,
},
},
},
Autoscaler: &AutoscalerSpec{
Expand Down Expand Up @@ -601,15 +609,19 @@ func TestOTELColValidatingWebhook(t *testing.T) {
thrift_http:
endpoint: 0.0.0.0:15268
`,
Ports: []v1.ServicePort{
Ports: []PortsSpec{
{
Name: "port1",
Port: 5555,
ServicePort: v1.ServicePort{
Name: "port1",
Port: 5555,
},
},
{
Name: "port2",
Port: 5554,
Protocol: v1.ProtocolUDP,
ServicePort: v1.ServicePort{
Name: "port2",
Port: 5554,
Protocol: v1.ProtocolUDP,
},
},
},
Autoscaler: &AutoscalerSpec{
Expand Down Expand Up @@ -687,12 +699,14 @@ func TestOTELColValidatingWebhook(t *testing.T) {
name: "invalid port name",
otelcol: OpenTelemetryCollector{
Spec: OpenTelemetryCollectorSpec{
Ports: []v1.ServicePort{
Ports: []PortsSpec{
{
// this port name contains a non alphanumeric character, which is invalid.
Name: "-test🦄port",
Port: 12345,
Protocol: v1.ProtocolTCP,
ServicePort: v1.ServicePort{
// this port name contains a non alphanumeric character, which is invalid.
Name: "-test🦄port",
Port: 12345,
Protocol: v1.ProtocolTCP,
},
},
},
},
Expand All @@ -703,10 +717,12 @@ func TestOTELColValidatingWebhook(t *testing.T) {
name: "invalid port name, too long",
otelcol: OpenTelemetryCollector{
Spec: OpenTelemetryCollectorSpec{
Ports: []v1.ServicePort{
Ports: []PortsSpec{
{
Name: "aaaabbbbccccdddd", // len: 16, too long
Port: 5555,
ServicePort: v1.ServicePort{
Name: "aaaabbbbccccdddd", // len: 16, too long
Port: 5555,
},
},
},
},
Expand All @@ -717,10 +733,12 @@ func TestOTELColValidatingWebhook(t *testing.T) {
name: "invalid port num",
otelcol: OpenTelemetryCollector{
Spec: OpenTelemetryCollectorSpec{
Ports: []v1.ServicePort{
Ports: []PortsSpec{
{
Name: "aaaabbbbccccddd", // len: 15
// no port set means it's 0, which is invalid
ServicePort: v1.ServicePort{
Name: "aaaabbbbccccddd", // len: 15
// no port set means it's 0, which is invalid
},
},
},
},
Expand Down
45 changes: 43 additions & 2 deletions apis/v1alpha1/convert.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -20,6 +20,7 @@ import (

"gopkg.in/yaml.v3"
appsv1 "k8s.io/api/apps/v1"
v1 "k8s.io/api/core/v1"
metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
"sigs.k8s.io/controller-runtime/pkg/conversion"

Expand Down Expand Up @@ -96,7 +97,7 @@ func tov1beta1(in OpenTelemetryCollector) (v1beta1.OpenTelemetryCollector, error
Image: copy.Spec.Image,
ImagePullPolicy: copy.Spec.ImagePullPolicy,
VolumeMounts: copy.Spec.VolumeMounts,
Ports: copy.Spec.Ports,
Ports: tov1beta1Ports(copy.Spec.Ports),
Env: copy.Spec.Env,
EnvFrom: copy.Spec.EnvFrom,
VolumeClaimTemplates: copy.Spec.VolumeClaimTemplates,
Expand Down Expand Up @@ -144,6 +145,26 @@ func tov1beta1(in OpenTelemetryCollector) (v1beta1.OpenTelemetryCollector, error
}, nil
}

func tov1beta1Ports(in []PortsSpec) []v1beta1.PortsSpec {
var ports []v1beta1.PortsSpec

for _, p := range in {
ports = append(ports, v1beta1.PortsSpec{
ServicePort: v1.ServicePort{
Name: p.Name,
Protocol: p.Protocol,
AppProtocol: p.AppProtocol,
Port: p.Port,
TargetPort: p.TargetPort,
NodePort: p.NodePort,
},
HostPort: p.HostPort,
})
}

return ports
}

func tov1beta1TA(in OpenTelemetryTargetAllocator) v1beta1.TargetAllocatorEmbedded {
return v1beta1.TargetAllocatorEmbedded{
Replicas: in.Replicas,
Expand Down Expand Up @@ -249,6 +270,26 @@ func tov1beta1ConfigMaps(in []ConfigMapsSpec) []v1beta1.ConfigMapsSpec {
return mapsSpecs
}

func tov1alpha1Ports(in []v1beta1.PortsSpec) []PortsSpec {
var ports []PortsSpec

for _, p := range in {
ports = append(ports, PortsSpec{
ServicePort: v1.ServicePort{
Name: p.Name,
Protocol: p.Protocol,
AppProtocol: p.AppProtocol,
Port: p.Port,
TargetPort: p.TargetPort,
NodePort: p.NodePort,
},
HostPort: p.HostPort,
})
}

return ports
}

func tov1alpha1(in v1beta1.OpenTelemetryCollector) (*OpenTelemetryCollector, error) {
copy := in.DeepCopy()
configYaml, err := copy.Spec.Config.Yaml()
Expand Down Expand Up @@ -287,7 +328,7 @@ func tov1alpha1(in v1beta1.OpenTelemetryCollector) (*OpenTelemetryCollector, err
ImagePullPolicy: copy.Spec.ImagePullPolicy,
Config: configYaml,
VolumeMounts: copy.Spec.VolumeMounts,
Ports: copy.Spec.Ports,
Ports: tov1alpha1Ports(copy.Spec.Ports),
Env: copy.Spec.Env,
EnvFrom: copy.Spec.EnvFrom,
VolumeClaimTemplates: copy.Spec.VolumeClaimTemplates,
Expand Down
7 changes: 4 additions & 3 deletions apis/v1alpha1/convert_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -177,11 +177,12 @@ func Test_tov1beta1AndBack(t *testing.T) {
Name: "aaa",
},
},
Ports: []v1.ServicePort{
{
Ports: []PortsSpec{{
ServicePort: v1.ServicePort{
Name: "otlp",
},
},
HostPort: 0,
}},
Env: []v1.EnvVar{
{
Name: "foo",
Expand Down
12 changes: 11 additions & 1 deletion apis/v1alpha1/opentelemetrycollector_types.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -186,7 +186,7 @@ type OpenTelemetryCollectorSpec struct {
// used to open additional ports that can't be inferred by the operator, like for custom receivers.
// +optional
// +listType=atomic
Ports []v1.ServicePort `json:"ports,omitempty"`
Ports []PortsSpec `json:"ports,omitempty"`
// ENV vars to set on the OpenTelemetry Collector's Pods. These can then in certain cases be
// consumed in the config file for the Collector.
// +optional
Expand Down Expand Up @@ -291,6 +291,16 @@ type OpenTelemetryCollectorSpec struct {
DeploymentUpdateStrategy appsv1.DeploymentStrategy `json:"deploymentUpdateStrategy,omitempty"`
}

// PortsSpec defines the OpenTelemetryCollector's container/service ports additional specifications.
type PortsSpec struct {
Copy link
Member

Choose a reason for hiding this comment

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

This ends up being a breaking change, so maybe I was wrong when I said it should live in the alpha api. @open-telemetry/operator-maintainers can you confirm?

Copy link
Contributor

Choose a reason for hiding this comment

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

Is it? Looks like we're just adding fields to the CRDs. I'm personally a bit worried that we're reinventing the wheel with this PortSpec definition, and the community has converged on some standard for this "ContainerPort and ServicePort in one" concept.

Copy link
Member

Choose a reason for hiding this comment

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

I put my comment in the wrong spot.

Changing from Ports []v1.ServicePort json:"ports,omitempty" to Ports []PortsSpec json:"ports,omitempty" is the breaking change I was referring to.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

hi @swiatekm-sumo

While I aggree that mixing values for different reasons can be messy, the current ports definition already present in the spec is used both to populate the Service object and the ContainerPorts object.

We could do a dedicated new field or something, but then we would have multiples places for settings being used in also multiples places.

Putting in the same port context seemed the cleanest place.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@TylerHelmuth Can you elaborate on how this is a breaking change? the interface/spec with the client remains the same, minus the new field hostPort.

Copy link
Contributor

Choose a reason for hiding this comment

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

@bmiguel-teixeira I agree with your approach here. My point was that this sounds like a problem other operators also had to solve, and I'm wondering if there's some de-facto standard way of doing it.

@TylerHelmuth PortsSpec embeds ServicePort, so this doesn't look like a breaking change from the perspective of a CRD user. Can you describe a specific situation where this would break someone?

Copy link
Member

Choose a reason for hiding this comment

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

I misinterpreted what ServicePort represented, I didn't realize it was a full struct. I agree this is being done in a non-breaking way.

Copy link
Contributor

Choose a reason for hiding this comment

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

yeah I looked around and didn't see anything from the obvious sources...

// Allows defining which port to bind to the host in the Container.
// +optional
HostPort int32 `json:"hostPort,omitempty"`

// Maintain previous fields in new struct
v1.ServicePort `json:",inline"`
}

// OpenTelemetryTargetAllocator defines the configurations for the Prometheus target allocator.
type OpenTelemetryTargetAllocator struct {
// Replicas is the number of pod instances for the underlying TargetAllocator. This should only be set to a value
Expand Down
18 changes: 17 additions & 1 deletion apis/v1alpha1/zz_generated.deepcopy.go
View file
Open in desktop

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

14 changes: 12 additions & 2 deletions apis/v1beta1/common.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -84,6 +84,16 @@ type PodDisruptionBudgetSpec struct {
MaxUnavailable *intstr.IntOrString `json:"maxUnavailable,omitempty"`
}

// PortsSpec defines the OpenTelemetryCollector's container/service ports additional specifications.
type PortsSpec struct {
// Allows defining which port to bind to the host in the Container.
// +optional
HostPort int32 `json:"hostPort,omitempty"`

// Maintain previous fields in new struct
v1.ServicePort `json:",inline"`
}

type OpenTelemetryCommonFields struct {
// ManagementState defines if the CR should be managed by the operator or not.
// Default is managed.
Expand Down Expand Up @@ -151,12 +161,12 @@ type OpenTelemetryCommonFields struct {
// +optional
// +listType=atomic
VolumeMounts []v1.VolumeMount `json:"volumeMounts,omitempty"`
// Ports allows a set of ports to be exposed by the underlying v1.Service. By default, the operator
// Ports allows a set of ports to be exposed by the underlying v1.Service & v1.ContainerPort. By default, the operator
// will attempt to infer the required ports by parsing the .Spec.Config property but this property can be
// used to open additional ports that can't be inferred by the operator, like for custom receivers.
// +optional
// +listType=atomic
Ports []v1.ServicePort `json:"ports,omitempty"`
Ports []PortsSpec `json:"ports,omitempty"`
// Environment variables to set on the generated pods.
// +optional
Env []v1.EnvVar `json:"env,omitempty"`
Expand Down
18 changes: 17 additions & 1 deletion apis/v1beta1/zz_generated.deepcopy.go
View file
Open in desktop

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

8 changes: 7 additions & 1 deletion bundle/manifests/opentelemetry.io_opentelemetrycollectors.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4184,14 +4184,20 @@ spec:
Ports allows a set of ports to be exposed by the underlying v1.Service. By default, the operator
will attempt to infer the required ports by parsing the .Spec.
items:
description: ServicePort contains information on service's port.
description: PortsSpec defines the OpenTelemetryCollector's container/service
ports additional specifications.
properties:
appProtocol:
description: |-
The application protocol for this port.
This is used as a hint for implementations to offer richer behavior for protocols that they understand.
This field follows standard Kubernetes label syntax.
type: string
hostPort:
description: Allows defining which port to bind to the host
in the Container.
format: int32
type: integer
name:
description: |-
The name of this port within the service. This must be a DNS_LABEL.
Expand Down
Loading
Loading