-
Notifications
You must be signed in to change notification settings - Fork 87
mount trusted CA bundle in runner pods #1258
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
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -2,6 +2,7 @@ package handlers | |
|
|
||
| import ( | ||
| "context" | ||
| "fmt" | ||
| "testing" | ||
|
|
||
| "ambient-code-operator/internal/config" | ||
|
|
@@ -13,6 +14,7 @@ import ( | |
| "k8s.io/apimachinery/pkg/runtime" | ||
| k8stypes "k8s.io/apimachinery/pkg/types" | ||
| "k8s.io/client-go/kubernetes/fake" | ||
| clienttesting "k8s.io/client-go/testing" | ||
| ) | ||
|
|
||
| // setupTestClient initializes a fake Kubernetes client for testing | ||
|
|
@@ -568,6 +570,193 @@ func TestDeleteAmbientVertexSecret_NotFound(t *testing.T) { | |
| } | ||
| } | ||
|
|
||
| // TestApplyTrustedCABundle_ConfigMapPresent verifies that applyTrustedCABundle adds the volume | ||
| // and VolumeMount when the trusted-ca-bundle ConfigMap exists in the session namespace. | ||
| func TestApplyTrustedCABundle_ConfigMapPresent(t *testing.T) { | ||
| cm := &corev1.ConfigMap{ | ||
| ObjectMeta: metav1.ObjectMeta{ | ||
| Name: types.TrustedCABundleConfigMapName, | ||
| Namespace: "session-ns", | ||
| }, | ||
| Data: map[string]string{ | ||
| "ca-bundle.crt": "--- fake CA data ---", | ||
| }, | ||
| } | ||
| setupTestClient(cm) | ||
|
|
||
| pod := &corev1.Pod{ | ||
| Spec: corev1.PodSpec{ | ||
| Containers: []corev1.Container{ | ||
| {Name: "ambient-code-runner"}, | ||
| }, | ||
| }, | ||
| } | ||
|
|
||
| applyTrustedCABundle(config.K8sClient, "session-ns", pod) | ||
|
|
||
| if len(pod.Spec.Volumes) != 1 { | ||
| t.Fatalf("expected 1 volume, got %d", len(pod.Spec.Volumes)) | ||
| } | ||
| vol := pod.Spec.Volumes[0] | ||
| if vol.Name != "trusted-ca-bundle" { | ||
| t.Errorf("expected volume name 'trusted-ca-bundle', got %q", vol.Name) | ||
| } | ||
| if vol.ConfigMap == nil || vol.ConfigMap.Name != types.TrustedCABundleConfigMapName { | ||
| t.Errorf("expected ConfigMap volume sourced from %q", types.TrustedCABundleConfigMapName) | ||
| } | ||
|
|
||
| mounts := pod.Spec.Containers[0].VolumeMounts | ||
| if len(mounts) != 1 { | ||
| t.Fatalf("expected 1 VolumeMount, got %d", len(mounts)) | ||
| } | ||
| m := mounts[0] | ||
| if m.Name != "trusted-ca-bundle" { | ||
| t.Errorf("expected mount name 'trusted-ca-bundle', got %q", m.Name) | ||
| } | ||
| if m.MountPath != "/etc/pki/tls/certs/ca-bundle.crt" { | ||
| t.Errorf("unexpected MountPath: %q", m.MountPath) | ||
| } | ||
| if m.SubPath != "ca-bundle.crt" { | ||
| t.Errorf("expected SubPath 'ca-bundle.crt', got %q", m.SubPath) | ||
| } | ||
| if !m.ReadOnly { | ||
| t.Error("expected ReadOnly=true") | ||
| } | ||
| } | ||
|
|
||
| // TestApplyTrustedCABundle_ConfigMapAbsent verifies that applyTrustedCABundle leaves the pod | ||
| // unchanged when the trusted-ca-bundle ConfigMap is not present in the session namespace. | ||
| func TestApplyTrustedCABundle_ConfigMapAbsent(t *testing.T) { | ||
| setupTestClient() // no ConfigMap | ||
|
|
||
| pod := &corev1.Pod{ | ||
| Spec: corev1.PodSpec{ | ||
| Containers: []corev1.Container{ | ||
| {Name: "ambient-code-runner"}, | ||
| }, | ||
| }, | ||
| } | ||
|
|
||
| applyTrustedCABundle(config.K8sClient, "session-ns", pod) | ||
|
|
||
| if len(pod.Spec.Volumes) != 0 { | ||
| t.Errorf("expected no volumes, got %d", len(pod.Spec.Volumes)) | ||
| } | ||
| if len(pod.Spec.Containers[0].VolumeMounts) != 0 { | ||
| t.Errorf("expected no VolumeMounts, got %d", len(pod.Spec.Containers[0].VolumeMounts)) | ||
| } | ||
| } | ||
|
|
||
| // TestApplyTrustedCABundle_ExistingMountsPreserved verifies that applyTrustedCABundle appends | ||
| // to, rather than replacing, existing VolumeMounts on the runner container. | ||
| func TestApplyTrustedCABundle_ExistingMountsPreserved(t *testing.T) { | ||
| cm := &corev1.ConfigMap{ | ||
| ObjectMeta: metav1.ObjectMeta{ | ||
| Name: types.TrustedCABundleConfigMapName, | ||
| Namespace: "session-ns", | ||
| }, | ||
| Data: map[string]string{ | ||
| "ca-bundle.crt": "--- fake CA data ---", | ||
| }, | ||
| } | ||
| setupTestClient(cm) | ||
|
|
||
| existingMount := corev1.VolumeMount{ | ||
| Name: "runner-token", | ||
| MountPath: "/var/run/secrets/ambient", | ||
| ReadOnly: true, | ||
| } | ||
| pod := &corev1.Pod{ | ||
| Spec: corev1.PodSpec{ | ||
| Volumes: []corev1.Volume{ | ||
| {Name: "runner-token"}, | ||
| }, | ||
| Containers: []corev1.Container{ | ||
| { | ||
| Name: "ambient-code-runner", | ||
| VolumeMounts: []corev1.VolumeMount{existingMount}, | ||
| }, | ||
| }, | ||
| }, | ||
| } | ||
|
|
||
| applyTrustedCABundle(config.K8sClient, "session-ns", pod) | ||
|
|
||
| if len(pod.Spec.Volumes) != 2 { | ||
| t.Fatalf("expected 2 volumes (runner-token + trusted-ca-bundle), got %d", len(pod.Spec.Volumes)) | ||
| } | ||
| mounts := pod.Spec.Containers[0].VolumeMounts | ||
| if len(mounts) != 2 { | ||
| t.Fatalf("expected 2 VolumeMounts, got %d", len(mounts)) | ||
| } | ||
| // Existing mount must still be at index 0 | ||
| if mounts[0].Name != "runner-token" { | ||
| t.Errorf("expected first mount to be 'runner-token', got %q", mounts[0].Name) | ||
| } | ||
| if mounts[1].Name != "trusted-ca-bundle" { | ||
| t.Errorf("expected second mount to be 'trusted-ca-bundle', got %q", mounts[1].Name) | ||
| } | ||
| } | ||
|
|
||
| // TestApplyTrustedCABundle_MissingKey verifies that applyTrustedCABundle leaves the pod | ||
| // unchanged when the ConfigMap exists but lacks the ca-bundle.crt key. | ||
| func TestApplyTrustedCABundle_MissingKey(t *testing.T) { | ||
| cm := &corev1.ConfigMap{ | ||
| ObjectMeta: metav1.ObjectMeta{ | ||
| Name: types.TrustedCABundleConfigMapName, | ||
| Namespace: "session-ns", | ||
| }, | ||
| Data: map[string]string{ | ||
| "wrong-key.pem": "--- fake CA data ---", | ||
| }, | ||
| } | ||
| setupTestClient(cm) | ||
|
|
||
| pod := &corev1.Pod{ | ||
| Spec: corev1.PodSpec{ | ||
| Containers: []corev1.Container{ | ||
| {Name: "ambient-code-runner"}, | ||
| }, | ||
| }, | ||
| } | ||
|
|
||
| applyTrustedCABundle(config.K8sClient, "session-ns", pod) | ||
|
|
||
| if len(pod.Spec.Volumes) != 0 { | ||
| t.Errorf("expected no volumes when key is missing, got %d", len(pod.Spec.Volumes)) | ||
| } | ||
| if len(pod.Spec.Containers[0].VolumeMounts) != 0 { | ||
| t.Errorf("expected no VolumeMounts when key is missing, got %d", len(pod.Spec.Containers[0].VolumeMounts)) | ||
| } | ||
| } | ||
|
|
||
| // TestApplyTrustedCABundle_APIError verifies that applyTrustedCABundle leaves the pod | ||
| // unchanged when the ConfigMap GET returns a non-NotFound error. | ||
| func TestApplyTrustedCABundle_APIError(t *testing.T) { | ||
| fakeClient := fake.NewSimpleClientset() | ||
| fakeClient.PrependReactor("get", "configmaps", func(action clienttesting.Action) (bool, runtime.Object, error) { | ||
| return true, nil, fmt.Errorf("connection refused") | ||
| }) | ||
| config.K8sClient = fakeClient | ||
|
|
||
| pod := &corev1.Pod{ | ||
| Spec: corev1.PodSpec{ | ||
| Containers: []corev1.Container{ | ||
| {Name: "ambient-code-runner"}, | ||
| }, | ||
| }, | ||
| } | ||
|
|
||
| applyTrustedCABundle(config.K8sClient, "session-ns", pod) | ||
|
|
||
| if len(pod.Spec.Volumes) != 0 { | ||
| t.Errorf("expected no volumes on API error, got %d", len(pod.Spec.Volumes)) | ||
| } | ||
| if len(pod.Spec.Containers[0].VolumeMounts) != 0 { | ||
| t.Errorf("expected no VolumeMounts on API error, got %d", len(pod.Spec.Containers[0].VolumeMounts)) | ||
| } | ||
| } | ||
|
Comment on lines
+573
to
+758
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Tests miss required coverage for These new tests only verify Proposed test adjustment func TestApplyTrustedCABundle_ConfigMapPresent(t *testing.T) {
@@
pod := &corev1.Pod{
Spec: corev1.PodSpec{
Containers: []corev1.Container{
+ {Name: "init-hydrate"},
+ {Name: "state-sync"},
{Name: "ambient-code-runner"},
},
},
}
@@
- mounts := pod.Spec.Containers[0].VolumeMounts
- if len(mounts) != 1 {
- t.Fatalf("expected 1 VolumeMount, got %d", len(mounts))
- }
- m := mounts[0]
- if m.Name != "trusted-ca-bundle" {
- t.Errorf("expected mount name 'trusted-ca-bundle', got %q", m.Name)
- }
- if m.MountPath != "/etc/pki/tls/certs/ca-bundle.crt" {
- t.Errorf("unexpected MountPath: %q", m.MountPath)
- }
- if m.SubPath != "ca-bundle.crt" {
- t.Errorf("expected SubPath 'ca-bundle.crt', got %q", m.SubPath)
- }
- if !m.ReadOnly {
- t.Error("expected ReadOnly=true")
- }
+ expected := map[string]bool{
+ "init-hydrate": false,
+ "state-sync": false,
+ "ambient-code-runner": false,
+ }
+ for _, c := range pod.Spec.Containers {
+ for _, m := range c.VolumeMounts {
+ if m.Name == "trusted-ca-bundle" &&
+ m.MountPath == "/etc/pki/tls/certs/ca-bundle.crt" &&
+ m.SubPath == "ca-bundle.crt" &&
+ m.ReadOnly {
+ if _, ok := expected[c.Name]; ok {
+ expected[c.Name] = true
+ }
+ }
+ }
+ }
+ for name, ok := range expected {
+ if !ok {
+ t.Fatalf("expected trusted-ca-bundle mount on container %q", name)
+ }
+ }
}🤖 Prompt for AI Agents |
||
|
|
||
| // TestDeleteAmbientVertexSecret_NilAnnotations tests handling of secret with nil annotations | ||
| func TestDeleteAmbientVertexSecret_NilAnnotations(t *testing.T) { | ||
| secret := &corev1.Secret{ | ||
|
|
||
Uh oh!
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Mount the bundle into
init-hydrateandstate-synctoo.This only patches
ambient-code-runner.init-hydratelives inpod.Spec.InitContainersand is configured with repo/S3 inputs (Lines 901-946), andstate-syncis configured for S3 (Lines 1230-1248). On private-CA clusters, those containers can still fail TLS before the main runner starts, so private repo hydration/state sync is still broken unless they get the same trust bundle when present.🤖 Prompt for AI Agents
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@coderabbitai how can those require the CA?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@ktdreyerGood challenge — it depends on the deployment:init-hydrate's backend API call (BACKEND_API_URL) uses plainhttp://— no TLS at all, so definitely no CA needed there.s3Endpoint) — if it's internal MinIO over HTTP (the typical default here), no CA needed. But if an operator configures an internal HTTPS S3 endpoint backed by a corporate CA,init-hydrateandstate-syncwould fail TLS.init-hydrate— if repos are hosted on an internal Gitlab/Gitea with a private CA cert, cloning over HTTPS would fail without the bundle.So the containers can need it, but only when:
If your target environments use plain-HTTP MinIO and public-CA git hosts, the runner container is the only one making connections (to Anthropic/Claude APIs, etc.) that realistically need the bundle. In that case you can safely ignore my comment. Your call on whether the edge cases (HTTPS S3, private git) are in scope for this PR.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Neither of those conditions are true. We'll ignore this for now.