diff --git a/docs/how-to/deploy-aggregated-apiserver.md b/docs/how-to/deploy-aggregated-apiserver.md index e0f14eb2..dc399a9f 100644 --- a/docs/how-to/deploy-aggregated-apiserver.md +++ b/docs/how-to/deploy-aggregated-apiserver.md @@ -117,9 +117,10 @@ These resources are backed by Coder, not etcd, so some Kubernetes behavior diffe In a cluster, the aggregated API server serves a certificate signed by its own CA. Both live in the Secret `coder-k8s-apiserver-tls` in the server's namespace (type `coder.com/aggregated-apiserver-serving-ca`, label `app.kubernetes.io/component: aggregated-apiserver-serving-ca`). The certificate is valid for `coder-k8s-apiserver`, `coder-k8s-apiserver.`, `coder-k8s-apiserver..svc`, and `coder-k8s-apiserver..svc.cluster.local`. - The server creates the Secret on first start and reuses it afterwards. With several replicas, they all use the same Secret. +- If the Secret already exists as an empty placeholder (type `coder.com/aggregated-apiserver-serving-ca`, no `data` keys at all, and not `immutable`), the server fills it with a new CA instead of creating it, so it does not need `create` on Secrets. If the Secret does not exist and the server may not create Secrets, it does not start, and the log says to create the placeholder. - The serving certificate is valid for 1 year. The server checks it at startup and every 12 hours, and renews it with the same CA when less than a third of its lifetime is left. The new certificate is served without a restart. - The CA is valid for 10 years. To replace it earlier (for example after the Secret was exposed), see [Replace the CA](#replace-the-ca). -- If the Secret exists but is unusable (a missing key, unparsable PEM, a key that does not match its certificate, a serving certificate not signed by the CA, or an expired CA), the server does not start and the log names the field. Fix the Secret or delete it. +- If the Secret exists but is unusable and is not an empty placeholder (a missing key, unparsable PEM, a key that does not match its certificate, a serving certificate not signed by the CA, or an expired CA), the server does not start and the log names the field. Fix the Secret or delete it. - Outside a cluster (for example `go run`), the server serves a self-signed certificate for `localhost` instead. !!! warning "The Secret holds the CA private key" diff --git a/docs/how-to/troubleshooting.md b/docs/how-to/troubleshooting.md index 39e93996..0ac134ce 100644 --- a/docs/how-to/troubleshooting.md +++ b/docs/how-to/troubleshooting.md @@ -142,7 +142,11 @@ kubectl -n coder-system delete secret coder-k8s-apiserver-tls kubectl -n coder-system rollout restart deployment/coder-k8s ``` -A read or create error instead of a field name means the ServiceAccount cannot get or create Secrets in its namespace. +An error without a field name means the ServiceAccount is missing a permission on this Secret. The message says which: + +- `get secret …`: it may not read the Secret. Grant `get` on `coder-k8s-apiserver-tls`. +- `create secret …`: the Secret does not exist and the ServiceAccount may not create Secrets. Grant `create`, or create the empty placeholder that the message describes (the server fills it). +- `fill placeholder secret …` or `update secret …`: the ServiceAccount may not update the Secret, to fill a placeholder or to renew the serving certificate. Grant `update` on `coder-k8s-apiserver-tls`. ## Aggregated requests fail with `401 Unauthorized` or `403 Forbidden` diff --git a/internal/aggregated/servingcert/manager.go b/internal/aggregated/servingcert/manager.go index ad38e28a..dd4b516c 100644 --- a/internal/aggregated/servingcert/manager.go +++ b/internal/aggregated/servingcert/manager.go @@ -53,6 +53,9 @@ func NewManager(client kubernetes.Interface, namespace string) (*Manager, error) // Ensure loads, creates, or renews the Secret and makes it the served certificate. Invalid // trust material returns a *CorruptSecretError and is never overwritten. +// +// An existing placeholder (see IsPlaceholder) is filled with a new CA instead of being created, +// so an identity that may only get and update this one Secret can still bootstrap it. func (m *Manager) Ensure(ctx context.Context) (*Bundle, error) { if ctx == nil { return nil, fmt.Errorf("assertion failed: context must not be nil") @@ -75,6 +78,11 @@ func (m *Manager) Ensure(ctx context.Context) (*Bundle, error) { if apierrors.IsAlreadyExists(err) { continue // Another replica created it first; adopt theirs. } + if apierrors.IsForbidden(err) { + return nil, fmt.Errorf("create secret %s/%s: %w; if this identity may not create Secrets, "+ + "create an empty placeholder Secret %q of type %q (no data) and the server fills it", + m.namespace, SecretName, err, SecretName, SecretType) + } if err != nil { return nil, fmt.Errorf("create secret %s/%s: %w", m.namespace, SecretName, err) } @@ -84,6 +92,40 @@ func (m *Manager) Ensure(ctx context.Context) (*Bundle, error) { return nil, fmt.Errorf("get secret %s/%s: %w", m.namespace, SecretName, err) } + if secret.Type == SecretType && len(secret.Data) == 0 && isImmutable(secret) { + // Kubernetes forbids data changes on an immutable Secret, so it can never be filled. + return nil, corrupt(m.namespace, "empty placeholder is immutable and cannot be filled; recreate it without immutable: true") + } + if IsPlaceholder(secret) { + bundle, genErr := Generate(m.namespace, now) + if genErr != nil { + return nil, genErr + } + filled := secret.DeepCopy() + filled.Data = bundle.Data() + // Mark it like a created Secret, keeping any labels the placeholder already has. + if filled.Labels == nil { + filled.Labels = map[string]string{} + } + for k, v := range SecretLabels { + filled.Labels[k] = v + } + // Without a resourceVersion the update would be unconditional and could replace a CA + // that another replica wrote in the meantime. + if filled.ResourceVersion == "" { + return nil, fmt.Errorf("assertion failed: placeholder secret %s/%s has no resourceVersion", m.namespace, SecretName) + } + _, err = secrets.Update(ctx, filled, metav1.UpdateOptions{}) + if apierrors.IsConflict(err) { + continue // Another replica filled it first; re-read and adopt theirs. + } + if err != nil { + return nil, fmt.Errorf("fill placeholder secret %s/%s: %w", m.namespace, SecretName, err) + } + log.Info("Created aggregated API server CA and serving certificate in the placeholder Secret", "namespace", m.namespace, "secret", SecretName) + return bundle, m.serve(bundle) + } + bundle, err := Parse(secret, m.namespace, now) if err != nil { return nil, err diff --git a/internal/aggregated/servingcert/servingcert.go b/internal/aggregated/servingcert/servingcert.go index d50aeb07..4b255457 100644 --- a/internal/aggregated/servingcert/servingcert.go +++ b/internal/aggregated/servingcert/servingcert.go @@ -177,6 +177,16 @@ func (b *Bundle) issueServingCert(namespace string, now time.Time) error { return nil } +// IsPlaceholder reports whether secret is an empty placeholder the server may fill: exactly the +// managed type and zero data keys. Anything else is parsed, and invalid material stays corrupt. +func IsPlaceholder(secret *corev1.Secret) bool { + return secret != nil && secret.Type == SecretType && len(secret.Data) == 0 && !isImmutable(secret) +} + +func isImmutable(secret *corev1.Secret) bool { + return secret.Immutable != nil && *secret.Immutable +} + // Parse validates the Secret's trust material. Each failure names the field that is wrong. func Parse(secret *corev1.Secret, namespace string, now time.Time) (*Bundle, error) { if secret == nil { diff --git a/internal/aggregated/servingcert/servingcert_test.go b/internal/aggregated/servingcert/servingcert_test.go index 23795c72..e503b8ab 100644 --- a/internal/aggregated/servingcert/servingcert_test.go +++ b/internal/aggregated/servingcert/servingcert_test.go @@ -115,6 +115,158 @@ func TestEnsureCreatesMarkedSecretAndServesIt(t *testing.T) { if listener.count.Load() != 1 { t.Fatalf("listener notified %d times, want 1", listener.count.Load()) } + // The create path (used by --app=all, where the Secret does not exist yet) stays get + create. + if got := verbs(client); !slices.Equal(got, []string{"get", "create", "get"}) { + t.Fatalf("secret calls = %v, want [get create get] (the last get is this test's read)", got) + } +} + +func TestEnsureCreateForbiddenNamesPlaceholder(t *testing.T) { + client := fake.NewClientset() + client.PrependReactor("create", "secrets", func(k8stesting.Action) (bool, runtime.Object, error) { + return true, nil, apierrors.NewForbidden(schema.GroupResource{Resource: "secrets"}, "", errors.New("no create")) + }) + m := newTestManager(t, client, time.Now()) + _, err := m.Ensure(t.Context()) + if err == nil { + t.Fatal("expected an error when create is forbidden") + } + var corruptErr *CorruptSecretError + if errors.As(err, &corruptErr) || !apierrors.IsForbidden(err) { + t.Fatalf("expected a wrapped Forbidden error, got %v", err) + } + if !strings.Contains(err.Error(), "empty placeholder Secret") || !strings.Contains(err.Error(), string(SecretType)) { + t.Fatalf("error must tell how to provide a placeholder: %q", err) + } + if got := verbs(client); !slices.Equal(got, []string{"get", "create"}) { + t.Fatalf("secret calls = %v, want [get create]", got) + } +} + +func TestEnsureFillsPlaceholder(t *testing.T) { + for name, data := range map[string]map[string][]byte{"nil data": nil, "empty data": {}} { + t.Run(name, func(t *testing.T) { + client := fake.NewClientset(placeholder(corev1.Secret{Type: SecretType, Data: data})) + var updatedRV string + client.PrependReactor("update", "secrets", func(a k8stesting.Action) (bool, runtime.Object, error) { + updatedRV = a.(k8stesting.UpdateAction).GetObject().(*corev1.Secret).ResourceVersion + return false, nil, nil // let the tracker store it + }) + m := newTestManager(t, client, time.Now()) + listener := &countingListener{} + m.AddListener(listener) + + b, err := m.Ensure(t.Context()) + if err != nil { + t.Fatal(err) + } + if got := verbs(client); !slices.Equal(got, []string{"get", "update"}) { + t.Fatalf("secret calls = %v, want [get update] (never create)", got) + } + if updatedRV != "7" { + t.Fatalf("update resourceVersion = %q, want the fetched %q", updatedRV, "7") + } + stored, err := client.CoreV1().Secrets(testNS).Get(t.Context(), SecretName, metav1.GetOptions{}) + if err != nil { + t.Fatal(err) + } + if stored.Type != SecretType || stored.Labels["kept"] != "yes" { + t.Fatalf("fill must keep the placeholder's type and metadata: type %q, labels %v", stored.Type, stored.Labels) + } + for k, v := range SecretLabels { + if stored.Labels[k] != v { + t.Fatalf("filled Secret must carry managed label %s=%s like a created one; labels %v", k, v, stored.Labels) + } + } + if _, err := Parse(stored, testNS, time.Now()); err != nil { + t.Fatalf("filled Secret must be valid: %v", err) + } + cert, _ := m.CurrentCertKeyContent() + if string(cert) != string(stored.Data[CertKey]) || string(m.CABundle()) != string(b.CACertPEM) { + t.Fatal("served certificate and CA must be the ones written to the placeholder") + } + if listener.count.Load() != 1 { + t.Fatalf("listener notified %d times, want 1", listener.count.Load()) + } + }) + } +} + +func TestEnsureRefusesPlaceholderWithoutResourceVersion(t *testing.T) { + secret := placeholder(corev1.Secret{Type: SecretType}) + secret.ResourceVersion = "" + client := fake.NewClientset(secret) + m := newTestManager(t, client, time.Now()) + if _, err := m.Ensure(t.Context()); err == nil || !strings.Contains(err.Error(), "assertion failed") { + t.Fatalf("expected an assertion failure, got %v", err) + } + assertNoWrites(t, client) +} + +func TestEnsureAdoptsOtherReplicasPlaceholderFill(t *testing.T) { + now := time.Now() + theirs, err := Generate(testNS, now) + if err != nil { + t.Fatal(err) + } + client := fake.NewClientset(placeholder(corev1.Secret{Type: SecretType})) + var updates atomic.Int32 + client.PrependReactor("update", "secrets", func(k8stesting.Action) (bool, runtime.Object, error) { + updates.Add(1) + if err := client.Tracker().Update(schema.GroupVersionResource{Version: "v1", Resource: "secrets"}, secretFor(theirs), testNS); err != nil { + t.Fatal(err) + } + return true, nil, apierrors.NewConflict(schema.GroupResource{Resource: "secrets"}, SecretName, errors.New("modified")) + }) + m := newTestManager(t, client, now) + if _, err := m.Ensure(t.Context()); err != nil { + t.Fatal(err) + } + if string(m.CABundle()) != string(theirs.CACertPEM) { + t.Fatal("after a conflict the other replica's CA must be adopted") + } + if updates.Load() != 1 { + t.Fatalf("updates = %d, want 1", updates.Load()) + } +} + +// Only the exact placeholder (managed type, zero data keys) is filled. Everything else is corrupt +// and left alone. +func TestEnsureRejectsNonPlaceholderSecrets(t *testing.T) { + now := time.Now() + good, err := Generate(testNS, now.Add(-time.Hour)) + if err != nil { + t.Fatal(err) + } + immutable := true + tests := []struct { + name string + secret corev1.Secret + want string + }{ + {"immutable empty placeholder", corev1.Secret{Type: SecretType, Immutable: &immutable}, "placeholder is immutable"}, + {"opaque with no data", corev1.Secret{Type: corev1.SecretTypeOpaque}, `type is "Opaque"`}, + {"no type with no data", corev1.Secret{}, `type is ""`}, + {"TLS with no data", corev1.Secret{Type: corev1.SecretTypeTLS, Data: map[string][]byte{}}, `type is "kubernetes.io/tls"`}, + {"one empty key", corev1.Secret{Type: SecretType, Data: map[string][]byte{CACertKey: {}}}, `data["ca.crt"] is missing or empty`}, + {"only ca.crt", corev1.Secret{Type: SecretType, Data: map[string][]byte{CACertKey: good.CACertPEM}}, `data["ca.key"] is missing or empty`}, + {"unrelated key", corev1.Secret{Type: SecretType, Data: map[string][]byte{"note": []byte("x")}}, `data["ca.crt"] is missing or empty`}, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + client := fake.NewClientset(placeholder(tt.secret)) + m := newTestManager(t, client, now) + _, err := m.Ensure(t.Context()) + var corruptErr *CorruptSecretError + if !errors.As(err, &corruptErr) || !strings.Contains(err.Error(), tt.want) { + t.Fatalf("expected *CorruptSecretError naming %q, got %v", tt.want, err) + } + assertNoWrites(t, client) + if cert, _ := m.CurrentCertKeyContent(); cert != nil { + t.Fatal("nothing may be served from a Secret that is not a placeholder") + } + }) + } } func TestEnsureAdoptsValidSecretUnchanged(t *testing.T) { @@ -428,6 +580,23 @@ func secretFor(b *Bundle) *corev1.Secret { } } +// placeholder returns s as the managed Secret with resourceVersion "7" and a marker label. +func placeholder(s corev1.Secret) *corev1.Secret { + s.ObjectMeta = metav1.ObjectMeta{Name: SecretName, Namespace: testNS, ResourceVersion: "7", Labels: map[string]string{"kept": "yes"}} + return &s +} + +// verbs lists the verbs of every recorded action on Secrets, in order. +func verbs(client *fake.Clientset) []string { + var out []string + for _, a := range client.Actions() { + if a.GetResource().Resource == "secrets" { + out = append(out, a.GetVerb()) + } + } + return out +} + func assertNoWrites(t *testing.T, client *fake.Clientset) { t.Helper() for _, a := range client.Actions() { @@ -472,3 +641,13 @@ func selfSignedCA(t *testing.T, notBefore, notAfter time.Time) ([]byte, []byte) type countingListener struct{ count atomic.Int32 } func (l *countingListener) Enqueue() { l.count.Add(1) } + +func TestIsPlaceholderExcludesImmutable(t *testing.T) { + immutable, mutable := true, false + if IsPlaceholder(&corev1.Secret{Type: SecretType, Immutable: &immutable}) { + t.Fatal("an immutable Secret can never be filled, so it is not a placeholder") + } + if !IsPlaceholder(&corev1.Secret{Type: SecretType, Immutable: &mutable}) { + t.Fatal("immutable: false is still a placeholder") + } +}