feat(redis): Inject creds via volume#2059
feat(redis): Inject creds via volume#2059olivergondza wants to merge 14 commits intoargoproj-labs:masterfrom
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughA refactor centralizing Redis auth and templating: introduces Changes
Sequence Diagram(s)sequenceDiagram
participant Operator as ArgoCD Operator
participant Argoutil as argoutil (helpers)
participant Secret as K8s Secret (argocd-redis-initial-password)
participant Pod as ArgoCD Pod (Server/Repo/Controller)
participant Redis as Redis / HA
Operator->>Argoutil: call MountRedisAuthToArgo(cr)
Argoutil-->>Operator: return (Volume, VolumeMount) referencing Secret
Operator->>Kubernetes: create/update Deployment/StatefulSet with Volume & Mount
Kubernetes->>Secret: mount secret at /app/config/redis-auth/auth
Pod->>Argoutil: at init, haproxy_init.sh / init.sh reads /app/config/redis-auth/auth
Argoutil-->>Pod: inject REDISCLI_AUTH / substitute __REPLACE_DEFAULT_AUTH__
Pod->>Redis: redis-cli with REDISCLI_AUTH -> AUTHenticated PING
Redis-->>Pod: PONG
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (6)
go.mod (1)
189-190:⚠️ Potential issue | 🟡 MinorStale comment: replace block references
v3.2.1but dependency is nowv3.3.0-rc3.Proposed fix
- // This replace block is from Argo CD v3.2.1 go.mod + // This replace block is from Argo CD v3.3.0-rc3 go.modtests/ginkgo/parallel/1-066_validate_redis_secure_comm_no_autotls_no_ha_test.go (1)
150-153:⚠️ Potential issue | 🟡 MinorPre-existing: missing space/separator in downstream string concatenation.
Line 152 appends
"redis-server --protected-mode no"directly toexpectedStringwithout a leading space, which would produce...--tls-auth-clients noredis-server --protected-mode no. This is pre-existing (not introduced in this PR), but it may cause the downstream test assertion to fail or pass incorrectly.Proposed fix
- expectedString += "redis-server --protected-mode no" + expectedString += " redis-server --protected-mode no"controllers/argocdagent/agent/deployment.go (1)
218-270:⚠️ Potential issue | 🟠 Major
updateDeploymentIfChangeddoes not reconcileVolumesorVolumeMountschanges.The function checks image, name, env, args, security context, ports, and service account — but skips
VolumesandVolumeMounts. If the Redis auth volume/mount definition changes (e.g., secret name changes due to CR rename or key mapping updates), the existing deployment won't be updated.Proposed fix — add volume and volumeMount drift detection
if !reflect.DeepEqual(deployment.Spec.Template.Spec.ServiceAccountName, saName) { log.Info("deployment service account name is being updated") changed = true deployment.Spec.Template.Spec.ServiceAccountName = saName } + desiredSpec := buildAgentSpec(compName, saName, cr) + if !reflect.DeepEqual(deployment.Spec.Template.Spec.Volumes, desiredSpec.Template.Spec.Volumes) { + log.Info("deployment volumes are being updated") + changed = true + deployment.Spec.Template.Spec.Volumes = desiredSpec.Template.Spec.Volumes + } + + if !reflect.DeepEqual(deployment.Spec.Template.Spec.Containers[0].VolumeMounts, desiredSpec.Template.Spec.Containers[0].VolumeMounts) { + log.Info("deployment container volume mounts are being updated") + changed = true + deployment.Spec.Template.Spec.Containers[0].VolumeMounts = desiredSpec.Template.Spec.Containers[0].VolumeMounts + } + return deployment, changedcontrollers/argocd/deployment.go (1)
223-239:⚠️ Potential issue | 🟠 MajorRedis non-HA deployment lacks server-side authentication enforcement.
The
getArgoRedisArgsfunction (lines 223–239) no longer includes--requirepass, and the non-HA Redis deployment does not use theredis.confConfigMap template (which containsrequirepass __REPLACE_DEFAULT_AUTH__). While the auth secret is mounted viaredisMount, the Redis server binary does not automatically read credentials from it.In contrast, the HA Redis StatefulSet passes
/data/conf/redis.confas an argument (statefulset.go:143), enabling authentication via the template. The non-HA deployment should either:
- Add
--requirepasswith the password from the secret, or- Mount and use the
redis.confConfigMap like the HA deployment doesWithout server-side authentication, the Redis instance runs unprotected despite clients being configured with
REDIS_CREDS_DIR_PATH.controllers/argocdagent/deployment.go (1)
252-310:⚠️ Potential issue | 🟠 MajorMissing volume/volumeMount drift detection in
updateDeploymentIfChanged.Both the principal and agent
updateDeploymentIfChangedfunctions (controllers/argocdagent/deployment.go:252 and controllers/argocdagent/agent/deployment.go:218) check image, env, args, ports, security context, and service account — but do not compareVolumeMountsorVolumes. During an upgrade, existing deployments won't receive the newredis-initial-passvolume/mount until manually recreated.This should be fixed in both files to ensure smooth upgrades.
Proposed fix — add volume and volumeMount checks
if !reflect.DeepEqual(deployment.Spec.Template.Spec.ServiceAccountName, saName) { log.Info("deployment service account name is being updated") changed = true deployment.Spec.Template.Spec.ServiceAccountName = saName } + desiredSpec := buildPrincipalSpec(compName, saName, cr) + if !reflect.DeepEqual(deployment.Spec.Template.Spec.Containers[0].VolumeMounts, desiredSpec.Template.Spec.Containers[0].VolumeMounts) { + log.Info("deployment container volume mounts are being updated") + changed = true + deployment.Spec.Template.Spec.Containers[0].VolumeMounts = desiredSpec.Template.Spec.Containers[0].VolumeMounts + } + + if !reflect.DeepEqual(deployment.Spec.Template.Spec.Volumes, desiredSpec.Template.Spec.Volumes) { + log.Info("deployment volumes are being updated") + changed = true + deployment.Spec.Template.Spec.Volumes = desiredSpec.Template.Spec.Volumes + } + return deployment, changedApply the same pattern in controllers/argocdagent/agent/deployment.go using
buildAgentSpec.controllers/argocd/statefulset.go (1)
822-833:⚠️ Potential issue | 🟡 MinorPre-existing issue: export path overwrites all controller volumes including the
redisAuthVolume.When an ArgoCD export exists, line 833 (
podSpec.Volumes = getArgoImportVolumes(export)) overwritespodSpec.Volumesthat was set at line 779, which includesredisAuthVolumeand other controller volumes. The import init container only requires 3 specific volumes (backup-storage, secret-storage, tmp), but the subsequent main application containers need the full volume set. This overwrites prevents access to redisAuthVolume, TLS volumes, cmd-params, and custom controller volumes during import/restore operations. This is a pre-existing issue affecting all volumes, but worth noting since this PR adds another volume that would be silently dropped in this code path.
🤖 Fix all issues with AI agents
In `@build/redis/haproxy_init.sh.tpl`:
- Around line 42-43: The substitution fails when AUTH contains backslashes
because ESCAPED_AUTH only escapes '/' and '&'; update the escaping pipeline for
AUTH so you first escape backslashes and then escape '/' and '&' (i.e.,
transform '\' into '\\' before the existing sed escapes) and continue to use
ESCAPED_AUTH in the sed -i replacement of __REPLACE_DEFAULT_AUTH__ in
haproxy_init.sh.tpl; locate the AUTH -> ESCAPED_AUTH assignment and the sed -i
"s/__REPLACE_DEFAULT_AUTH__/${ESCAPED_AUTH}/" "$HAPROXY_CONF" lines and apply
the additional backslash-escaping step (escape backslashes first, then slash and
ampersand).
In `@build/util/Dockerfile`:
- Around line 1-2: Version comment mismatch between the FROM line in
build/util/Dockerfile (commented as v3.3.3-rc3 for
quay.io/argoproj/argocd@sha256:a7c8c83617f188b20d9739d8f347c0fde0740bbf5b9a8e454f46b704ab396282)
and the version noted in common/defaults.go (v3.3.0-rc3). Pick the correct
semantic version that matches the referenced digest and make the comments
consistent: update either the comment in build/util/Dockerfile or the version
string/comment in common/defaults.go so both state the same version (use the
digest as ground truth), and run a quick CI/resolution to ensure no other doc
strings reference the old mismatched tag.
In `@go.mod`:
- Line 6: Update the Argo CD dependency from the release candidate to the stable
tag by replacing the module version string "github.com/argoproj/argo-cd/v3
v3.3.0-rc3" with "github.com/argoproj/argo-cd/v3 v3.3.0" in the go.mod entry and
then run the usual Go module refresh (e.g., go mod tidy / go get) to ensure the
lockfile and vendor state are updated.
In
`@tests/ginkgo/sequential/1-067_validate_redis_secure_comm_no_autotls_ha_test.go`:
- Around line 219-231: This HA test ("verify redis credential distribution") is
missing the 3-node prerequisite check; before creating the HA-enabled ArgoCD
object, call the same cluster-size assertion used in the sibling test by
invoking nodeFixture.ExpectHasAtLeastXNodes(3) (or the test harness equivalent)
to ensure the cluster has at least 3 nodes; add this check right after obtaining
the test namespace and before constructing/creating the ArgoCD resource so Redis
HA prerequisites are verified.
🧹 Nitpick comments (6)
tests/ginkgo/sequential/1-052_validate_argocd_agent_agent_test.go (1)
343-345: Test verifies absence ofREDIS_PASSWORDbut doesn't verify the new volume-based credential delivery.The assertion correctly checks that
REDIS_PASSWORDis no longer set as an env var. However, there's no corresponding positive assertion that the new mechanism is in place — e.g., verifying thatREDIS_CREDS_DIR_PATHis set in the container env or that theredis-initial-passvolume/mount exists on the deployment. Consider adding assertions for the new credential delivery to catch regressions in both directions.controllers/argocdagent/agent/deployment_test.go (1)
432-448: Redis auth volume mount assertion only checks the name, not themountPath.The
userpass-passwdmount is validated for bothNameandMountPath(lines 435-436), butredis-initial-pass(line 438) only verifies the name. Consider also asserting the expected mount path to prevent silent misconfiguration.Similarly, the volume assertion on line 447 only checks the name but not the secret source. The
userpass-passwdvolume checksSecretNameandOptional(lines 444-445) — theredis-initial-passvolume should receive equivalent validation.♻️ Proposed fix to add more thorough assertions
assert.Equal(t, "redis-initial-pass", container.VolumeMounts[1].Name) + assert.Equal(t, "/app/config/redis-auth", container.VolumeMounts[1].MountPath) // Verify specific volume details assert.Len(t, deployment.Spec.Template.Spec.Volumes, 2) userpassVolume := deployment.Spec.Template.Spec.Volumes[0] assert.Equal(t, "userpass-passwd", userpassVolume.Name) assert.Equal(t, "argocd-agent-agent-userpass", userpassVolume.Secret.SecretName) assert.Equal(t, ptr.To(true), userpassVolume.Secret.Optional) assert.Equal(t, "redis-initial-pass", deployment.Spec.Template.Spec.Volumes[1].Name) + assert.NotNil(t, deployment.Spec.Template.Spec.Volumes[1].Secret)build/redis/init.sh.tpl (1)
256-263:sedescaping only handles/and&— may break on passwords containing\or other special characters.Line 262 escapes only
/and&for thesedsubstitution. If the generated password ever contains backslashes, backticks, or othersed-metacharacters (e.g., due to a future change toRedisDefaultAdminPasswordNumSymbolsor a user-supplied password), thesedon line 263 will silently produce a corrupt config or fail.Currently safe because
RedisDefaultAdminPasswordNumSymbols = 0incommon/defaults.go, but this is fragile. Consider using a delimiter that's less likely to collide (e.g.,|or#) or a more robust replacement approach.♻️ More robust escaping example
-ESCAPED_AUTH=$(echo "${AUTH}" | sed -e 's/[\/&]/\\&/g'); -sed -i "s/__REPLACE_DEFAULT_AUTH__/${ESCAPED_AUTH}/" "${REDIS_CONF}" "${SENTINEL_CONF}" +ESCAPED_AUTH=$(printf '%s\n' "${AUTH}" | sed -e 's/[\/&\\]/\\&/g'); +sed -i "s/__REPLACE_DEFAULT_AUTH__/${ESCAPED_AUTH}/" "${REDIS_CONF}" "${SENTINEL_CONF}"controllers/argocdagent/agent/deployment.go (1)
101-101: Extract duplicate Redis authentication utilities to avoid code duplication.The identical
MountRedisAuthToArgoandGetRedisAuthEnvimplementations exist in bothcontrollers/argocd/redis.goandcontrollers/argocdagent/redis.go. The codebase already contains a TODO noting this duplication. Consider creating a shared utility module (e.g.,controllers/argoutil/redis.go) and updating both packages to import from it.controllers/argocdagent/redis.go (1)
1-46: Track the TODO to deduplicate this file.This file is a verbatim copy of
controllers/argocd/redis.go. The TODO on line 1 acknowledges this. Consider extractingMountRedisAuthToArgoandGetRedisAuthEnv(along with the constants) into a shared package (e.g.,controllers/argoutilor a newcontrollers/redisutil) to eliminate the duplication and ensure future changes are applied consistently.Would you like me to open an issue to track deduplicating these Redis auth utilities into a shared package?
controllers/argocdagent/deployment_test.go (1)
427-435: Consider a more direct assertion for theOptionalfield.Line 432 uses
assert.NotEqual(t, ptr.To(true), redisAuthVolume.Secret.Optional)which passes becauseOptionalisnil. A directassert.Nilwould make the intent clearer and avoid a false pass ifOptionalwere set tofalse.Proposed change
- assert.NotEqual(t, ptr.To(true), redisAuthVolume.Secret.Optional) + assert.Nil(t, redisAuthVolume.Secret.Optional, "Redis auth secret should be required (Optional not set)")
tests/ginkgo/sequential/1-067_validate_redis_secure_comm_no_autotls_ha_test.go
Outdated
Show resolved
Hide resolved
olivergondza
force-pushed
the
redis-creds-via-volume
branch
from
8f21344 to
8da0f53
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
controllers/argocdagent/agent/deployment.go (1)
218-269:⚠️ Potential issue | 🟠 Major
updateDeploymentIfChangeddoes not reconcile Volumes or VolumeMounts.The function checks and updates many fields (image, env, args, security context, ports, service account name) but never compares or updates
VolumesorVolumeMounts. On upgrade from a prior version, existing agent deployments won't receive the newredis-initial-passvolume/mount until they are recreated.Proposed fix — add volume and volumemount reconciliation
+ redisAuthVolume, redisAuthMount := argocdagent.MountRedisAuthToArgo(cr) + expectedVolumeMounts := append(buildVolumeMounts(), redisAuthMount) + if !reflect.DeepEqual(deployment.Spec.Template.Spec.Containers[0].VolumeMounts, expectedVolumeMounts) { + log.Info("deployment container volume mounts are being updated") + changed = true + deployment.Spec.Template.Spec.Containers[0].VolumeMounts = expectedVolumeMounts + } + + expectedVolumes := append(buildVolumes(), redisAuthVolume) + if !reflect.DeepEqual(deployment.Spec.Template.Spec.Volumes, expectedVolumes) { + log.Info("deployment volumes are being updated") + changed = true + deployment.Spec.Template.Spec.Volumes = expectedVolumes + } + return deployment, changedcontrollers/argocdagent/deployment.go (1)
252-310:⚠️ Potential issue | 🟠 Major
updateDeploymentIfChangeddoes not reconcile volumes or volume mounts.The function compares image, env, args, ports, security context, and service account — but never compares
VolumesorVolumeMounts. If the Redis auth secret name changes (e.g., due to a CR rename) or any volume configuration drifts, the deployment won't be updated. The sibling reconciler inrepo_server.go(lines 407-423) does compare both volumes and volume mounts.Proposed fix — add volume and volume mount comparison
if !reflect.DeepEqual(deployment.Spec.Template.Spec.ServiceAccountName, saName) { log.Info("deployment service account name is being updated") changed = true deployment.Spec.Template.Spec.ServiceAccountName = saName } + desiredSpec := buildPrincipalSpec(compName, saName, cr) + if !reflect.DeepEqual(deployment.Spec.Template.Spec.Volumes, desiredSpec.Template.Spec.Volumes) { + log.Info("deployment volumes are being updated") + changed = true + deployment.Spec.Template.Spec.Volumes = desiredSpec.Template.Spec.Volumes + } + if !reflect.DeepEqual(deployment.Spec.Template.Spec.Containers[0].VolumeMounts, desiredSpec.Template.Spec.Containers[0].VolumeMounts) { + log.Info("deployment volume mounts are being updated") + changed = true + deployment.Spec.Template.Spec.Containers[0].VolumeMounts = desiredSpec.Template.Spec.Containers[0].VolumeMounts + } + return deployment, changed
🤖 Fix all issues with AI agents
In
`@tests/ginkgo/sequential/1-067_validate_redis_secure_comm_no_autotls_ha_test.go`:
- Around line 233-252: The test currently only waits for
argocdFixture.BeAvailable() and then reads pod logs, which can be flaky if
replicas aren't ready; modify the test to wait for all expected
deployments/statefulsets to have the desired ready replicas before checking logs
by calling the existing helper expectComponentsAreRunning() (or replicate its
logic) for the same expectedComponents slice (or use argoCD.Name + "-" prefixes)
to ensure each component's replicas are ready, then proceed to use
osFixture.ExecCommandWithOutputParam and assert log contents as before.
🧹 Nitpick comments (5)
controllers/argocd/redis.go (1)
1-45: Clean implementation of centralized Redis auth helpers.The volume/mount construction and env var helper are well-structured. The comment referencing the Argo CD docs is helpful for maintainability.
One optional improvement: consider setting
ReadOnly: trueon the volume mount since these credentials should only be read by the consuming containers, not written to.Suggested improvement
mount = corev1.VolumeMount{ Name: redisAuthVolumeName, MountPath: redisAuthMountPath, + ReadOnly: true, }controllers/argocdagent/redis.go (1)
1-46: Duplicated Redis auth helpers across packages — consider extracting toargoutil.This file is a verbatim copy of
controllers/argocd/redis.go(same constants, same functions). TheTODOon line 1 acknowledges this. Since bothMountRedisAuthToArgoandGetRedisAuthEnvonly depend onargoutil.GetSecretNameWithSuffixand core Kubernetes types, they could live in thecontrollers/argoutilpackage to eliminate the duplication and ensure both consumers stay in sync.Same optional suggestion as the other copy: consider
ReadOnly: trueon the volume mount.controllers/argocdagent/agent/deployment_test.go (1)
407-448: Volume mount and volume assertions validate redis auth integration.The test correctly verifies the presence and naming of the
redis-initial-passvolume and mount. Consider also asserting the mount path (/app/config/redis-auth/) for the second mount to fully validate the wiring, though this is a minor gap since it's covered by the constant in the implementation.controllers/argocdagent/deployment_test.go (1)
430-435: Weak assertion onOptionalfield — considerassert.Nilinstead.Line 432 uses
assert.NotEqual(t, ptr.To(true), redisAuthVolume.Secret.Optional)which passes for bothnilandptr.To(false). SinceMountRedisAuthToArgodoesn't setOptionalat all (leaving itnil), a more precise assertion would strengthen this test.Proposed fix
- assert.NotEqual(t, ptr.To(true), redisAuthVolume.Secret.Optional) + assert.Nil(t, redisAuthVolume.Secret.Optional, "Redis auth volume should not be optional")controllers/argocd/deployment_test.go (1)
2370-2383: Inconsistent use ofredisAuthVolumeNameconstant vs hardcoded string.
repoServerDefaultVolumes()hardcodes"redis-initial-pass"on Line 2371, whileserverDefaultVolumes()(Line 2480) uses theredisAuthVolumeNameconstant. If the constant changes, this test helper will silently drift out of sync.Use the constant consistently
- { - Name: "redis-initial-pass", + { + Name: redisAuthVolumeName,
tests/ginkgo/sequential/1-067_validate_redis_secure_comm_no_autotls_ha_test.go
Show resolved
Hide resolved
olivergondza
force-pushed
the
redis-creds-via-volume
branch
from
8da0f53 to
668da65
Compare
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
controllers/argocdagent/agent/deployment.go (1)
219-271:⚠️ Potential issue | 🟠 Major
updateDeploymentIfChangeddoes not reconcile VolumeMounts or Volumes.The function checks image, env, args, security context, ports, and service account, but never compares
VolumeMountsorVolumes. If the Redis auth secret name changes (e.g., after a CR rename or secret rotation), the existing deployment won't be updated to reflect the new volume configuration.The same issue exists in
controllers/argocdagent/deployment.goat lines 252–310 (updateDeploymentIfChangedfor the principal deployment).🔧 Proposed fix: add volume/mount drift detection
+ desiredSpec := buildAgentSpec(compName, saName, cr) + + if !reflect.DeepEqual(deployment.Spec.Template.Spec.Containers[0].VolumeMounts, desiredSpec.Template.Spec.Containers[0].VolumeMounts) { + log.Info("deployment container volume mounts are being updated") + changed = true + deployment.Spec.Template.Spec.Containers[0].VolumeMounts = desiredSpec.Template.Spec.Containers[0].VolumeMounts + } + + if !reflect.DeepEqual(deployment.Spec.Template.Spec.Volumes, desiredSpec.Template.Spec.Volumes) { + log.Info("deployment volumes are being updated") + changed = true + deployment.Spec.Template.Spec.Volumes = desiredSpec.Template.Spec.Volumes + } + return deployment, changed
🤖 Fix all issues with AI agents
In `@build/redis/init.sh.tpl`:
- Around line 256-263: The sed escaping for AUTH doesn't handle backslashes,
corrupting passwords with '\' — update the escaping logic around AUTH to also
escape backslashes before running the sed replacement: when building
ESCAPED_AUTH (and ESCAPED_AUTH_SENTINEL) first replace each '\' with '\\' and
then escape '/' and '&' (or use a single safe sequence that escapes \, /, and &)
so the sed replacement on REDIS_CONF and SENTINEL_CONF uses a fully escaped
replacement string; apply the same fix to the ESCAPED_AUTH_SENTINEL variable
referenced later.
- Line 174: The sentinel commands (redis-cli sentinel failover and sentinel
get-master-addr-by-name) are missing authentication and the placeholder
injection is wrong; update the calls that use sentinel (referenced by the
redis-cli invocations that check NOGOODSLAVE and the get-master-addr-by-name
calls) to supply Sentinel auth either by adding -a "${SENTINELAUTH}" to those
redis-cli sentinel commands or by prefixing with env
REDISCLI_AUTH="${SENTINELAUTH}" when SENTINELAUTH is set, and modify the
template replacement logic to target a new placeholder
__REPLACE_DEFAULT_SENTINEL_AUTH__ in sentinel.conf.tpl (or add that placeholder
to the template) so requirepass/auth-pass is actually written; alternatively,
explicitly document lack of Sentinel requirepass support if you choose not to
implement auth.
In `@controllers/argocd/deployment_test.go`:
- Around line 2370-2383: Replace the hardcoded volume name "redis-initial-pass"
with the redisAuthVolumeName constant in repoServerDefaultVolumes() so it
matches the other places (repoServerDefaultVolumeMounts(),
serverDefaultVolumes(), serverDefaultVolumeMounts()); update the Name field in
the Volume spec to use redisAuthVolumeName instead of the literal string to
ensure consistent use of the constant across the codebase.
🧹 Nitpick comments (1)
tests/ginkgo/sequential/1-067_validate_redis_secure_comm_no_autotls_ha_test.go (1)
26-27: Consider avoiding direct import of controller internals in E2E tests.Importing
controllers/argoutilcouples this E2E test to internal controller logic. IfGetSecretNameWithSuffixchanges its naming scheme, this test silently follows rather than catching the regression. A simpler approach would be to hardcode the expected secret name (e.g.,"argocd-redis-initial-password") or usekubectl get secretto discover it, keeping E2E tests decoupled from implementation details.
olivergondza
force-pushed
the
redis-creds-via-volume
branch
from
668da65 to
5c382f7
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
controllers/argocdagent/deployment.go (1)
252-310:⚠️ Potential issue | 🟠 Major
updateDeploymentIfChangeddoes not reconcile Volumes or VolumeMounts.Existing principal deployments won't pick up the new
redisAuthVolume/redisAuthMountuntil they're deleted and recreated, since this function only compares Selector, Image, ImagePullPolicy, Name, Env, Args, SecurityContext, Ports, and ServiceAccountName.Consider adding volume and volume-mount drift detection, similar to how Env or Args are handled.
Proposed fix
if !reflect.DeepEqual(deployment.Spec.Template.Spec.ServiceAccountName, saName) { log.Info("deployment service account name is being updated") changed = true deployment.Spec.Template.Spec.ServiceAccountName = saName } + + desiredSpec := buildPrincipalSpec(compName, saName, cr) + if !reflect.DeepEqual(deployment.Spec.Template.Spec.Volumes, desiredSpec.Template.Spec.Volumes) { + log.Info("deployment volumes are being updated") + changed = true + deployment.Spec.Template.Spec.Volumes = desiredSpec.Template.Spec.Volumes + } + + if !reflect.DeepEqual(deployment.Spec.Template.Spec.Containers[0].VolumeMounts, desiredSpec.Template.Spec.Containers[0].VolumeMounts) { + log.Info("deployment container volume mounts are being updated") + changed = true + deployment.Spec.Template.Spec.Containers[0].VolumeMounts = desiredSpec.Template.Spec.Containers[0].VolumeMounts + } return deployment, changedcontrollers/argocdagent/agent/deployment.go (1)
217-269:⚠️ Potential issue | 🟠 MajorSame gap as in the principal deployment:
updateDeploymentIfChangeddoes not reconcile Volumes or VolumeMounts.Existing agent deployments won't pick up new volume/mount changes. See the same comment on
controllers/argocdagent/deployment.goLine 252-310.controllers/argocd/deployment.go (1)
223-239:⚠️ Potential issue | 🔴 CriticalStandalone Redis lacks authentication configuration.
getArgoRedisArgs()does not include--requirepass. The auth secret is mounted at/app/config/redis-auth/authviaredisMount, but the standard Redis container image does not automatically read credentials from a mounted file path—it requires either--requirepass <password>, anaclfiledirective, or a config file.Unlike HA Redis which uses a
redis.conffile with authentication configured, standalone Redis receives only basic args (--save,--appendonly, TLS settings) and no way to enforce authentication. While ArgoCD client components receiveREDIS_CREDS_DIR_PATHenv var to locate credentials, the Redis server itself has no mechanism to require them, leaving standalone Redis unprotected.
🤖 Fix all issues with AI agents
In `@controllers/argoutil/redis.go`:
- Around line 318-320: GenerateAgentPrincipalRedisProxyServiceName currently
uses the raw crName which can produce >63-char Kubernetes service names; change
it to use the truncated CR name helper (e.g., call GetTruncatedCRName(crName))
or the shared nameWithSuffix/GetSecretNameWithSuffix pattern before appending
the "-agent-principal-redisproxy" suffix so the final service name stays within
the 63-character limit; update GenerateAgentPrincipalRedisProxyServiceName to
derive a truncated base via GetTruncatedCRName (or reuse nameWithSuffix) and
then format the final service name using that truncated base.
In `@controllers/argoutil/resource.go`:
- Around line 135-139: FqdnServiceRef currently builds FQDNs using
NameWithSuffix(cr.ObjectMeta, service) which uses the raw CR name, causing
mismatches for long CR names; update FqdnServiceRef to construct service names
the same way services are created by using GetTruncatedCRName(cr) (the same
truncation used by nameWithSuffix) when combining with the service suffix so the
returned FQDN matches the actual service name (update FqdnServiceRef to call
GetTruncatedCRName and then append the service suffix instead of using
NameWithSuffix).
🧹 Nitpick comments (3)
controllers/argocdagent/agent/deployment_test.go (1)
432-448: Consider usingrequire.Leninstead ofassert.Lento prevent index-out-of-bounds panic.If
assert.Lenfails on line 433, execution continues to line 438 which accessescontainer.VolumeMounts[1], causing a panic. Same applies to the volumes check on lines 441/447. Usingrequire.Lenwould stop the test immediately on failure, providing a cleaner error.This is a minor robustness concern for test readability.
Proposed fix
- assert.Len(t, container.VolumeMounts, 2) + require.Len(t, container.VolumeMounts, 2) userpassMount := container.VolumeMounts[0] assert.Equal(t, "userpass-passwd", userpassMount.Name) assert.Equal(t, "/app/config/creds", userpassMount.MountPath) assert.Equal(t, "redis-initial-pass", container.VolumeMounts[1].Name) // Verify specific volume details - assert.Len(t, deployment.Spec.Template.Spec.Volumes, 2) + require.Len(t, deployment.Spec.Template.Spec.Volumes, 2)Note: This would require adding
"github.com/stretchr/testify/require"to the imports.controllers/argocdagent/deployment_test.go (1)
388-435: Tests correctly verify the new Redis auth volume/mount integration.Minor note: Line 432's
assert.NotEqual(t, ptr.To(true), redisAuthVolume.Secret.Optional)is a weak check — it passes for bothnilandptr.To(false). If the intent is thatOptionalis unset (nil),assert.Nilis clearer.Suggested improvement
- assert.NotEqual(t, ptr.To(true), redisAuthVolume.Secret.Optional) + assert.Nil(t, redisAuthVolume.Secret.Optional, "Redis auth volume secret should not be optional")controllers/argocd/statefulset.go (1)
418-426: Redundant call toargoutil.GetRedisHAContainerImage(cr)on line 426.
desiredImage(line 420) already holds the result ofargoutil.GetRedisHAContainerImage(cr). Line 426 calls it again instead of reusingdesiredImage.Proposed fix
- existing.Spec.Template.Spec.Containers[i].Image = argoutil.GetRedisHAContainerImage(cr) + existing.Spec.Template.Spec.Containers[i].Image = desiredImage
Mangaal
left a comment
Mangaal
left a comment
There was a problem hiding this comment.
Hi @olivergondza ,
I have left a comment. Please feel free to resolve if they are not relevant.
I will also test this locally.
Thanks for your work on this.
|
Thanks, @Mangaal. I am not well versed with operator's Redis use, so I will certainly appreciate it if you take a look. |
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In
`@tests/ginkgo/sequential/1-067_validate_redis_secure_comm_no_autotls_ha_test.go`:
- Around line 237-238: The test's Eventually assertion using argoCD and
BeAvailable currently uses a 5-minute timeout which can flake; update the
Eventually(argoCD, "5m", "5s").Should(argocdFixture.BeAvailable()) call to use
the same timeout as the sibling HA test by changing the duration arguments to
"10m", "10s" so the call becomes Eventually(argoCD, "10m",
"10s").Should(argocdFixture.BeAvailable()).
- Line 253: Update the typo in the comment "Some logs how redis disconnect
manifests" to a grammatically correct phrase such as "Some logs showing how
Redis disconnect manifests" (or "Some logs of how Redis disconnects manifest"),
and ensure "Redis" is capitalized; locate the comment by searching for the exact
string "Some logs how redis disconnect manifests" in the test file and replace
it accordingly.
tests/ginkgo/sequential/1-067_validate_redis_secure_comm_no_autotls_ha_test.go
Outdated
Show resolved
Hide resolved
tests/ginkgo/sequential/1-067_validate_redis_secure_comm_no_autotls_ha_test.go
Outdated
Show resolved
Hide resolved
| return pl | ||
| } | ||
|
|
||
| // getRedisConfigPath will return the path for the Redis configuration templates. |
There was a problem hiding this comment.
There redis specific methods are moved to its own file in argoutil.
olivergondza
force-pushed
the
redis-creds-via-volume
branch
5 times, most recently
from
c705d1d to
e6b65ae
Compare
olivergondza
force-pushed
the
redis-creds-via-volume
branch
from
9378235 to
c98c290
Compare
|
@Mangaal, please take another look. CI tests may not be passing cleanly yet... |
Mangaal
left a comment
Mangaal
left a comment
There was a problem hiding this comment.
The overall PR looks good to me, and I have also tested it locally.
The argocd components and redis are using file mounted secrets, except for the agent.
Thanks @olivergondza
|
I hope all tests pass successfully this time. 😄 |
olivergondza
force-pushed
the
redis-creds-via-volume
branch
from
287a606 to
d749cbe
Compare
olivergondza
force-pushed
the
redis-creds-via-volume
branch
from
d749cbe to
df0871e
Compare
|
just a doubt, is upgrading to the version which has this PR will be a breaking change ? |
|
small doubt, is the path for file containing creds can be in any directory ? or only should be avaialble in hardcoded location ? |
|
All Doubts I have:
|
|
@akhilnittala, thanks for your feedback.
I believe this does not require any migration, because everything this is changing is happening inside pods and is fully deleted/recreated with their every restart (pod volume, container volume mount). So what I expect to happen is that as soon as new version of operator kicks in, the reconciliation starts producing updated version of manifests (different envvar, + pod volume, + container volume mount) and rolls out the updated pods in a way that is transparent to the users.
This is an interesting idea, but it is out of the scope of this effort. We were not proactively restarting things even before, so this does not violate users' expectations in any way. If we choose to implement this, we have to be careful so restart of the redis, sentinels and the connected components does not create more damage than good.
Good point. It used to issue a warning, but I have added explicit exit to fail fast and loud.
Sorry, I cannot tell if this can be a problem. But I would expect that reasonable SCC permits reading secret files mounted to the container. I guess we will learn that no later than in the PR for gitops-operator :D |
Thanks @olivergondza for your answers. Sounds good for me. |
… mount Signed-off-by: Oliver Gondža <ogondza@gmail.com>
…nvvar Signed-off-by: Oliver Gondža <ogondza@gmail.com>
Signed-off-by: Oliver Gondža <ogondza@gmail.com>
Signed-off-by: Oliver Gondža <ogondza@gmail.com>
Signed-off-by: Oliver Gondža <ogondza@gmail.com>
Signed-off-by: Oliver Gondža <ogondza@gmail.com>
Signed-off-by: Oliver Gondža <ogondza@gmail.com>
Agent is not ready yet. Ref.: https://issues.redhat.com/browse/GITOPS-9070 Signed-off-by: Oliver Gondža <ogondza@gmail.com>
Signed-off-by: Oliver Gondža <ogondza@gmail.com>
…writes Signed-off-by: Oliver Gondža <ogondza@gmail.com>
Signed-off-by: Oliver Gondža <ogondza@gmail.com>
Signed-off-by: Oliver Gondža <ogondza@gmail.com>
olivergondza
force-pushed
the
redis-creds-via-volume
branch
from
5f1a593 to
0b78b2b
Compare
Signed-off-by: Oliver Gondža <ogondza@gmail.com>
This is now needed when we distribute redis creds are a volume. Signed-off-by: Oliver Gondža <ogondza@gmail.com>
olivergondza
force-pushed
the
redis-creds-via-volume
branch
from
bf87cf4 to
d9a6006
Compare
olivergondza
requested review from
Mangaal and
svghadi
and removed request for
Mangaal
3 participants
What type of PR is this?
/kind enhancement
What does this PR do / why we need it:
Improve security by injection redis credentials to components by avoiding envvars, and cmdline arguments.
Agent/principal will be updated in follow-up PRs as it does not support
REDIS_CREDS_DIR_PATHfor now.Have you updated the necessary documentation?
Which issue(s) this PR fixes:
Fixes #?
How to test changes / Special notes to the reviewer:
Summary by CodeRabbit
Refactor
Tests