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

Add TLS support to auto-instrumentation #3338

Merged
merged 4 commits into from
Oct 11, 2024
Merged
Show file tree
Hide file tree
Changes from 1 commit
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
34 changes: 34 additions & 0 deletions .chloggen/inst-tls.yaml
Original file line number Diff line number Diff line change
@@ -0,0 +1,34 @@
# 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: auto-instrumentation

# A brief description of the change. Surround your text with quotes ("") if it needs to start with a backtick (`).
note: Add support for specifying exporter TLS certificates in auto-instrumentation.

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

# (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: |
Now Instrumentation CR supports specifying TLS certificates for exporter:
```yaml
spec:
exporter:
endpoint: https://otel-collector:4317
tls:
secretName: otel-tls-certs
configMapName: otel-ca-bundle
# otel-ca-bundle
ca: ca.crt
# present in otel-tls-certs
cert: tls.crt
# present in otel-tls-certs
key: tls.key
```

* Propagating secrets across namespaces can be done with https://github.com/EmberStack/kubernetes-reflector or https://github.com/zakkg3/ClusterSecret
* Restarting workloads on certificate renewal can be done with https://github.com/stakater/Reloader or https://github.com/wave-k8s/wave
29 changes: 29 additions & 0 deletions apis/v1alpha1/instrumentation_types.go
Original file line number Diff line number Diff line change
Expand Up @@ -97,8 +97,37 @@ type Resource struct {
// Exporter defines OTLP exporter configuration.
type Exporter struct {
// Endpoint is address of the collector with OTLP endpoint.
// If the endpoint defines https:// scheme TLS has to be specified.
// +optional
Endpoint string `json:"endpoint,omitempty"`

// TLS defines certificates for TLS.
// TLS needs to be enabled by specifying https:// scheme in the Endpoint.
TLS *TLS `json:"tls,omitempty"`
}

// TLS defines TLS configuration for exporter.
type TLS struct {
// SecretName defines secret name that will be used to configure TLS on the exporter.
// It is user responsibility to create the secret in the namespace of the workload.
// The secret must contain client certificate (Cert) and private key (Key).
// The CA certificate might be defined in the secret or in the config map.
SecretName string `json:"secretName,omitempty"`

// ConfigMapName defines configmap name with CA certificate. If it is not defined CA certificate will be
// used from the secret defined in SecretName.
ConfigMapName string `json:"configMapName,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'm not sure about this name because it is something associated to the CA directly. Maybe configMapName -> caConfigMap.


// CA defines the key of certificate (e.g. ca.crt) in the configmap map, secret or absolute path to a certificate.
// The absolute path can be used when certificate is already present on the workload filesystem e.g.
// /var/run/secrets/kubernetes.io/serviceaccount/service-ca.crt
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
// /var/run/secrets/kubernetes.io/serviceaccount/service-ca.crt
// /var/run/secrets/kubernetes.io/serviceaccount/service-ca.crt
// +optional

Copy link
Member Author

Choose a reason for hiding this comment

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

@frzifus what does the +optional do ? It does not seem to have any effect on the generated bundle or code.

We should get rid of it https://book.kubebuilder.io/reference/markers I will submit a follow up PR to remove +optional

CA string `json:"ca,omitempty"`
// Cert defines the key (e.g. tls.crt) of the client certificate in the secret or absolute path to a certificate.
// The absolute path can be used when certificate is already present on the workload filesystem.
Cert string `json:"cert,omitempty"`
// Key defines a key (e.g. tls.key) of the private key in the secret or absolute path to a certificate.
// The absolute path can be used when certificate is already present on the workload filesystem.
Key string `json:"key,omitempty"`
}

// Sampler defines sampling configuration.
Expand Down
8 changes: 8 additions & 0 deletions apis/v1alpha1/instrumentation_webhook.go
Original file line number Diff line number Diff line change
Expand Up @@ -236,6 +236,14 @@ func (w InstrumentationWebhook) validate(r *Instrumentation) (admission.Warnings
default:
return warnings, fmt.Errorf("spec.sampler.type is not valid: %s", r.Spec.Sampler.Type)
}

if r.Spec.Exporter.TLS != nil {
Copy link
Contributor

Choose a reason for hiding this comment

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

let's also add a validation that if the scheme for the endpoint contains https TLS must be defined.

Copy link
Member Author

Choose a reason for hiding this comment

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

no, this can break already existing installations. Users could configure env vars in the env section or directly on their deployments.

Copy link
Contributor

Choose a reason for hiding this comment

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

let's go the reverse and if TLS is set we should block when https is not set. If HTTPS is set, TLS doesn't need to be set.

tls := r.Spec.Exporter.TLS
if tls.Key != "" && tls.Cert == "" || tls.Cert != "" && tls.Key == "" {
warnings = append(warnings, "both exporter.tls.key and exporter.tls.cert mut be set")
}
}

return warnings, nil
}

Expand Down
51 changes: 51 additions & 0 deletions apis/v1alpha1/instrumentation_webhook_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -113,6 +113,57 @@ func TestInstrumentationValidatingWebhook(t *testing.T) {
},
},
},
{
name: "tls cert set but missing key",
inst: Instrumentation{
Spec: InstrumentationSpec{
Sampler: Sampler{
Type: ParentBasedTraceIDRatio,
Argument: "0.99",
},
Exporter: Exporter{
TLS: &TLS{
Cert: "cert",
},
},
},
},
warnings: []string{"both exporter.tls.key and exporter.tls.cert mut be set"},
},
{
name: "tls key set but missing cert",
inst: Instrumentation{
Spec: InstrumentationSpec{
Sampler: Sampler{
Type: ParentBasedTraceIDRatio,
Argument: "0.99",
},
Exporter: Exporter{
TLS: &TLS{
Key: "key",
},
},
},
},
warnings: []string{"both exporter.tls.key and exporter.tls.cert mut be set"},
},
{
name: "no warning set",
inst: Instrumentation{
Spec: InstrumentationSpec{
Sampler: Sampler{
Type: ParentBasedTraceIDRatio,
Argument: "0.99",
},
Exporter: Exporter{
TLS: &TLS{
Key: "key",
Cert: "cert",
},
},
},
},
},
}

for _, test := range tests {
Expand Down
22 changes: 21 additions & 1 deletion 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 @@ -99,7 +99,7 @@ metadata:
categories: Logging & Tracing,Monitoring
certified: "false"
containerImage: ghcr.io/open-telemetry/opentelemetry-operator/opentelemetry-operator
createdAt: "2024-10-08T09:52:53Z"
createdAt: "2024-10-10T15:31:51Z"
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 Expand Up @@ -284,7 +284,9 @@ spec:
- ""
resources:
- namespaces
- secrets
verbs:
- get
- list
- watch
- apiGroups:
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -409,6 +409,19 @@ spec:
properties:
endpoint:
type: string
tls:
properties:
ca:
type: string
cert:
type: string
configMapName:
type: string
key:
type: string
secretName:
type: string
type: object
type: object
go:
properties:
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -99,7 +99,7 @@ metadata:
categories: Logging & Tracing,Monitoring
certified: "false"
containerImage: ghcr.io/open-telemetry/opentelemetry-operator/opentelemetry-operator
createdAt: "2024-10-08T09:52:57Z"
createdAt: "2024-10-10T15:31:51Z"
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 Expand Up @@ -284,7 +284,9 @@ spec:
- ""
resources:
- namespaces
- secrets
verbs:
- get
- list
- watch
- apiGroups:
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -409,6 +409,19 @@ spec:
properties:
endpoint:
type: string
tls:
properties:
ca:
type: string
cert:
type: string
configMapName:
type: string
key:
type: string
secretName:
type: string
type: object
type: object
go:
properties:
Expand Down
13 changes: 13 additions & 0 deletions config/crd/bases/opentelemetry.io_instrumentations.yaml
Original file line number Diff line number Diff line change
Expand Up @@ -407,6 +407,19 @@ spec:
properties:
endpoint:
type: string
tls:
properties:
ca:
type: string
cert:
type: string
configMapName:
type: string
key:
type: string
secretName:
type: string
type: object
type: object
go:
properties:
Expand Down
2 changes: 2 additions & 0 deletions config/rbac/role.yaml
Original file line number Diff line number Diff line change
Expand Up @@ -30,7 +30,9 @@ rules:
- ""
resources:
- namespaces
- secrets
verbs:
- get
- list
- watch
- apiGroups:
Expand Down
75 changes: 74 additions & 1 deletion docs/api.md
Original file line number Diff line number Diff line change
Expand Up @@ -1625,7 +1625,80 @@ Exporter defines exporter configuration.
<td><b>endpoint</b></td>
<td>string</td>
<td>
Endpoint is address of the collector with OTLP endpoint.<br/>
Endpoint is address of the collector with OTLP endpoint.
If the endpoint defines https:// scheme TLS has to be specified.<br/>
</td>
<td>false</td>
</tr><tr>
<td><b><a href="#instrumentationspecexportertls">tls</a></b></td>
<td>object</td>
<td>
TLS defines certificates for TLS.
TLS needs to be enabled by specifying https:// scheme in the Endpoint.<br/>
</td>
<td>false</td>
</tr></tbody>
</table>


### Instrumentation.spec.exporter.tls
<sup><sup>[↩ Parent](#instrumentationspecexporter)</sup></sup>



TLS defines certificates for TLS.
TLS needs to be enabled by specifying https:// scheme in the Endpoint.

<table>
<thead>
<tr>
<th>Name</th>
<th>Type</th>
<th>Description</th>
<th>Required</th>
</tr>
</thead>
<tbody><tr>
<td><b>ca</b></td>
<td>string</td>
<td>
CA defines the key of certificate (e.g. ca.crt) in the configmap map, secret or absolute path to a certificate.
The absolute path can be used when certificate is already present on the workload filesystem e.g.
/var/run/secrets/kubernetes.io/serviceaccount/service-ca.crt<br/>
</td>
<td>false</td>
</tr><tr>
<td><b>cert</b></td>
<td>string</td>
<td>
Cert defines the key (e.g. tls.crt) of the client certificate in the secret or absolute path to a certificate.
The absolute path can be used when certificate is already present on the workload filesystem.<br/>
</td>
<td>false</td>
</tr><tr>
<td><b>configMapName</b></td>
<td>string</td>
<td>
ConfigMapName defines configmap name with CA certificate. If it is not defined CA certificate will be
used from the secret defined in SecretName.<br/>
</td>
<td>false</td>
</tr><tr>
<td><b>key</b></td>
<td>string</td>
<td>
Key defines a key (e.g. tls.key) of the private key in the secret or absolute path to a certificate.
The absolute path can be used when certificate is already present on the workload filesystem.<br/>
</td>
<td>false</td>
</tr><tr>
<td><b>secretName</b></td>
<td>string</td>
<td>
SecretName defines secret name that will be used to configure TLS on the exporter.
It is user responsibility to create the secret in the namespace of the workload.
The secret must contain client certificate (Cert) and private key (Key).
The CA certificate might be defined in the secret or in the config map.<br/>
</td>
<td>false</td>
</tr></tbody>
Expand Down
2 changes: 1 addition & 1 deletion internal/webhook/podmutation/webhookhandler.go
Original file line number Diff line number Diff line change
Expand Up @@ -30,7 +30,7 @@ import (
)

// +kubebuilder:webhook:path=/mutate-v1-pod,mutating=true,failurePolicy=ignore,groups="",resources=pods,verbs=create,versions=v1,name=mpod.kb.io,sideEffects=none,admissionReviewVersions=v1
// +kubebuilder:rbac:groups="",resources=namespaces,verbs=list;watch
// +kubebuilder:rbac:groups="",resources=namespaces;secrets,verbs=get;list;watch
// +kubebuilder:rbac:groups=opentelemetry.io,resources=opentelemetrycollectors,verbs=get;list;watch
// +kubebuilder:rbac:groups=opentelemetry.io,resources=instrumentations,verbs=get;list;watch
// +kubebuilder:rbac:groups="apps",resources=replicasets,verbs=get;list;watch
Expand Down
Loading
Loading