From 453b7b2b9f5fbd1cd0c02b877eee750bde7b82d4 Mon Sep 17 00:00:00 2001 From: CrystalChun Date: Wed, 7 Oct 2026 14:03:41 -0500 Subject: [PATCH] OSAC-4909: Reflect vault provisioning failures in tenant status MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit When `ensureVaultNamespace` fails (e.g., OpenBAO is unreachable), the error propagated up to `Run()` which returned before calling `Tenants/Update`. This left the tenant stuck in `PENDING` or `SYNCED` state with no indication of failure, causing an infinite retry loop with no user-visible status change. Modified `ensureVaultNamespace` to handle errors by: - Updating the `VaultReady` condition to `FALSE` with reason `ProvisionFailed` - Returning `nil` instead of the error, so the status update is persisted Added early return guards after the `ensureVaultNamespace` call in both: - `syncToIDP` — prevents `persistBreakGlassSecret` and subsequent operations from overwriting the `FAILED` state with `SYNCED` - `update` (SYNCED path) — prevents `checkDefaultNetworkingReadiness` from running after a vault failure This follows the same error-handling pattern used for `CreateTenant` failures. Assisted-by: Claude Code --- .../tenant/tenant_compute_readiness_test.go | 26 ++-- .../tenant/tenant_reconciler_function.go | 13 +- .../tenant/tenant_reconciler_function_test.go | 122 +++++++++++++++++- 3 files changed, 151 insertions(+), 10 deletions(-) diff --git a/fulfillment-service/internal/controllers/tenant/tenant_compute_readiness_test.go b/fulfillment-service/internal/controllers/tenant/tenant_compute_readiness_test.go index fdd37271eb..709be5597a 100644 --- a/fulfillment-service/internal/controllers/tenant/tenant_compute_readiness_test.go +++ b/fulfillment-service/internal/controllers/tenant/tenant_compute_readiness_test.go @@ -320,6 +320,12 @@ var _ = Describe("Tenant compute infrastructure readiness", func() { failure := errors.New("subsystem unavailable") if subsystem == "IDP" { idpClient.EXPECT().GetTenant(gomock.Any(), "tenant-a").Return(nil, failure) + observe(clientWith(object(osacv1alpha1.TenantPhaseReady))) + Expect(reconciler.Run(ctx, tenant)).To(MatchError(ContainSubstring("subsystem unavailable"))) + Expect(tenants.updates).To(HaveLen(1)) + saved := tenants.updates[0].GetObject() + Expect(saved.GetStatus().GetState()).To(Equal(privatev1.TenantState_TENANT_STATE_SYNCED)) + Expect(condition(saved).GetStatus()).To(Equal(ready)) } else { idpClient.EXPECT().GetTenant(gomock.Any(), "tenant-a").Return(&idp.Tenant{Name: "tenant-a"}, nil) if subsystem == "vault" { @@ -327,20 +333,24 @@ var _ = Describe("Tenant compute infrastructure readiness", func() { vaultClient := vault.NewMockLifecycleClient(ctrl) reconciler.vaultLifecycle = vaultClient vaultClient.EXPECT().EnsureTenantNamespace(gomock.Any(), "tenant-a").Return(failure) + observe(clientWith(object(osacv1alpha1.TenantPhaseReady))) + Expect(reconciler.Run(ctx, tenant)).To(Succeed()) + Expect(tenants.updates).To(HaveLen(1)) + saved := tenants.updates[0].GetObject() + Expect(saved.GetStatus().GetState()).To(Equal(privatev1.TenantState_TENANT_STATE_SYNCED)) + Expect(condition(saved).GetStatus()).To(Equal(ready)) } else { vn := NewMockVirtualNetworksClient(ctrl) reconciler.virtualNetworksClient = vn vn.EXPECT().List(gomock.Any(), gomock.Any()).Return(nil, failure) + observe(clientWith(object(osacv1alpha1.TenantPhaseReady))) + Expect(reconciler.Run(ctx, tenant)).To(MatchError(ContainSubstring("subsystem unavailable"))) + Expect(tenants.updates).To(HaveLen(1)) + saved := tenants.updates[0].GetObject() + Expect(saved.GetStatus().GetState()).To(Equal(privatev1.TenantState_TENANT_STATE_SYNCED)) + Expect(condition(saved).GetStatus()).To(Equal(ready)) } } - original := proto.Clone(tenant).(*privatev1.Tenant) - observe(clientWith(object(osacv1alpha1.TenantPhaseReady))) - Expect(reconciler.Run(ctx, tenant)).To(MatchError(ContainSubstring("subsystem unavailable"))) - Expect(tenants.updates).To(HaveLen(1)) - saved := tenants.updates[0].GetObject() - Expect(saved.GetStatus().GetState()).To(Equal(privatev1.TenantState_TENANT_STATE_SYNCED)) - Expect(condition(saved).GetStatus()).To(Equal(ready)) - Expect(proto.Equal(saved.GetStatus().GetConditions()[0], original.GetStatus().GetConditions()[0])).To(BeTrue()) }, Entry("IDP error", "IDP"), Entry("vault error", "vault"), Entry("network error", "network")) It("does not poll infrastructure during tenant deletion", func() { tenant.GetMetadata().SetDeletionTimestamp(timestamppb.Now()) diff --git a/fulfillment-service/internal/controllers/tenant/tenant_reconciler_function.go b/fulfillment-service/internal/controllers/tenant/tenant_reconciler_function.go index 7fa3ac8164..62149a7e0a 100644 --- a/fulfillment-service/internal/controllers/tenant/tenant_reconciler_function.go +++ b/fulfillment-service/internal/controllers/tenant/tenant_reconciler_function.go @@ -241,6 +241,9 @@ func (t *task) updateLifecycle(ctx context.Context) error { if err := t.ensureVaultNamespace(ctx); err != nil { return err } + if t.tenant.GetStatus().GetState() == privatev1.TenantState_TENANT_STATE_FAILED { + return nil + } if err := t.ensureDefaultNetworking(ctx); err != nil { return err } @@ -287,6 +290,11 @@ func (t *task) syncToIDP(ctx context.Context) error { if err := t.ensureVaultNamespace(ctx); err != nil { return err } + // If vault is enabled and provisioning failed, the condition is FALSE and status will be persisted. + // Skip break-glass secret persistence since it requires vault. + if t.r.vaultLifecycle != nil && !t.isConditionTrue(privatev1.TenantConditionType_TENANT_CONDITION_TYPE_VAULT_READY) { + return nil + } if err := t.persistBreakGlassSecret(ctx); err != nil { t.r.logger.ErrorContext(ctx, "Failed to persist break-glass credentials secret", @@ -734,7 +742,10 @@ func (t *task) ensureVaultNamespace(ctx context.Context) error { slog.String("tenant_name", tenantName), slog.Any("error", err), ) - return fmt.Errorf("failed to provision vault namespace: %w", err) + + t.updateCondition(condType, privatev1.ConditionStatus_CONDITION_STATUS_FALSE, + "ProvisionFailed", fmt.Sprintf("Failed to provision vault namespace: %v", err)) + return nil } t.updateCondition(condType, privatev1.ConditionStatus_CONDITION_STATUS_TRUE, diff --git a/fulfillment-service/internal/controllers/tenant/tenant_reconciler_function_test.go b/fulfillment-service/internal/controllers/tenant/tenant_reconciler_function_test.go index 1563b1e255..9413c66484 100644 --- a/fulfillment-service/internal/controllers/tenant/tenant_reconciler_function_test.go +++ b/fulfillment-service/internal/controllers/tenant/tenant_reconciler_function_test.go @@ -2382,7 +2382,7 @@ var _ = Describe("Vault namespace provisioning", func() { mockVaultClient.EXPECT().EnsureTenantNamespace(gomock.Any(), "retry-transit").Return(nil), ) t := &task{r: reconciler, tenant: tenant} - Expect(t.update(ctx)).To(MatchError(ContainSubstring("failed to mount Transit"))) + Expect(t.update(ctx)).To(Succeed()) // Failed reconciliation restores a snapshot, so inspect the task's current tenant. Expect(findCondition(t.tenant)).ToNot(BeNil()) Expect(findCondition(t.tenant).GetStatus()).To(Equal(privatev1.ConditionStatus_CONDITION_STATUS_FALSE)) @@ -2391,7 +2391,127 @@ var _ = Describe("Vault namespace provisioning", func() { Expect(findCondition(t.tenant).GetStatus()).To(Equal(privatev1.ConditionStatus_CONDITION_STATUS_TRUE)) Expect(findCondition(t.tenant).GetReason()).To(Equal("NamespaceReady")) }) + It("stays PENDING with condition FALSE when vault provisioning fails during initial sync", func() { + reconciler := &function{ + logger: logger, + idpManager: idpManager, + vaultLifecycle: mockVaultClient, + } + + tenant := privatev1.Tenant_builder{ + Id: "org-vault-fail", + Metadata: privatev1.Metadata_builder{ + Name: "vault-fail-org", + Finalizers: []string{finalizers.Controller}, + Tenant: "tenant-1", + }.Build(), + Status: privatev1.TenantStatus_builder{ + BreakGlassCredentials: privatev1.BreakGlassCredentials_builder{ + Username: "vault-fail-org-osac-break-glass", + Password: testPreGeneratedPassword, + }.Build(), + }.Build(), + }.Build() + + mockIDPClient.EXPECT(). + CreateTenant(gomock.Any(), gomock.Any()). + Return(&idp.Tenant{Name: "vault-fail-org", Enabled: true}, nil) + mockIDPClient.EXPECT(). + CreateUser(gomock.Any(), "vault-fail-org", gomock.Any()). + Return(&idp.User{ID: "user-fail"}, nil) + mockIDPClient.EXPECT(). + AssignIdpManagerPermissions(gomock.Any(), "user-fail"). + Return(nil) + + mockVaultClient.EXPECT(). + EnsureTenantNamespace(gomock.Any(), "vault-fail-org"). + Return(fmt.Errorf("vault connection refused")) + + t := &task{r: reconciler, tenant: tenant} + err := t.update(ctx) + Expect(err).ToNot(HaveOccurred()) + Expect(tenant.GetStatus().GetState()).To(Equal(privatev1.TenantState_TENANT_STATE_PENDING)) + Expect(tenant.GetStatus().GetIdpTenantName()).To(Equal("vault-fail-org")) + cond := findCondition(tenant) + Expect(cond).ToNot(BeNil()) + Expect(cond.GetStatus()).To(Equal(privatev1.ConditionStatus_CONDITION_STATUS_FALSE)) + Expect(cond.GetReason()).To(Equal("ProvisionFailed")) + Expect(cond.GetMessage()).To(ContainSubstring("vault connection refused")) + }) + + It("stays SYNCED with condition FALSE when vault provisioning fails for synced tenant", func() { + reconciler := &function{ + logger: logger, + idpManager: idpManager, + vaultLifecycle: mockVaultClient, + } + + tenant := privatev1.Tenant_builder{ + Id: "org-synced-vault-fail", + Metadata: privatev1.Metadata_builder{ + Name: "synced-vault-fail-org", + Finalizers: []string{finalizers.Controller}, + Tenant: "tenant-1", + }.Build(), + Status: privatev1.TenantStatus_builder{ + State: privatev1.TenantState_TENANT_STATE_SYNCED, + IdpTenantName: "synced-vault-fail-org", + }.Build(), + }.Build() + + mockIDPClient.EXPECT(). + GetTenant(gomock.Any(), "synced-vault-fail-org"). + Return(&idp.Tenant{Name: "synced-vault-fail-org"}, nil) + + mockVaultClient.EXPECT(). + EnsureTenantNamespace(gomock.Any(), "synced-vault-fail-org"). + Return(fmt.Errorf("dial tcp: no such host")) + + t := &task{r: reconciler, tenant: tenant} + err := t.update(ctx) + Expect(err).ToNot(HaveOccurred()) + Expect(tenant.GetStatus().GetState()).To(Equal(privatev1.TenantState_TENANT_STATE_SYNCED)) + cond := findCondition(tenant) + Expect(cond).ToNot(BeNil()) + Expect(cond.GetStatus()).To(Equal(privatev1.ConditionStatus_CONDITION_STATUS_FALSE)) + Expect(cond.GetReason()).To(Equal("ProvisionFailed")) + Expect(cond.GetMessage()).To(ContainSubstring("dial tcp: no such host")) + }) + It("provisions a vault namespace for the system tenant", func() { + reconciler := &function{ + logger: logger, + idpManager: idpManager, + vaultLifecycle: mockVaultClient, + } + + tenant := privatev1.Tenant_builder{ + Id: "org-system", + Metadata: privatev1.Metadata_builder{ + Name: auth.SystemTenant, + Finalizers: []string{finalizers.Controller}, + Tenant: auth.SystemTenant, + }.Build(), + Status: privatev1.TenantStatus_builder{ + State: privatev1.TenantState_TENANT_STATE_SYNCED, + IdpTenantName: auth.SystemTenant, + }.Build(), + }.Build() + + mockIDPClient.EXPECT(). + GetTenant(gomock.Any(), auth.SystemTenant). + Return(&idp.Tenant{Name: auth.SystemTenant}, nil) + mockVaultClient.EXPECT(). + EnsureTenantNamespace(gomock.Any(), auth.SystemTenant). + Return(nil) + + t := &task{r: reconciler, tenant: tenant} + err := t.update(ctx) + Expect(err).ToNot(HaveOccurred()) + cond := findCondition(tenant) + Expect(cond).ToNot(BeNil()) + Expect(cond.GetStatus()).To(Equal(privatev1.ConditionStatus_CONDITION_STATUS_TRUE)) + }) }) var _ = Describe("Vault namespace cleanup during deletion", func() {