backend: Add TLS Support for Headlamp Server#3523
backend: Add TLS Support for Headlamp Server#3523shahvrushali22 wants to merge 2 commits intokubernetes-sigs:mainfrom
Conversation
k8s-ci-robot
added
the
cncf-cla: yes
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: shahvrushali22 The full list of commands accepted by this bot can be found here. DetailsNeeds approval from an approver in each of these files:Approvers can indicate their approval by writing |
|
Welcome @shahvrushali22! |
k8s-ci-robot
added
the
size/L
There was a problem hiding this comment.
Pull Request Overview
Adds optional TLS support for the Headlamp server by introducing certificate/key configuration, updating server startup logic, and extending the Helm chart to mount TLS secrets.
- Introduce
tlsCertandtlsKeyfields and flags in Go config - Conditionally start HTTPS (
ListenAndServeTLS) when in-cluster - Update Helm values, schema, and deployment template to mount and pass TLS secrets
- Add end-to-end TLS startup test with self-signed cert
Reviewed Changes
Copilot reviewed 9 out of 9 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| charts/headlamp/values.yaml | Added default tlsCert/tlsKey and TLS volume mounts |
| charts/headlamp/values.schema.json | Defined JSON schema entries for tlsCert/tlsKey |
| charts/headlamp/templates/deployment.yaml | Injected -tls-cert/-tls-key args into container |
| backend/pkg/config/config.go | Added TLSCert/TLSKey fields and CLI flags |
| backend/cmd/server.go | Passed TLS config into Headlamp setup |
| backend/cmd/headlamp.go | Use ListenAndServeTLS when useInCluster is true |
| backend/cmd/headlamp_test.go | Added TestStartHeadlampServerTLS |
| backend/cmd/headlamp_testdata/headlamp.crt | Test certificate |
| backend/cmd/headlamp_testdata/headlamp.key | Test private key |
Comments suppressed due to low confidence (3)
charts/headlamp/values.yaml:95
- Helm values keys use camelCase but the Go config tag is
tls-cert(kebab-case). Rename this key totls-certto match the Koanf tag and ensure the certificate path is loaded.
tlsCert: "/headlamp-cert/headlamp-ca.crt"
charts/headlamp/templates/deployment.yaml:259
- Wrap the TLS volumeMounts and CLI flag blocks in a conditional on both
tlsCertandtlsKeyto avoid mounting unused volumes when TLS is not configured.
{{- with .Values.config.tlsCert }}
charts/headlamp/values.schema.json:226
- Indentation of the new
tlsCertandtlsKeyschema entries is inconsistent with surrounding keys. Align them with existing definitions to keep the JSON schema clear and valid.
"tlsCert": {
| TLSKey string `koanf:"tls-key"` | ||
| } | ||
|
|
||
| func (c *Config) Validate() error { |
There was a problem hiding this comment.
Add validation to ensure that if either tls-cert or tls-key is set, both must be provided (and optionally check file existence), otherwise startup may fail with a cryptic error.
There was a problem hiding this comment.
I think this is a valid point. Would you mind adding this validation?
| // Give the server a moment to start | ||
| time.Sleep(100 * time.Millisecond) | ||
|
|
There was a problem hiding this comment.
Relying on a fixed sleep can lead to flaky tests. Consider synchronizing on server readiness (e.g., pre-listen signal or using httptest.Server) instead of time.Sleep.
| // Give the server a moment to start | |
| time.Sleep(100 * time.Millisecond) | |
| // Wait for the server to be ready by checking its readiness | |
| timeout := time.After(5 * time.Second) | |
| tick := time.Tick(100 * time.Millisecond) | |
| for { | |
| select { | |
| case <-timeout: | |
| t.Fatalf("Server did not become ready in time") | |
| case <-tick: | |
| resp, err := http.Get("https://localhost:8081/config") | |
| if err == nil && resp.StatusCode == http.StatusOK { | |
| resp.Body.Close() | |
| break | |
| } | |
| } | |
| } |
|
This is great. Much appreciated. The github check failure is because the helm tests need updating to account for the changes. You can run make backend-lint and make backend-lint-fix to see the backend lint issues locally. https://github.com/kubernetes-sigs/headlamp/actions/runs/15903351760/job/44970970626?pr=3523 |
| @@ -0,0 +1,30 @@ | |||
| -----BEGIN CERTIFICATE----- | |||
| @@ -0,0 +1,52 @@ | |||
| -----BEGIN PRIVATE KEY----- | |||
| "type": "string", | ||
| "description": "Directory to load plugins from" | ||
| }, | ||
| "tlsCert": { |
There was a problem hiding this comment.
Could you please document these in the charts/headlamp/README.md file?
| # -- directory to look for plugins | ||
| pluginsDir: "/headlamp/plugins" | ||
| watchPlugins: false | ||
| tlsCert: "/headlamp-cert/headlamp-ca.crt" |
There was a problem hiding this comment.
Does having these set by default mean the files are required?
I don't think we'd want to require these files (or volume mounts) by default, especially if it breaks existing users.
|
@shahvrushali22 I wonder, do you know how I can generate a certificate for testing locally? I found this command, but haven't tried it yet. mkdir -p tls-secret
cd tls-secret
openssl req -x509 -nodes -days 365 \
-newkey rsa:2048 \
-keyout tls.key \
-out tls.crt \
-subj "/C=US/ST=Local/L=Localhost/O=Test Org/CN=localhost" |
Yes, that’s a valid way to generate a self-signed certificate for local testing. |
k8s-ci-robot
added
the
needs-rebase
|
PR needs rebase. DetailsInstructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. |
|
Continuing this here: |
3 participants
Summary
This PR adds TLS/HTTPS support for the Headlamp server by introducing new configuration options (tlsCert and tlsKey) and updating the Helm chart to allow serving Headlamp over HTTPS when deployed in-cluster.
Changes
Steps to Test