SPIRE-129: Add native UpstreamAuthority plugin support (cert-manager, Vault)#113
Open
anirudhAgniRedhat wants to merge 4 commits into
Open
SPIRE-129: Add native UpstreamAuthority plugin support (cert-manager, Vault)#113anirudhAgniRedhat wants to merge 4 commits into
anirudhAgniRedhat wants to merge 4 commits into
Conversation
|
@anirudhAgniRedhat: This pull request references SPIRE-129 which is a valid jira issue. Warning: The referenced jira issue has an invalid target version for the target branch this PR targets: expected the story to target the "5.0.0" version, but no target version was set. DetailsIn response to this:
Instructions 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 openshift-eng/jira-lifecycle-plugin repository. |
openshift-ci-robot
added
the
jira/valid-reference
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughAdds an optional SpireServer.spec.upstreamAuthority to select exactly one external PKI backend (cert-manager or Vault); updates API types, CRD schema/validation, CSV/RBAC, ConfigMap/StatefulSet generation, reconcile validation, RBAC reconciliation, and unit tests covering the new behavior. ChangesUpstream Authority feature
Sequence Diagram(s)sequenceDiagram
participant CR as SpireServer CR
participant Operator as Operator Controller
participant K8s as Kubernetes API
participant PKI as External PKI (Vault / cert-manager)
CR->>Operator: Create/Update SpireServer with spec.upstreamAuthority
Operator->>Operator: validateUpstreamAuthority()
alt validation fails
Operator->>K8s: update SpireServer.status (ConfigurationValid=false)
Operator-->>CR: reconciliation abort (error)
else validation passes
Operator->>K8s: reconcile ClusterRole (add cert-manager rule if cert-manager configured)
Operator->>K8s: create/update ConfigMap (server.conf with UpstreamAuthority plugin)
Operator->>K8s: create/update StatefulSet (add token projection / CA secret if Vault)
K8s->>PKI: runtime uses projected token or cert-manager CertificateRequest
Operator-->>CR: reconciliation complete (resources applied)
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes 🚥 Pre-merge checks | ✅ 10 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (10 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
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.12.1)Error: can't load config: unsupported version of the configuration: "" See https://golangci-lint.run/docs/product/migration-guide for migration instructions Tip 💬 Introducing Slack Agent: The best way for teams to turn conversations into code.Slack Agent is built on CodeRabbit's deep understanding of your code, so your team can collaborate across the entire SDLC without losing context.
Built for teams:
One agent for your entire SDLC. Right inside Slack. Comment |
openshift-ci
Bot
added
the
approved
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Nitpick comments (2)
pkg/controller/spire-server/configmap.go (1)
381-383: Centralize upstream CA path constants across config and pod wiring.Line 382 hardcodes
/run/spire/upstream-ca/ca.crt, while StatefulSet mount path is separately hardcoded. Keeping these in one shared constant reduces drift risk.♻️ Suggested direction
const ( defaultCaKeyType = "rsa-2048" vaultTokenPath = "/var/run/secrets/tokens/vault" + upstreamCAMountPath = "/run/spire/upstream-ca" + upstreamCACertPath = upstreamCAMountPath + "/ca.crt" ) ... - pluginData["ca_cert_path"] = "/run/spire/upstream-ca/ca.crt" + pluginData["ca_cert_path"] = upstreamCACertPathThen reuse
upstreamCAMountPathinpkg/controller/spire-server/statefulset.gomount configuration.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/controller/spire-server/configmap.go` around lines 381 - 383, Introduce a shared constant (e.g., upstreamCAMountPath) and replace the hardcoded string in the configmap generation (where pluginData["ca_cert_path"] is assigned when v.CACertSecretRef != nil) with that constant; then update the StatefulSet mount configuration in pkg/controller/spire-server/statefulset.go to reuse upstreamCAMountPath for the mount path instead of its own hardcoded value so both places reference the same identifier.pkg/controller/spire-server/statefulset_test.go (1)
1112-1116: Also assert absence of unexpected volume mounts in negative-path tests.These checks currently validate only volumes. Adding mount assertions will catch dangling mount regressions too.
✅ Suggested addition
func TestGenerateStatefulSet_CertManager(t *testing.T) { ... for _, vol := range sts.Spec.Template.Spec.Volumes { if vol.Name == "vault-token" || vol.Name == "upstream-ca" { t.Errorf("Unexpected volume %q for cert-manager upstream authority", vol.Name) } } + spireContainer := findContainerByName(sts.Spec.Template.Spec.Containers, "spire-server") + for _, m := range spireContainer.VolumeMounts { + if m.Name == "vault-token" || m.Name == "upstream-ca" { + t.Errorf("Unexpected volume mount %q for cert-manager upstream authority", m.Name) + } + } } func TestGenerateStatefulSet_NoUpstreamAuthority(t *testing.T) { ... for _, vol := range sts.Spec.Template.Spec.Volumes { if vol.Name == "vault-token" || vol.Name == "upstream-ca" { t.Errorf("Unexpected volume %q when no upstream authority configured", vol.Name) } } + spireContainer := findContainerByName(sts.Spec.Template.Spec.Containers, "spire-server") + for _, m := range spireContainer.VolumeMounts { + if m.Name == "vault-token" || m.Name == "upstream-ca" { + t.Errorf("Unexpected volume mount %q when no upstream authority configured", m.Name) + } + } }Also applies to: 1129-1133
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/controller/spire-server/statefulset_test.go` around lines 1112 - 1116, The test currently only checks for absence of volumes named "vault-token" and "upstream-ca" (iterating sts.Spec.Template.Spec.Volumes); extend the negative-path assertions to also iterate through sts.Spec.Template.Spec.Containers and sts.Spec.Template.Spec.InitContainers and verify none of their VolumeMounts have Name == "vault-token" or "upstream-ca", using the same t.Errorf message format (e.g., "Unexpected volumeMount %q for cert-manager upstream authority") so dangling mounts are caught as well.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@api/v1alpha1/spire_server_config_types.go`:
- Around line 398-403: The IssuerGroup field currently allows an explicit empty
string which bypasses the default; add a non-empty validation (e.g.
+kubebuilder:validation:MinLength=1 or a non-empty pattern) to IssuerGroup so
the default is honored when the field is omitted but empty strings are rejected,
and apply the same non-empty validation to the other defaultable path/group
fields referenced at lines 413-417 and 441-445 so none of those can be set to an
explicit empty string.
- Around line 408-411: The VaultAddr field currently allows both http and https;
tighten the kubebuilder validation Pattern on VaultAddr in
spire_server_config_types.go (field name: VaultAddr, struct tag
json:"vaultAddr") to require only HTTPS (e.g., change Pattern to ^https://.+)
and update the comment to state "HTTPS URL" so the CRD enforces secure transport
for token/cert flows.
In `@pkg/controller/spire-server/rbac_test.go`:
- Around line 2404-2410: The test is appending the cert-manager PolicyRule
manually which validates test code instead of production logic; instead call the
actual reconciler/helper used in production (reconcileClusterRole) with a Server
object whose Spec.UpstreamAuthority.CertManager is set, obtain the
returned/created ClusterRole (rbacv1.ClusterRole) and assert that its Rules
include APIGroups "cert-manager.io" and Resources "certificaterequests" with the
expected Verbs, removing the manual cr.Rules append; use the same approach for
the other two similar cases so all assertions exercise reconcileClusterRole and
inspect the real ClusterRole rules.
In `@pkg/controller/spire-server/validation.go`:
- Around line 196-197: The runtime check in validation.go currently accepts both
"http://" and "https://" for v.VaultAddr (v.VaultAddr) — change it to require
only "https://" (reject any "http://") and update the error message to say
"vault.vaultAddr must start with https://, got %s"; ensure any
documentation/comments reference using insecureSkipVerify for bypassing TLS
certificate validation when explicitly desired, and mirror this stricter
HTTPS-only rule in the CRD/API schema/validation so admission-time validation
matches the runtime check.
In `@pkg/controller/zero-trust-workload-identity-manager/controller.go`:
- Line 195: Remove the kubebuilder RBAC marker that grants cert-manager
CertificateRequest access (the line containing
"+kubebuilder:rbac:groups=cert-manager.io,resources=certificaterequests,verbs=create;get;list;delete"
in pkg/controller/zero-trust-workload-identity-manager/controller.go), since
CertificateRequest RBAC belongs to the SPIRE server path (see
pkg/controller/spire-server/rbac.go); after removing this marker, regenerate the
RBAC manifests and CSV from markers so the operator bundle reflects the
tightened least-privilege permissions.
---
Nitpick comments:
In `@pkg/controller/spire-server/configmap.go`:
- Around line 381-383: Introduce a shared constant (e.g., upstreamCAMountPath)
and replace the hardcoded string in the configmap generation (where
pluginData["ca_cert_path"] is assigned when v.CACertSecretRef != nil) with that
constant; then update the StatefulSet mount configuration in
pkg/controller/spire-server/statefulset.go to reuse upstreamCAMountPath for the
mount path instead of its own hardcoded value so both places reference the same
identifier.
In `@pkg/controller/spire-server/statefulset_test.go`:
- Around line 1112-1116: The test currently only checks for absence of volumes
named "vault-token" and "upstream-ca" (iterating
sts.Spec.Template.Spec.Volumes); extend the negative-path assertions to also
iterate through sts.Spec.Template.Spec.Containers and
sts.Spec.Template.Spec.InitContainers and verify none of their VolumeMounts have
Name == "vault-token" or "upstream-ca", using the same t.Errorf message format
(e.g., "Unexpected volumeMount %q for cert-manager upstream authority") so
dangling mounts are caught as well.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository: openshift/coderabbit/.coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 8f538eb3-8afd-480a-bcc8-0a67fe28ed31
⛔ Files ignored due to path filters (1)
api/v1alpha1/zz_generated.deepcopy.gois excluded by!**/zz_generated*
📒 Files selected for processing (15)
api/v1alpha1/spire_server_config_types.gobundle/manifests/operator.openshift.io_spireservers.yamlbundle/manifests/zero-trust-workload-identity-manager.clusterserviceversion.yamlconfig/crd/bases/operator.openshift.io_spireservers.yamlconfig/rbac/role.yamlpkg/controller/spire-server/configmap.gopkg/controller/spire-server/configmaps_test.gopkg/controller/spire-server/controller.gopkg/controller/spire-server/rbac.gopkg/controller/spire-server/rbac_test.gopkg/controller/spire-server/statefulset.gopkg/controller/spire-server/statefulset_test.gopkg/controller/spire-server/validation.gopkg/controller/spire-server/validation_test.gopkg/controller/zero-trust-workload-identity-manager/controller.go
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
pkg/controller/spire-server/statefulset_test.go (1)
1096-1134: Also assert the mounts are absent in the negative cases.Lines 1112-1133 only check volumes. If a later change adds a stray
vault-tokenorupstream-camount without the matching volume, these tests still pass even though the Pod spec would be invalid.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/controller/spire-server/statefulset_test.go` around lines 1096 - 1134, The tests TestGenerateStatefulSet_CertManager and TestGenerateStatefulSet_NoUpstreamAuthority currently only assert volumes are absent; also iterate the PodSpec containers (and initContainers if used) from the StatefulSet returned by GenerateSpireServerStatefulSet and assert that no VolumeMounts exist with Name "vault-token" or "upstream-ca"; add these checks in both tests (scan sts.Spec.Template.Spec.Containers and sts.Spec.Template.Spec.InitContainers and fail the test if any mount with those names is found) so mounts and volumes are both verified absent.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@config/crd/bases/operator.openshift.io_spireservers.yaml`:
- Around line 1541-1545: The CRD schema for vaultAddr currently allows http;
update the pattern for the vaultAddr property from ^https?://.+ to require HTTPS
only (e.g., ^https://.+) so the CRD enforces secure transport, and mirror the
same restriction in the validation logic in
pkg/controller/spire-server/validation.go for the vaultAddr validation routine
to keep runtime checks consistent with the CRD.
- Around line 1220-1223: The CEL validation expression for the
x-kubernetes-validations rule that checks bundleEndpointProfile uses a
curly/typographic quote for the empty-string literal, breaking CEL; locate the
rule referencing self.bundleEndpointProfile == ''https_spiffe'' ?
has(self.endpointSpiffeId) && self.endpointSpiffeId != ” : true and replace the
typographic double-quote character with two single quotes (''), ensuring the
expression reads self.bundleEndpointProfile == ''https_spiffe'' ?
has(self.endpointSpiffeId) && self.endpointSpiffeId != '' : true; apply the same
fix in both CRD occurrences where this validation appears (the rule referencing
bundleEndpointProfile and endpointSpiffeId).
---
Nitpick comments:
In `@pkg/controller/spire-server/statefulset_test.go`:
- Around line 1096-1134: The tests TestGenerateStatefulSet_CertManager and
TestGenerateStatefulSet_NoUpstreamAuthority currently only assert volumes are
absent; also iterate the PodSpec containers (and initContainers if used) from
the StatefulSet returned by GenerateSpireServerStatefulSet and assert that no
VolumeMounts exist with Name "vault-token" or "upstream-ca"; add these checks in
both tests (scan sts.Spec.Template.Spec.Containers and
sts.Spec.Template.Spec.InitContainers and fail the test if any mount with those
names is found) so mounts and volumes are both verified absent.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository: openshift/coderabbit/.coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 78b1c609-465c-4afa-869d-7fb68e5de8c0
📒 Files selected for processing (6)
api/v1alpha1/spire_server_config_types.gobundle/manifests/operator.openshift.io_spireservers.yamlbundle/manifests/zero-trust-workload-identity-manager.clusterserviceversion.yamlconfig/crd/bases/operator.openshift.io_spireservers.yamlpkg/controller/spire-server/rbac_test.gopkg/controller/spire-server/statefulset_test.go
🚧 Files skipped from review as they are similar to previous changes (2)
- pkg/controller/spire-server/rbac_test.go
- api/v1alpha1/spire_server_config_types.go
anirudhAgniRedhat
force-pushed
the
upstream-authority-support
branch
from
e617a9e to
a4b0887
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (2)
pkg/controller/spire-server/statefulset.go (1)
272-303: Avoid hardcoded token path components in two places.The token mount path (
/var/run/secrets/tokens) and token filename (vault) are currently duplicated assumptions. Reusing the same canonical token path source used by config generation would reduce drift risk.♻️ Suggested refactor
import ( "context" "fmt" + "path" @@ - expirationSeconds := int64(600) + expirationSeconds := int64(600) @@ ServiceAccountToken: &corev1.ServiceAccountTokenProjection{ Audience: audience, ExpirationSeconds: &expirationSeconds, - Path: "vault", + Path: path.Base(vaultTokenPath), }, @@ corev1.VolumeMount{ Name: "vault-token", - MountPath: "/var/run/secrets/tokens", + MountPath: path.Dir(vaultTokenPath), ReadOnly: true, }, )🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/controller/spire-server/statefulset.go` around lines 272 - 303, Remove the duplicated hardcoded token path and filename by using a single shared source/constant for both the ServiceAccountTokenProjection.Path and the VolumeMount.MountPath; e.g., obtain tokenFileName and tokenMountPath from the same config-generation helper (or declare a package-level constant used by both locations) and replace the literal "vault" and "/var/run/secrets/tokens" in the block that builds the StatefulSet (the Projected ServiceAccountTokenProjection.Path and the corev1.VolumeMount.MountPath for the "vault-token" volume) so both use the same canonical values.pkg/controller/spire-server/configmaps_test.go (1)
1831-1898: Add a Vault-defaults test case for mount-point fallback paths.Current Vault tests set explicit
pkiMountPointandk8sAuthMountPoint, so default branches ("pki"and"kubernetes") stay untested.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/controller/spire-server/configmaps_test.go` around lines 1831 - 1898, Add a new test (based on TestGenerateServerConfMap_WithVaultUpstreamAuthority) that leaves PKIMountPoint and K8sAuth.K8sAuthMountPoint unset on the v1alpha1.UpstreamAuthorityVault and v1alpha1.VaultK8sAuthConfig, call generateServerConfMap, and assert the resulting plugin_data uses the fallback mount names "pki" for pki_mount_point and "kubernetes" for k8s_auth.k8s_auth_mount_point (also verify token_path and other existing defaults remain unchanged); reference TestGenerateServerConfMap_WithVaultUpstreamAuthority, generateServerConfMap, UpstreamAuthorityVault.PKIMountPoint and VaultK8sAuthConfig.K8sAuthMountPoint to locate where to add this new test case.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@pkg/controller/spire-server/configmap.go`:
- Around line 326-329: The code calls
buildUpstreamAuthorityPlugin(config.UpstreamAuthority) without ensuring the
Vault config is non-nil, causing a panic when upstreamAuthority is empty and
CertManager == nil; update the rendering path that handles
config.UpstreamAuthority to defensively check for a non-nil Vault config (or
that CertManager != nil) before treating the authority as Vault, and modify
buildUpstreamAuthorityPlugin (or its caller) to return a safe empty plugin or
skip setting plugins["UpstreamAuthority"] when both CertManager and Vault are
nil; ensure references to CertManager and Vault inside
buildUpstreamAuthorityPlugin are nil-guarded so an empty upstreamAuthority: {}
does not dereference Vault (applies to the callers around the
config.UpstreamAuthority usage and the buildUpstreamAuthorityPlugin function).
In `@pkg/controller/spire-server/rbac_test.go`:
- Around line 2473-2481: The test currently compares rule.Verbs to expectedVerbs
by index, making it order-sensitive; change the assertion in rbac_test.go to be
order-insensitive by treating verbs as sets: build a map[string]bool (or sort
both slices) from rule.Verbs and verify every entry in expectedVerbs exists and
the lengths match, and replace the t.Errorf messages to report
missing/unexpected verbs rather than index mismatches (operate on rule.Verbs and
expectedVerbs variables and update the failing messages sent via t.Errorf).
---
Nitpick comments:
In `@pkg/controller/spire-server/configmaps_test.go`:
- Around line 1831-1898: Add a new test (based on
TestGenerateServerConfMap_WithVaultUpstreamAuthority) that leaves PKIMountPoint
and K8sAuth.K8sAuthMountPoint unset on the v1alpha1.UpstreamAuthorityVault and
v1alpha1.VaultK8sAuthConfig, call generateServerConfMap, and assert the
resulting plugin_data uses the fallback mount names "pki" for pki_mount_point
and "kubernetes" for k8s_auth.k8s_auth_mount_point (also verify token_path and
other existing defaults remain unchanged); reference
TestGenerateServerConfMap_WithVaultUpstreamAuthority, generateServerConfMap,
UpstreamAuthorityVault.PKIMountPoint and VaultK8sAuthConfig.K8sAuthMountPoint to
locate where to add this new test case.
In `@pkg/controller/spire-server/statefulset.go`:
- Around line 272-303: Remove the duplicated hardcoded token path and filename
by using a single shared source/constant for both the
ServiceAccountTokenProjection.Path and the VolumeMount.MountPath; e.g., obtain
tokenFileName and tokenMountPath from the same config-generation helper (or
declare a package-level constant used by both locations) and replace the literal
"vault" and "/var/run/secrets/tokens" in the block that builds the StatefulSet
(the Projected ServiceAccountTokenProjection.Path and the
corev1.VolumeMount.MountPath for the "vault-token" volume) so both use the same
canonical values.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository: openshift/coderabbit/.coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 48427ed6-d1ad-490c-9528-1c9f42cda44e
⛔ Files ignored due to path filters (1)
api/v1alpha1/zz_generated.deepcopy.gois excluded by!**/zz_generated*
📒 Files selected for processing (15)
api/v1alpha1/spire_server_config_types.gobundle/manifests/operator.openshift.io_spireservers.yamlbundle/manifests/zero-trust-workload-identity-manager.clusterserviceversion.yamlconfig/crd/bases/operator.openshift.io_spireservers.yamlconfig/rbac/role.yamlpkg/controller/spire-server/configmap.gopkg/controller/spire-server/configmaps_test.gopkg/controller/spire-server/controller.gopkg/controller/spire-server/rbac.gopkg/controller/spire-server/rbac_test.gopkg/controller/spire-server/statefulset.gopkg/controller/spire-server/statefulset_test.gopkg/controller/spire-server/validation.gopkg/controller/spire-server/validation_test.gopkg/controller/zero-trust-workload-identity-manager/controller.go
✅ Files skipped from review due to trivial changes (4)
- pkg/controller/zero-trust-workload-identity-manager/controller.go
- config/rbac/role.yaml
- pkg/controller/spire-server/statefulset_test.go
- config/crd/bases/operator.openshift.io_spireservers.yaml
Contributor
Author
iamP1
reviewed
May 5, 2026
| - certificaterequests | ||
| verbs: | ||
| - create | ||
| - delete |
Contributor
There was a problem hiding this comment.
do we need delete permission as well?
Contributor
Author
There was a problem hiding this comment.
yes, delete is needed. The SPIRE plugin performs optimistic cleanup of stale CertificateRequests after each CA rotation. Without delete permission, those cleanup calls would fail (the plugin logs the error and continues, so it's non-fatal -- but stale CertificateRequest objects would accumulate in the namespace indefinitely).
The current RBAC (create, get, list, delete) is correct and complete for the cert-manager plugin's needs. No change needed.
| return nil | ||
| } | ||
|
|
||
| func buildCertManagerPluginData(cm *v1alpha1.UpstreamAuthorityCertManager) map[string]interface{} { |
Contributor
There was a problem hiding this comment.
map[string]interface{}, Why map of interface is being used? Isn't the value always going to be string?
Contributor
Author
There was a problem hiding this comment.
It's consistent with every other builder function in the file (buildDataStorePluginData, buildVaultPluginData, generateFederationConfig, etc.) -- they all return map[string]interface{}. Changing just this one would create an inconsistency for no functional benefit.
rausingh-rh
reviewed
May 6, 2026
| Name: "upstream-ca", | ||
| VolumeSource: corev1.VolumeSource{ | ||
| Secret: &corev1.SecretVolumeSource{ | ||
| SecretName: v.CACertSecretRef.Name, |
Contributor
There was a problem hiding this comment.
can we check if v.CACertSecretRef secret exists in validation.go?
Contributor
Author
There was a problem hiding this comment.
We can do it, but we have not done this in other places in the operator controllers.
Also, we need to increase the cache size as we use the custom client based on the cache.
Contributor
There was a problem hiding this comment.
Marking this as resolved as the behavior is consistent across the operator.
Contributor
There was a problem hiding this comment.
If required, it can be tracked as a revamp in follow-up PR: add Secret existence validation across all controllers, along with the cache expansion needed to support it.
Contributor
There was a problem hiding this comment.
In the meanwhile - let's update the ADR in the validation section.
| - get | ||
| - list | ||
| - watch | ||
| - apiGroups: |
Contributor
There was a problem hiding this comment.
The ADR specified namespace-scoped RBAC, is the clusterrole approach intentional?
Contributor
Author
There was a problem hiding this comment.
Yes, intentional. The spire-server SA already has a ClusterRole with cluster-wide permissions (tokenreviews, nodes, pods). Adding CertificateRequest access there follows the same pattern. SPIRE only ever targets the namespace in its config the broader scope doesn't change runtime behavior, just avoids the complexity of managing cross-namespace RBAC lifecycle.
Contributor
There was a problem hiding this comment.
Ack, the ClusterRole approach is simpler and consistent with the existing spire-server SA permission. Can you update the ADR to reflect this design decision? Post that, it can be marked as resolved.
rausingh-rh
reviewed
May 7, 2026
rausingh-rh
reviewed
May 7, 2026
- Extract all paths, plugin names, and defaults into named constants shared between configmap.go and statefulset.go - Use net/url.Parse for Vault URL validation instead of string prefix - Add nil guard in buildUpstreamAuthorityPlugin to prevent panic when both CertManager and Vault are nil - Add security note to vaultAddr field description for HTTP usage Co-authored-by: Cursor <cursoragent@cursor.com>
There was a problem hiding this comment.
🧹 Nitpick comments (1)
pkg/controller/spire-server/statefulset.go (1)
298-305: ⚡ Quick winAvoid positional container indexing for volume mounts.
Using
Containers[0]couples correctness to container order. Resolve thespire-servercontainer by name before appending mounts.Proposed refactor
+func getContainerByName(containers []corev1.Container, name string) *corev1.Container { + for i := range containers { + if containers[i].Name == name { + return &containers[i] + } + } + return nil +} + func addUpstreamAuthorityToStatefulSet(sts *appsv1.StatefulSet, ua *v1alpha1.UpstreamAuthorityConfig) { @@ + spireServerContainer := getContainerByName(sts.Spec.Template.Spec.Containers, "spire-server") + if spireServerContainer == nil { + return + } + if v.K8sAuth != nil { @@ - sts.Spec.Template.Spec.Containers[0].VolumeMounts = append( - sts.Spec.Template.Spec.Containers[0].VolumeMounts, + spireServerContainer.VolumeMounts = append( + spireServerContainer.VolumeMounts, corev1.VolumeMount{ Name: "vault-token", MountPath: vaultTokenMountDir, ReadOnly: true, }, ) } @@ - sts.Spec.Template.Spec.Containers[0].VolumeMounts = append( - sts.Spec.Template.Spec.Containers[0].VolumeMounts, + spireServerContainer.VolumeMounts = append( + spireServerContainer.VolumeMounts, corev1.VolumeMount{ Name: "upstream-ca", MountPath: upstreamCAMountPath, ReadOnly: true, }, ) } }Also applies to: 326-333
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@pkg/controller/spire-server/statefulset.go` around lines 298 - 305, The code appends a VolumeMount using positional indexing into sts.Spec.Template.Spec.Containers[0]; instead, find the container by name ("spire-server") in sts.Spec.Template.Spec.Containers and append the corev1.VolumeMount{Name:"vault-token", MountPath: vaultTokenMountDir, ReadOnly:true} to that container's VolumeMounts slice (modify the container in place or update the slice entry). Apply the same pattern for the other similar block (the second occurrence that currently uses Containers[0], lines around the 326-333 region) so both mounts are added to the spire-server container by name rather than by index.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@pkg/controller/spire-server/statefulset.go`:
- Around line 298-305: The code appends a VolumeMount using positional indexing
into sts.Spec.Template.Spec.Containers[0]; instead, find the container by name
("spire-server") in sts.Spec.Template.Spec.Containers and append the
corev1.VolumeMount{Name:"vault-token", MountPath: vaultTokenMountDir,
ReadOnly:true} to that container's VolumeMounts slice (modify the container in
place or update the slice entry). Apply the same pattern for the other similar
block (the second occurrence that currently uses Containers[0], lines around the
326-333 region) so both mounts are added to the spire-server container by name
rather than by index.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository: openshift/coderabbit/.coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 64254691-57ef-4313-9ca5-980217c7051b
📒 Files selected for processing (4)
pkg/controller/spire-server/configmap.gopkg/controller/spire-server/statefulset.gopkg/controller/spire-server/validation.gopkg/controller/spire-server/validation_test.go
🚧 Files skipped from review as they are similar to previous changes (2)
- pkg/controller/spire-server/validation.go
- pkg/controller/spire-server/validation_test.go
|
@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. |
Contributor
|
/lgtm |
openshift-ci
Bot
added
the
do-not-merge/hold
Contributor
|
/lgtm |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: anirudhAgniRedhat, TrilokGeer 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 |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
5 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Add optional
upstreamAuthorityfield to SpireServer CR, allowing the SPIRE intermediate CA to be signed by an external PKI (cert-manager or Vault) instead of using a self-signed CA.Supported plugins:
k8s_auth), zero user-managed credentialsWhat the operator does on reconcile:
UpstreamAuthorityplugin block in server ConfigMapcert-manager.io/certificaterequestsrule to spire-server ClusterRoleConfigurationValid=Falseon errorupstreamAuthorityis removedNo new controllers, webhooks, or CRDs.
Test plan
Summary by CodeRabbit
New Features
Tests