Skip to content

Conversation

@psokolinski
Copy link
Contributor

@psokolinski psokolinski commented Oct 8, 2025

This PR adds an option to use the existing secret instead of creating a new one to store the client ID and secret.
Tested and it is working.

Summary by CodeRabbit

  • New Features

    • Support using an existing Kubernetes Secret for authentication (alternative to direct clientId/clientSecret).
    • Add optional nodeSelector and tolerations for pod scheduling control.
    • Add apiUrl configuration option.
    • Introduce pre-deploy configuration validation with clear error messages and examples.
  • Bug Fixes

    • Ensure credentials are encoded/handled correctly when creating a Secret.
  • Documentation

    • Update guidance and configuration tables to reflect authentication options, apiUrl, and scheduling fields.

@psokolinski psokolinski self-assigned this Oct 8, 2025
@coderabbitai
Copy link

coderabbitai bot commented Oct 8, 2025

Walkthrough

Adds optional secret-based auth via jit.existingSecret, introduces a validation template enforcing auth and cluster.name rules, makes Secret rendering conditional, computes the credentials secret name, supports nodeSelector and tolerations in pod spec, and updates README, values.yaml, and values.schema.json.

Changes

Cohort / File(s) Summary
Documentation updates
charts/jit-k8s-agent/README.md
Documented jit.existingSecret, nodeSelector, tolerations; updated auth guidance to allow either clientId/clientSecret or existingSecret.
Template logic and control flow
charts/jit-k8s-agent/templates/_job_helper.tpl, charts/jit-k8s-agent/templates/_validation.tpl, charts/jit-k8s-agent/templates/secret.yaml
Added jit-k8s-agent.validateValues template to validate cluster.name and auth options; introduced computed $jitCredentialsSecret (prefers jit.existingSecret); secret creation wrapped conditionally (skip if existingSecret set); replaced hard-coded secretRef with computed name; added conditional nodeSelector and tolerations; use `toString
Schema and defaults
charts/jit-k8s-agent/values.schema.json, charts/jit-k8s-agent/values.yaml
Added jit.existingSecret and jit.apiUrl to schema; removed strict required clientId/clientSecret pair and updated descriptions to reflect optional direct credentials vs existing secret; values.yaml adds existingSecret, nodeSelector, and tolerations entries.

Sequence Diagram(s)

sequenceDiagram
    autonumber
    actor User
    participant Helm as Helm (install/upgrade)
    participant Chart as jit-k8s-agent Chart
    participant Val as validateValues tpl
    participant K8s as Kubernetes API

    User->>Helm: helm upgrade --install ...
    Helm->>Chart: Render templates
    Chart->>Val: include "jit-k8s-agent.validateValues"
    alt Validation fails
        Val-->>Helm: fail with aggregated errors
        Helm-->>User: Render error (missing cluster.name / auth)
    else Validation passes
        Note over Chart: Determine secret name\n$jitCredentialsSecret
        alt .Values.jit.existingSecret set
            Chart-->>K8s: Skip creating Secret
        else No existingSecret
            Chart->>K8s: Create Secret with clientId/clientSecret
        end
        Chart->>K8s: Create Job/Pod spec\n- secretRef: $jitCredentialsSecret\n- optional nodeSelector\n- optional tolerations
        K8s-->>User: Resources applied
    end
Loading

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~20 minutes

Suggested reviewers

  • LironJit

Poem

I hop through charts with tidy cheer,
Secrets bloom or disappear—no fear!
Nodes get chosen, taints forgivable,
Validation makes the flow predictable.
I nibble bugs and then commit—🐇✨

Pre-merge checks and finishing touches

✅ Passed checks (3 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title Check ✅ Passed The title concisely describes the primary change—adding support for using an existing Kubernetes secret for credentials—and is directly aligned with the pull request’s main objective. Although the phrasing is slightly awkward, it accurately reflects the core update without introducing unrelated details.
Docstring Coverage ✅ Passed No functions found in the changes. Docstring coverage check skipped.
✨ Finishing touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Post copyable unit tests in a comment
  • Commit unit tests in branch SC-31363-v2

📜 Recent review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 5e07564 and 43e6e4e.

📒 Files selected for processing (2)
  • charts/jit-k8s-agent/templates/_validation.tpl (1 hunks)
  • charts/jit-k8s-agent/templates/secret.yaml (2 hunks)
🧰 Additional context used
🪛 YAMLlint (1.37.1)
charts/jit-k8s-agent/templates/secret.yaml

[error] 1-1: syntax error: expected the node content, but found '-'

(syntax)

⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
  • GitHub Check: Jit Security
🔇 Additional comments (2)
charts/jit-k8s-agent/templates/secret.yaml (1)

3-8: Credential guard ensures empty values fail fast. Blocking secret creation when either credential is missing aligns with the shared validation logic and prevents rendering invalid manifests. Nicely done.

charts/jit-k8s-agent/templates/_validation.tpl (1)

16-27: Validation covers partial credential configs. The new branches for single-field credentials close the earlier gap and give operators clear guidance. Thanks for tightening this up.


Comment @coderabbitai help to get the list of available commands and usage tips.

Copy link

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

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

Actionable comments posted: 2

📜 Review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between f158b13 and 5e07564.

📒 Files selected for processing (6)
  • charts/jit-k8s-agent/README.md (1 hunks)
  • charts/jit-k8s-agent/templates/_job_helper.tpl (2 hunks)
  • charts/jit-k8s-agent/templates/_validation.tpl (1 hunks)
  • charts/jit-k8s-agent/templates/secret.yaml (2 hunks)
  • charts/jit-k8s-agent/values.schema.json (1 hunks)
  • charts/jit-k8s-agent/values.yaml (2 hunks)
🧰 Additional context used
🪛 YAMLlint (1.37.1)
charts/jit-k8s-agent/templates/secret.yaml

[error] 1-1: syntax error: expected the node content, but found '-'

(syntax)

⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
  • GitHub Check: Iac Misconfig Detection Kubernetes
  • GitHub Check: Jit Security
🔇 Additional comments (19)
charts/jit-k8s-agent/templates/secret.yaml (2)

1-1: LGTM!

Including validation at the start of the template ensures configuration issues are caught early, preventing invalid deployments.


2-14: LGTM!

The conditional rendering correctly prevents secret creation when existingSecret is provided, avoiding conflicts with user-managed secrets.

charts/jit-k8s-agent/README.md (3)

41-41: LGTM!

The parameter documentation clearly describes the existingSecret option and properly cross-references the authentication note.


49-50: LGTM!

The documentation for nodeSelector and tolerations is clear and follows the existing table format consistently.


54-54: LGTM!

The updated authentication note clearly explains both credential options (direct credentials or existing secret) and properly links to external documentation.

charts/jit-k8s-agent/templates/_job_helper.tpl (4)

2-2: LGTM!

The computed secret name correctly prioritizes existingSecret and falls back to the chart-generated secret name, ensuring consistency with the secret creation logic.


6-9: LGTM!

The conditional tolerations block is correctly structured with proper indentation for the pod spec level.


10-13: LGTM!

The conditional nodeSelector block follows the same pattern as tolerations with correct indentation and formatting.


31-31: LGTM!

Using the computed $jitCredentialsSecret variable for both credential references ensures consistency and enables the existing-secret feature.

Also applies to: 36-36

charts/jit-k8s-agent/values.yaml (2)

10-15: LGTM!

The comments clearly distinguish the two authentication options and provide helpful inline guidance for users. The default values are appropriate.


31-35: LGTM!

The nodeSelector and tolerations fields are properly documented and have appropriate default values (empty map and array respectively).

charts/jit-k8s-agent/templates/_validation.tpl (4)

1-5: LGTM!

The template structure correctly initializes an error list for collecting validation failures.


7-9: LGTM!

The cluster.name validation correctly ensures this required field is provided.


25-38: LGTM!

The error message construction provides clear, actionable guidance with examples for both authentication methods, improving user experience during misconfiguration.


40-40: LGTM!

Template closure is correct.

charts/jit-k8s-agent/values.schema.json (4)

19-26: LGTM!

The updated descriptions correctly reflect that clientId and clientSecret are conditionally required based on whether existingSecret is used.


27-34: LGTM!

The new properties are properly typed and documented. The existingSecret description correctly notes that it takes precedence over direct credentials.


36-36: LGTM!

The jit object description clearly explains the two authentication options, matching the validation logic and documentation.


19-37: Approve JSON schema and template validation approach
Helm validates schemas before rendering; removing clientId/clientSecret from required lets the template-based checks enforce the either/or constraint correctly.

@psokolinski psokolinski merged commit e178e2d into main Oct 8, 2025
6 checks passed
@psokolinski psokolinski deleted the SC-31363-v2 branch October 8, 2025 10:37
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants