diff --git a/pkg/k8s/resources/manifests_test.go b/pkg/k8s/resources/manifests_test.go index ed9b0d0..ad22088 100644 --- a/pkg/k8s/resources/manifests_test.go +++ b/pkg/k8s/resources/manifests_test.go @@ -16,6 +16,7 @@ import ( appsv1 "k8s.io/api/apps/v1" corev1 "k8s.io/api/core/v1" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" + "k8s.io/apimachinery/pkg/labels" "k8s.io/apimachinery/pkg/runtime" k8stesting "k8s.io/client-go/kubernetes/fake" clienttesting "k8s.io/client-go/testing" @@ -673,17 +674,66 @@ spec: deploy, err := client.AppsV1().Deployments("default").Get(context.Background(), deployName, metav1.GetOptions{}) require.NoError(t, err) - // Labels referenced by fieldRef should be preserved. - assert.Equal(t, "v1", deploy.Labels["version"], "fieldRef-referenced label 'version' should be preserved") - assert.Equal(t, "us-east-1", deploy.Labels["region"], "fieldRef-referenced label 'region' should be preserved") + // The env vars that read labels get the labels' values inline. + env := deploy.Spec.Template.Spec.Containers[0].Env + assert.Equal(t, []corev1.EnvVar{ + {Name: "APP_VERSION", Value: "v1"}, + {Name: "REGION", Value: "us-east-1"}, + }, env) + + // So no source label is kept, on the deployment or its pods. + for _, key := range []string{"app", "version", "region"} { + assert.Empty(t, deploy.Labels[key], "source label %q should be stripped", key) + assert.Empty(t, deploy.Spec.Template.Labels[key], "source pod label %q should be stripped", key) + } +} + +// A Service that selects on a label its pods also read through the downward API, +// as vercel/api's select on app.kubernetes.io/name and read SERVICE_NAME from +// it, must not select the bridge pod. +func TestStripUnreferencedLabels_ServiceDoesNotSelectBridge(t *testing.T) { + manifests := packTestManifests(t, map[string]string{ + "deploy.yaml": `apiVersion: apps/v1 +kind: Deployment +metadata: + name: my-app + labels: + app.kubernetes.io/name: my-app +spec: + selector: + matchLabels: + app.kubernetes.io/name: my-app + template: + metadata: + labels: + app.kubernetes.io/name: my-app + spec: + containers: + - name: app + image: my-app:latest + ports: + - containerPort: 3000 + env: + - name: SERVICE_NAME + valueFrom: + fieldRef: + apiVersion: v1 + fieldPath: metadata.labels['app.kubernetes.io/name'] +`, + }) + + client := k8stesting.NewSimpleClientset() + deployName, _, err := testCreateFromManifests(t, client, manifests, "default") + require.NoError(t, err) - // Non-referenced source labels should be stripped. - assert.Empty(t, deploy.Labels["app"], "non-referenced label 'app' should be stripped") + deploy, err := client.AppsV1().Deployments("default").Get(context.Background(), deployName, metav1.GetOptions{}) + require.NoError(t, err) - // Pod template labels should also preserve fieldRef-referenced labels. - podLabels := deploy.Spec.Template.Labels - assert.Equal(t, "v1", podLabels["version"]) - assert.Equal(t, "us-east-1", podLabels["region"]) + selector := labels.SelectorFromSet(labels.Set{"app.kubernetes.io/name": "my-app"}) + assert.False(t, selector.Matches(labels.Set(deploy.Spec.Template.Labels)), + "the real Service's selector matches the bridge pod: %v", deploy.Spec.Template.Labels) + assert.Contains(t, deploy.Spec.Template.Spec.Containers[0].Env, + corev1.EnvVar{Name: "SERVICE_NAME", Value: "my-app"}) } func TestStripUnreferencedLabels_InitContainerFieldRef(t *testing.T) { @@ -728,10 +778,12 @@ spec: deploy, err := client.AppsV1().Deployments("default").Get(context.Background(), deployName, metav1.GetOptions{}) require.NoError(t, err) - // Label referenced by initContainer fieldRef should be preserved. - assert.Equal(t, "backend", deploy.Labels["tier"], "fieldRef label from initContainer should be preserved") + // The initContainer's env var gets the label's value inline. + assert.Equal(t, []corev1.EnvVar{{Name: "TIER", Value: "backend"}}, + deploy.Spec.Template.Spec.InitContainers[0].Env) - // Non-referenced label should be stripped. + // So neither label is kept. + assert.Empty(t, deploy.Labels["tier"]) assert.Empty(t, deploy.Labels["app"]) } diff --git a/pkg/k8s/resources/transform_ca.go b/pkg/k8s/resources/transform_ca.go index 1be6a12..c9fa427 100644 --- a/pkg/k8s/resources/transform_ca.go +++ b/pkg/k8s/resources/transform_ca.go @@ -33,7 +33,14 @@ func InjectCA(ns string) Transformer { return fmt.Errorf("generate CA: %w", err) } + // One secret per bridge. Rename only suffixes the device ID, so a fixed + // name was shared by every bridge from a device: each create replaced the + // CA the others had mounted, concurrent creates raced to create it, and + // removing the bridge that last labeled it deleted it for all of them. secretName := "bridge-ca" + if tc.BridgeName != "" { + secretName += "-" + tc.BridgeName + } secret := &corev1.Secret{ ObjectMeta: metav1.ObjectMeta{ @@ -51,7 +58,7 @@ func InjectCA(ns string) Transformer { }) // Mount the secret into the deployment. The volume references - // secretName "bridge-ca" which SuffixNames + RewriteRefs will update. + // secretName, which SuffixNames + RewriteRefs will update. deploy, err := findApplicationDeployment(b, tc.SourceName) if err != nil { return &DeploymentNotFoundError{Name: tc.SourceName, Namespace: tc.SourceNamespace} diff --git a/pkg/k8s/resources/transform_ca_test.go b/pkg/k8s/resources/transform_ca_test.go new file mode 100644 index 0000000..b890ca3 --- /dev/null +++ b/pkg/k8s/resources/transform_ca_test.go @@ -0,0 +1,83 @@ +package resources + +import ( + "context" + "testing" + + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" + + "github.com/vercel/bridge/pkg/k8s/meta" + + appsv1 "k8s.io/api/apps/v1" + corev1 "k8s.io/api/core/v1" +) + +// TestInjectCA_SecretPerBridge verifies that two bridges from one device get +// their own CA secrets, each labeled for, and mounted by, its own bridge. A +// shared secret let one bridge's create replace another's CA and its remove +// delete it. +func TestInjectCA_SecretPerBridge(t *testing.T) { + const suffix = "-dev123" + type bridged struct { + secretName, secretDeployLabel, mountedSecret string + } + bridge := func(bridgeName string) bridged { + bundle, err := SourceFromManifests(packTestManifests(t, map[string]string{"manifests.yaml": `apiVersion: apps/v1 +kind: Deployment +metadata: + name: app +spec: + selector: + matchLabels: + app: app + template: + metadata: + labels: + app: app + spec: + containers: + - name: app + image: app:latest +`})) + require.NoError(t, err) + + tc := &TransformContext{Context: context.Background(), DeviceID: "dev123", BridgeName: bridgeName, SourceName: "app", SourceNamespace: "default"} + require.NoError(t, Transform(tc, bundle, []Transformer{ + InjectCA("default"), + Rename(func(name string) string { + if name == "app" { + return "bdg-" + bridgeName + suffix + } + return "bdg-" + name + suffix + }), + InjectLabels(), + RewriteRefs(), + })) + + var out bridged + for _, r := range bundle.Resources { + switch obj := r.Object.(type) { + case *corev1.Secret: + out.secretName = obj.Name + out.secretDeployLabel = obj.Labels[meta.LabelBridgeDeployment] + case *appsv1.Deployment: + for _, v := range obj.Spec.Template.Spec.Volumes { + if v.Name == caSecretVolumeName { + out.mountedSecret = v.Secret.SecretName + } + } + } + } + return out + } + + a, b := bridge("alpha"), bridge("beta") + + assert.Equal(t, "bdg-bridge-ca-alpha"+suffix, a.secretName) + assert.Equal(t, "bdg-bridge-ca-beta"+suffix, b.secretName) + assert.Equal(t, "bdg-alpha"+suffix, a.secretDeployLabel, "removed with its own bridge") + assert.Equal(t, "bdg-beta"+suffix, b.secretDeployLabel, "removed with its own bridge") + assert.Equal(t, a.secretName, a.mountedSecret) + assert.Equal(t, b.secretName, b.mountedSecret) +} diff --git a/pkg/k8s/resources/transforms.go b/pkg/k8s/resources/transforms.go index 1b74712..cea8e50 100644 --- a/pkg/k8s/resources/transforms.go +++ b/pkg/k8s/resources/transforms.go @@ -127,11 +127,13 @@ func filterMounts(mounts []corev1.VolumeMount, validVolumes map[string]bool) []c } // StripUnreferencedLabels returns a Transformer that removes all labels from -// Deployments (both object metadata and pod template metadata) except those -// referenced by fieldRef expressions in the pod spec (e.g. downward API env -// vars like metadata.labels['key']). This prevents existing Services from -// selecting bridge pods via inherited source labels while preserving labels -// required by the pod's runtime configuration. +// Deployments (both object metadata and pod template metadata). This prevents +// existing Services from selecting bridge pods via inherited source labels. +// Downward API env vars that read a pod label (metadata.labels['key']) first +// get that label's value inline, so the pod's runtime configuration is +// unchanged. Keeping such a label instead would let a Service that selects on +// it, as one that selects on app.kubernetes.io/name while its pods read their +// name from that label, route the real service's traffic to the bridge. // Runs before InjectLabels — operates on original source labels. func StripUnreferencedLabels() Transformer { return TransformFunc(func(_ *TransformContext, b *Bundle) error { @@ -140,6 +142,7 @@ func StripUnreferencedLabels() Transformer { if !ok { continue } + inlineLabelFieldRefs(&deploy.Spec.Template) referenced := collectFieldRefLabels(&deploy.Spec.Template.Spec) deploy.Labels = keepOnly(deploy.Labels, referenced) deploy.Spec.Template.Labels = keepOnly(deploy.Spec.Template.Labels, referenced) @@ -148,6 +151,28 @@ func StripUnreferencedLabels() Transformer { }) } +// inlineLabelFieldRefs replaces each env var that reads a pod label through +// the downward API with that label's value, as the downward API would resolve +// it: an absent label is the empty string. +func inlineLabelFieldRefs(template *corev1.PodTemplateSpec) { + inline := func(containers []corev1.Container) { + for i := range containers { + for j := range containers[i].Env { + env := &containers[i].Env[j] + if env.ValueFrom == nil || env.ValueFrom.FieldRef == nil { + continue + } + if key, ok := parseLabelFieldPath(env.ValueFrom.FieldRef.FieldPath); ok { + env.Value = template.Labels[key] + env.ValueFrom = nil + } + } + } + } + inline(template.Spec.InitContainers) + inline(template.Spec.Containers) +} + // collectFieldRefLabels scans all containers in a pod spec for env vars that // use fieldRef with metadata.labels['...'] and returns the set of label keys. func collectFieldRefLabels(podSpec *corev1.PodSpec) map[string]bool {