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

Enabling collector to mount multiple configMaps #1989

Merged
merged 19 commits into from
Sep 18, 2023
Merged
Show file tree
Hide file tree
Changes from 13 commits
Commits
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/add-more-configmaps.yaml
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. operator, target allocator, github action)
component: operator

# A brief description of the change. Surround your text with quotes ("") if it needs to start with a backtick (`).
note: Allow the collector CRD to specify a list of configmaps to mount

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

# (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:
11 changes: 11 additions & 0 deletions apis/v1alpha1/opentelemetrycollector_types.go
Original file line number Diff line number Diff line change
Expand Up @@ -264,6 +264,11 @@ type OpenTelemetryCollectorSpec struct {
// This is only relevant to statefulset, and deployment mode
// +optional
TopologySpreadConstraints []v1.TopologySpreadConstraint `json:"topologySpreadConstraints,omitempty"`

// ConfigMaps is a list of ConfigMaps in the same namespace as the OpenTelemetryCollector
// object, which shall be mounted into the Collector Pods.
// Each ConfigMap will be added to the Collector's Deployments as a volume named `configmap-<configmap-name>`.
ConfigMaps []ConfigMapsSpec `json:"configmaps,omitempty"`
Copy link
Contributor

Choose a reason for hiding this comment

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

I think we should just decide where to mount these in the collector binary rather than leaving that up to users. Prometheus says they mount at /etc/prometheus/configmaps/<configmap-name> IMO we should do something similar.

Copy link
Member

Choose a reason for hiding this comment

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

If Prometheus sets a precedence for not letting this be configurable then I am ok following that. The most robust solution though would be to allow it to be configurable and fallback to a default value if no path is provided.

Copy link
Contributor

Choose a reason for hiding this comment

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

I don't like the prometheus-operator solution. I'd argue it makes things more complex for both the user and the operator code. The user, because they need to both read the documentation and then ensure they're setting the value correctly wherever they need the file name. And for the operator, because it now has to take care of generating the mount path, and the how the path is generated is now part of our public API.

As opposed to the user just setting whatever they want and the operator applying it without any additional checks. Arguably this can lead to footguns where the mount overrides something important, but I think it's the user's responsibility to make sure this doesn't happen - and this may even be a feature in some circumstances.

All that said, if we're following prometheus-operator conventions everywhere else, we should probably do it here as well for consistency if nothing else.

Copy link
Contributor

Choose a reason for hiding this comment

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

I like this argument :) I think making it configurable with a default if not set seems like a good middle ground here. Does that work for you @swiatekm-sumo?

Copy link
Contributor

Choose a reason for hiding this comment

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

Making it configurable with a default comes with the worst of both worlds for us, but the best for the users, so I'm fine with it.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I believe the suggestion to make the configMaps' mountPaths configurable is the solution we found better for the users. So, I have changed the PR accordingly.

Copy link
Member

Choose a reason for hiding this comment

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

If the user doesn't configure the mountPath or name what happens? Do we need a validating case for that?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I don't think so, I would say both are required to mount extra configMaps.

Copy link
Member

Choose a reason for hiding this comment

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

If they are required we should validate that requirement.

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, I have changed the requirements and added some validations to the configmaps's definition.
When you get a chance could you please have a look?

}

// OpenTelemetryTargetAllocator defines the configurations for the Prometheus target allocator.
Expand Down Expand Up @@ -506,6 +511,12 @@ type MetricSpec struct {
Pods *autoscalingv2.PodsMetricSource `json:"pods,omitempty"`
}

type ConfigMapsSpec struct {
// Configmap defines name and path where the configMaps should be mounted.
Name string `json:"name,omitempty"`
MountPath string `json:"mountpath,omitempty"`
swiatekm marked this conversation as resolved.
Show resolved Hide resolved
}

func init() {
SchemeBuilder.Register(&OpenTelemetryCollector{}, &OpenTelemetryCollectorList{})
}
20 changes: 20 additions & 0 deletions apis/v1alpha1/zz_generated.deepcopy.go

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

Original file line number Diff line number Diff line change
Expand Up @@ -31,7 +31,7 @@ metadata:
categories: Logging & Tracing,Monitoring
certified: "false"
containerImage: ghcr.io/open-telemetry/opentelemetry-operator/opentelemetry-operator
createdAt: "2023-08-28T17:54:06Z"
createdAt: "2023-09-07T15:34:24Z"
description: Provides the OpenTelemetry components, including the Collector
operators.operatorframework.io/builder: operator-sdk-v1.29.0
operators.operatorframework.io/project_layout: go.kubebuilder.io/v3
Expand Down
14 changes: 14 additions & 0 deletions bundle/manifests/opentelemetry.io_opentelemetrycollectors.yaml
Original file line number Diff line number Diff line change
Expand Up @@ -2120,6 +2120,20 @@ spec:
configuration. Refer to the OpenTelemetry Collector documentation
for details.
type: string
configmaps:
description: ConfigMaps is a list of ConfigMaps in the same namespace
as the OpenTelemetryCollector object, which shall be mounted into
the Collector Pods.
items:
properties:
mountpath:
type: string
name:
description: Configmap defines name and path where the configMaps
should be mounted.
type: string
type: object
type: array
env:
description: ENV vars to set on the OpenTelemetry Collector's Pods.
These can then in certain cases be consumed in the config file for
Expand Down
14 changes: 14 additions & 0 deletions config/crd/bases/opentelemetry.io_opentelemetrycollectors.yaml
Original file line number Diff line number Diff line change
Expand Up @@ -2117,6 +2117,20 @@ spec:
configuration. Refer to the OpenTelemetry Collector documentation
for details.
type: string
configmaps:
description: ConfigMaps is a list of ConfigMaps in the same namespace
as the OpenTelemetryCollector object, which shall be mounted into
the Collector Pods.
items:
properties:
mountpath:
type: string
name:
description: Configmap defines name and path where the configMaps
should be mounted.
type: string
type: object
type: array
env:
description: ENV vars to set on the OpenTelemetry Collector's Pods.
These can then in certain cases be consumed in the config file for
Expand Down
41 changes: 41 additions & 0 deletions docs/api.md
Original file line number Diff line number Diff line change
Expand Up @@ -3688,6 +3688,13 @@ OpenTelemetryCollectorSpec defines the desired state of OpenTelemetryCollector.
Config is the raw JSON to be used as the collector's configuration. Refer to the OpenTelemetry Collector documentation for details.<br/>
</td>
<td>false</td>
</tr><tr>
<td><b><a href="#opentelemetrycollectorspecconfigmapsindex">configmaps</a></b></td>
<td>[]object</td>
<td>
ConfigMaps is a list of ConfigMaps in the same namespace as the OpenTelemetryCollector object, which shall be mounted into the Collector Pods.<br/>
</td>
<td>false</td>
</tr><tr>
<td><b><a href="#opentelemetrycollectorspecenvindex">env</a></b></td>
<td>[]object</td>
Expand Down Expand Up @@ -8082,6 +8089,40 @@ target specifies the target value for the given metric
</table>


### OpenTelemetryCollector.spec.configmaps[index]
<sup><sup>[↩ Parent](#opentelemetrycollectorspec)</sup></sup>





<table>
<thead>
<tr>
<th>Name</th>
<th>Type</th>
<th>Description</th>
<th>Required</th>
</tr>
</thead>
<tbody><tr>
<td><b>mountpath</b></td>
<td>string</td>
<td>
<br/>
</td>
<td>false</td>
</tr><tr>
<td><b>name</b></td>
<td>string</td>
<td>
Configmap defines name and path where the configMaps should be mounted.<br/>
</td>
<td>false</td>
</tr></tbody>
</table>


### OpenTelemetryCollector.spec.env[index]
<sup><sup>[↩ Parent](#opentelemetrycollectorspec)</sup></sup>

Expand Down
8 changes: 8 additions & 0 deletions internal/manifests/collector/container.go
Original file line number Diff line number Diff line change
Expand Up @@ -104,6 +104,14 @@ func Container(cfg config.Config, logger logr.Logger, otelcol v1alpha1.OpenTelem
},
})

if len(otelcol.Spec.ConfigMaps) > 0 {
for keyCfgMap := range otelcol.Spec.ConfigMaps {
volumeMounts = append(volumeMounts, corev1.VolumeMount{
yuriolisa marked this conversation as resolved.
Show resolved Hide resolved
MountPath: otelcol.Spec.ConfigMaps[keyCfgMap].MountPath + "/" + otelcol.Spec.ConfigMaps[keyCfgMap].Name,
TylerHelmuth marked this conversation as resolved.
Show resolved Hide resolved
swiatekm marked this conversation as resolved.
Show resolved Hide resolved
})
}
}

if otelcol.Spec.TargetAllocator.Enabled {
// We need to add a SHARD here so the collector is able to keep targets after the hashmod operation which is
// added by default by the Prometheus operator's config generator.
Expand Down
24 changes: 24 additions & 0 deletions internal/manifests/collector/container_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -318,6 +318,30 @@ func TestContainerCustomVolumes(t *testing.T) {
assert.Equal(t, "custom-volume-mount", c.VolumeMounts[1].Name)
}

func TestContainerCustomConfigMapsVolumes(t *testing.T) {
// prepare
otelcol := v1alpha1.OpenTelemetryCollector{
Spec: v1alpha1.OpenTelemetryCollectorSpec{
ConfigMaps: []v1alpha1.ConfigMapsSpec{{
Name: "configmap-test",
MountPath: "/var/conf",
}, {
Name: "configmap-test2",
MountPath: "/var/conf/dir",
}},
},
}
cfg := config.New()

// test
c := Container(cfg, logger, otelcol, true)

// verify
assert.Len(t, c.VolumeMounts, 3)
assert.Equal(t, "/var/conf/configmap-test", c.VolumeMounts[1].MountPath)
assert.Equal(t, "/var/conf/dir/configmap-test2", c.VolumeMounts[2].MountPath)
}

func TestContainerCustomSecurityContext(t *testing.T) {
// default config without security context
c1 := Container(config.New(), logger, v1alpha1.OpenTelemetryCollector{Spec: v1alpha1.OpenTelemetryCollectorSpec{}}, true)
Expand Down
15 changes: 15 additions & 0 deletions internal/manifests/collector/volume.go
Original file line number Diff line number Diff line change
Expand Up @@ -42,5 +42,20 @@ func Volumes(cfg config.Config, otelcol v1alpha1.OpenTelemetryCollector) []corev
volumes = append(volumes, otelcol.Spec.Volumes...)
}

if len(otelcol.Spec.ConfigMaps) > 0 {
for keyCfgMap := range otelcol.Spec.ConfigMaps {
volumes = append(volumes, corev1.Volume{
Name: "configmap_" + otelcol.Spec.ConfigMaps[keyCfgMap].Name,
VolumeSource: corev1.VolumeSource{
ConfigMap: &corev1.ConfigMapVolumeSource{
LocalObjectReference: corev1.LocalObjectReference{
Name: otelcol.Spec.ConfigMaps[keyCfgMap].Name,
},
},
},
})
}
}

return volumes
}
26 changes: 26 additions & 0 deletions internal/manifests/collector/volume_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -61,3 +61,29 @@ func TestVolumeAllowsMoreToBeAdded(t *testing.T) {
// check that it's the otc-internal volume, with the config map
assert.Equal(t, "my-volume", volumes[1].Name)
}

func TestVolumeWithMoreConfigMaps(t *testing.T) {
// prepare
otelcol := v1alpha1.OpenTelemetryCollector{
Spec: v1alpha1.OpenTelemetryCollectorSpec{
ConfigMaps: []v1alpha1.ConfigMapsSpec{{
Name: "configmap-test",
MountPath: "/var/conf",
}, {
Name: "configmap-test2",
MountPath: "/var/conf",
}},
},
}
cfg := config.New()

// test
volumes := Volumes(cfg, otelcol)

// verify
assert.Len(t, volumes, 3)

// check if the volume with the configmap prefix is mounted after defining the config map.
assert.Equal(t, "configmap_configmap-test", volumes[1].Name)
assert.Equal(t, "configmap_configmap-test2", volumes[2].Name)
}