From b52731e5a7ad639f1c662736f0ccde866c0721e9 Mon Sep 17 00:00:00 2001 From: vmelikyan Date: Thu, 16 Apr 2026 10:31:57 -0700 Subject: [PATCH] fix secret sync wait to require requested keys --- .../__tests__/forwardedEnv.test.ts | 12 +- src/server/lib/agentSession/forwardedEnv.ts | 6 +- .../__tests__/secretProcessor.test.ts | 131 ++++++++++++++++-- src/server/services/deploy.ts | 14 +- src/server/services/secretProcessor.ts | 31 +++-- 5 files changed, 165 insertions(+), 29 deletions(-) diff --git a/src/server/lib/agentSession/__tests__/forwardedEnv.test.ts b/src/server/lib/agentSession/__tests__/forwardedEnv.test.ts index 2061dc83..691072b3 100644 --- a/src/server/lib/agentSession/__tests__/forwardedEnv.test.ts +++ b/src/server/lib/agentSession/__tests__/forwardedEnv.test.ts @@ -83,7 +83,7 @@ describe('forwardedEnv', () => { (SecretProcessor as unknown as jest.Mock).mockImplementation(() => ({ processEnvSecrets: jest.fn().mockResolvedValue({ secretRefs: [], - secretNames: [], + expectedKeysPerSecret: {}, warnings: [], }), waitForSecretSync: jest.fn().mockResolvedValue(undefined), @@ -192,7 +192,9 @@ describe('forwardedEnv', () => { key: 'npmToken', }, ], - secretNames: ['agent-env-session-123-aws-secrets'], + expectedKeysPerSecret: { + 'agent-env-session-123-aws-secrets': ['PRIVATE_REGISTRY_TOKEN'], + }, warnings: [], }); const waitForSecretSync = jest.fn().mockResolvedValue(undefined); @@ -236,7 +238,11 @@ describe('forwardedEnv', () => { namespace: 'test-ns', buildUuid: 'build-123', }); - expect(waitForSecretSync).toHaveBeenCalledWith(['agent-env-session-123-aws-secrets'], 'test-ns', 30000); + expect(waitForSecretSync).toHaveBeenCalledWith( + { 'agent-env-session-123-aws-secrets': ['PRIVATE_REGISTRY_TOKEN'] }, + 'test-ns', + 30000 + ); expect(result.secretProviders).toEqual(['aws']); }); diff --git a/src/server/lib/agentSession/forwardedEnv.ts b/src/server/lib/agentSession/forwardedEnv.ts index b9935642..3e2055f0 100644 --- a/src/server/lib/agentSession/forwardedEnv.ts +++ b/src/server/lib/agentSession/forwardedEnv.ts @@ -150,12 +150,14 @@ export async function resolveForwardedAgentEnv( throw new Error(secretResult.warnings.join(' ')); } - if (secretResult.secretNames.length > 0) { + const secretNames = Object.keys(secretResult.expectedKeysPerSecret); + + if (secretNames.length > 0) { const providerTimeouts = Object.values(secretProviders) .map((provider) => provider.secretSyncTimeout) .filter((timeout): timeout is number => timeout !== undefined); const timeout = providerTimeouts.length > 0 ? Math.max(...providerTimeouts) * 1000 : 60000; - await secretProcessor.waitForSecretSync(secretResult.secretNames, namespace, timeout); + await secretProcessor.waitForSecretSync(secretResult.expectedKeysPerSecret, namespace, timeout); } return { diff --git a/src/server/services/__tests__/secretProcessor.test.ts b/src/server/services/__tests__/secretProcessor.test.ts index 58fc2b2e..231bd0d0 100644 --- a/src/server/services/__tests__/secretProcessor.test.ts +++ b/src/server/services/__tests__/secretProcessor.test.ts @@ -56,10 +56,10 @@ describe('SecretProcessor', () => { }); describe('waitForSecretSync', () => { - it('resolves when secret exists with data', async () => { + it('resolves when secret contains expected keys', async () => { const mockReadNamespacedSecret = jest.fn().mockResolvedValue({ body: { - data: { key: 'dmFsdWU=' }, + data: { API_TOKEN: 'dmFsdWU=' }, }, }); @@ -67,11 +67,29 @@ describe('SecretProcessor', () => { readNamespacedSecret: mockReadNamespacedSecret, }); - await expect(processor.waitForSecretSync(['my-secret'], 'test-ns', 5000)).resolves.toBeUndefined(); + await expect( + processor.waitForSecretSync({ 'my-secret': ['API_TOKEN'] }, 'test-ns', 5000) + ).resolves.toBeUndefined(); expect(mockReadNamespacedSecret).toHaveBeenCalledWith('my-secret', 'test-ns'); }); + it('treats empty secret values as present keys', async () => { + const mockReadNamespacedSecret = jest.fn().mockResolvedValue({ + body: { + data: { EMPTY_TOKEN: '' }, + }, + }); + + jest.spyOn(processor as any, 'getK8sClient').mockReturnValue({ + readNamespacedSecret: mockReadNamespacedSecret, + }); + + await expect( + processor.waitForSecretSync({ 'my-secret': ['EMPTY_TOKEN'] }, 'test-ns', 5000) + ).resolves.toBeUndefined(); + }); + it('throws error on timeout when secret does not exist', async () => { const mockReadNamespacedSecret = jest.fn().mockRejectedValue({ statusCode: 404 }); @@ -79,10 +97,12 @@ describe('SecretProcessor', () => { readNamespacedSecret: mockReadNamespacedSecret, }); - await expect(processor.waitForSecretSync(['my-secret'], 'test-ns', 1000)).rejects.toThrow('Secret sync timeout'); + await expect(processor.waitForSecretSync({ 'my-secret': ['API_TOKEN'] }, 'test-ns', 1000)).rejects.toThrow( + /Secret sync timeout.*missing keys=\[API_TOKEN\]/ + ); }); - it('throws error on timeout when secret has no data', async () => { + it('throws with missing keys listed when Secret data is empty', async () => { const mockReadNamespacedSecret = jest.fn().mockResolvedValue({ body: { data: null }, }); @@ -91,7 +111,89 @@ describe('SecretProcessor', () => { readNamespacedSecret: mockReadNamespacedSecret, }); - await expect(processor.waitForSecretSync(['my-secret'], 'test-ns', 1000)).rejects.toThrow('Secret sync timeout'); + await expect(processor.waitForSecretSync({ 'my-secret': ['API_TOKEN'] }, 'test-ns', 1000)).rejects.toThrow( + /Secret sync timeout.*missing keys=\[API_TOKEN\]/ + ); + }); + + it('waits until an existing secret contains newly requested keys', async () => { + const mockReadNamespacedSecret = jest + .fn() + .mockResolvedValueOnce({ + body: { data: { EXISTING_TOKEN: 'b2xk' } }, + }) + .mockResolvedValueOnce({ + body: { data: { EXISTING_TOKEN: 'b2xk', NEW_TOKEN: 'bmV3' } }, + }); + + jest.spyOn(processor as any, 'getK8sClient').mockReturnValue({ + readNamespacedSecret: mockReadNamespacedSecret, + }); + jest.spyOn(processor as any, 'sleep').mockResolvedValue(undefined); + + await expect( + processor.waitForSecretSync({ 'sample-service-aws-secrets': ['EXISTING_TOKEN', 'NEW_TOKEN'] }, 'test-ns', 5000) + ).resolves.toBeUndefined(); + + expect(mockReadNamespacedSecret).toHaveBeenCalledTimes(2); + }); + + it('waits through not found and partial keys until all requested keys land', async () => { + const mockReadNamespacedSecret = jest + .fn() + .mockRejectedValueOnce({ statusCode: 404 }) + .mockResolvedValueOnce({ + body: { data: { FIRST_TOKEN: 'Zmlyc3Q=' } }, + }) + .mockResolvedValueOnce({ + body: { data: { FIRST_TOKEN: 'Zmlyc3Q=', SECOND_TOKEN: 'c2Vjb25k' } }, + }); + + jest.spyOn(processor as any, 'getK8sClient').mockReturnValue({ + readNamespacedSecret: mockReadNamespacedSecret, + }); + jest.spyOn(processor as any, 'sleep').mockResolvedValue(undefined); + + await processor.waitForSecretSync( + { 'sample-service-aws-secrets': ['FIRST_TOKEN', 'SECOND_TOKEN'] }, + 'test-ns', + 5000 + ); + + expect(mockReadNamespacedSecret).toHaveBeenCalledTimes(3); + }); + + it('waits until the last requested key lands', async () => { + const mockReadNamespacedSecret = jest + .fn() + .mockResolvedValueOnce({ + body: { data: { FIRST_TOKEN: 'Zmlyc3Q=' } }, + }) + .mockResolvedValueOnce({ + body: { data: { FIRST_TOKEN: 'Zmlyc3Q=', SECOND_TOKEN: 'c2Vjb25k' } }, + }) + .mockResolvedValueOnce({ + body: { + data: { + FIRST_TOKEN: 'Zmlyc3Q=', + SECOND_TOKEN: 'c2Vjb25k', + THIRD_TOKEN: 'dGhpcmQ=', + }, + }, + }); + + jest.spyOn(processor as any, 'getK8sClient').mockReturnValue({ + readNamespacedSecret: mockReadNamespacedSecret, + }); + jest.spyOn(processor as any, 'sleep').mockResolvedValue(undefined); + + await processor.waitForSecretSync( + { 'sample-service-aws-secrets': ['FIRST_TOKEN', 'SECOND_TOKEN', 'THIRD_TOKEN'] }, + 'test-ns', + 5000 + ); + + expect(mockReadNamespacedSecret).toHaveBeenCalledTimes(3); }); it('waits for multiple secrets', async () => { @@ -103,7 +205,7 @@ describe('SecretProcessor', () => { readNamespacedSecret: mockReadNamespacedSecret, }); - await processor.waitForSecretSync(['secret-1', 'secret-2'], 'test-ns', 5000); + await processor.waitForSecretSync({ 'secret-1': ['key'], 'secret-2': ['key'] }, 'test-ns', 5000); expect(mockReadNamespacedSecret).toHaveBeenCalledWith('secret-1', 'test-ns'); expect(mockReadNamespacedSecret).toHaveBeenCalledWith('secret-2', 'test-ns'); @@ -126,6 +228,9 @@ describe('SecretProcessor', () => { expect(result.secretRefs).toHaveLength(1); expect(result.secretRefs[0].envKey).toBe('DB_PASSWORD'); + expect(result.expectedKeysPerSecret).toEqual({ + 'api-server-aws-secrets': ['DB_PASSWORD'], + }); expect(result.warnings).toHaveLength(0); }); @@ -141,6 +246,7 @@ describe('SecretProcessor', () => { }); expect(result.secretRefs).toHaveLength(0); + expect(result.expectedKeysPerSecret).toEqual({}); expect(result.warnings).toHaveLength(1); expect(result.warnings[0]).toContain('disabled'); }); @@ -162,6 +268,7 @@ describe('SecretProcessor', () => { }); expect(result.secretRefs).toHaveLength(0); + expect(result.expectedKeysPerSecret).toEqual({}); expect(result.warnings).toHaveLength(1); expect(result.warnings[0]).toContain('not configured'); }); @@ -222,6 +329,7 @@ describe('SecretProcessor', () => { }); expect(result.secretRefs).toHaveLength(0); + expect(result.expectedKeysPerSecret).toEqual({}); expect(result.warnings).toHaveLength(0); }); @@ -240,11 +348,12 @@ describe('SecretProcessor', () => { }); expect(result.secretRefs).toHaveLength(1); + expect(result.expectedKeysPerSecret).toEqual({}); expect(result.warnings).toHaveLength(1); expect(result.warnings[0]).toContain('Failed to apply ExternalSecret'); }); - it('returns secret names for mounting', async () => { + it('returns expected keys by secret for mounting', async () => { const env = { AWS_SECRET: '{{aws:path:key}}', GCP_SECRET: '{{gcp:path:key}}', @@ -261,8 +370,10 @@ describe('SecretProcessor', () => { namespace: 'ns', }); - expect(result.secretNames).toContain('api-server-aws-secrets'); - expect(result.secretNames).toContain('api-server-gcp-secrets'); + expect(result.expectedKeysPerSecret).toEqual({ + 'api-server-aws-secrets': ['AWS_SECRET'], + 'api-server-gcp-secrets': ['GCP_SECRET'], + }); }); }); }); diff --git a/src/server/services/deploy.ts b/src/server/services/deploy.ts index af93cfd6..e080a061 100644 --- a/src/server/services/deploy.ts +++ b/src/server/services/deploy.ts @@ -1156,8 +1156,10 @@ export default class DeployService extends BaseService { ); } - if (secretResult.secretNames.length > 0) { - getLogger().info(`Build: waiting for secrets to sync secrets=[${secretResult.secretNames.join(', ')}]`); + const secretNames = Object.keys(secretResult.expectedKeysPerSecret); + + if (secretNames.length > 0) { + getLogger().info(`Build: waiting for secrets to sync secrets=[${secretNames.join(', ')}]`); const providerTimeouts = Object.values(secretProviders) .map((p) => p.secretSyncTimeout) @@ -1165,8 +1167,12 @@ export default class DeployService extends BaseService { const timeout = providerTimeouts.length > 0 ? Math.max(...providerTimeouts) * 1000 : 60000; try { - await secretProcessor.waitForSecretSync(secretResult.secretNames, deploy.build.namespace, timeout); - buildSecretNames = secretResult.secretNames; + await secretProcessor.waitForSecretSync( + secretResult.expectedKeysPerSecret, + deploy.build.namespace, + timeout + ); + buildSecretNames = secretNames; getLogger().info(`Build: secrets synced count=${buildSecretNames.length}`); } catch (error) { getLogger().error({ error }, `Build: secret sync failed service=${deployable.name}`); diff --git a/src/server/services/secretProcessor.ts b/src/server/services/secretProcessor.ts index 05f32df5..a49cc5f4 100644 --- a/src/server/services/secretProcessor.ts +++ b/src/server/services/secretProcessor.ts @@ -36,7 +36,7 @@ export interface ProcessEnvSecretsOptions { export interface ProcessEnvSecretsResult { secretRefs: SecretRefWithEnvKey[]; - secretNames: string[]; + expectedKeysPerSecret: Record; warnings: string[]; } @@ -57,28 +57,39 @@ export class SecretProcessor { return this.k8sClient; } - async waitForSecretSync(secretNames: string[], namespace: string, timeoutMs?: number): Promise { + // ExternalSecret updates reuse Secret names, so wait for this apply's keys instead of any existing data. + async waitForSecretSync( + expectedKeysPerSecret: Record, + namespace: string, + timeoutMs?: number + ): Promise { const timeout = timeoutMs ?? DEFAULT_SECRET_SYNC_TIMEOUT; const pollInterval = 1000; const startTime = Date.now(); const k8sClient = this.getK8sClient(); - for (const secretName of secretNames) { + for (const [secretName, expectedKeys] of Object.entries(expectedKeysPerSecret)) { let synced = false; + let missingKeys = expectedKeys; while (!synced) { if (Date.now() - startTime > timeout) { - throw new Error(`Secret sync timeout: ${secretName} not ready after ${timeout}ms`); + throw new Error( + `Secret sync timeout: ${secretName} missing keys=[${missingKeys.join(', ')}] after ${timeout}ms` + ); } try { const response = await k8sClient.readNamespacedSecret(secretName, namespace); - if (response.body.data && Object.keys(response.body.data).length > 0) { + const data = response.body.data || {}; + missingKeys = expectedKeys.filter((key) => !Object.prototype.hasOwnProperty.call(data, key)); + + if (missingKeys.length === 0) { getLogger().info(`Secret: synced name=${secretName} namespace=${namespace}`); synced = true; } else { - getLogger().debug(`Secret: waiting for data name=${secretName}`); + getLogger().debug(`Secret: waiting for keys name=${secretName} missing=[${missingKeys.join(', ')}]`); await this.sleep(pollInterval); } } catch (error: any) { @@ -120,11 +131,11 @@ export class SecretProcessor { } if (validRefs.length === 0) { - return { secretRefs: [], secretNames: [], warnings }; + return { secretRefs: [], expectedKeysPerSecret: {}, warnings }; } const grouped = groupSecretRefsByProvider(validRefs); - const secretNames: string[] = []; + const expectedKeysPerSecret: Record = {}; for (const [provider, refs] of Object.entries(grouped)) { const providerConfig = this.secretProviders![provider]; @@ -141,7 +152,7 @@ export class SecretProcessor { try { await applyExternalSecret(manifest, namespace); - secretNames.push(secretName); + expectedKeysPerSecret[secretName] = [...new Set(refs.map((ref) => ref.envKey))]; } catch (error) { const errorMsg = (error as any)?.message || (error as any)?.stderr || String(error); const warning = `Failed to apply ExternalSecret for ${serviceName}: ${errorMsg}`; @@ -149,6 +160,6 @@ export class SecretProcessor { } } - return { secretRefs: validRefs, secretNames, warnings }; + return { secretRefs: validRefs, expectedKeysPerSecret, warnings }; } }