From f6f4d7de460273970ca21cae30465d4b549b94b9 Mon Sep 17 00:00:00 2001 From: Adam Weingarten <6517820+aweingarten@users.noreply.github.com> Date: Tue, 28 Jul 2026 10:50:58 -0400 Subject: [PATCH 1/2] fix(argocd): restart redis and its clients when the password rotates MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `argocd-redis` runs `redis-server --requirepass $(REDIS_PASSWORD)`, so the password is baked into the process at pod start and never re-read. Its clients (repo-server, application-controller, server) read the same Secret key into an env var, also at their own start. `createArgoCdRedisSecret` server-side-applies that Secret on every install with no read-back and no restart, and apl disables the chart's own `redisSecretInit`, so nothing else is watching the Secret either. Rewriting it under a running redis pod therefore splits the two: redis keeps requiring the old password while any client that restarts afterwards presents the new one, and every Application flips to Unknown/ComparisonError: ComparisonError: Failed to load target state: ... failed to list refs: WRONGPASS invalid username-password pair or user is disabled Since repo-server caches git refs in redis, one bad AUTH turns all manifest generation into a ComparisonError at once and the whole app-of-apps stalls until redis is restarted by hand. Read the Secret before writing it and, when the value actually changed, restart redis and its consumers. The read-back matters: this runs on every install, and restarting the Argo stack on an unchanged password would be its own outage. The restart list is extracted as `restartArgoCdRedisConsumers` — it is the same set the v60 migration already restarts by hand, which is the tell that this belongs at the point of rotation rather than in one one-off migration. Refs #3424 --- src/common/k8s.test.ts | 85 ++++++++++++++++++++++++++++++++++++++++++ src/common/k8s.ts | 50 ++++++++++++++++++++++++- 2 files changed, 134 insertions(+), 1 deletion(-) diff --git a/src/common/k8s.test.ts b/src/common/k8s.test.ts index f2506be0ac..bb52654bbf 100644 --- a/src/common/k8s.test.ts +++ b/src/common/k8s.test.ts @@ -1037,3 +1037,88 @@ describe('getSealedSecretsPEM', () => { expect(MockX509Certificate).toHaveBeenCalledWith('single-cert') }) }) + +describe('createArgoCdRedisSecret', () => { + const password = 'new-password' + const objectApi = { patch: jest.fn() } + + beforeEach(() => { + jest.clearAllMocks() + objectApi.patch.mockResolvedValue({}) + jest.spyOn(k8s.k8s, 'object').mockReturnValue(objectApi as any) + }) + + afterEach(() => jest.restoreAllMocks()) + + const makeDeps = (overrides = {}) => ({ + getK8sSecret: jest.fn(async () => ({ auth: password })), + restartArgoCdRedisConsumers: jest.fn(async (_namespace: string) => {}), + ...overrides, + }) + + it('should restart redis and its consumers when the password changed', async () => { + const deps = makeDeps({ getK8sSecret: jest.fn(async () => ({ auth: 'old-password' })) }) + + await k8s.createArgoCdRedisSecret({ apps: { argocd: { redisPassword: password } } }, deps as any) + + expect(deps.restartArgoCdRedisConsumers).toHaveBeenCalledWith('argocd') + }) + + it('should not restart anything when the password is unchanged', async () => { + const deps = makeDeps() + + await k8s.createArgoCdRedisSecret({ apps: { argocd: { redisPassword: password } } }, deps as any) + + expect(objectApi.patch).toHaveBeenCalled() + expect(deps.restartArgoCdRedisConsumers).not.toHaveBeenCalled() + }) + + it('should not restart anything on a fresh install where the secret does not exist yet', async () => { + const deps = makeDeps({ getK8sSecret: jest.fn(async () => undefined) }) + + await k8s.createArgoCdRedisSecret({ apps: { argocd: { redisPassword: password } } }, deps as any) + + expect(deps.restartArgoCdRedisConsumers).not.toHaveBeenCalled() + }) + + it('should skip reconciliation entirely when no password is supplied', async () => { + const deps = makeDeps() + + await k8s.createArgoCdRedisSecret({ apps: { argocd: {} } }, deps as any) + + expect(objectApi.patch).not.toHaveBeenCalled() + expect(deps.restartArgoCdRedisConsumers).not.toHaveBeenCalled() + }) +}) + +describe('restartArgoCdRedisConsumers', () => { + const makeDeps = (overrides = {}) => ({ + restartDeployment: jest.fn(async (_name: string, _namespace: string) => {}), + restartStatefulSet: jest.fn(async (_name: string, _namespace: string) => {}), + ...overrides, + }) + + it('should restart redis before its clients', async () => { + const deps = makeDeps() + + await k8s.restartArgoCdRedisConsumers('argocd', deps as any) + + expect(deps.restartDeployment).toHaveBeenCalledWith('argocd-redis', 'argocd') + expect(deps.restartDeployment.mock.calls[0][0]).toBe('argocd-redis') + expect(deps.restartDeployment).toHaveBeenCalledWith('argocd-repo-server', 'argocd') + expect(deps.restartStatefulSet).toHaveBeenCalledWith('argocd-application-controller', 'argocd') + }) + + it('should keep going when a target does not exist', async () => { + const notFound = new MockApiException(404, 'not found', {}, {}) + const deps = makeDeps({ + restartDeployment: jest.fn(async (name: string) => { + if (name === 'argocd-server') throw notFound + }), + }) + + await expect(k8s.restartArgoCdRedisConsumers('argocd', deps as any)).resolves.toBeUndefined() + + expect(deps.restartDeployment).toHaveBeenCalledWith('argocd-repo-server', 'argocd') + }) +}) diff --git a/src/common/k8s.ts b/src/common/k8s.ts index 94dbc05224..dd2dab4cde 100644 --- a/src/common/k8s.ts +++ b/src/common/k8s.ts @@ -819,7 +819,45 @@ export async function setArgoCdAppSync( ) } -export const createArgoCdRedisSecret = async (values: Record): Promise => { +// argocd-redis runs `redis-server --requirepass $(REDIS_PASSWORD)`, so the password is baked into +// the process at pod start and never re-read. Its clients (repo-server, application-controller, +// server) read the same Secret key into an env var, also at their own start. Rewriting the Secret +// under a running redis pod therefore splits the two: redis keeps requiring the old password while +// any client that restarts afterwards presents the new one, and every Application flips to +// Unknown/ComparisonError (WRONGPASS) until redis is restarted by hand. +export const ARGOCD_REDIS_RESTART_TARGETS: Array<{ kind: 'deployment' | 'statefulset'; name: string }> = [ + { kind: 'deployment', name: 'argocd-redis' }, + { kind: 'deployment', name: 'argocd-server' }, + { kind: 'deployment', name: 'argocd-repo-server' }, + { kind: 'statefulset', name: 'argocd-application-controller' }, + { kind: 'deployment', name: 'argocd-application-controller' }, +] + +export const restartArgoCdRedisConsumers = async ( + namespace: string, + deps = { restartDeployment, restartStatefulSet }, +): Promise => { + const d = terminal('common:k8s:restartArgoCdRedisConsumers') + for (const target of ARGOCD_REDIS_RESTART_TARGETS) { + try { + if (target.kind === 'deployment') await deps.restartDeployment(target.name, namespace) + else await deps.restartStatefulSet(target.name, namespace) + d.info(`Restarted ${target.kind}/${target.name} after redis password change`) + } catch (error) { + // Not every target exists in every topology — a missing one is not a failure. + if (error instanceof ApiException && error.code === 404) { + d.debug(`Could not restart ${target.kind}/${target.name}: not found`) + continue + } + d.warn(`Could not restart ${target.kind}/${target.name}:`, error) + } + } +} + +export const createArgoCdRedisSecret = async ( + values: Record, + deps = { getK8sSecret, restartArgoCdRedisConsumers }, +): Promise => { const d = terminal('common:k8s:createArgoCdRedisSecret') const argocdNamespace = 'argocd' const secretName = 'argocd-redis' @@ -831,6 +869,11 @@ export const createArgoCdRedisSecret = async (values: Record): Prom return } + // Read before writing so the restart below is limited to an actual rotation. This runs on every + // install, and restarting the whole Argo stack on an unchanged password would be its own outage. + const existingSecret = await deps.getK8sSecret(secretName, argocdNamespace).catch(() => undefined) + const passwordChanged = existingSecret !== undefined && existingSecret.auth !== redisPassword + try { await k8s.object().patch( { @@ -879,6 +922,11 @@ export const createArgoCdRedisSecret = async (values: Record): Prom d.error(`Failed to patch Secret ${secretName} with server-side apply:`, error) throw error } + + if (passwordChanged) { + d.info(`Password of Secret ${secretName} changed, restarting redis and its consumers`) + await deps.restartArgoCdRedisConsumers(argocdNamespace) + } } export async function restartDeployment(name: string, namespace: string): Promise { From debaf027d8824d5a59c5668a1a386137a82715ef Mon Sep 17 00:00:00 2001 From: Adam Weingarten <6517820+aweingarten@users.noreply.github.com> Date: Fri, 7 Aug 2026 08:12:04 -0400 Subject: [PATCH 2/2] fix(argocd): stop the redis restart from making a split worse MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Two failure paths the first cut got wrong: - If argocd-redis itself fails to restart, restarting its clients hands them the new password while redis still requires the old one — exactly the WRONGPASS split this is meant to prevent. Redis now runs first and is the only target allowed to abort the sequence; a 404 means there is nothing to keep in step, anything else is raised. - `.catch(() => undefined)` on the read swallowed RBAC and transient errors, so a real rotation could silently skip the restart. getK8sSecret already maps 404 to undefined, so the catch only ever hid real failures. A failed read now assumes rotation — a redundant restart is cheap, a missed one is an outage. --- src/common/k8s.test.ts | 41 +++++++++++++++++++++++++++++++++++++++++ src/common/k8s.ts | 33 ++++++++++++++++++++++++++++++--- 2 files changed, 71 insertions(+), 3 deletions(-) diff --git a/src/common/k8s.test.ts b/src/common/k8s.test.ts index bb52654bbf..3476ef3b95 100644 --- a/src/common/k8s.test.ts +++ b/src/common/k8s.test.ts @@ -1081,6 +1081,19 @@ describe('createArgoCdRedisSecret', () => { expect(deps.restartArgoCdRedisConsumers).not.toHaveBeenCalled() }) + it('should restart when the existing secret cannot be read at all', async () => { + const deps = makeDeps({ + getK8sSecret: jest.fn(async () => { + throw new Error('forbidden') + }), + }) + + await k8s.createArgoCdRedisSecret({ apps: { argocd: { redisPassword: password } } }, deps as any) + + expect(objectApi.patch).toHaveBeenCalled() + expect(deps.restartArgoCdRedisConsumers).toHaveBeenCalledWith('argocd') + }) + it('should skip reconciliation entirely when no password is supplied', async () => { const deps = makeDeps() @@ -1121,4 +1134,32 @@ describe('restartArgoCdRedisConsumers', () => { expect(deps.restartDeployment).toHaveBeenCalledWith('argocd-repo-server', 'argocd') }) + + it('should not restart the clients when redis itself fails to restart', async () => { + const deps = makeDeps({ + restartDeployment: jest.fn(async (name: string) => { + if (name === 'argocd-redis') throw new Error('boom') + }), + }) + + await expect(k8s.restartArgoCdRedisConsumers('argocd', deps as any)).rejects.toThrow('boom') + + expect(deps.restartDeployment).toHaveBeenCalledTimes(1) + expect(deps.restartDeployment).toHaveBeenCalledWith('argocd-redis', 'argocd') + expect(deps.restartStatefulSet).not.toHaveBeenCalled() + }) + + it('should leave the clients alone when redis is not present at all', async () => { + const notFound = new MockApiException(404, 'not found', {}, {}) + const deps = makeDeps({ + restartDeployment: jest.fn(async (name: string) => { + if (name === 'argocd-redis') throw notFound + }), + }) + + await expect(k8s.restartArgoCdRedisConsumers('argocd', deps as any)).resolves.toBeUndefined() + + expect(deps.restartDeployment).toHaveBeenCalledTimes(1) + expect(deps.restartStatefulSet).not.toHaveBeenCalled() + }) }) diff --git a/src/common/k8s.ts b/src/common/k8s.ts index dd2dab4cde..174e1246ac 100644 --- a/src/common/k8s.ts +++ b/src/common/k8s.ts @@ -838,7 +838,25 @@ export const restartArgoCdRedisConsumers = async ( deps = { restartDeployment, restartStatefulSet }, ): Promise => { const d = terminal('common:k8s:restartArgoCdRedisConsumers') - for (const target of ARGOCD_REDIS_RESTART_TARGETS) { + const [redis, ...consumers] = ARGOCD_REDIS_RESTART_TARGETS + + // Redis has to come back on the new password before anything is pointed at it. If it does not + // restart, restarting the clients is worse than doing nothing: they would pick up the new + // password while redis still requires the old one, which is the split this function exists to + // prevent. So redis, and only redis, is allowed to stop the sequence. + try { + await deps.restartDeployment(redis.name, namespace) + d.info(`Restarted ${redis.kind}/${redis.name} after redis password change`) + } catch (error) { + if (error instanceof ApiException && error.code === 404) { + d.debug(`Could not restart ${redis.kind}/${redis.name}: not found — leaving its clients alone`) + return + } + d.error(`Could not restart ${redis.kind}/${redis.name}, not restarting its clients:`, error) + throw error + } + + for (const target of consumers) { try { if (target.kind === 'deployment') await deps.restartDeployment(target.name, namespace) else await deps.restartStatefulSet(target.name, namespace) @@ -871,8 +889,17 @@ export const createArgoCdRedisSecret = async ( // Read before writing so the restart below is limited to an actual rotation. This runs on every // install, and restarting the whole Argo stack on an unchanged password would be its own outage. - const existingSecret = await deps.getK8sSecret(secretName, argocdNamespace).catch(() => undefined) - const passwordChanged = existingSecret !== undefined && existingSecret.auth !== redisPassword + // getK8sSecret already maps 404 to undefined (first install — nothing to restart), so anything + // that throws here is a real read failure and leaves us unable to tell. Assume it rotated: a + // redundant restart costs seconds, a missed one leaves redis and its clients split on WRONGPASS. + let passwordChanged: boolean + try { + const existingSecret = await deps.getK8sSecret(secretName, argocdNamespace) + passwordChanged = existingSecret !== undefined && existingSecret.auth !== redisPassword + } catch (error) { + d.warn(`Could not read Secret ${secretName} to detect a password change, assuming it rotated:`, error) + passwordChanged = true + } try { await k8s.object().patch(