Expose container ports on the collector pod #1070

kristinapathak · 2022-09-01T00:41:46Z

fixes #1011, adding a way to expose container ports for service and pod monitoring.

Uses functions from this package to validate port name and number. If the name is too long, we cut it off to fit.

pkg/collector/container.go

apis/v1alpha1/opentelemetrycollector_types.go

pkg/collector/container.go

pkg/collector/adapters/config_to_ports_test.go

pkg/collector/container.go

secustor · 2022-09-09T07:50:54Z

pkg/collector/parser/receiver.go

+	// ignore kubeletstats receiver as it holds the field key endpoint, and it
+	// is a scraper, we only expose endpoint through k8s service objects for
+	// receivers that aren't scrapers.
+	case name == "kubeletstats":


IMO this there should be a config option, with sane defaults, to control which receivers types should be ignored.
I'm thinking about custom build instances of the collector which could contain receivers not available in contrib.

singleContainerPortFromConfigEndpoint() is based on singlePortFromConfigEndpoint() so both functions have this logic. Would you prefer that I add configuration for this in this PR? I'm also happy to open an issue and fix this separately.

@pavolloffay What do you think about making this configurable?

IMO this should be a separate issue given this logic is already in the operator. The concern is valid, but i think adding the logic to this PR is going to expand the scope of this.

I created an issue for this:
#1095

pkg/collector/container_test.go

pavolloffay · 2022-09-12T16:29:06Z

apis/v1alpha1/opentelemetrycollector_types.go

@@ -81,6 +81,13 @@ type OpenTelemetryCollectorSpec struct {
 	// +optional
 	// +listType=atomic
 	Ports []v1.ServicePort `json:"ports,omitempty"`
+	// ContainerPorts allows a set of ports to be exposed by the underlying v1.Container. 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.  Any ports


like for custom receivers.

I would assume that custom receiver ports should be defined in the spec.ports and the container ports are rather used for the ports that should not be exposed in the service.

Do we even need to have service ports and container ports? We could use the service ports to open the ports on the container. Is there any use-case when container ports != service ports?

I'm not sure - I included this for the flexibility and because the types are different. I can change it so we parse the configured service ports into container ports if that's the direction we want to go in?

@pavolloffay, do you want me to change the logic to parse the container ports from the list of service ports? Or are you waiting for others' input on this?

If there is no hard requirement to expose additional configuration in the CR then I would opt-in for deriving the ports automatically for now

pavolloffay

@kevinearls could you please review as well? You were looking into this in the past.

pkg/collector/container.go

pavolloffay · 2022-09-20T08:11:01Z

pkg/collector/container.go

+		nameErrs := validation.IsValidPortName(truncName)
+		numErrs := validation.IsValidPortNum(int(p.Port))
+		if len(nameErrs) > 0 || len(numErrs) > 0 {
+			logger.Error(errInvalidPort, "dropping container port", "port.name", truncName, "port.num", p.Port,


I wonder if we should rather emit k8s event to let the user know that the validation failed.

@kevinearls you did some logging-related work recently.

Do you think we should use k8s events here or normal logs?

@pavolloffay I don't really know, to be honest. I think logging would be fine but we are not very consistent about this.

pavolloffay · 2022-09-20T08:16:10Z

pkg/collector/container.go

 // Container builds a container for the given collector.
 func Container(cfg config.Config, logger logr.Logger, otelcol v1alpha1.OpenTelemetryCollector) corev1.Container {
 	image := otelcol.Spec.Image
 	if len(image) == 0 {
 		image = cfg.CollectorImage()
 	}

+	// build container ports from service ports
+	ports := getConfigContainerPorts(logger, otelcol.Spec.Config)
+	for _, p := range otelcol.Spec.Ports {


This function does a couple of different things:

name truncation

validation

create a container port

The truncation and validation should be done in the defaulting and validating webhook. The otelcol spec received in this function should be valid and prepared for use.

The validating webhook can as well return user-facing error if the name validation fails.

I can't add the current truncation to the defaulting webhook because it causes an import cycle:

package github.com/open-telemetry/opentelemetry-operator/apis/v1alpha1 imports github.com/open-telemetry/opentelemetry-operator/pkg/naming imports github.com/open-telemetry/opentelemetry-operator/apis/v1alpha1: import cycle not allowed

Is it reasonable for us to error out when a Spec Port is too long (like any other port naming error)? Or should I truncate a different way?

If the user can change the spec in a meaningful way to meet the requirements than returning the error is fine

pavolloffay · 2022-09-20T08:17:54Z

pkg/collector/container_test.go

+			specPorts: []corev1.ServicePort{
+				{
+					// this port name contains a non alphanumeric character, which is invalid.
+					Name:     "-test🦄port",


kevinearls

LGTM

jaronoff97

I think the only change to be made is move some of the validation / mutation to the corresponding webhooks, otherwise this looks great. Awesome stuff 🥳

kristinapathak · 2022-09-27T03:13:46Z

apis/v1alpha1/opentelemetrycollector_webhook_test.go

+				assert.NoError(t, err)
+				return
+			}
+			assert.ErrorContains(t, err, test.expectedErr)


This is error checking against a string value which isn't ideal. Some other options are:

Create errors that are wrapped in the returned fmt.Errorf so that I can use assert.ErrorIs().

Check that the error isn't nil instead of what its value is: assert.Error().

kristinapathak · 2022-09-27T03:15:40Z

pkg/collector/container.go

+			truncName := naming.Truncate(p.Name, maxPortLen)
+			if p.Name != truncName {
+				logger.Info("truncating container port name",
+					"port.name.prev", p.Name, "port.name.new", truncName)
+			}
+			nameErrs := validation.IsValidPortName(truncName)
+			numErrs := validation.IsValidPortNum(int(p.Port))
+			if len(nameErrs) > 0 || len(numErrs) > 0 {
+				logger.Error(errInvalidPort, "dropping container port", "port.name", truncName, "port.num", p.Port,
+					"port.name.errs", nameErrs, "num.errs", numErrs)
+				continue
+			}


I might be able to move this truncation+validation to inside the adapters.ConfigToReceiverPorts() function - what do you think, @pavolloffay?

validation and truncation can definitely live in other package to avoid import cycle

I tried to move it and it caused an import cycle. 😞 So I'm just going to leave the code as it is.

kristinapathak · 2022-09-28T17:46:48Z

apis/v1alpha1/opentelemetrycollector_webhook.go

 	// validate autoscale with horizontal pod autoscaler
 	if r.Spec.MaxReplicas != nil {
-		if *r.Spec.MaxReplicas < int32(1) {
+		if *r.Spec.MaxReplicas <= int32(1) {


I changed this to match the error string explanation (max replicas should be more than one) but I don't think there's a problem if maxReplicas is 1? I can change the error string instead of changing this expression if others agree.

I'm not aware of any limitations regarding MaxReplicas and I think rather the error message should be changed.

This should be ideally changed in a separate PR.

Moved: #1136

I will remove these changes from this PR.

secustor · 2022-09-29T07:51:35Z

apis/v1alpha1/opentelemetrycollector_webhook.go

 	// validate autoscale with horizontal pod autoscaler
 	if r.Spec.MaxReplicas != nil {
-		if *r.Spec.MaxReplicas < int32(1) {
+		if *r.Spec.MaxReplicas <= int32(1) {


I'm not aware of any limitations regarding MaxReplicas and I think rather the error message should be changed.

* expose container ports * get container ports from service ports * add, fix tests * move logic changes to their own branch

kristinapathak force-pushed the expose-container-ports branch 2 times, most recently from 17c405b to ede60ac Compare September 2, 2022 00:44

kristinapathak marked this pull request as ready for review September 8, 2022 03:04

kristinapathak requested a review from a team September 8, 2022 03:04

jaronoff97 reviewed Sep 8, 2022

View reviewed changes

pkg/collector/container.go Outdated Show resolved Hide resolved

apis/v1alpha1/opentelemetrycollector_types.go Show resolved Hide resolved

pkg/collector/container.go Outdated Show resolved Hide resolved

github-advanced-security bot found potential problems Sep 8, 2022

View reviewed changes

pkg/collector/container.go Fixed Show fixed Hide fixed

secustor reviewed Sep 9, 2022

View reviewed changes

pavolloffay reviewed Sep 12, 2022

View reviewed changes

kristinapathak mentioned this pull request Sep 13, 2022

Configurable receiver types to ignore when parsing ports #1095

Open

kristinapathak requested review from pavolloffay and jaronoff97 and removed request for pavolloffay and jaronoff97 September 20, 2022 02:21

pavolloffay reviewed Sep 20, 2022

View reviewed changes

kevinearls approved these changes Sep 20, 2022

View reviewed changes

jaronoff97 reviewed Sep 20, 2022

View reviewed changes

jaronoff97 approved these changes Sep 20, 2022

View reviewed changes

kristinapathak force-pushed the expose-container-ports branch 2 times, most recently from ed81ddc to dcdd998 Compare September 27, 2022 03:08

kristinapathak commented Sep 27, 2022

View reviewed changes

kristinapathak requested a review from pavolloffay September 27, 2022 03:16

kristinapathak commented Sep 28, 2022

View reviewed changes

kristinapathak mentioned this pull request Sep 30, 2022

REQUEST: New membership for kristinapathak open-telemetry/community#1213

Closed

6 tasks

secustor approved these changes Oct 1, 2022

View reviewed changes

kristinapathak mentioned this pull request Oct 3, 2022

Add missing nil checks in collector validating webhook #1136

Merged

Kristina Pathak added 4 commits October 3, 2022 11:23

expose container ports

38ddff7

get container ports from service ports

5ffaf6a

add, fix tests

a76c99f

move logic changes to their own branch

322ee43

kristinapathak force-pushed the expose-container-ports branch from fe921c2 to 322ee43 Compare October 3, 2022 22:40

pavolloffay changed the title ~~expose container ports~~ Expose container ports on the collector pod Oct 4, 2022

pavolloffay approved these changes Oct 4, 2022

View reviewed changes

pavolloffay merged commit 10404eb into open-telemetry:main Oct 4, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Expose container ports on the collector pod #1070

Expose container ports on the collector pod #1070

kristinapathak commented Sep 1, 2022 •

edited

Loading

secustor Sep 9, 2022

kristinapathak Sep 9, 2022

secustor Sep 12, 2022

jaronoff97 Sep 12, 2022

kristinapathak Sep 13, 2022

pavolloffay Sep 12, 2022

kristinapathak Sep 12, 2022

kristinapathak Sep 14, 2022

pavolloffay Sep 15, 2022

pavolloffay left a comment

pavolloffay Sep 20, 2022

pavolloffay Sep 20, 2022

kevinearls Sep 20, 2022

pavolloffay Sep 20, 2022

pavolloffay Sep 20, 2022

kristinapathak Sep 27, 2022

pavolloffay Sep 30, 2022

pavolloffay Sep 20, 2022

kevinearls left a comment

jaronoff97 left a comment

kristinapathak Sep 27, 2022

kristinapathak Sep 27, 2022

pavolloffay Sep 30, 2022

kristinapathak Oct 3, 2022

kristinapathak Sep 28, 2022 •

edited

Loading

secustor Sep 29, 2022

pavolloffay Sep 30, 2022

kristinapathak Oct 3, 2022

secustor Sep 29, 2022

Expose container ports on the collector pod #1070

Expose container ports on the collector pod #1070

Conversation

kristinapathak commented Sep 1, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pavolloffay left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kevinearls left a comment

Choose a reason for hiding this comment

jaronoff97 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kristinapathak Sep 28, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kristinapathak commented Sep 1, 2022 •

edited

Loading

kristinapathak Sep 28, 2022 •

edited

Loading