WIP: Restructure config generation for the operands#53
WIP: Restructure config generation for the operands#53anirudhAgniRedhat wants to merge 2 commits intoopenshift:mainfrom
Conversation
openshift-ci
bot
added
the
do-not-merge/work-in-progress
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: anirudhAgniRedhat The full list of commands accepted by this bot can be found here. The pull request process is described here DetailsNeeds approval from an approver in each of these files:
Approvers can indicate their approval by writing |
openshift-ci
bot
added
the
approved
WalkthroughAdds comprehensive typed configuration models for SPIRE agent, server, and OIDC discovery provider; introduces many package-level constants; refactors controllers and DaemonSet/StatefulSet generators to use constants; implements builder functions to produce typed configs; and extends test coverage for new structures and refactorings. Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Areas requiring extra attention:
Pre-merge checks and finishing touches❌ Failed checks (2 warnings)
✅ Passed checks (1 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro Cache: Disabled due to data retention organization setting Knowledge base: Disabled due to 📒 Files selected for processing (5)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 golangci-lint (2.5.0)Error: can't load config: unsupported version of the configuration: "" See https://golangci-lint.run/docs/product/migration-guide for migration instructions 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: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
pkg/controller/spire-server/controller.go (1)
269-284: Fix misleading update log for controller-manager ConfigMap.The "Updated ConfigMap..." log is outside the else branch, so it prints even when updates are skipped in create-only mode.
Apply:
@@ - } - r.log.Info("Updated ConfigMap with new config") + } + r.log.Info("Updated ConfigMap with new config")Move the log inside the
else { ... }block:@@ } else if err == nil && existingSpireControllerManagerCM.Data["controller-manager-config.yaml"] != spireControllerManagerConfigMap.Data["controller-manager-config.yaml"] { if createOnlyMode { r.log.Info("Skipping spire controller manager ConfigMap update due to create-only mode") } else { existingSpireControllerManagerCM.Data = spireControllerManagerConfigMap.Data if err = r.ctrlClient.Update(ctx, &existingSpireControllerManagerCM); err != nil { ... } + r.log.Info("Updated ConfigMap with new config") } - r.log.Info("Updated ConfigMap with new config")
🧹 Nitpick comments (19)
pkg/controller/spire-oidc-discovery-provider/configmap_test.go (1)
302-431: Consider consolidating overlapping test logic.The
TestGenerateOIDCConfigMapFromCR_WithStructstest has significant overlap with the earlierTestGenerateOIDCConfigMapFromCRtest (lines 18-187). Both test suites validate:
- Valid OIDC config with default and custom values
- ConfigMap structure and JSON content
- Nil CR error handling
- Invalid JWT issuer error handling
While having multiple test perspectives can be valuable, the duplication here adds maintenance burden without significantly improving coverage.
Consider either:
- Merging the unique validation logic from both test functions into a single comprehensive test suite
- Clearly differentiating the test objectives (e.g., one focuses on ConfigMap metadata, the other on JSON structure)
- Extracting common validation logic into helper functions to reduce duplication
pkg/controller/spiffe-csi-driver/constants.go (1)
79-87: Unconventional but acceptable constant pattern.Using
corev1.HostPathTypeconstants (lines 85-86) as aliased constants is unusual since these are already constants in the Kubernetes API. However, this approach maintains consistency with the PR's goal of centralizing all DaemonSet configuration values, making them easier to locate and modify.If you prefer a more conventional approach, you could use the
corev1constants directly indaemonset.goinstead of aliasing them here. However, the current approach is not incorrect and has the benefit of co-locating all configuration.pkg/controller/spire-server/controller.go (1)
5-5: Hash server.conf directly; drop double-marshal and unused import.Compute the hash from the actual ConfigMap payload to avoid redundant marshal, potential formatting drift, and keep parity with the YAML path. Also removes the encoding/json import here.
Apply:
@@ -import ( - "context" - "encoding/json" +import ( + "context" @@ - // Generate config map data for hash calculation - serverConfMap := generateServerConfMap(&server.Spec) - spireServerConfJSON, err := json.Marshal(serverConfMap) - if err != nil { - r.log.Error(err, "failed to marshal spire server config map to JSON") - return ctrl.Result{}, err - } - - spireServerConfigMapHash := generateConfigHash(spireServerConfJSON) + // Derive hash from the exact payload applied to the ConfigMap + serverConfJSON := spireServerConfigMap.Data["server.conf"] + spireServerConfigMapHash := generateConfigHashFromString(serverConfJSON)Also applies to: 224-232
pkg/controller/spire-server/configmap_test.go (1)
347-362: Consider asserting plugin parity in backward-compat test.You compare server and health_checks only. Add a lenient comparison for plugins (e.g., deep-equal after JSON round-trip) to catch drift between paths.
Example:
assert.Equal(t, newParsed["plugins"], oldParsed["plugins"])pkg/controller/spire-server/configmap.go (1)
97-104: Parse MemoryEnabled via helper for robustness.Accept truthy variants ("true", "True", "1") using the existing utility.
Apply:
- if spec.KeyManager != nil && spec.KeyManager.MemoryEnabled == "true" { + if spec.KeyManager != nil && utils.StringToBool(spec.KeyManager.MemoryEnabled) {pkg/controller/spire-server/constants.go (1)
4-30: Consider consolidating duplicate constants and aligning port types.Several constants are duplicated with different types or names:
DefaultServerBindPort(string "8081") duplicatesSpireServerPortGRPC(int32 8081) at line 65DefaultHealthCheckBindPort(string "8080") duplicatesSpireServerPortHealthz(int32 8080) at line 66DefaultHealthCheckLivePath("/live") duplicatesSpireServerProbePathLiveat line 71DefaultHealthCheckReadyPath("/ready") duplicatesSpireServerProbePathReadyat line 72This can lead to maintenance issues if one is updated but not the other.
Consider whether you need both string and int32 versions of the same port, or if you can standardize on one type and convert as needed.
pkg/controller/spire-agent/configmap_test.go (1)
411-426: Add OwnerReference assertions for GC.When you add OwnerReferences in the generator, assert that cm.OwnerReferences has a controller ref to the SpireAgent. This prevents orphaned ConfigMaps.
Example add:
require.Len(t, cm.OwnerReferences, 1) assert.True(t, *cm.OwnerReferences[0].Controller) assert.Equal(t, tt.spireAgentConfig.Name, cm.OwnerReferences[0].Name)Also applies to: 427-465
pkg/controller/spire-agent/daemonset.go (3)
113-116: Remove or mount Admin socket EmptyDir.Volume SpireAgentVolumeNameAdminSocketDir is defined but never mounted. Drop it or add the corresponding VolumeMount to avoid dead config and reduce container attack surface.
- {Name: SpireAgentVolumeNameAdminSocketDir, VolumeSource: corev1.VolumeSource{EmptyDir: &corev1.EmptyDirVolumeSource{}}},
50-56: Disable default SA token mount when using projected token.Since a scoped projected token is provided, set AutomountServiceAccountToken=false to prevent the default token from also mounting.
Template: corev1.PodTemplateSpec{ ObjectMeta: metav1.ObjectMeta{ // ... }, Spec: corev1.PodSpec{ + AutomountServiceAccountToken: boolPtr(false), HostPID: true,Also add:
func boolPtr(v bool) *bool { return &v }Also applies to: 122-135
61-69: PATH override is risky.Overriding PATH to "/opt/spire/bin:/bin" drops common dirs like /usr/bin. Consider appending instead of replacing to avoid surprising runtime behavior.
- {Name: SpireAgentEnvPath, Value: SpireAgentEnvPathValue}, + {Name: SpireAgentEnvPath, Value: SpireAgentEnvPathValue + ":/usr/sbin:/usr/bin:/sbin:/usr/local/bin"},pkg/controller/spire-agent/configmap.go (6)
137-149: Use constant for ConfigMap name.Avoid literal "spire-agent" here; it should match SpireAgentConfigMapNameAgent used by the DaemonSet.
- Name: "spire-agent", + Name: SpireAgentConfigMapNameAgent,
125-131: Nil guard for input.Protect against accidental nil deref if callers pass a nil SpireAgent.
-func GenerateSpireAgentConfigMap(spireAgentConfig *v1alpha1.SpireAgent) (*corev1.ConfigMap, string, error) { +func GenerateSpireAgentConfigMap(spireAgentConfig *v1alpha1.SpireAgent) (*corev1.ConfigMap, string, error) { + if spireAgentConfig == nil { + return nil, "", fmt.Errorf("nil SpireAgent") + }
129-136: Hash compact JSON; store pretty JSON.Compute the hash on compact JSON to reduce whitespace-only churn, while still storing pretty output.
- // Marshal to JSON - agentConfigJSON, err := json.MarshalIndent(agentConfig, "", " ") + // Marshal JSON (compact for hashing; pretty for storage) + compactJSON, err := json.Marshal(agentConfig) if err != nil { return nil, "", fmt.Errorf("failed to marshal agent config: %w", err) } - spireAgentConfigHash := utils.GenerateConfigHash(agentConfigJSON) + spireAgentConfigHash := utils.GenerateConfigHash(compactJSON) + agentConfigJSON, _ := json.MarshalIndent(agentConfig, "", " ")
137-145: Set OwnerReference for GC.Attach the SpireAgent as controller owner to the ConfigMap so it’s garbage‑collected with the CR.
ObjectMeta: metav1.ObjectMeta{ - Name: SpireAgentConfigMapNameAgent, + Name: SpireAgentConfigMapNameAgent, Namespace: utils.OperatorNamespace, Labels: utils.SpireAgentLabels(spireAgentConfig.Spec.Labels), Annotations: map[string]string{ utils.AppManagedByLabelKey: utils.AppManagedByLabelValue, }, + OwnerReferences: []metav1.OwnerReference{ + { + APIVersion: spireAgentConfig.APIVersion, + Kind: spireAgentConfig.Kind, + Name: spireAgentConfig.Name, + UID: spireAgentConfig.UID, + Controller: boolPtr(true), + }, + }, },Add helper (if not already present):
func boolPtr(v bool) *bool { return &v }
55-66: Validate clusterName when k8s_psat is enabled.If NodeAttestor.K8sPSATEnabled is "true" with empty ClusterName, the agent config will be invalid. Add a guard that returns an error (or at least logs) so the operator fails early.
- if spec.NodeAttestor != nil && spec.NodeAttestor.K8sPSATEnabled == "true" { + if spec.NodeAttestor != nil && spec.NodeAttestor.K8sPSATEnabled == "true" { + if spec.ClusterName == "" { + // decide: return an error to caller or annotate status + }
82-98: Avoid string literals for verification types.Use typed constants (e.g., VerificationTypeHostCert, VerificationTypeAPIServerCA, VerificationTypeSkip, VerificationTypeAuto) to prevent typos and aid discoverability.
pkg/controller/spire-agent/constants.go (3)
22-24: Unify node name env constant.DefaultNodeNameEnv and SpireAgentEnvNodeName duplicate "MY_NODE_NAME". Define once and reuse to prevent drift.
- // Workload attestor configuration - DefaultNodeNameEnv = "MY_NODE_NAME" + // Workload attestor configuration + DefaultNodeNameEnv = "MY_NODE_NAME" // source of truth ... - SpireAgentEnvNodeName = "MY_NODE_NAME" + SpireAgentEnvNodeName = DefaultNodeNameEnvAlso applies to: 42-46
85-88: Remove unused HostPath type string or use corev1 type.SpireAgentHostPathTypeDirectoryOrCreate is a string constant and appears unused; prefer corev1.HostPathDirectoryOrCreate where needed.
26-91: Add constants for verification types.To support the switch in configmap.go, add:
const ( + // Verification types for WorkloadAttestor (k8s) + VerificationTypeHostCert = "hostCert" + VerificationTypeAPIServerCA = "apiServerCA" + VerificationTypeSkip = "skip" + VerificationTypeAuto = "auto" )Then use these constants in buildSpireAgentConfig.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
Cache: Disabled due to data retention organization setting
Knowledge base: Disabled due to Reviews -> Disable Knowledge Base setting
📒 Files selected for processing (19)
pkg/config/spire_agent_config.go(1 hunks)pkg/config/spire_oidc_config.go(1 hunks)pkg/config/spire_server_config.go(1 hunks)pkg/controller/spiffe-csi-driver/constants.go(1 hunks)pkg/controller/spiffe-csi-driver/daemonset.go(4 hunks)pkg/controller/spiffe-csi-driver/daemonset_test.go(2 hunks)pkg/controller/spire-agent/configmap.go(1 hunks)pkg/controller/spire-agent/configmap_test.go(9 hunks)pkg/controller/spire-agent/constants.go(1 hunks)pkg/controller/spire-agent/daemonset.go(4 hunks)pkg/controller/spire-oidc-discovery-provider/configmap_test.go(2 hunks)pkg/controller/spire-oidc-discovery-provider/configmaps.go(1 hunks)pkg/controller/spire-oidc-discovery-provider/constants.go(1 hunks)pkg/controller/spire-oidc-discovery-provider/deployments.go(2 hunks)pkg/controller/spire-server/configmap.go(2 hunks)pkg/controller/spire-server/configmap_test.go(7 hunks)pkg/controller/spire-server/constants.go(1 hunks)pkg/controller/spire-server/controller.go(2 hunks)pkg/controller/spire-server/statefulset.go(2 hunks)
🔇 Additional comments (42)
pkg/config/spire_oidc_config.go (1)
1-134: LGTM! Well-structured configuration types.The configuration structs are well-designed with:
- Clear documentation for each field
- Proper JSON tags with appropriate use of
omitemptyfor optional fields- Helpful reference to upstream SPIRE documentation
- Logical grouping of related configuration options
The data model provides a clean foundation for structured configuration generation.
pkg/controller/spire-oidc-discovery-provider/constants.go (3)
4-27: LGTM! Well-organized configuration constants.The default configuration constants are clearly named and grouped logically. The constants cover agent socket, workload API, serving certificates, health checks, and domain configuration with sensible default values.
30-74: LGTM! Comprehensive deployment constants.The deployment constants provide a centralized, maintainable set of identifiers for:
- Deployment metadata and resource names
- Volume names and CSI configuration
- ConfigMap and Secret names
- Container configuration and mount paths
- Port definitions and probe settings
This significantly improves maintainability by eliminating scattered hard-coded values.
76-84: LGTM! Helper function correctly constructs default domains.The
getDefaultDomainsfunction properly builds a domain list including the standard OIDC service names at different namespace scopes (short name, namespaced, FQDN) plus the stripped JWT issuer. The private scope is appropriate for this internal helper.pkg/controller/spire-oidc-discovery-provider/deployments.go (5)
24-27: LGTM! Default replica count properly centralized.The replacement of the hard-coded
1withSpireOIDCDefaultReplicaCountmaintains the same default behavior while improving maintainability. The override logic viaconfig.Spec.ReplicaCountis preserved.
31-48: LGTM! Deployment metadata uses centralized constants.The deployment name and service account name now reference centralized constants, improving consistency across the codebase.
49-81: LGTM! Volume configuration properly centralized.All volume definitions now use constants for:
- Volume names (WorkloadAPI, OIDCSockets, OIDCConfig, TLSCerts)
- CSI driver name
- ConfigMap and Secret names
This eliminates duplication and reduces the risk of typos or inconsistencies.
82-97: LGTM! Container configuration uses centralized constants.Container name, command-line arguments, ports, and volume mounts all use constants, making the deployment configuration more maintainable and consistent.
98-117: LGTM! Probe configuration properly centralized.Readiness and liveness probes now use constants for paths, port names, and timing parameters. This ensures consistency with the health check configuration defined elsewhere.
pkg/controller/spire-oidc-discovery-provider/configmaps.go (2)
16-45: LGTM! Well-structured config builder function.The
buildSpireOIDCDiscoveryProviderConfigfunction properly constructs the OIDC configuration by:
- Handling default agent socket name when empty
- Building domains list using the helper function
- Populating all required configuration sections (workload API, serving certs, health checks)
- Using centralized constants for default values
The function creates a clean separation between CR spec and the actual OIDC provider configuration format.
47-80: LGTM! ConfigMap generation properly refactored.The
GenerateOIDCConfigMapFromCRfunction has been streamlined to:
- Validate and strip the JWT issuer protocol
- Delegate config building to the helper function
- Marshal the structured config to JSON
- Maintain proper error handling throughout
The refactoring improves code organization and testability while preserving all original functionality.
pkg/controller/spire-oidc-discovery-provider/configmap_test.go (4)
18-187: LGTM! Comprehensive ConfigMap generation tests.The test cases thoroughly validate:
- Default values when fields are not specified
- Custom values propagation from CR to ConfigMap
- Empty AgentSocketName falling back to default
- Complete OIDC config structure with all expected fields
- Health checks, serving certificates, and workload API configuration
The assertions are precise and cover both ConfigMap metadata and the JSON configuration content.
190-210: LGTM! JSON formatting validation.The test properly verifies that the generated JSON is:
- Valid and parseable
- Properly indented for human readability
This ensures the ConfigMap data is both machine-readable and maintainable.
212-300: LGTM! Direct config builder function tests.The table-driven tests for
buildSpireOIDCDiscoveryProviderConfigprovide excellent coverage:
- Minimal config with default agent socket
- Custom agent socket name
- Complex JWT issuer with port
Each test validates the complete configuration structure including domains, logging, workload API, serving certs, and health checks.
433-458: LGTM! JSON marshaling round-trip test.The test validates that the configuration can be marshaled to JSON and unmarshaled back without data loss, ensuring the struct tags are correctly defined and the configuration is serializable.
pkg/controller/spiffe-csi-driver/constants.go (5)
7-15: LGTM! Well-organized DaemonSet constants.The constants are clearly named and properly typed. The int32 type for
SpiffeCSIMaxUnavailablecorrectly matches the Kubernetes API requirements.
17-31: LGTM! Init container constants are well-defined.The SELinux command constants accurately reflect the
chconcommand structure. The trailing slash inSpiffeCSIArgTargetDiris appropriate for the directory-level SELinux context operation.
33-55: LGTM! Excellent dual-type constant pattern.Providing both
SpiffeCSIHealthPort(string) andSpiffeCSIHealthPortInt(int32) is a good practice that enables type-safe usage in different Kubernetes API contexts (e.g., command-line arguments vs. port specifications).
63-77: LGTM! Clear separation of volume names and mount paths.The organization clearly distinguishes between volume names (Kubernetes resource identifiers) and mount paths (filesystem paths), which improves maintainability.
89-103: LGTM! Probe and security constants are properly defined.The int32 types for probe timing values correctly match the Kubernetes API requirements, and the security configuration constants are appropriately scoped.
pkg/controller/spiffe-csi-driver/daemonset.go (5)
24-29: LGTM! Proper use of constants for DaemonSet metadata.The metadata correctly uses
SpiffeCSIDaemonSetNamewhile appropriately delegating to utility functions for namespace and labels.
52-75: LGTM! Init container fully migrated to constants.All init container configuration values have been successfully replaced with constants, including the command, security context, volume mounts, and termination message settings.
76-120: LGTM! CSI driver container fully migrated to constants.The driver container configuration comprehensively uses constants for container name, arguments, environment variables, security context, and volume mounts. The argument pairing (e.g., flag + value) is correct and maintains the expected command-line structure.
121-157: LGTM! Registrar container fully migrated to constants.The node driver registrar container configuration correctly uses constants throughout, including arguments, volume mounts, ports, and liveness probe settings. The port configuration properly uses both the int32 value for
ContainerPortand the string name for the probe reference.
159-196: LGTM! Volume definitions fully migrated to constants.All four volume definitions correctly use constants for volume names, host paths, and host path types. The consistent usage of
hostPathTypePtr()with the aliasedcorev1.HostPathTypeconstants maintains type safety.pkg/controller/spiffe-csi-driver/daemonset_test.go (4)
10-16: LGTM! Appropriate test dependencies added.The addition of
testify/assertandtestify/requireimproves test readability and provides better assertion failure messages compared to manual error handling.
487-657: LGTM! Comprehensive test coverage for constant usage.This table-driven test thoroughly validates that the DaemonSet generation correctly uses all defined constants. The test structure is well-organized with separate validation functions for each concern (metadata, init container, driver container, registrar container, and volumes).
The use of
require.Len()before indexing into slices prevents potential panics and provides clear test failure messages.
660-667: LGTM! Simple and effective helper function.The
findVolumehelper correctly returns a pointer to the matching volume ornilif not found, which enables clean assertions in the test validation functions.
669-708: LGTM! Valuable constant value validation.This test serves dual purposes:
- Documents the expected constant values in an executable format
- Acts as a safety net against unintended constant value changes
The organization into subtests by category makes it easy to understand which constants are related and what their expected values should be.
pkg/controller/spire-server/configmap_test.go (4)
27-83: Solid coverage for minimal builder path.Assertions on defaults (log level/format, telemetry, plugins) look correct and guard regressions well.
206-236: JSON payload validation looks good.Top-level keys and NodeAttestor structure checks are clear and stable.
709-776: Controller-manager YAML tests are precise.Error message matching and key substrings keep this robust without overfitting to ordering.
877-906: Helper config is clear; keep it as the single source for tests.This centralizes defaults well.
pkg/controller/spire-server/statefulset.go (1)
33-46: Constants cleanup LGTM.Consistent names/paths/ports improve maintainability and reduce drift across resources.
Also applies to: 63-92, 95-118, 120-126, 133-141
pkg/config/spire_server_config.go (2)
7-177: Config model looks comprehensive and consistent.JSON tags and structure read cleanly and match expected SPIRE schema.
251-267: Telemetry types are well-scoped.Optionality via pointers avoids noisy JSON; good defaults.
pkg/controller/spire-server/constants.go (3)
97-97: Mount path is correctly configured using SubPath for file-level mounting.The verification confirms the constant
SpireServerMountPathControllerManagerConfigis used in the StatefulSet volume mount configuration atpkg/controller/spire-server/statefulset.go:114withSubPath: SpireServerConfigPathControllerManager. This is the proper Kubernetes pattern for mounting a specific file from a volume to the target mount path. The implementation is correct and intentional.
111-116: Function is properly used—no issues found.Verification confirms that
getNodeAttestorServiceAccountAllowListis actively called inpkg/controller/spire-server/configmap.go:120, so there is no dead code concern. The function serves its intended purpose of providing the default service account allowlist for the node attestor.
52-53: I need to verify the mount path constants to understand the complete context:No changes needed—path formats are correct for their respective use cases.
The different path formats serve distinct purposes: the SubPath (a relative path) is appended to the base volume's host path, making
controller-manager-config.yamlthe correct format for the volume mount. The server config path, passed directly as a command argument, correctly uses an absolute path. Both usages align with standard Kubernetes patterns.pkg/controller/spire-agent/configmap_test.go (2)
589-811: Nice coverage on the struct builder.The table-driven tests exercise verification modes and plugin data mapping well. LGTM.
813-895: JSON structure checks are solid and deterministic.Good end-to-end validation of ConfigMap contents and hash determinism.
pkg/config/spire_agent_config.go (1)
29-31: Port fields as strings are correct and align with SPIRE specification.SPIRE's official documentation shows that port fields (server_port, bind_port) are represented as quoted strings in JSON configuration, not numbers. Your code correctly models these as string types. No changes needed.
| // For AWS IID | ||
| AccessKeyID string `json:"access_key_id,omitempty"` | ||
| SecretAccessKey string `json:"secret_access_key,omitempty"` | ||
| SecurityToken string `json:"security_token,omitempty"` | ||
| Region string `json:"region,omitempty"` | ||
|
|
||
| // For Azure MSI | ||
| TenantID string `json:"tenant_id,omitempty"` | ||
| ResourceID string `json:"resource_id,omitempty"` | ||
|
|
||
| // For GCP IIT | ||
| ProjectID string `json:"project_id,omitempty"` | ||
| ServiceAccountFile string `json:"service_account_file,omitempty"` | ||
| ZoneID string `json:"zone_id,omitempty"` | ||
|
|
||
| // For TPM | ||
| TPMPath string `json:"tpm_path,omitempty"` | ||
| DevicePath string `json:"device_path,omitempty"` | ||
| } |
There was a problem hiding this comment.
Avoid placing credentials in ConfigMaps.
Fields like AWS access keys, GCP service account file, and Vault token will serialize into the agent.conf ConfigMap if populated, leaking secrets to etcd. Prefer Secret references (e.g., Secret name/key) and mount/expand at runtime. If you must support inline values, gate them with validation to reject non-empty values and document secure alternatives.
Proposed direction:
- Replace sensitive fields with:
<Provider>SecretRefNameand<Provider>SecretRefKeystring fields.
- Read from Secret and inject via volumeMounts/env, not ConfigMap.
Also applies to: 172-199
🤖 Prompt for AI Agents
In pkg/config/spire_agent_config.go around lines 121 to 139 (and also apply the
same change for lines ~172-199), the struct contains raw credential fields
(AccessKeyID, SecretAccessKey, SecurityToken, ServiceAccountFile, Vault token,
etc.) which will serialize into ConfigMaps and leak secrets; replace each
sensitive inline field with two string fields per provider:
<Provider>SecretRefName and <Provider>SecretRefKey (e.g.,
AWSSecretRefName/AWSSecretRefKey,
GCPServiceAccountSecretName/GCPServiceAccountSecretKey,
VaultTokenSecretName/VaultTokenSecretKey), remove or deprecate the inline
credential fields, update validation to reject non-empty legacy inline fields
(fail fast with a clear error instructing using Secret refs), and update
documentation/comments to instruct operators to supply secrets via Kubernetes
Secrets and consume them via volumeMounts or envFrom rather than ConfigMaps;
ensure any code reading these config fields looks up the referenced Secret at
runtime and injects the secrets into the agent process instead of relying on
serialized config.
There was a problem hiding this comment.
not applicable as spire use this format
https://github.com/spiffe/spire/blob/main/doc/spire_agent.md
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Fetch SPIRE agent documentation to verify the configuration format
curl -sL https://raw.githubusercontent.com/spiffe/spire/main/doc/spire_agent.md | head -500
Length of output: 42303
@anirudhAgniRedhat Thanks for the clarification! You're absolutely right that the struct schema mirrors SPIRE's upstream configuration format.
To clarify the concern: the issue isn't with defining these fields in the Go struct (which correctly represents SPIRE's schema), but rather with how the operator populates them.
From the SPIRE documentation, I can see that SPIRE supports both:
plugin_data- inline configuration (which could include sensitive values)plugin_data_file- loading configuration from files (which could be mounted from Secrets)
Question: How does the operator handle sensitive values in practice? For example:
- Are fields like
AccessKeyID,SecretAccessKey, etc. populated from Kubernetes Secrets and mounted as files (which would be secure)? - Or are they expected to be provided inline in the CR spec and written directly to ConfigMaps (which would be the security concern)?
If sensitive fields are meant to be empty/unused in the operator's implementation, it might be helpful to add documentation comments noting that these should be provided via Secret mounts rather than inline configuration.
anirudhAgniRedhat
force-pushed
the
configStruct
branch
from
e18d0d8 to
0f89594
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)
pkg/controller/spire-agent/configmap_test.go (1)
443-448: Avoid panics: use safe type assertions for sections.Guard map type assertions with ok checks to fail tests clearly instead of panicking.
- agentSection := configData["agent"].(map[string]interface{}) + agentSection, ok := configData["agent"].(map[string]interface{}) + require.True(t, ok, "agent section must be an object") assert.Equal(t, tt.spireAgentConfig.Spec.TrustDomain, agentSection["trust_domain"]) assert.Equal(t, "/var/lib/spire", agentSection["data_dir"]) assert.Equal(t, "info", agentSection["log_level"]) assert.Equal(t, "text", agentSection["log_format"])
♻️ Duplicate comments (2)
pkg/controller/spire-server/configmap.go (1)
165-179: Add ClusterName validation to prevent empty NodeAttestor key.The validation checks multiple required fields but omits
spec.ClusterName, which is used as a map key at line 127 inbuildSpireServerConfig. An empty cluster name would result in the k8s_psat Clusters map being keyed by an empty string.Apply this diff:
func GenerateSpireServerConfigMap(spec *v1alpha1.SpireServerSpec) (*corev1.ConfigMap, error) { if spec == nil { return nil, fmt.Errorf("spec is nil") } if spec.TrustDomain == "" { return nil, fmt.Errorf("trust_domain is empty") } + if spec.ClusterName == "" { + return nil, fmt.Errorf("cluster name is empty") + } if spec.BundleConfigMap == "" { return nil, fmt.Errorf("bundle configmap is empty") }Based on learnings
pkg/controller/spire-agent/configmap.go (1)
129-141: Error handling improved; optional: surface error in return for deprecated API.You now log marshal/unmarshal errors—good. For easier debugging of legacy callers, consider returning a map with a sentinel error key instead of an empty map.
- return make(map[string]interface{}) + return map[string]interface{}{"error": "failed to marshal agent config"} @@ - return make(map[string]interface{}) + return map[string]interface{}{"error": "failed to unmarshal agent config"}
🧹 Nitpick comments (10)
pkg/controller/spiffe-csi-driver/constants.go (1)
45-49: Consider consolidating duplicate path values.Lines 45 and 49 define constants with the same value
"/spiffe-csi/csi.sock". While they serve different semantic purposes (driver socket path vs registrar address), they reference the same physical socket. You might consider using a single base constant to follow the DRY principle:SpiffeCSISocketPath = "/spiffe-csi/csi.sock" SpiffeCSICSIAddressPath = SpiffeCSISocketPathThat said, the current approach is clear and maintainable, so this is purely optional.
pkg/controller/spire-server/configmap.go (1)
38-44: Consider validating CASubject fields before wrapping in arrays.The code wraps
spec.CASubject.Countryandspec.CASubject.Organizationin arrays without checking if they're empty strings. While the CASubject nil check exists at line 177, empty string values would still create arrays containing empty strings in the configuration.Consider adding validation or conditional logic:
CASubject: []config.CASubject{ { - Country: []string{spec.CASubject.Country}, - Organization: []string{spec.CASubject.Organization}, + Country: conditionalStringArray(spec.CASubject.Country), + Organization: conditionalStringArray(spec.CASubject.Organization), CommonName: spec.CASubject.CommonName, }, },Where
conditionalStringArrayreturns nil or empty slice for empty strings, or returns a slice with the value otherwise.pkg/controller/spire-agent/configmap_test.go (3)
411-420: Avoid mutating maps returned by helpers (potential aliasing).If utils.SpireAgentLabels returns an internal map, mutating it in-place can cause subtle issues. Make a copy before merging custom labels.
- expectedLabels := utils.SpireAgentLabels(nil) - - // Add custom labels from the SpireAgentConfig - for key, value := range tt.spireAgentConfig.Spec.Labels { - expectedLabels[key] = value - } + base := utils.SpireAgentLabels(nil) + expectedLabels := make(map[string]string, len(base)+len(tt.spireAgentConfig.Spec.Labels)) + for k, v := range base { + expectedLabels[k] = v + } + for k, v := range tt.spireAgentConfig.Spec.Labels { + expectedLabels[k] = v + }If utils.SpireAgentLabels always returns a fresh map, this is a no-op safety improvement. Otherwise, it prevents cross-test interference.
336-339: Comment nit: not actually mocking OperatorNamespace.The comment says “Mock the utils.OperatorNamespace,” but the test only reads it. Either set it via a test hook or update the comment to avoid confusion.
589-812: Add negative-path coverage for builder validation.Exercise error cases to lock behavior:
- PSAT enabled with empty ClusterName should error.
- hostCert verification without HostCertBasePath should error.
@@ func TestBuildSpireAgentConfig(t *testing.T) { tests := []struct { name string spec *v1alpha1.SpireAgentSpec validate func(t *testing.T, cfg *config.SpireAgentConfig) }{ + { + name: "psat enabled requires clusterName", + spec: &v1alpha1.SpireAgentSpec{ + TrustDomain: "example.org", + NodeAttestor: &v1alpha1.NodeAttestor{ + K8sPSATEnabled: "true", + }, + ClusterName: "", + }, + validate: func(t *testing.T, _ *config.SpireAgentConfig) { + t.Fatalf("expected error, got success") + }, + }, + { + name: "hostCert verification requires base path", + spec: &v1alpha1.SpireAgentSpec{ + TrustDomain: "example.org", + WorkloadAttestors: &v1alpha1.WorkloadAttestors{ + K8sEnabled: "true", + WorkloadAttestorsVerification: &v1alpha1.WorkloadAttestorsVerification{ + Type: "hostCert", + HostCertBasePath: "", + }, + }, + }, + validate: func(t *testing.T, _ *config.SpireAgentConfig) { + t.Fatalf("expected error, got success") + }, + }, } @@ - cfg, err := buildSpireAgentConfig(tt.spec) - require.NoError(t, err) - require.NotNil(t, cfg) - tt.validate(t, cfg) + cfg, err := buildSpireAgentConfig(tt.spec) + if tt.validate == nil { + require.Error(t, err) + return + } + require.NoError(t, err) + require.NotNil(t, cfg) + tt.validate(t, cfg)Adjust as needed if you prefer separate tests for error paths.
pkg/controller/spire-agent/configmap.go (1)
15-43: Validate required TrustDomain early.Empty trust domain should fail fast to prevent generating unusable configs.
func buildSpireAgentConfig(spec *v1alpha1.SpireAgentSpec) (*config.SpireAgentConfig, error) { + if spec.TrustDomain == "" { + return nil, fmt.Errorf("trustDomain is required") + } agentConfig := &config.SpireAgentConfig{pkg/controller/spire-agent/daemonset.go (2)
39-41: Optional: use intstr.FromInt for clarity.Construct MaxUnavailable via helper for readability.
- MaxUnavailable: &intstr.IntOrString{IntVal: SpireAgentMaxUnavailable}, + MaxUnavailable: func() *intstr.IntOrString { + v := intstr.FromInt(int(SpireAgentMaxUnavailable)) + return &v + }(),
113-115: Remove unused volume: SpireAgentVolumeNameAdminSocketDir is defined but not mounted.The volume at line 113 is never referenced in the container's VolumeMounts (lines 94-98). Remove it to keep the manifest clean:
- {Name: SpireAgentVolumeNameAdminSocketDir, VolumeSource: corev1.VolumeSource{EmptyDir: &corev1.EmptyDirVolumeSource{}}}, {Name: SpireAgentVolumeNamePersistence, VolumeSource: corev1.VolumeSource{EmptyDir: &corev1.EmptyDirVolumeSource{}}},If this volume is intended for future use or a sidecar container, add a TODO comment with context instead.
pkg/controller/spire-agent/constants.go (2)
47-54: Prevent drift between health port string and int values.DefaultHealthCheckBindPort ("9982") and SpireAgentPortHealthz (9982) must stay aligned. Add a small unit test asserting they match, or derive one from the other at build time.
I can draft a constants_test.go asserting equality if helpful.
Also applies to: 12-21
85-88: Remove unused constantSpireAgentHostPathTypeDirectoryOrCreateor convert to typed constant.The constant at line 87 is never used in the codebase. The daemonset builder at
pkg/controller/spire-agent/daemonset.go:142usescorev1.HostPathDirectoryOrCreatedirectly instead. Either remove the constant or refactor it to match the typed pattern used in the similarSpiffeCSIHostPathTypeDirectoryOrCreateconstant inpkg/controller/spiffe-csi-driver/constants.go:88.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
Cache: Disabled due to data retention organization setting
Knowledge base: Disabled due to Reviews -> Disable Knowledge Base setting
📒 Files selected for processing (19)
pkg/config/spire_agent_config.go(1 hunks)pkg/config/spire_oidc_config.go(1 hunks)pkg/config/spire_server_config.go(1 hunks)pkg/controller/spiffe-csi-driver/constants.go(1 hunks)pkg/controller/spiffe-csi-driver/daemonset.go(4 hunks)pkg/controller/spiffe-csi-driver/daemonset_test.go(2 hunks)pkg/controller/spire-agent/configmap.go(1 hunks)pkg/controller/spire-agent/configmap_test.go(9 hunks)pkg/controller/spire-agent/constants.go(1 hunks)pkg/controller/spire-agent/daemonset.go(4 hunks)pkg/controller/spire-oidc-discovery-provider/configmap_test.go(2 hunks)pkg/controller/spire-oidc-discovery-provider/configmaps.go(1 hunks)pkg/controller/spire-oidc-discovery-provider/constants.go(1 hunks)pkg/controller/spire-oidc-discovery-provider/deployments.go(2 hunks)pkg/controller/spire-server/configmap.go(2 hunks)pkg/controller/spire-server/configmap_test.go(7 hunks)pkg/controller/spire-server/constants.go(1 hunks)pkg/controller/spire-server/controller.go(2 hunks)pkg/controller/spire-server/statefulset.go(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (6)
- pkg/config/spire_agent_config.go
- pkg/config/spire_oidc_config.go
- pkg/controller/spiffe-csi-driver/daemonset_test.go
- pkg/controller/spiffe-csi-driver/daemonset.go
- pkg/controller/spire-server/constants.go
- pkg/controller/spire-server/statefulset.go
🔇 Additional comments (20)
pkg/controller/spiffe-csi-driver/constants.go (1)
1-106: Excellent organization and structure!This constants file is well-organized with clear sections and consistent naming conventions. Centralizing these configuration values will significantly improve maintainability and reduce the risk of typos or inconsistencies across the codebase. The explanatory comments for the constructed paths (lines 51-53, 84-85) are particularly helpful.
pkg/controller/spire-oidc-discovery-provider/deployments.go (1)
24-117: Excellent refactoring—constants improve maintainability.The systematic replacement of hardcoded values with well-named constants throughout the deployment specification is a solid improvement. The changes maintain the same functionality while making the codebase more maintainable and reducing the risk of inconsistencies.
pkg/controller/spire-oidc-discovery-provider/constants.go (3)
4-27: Well-organized OIDC configuration defaults.The configuration constants are clearly documented and provide sensible defaults for the OIDC discovery provider. The naming convention with
Defaultprefixes makes it clear these are fallback values.
30-74: Comprehensive deployment constants with consistent naming.The deployment constants are well-structured and follow a consistent naming pattern with the
SpireOIDCprefix. The grouping by concern (metadata, volumes, CSI, ConfigMap/Secret, container config, mount paths, ports, probes) makes them easy to navigate.
76-84: Helper function appropriately scoped.The
getDefaultDomainsfunction is correctly scoped as package-private and provides a clean way to construct the default domain list. The function signature is clear and the implementation is straightforward.pkg/controller/spire-oidc-discovery-provider/configmaps.go (2)
16-45: Well-structured configuration builder.The
buildSpireOIDCDiscoveryProviderConfigfunction provides a clean, typed approach to constructing the OIDC discovery provider configuration. The function properly handles default values (e.g.,AgentSocketName) and uses the constants defined inconstants.goeffectively.
48-80: Cleaner ConfigMap generation with typed config.The refactored
GenerateOIDCConfigMapFromCRfunction is more maintainable by delegating config construction to the dedicated builder function. The separation of concerns between config building and JSON marshaling improves code clarity.pkg/controller/spire-oidc-discovery-provider/configmap_test.go (3)
212-300: Comprehensive builder function tests.The
TestBuildSpireOIDCDiscoveryProviderConfigtest provides excellent coverage of the configuration builder with table-driven tests covering default agent socket, custom socket names, and complex JWT issuers. The validation functions thoroughly check all aspects of the generated configuration.
302-431: Thorough ConfigMap generation validation.The
TestGenerateOIDCConfigMapFromCR_WithStructstest comprehensively validates ConfigMap generation, including JSON structure validation, label propagation, and error cases (nil CR, invalid JWT issuer). The separation ofvalidateCMandvalidateJSONcallbacks provides flexible validation patterns.
433-458: JSON marshaling round-trip validation.The
TestOIDCConfig_JSONMarshalingtest ensures that the configuration can be marshaled to JSON and unmarshaled back without data loss. This is important for verifying the JSON tags on the config structs are correct.pkg/config/spire_server_config.go (1)
1-294: LGTM! Well-structured configuration model.The comprehensive configuration types for SPIRE Server are well-designed, properly documented with references to official SPIRE documentation, and include appropriate JSON tags for marshaling. The use of
metav1.Durationfor TTL fields integrates well with Kubernetes API machinery.pkg/controller/spire-server/controller.go (1)
224-234: LGTM! Improved error handling for config generation.The refactored approach properly handles errors from
generateServerConfMapbefore marshaling, providing better error visibility and aligning with the updated function signature.pkg/controller/spire-server/configmap_test.go (3)
20-170: LGTM! Comprehensive test coverage for the new config builder.The tests thoroughly validate the
buildSpireServerConfigfunction across minimal configurations, memory key managers, and PostgreSQL with TLS. The use ofrequireandassertfrom testify provides stricter validation than the standard testing package.
172-315: LGTM! Thorough validation of ConfigMap generation.The test suite properly validates both success and error cases, including JSON structure validation and mandatory field checks. The JSON unmarshaling verification ensures the generated configuration is valid.
317-363: LGTM! Good backward compatibility verification.Testing that both the legacy
generateServerConfMapand newbuildSpireServerConfigproduce equivalent output ensures no breaking changes during the refactoring.pkg/controller/spire-server/configmap.go (1)
206-222: Good improvement! Error handling properly implemented.The function now correctly returns errors from
json.Marshalandjson.Unmarshaloperations instead of silently ignoring them, addressing the previous review feedback. The deprecation notice also guides developers toward the preferredbuildSpireServerConfigapproach.pkg/controller/spire-agent/configmap_test.go (1)
822-896: Nice end-to-end JSON shape assertions.Good coverage ensuring top-level keys and plugin sections exist; validates struct-to-JSON path coherently.
pkg/controller/spire-agent/configmap.go (1)
153-161: Deterministic JSON + hashing path looks solid.Struct-based MarshalIndent followed by hashing ensures stable ConfigMap data and consistent rollouts.
pkg/controller/spire-agent/daemonset.go (1)
57-61: LGTM on constants adoption.Using named constants for args, ports, probes improves consistency and testability.
Also applies to: 71-92
pkg/controller/spire-agent/constants.go (1)
28-91: Good centralization of identifiers.Consolidating names, paths, and timings here reduces literals in controllers and tests.
anirudhAgniRedhat
force-pushed
the
configStruct
branch
from
0f89594 to
98ee0ee
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)
pkg/controller/spire-agent/daemonset.go (1)
14-151: LGTM! Excellent refactor to use constants.The replacement of hard-coded literals with well-named constants throughout the DaemonSet generation significantly improves maintainability and readability. All substitutions are semantically equivalent and the logic remains unchanged.
♻️ Duplicate comments (3)
pkg/controller/spire-agent/configmap_test.go (1)
449-457: Good: safe assertion for health_checks.The guarded type assertion prevents panics and matches prior guidance.
pkg/config/spire_agent_config.go (1)
121-126: Blocker: Secrets will leak via ConfigMaps — replace inline credentials with Secret refs.The following fields will serialize into agent.conf and land in a ConfigMap/etcd if set: AWS credentials, GCP service account file, Vault token, and join_token. This is a critical secret‑handling flaw.
Recommended remediation:
- Remove or ignore inline secret fields from JSON (use
json:"-") and introduce SecretRef fields (name/key) per provider.- Validate: fail fast if any deprecated inline secret field is non‑empty; instruct operators to use Kubernetes Secrets.
- Adjust builders to read SecretRefs and inject via env/volume, not by serializing into ConfigMaps.
Example (illustrative) changes:
type AgentNodeAttestorPluginData struct { @@ - AccessKeyID string `json:"access_key_id,omitempty"` - SecretAccessKey string `json:"secret_access_key,omitempty"` - SecurityToken string `json:"security_token,omitempty"` + // Deprecated: do not serialize secrets; use AWSSecretRef* + AccessKeyID string `json:"-"` // deprecated + SecretAccessKey string `json:"-"` // deprecated + SecurityToken string `json:"-"` // deprecated + AWSSecretRefName string `json:"aws_secret_ref_name,omitempty"` + AWSSecretRefKey string `json:"aws_secret_ref_key,omitempty"` @@ - ServiceAccountFile string `json:"service_account_file,omitempty"` + // Deprecated: use GCPServiceAccountSecret* instead + ServiceAccountFile string `json:"-"` // deprecated + GCPServiceAccountSecretName string `json:"gcp_sa_secret_name,omitempty"` + GCPServiceAccountSecretKey string `json:"gcp_sa_secret_key,omitempty"` } type SVIDStorePluginData struct { @@ - AccessKeyID string `json:"access_key_id,omitempty"` - SecretAccessKey string `json:"secret_access_key,omitempty"` + // Deprecated: use AWSSecretRef* + AccessKeyID string `json:"-"` // deprecated + SecretAccessKey string `json:"-"` // deprecated + AWSSecretRefName string `json:"aws_secret_ref_name,omitempty"` + AWSSecretRefKey string `json:"aws_secret_ref_key,omitempty"` @@ - ServiceAccountFile string `json:"service_account_file,omitempty"` + // Deprecated: use GCPServiceAccountSecret* + ServiceAccountFile string `json:"-"` // deprecated + GCPServiceAccountSecretName string `json:"gcp_sa_secret_name,omitempty"` + GCPServiceAccountSecretKey string `json:"gcp_sa_secret_key,omitempty"` @@ - Token string `json:"token,omitempty"` + // Deprecated: use VaultTokenSecret* + Token string `json:"-"` // deprecated + VaultTokenSecretName string `json:"vault_token_secret_name,omitempty"` + VaultTokenSecretKey string `json:"vault_token_secret_key,omitempty"`Also consider treating
AgentConfig.JoinTokenas secret (SecretRef + deprecate inline).Also applies to: 131-135, 172-191, 50-52
pkg/controller/spire-server/configmap.go (1)
165-179: Validate ClusterName to prevent empty NodeAttestor cluster key.The validation for
spec.ClusterNameis still missing, despite being flagged in a previous review. Without this check, an empty cluster name will result in the k8s_psat Clusters map being keyed by an empty string (see line 127), which breaks the configuration.Apply this diff to add the validation:
func GenerateSpireServerConfigMap(spec *v1alpha1.SpireServerSpec) (*corev1.ConfigMap, error) { if spec == nil { return nil, fmt.Errorf("spec is nil") } if spec.TrustDomain == "" { return nil, fmt.Errorf("trust_domain is empty") } + if spec.ClusterName == "" { + return nil, fmt.Errorf("cluster name is empty") + } if spec.BundleConfigMap == "" { return nil, fmt.Errorf("bundle configmap is empty") }
🧹 Nitpick comments (7)
pkg/controller/spire-oidc-discovery-provider/constants.go (1)
19-19: Consider consolidating port value representations.
DefaultHealthCheckBindPortis defined as string"8008"(line 19), whileSpireOIDCPortHealthzis defined asint32(8008)(line 64). Both represent the same port but in different types. If they serve different purposes (string for bind addresses, int32 for container ports), consider adding a comment explaining the distinction. Otherwise, derive one from the other to maintain a single source of truth.Example:
SpireOIDCPortHealthz int32 = 8008 DefaultHealthCheckBindPort = fmt.Sprintf("%d", SpireOIDCPortHealthz)Also applies to: 64-64
pkg/config/spire_oidc_config.go (1)
33-38: Guard unsafe HTTP exposure.
allow_insecure_scheme/insecure_addrshould be restricted to dev/test. Ensure the builder defaults these off, gates with an explicit “development” flag, and emits a warning when enabled.pkg/controller/spire-agent/configmap_test.go (2)
443-448: Harden remaining type assertions for agent/plugins.Mirror the safe pattern used for health_checks to avoid panics if JSON shape changes.
- agentSection := configData["agent"].(map[string]interface{}) + agentVal, ok := configData["agent"] + require.True(t, ok, "agent must exist") + agentSection, ok := agentVal.(map[string]interface{}) + require.True(t, ok, "agent must be an object") - pluginsSection := configData["plugins"].(map[string]interface{}) + pluginsVal, ok := configData["plugins"] + require.True(t, ok, "plugins must exist") + pluginsSection, ok := pluginsVal.(map[string]interface{}) + require.True(t, ok, "plugins must be an object")Also applies to: 460-462
592-815: Add negative test: PSAT enabled without ClusterName should error.Builder returns an error when k8s_psat is enabled but ClusterName is empty. Add a case to assert this.
@@ func TestBuildSpireAgentConfig(t *testing.T) { tests := []struct { name string spec *v1alpha1.SpireAgentSpec validate func(t *testing.T, cfg *config.SpireAgentConfig) }{ + { + name: "psat enabled without cluster name -> error", + spec: &v1alpha1.SpireAgentSpec{ + TrustDomain: "example.org", + NodeAttestor: &v1alpha1.NodeAttestor{ + K8sPSATEnabled: "true", + }, + ClusterName: "", + }, + validate: func(t *testing.T, cfg *config.SpireAgentConfig) { + t.Fatalf("expected error, got config: %+v", cfg) + }, + }, } @@ - cfg, err := buildSpireAgentConfig(tt.spec) - require.NoError(t, err) - require.NotNil(t, cfg) - tt.validate(t, cfg) + cfg, err := buildSpireAgentConfig(tt.spec) + if strings.Contains(tt.name, "error") { + require.Error(t, err) + return + } + require.NoError(t, err) + require.NotNil(t, cfg) + tt.validate(t, cfg)pkg/controller/spire-agent/configmap.go (2)
121-128: Nil guard for deprecated path.
GenerateAgentConfigdereferencescfgwithout checking; add a quick guard.func GenerateAgentConfig(cfg *v1alpha1.SpireAgent) map[string]interface{} { + if cfg == nil { + log.Printf("nil SpireAgent input") + return make(map[string]interface{}) + }
15-17: Consistency: align builder signatures across components.Agent builder returns
(*cfg, error); server builder (from tests) returnscfgonly. Recommend returning(cfg, error)for server too for symmetric validation and clearer failure paths.pkg/controller/spire-server/configmap_test.go (1)
206-236: Optional: harden JSON assertions.Adopt the guarded pattern (ok checks) for
server,plugins, etc., similar to agent tests to avoid panics on shape changes.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
Cache: Disabled due to data retention organization setting
Knowledge base: Disabled due to Reviews -> Disable Knowledge Base setting
📒 Files selected for processing (19)
pkg/config/spire_agent_config.go(1 hunks)pkg/config/spire_oidc_config.go(1 hunks)pkg/config/spire_server_config.go(1 hunks)pkg/controller/spiffe-csi-driver/constants.go(1 hunks)pkg/controller/spiffe-csi-driver/daemonset.go(4 hunks)pkg/controller/spiffe-csi-driver/daemonset_test.go(2 hunks)pkg/controller/spire-agent/configmap.go(1 hunks)pkg/controller/spire-agent/configmap_test.go(9 hunks)pkg/controller/spire-agent/constants.go(1 hunks)pkg/controller/spire-agent/daemonset.go(4 hunks)pkg/controller/spire-oidc-discovery-provider/configmap_test.go(2 hunks)pkg/controller/spire-oidc-discovery-provider/configmaps.go(1 hunks)pkg/controller/spire-oidc-discovery-provider/constants.go(1 hunks)pkg/controller/spire-oidc-discovery-provider/deployments.go(2 hunks)pkg/controller/spire-server/configmap.go(2 hunks)pkg/controller/spire-server/configmap_test.go(7 hunks)pkg/controller/spire-server/constants.go(1 hunks)pkg/controller/spire-server/controller.go(2 hunks)pkg/controller/spire-server/statefulset.go(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (7)
- pkg/controller/spiffe-csi-driver/daemonset.go
- pkg/controller/spire-server/controller.go
- pkg/controller/spire-oidc-discovery-provider/configmaps.go
- pkg/controller/spire-oidc-discovery-provider/configmap_test.go
- pkg/controller/spiffe-csi-driver/daemonset_test.go
- pkg/controller/spire-server/constants.go
- pkg/controller/spire-server/statefulset.go
🔇 Additional comments (19)
pkg/controller/spiffe-csi-driver/constants.go (10)
1-5: LGTM: Clean package structure.The package declaration and imports are appropriate. The corev1 import provides access to Kubernetes types used for typed constants throughout the file.
7-15: LGTM: Well-organized DaemonSet configuration.The constants are clearly named and appropriately typed for DaemonSet configuration.
17-31: LGTM: Type-safe init container configuration.Good use of the typed
corev1.TerminationMessageReadFileconstant for better type safety and IDE support.
33-57: Well-structured constants with good documentation.The main container constants are clearly organized. The dual format for the health port (string at line 55, int32 at line 56) is necessary for different Kubernetes API requirements.
The string concatenation at line 53 is well-documented, and the comment clearly explains the dependency on
SpiffeCSIDefaultPluginName.Verify that any future changes to
SpiffeCSIDefaultPluginNamealso update the dependent path constants (lines 53, 85) to maintain consistency. The current approach is acceptable, but this coupling should be kept in mind during maintenance.
59-63: LGTM: Standard environment variable configuration.These constants follow the standard Kubernetes downward API pattern for accessing node information.
65-71: LGTM: Clear volume naming.Volume name constants are well-organized and follow consistent naming conventions.
73-79: LGTM: Standard CSI mount paths.The mount path constants follow standard Kubernetes CSI driver conventions.
81-90: LGTM: Type-safe host path configuration.Good use of typed
corev1.HostPathTypeconstants. The string concatenation at line 85 follows the same well-documented pattern as line 53.
92-100: LGTM: Reasonable probe configuration.The probe constants are well-organized with sensible timing defaults for liveness checks.
102-106: LGTM: Security best practice.Dropping all capabilities is a standard Kubernetes security hardening practice, and this constant makes it explicit in the configuration.
pkg/controller/spire-oidc-discovery-provider/deployments.go (1)
24-127: LGTM! Excellent refactoring to use constants.The replacement of hardcoded values with well-named constants significantly improves maintainability and provides a single source of truth for configuration values. All constant references are consistent and appropriate.
pkg/controller/spire-oidc-discovery-provider/constants.go (3)
26-26: Verify hardcoded cluster domain assumption.
DefaultOIDCServiceFullyQualifiedDNhardcodes the cluster domain assvc.cluster.local, which is the Kubernetes default but can be configured differently in some clusters. Verify that this hardcoded value is appropriate for all target environments, or consider making the cluster domain configurable if this needs to support non-default configurations.
30-74: Well-organized deployment constants.The deployment constants are logically grouped and follow a consistent naming convention. The reuse of health check path constants (lines 68-69) is a good practice that avoids duplication.
76-84: No action required—getDefaultDomainsis actively used in the codebase.The function is invoked in
pkg/controller/spire-oidc-discovery-provider/configmaps.go:24, so it is not dead code and does not need to be removed.Likely an incorrect or invalid review comment.
pkg/controller/spire-agent/configmap.go (1)
129-141: Good: JSON error handling added.Marshaling/unmarshaling errors are now handled; avoids silent failures.
pkg/controller/spire-server/configmap.go (3)
22-154: LGTM! Well-structured builder function.The
buildSpireServerConfigfunction follows a clear builder pattern, constructing the configuration struct systematically. The logic for building DataStore, KeyManager, NodeAttestor, and Notifier plugins is consistent and appropriate.
164-202: LGTM! Clean refactor with proper error handling.The function now uses the builder pattern and handles errors appropriately. The separation of concerns between validation, building, and marshaling is well done.
204-222: LGTM! Error handling improved.The function now properly returns errors instead of silently ignoring them, addressing the previous review feedback. The deprecation notice and backward compatibility approach are appropriate.
pkg/config/spire_server_config.go (1)
1-294: LGTM! Comprehensive and well-documented configuration schema.The configuration structs are well-defined with:
- Proper JSON tags for serialization
- Appropriate use of
omitemptyfor optional fields- Clear field documentation
- Correct type choices (metav1.Duration for durations, pointers for optional nested structs)
- Reference to official SPIRE documentation
The schema comprehensively covers server, plugins, health checks, and telemetry configuration.
| // buildSpireAgentConfig creates a SpireAgentConfig from the operator API spec | ||
| func buildSpireAgentConfig(spec *v1alpha1.SpireAgentSpec) (*config.SpireAgentConfig, error) { | ||
| agentConfig := &config.SpireAgentConfig{ |
There was a problem hiding this comment.
Validate required inputs early (trust_domain).
Reject empty spec.TrustDomain to avoid generating invalid agent configs.
func buildSpireAgentConfig(spec *v1alpha1.SpireAgentSpec) (*config.SpireAgentConfig, error) {
+ if spec == nil {
+ return nil, fmt.Errorf("nil spec")
+ }
+ if strings.TrimSpace(spec.TrustDomain) == "" {
+ return nil, fmt.Errorf("trust_domain is required")
+ }📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| // buildSpireAgentConfig creates a SpireAgentConfig from the operator API spec | |
| func buildSpireAgentConfig(spec *v1alpha1.SpireAgentSpec) (*config.SpireAgentConfig, error) { | |
| agentConfig := &config.SpireAgentConfig{ | |
| // buildSpireAgentConfig creates a SpireAgentConfig from the operator API spec | |
| func buildSpireAgentConfig(spec *v1alpha1.SpireAgentSpec) (*config.SpireAgentConfig, error) { | |
| if spec == nil { | |
| return nil, fmt.Errorf("nil spec") | |
| } | |
| if strings.TrimSpace(spec.TrustDomain) == "" { | |
| return nil, fmt.Errorf("trust_domain is required") | |
| } | |
| agentConfig := &config.SpireAgentConfig{ |
🤖 Prompt for AI Agents
In pkg/controller/spire-agent/configmap.go around lines 15 to 17, the builder
does not validate required inputs: check spec.TrustDomain at the start of
buildSpireAgentConfig and reject empty values by returning a clear error (e.g.,
fmt.Errorf("spec.TrustDomain is required")). Add the validation before
constructing agentConfig so the function fails fast on invalid input and avoid
generating invalid agent configs.
| package spire_agent | ||
|
|
||
| // Agent configuration constants | ||
| const ( | ||
| // Agent main configuration | ||
| DefaultAgentDataDir = "/var/lib/spire" | ||
| DefaultAgentServerAddress = "spire-server.zero-trust-workload-identity-manager" | ||
| DefaultAgentServerPort = "443" | ||
| DefaultAgentSocketPath = "/tmp/spire-agent/public/spire-agent.sock" | ||
| DefaultAgentTrustBundlePath = "/run/spire/bundle/bundle.crt" | ||
|
|
||
| // Health check configuration | ||
| DefaultHealthCheckBindAddress = "0.0.0.0" | ||
| DefaultHealthCheckBindPort = "9982" | ||
| DefaultHealthCheckLivePath = "/live" | ||
| DefaultHealthCheckReadyPath = "/ready" | ||
|
|
||
| // Telemetry configuration | ||
| DefaultPrometheusHost = "0.0.0.0" | ||
| DefaultPrometheusPort = "9402" | ||
|
|
||
| // Workload attestor configuration | ||
| DefaultNodeNameEnv = "MY_NODE_NAME" | ||
| ) | ||
|
|
||
| // DaemonSet constants | ||
| const ( | ||
| // DaemonSet metadata | ||
| SpireAgentDaemonSetName = "spire-agent" | ||
| SpireAgentServiceAccountName = "spire-agent" | ||
|
|
||
| // Annotation keys | ||
| SpireAgentAnnotationDefaultContainer = "kubectl.kubernetes.io/default-container" | ||
|
|
||
| // Container configuration | ||
| SpireAgentContainerName = "spire-agent" | ||
|
|
||
| // Container arguments | ||
| SpireAgentArgConfig = "-config" | ||
| SpireAgentConfigPath = "/opt/spire/conf/agent/agent.conf" | ||
|
|
||
| // Environment variables | ||
| SpireAgentEnvPath = "PATH" | ||
| SpireAgentEnvPathValue = "/opt/spire/bin:/bin" | ||
| SpireAgentEnvNodeName = DefaultNodeNameEnv // "MY_NODE_NAME" | ||
|
|
||
| // Port configuration | ||
| SpireAgentPortNameHealthz = "healthz" | ||
| SpireAgentPortHealthz int32 = 9982 // Derived from DefaultHealthCheckBindPort | ||
|
|
||
| // Probe paths (reference canonical definitions from default constants) | ||
| SpireAgentProbePathLive = DefaultHealthCheckLivePath // "/live" | ||
| SpireAgentProbePathReady = DefaultHealthCheckReadyPath // "/ready" | ||
|
|
||
| // Probe timing | ||
| SpireAgentLivenessInitialDelay int32 = 15 | ||
| SpireAgentLivenessPeriod int32 = 60 | ||
| SpireAgentReadinessInitialDelay int32 = 10 | ||
| SpireAgentReadinessPeriod int32 = 30 | ||
|
|
||
| // Volume names | ||
| SpireAgentVolumeNameConfig = "spire-config" | ||
| SpireAgentVolumeNamePersistence = "spire-agent-persistence" | ||
| SpireAgentVolumeNameBundle = "spire-bundle" | ||
| SpireAgentVolumeNameSocketDir = "spire-agent-socket-dir" | ||
| SpireAgentVolumeNameToken = "spire-token" | ||
| SpireAgentVolumeNameAdminSocketDir = "spire-agent-admin-socket-dir" | ||
|
|
||
| // Mount paths | ||
| SpireAgentMountPathConfig = "/opt/spire/conf/agent" | ||
| SpireAgentMountPathPersistence = DefaultAgentDataDir // "/var/lib/spire" | ||
| SpireAgentMountPathBundle = "/run/spire/bundle" | ||
| SpireAgentMountPathSocketDir = "/tmp/spire-agent/public" | ||
| SpireAgentMountPathToken = "/var/run/secrets/tokens" | ||
|
|
||
| // ConfigMap names | ||
| SpireAgentConfigMapNameAgent = "spire-agent" | ||
| SpireAgentConfigMapNameBundle = "spire-bundle" | ||
|
|
||
| // Service account token configuration | ||
| SpireAgentTokenPath = "spire-agent" | ||
| SpireAgentTokenExpirationSeconds int64 = 7200 | ||
| SpireAgentTokenAudience = "spire-server" | ||
|
|
||
| // Host path configuration | ||
| SpireAgentHostPathAgentSockets = "/run/spire/agent-sockets" | ||
| SpireAgentHostPathTypeDirectoryOrCreate = "DirectoryOrCreate" | ||
|
|
||
| // Update strategy | ||
| SpireAgentMaxUnavailable int32 = 1 | ||
| ) |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
LGTM! Well-organized constant definitions.
The constants are logically grouped and consistently named, providing a centralized source of truth for SPIRE Agent configuration values. Key strengths:
- Clear grouping by functional area (agent config, DaemonSet, volumes, etc.)
- Explicit types where needed (int32, int64)
- Appropriate cross-referencing between related constants
- Descriptive names that clarify purpose
🤖 Prompt for AI Agents
In pkg/controller/spire-agent/constants.go around lines 1 to 91, the reviewer
approved the constant definitions and found them well-organized with no required
changes; no code fixes are needed—proceed to merge the changes as-is.
| ConnectionString: "postgresql://user:password@postgres:5432/spire", | ||
| MaxOpenConns: 50, | ||
| MaxIdleConns: 10, | ||
| ConnMaxLifetime: 3600, | ||
| DisableMigration: "false", | ||
| RootCAPath: "/etc/ssl/certs/ca.pem", | ||
| ClientCertPath: "/etc/ssl/certs/client.crt", | ||
| ClientKeyPath: "/etc/ssl/private/client.key", | ||
| Options: []string{"sslmode=require"}, | ||
| }, |
There was a problem hiding this comment.
Blocker: DB credentials serialized into ConfigMap.
Tests show connection_string (with password) landing in server.conf ConfigMap. This leaks secrets to etcd.
Direction:
- Replace inline DS secrets with SecretRef fields (name/key) in server config types.
- Update builder: read from Secret, not from spec literal; inject via env/volume or sidecar; never serialize credentials into ConfigMap.
- Adjust tests to reference SecretRefs and assert omission of secret material in JSON.
Short‑term: redact or drop password from connection_string in tests to avoid normalizing leakage.
Also applies to: 172-237, 299-307
🤖 Prompt for AI Agents
In pkg/controller/spire-server/configmap_test.go around lines 131-140 (and also
apply same fixes to 172-237 and 299-307), the test currently embeds the full DB
connection string (including password) into the ConfigMap which leaks secrets;
change the server config type to replace inline credential fields with a
SecretRef (name/key) for the DSN/credentials, update the builder to obtain
credentials from the referenced Secret at runtime (via Secret volume/env or
sidecar) and ensure it does not serialize secret material into
server.conf/ConfigMap, and update the tests to create a Secret and reference it
via SecretRef and assert the produced JSON/ConfigMap omits any secret content
(short-term alternative: redact or remove the password from connection_string in
tests to avoid normalizing secret leakage).
There was a problem hiding this comment.
upstram spire takes input in this format
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
SPIRE server datastore configuration connection string credentials
💡 Result:
Short answer — SPIRE Server’s datastore is configured in server.conf (plugins → DataStore "sql" → plugin_data). Examples:
-
SQLite (default)
connection_string = "/opt/spire/data/server/datastore.sqlite3". [1][2] -
MySQL
connection_string = "username:password@tcp(host:3306)/dbname?parseTime=true"
(replace username/password/host/port/dbname). SPIRE requires a dedicated DB and a user that can CREATE/INSERT/DROP as needed. [1] -
Postgres
connection_string = "dbname=DBNAME user=USERNAME password=PASSWORD host=HOST port=PORT"
(or a standard Postgres URI). SPIRE requires a dedicated DB and a user with INSERT/DROP privileges. [1]
Notes and best practices
- server.conf supports env expansion when started with -expandEnv, e.g. connection_string = "${DB_CONN}". Use that to avoid embedding secrets in the file. [2]
- SPIRE’s SQL plugin accepts mysql/postgres/sqlite3 (and AWS variants). [4]
- Don’t store plaintext credentials in world-readable files; use environment variables or a secrets manager and pass via -expandEnv or your system’s secret injection. [1][2]
References
[1] SPIRE docs — Configuring how to store server data (including MySQL/Postgres examples).
[2] SPIRE Server configuration reference and sample server.conf.
[3] sqlstore package (SPIRE) — supported database backends.
@anirudhAgniRedhat You're correct that SPIRE expects the connection_string format in its config file. However, SPIRE supports environment variable expansion via the -expandEnv flag, which provides a secure solution.
Recommended approach:
-
Use environment variable placeholders in ConfigMap:
connection_string = "${SPIRE_DB_CONNECTION_STRING}" -
Add
-expandEnvflag to SPIRE server container args:Args: []string{"-expandEnv", "-config", "/opt/spire/conf/server/server.conf"}
-
Inject actual credentials from Secret as environment variable:
- Add a
connectionStringSecretReffield to yourDataStoreCRD - In the StatefulSet, mount the Secret as an env var
SPIRE_DB_CONNECTION_STRING
- Add a
-
Update tests to verify:
- ConfigMap contains only
"${SPIRE_DB_CONNECTION_STRING}"(no plaintext credentials) - StatefulSet correctly references the Secret
- ConfigMap contains only
This approach satisfies SPIRE's configuration requirements while keeping credentials in Secrets (not ConfigMaps), preventing secret leakage to etcd.
Would you like me to help draft the CRD and implementation changes?
anirudhAgniRedhat
force-pushed
the
configStruct
branch
from
98ee0ee to
c60761e
Compare
There was a problem hiding this comment.
Actionable comments posted: 7
♻️ Duplicate comments (4)
pkg/controller/spire-agent/constants.go (1)
1-91: Constants set looks clean and consistent.No changes requested.
pkg/config/spire_agent_config.go (1)
121-139: Secrets in plugin data will leak via ConfigMap (duplicate of prior feedback).AWS keys, GCP service account file, and Vault token fields are sensitive. Replace with Secret refs and reject inline population.
Also applies to: 172-199
pkg/controller/spire-server/configmap_test.go (1)
129-140: DB credentials in ConfigMap tests normalize a security anti-pattern.While this is a test file, the connection string includes passwords in plaintext (e.g., line 131:
"postgresql://user:password@postgres:5432/spire"). This test validates the current implementation that serializes credentials into the ConfigMap, which leaks secrets to etcd.As noted in past review comments, the proper approach is to:
- Use SecretRef fields in the config types (not inline credentials)
- Update the builder to read from Secrets at runtime
- Never serialize credentials into ConfigMaps
- Update tests to reference SecretRefs and assert that secret material is omitted from JSON
Also applies to lines 150, 197, 341, 906 where connection strings with passwords appear.
pkg/controller/spire-agent/configmap.go (1)
15-17: Missing validation for required inputs.The builder should validate that
specis non-nil and thatspec.TrustDomainis non-empty before proceeding. Without this, the function will generate invalid configs or panic on nil pointer dereference.Apply this diff:
func buildSpireAgentConfig(spec *v1alpha1.SpireAgentSpec) (*config.SpireAgentConfig, error) { + if spec == nil { + return nil, fmt.Errorf("spec is nil") + } + if strings.TrimSpace(spec.TrustDomain) == "" { + return nil, fmt.Errorf("spec.TrustDomain is required") + } + agentConfig := &config.SpireAgentConfig{Add the
stringsimport if not already present.
🧹 Nitpick comments (7)
pkg/controller/spire-oidc-discovery-provider/configmaps.go (1)
16-45: Consider makingbuildSpireOIDCDiscoveryProviderConfigprivate.The function is currently exported but only called within the same package (by
GenerateOIDCConfigMapFromCRand tests). Since tests in the same package can access private functions, consider renaming it tobuildSpireOIDCDiscoveryProviderConfig→buildSpireOIDCDiscoveryProviderConfigunless you plan to use it from other packages.If keeping it exported is intentional for future cross-package usage or extensibility, you can safely ignore this suggestion.
pkg/controller/spiffe-csi-driver/daemonset.go (1)
96-103: Privileged + drop ALL is redundant.When Privileged is true, capability drops are ignored. Consider removing Capabilities.Drop or dropping Privileged (if not strictly required for mounts) to simplify.
Please confirm the driver truly requires privileged on your target platforms; if not, we can harden this by using specific caps instead.
pkg/controller/spiffe-csi-driver/daemonset_test.go (3)
112-119: Avoid hard-coded init image; assert via utility to prevent drift.Use utils.GetSpiffeCsiInitContainerImage() instead of a literal.
- if container.Image != "registry.access.redhat.com/ubi9:latest" { - t.Errorf("Expected init container image 'registry.access.redhat.com/ubi9:latest', got '%s'", container.Image) - } + if container.Image != utils.GetSpiffeCsiInitContainerImage() { + t.Errorf("Expected init container image from utils.GetSpiffeCsiInitContainerImage(), got '%s'", container.Image) + }
116-119: Bind expected command to constants used by the generator.Prevents mismatch with future constant updates.
- expectedCommand := []string{"chcon", "-Rvt", "container_file_t", "spire-agent-socket/"} + expectedCommand := []string{SpiffeCSICommandChcon, SpiffeCSIArgRecursive, SpiffeCSIArgSELinuxType, SpiffeCSIArgTargetDir}
130-134: Capabilities: prefer the constant (and ensure correct casing).Literal "all" may not match constant casing and Kubernetes expects "ALL".
- expectedCapabilities := []corev1.Capability{"all"} + expectedCapabilities := []corev1.Capability{SpiffeCSICapabilityDropAll}Also applies to: 212-216
pkg/controller/spire-agent/configmap.go (2)
88-105: Consider validating unrecognized verification types.When an unrecognized verification type is provided, the switch silently falls through without setting any fields. Consider adding a default case to return an error for invalid types.
Apply this diff:
switch verification.Type { case "hostCert": // Validate HostCertBasePath is non-empty when hostCert verification is enabled if verification.HostCertBasePath == "" { return nil, fmt.Errorf("hostCertBasePath is required when workload attestor verification type is 'hostCert'") } workloadAttestorPluginData.SkipKubeletVerification = false workloadAttestorPluginData.VerifyKubeletCertificate = true workloadAttestorPluginData.KubeletCAPath = verification.HostCertBasePath case "apiServerCA": workloadAttestorPluginData.SkipKubeletVerification = false workloadAttestorPluginData.VerifyKubeletCertificate = true case "skip": workloadAttestorPluginData.SkipKubeletVerification = true case "auto": // Let SPIRE decide workloadAttestorPluginData.SkipKubeletVerification = false + default: + return nil, fmt.Errorf("invalid workload attestor verification type: %s (must be one of: hostCert, apiServerCA, skip, auto)", verification.Type) }
119-144: Improve error reporting in deprecated wrapper.The deprecated
GenerateAgentConfiglogs errors but returns an empty map. Consider logging with more context (e.g., including the error details in a structured way) or adding a comment that this behavior is intentional for backward compatibility.Current implementation swallows errors. If this is intentional for backward compatibility, add a comment:
// GenerateAgentConfig creates a structured agent configuration from the operator API spec (deprecated) // Use buildSpireAgentConfig instead +// Note: Returns empty map on error for backward compatibility. Errors are logged. func GenerateAgentConfig(cfg *v1alpha1.SpireAgent) map[string]interface{} {
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
Cache: Disabled due to data retention organization setting
Knowledge base: Disabled due to Reviews -> Disable Knowledge Base setting
📒 Files selected for processing (20)
pkg/config/spire_agent_config.go(1 hunks)pkg/config/spire_oidc_config.go(1 hunks)pkg/config/spire_server_config.go(1 hunks)pkg/controller/spiffe-csi-driver/constants.go(1 hunks)pkg/controller/spiffe-csi-driver/daemonset.go(4 hunks)pkg/controller/spiffe-csi-driver/daemonset_test.go(2 hunks)pkg/controller/spire-agent/configmap.go(1 hunks)pkg/controller/spire-agent/configmap_test.go(9 hunks)pkg/controller/spire-agent/constants.go(1 hunks)pkg/controller/spire-agent/daemonset.go(4 hunks)pkg/controller/spire-oidc-discovery-provider/configmap_test.go(2 hunks)pkg/controller/spire-oidc-discovery-provider/configmaps.go(1 hunks)pkg/controller/spire-oidc-discovery-provider/constants.go(1 hunks)pkg/controller/spire-oidc-discovery-provider/deployments.go(2 hunks)pkg/controller/spire-server/configmap.go(2 hunks)pkg/controller/spire-server/configmap_test.go(1 hunks)pkg/controller/spire-server/configmaps_test.go(0 hunks)pkg/controller/spire-server/constants.go(1 hunks)pkg/controller/spire-server/controller.go(2 hunks)pkg/controller/spire-server/statefulset.go(2 hunks)
💤 Files with no reviewable changes (1)
- pkg/controller/spire-server/configmaps_test.go
🚧 Files skipped from review as they are similar to previous changes (3)
- pkg/controller/spire-server/controller.go
- pkg/config/spire_oidc_config.go
- pkg/controller/spire-server/constants.go
🔇 Additional comments (17)
pkg/controller/spire-oidc-discovery-provider/configmaps.go (1)
59-60: Clean refactoring to typed config builder.The change from inline map construction to a typed config struct with a dedicated builder function improves type safety and maintainability.
pkg/controller/spire-oidc-discovery-provider/deployments.go (1)
24-127: Excellent refactoring to eliminate magic strings.Replacing hard-coded literals with well-named constants significantly improves maintainability and makes the deployment configuration self-documenting. The constant names are clear and consistent.
pkg/controller/spire-oidc-discovery-provider/constants.go (2)
4-74: Well-organized constant definitions.The constants are clearly documented, consistently named, and logically grouped. This centralization will make configuration changes much easier.
53-53: Remove or utilize the unusedSpireOIDCConfigSubPathconstant.Verification confirms the constant is defined but not used anywhere in the codebase. Since this is a WIP PR, either remove the constant if not needed, or implement its usage.
pkg/controller/spire-oidc-discovery-provider/configmap_test.go (3)
212-300: Comprehensive test coverage for config builder.The table-driven tests effectively validate the config builder with various scenarios including defaults, custom values, and edge cases. The validation functions provide clear assertions.
302-431: Excellent test structure for ConfigMap generation.The tests thoroughly cover:
- Valid config generation with correct JSON structure
- Label propagation
- Error handling (nil CR, invalid JWT issuer)
- JSON validation
The use of validation callbacks makes the tests flexible and maintainable.
433-458: Round-trip marshaling test ensures serialization correctness.Testing that JSON marshaling and unmarshaling preserves all fields is essential for config persistence. This test provides confidence that the config types have correct JSON tags.
pkg/controller/spiffe-csi-driver/daemonset.go (1)
24-91: Refactor to constants looks good.Selectors, update strategy, args/env/volume wiring all consistently pull from constants.
Also applies to: 126-176, 178-196
pkg/controller/spire-agent/configmap_test.go (2)
592-815: LGTM! Comprehensive test coverage for the new builder.The new
TestBuildSpireAgentConfigprovides excellent coverage of the typed config builder across multiple scenarios:
- Minimal agent config with defaults
- Node attestor with k8s_psat
- Workload attestor variations (basic, hostCert, apiServerCA, skip, auto verification modes)
- Combined node and workload attestors
The test structure is clear, validations are thorough, and the use of validation functions keeps each test case focused.
817-899: LGTM! Effective validation of struct-based ConfigMap generation.The
TestGenerateSpireAgentConfigMap_WithStructstest validates the end-to-end flow from spec to ConfigMap with proper JSON structure checks. The validation ensures:
- ConfigMap metadata is correct
- Generated JSON is valid
- Required sections (agent, plugins, health_checks, telemetry) are present
- Plugin structures are correctly nested
pkg/controller/spire-agent/daemonset.go (1)
14-151: LGTM! Effective constant-driven refactoring.The replacement of hard-coded literals with constants significantly improves maintainability. The changes are purely declarative substitutions that:
- Centralize configuration values
- Make the code more readable
- Reduce the risk of typos and inconsistencies
- Enable easier updates across the codebase
All constants are used consistently and the control flow remains unchanged.
pkg/controller/spire-server/configmap_test.go (2)
20-170: Comprehensive builder validation with good coverage.The
TestBuildSpireServerConfigtest provides excellent coverage:
- Minimal config with required fields
- Memory vs disk key manager
- PostgreSQL with TLS options
- Validates all major config sections (server, CA subject, health checks, telemetry, plugins)
The test structure is clear and validations are thorough.
238-288: Good validation of error cases.The error case tests properly validate that required fields are checked:
- Missing trust domain
- Missing cluster name
- Missing bundle configmap
- Missing datastore
- Missing CA subject
These validations help ensure the builder fails fast on invalid input.
pkg/controller/spire-server/configmap.go (2)
163-205: LGTM! Comprehensive validation and error handling.The
GenerateSpireServerConfigMapfunction properly validates all required inputs before building the config:
- spec is non-nil
- TrustDomain is non-empty
- ClusterName is non-empty
- BundleConfigMap is non-empty
- Datastore is non-nil
- CASubject is non-nil
Error handling for JSON marshaling is also correct. This provides good defense against invalid inputs.
207-225: Deprecated function properly handles errors.The
generateServerConfMapfunction is correctly marked as deprecated and now returns errors instead of swallowing them. The error messages are clear and actionable.pkg/controller/spiffe-csi-driver/constants.go (1)
1-106: LGTM! Well-organized constants with clear documentation.The new constants file provides excellent centralization of configuration values:
- Logical grouping by purpose (DaemonSet, containers, volumes, etc.)
- Clear naming conventions (e.g.,
SpiffeCSIprefix)- Proper use of typed constants (e.g.,
corev1.TerminationMessageReadFile)- Good documentation comments explaining derived values (lines 51-52, 84-85)
This will significantly improve maintainability and reduce the risk of inconsistencies across the codebase.
pkg/controller/spire-server/statefulset.go (1)
19-147: LGTM! Consistent constant-driven refactoring.The StatefulSet generation mirrors the DaemonSet refactoring approach, replacing hard-coded literals with well-named constants. The changes:
- Improve maintainability and consistency
- Make the code more readable
- Enable easier updates across the codebase
- Maintain the same functional behavior
All constants are used consistently and the control flow is unchanged.
| // ServerPort is the port number of the SPIRE server | ||
| ServerPort string `json:"server_port"` | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
server_port should be an integer.
SPIRE expects a numeric port; emitting a JSON string may break parsing.
- ServerPort string `json:"server_port"`
+ ServerPort int `json:"server_port"`📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| // ServerPort is the port number of the SPIRE server | |
| ServerPort string `json:"server_port"` | |
| // ServerPort is the port number of the SPIRE server | |
| ServerPort int `json:"server_port"` |
🤖 Prompt for AI Agents
In pkg/config/spire_agent_config.go around lines 29 to 31, the ServerPort field
is defined as a string which emits JSON as a string; change its type to an
integer (int) so the JSON emits a numeric port value expected by SPIRE, update
any dependent code or tests that set or parse this field to use an int, and add
basic validation/zero-value handling where this config is read to ensure a valid
port range if not already present.
| // JoinToken is the join token for attestation | ||
| JoinToken string `json:"join_token,omitempty"` | ||
|
|
There was a problem hiding this comment.
Avoid embedding join_token in ConfigMaps; use Secret references.
Inline JoinToken will serialize into a ConfigMap. Prefer SecretRef fields and fetch at runtime.
Suggested change:
- // JoinToken is the join token for attestation
- JoinToken string `json:"join_token,omitempty"`
+ // JoinToken via Secret reference (name/key). Inline token is deprecated for security.
+ JoinTokenSecretRefName string `json:"join_token_secret_ref_name,omitempty"`
+ JoinTokenSecretRefKey string `json:"join_token_secret_ref_key,omitempty"`
+ // Deprecated: use JoinTokenSecretRefName/JoinTokenSecretRefKey
+ JoinToken string `json:"join_token,omitempty"`📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| // JoinToken is the join token for attestation | |
| JoinToken string `json:"join_token,omitempty"` | |
| // JoinToken via Secret reference (name/key). Inline token is deprecated for security. | |
| JoinTokenSecretRefName string `json:"join_token_secret_ref_name,omitempty"` | |
| JoinTokenSecretRefKey string `json:"join_token_secret_ref_key,omitempty"` | |
| // Deprecated: use JoinTokenSecretRefName/JoinTokenSecretRefKey | |
| JoinToken string `json:"join_token,omitempty"` | |
| BindPort string `json:"bind_port"` | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Ports should be integers to match SPIRE schema.
bind_port, health_checks.bind_port, upstream authority server_port, and Prometheus port should be numbers, not strings.
Apply these diffs:
- BindPort string `json:"bind_port"`
+ BindPort int `json:"bind_port"`- BindPort string `json:"bind_port"`
+ BindPort int `json:"bind_port"`- ServerPort string `json:"server_port,omitempty"`
+ ServerPort int `json:"server_port,omitempty"`- Port string `json:"port,omitempty"`
+ Port int `json:"port,omitempty"`Also applies to: 253-255, 247-248, 271-273
🤖 Prompt for AI Agents
pkg/config/spire_server_config.go lines 22-23 (and also apply same change at
lines 247-248, 253-255, 271-273): the struct fields representing ports are
declared as string but must be numeric to match the SPIRE schema; change the Go
types for bind_port, health_checks.bind_port, upstream authority server_port,
and the Prometheus port from string to int (or another integer type consistent
with the repo convention), keep the existing json tags unchanged, and update any
code that constructs/parses these structs to use integers (parsing from strings
where necessary) so JSON marshaling/unmarshaling and validation match the
schema.
| // CASubject represents the Subject that CA certificates should use | ||
| CASubject []CASubject `json:"ca_subject"` | ||
|
|
There was a problem hiding this comment.
ca_subject should be a single object, not an array.
SPIRE server expects ca_subject as a single struct. Using []CASubject will emit an array and break parsing.
Apply this diff:
- // CASubject represents the Subject that CA certificates should use
- CASubject []CASubject `json:"ca_subject"`
+ // CASubject represents the Subject that CA certificates should use
+ CASubject CASubject `json:"ca_subject"`📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| // CASubject represents the Subject that CA certificates should use | |
| CASubject []CASubject `json:"ca_subject"` | |
| // CASubject represents the Subject that CA certificates should use | |
| CASubject CASubject `json:"ca_subject"` |
🤖 Prompt for AI Agents
In pkg/config/spire_server_config.go around lines 36 to 38, the struct field
CASubject is declared as a slice ([]CASubject) but SPIRE expects a single
object; change the field type from []CASubject to CASubject (keep the json tag
as "ca_subject") so the marshaled/configured value is a single object instead of
an array, and update any usages or tests that construct or access CASubject
accordingly.
| // CATTL is the TTL of the server CA | ||
| CATTL metav1.Duration `json:"ca_ttl"` | ||
|
|
||
| // DefaultX509SVIDTTL is the default TTL for X509-SVIDs | ||
| DefaultX509SVIDTTL metav1.Duration `json:"default_x509_svid_ttl"` | ||
|
|
||
| // DefaultJWTSVIDTTL is the default TTL for JWT-SVIDs | ||
| DefaultJWTSVIDTTL metav1.Duration `json:"default_jwt_svid_ttl"` | ||
|
|
There was a problem hiding this comment.
Danger: zero TTLs will be serialized; switch away from non-pointer metav1.Duration.
Using metav1.Duration (non-pointer) forces zero-values like "0s" into the config, overriding SPIRE defaults. Make these string durations (e.g., "24h") or pointer types.
Apply this diff to align with other duration fields already modeled as strings:
- CATTL metav1.Duration `json:"ca_ttl"`
- DefaultX509SVIDTTL metav1.Duration `json:"default_x509_svid_ttl"`
- DefaultJWTSVIDTTL metav1.Duration `json:"default_jwt_svid_ttl"`
+ CATTL string `json:"ca_ttl,omitempty"`
+ DefaultX509SVIDTTL string `json:"default_x509_svid_ttl,omitempty"`
+ DefaultJWTSVIDTTL string `json:"default_jwt_svid_ttl,omitempty"`📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| // CATTL is the TTL of the server CA | |
| CATTL metav1.Duration `json:"ca_ttl"` | |
| // DefaultX509SVIDTTL is the default TTL for X509-SVIDs | |
| DefaultX509SVIDTTL metav1.Duration `json:"default_x509_svid_ttl"` | |
| // DefaultJWTSVIDTTL is the default TTL for JWT-SVIDs | |
| DefaultJWTSVIDTTL metav1.Duration `json:"default_jwt_svid_ttl"` | |
| // CATTL is the TTL of the server CA | |
| CATTL string `json:"ca_ttl,omitempty"` | |
| // DefaultX509SVIDTTL is the default TTL for X509-SVIDs | |
| DefaultX509SVIDTTL string `json:"default_x509_svid_ttl,omitempty"` | |
| // DefaultJWTSVIDTTL is the default TTL for JWT-SVIDs | |
| DefaultJWTSVIDTTL string `json:"default_jwt_svid_ttl,omitempty"` |
🤖 Prompt for AI Agents
In pkg/config/spire_server_config.go around lines 42 to 50, the three TTL fields
are declared as non-pointer metav1.Duration which serializes zero-values ("0s")
and overrides SPIRE defaults; change those fields to use string durations
(matching other fields) or pointer types (e.g., *metav1.Duration) so zero/unset
values are omitted. Update the field types in the struct, adjust any code that
reads/writes these fields to parse/format string durations (or handle nil
pointers), and run tests to ensure defaulting behavior and serialization now
omit zero TTLs.
| // CacheReloadInterval is the interval to reload in-memory entry cache | ||
| CacheReloadInterval string `json:"cache_reload_interval,omitempty"` | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Duplicate cache_reload_interval; keep it only under experimental.
Defining cache_reload_interval both top-level and under experimental is confusing and can yield conflicting config.
Apply this diff to drop the top-level field:
- // CacheReloadInterval is the interval to reload in-memory entry cache
- CacheReloadInterval string `json:"cache_reload_interval,omitempty"`Also applies to: 91-93
🤖 Prompt for AI Agents
In pkg/config/spire_server_config.go around lines 72-74 (and similarly 91-93),
remove the top-level CacheReloadInterval string field declaration so that
cache_reload_interval exists only under the Experimental struct; delete the two
lines defining CacheReloadInterval (including its comment and struct tag) and
any duplicate comment, ensure the Experimental struct retains the
cache_reload_interval field and JSON tag, then run gofmt and `go vet`/unit tests
to confirm no references remain to the removed top-level field.
| // buildSpireServerConfig creates a SpireServerConfig from the operator API spec | ||
| func buildSpireServerConfig(spec *v1alpha1.SpireServerSpec) *config.SpireServerConfig { | ||
| serverConfig := &config.SpireServerConfig{ |
There was a problem hiding this comment.
Builder should validate required inputs.
The buildSpireServerConfig function doesn't validate its inputs, relying on the caller to do so. However, since this function can be called directly (as shown in tests), it should validate that spec is non-nil and required fields are non-empty to prevent generating invalid configs.
Apply this diff:
// buildSpireServerConfig creates a SpireServerConfig from the operator API spec
func buildSpireServerConfig(spec *v1alpha1.SpireServerSpec) *config.SpireServerConfig {
+ // Note: Caller is expected to validate spec before calling this function.
+ // For production use via GenerateSpireServerConfigMap, validation is performed there.
+ // Direct usage (e.g., in tests) should ensure spec is valid.
+ if spec == nil {
+ // Return a minimal config to avoid panics, but this is a programming error
+ return &config.SpireServerConfig{}
+ }
+
serverConfig := &config.SpireServerConfig{Alternatively, change the signature to return an error and validate properly:
-func buildSpireServerConfig(spec *v1alpha1.SpireServerSpec) *config.SpireServerConfig {
+func buildSpireServerConfig(spec *v1alpha1.SpireServerSpec) (*config.SpireServerConfig, error) {
+ if spec == nil {
+ return nil, fmt.Errorf("spec is nil")
+ }
+ if spec.TrustDomain == "" {
+ return nil, fmt.Errorf("trust_domain is required")
+ }
+ if spec.ClusterName == "" {
+ return nil, fmt.Errorf("cluster name is required")
+ }Then update callers (including tests) to handle the error.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| // buildSpireServerConfig creates a SpireServerConfig from the operator API spec | |
| func buildSpireServerConfig(spec *v1alpha1.SpireServerSpec) *config.SpireServerConfig { | |
| serverConfig := &config.SpireServerConfig{ | |
| // buildSpireServerConfig creates a SpireServerConfig from the operator API spec | |
| func buildSpireServerConfig(spec *v1alpha1.SpireServerSpec) *config.SpireServerConfig { | |
| // Note: Caller is expected to validate spec before calling this function. | |
| // For production use via GenerateSpireServerConfigMap, validation is performed there. | |
| // Direct usage (e.g., in tests) should ensure spec is valid. | |
| if spec == nil { | |
| // Return a minimal config to avoid panics, but this is a programming error | |
| return &config.SpireServerConfig{} | |
| } | |
| serverConfig := &config.SpireServerConfig{ |
🤖 Prompt for AI Agents
In pkg/controller/spire-server/configmap.go around lines 22–24, the builder must
validate inputs: change the signature of buildSpireServerConfig to return
(*config.SpireServerConfig, error), check that spec is non-nil and validate all
fields the builder relies on are present/non-empty (any string identifiers,
trust domain, or paths used later), and return a descriptive error if validation
fails; update all callers and tests to handle the error return and use the
validated config when non-nil.
c60761e to
12fecee
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (14)
pkg/config/spire_agent_config.go (4)
29-31: **** ServerPort should be an integer to match SPIRE's expected numeric port value in JSON. String serialization may cause parsing issues.
50-52: **** Embedding JoinToken directly in the struct will serialize it into ConfigMaps, exposing secrets in etcd. Consider using Secret references instead.
121-139: **** AWS credentials (AccessKeyID, SecretAccessKey, SecurityToken) in the config struct will serialize into ConfigMaps if populated. While SPIRE's schema supports these fields, the operator should populate them from Secret references and mount via volumes/env, not serialize into ConfigMaps.
172-199: **** SVIDStorePluginData contains sensitive fields (AWS credentials, GCP service account file, Vault token) that will serialize into ConfigMaps. The operator should use Secret references and runtime injection instead of inline values.pkg/controller/spire-server/configmap_test.go (2)
129-159: **** Test embeds a database password directly in the ConnectionString ("postgresql://user:password@postgres:5432/spire") and validates it appears in the ConfigMap. This normalizes secret leakage. Consider testing with placeholder values or environment variable references (e.g., "${DB_CONN}") and documenting that the operator should use SPIRE's-expandEnvflag with Secret-backed env vars.
894-922: **** The test helpercreateValidConfig()includes a hardcoded password in the database ConnectionString. For test fixtures that feed ConfigMap generation, consider using a placeholder or documenting that real deployments must use Secret references with environment variable expansion.pkg/config/spire_server_config.go (7)
22-23: **** BindPort should be an integer type (int) to emit numeric JSON that matches SPIRE's schema, not a string.
36-38: **** CASubject is declared as[]CASubject(array) but SPIRE expects a single object. Change toCASubject CASubjectto serialize correctly.
42-50: **** CATTL, DefaultX509SVIDTTL, and DefaultJWTSVIDTTL use non-pointermetav1.Duration, which will serialize zero values ("0s") and override SPIRE defaults. Consider using string durations withomitemptyor pointer types (*metav1.Duration).
72-74: **** CacheReloadInterval is defined both at the top level (lines 72-74) and under ServerExperimental (lines 91-93), creating ambiguity. Remove the top-level field and keep only the experimental one.Also applies to: 91-93
247-248: **** UpstreamAuthorityPluginData.ServerPort should be an integer to match SPIRE's numeric port expectation.
253-255: **** HealthChecks.BindPort should be an integer type for correct JSON serialization.
271-273: **** PrometheusConfig.Port should be an integer type, not a string.pkg/controller/spire-agent/configmap.go (1)
15-27: Validate required TrustDomain at function entry.An empty TrustDomain will produce an invalid agent config. Add validation at the start of the function to fail fast.
Apply this diff:
func buildSpireAgentConfig(spec *v1alpha1.SpireAgentSpec) (*config.SpireAgentConfig, error) { + if spec == nil { + return nil, fmt.Errorf("spec cannot be nil") + } + if spec.TrustDomain == "" { + return nil, fmt.Errorf("trust_domain is required") + } + agentConfig := &config.SpireAgentConfig{
🧹 Nitpick comments (1)
pkg/controller/spire-agent/configmap.go (1)
88-106: Fix indentation and add default case for robustness.The switch cases starting at line 97 are misaligned. Additionally, a default case would catch unexpected verification types and prevent silent misconfigurations.
Apply this diff:
switch verification.Type { case "hostCert": // Validate HostCertBasePath is non-empty when hostCert verification is enabled if verification.HostCertBasePath == "" { return nil, fmt.Errorf("hostCertBasePath is required when workload attestor verification type is 'hostCert'") } workloadAttestorPluginData.SkipKubeletVerification = false workloadAttestorPluginData.VerifyKubeletCertificate = true workloadAttestorPluginData.KubeletCAPath = verification.HostCertBasePath - case "apiServerCA": - workloadAttestorPluginData.SkipKubeletVerification = false - workloadAttestorPluginData.VerifyKubeletCertificate = true - case "skip": - workloadAttestorPluginData.SkipKubeletVerification = true - case "auto": - // Let SPIRE decide - workloadAttestorPluginData.SkipKubeletVerification = false - } + case "apiServerCA": + workloadAttestorPluginData.SkipKubeletVerification = false + workloadAttestorPluginData.VerifyKubeletCertificate = true + case "skip": + workloadAttestorPluginData.SkipKubeletVerification = true + case "auto": + // Let SPIRE decide + workloadAttestorPluginData.SkipKubeletVerification = false + default: + return nil, fmt.Errorf("unsupported workload attestor verification type: %s", verification.Type) + }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
Cache: Disabled due to data retention organization setting
Knowledge base: Disabled due to Reviews -> Disable Knowledge Base setting
📒 Files selected for processing (20)
pkg/config/spire_agent_config.go(1 hunks)pkg/config/spire_oidc_config.go(1 hunks)pkg/config/spire_server_config.go(1 hunks)pkg/controller/spiffe-csi-driver/constants.go(1 hunks)pkg/controller/spiffe-csi-driver/daemonset.go(4 hunks)pkg/controller/spiffe-csi-driver/daemonset_test.go(2 hunks)pkg/controller/spire-agent/configmap.go(1 hunks)pkg/controller/spire-agent/configmap_test.go(9 hunks)pkg/controller/spire-agent/constants.go(1 hunks)pkg/controller/spire-agent/daemonset.go(4 hunks)pkg/controller/spire-oidc-discovery-provider/configmap_test.go(2 hunks)pkg/controller/spire-oidc-discovery-provider/configmaps.go(1 hunks)pkg/controller/spire-oidc-discovery-provider/constants.go(1 hunks)pkg/controller/spire-oidc-discovery-provider/deployments.go(2 hunks)pkg/controller/spire-server/configmap.go(2 hunks)pkg/controller/spire-server/configmap_test.go(1 hunks)pkg/controller/spire-server/configmaps_test.go(0 hunks)pkg/controller/spire-server/constants.go(1 hunks)pkg/controller/spire-server/controller.go(2 hunks)pkg/controller/spire-server/statefulset.go(2 hunks)
💤 Files with no reviewable changes (1)
- pkg/controller/spire-server/configmaps_test.go
🚧 Files skipped from review as they are similar to previous changes (5)
- pkg/controller/spire-server/controller.go
- pkg/controller/spire-oidc-discovery-provider/configmap_test.go
- pkg/controller/spire-server/configmap.go
- pkg/config/spire_oidc_config.go
- pkg/controller/spire-oidc-discovery-provider/constants.go
🔇 Additional comments (17)
pkg/controller/spiffe-csi-driver/constants.go (2)
51-53: Path construction is clear and well-documented.The inline comment explaining that the path derives from the plugin name is helpful. The concatenation approach keeps the relationship explicit.
1-106: Well-organized constants file.The constants are logically grouped by category (DaemonSet metadata, init container, main containers, environment, volumes, paths, probes, security). This centralization improves maintainability and reduces hard-coded literals across the codebase.
pkg/controller/spire-server/constants.go (2)
7-7: Port constants are strings but may need to be integers.DefaultServerBindPort, DefaultHealthCheckBindPort, and DefaultPrometheusPort are defined as strings. These feed into config structs where past reviews indicated ports should be integers for proper JSON serialization. Consider defining these as integers (e.g.,
const DefaultServerBindPort = 8081) and converting to strings only when necessary for backward compatibility.Also applies to: 15-15, 21-21
111-116: Helper function is straightforward and correct.
getNodeAttestorServiceAccountAllowList()constructs the service account allow list from namespace and account constants. The implementation is clear.pkg/controller/spire-server/statefulset.go (1)
19-146: Consistent replacement of literals with constants.The StatefulSet generation now uses centralized constants for names, ports, paths, volumes, and probes. The logic flow is unchanged—this is a straightforward refactoring that improves maintainability. The constant names clearly correspond to their usage (e.g.,
SpireServerPortNameGRPC,SpireServerMountPathConfig).pkg/controller/spiffe-csi-driver/daemonset.go (1)
12-201: Clean refactoring to use centralized constants.The DaemonSet generation replaces all hard-coded literals with constants from
constants.go. The changes are mechanical and improve maintainability. Container names, arguments, environment variables, volume names, mount paths, and probe configuration all now reference well-named constants.pkg/controller/spire-oidc-discovery-provider/configmaps.go (2)
16-44: Well-structured config builder function.The
buildSpireOIDCDiscoveryProviderConfigfunction centralizes OIDC configuration construction with clear default handling (e.g., DefaultAgentSocketName fallback at lines 18-21). The typed config approach usingconfig.SpireOIDCDiscoveryProviderConfigimproves type safety over the previous map-based construction.
46-79: ConfigMap generation properly uses typed config.The refactored
GenerateOIDCConfigMapFromCRnow builds a typed config struct and marshals it to JSON, replacing the previous inline map approach. This pattern is consistent with the broader PR refactoring and improves maintainability and testability.pkg/controller/spiffe-csi-driver/daemonset_test.go (1)
487-708: LGTM! Excellent constant-driven test coverage.The new tests comprehensively validate that the DaemonSet generation logic correctly uses centralized constants for:
- Metadata (names, service accounts, update strategy)
- Init and main container configurations
- Volume names, mount paths, and host paths
- Ports, probes, and timing values
The
findVolumehelper function is clean and the constant validation tests provide good guardrails against accidental constant changes.pkg/controller/spire-agent/configmap_test.go (3)
450-457: Good fix! Safe type assertion protects against panics.The two-step type assertion with explicit checks prevents runtime panics if the config structure is unexpected.
592-815: LGTM! Comprehensive test coverage for the structured config builder.The test cases thoroughly validate:
- Basic agent config construction with defaults
- NodeAttestor plugin data structures
- WorkloadAttestor configurations with various verification modes (hostCert, apiServerCA, skip, auto)
- Combined attestor scenarios
The validation functions properly check typed fields and plugin data structures.
817-899: LGTM! Validates JSON output from structured configs.The test confirms that the ConfigMap generation produces valid JSON with the expected structure for all top-level sections and plugin configurations.
pkg/controller/spire-agent/configmap.go (2)
119-144: LGTM! Error handling and logging are appropriate.The function now properly handles and logs errors from the builder and JSON marshaling operations, returning an empty map for backward compatibility.
146-176: LGTM! Clean integration of structured config builder.The ConfigMap generation now uses the typed builder and marshals the result to JSON, with proper error propagation.
pkg/controller/spire-agent/daemonset.go (1)
28-142: LGTM! Consistent constant usage throughout.All hardcoded literals have been cleanly replaced with named constants, improving maintainability and reducing the risk of typos across DaemonSet metadata, container configuration, volumes, and health checks.
pkg/controller/spire-oidc-discovery-provider/deployments.go (1)
24-116: LGTM! Consistent constant usage across OIDC deployment.All hardcoded literals replaced with descriptive constants for deployment name, service accounts, volumes, mounts, ports, and probe configurations.
pkg/controller/spire-agent/constants.go (1)
1-91: LGTM! Well-organized constant definitions.The constants are logically grouped and consistently named, providing clear defaults for agent configuration, DaemonSet metadata, volumes, paths, and probe timings.
Based on learnings
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (4)
pkg/controller/spire-server/configmap_test.go (3)
893-922: Critical: Test helper embeds plaintext DB credentials.The
createValidConfighelper at line 906 contains a hardcoded password in the connection string:"postgresql://postgres:password@postgres:5432/spire". This helper is used by multiple tests throughout the file, causing all dependent tests to normalize secret leakage into ConfigMaps.Refactor to use a secure pattern:
- ConnectionString: "postgresql://postgres:password@postgres:5432/spire", + ConnectionString: "${SPIRE_DB_CONNECTION_STRING}",Then update dependent tests to:
- Assert the ConfigMap contains only the placeholder
- Add separate test coverage for Secret-to-environment-variable injection in the StatefulSet
374-480: Critical: Test validates plaintext credentials in ConfigMap.This test uses
createValidConfig()(which contains a hardcoded password at line 906) and explicitly asserts at lines 447-451 that the password-containing connection string appears in the generated config. This validates the insecure behavior of serializing secrets into ConfigMaps.Update this test to:
- Use a connection string placeholder (e.g.,
"${SPIRE_DB_CONNECTION_STRING}")- Assert that the ConfigMap contains only the placeholder, not plaintext credentials
- Verify that secret material is omitted from the JSON output
Also applies to: 482-563 (TestGenerateServerConfMapTTLFields), 565-615 (TestGenerateSpireServerConfigMapWithTTLFields)
115-160: Critical: Tests still embed plaintext DB credentials in ConfigMap.This test case validates that a password-containing connection string (
"postgresql://user:password@postgres:5432/spire") is serialized into the ConfigMap, which is the same security issue flagged in the previous review.While SPIRE does accept this connection string format, SPIRE supports the
-expandEnvflag to enable environment variable expansion. Tests should demonstrate the secure pattern:
- Store
connection_string = "${SPIRE_DB_CONNECTION_STRING}"in ConfigMap (placeholder only)- Add
-expandEnvflag to SPIRE server container args- Inject actual credentials from Secret as env var
Update this test to:
- Use a placeholder like
"${SPIRE_DB_CONNECTION_STRING}"in the connection string- Assert that the ConfigMap contains only the placeholder (no plaintext credentials)
- Add test coverage for the StatefulSet to verify Secret-to-env-var wiring
This applies to all PostgreSQL test cases in this file.
pkg/controller/spire-agent/configmap.go (1)
15-28: Validate required inputs early.The function does not validate that
spec.TrustDomainis non-empty, which can result in invalid SPIRE agent configurations. This validation was previously requested but not implemented.Apply this diff to add the validation:
func buildSpireAgentConfig(spec *v1alpha1.SpireAgentSpec) (*config.SpireAgentConfig, error) { + if spec == nil { + return nil, fmt.Errorf("spec cannot be nil") + } + if strings.TrimSpace(spec.TrustDomain) == "" { + return nil, fmt.Errorf("trust_domain is required") + } + agentConfig := &config.SpireAgentConfig{You'll also need to add the
stringsimport:import ( "encoding/json" "fmt" "log" + "strings"
🧹 Nitpick comments (2)
pkg/controller/spire-server/configmap_test.go (1)
374-480: Consider using testify assertions consistently.This test function uses manual assertion style with
ifstatements andt.Errorf()(e.g., lines 384-390, 397-402, 420-429), while other test functions in this file use testify'sassertandrequire(e.g.,TestBuildSpireServerConfigat lines 50-76).For consistency and improved readability, consider refactoring these to use testify assertions:
- if server["trust_domain"] != validConfig.TrustDomain { - t.Errorf("Expected trust_domain %q, got %v", validConfig.TrustDomain, server["trust_domain"]) - } + assert.Equal(t, validConfig.TrustDomain, server["trust_domain"])This pattern applies throughout lines 374-615 (TestGenerateServerConfMap, TestGenerateServerConfMapTTLFields, TestGenerateSpireServerConfigMapWithTTLFields).
pkg/controller/spire-agent/configmap.go (1)
100-116: Document the auto mode limitation.The comment indicates "Auto mode currently defaults to skip since we can't reliably auto-detect CA paths," which suggests incomplete functionality. If auto mode is intended for future implementation, consider adding a TODO comment or user-facing warning.
Consider enhancing the switch statement:
switch verification.Type { case "hostCert": // Validate HostCertBasePath is non-empty when hostCert verification is enabled if verification.HostCertBasePath == "" { return nil, fmt.Errorf("hostCertBasePath is required when workload attestor verification type is 'hostCert'") } workloadAttestorPluginData.SkipKubeletVerification = false workloadAttestorPluginData.VerifyKubeletCertificate = true workloadAttestorPluginData.KubeletCAPath = getKubeletCAPath() case "apiServerCA": workloadAttestorPluginData.SkipKubeletVerification = false workloadAttestorPluginData.VerifyKubeletCertificate = true case "skip", "auto": - // For both skip and auto modes, skip kubelet verification - // Auto mode currently defaults to skip since we can't reliably auto-detect CA paths + // TODO: Implement auto-detection of kubelet CA paths for auto mode + // For now, both skip and auto modes skip kubelet verification workloadAttestorPluginData.SkipKubeletVerification = true }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
Cache: Disabled due to data retention organization setting
Knowledge base: Disabled due to Reviews -> Disable Knowledge Base setting
📒 Files selected for processing (4)
pkg/controller/spire-agent/configmap.go(1 hunks)pkg/controller/spire-agent/constants.go(1 hunks)pkg/controller/spire-agent/daemonset.go(3 hunks)pkg/controller/spire-server/configmap_test.go(7 hunks)
🔇 Additional comments (6)
pkg/controller/spire-agent/configmap.go (2)
55-72: LGTM! Good validation for NodeAttestor configuration.The conditional plugin addition correctly validates that
ClusterNameis non-empty when k8s_psat attestation is enabled, and usesStringToBoolfor resilient boolean parsing.
130-187: LGTM! Error handling improvements implemented.Both
GenerateAgentConfigandGenerateSpireAgentConfigMapnow properly handle errors from the builder function. The JSON marshal/unmarshal error handling addresses previous review feedback, and backward compatibility is maintained inGenerateAgentConfigby returning an empty map on failure.pkg/controller/spire-agent/daemonset.go (3)
26-36: LGTM! Consistent kubelet PKI mount logic.The conditional mounting logic correctly mirrors the validation in
configmap.go, ensuring the DaemonSet only mounts the kubelet PKI directory when hostCert verification mode is enabled with a valid path.
38-104: LGTM! Well-structured constant usage.The DaemonSet construction properly uses constants from
constants.gothroughout, replacing hard-coded literals. This improves maintainability and consistency across the codebase.
105-182: LGTM! Clean conditional resource construction.The inline functions for constructing volume mounts and volumes make the conditional kubelet PKI mounting logic clear and avoid mutation. The appropriate use of
HostPathDirectoryfor kubelet PKI (read-only system directory) vsHostPathDirectoryOrCreatefor agent sockets is correct.pkg/controller/spire-agent/constants.go (1)
1-92: LGTM! Well-organized constant definitions.The constants are logically grouped by functional area and consistently named. The distinction between
DefaultAgentSocketPath(/tmp for agent runtime) andSpireAgentHostPathAgentSockets(/run for host mount) is appropriate for SPIRE's architecture.
anirudhAgniRedhat
force-pushed
the
configStruct
branch
from
738e646 to
20316d6
Compare
|
@anirudhAgniRedhat: all tests passed! Full PR test history. Your PR dashboard. DetailsInstructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. I understand the commands that are listed here. |
|
Issues go stale after 90d of inactivity. Mark the issue as fresh by commenting If this issue is safe to close now please do so with /lifecycle stale |
openshift-merge-robot
added
the
needs-rebase
|
PR needs rebase. DetailsInstructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. |
openshift-ci
bot
added
the
lifecycle/stale
|
Stale issues rot after 30d of inactivity. Mark the issue as fresh by commenting If this issue is safe to close now please do so with /lifecycle rotten |
openshift-ci
bot
added
lifecycle/rotten
3 participants
No description provided.