Skip to content

feat: add argo rollouts to secret auto reload watchlist#43

Open
KevinYobeth wants to merge 3 commits intoInfisical:mainfrom
KevinYobeth:main
Open

feat: add argo rollouts to secret auto reload watchlist#43
KevinYobeth wants to merge 3 commits intoInfisical:mainfrom
KevinYobeth:main

Conversation

@KevinYobeth
Copy link
Copy Markdown

This PR adds support for managing Argo Rollouts resources in the secrets operator, allowing secrets updates to trigger rollout updates similar to how deployments and statefulsets are handled

Related Issue: Infisical/infisical#1305

Copy link
Copy Markdown

@greptile-apps greptile-apps bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Greptile Overview

Greptile Summary

Overview

This PR adds support for Argo Rollouts to the secrets operator's auto-reload watchlist, enabling secret updates to trigger rollout restarts similar to how Deployments, DaemonSets, and StatefulSets are handled.

Changes Made

Core Implementation:

  • Added IsRolloutUsingManagedSecret() function to detect if a Rollout uses managed secrets
  • Added ReconcileRollout() function to update Rollout annotations and trigger restarts
  • Integrated Rollout reconciliation into the main ReconcileDeploymentsWithManagedSecrets() flow
  • Added error handling for cases where Argo Rollouts CRD is not installed (gracefully continues)

Infrastructure Updates:

  • Added Argo Rollouts v1alpha1 scheme registration in cmd/main.go
  • Added RBAC permissions for argoproj.io/rollouts (get, list, watch, update, patch) across all deployment manifests:
    • config/rbac/role.yaml
    • helm-charts/secrets-operator/templates/manager-rbac.yaml
    • kubectl-install/install-secrets-operator.yaml
  • Added github.com/argoproj/argo-rollouts v1.7.2 dependency in go.mod

Implementation Pattern

The implementation follows the established pattern used for Deployments, DaemonSets, and StatefulSets:

  1. Lists all Rollouts in the namespace
  2. Filters for those with annotation secrets.infisical.com/auto-reload: "true"
  3. Checks if the Rollout uses the managed secret (via envFrom, env, or volumes)
  4. Updates annotations to trigger pod restart when secret version changes
  5. Uses goroutines for concurrent reconciliation

Key Behavior

  • Optional CRD: If Argo Rollouts CRD is not installed, the operator logs an error but continues functioning for other resource types
  • Namespace-scoped: Rollout reconciliation respects the operator's namespace scope setting
  • Version tracking: Uses annotation secrets.infisical.com/managed-secret.<secret-name> to track secret versions

Issues Found

  1. Potential nil pointer panic (line 331): Missing nil check before assigning to rollout.Annotations map
  2. Init containers not checked: The IsRolloutUsingManagedSecret function doesn't check init containers (consistent with existing functions but worth improving)

Related Issue

Closes Infisical/infisical#1305

Confidence Score: 4/5

  • This PR is generally safe to merge with one logical issue that should be addressed to prevent potential runtime panics
  • Score of 4 reflects a well-structured implementation that follows existing patterns and includes proper RBAC configuration across all deployment manifests. The main concern is a potential nil pointer panic when accessing rollout.Annotations (line 331) without a nil check. While this is unlikely to occur in practice since Kubernetes resources typically have annotations initialized, it represents a logical flaw that could cause the operator to crash. The implementation is otherwise solid: error handling for missing CRD is appropriate, RBAC permissions are correctly scoped, and the integration follows established patterns. Init container support is missing but this is consistent with existing code and a style improvement rather than a critical issue.
  • Pay close attention to internal/controllerhelpers/controllerhelpers.go line 331 - the nil pointer issue should be fixed before merge

Important Files Changed

File Analysis

Filename Score Overview
internal/controllerhelpers/controllerhelpers.go 3/5 Added Argo Rollouts support with functions IsRolloutUsingManagedSecret and ReconcileRollout. Implementation follows existing patterns for deployments/daemonsets/statefulsets. Issue: potential nil pointer panic if rollout.Annotations is nil (line 331), and missing nil check for Annotations map before assignment.
cmd/main.go 5/5 Added Argo Rollouts scheme registration. Clean import and initialization following Kubernetes operator best practices.
config/rbac/role.yaml 5/5 Added RBAC permissions for argoproj.io rollouts (get, list, watch, update, patch). Permissions are appropriate for the auto-reload feature.
helm-charts/secrets-operator/templates/manager-rbac.yaml 5/5 Added RBAC permissions for argoproj.io rollouts in Helm chart. Consistent with config/rbac/role.yaml.
kubectl-install/install-secrets-operator.yaml 5/5 Added RBAC permissions for argoproj.io rollouts in kubectl install manifest. Consistent with other RBAC files.
go.mod 4/5 Added github.com/argoproj/argo-rollouts v1.7.2 dependency. Some transitive dependencies were removed due to the dependency resolution.

Comment thread internal/controllerhelpers/controllerhelpers.go
Comment on lines +292 to +313
func IsRolloutUsingManagedSecret(rollout argorollouts.Rollout, managedSecret v1alpha1.ManagedKubeSecretConfig) bool {
managedSecretName := managedSecret.SecretName
for _, container := range rollout.Spec.Template.Spec.Containers {
for _, envFrom := range container.EnvFrom {
if envFrom.SecretRef != nil && envFrom.SecretRef.LocalObjectReference.Name == managedSecretName {
return true
}
}
for _, env := range container.Env {
if env.ValueFrom != nil && env.ValueFrom.SecretKeyRef != nil && env.ValueFrom.SecretKeyRef.LocalObjectReference.Name == managedSecretName {
return true
}
}
}
for _, volume := range rollout.Spec.Template.Spec.Volumes {
if volume.Secret != nil && volume.Secret.SecretName == managedSecretName {
return true
}
}

return false
}
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The IsRolloutUsingManagedSecret function only checks regular containers, but not init containers. If a secret is used in an init container (via rollout.Spec.Template.Spec.InitContainers), the auto-reload won't detect it.

Consider adding init container checks for completeness:

// Check init containers as well
for _, container := range rollout.Spec.Template.Spec.InitContainers {
    for _, envFrom := range container.EnvFrom {
        if envFrom.SecretRef != nil && envFrom.SecretRef.LocalObjectReference.Name == managedSecretName {
            return true
        }
    }
    for _, env := range container.Env {
        if env.ValueFrom != nil && env.ValueFrom.SecretKeyRef != nil && env.ValueFrom.SecretKeyRef.LocalObjectReference.Name == managedSecretName {
            return true
        }
    }
}

Note: This same limitation exists in the other Is*UsingManagedSecret functions (lines 145-166, 168-190, 192-213), so this is consistent with the existing pattern, but worth improving across all resource types.

Co-authored-by: greptile-apps[bot] <165735046+greptile-apps[bot]@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant