Secure agent_config by using volume mounts instead of env vars #1718
Conversation
aayushchouhan09
requested review from
a team,
VershaAgrawal,
dannyzaken,
jackyalbo and
liranmauda
and removed request for
a team
WalkthroughReplaced env var injection of AGENT_CONFIG with a file-backed AGENT_CONFIG_PATH in the agent Pod manifest and bundle; reconciler now mounts an Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant R as BackingStore Reconciler
participant T as Pod Template
participant K as Kubernetes API
Note over R: Reconciling pool / pod spec
R->>T: Build/modify Pod template
activate T
T->>T: Add volume `agent-config-secret` (secretName placeholder)
T->>T: Add volumeMount at /etc/agent-config (readOnly)
T->>T: Set env `AGENT_CONFIG_PATH` = "/etc/agent-config/agent_config"
deactivate T
R->>K: Apply Pod template (create/update)
K-->>R: Status / result
Note right of T: previous flow removed: secret value injected into ENV `AGENT_CONFIG`
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Potential review focal points:
Possibly related PRs
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro 📒 Files selected for processing (4)
🚧 Files skipped from review as they are similar to previous changes (4)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (12)
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: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
pkg/backingstore/reconciler.go (1)
1165-1262: Add migration check for environment variable change.The
needUpdate()function doesn't detect the environment variable name change fromAGENT_CONFIGtoAGENT_CONFIG_PATH, nor does it check for the new volume mount. This means existing pods will continue running with the old insecure approach (config in env var) instead of automatically adopting the new secure volume mount approach.Apply this diff to detect pods that need migration:
func (r *Reconciler) needUpdate(pod *corev1.Pod) bool { var c = &pod.Spec.Containers[0] + + // Check for migration from AGENT_CONFIG to AGENT_CONFIG_PATH + volume mount + agentConfigPathVar := util.GetEnvVariable(&c.Env, "AGENT_CONFIG_PATH") + oldAgentConfigVar := util.GetEnvVariable(&c.Env, "AGENT_CONFIG") + hasAgentConfigVolume := false + for _, vm := range c.VolumeMounts { + if vm.MountPath == agentConfigSecretMountPath { + hasAgentConfigVolume = true + break + } + } + if oldAgentConfigVar != nil || agentConfigPathVar == nil || !hasAgentConfigVolume { + r.Logger.Warnf("Pod needs migration from AGENT_CONFIG env var to AGENT_CONFIG_PATH with volume mount") + return true + } + for _, name := range []string{"HTTP_PROXY", "HTTPS_PROXY", "NO_PROXY"} {
🧹 Nitpick comments (2)
pkg/backingstore/reconciler.go (2)
999-1002: Consider handling migration from old secret key.The secret key has changed from
AGENT_CONFIGtoagent_config(lowercase). While the lowercase naming follows better conventions, existing secrets containing the oldAGENT_CONFIGkey won't be detected, causing the config to be refetched and stored under the new key name. This leaves orphaned data in the secret.Consider adding a migration check:
if r.Secret.StringData["agent_config"] == "" { + // Migrate from old key name if present + if oldConfig := r.Secret.StringData["AGENT_CONFIG"]; oldConfig != "" { + r.Secret.StringData["agent_config"] = oldConfig + delete(r.Secret.StringData, "AGENT_CONFIG") + util.KubeUpdate(r.Secret) + } else { r.Secret.StringData["agent_config"] = res util.KubeUpdate(r.Secret) + } +} else { + // Clean up old key if present + if _, exists := r.Secret.StringData["AGENT_CONFIG"]; exists { + delete(r.Secret.StringData, "AGENT_CONFIG") + util.KubeUpdate(r.Secret) + } }
1292-1293: Consider using filepath.Join for path construction.String concatenation for building file paths is fragile and could break if
agentConfigSecretMountPathis modified to include a trailing slash.Apply this diff to use proper path joining:
+import ( + "path/filepath" + // ... other imports +) case "AGENT_CONFIG_PATH": - c.Env[j].Value = agentConfigSecretMountPath + "/agent_config" + c.Env[j].Value = filepath.Join(agentConfigSecretMountPath, "agent_config")
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
deploy/internal/pod-agent.yaml(1 hunks)pkg/backingstore/reconciler.go(5 hunks)pkg/bundle/deploy.go(2 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
pkg/backingstore/reconciler.go (2)
pkg/nb/types.go (1)
GetHostsPoolAgentConfigParams(455-458)pkg/util/util.go (2)
MergeVolumeMountList(1631-1643)MergeVolumeList(1616-1628)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (13)
- GitHub Check: run-kms-tls-sa-test
- GitHub Check: golangci-lint
- GitHub Check: cnpg-deployment-test
- GitHub Check: run-hac-test
- GitHub Check: run-operator-tests
- GitHub Check: run-azure-vault-test
- GitHub Check: run-kms-tls-token-test
- GitHub Check: run-kms-key-rotate-test
- GitHub Check: run-kms-dev-test
- GitHub Check: run-cli-tests
- GitHub Check: run-admission-test
- GitHub Check: run-kms-kmip-test
- GitHub Check: run-core-config-map-tests
🔇 Additional comments (4)
deploy/internal/pod-agent.yaml (1)
24-24: LGTM! Environment variable renamed appropriately.The rename from
AGENT_CONFIGtoAGENT_CONFIG_PATHcorrectly reflects the new architecture where the config is accessed via a file path rather than directly through an environment variable.pkg/backingstore/reconciler.go (3)
35-35: LGTM! Mount path constant defined clearly.The constant provides a clear, centralized definition for the agent config mount path.
1353-1370: LGTM! Volume mount implementation is secure.The implementation correctly:
- Mounts the agent config secret as a read-only volume
- Uses merge functions to prevent duplicate volume definitions
- Follows Kubernetes best practices for secret mounting
The read-only mount enhances security by preventing the agent from modifying its own configuration.
1292-1293: Verify noobaa-agent reads its config from the file specified by AGENT_CONFIG_PATH. Ensure the agent calls os.Getenv("AGENT_CONFIG_PATH") and loads that file (e.g., via os.ReadFile), rather than expecting raw config data in an env var.
|
I am not convinced that this is the way of mounting. |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
pkg/backingstore/reconciler.go (1)
1353-1365: Consider more robust placeholder replacement with validation.The current approach uses string comparisons to find and replace placeholders (
AGENT_CONFIG_MOUNT_PATHandAGENT_CONFIG_SECRET_NAME). This is fragile because:
- If the placeholders in the template YAML change, this code will silently fail to replace them
- There's no validation to confirm the replacements actually occurred
- String literals are repeated rather than defined as constants
Consider these improvements:
Option 1 (Recommended): Add constants and validation:
+const ( + agentConfigMountPathPlaceholder = "AGENT_CONFIG_MOUNT_PATH" + agentConfigSecretNamePlaceholder = "AGENT_CONFIG_SECRET_NAME" +) + func (r *Reconciler) updatePodTemplate() error { log := r.Logger.WithField("func", "updatePodTemplate") + mountPathReplaced := false + secretNameReplaced := false + // ... existing code ... // replace AGENT_CONFIG_MOUNT_PATH with actual mount path for i := range r.PodAgentTemplate.Spec.Containers[0].VolumeMounts { - if r.PodAgentTemplate.Spec.Containers[0].VolumeMounts[i].MountPath == "AGENT_CONFIG_MOUNT_PATH" { + if r.PodAgentTemplate.Spec.Containers[0].VolumeMounts[i].MountPath == agentConfigMountPathPlaceholder { r.PodAgentTemplate.Spec.Containers[0].VolumeMounts[i].MountPath = agentConfigSecretMountPath + mountPathReplaced = true } } // replace AGENT_CONFIG_SECRET_NAME with actual secret name for i := range r.PodAgentTemplate.Spec.Volumes { - if r.PodAgentTemplate.Spec.Volumes[i].Secret != nil && r.PodAgentTemplate.Spec.Volumes[i].Secret.SecretName == "AGENT_CONFIG_SECRET_NAME" { + if r.PodAgentTemplate.Spec.Volumes[i].Secret != nil && r.PodAgentTemplate.Spec.Volumes[i].Secret.SecretName == agentConfigSecretNamePlaceholder { r.PodAgentTemplate.Spec.Volumes[i].Secret.SecretName = r.Secret.Name + secretNameReplaced = true } } + + if !mountPathReplaced || !secretNameReplaced { + return fmt.Errorf("failed to replace pod template placeholders: mountPath=%v, secretName=%v", mountPathReplaced, secretNameReplaced) + } return r.updatePodResourcesTemplate(c) }Option 2: Use a more structured approach by directly setting the values in the template rather than relying on placeholder replacement.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
deploy/internal/pod-agent.yaml(3 hunks)pkg/backingstore/reconciler.go(5 hunks)pkg/bundle/deploy.go(4 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- pkg/bundle/deploy.go
- deploy/internal/pod-agent.yaml
🧰 Additional context used
🧬 Code graph analysis (1)
pkg/backingstore/reconciler.go (1)
pkg/nb/types.go (1)
GetHostsPoolAgentConfigParams(455-458)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (13)
- GitHub Check: run-hac-test
- GitHub Check: run-admission-test
- GitHub Check: run-operator-tests
- GitHub Check: golangci-lint
- GitHub Check: run-kms-tls-sa-test
- GitHub Check: run-azure-vault-test
- GitHub Check: run-kms-dev-test
- GitHub Check: run-cli-tests
- GitHub Check: run-kms-key-rotate-test
- GitHub Check: cnpg-deployment-test
- GitHub Check: run-kms-kmip-test
- GitHub Check: run-kms-tls-token-test
- GitHub Check: run-core-config-map-tests
🔇 Additional comments (2)
pkg/backingstore/reconciler.go (2)
35-35: LGTM!The constant definition is clear and follows Go naming conventions. Using a constant for the mount path ensures consistency throughout the reconciler.
1292-1293: LGTM! Security improvement by using file-based config.The change from
AGENT_CONFIG(env var) toAGENT_CONFIG_PATH(file path) correctly implements the volume mount approach, which is more secure as it avoids exposing sensitive config in environment variables.
@liranmauda If we put the volume mount directly in the But I think our old way of using utility functions is better and easier to understand. wdyt? |
tangledbytes
left a comment
tangledbytes
left a comment
There was a problem hiding this comment.
LGTM. Just one minor comment (feel free to ignore).
aayushchouhan09
force-pushed
the
agent-fix
branch
from
1f6d638 to
0748838
Compare
Signed-off-by: Aayush Chouhan <[email protected]>
aayushchouhan09
force-pushed
the
agent-fix
branch
from
0748838 to
0d3ea0e
Compare
4 participants
Explain the changes
AGENT_CONFIGenvironment variable with secure volume mount approachAGENT_CONFIG_PATHenvironment variable pointing to mounted config fileagent_configsecret key for naming consistencyIssues: Fixed #xxx / Gap #xxx
Testing Instructions:
AGENT_CONFIG_PATHin backingstore pod.Summary by CodeRabbit
Refactor
Chores