Add support for spiffe.io/helper-jwt-audience and spiffe.io/helper-jwt-filename annotations#84
Conversation
- Generate jwt_svids as an HCL list without emitting nulls - Set health_checks defaults (bind port + ready/live paths) - Run config writer init container as non-root for restricted pods Co-authored-by: Cursor <cursoragent@cursor.com>
Summary of ChangesHello @lucasmellos, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request significantly enhances the robustness, security, and compatibility of the Highlights
🧠 New Feature in Public Preview: You can now enable Memory to help Gemini Code Assist learn from your team's feedback. This makes future code reviews more consistent and personalized to your project's style. Click here to enable Memory in your admin console. Changelog
Activity
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
lucasmellos
changed the title
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
The pull request significantly enhances the spiffe-helper integration by addressing key configuration generation issues. It correctly generates jwt_svids as an HCL list, preventing null values that could cause parse failures. Explicit health check defaults are now set, improving probe reliability. Crucially, the config-writer init container now runs as non-root, resolving CreateContainerConfigError issues in environments enforcing runAsNonRoot. The changes also refactor the spiffe-helper into a sidecar container and introduce new annotations for JWT configuration, along with a dedicated jwt.go file for parsing these annotations. The test suite has been updated to reflect these changes and ensure correctness.
| jwtObjects := make([]cty.Value, len(params.JWTConfigs)) | ||
| for i, jwtConfig := range params.JWTConfigs { | ||
| objMap := map[string]cty.Value{ | ||
| "jwt_audience": cty.StringVal(jwtConfig.JWTAudience), | ||
| "jwt_svid_file_name": cty.StringVal(jwtConfig.JWTSVIDFilename), | ||
| } | ||
|
|
||
| // Only add jwt_extra_audiences if it has values | ||
| if len(jwtConfig.JWTExtraAudiences) > 0 { | ||
| extraAuds := make([]cty.Value, len(jwtConfig.JWTExtraAudiences)) | ||
| for j, aud := range jwtConfig.JWTExtraAudiences { | ||
| extraAuds[j] = cty.StringVal(aud) | ||
| } | ||
| objMap["jwt_extra_audiences"] = cty.ListVal(extraAuds) | ||
| } | ||
|
|
||
| jwtObjects[i] = cty.ObjectVal(objMap) | ||
| } | ||
|
|
||
| // Set jwt_svids as a list attribute | ||
| body.SetAttributeValue("jwt_svids", cty.ListVal(jwtObjects)) |
There was a problem hiding this comment.
The manual construction of HCL objects using cty.Value and cty.ObjectVal for jwt_svids is effective but verbose and tightly coupled to the SPIFFEHelperJWTConfig struct's fields. If the SPIFFEHelperJWTConfig struct changes (e.g., field names, types), this manual construction logic would need to be updated carefully, increasing maintenance burden. While necessary for the specific HCL output format and avoiding null values, it's a pattern that can become brittle.
There was a problem hiding this comment.
Good callout. We intentionally build jwt_svids via cty because spiffe-helper expects jwt_svids as a list attribute (not blocks) and gohcl was producing either block syntax or null values in some cases.
To reduce brittleness, I refactored the mapping into a single helper (jwtSVIDConfigToCtyValue) so future changes only need updating in one place. We also keep coverage that asserts the generated config contains no null.
| AllowPrivilegeEscalation: ptr.To(false), | ||
| RunAsUser: ptr.To(int64(65532)), | ||
| RunAsGroup: ptr.To(int64(65532)), | ||
| RunAsNonRoot: ptr.To(true), | ||
| Privileged: ptr.To(false), | ||
| Capabilities: &corev1.Capabilities{Drop: []corev1.Capability{"all"}}, |
There was a problem hiding this comment.
The SecurityContext for the init container sets AllowPrivilegeEscalation to ptr.To(false) and Privileged to ptr.To(false). It also drops all capabilities. This is excellent for security, ensuring the init container runs with least privilege. Setting RunAsUser and RunAsGroup to 65532 is a good practice for non-root execution, commonly used for unprivileged users in container images.
There was a problem hiding this comment.
Thanks — the intent here is to keep the config-writer init container as locked down as possible while still being compatible with pods enforcing runAsNonRoot (e.g. cert-manager).
| // Important: keep this non-nil so HCL encoding emits `[]` not `null`. | ||
| JWTExtraAudiences: []string{}, |
There was a problem hiding this comment.
The comment // Important: keep this non-nil so HCL encoding emits [] not null. indicates a potential quirk or limitation in the HCL encoding library where an empty slice might be encoded as null instead of an empty list []. While the explicit initialization JWTExtraAudiences: []string{} correctly addresses this, it's a defensive measure against an unexpected behavior of the HCL encoder. The optional tag in the struct (hcl:"jwt_extra_audiences,optional") should ideally handle this gracefully by omitting the field entirely if empty, or encoding it as an empty list.
There was a problem hiding this comment.
Agreed this is defensive. In the final config generation path we omit jwt_extra_audiences entirely when empty (so we never emit null). We still initialize JWTExtraAudiences to an empty slice in the annotation parser to keep downstream behavior consistent and avoid reintroducing null if we later add additional encoding paths that rely on struct encoding.
- Support `spiffe.io/helper-jwt-svid-file-mode` with default 600 - Propagate mode into generated spiffe-helper config - Fix go.mod go directive format Co-authored-by: Cursor <cursoragent@cursor.com>
Co-authored-by: Cursor <cursoragent@cursor.com>
1 participant
Summary
spiffe.io/helper-jwt-audiencespiffe.io/helper-jwt-filenamespiffe.io/helper-jwt-svid-file-mode(optional; default600)spiffe.io/helper-jwt-extra-audiences(optional)jwt_svidsas an HCL list (and avoid emittingnullvalues) to prevent spiffe-helper config parse failures.health_checksdefaults (bind port + readiness/liveness paths) so probes work reliably with spiffe-helper versions that support the health server.CreateContainerConfigErroron workloads enforcingrunAsNonRoot(e.g. cert-manager).Example
Test plan
go test ./...spec.initContainers[name=inject-spiffe-helper-config].securityContext.runAsUser: 65532health_checksuses port 8081 and/ready+/livejwt_svidsrenders without anynulljwt_svid_file_modedefaults to600(or matches the annotation)