[ENG-715] feat: Update workflows and add SAML SSO support#42
Conversation
- Updated GitHub Actions workflows to use actions/checkout@v5 and other updated action versions. - Bumped chart version to 0.7.0 and added SAML SSO configuration options in values.yaml and documentation. - Enhanced deployment templates to include SAML SSO environment variables and secret mounts. - Updated quickstart guide to include steps for enabling SAML SSO.
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (4)
🚧 Files skipped from review as they are similar to previous changes (3)
📝 WalkthroughWalkthroughAdds SAML SSO support to the Helm chart and documentation, bumps the chart version, and updates GitHub Actions workflows plus CI test dependencies and values for private-image lint/install runs. ChangesSAML SSO Feature
CI Workflow Updates
Estimated code review effort: 4 (Complex) | ~45 minutes 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Warning Tools execution failed with the following error: Failed to run tools: 13 INTERNAL: Received RST_STREAM with code 2 (Internal server error) 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: 1
🧹 Nitpick comments (4)
.github/workflows/lint-test.yaml (1)
79-79: 🔒 Security & Privacy | 🔵 Trivial | 💤 Low valueConsider moving
default_branchinto an env var to avoid direct expression interpolation inrun:.Static analysis flags direct expansion of
github.event.repository.default_branchinto the shell command as a template-injection pattern. Practical risk here is low (requires repo-admin control of branch naming), and the same pattern is used elsewhere in this file, but GitHub's hardening guidance recommends passing untrusted/semi-trusted context values viaenv:instead of inline interpolation.🔧 Optional hardening
- name: Run chart-testing (install) if: steps.list-changed.outputs.changed == 'true' - run: ct install --target-branch ${{ github.event.repository.default_branch }} + env: + TARGET_BRANCH: ${{ github.event.repository.default_branch }} + run: ct install --target-branch "$TARGET_BRANCH"🤖 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 @.github/workflows/lint-test.yaml at line 79, Move the `github.event.repository.default_branch` value out of the `run:` command and into an `env:` variable for the workflow step, then reference that variable in the `ct install` command. Update the step that currently uses `ct install --target-branch` so it reads from the environment instead of direct expression interpolation, and apply the same pattern consistently in this workflow where `default_branch` is used.Source: Linters/SAST tools
charts/currents/templates/_common.tpl (1)
237-263: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winUnguarded empty
issuerstill emits env var.Unlike
providerId,defaultRole, andallowedDomainswhich are wrapped in truthiness checks,SSO_SAML_ISSUER(Lines 241-242) is always rendered whenenabledis true, even ifissueris empty. Combined with the missing required-field validation noted invalues.yaml, this can silently produceSSO_SAML_ISSUER: ""at runtime.🤖 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 `@charts/currents/templates/_common.tpl` around lines 237 - 263, The currents.samlSSOEnv template always renders SSO_SAML_ISSUER whenever currents.sso.saml.enabled is true, even if issuer is empty. Add the same kind of truthiness guard used for providerId, defaultRole, and allowedDomains so the env var is only emitted when issuer is set, and keep the logic inside currents.samlSSOEnv consistent with the other optional SAML fields.docs/eks/quickstart.md (1)
236-250: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winSigned AuthnRequests setup not documented.
The values example only covers
enabled,issuer,providerId,metadataSecretName,defaultRole,allowedDomains.authnRequestsSigned(and the correspondingspCertKey/spKeyKeysecret entries) is a supported option pervalues.yaml/_common.tplbut isn't mentioned here, so a reader following only this quickstart won't discover how to enable request signing.🤖 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 `@docs/eks/quickstart.md` around lines 236 - 250, The SSO values example in the quickstart omits the supported signed AuthnRequests configuration, so readers won’t know how to enable it. Update the `currents.sso.saml` example to mention `authnRequestsSigned` and the required secret entries referenced by `values.yaml` and `_common.tpl` (`spCertKey` and `spKeyKey`), and add a short note explaining where these settings belong in the `currents:` block so the quickstart fully reflects the supported SAML options.charts/currents/values.yaml (1)
75-84: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winNo fail-fast validation for required SAML fields.
enabled,issuer, andmetadataSecretNameare documented as required when SAML is turned on, but nothing enforces this — an operator enabling SAML without settingissuer/metadataSecretNamegets a deployment with an emptySSO_SAML_ISSUERenv var and a volume referencing an empty secret name, likely failing at apply-time with an unclear Kubernetes error rather than a clear Helm template error.Consider using Helm's
requiredfunction in_common.tpl/deployment.yamlwhenenabledis true, consistent with fail-fast patterns elsewhere. This is optional given the rest of the chart follows the same "required in comment, unenforced" convention (e.g.objectStorage.secretName,clickhouse.host).🤖 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 `@charts/currents/values.yaml` around lines 75 - 84, Add fail-fast Helm validation for the SAML settings so enabling SAML without the required fields does not render a broken deployment. Update the SAML handling in deployment rendering (and/or the shared helpers in _common.tpl) to gate on currents.saml.enabled and use required for currents.saml.issuer and currents.saml.metadataSecretName before wiring SSO_SAML_ISSUER and the metadata secret volume. Keep the checks close to the existing SAML env/volume logic so the template fails with a clear Helm error instead of an empty secret reference or empty issuer.
🤖 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.
Inline comments:
In @.github/workflows/lint-test.yaml:
- Around line 12-15: The Checkout step uses actions/checkout without disabling
credential persistence, leaving the GITHUB_TOKEN in git config for the rest of
the job. Update the Checkout configuration to set persist-credentials to false
in the checkout action so the lint-test workflow does not retain credentials
while it runs third-party actions.
---
Nitpick comments:
In @.github/workflows/lint-test.yaml:
- Line 79: Move the `github.event.repository.default_branch` value out of the
`run:` command and into an `env:` variable for the workflow step, then reference
that variable in the `ct install` command. Update the step that currently uses
`ct install --target-branch` so it reads from the environment instead of direct
expression interpolation, and apply the same pattern consistently in this
workflow where `default_branch` is used.
In `@charts/currents/templates/_common.tpl`:
- Around line 237-263: The currents.samlSSOEnv template always renders
SSO_SAML_ISSUER whenever currents.sso.saml.enabled is true, even if issuer is
empty. Add the same kind of truthiness guard used for providerId, defaultRole,
and allowedDomains so the env var is only emitted when issuer is set, and keep
the logic inside currents.samlSSOEnv consistent with the other optional SAML
fields.
In `@charts/currents/values.yaml`:
- Around line 75-84: Add fail-fast Helm validation for the SAML settings so
enabling SAML without the required fields does not render a broken deployment.
Update the SAML handling in deployment rendering (and/or the shared helpers in
_common.tpl) to gate on currents.saml.enabled and use required for
currents.saml.issuer and currents.saml.metadataSecretName before wiring
SSO_SAML_ISSUER and the metadata secret volume. Keep the checks close to the
existing SAML env/volume logic so the template fails with a clear Helm error
instead of an empty secret reference or empty issuer.
In `@docs/eks/quickstart.md`:
- Around line 236-250: The SSO values example in the quickstart omits the
supported signed AuthnRequests configuration, so readers won’t know how to
enable it. Update the `currents.sso.saml` example to mention
`authnRequestsSigned` and the required secret entries referenced by
`values.yaml` and `_common.tpl` (`spCertKey` and `spKeyKey`), and add a short
note explaining where these settings belong in the `currents:` block so the
quickstart fully reflects the supported SAML options.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 891a52de-cbe2-423d-8d16-55b32296dd9e
📒 Files selected for processing (12)
.github/workflows/docs-check.yaml.github/workflows/lint-test.yaml.github/workflows/release.yaml.github/workflows/sync-docs.yamlcharts/currents/Chart.yamlcharts/currents/templates/_common.tplcharts/currents/templates/server/deployment.yamlcharts/currents/values.yamldocs/configuration.mddocs/eks/quickstart.mdsamples/eks/eks-config.yamlsamples/local/chart-config.yaml
- Introduced a new dependencies.yaml file to deploy ephemeral MongoDB and ClickHouse instances for chart testing. - Updated lint-test.yaml to create the `currents-ci` namespace and apply the new dependencies, ensuring the app pods can reach the database services. - Enhanced lint-values.yaml to reference the new in-cluster MongoDB and ClickHouse configurations and secrets.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (5)
.github/workflows/lint-test.yaml (4)
72-73: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick winUnbounded parallel
docker pull(-P 0).Pulling every private image at once can saturate runner bandwidth/disk and risks ECR pull throttling, especially as the image list grows.
♻️ Proposed fix
- echo "$images" | xargs -P 0 -n 1 docker pull + echo "$images" | xargs -P 4 -n 1 docker pull🤖 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 @.github/workflows/lint-test.yaml around lines 72 - 73, The concurrent image pull in the workflow is unbounded because xargs uses -P 0, which can overwhelm runner resources and trigger registry throttling. Update the docker pull step in the lint-test workflow to use a fixed, modest parallelism limit instead of unlimited concurrency, while keeping the existing xargs-based batching logic intact.
68-69: 🚀 Performance & Scalability | 🔵 TrivialFragile
awk-based image extraction.Matching on
$1 == "image:"is sensitive to rendered YAML formatting (indentation, quoting, multi-line values) and could silently miss or misparse images if the chart templates change. Consideryqfor a more robust extraction (e.g.,yq eval '.. | .image? // empty' -).🤖 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 @.github/workflows/lint-test.yaml around lines 68 - 69, The image extraction in the lint workflow is too dependent on rendered YAML formatting because the current `awk` pipeline only matches `$1 == "image:"`. Update the `helm template` pipeline in this workflow to use a more robust YAML-aware extractor such as `yq` when collecting image fields, and keep the existing filtering/sorting logic afterward so the `images` check still works reliably if template formatting changes.
51-56: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winECR account ID duplicated across two lines.
"513558712013"appears here and again as thegrepfilter at Line 69. If the account ever changes, one occurrence could be missed, silently breaking the private-image preload logic.♻️ Proposed fix
+ env: + CURRENTS_ECR_ACCOUNT: "513558712013" steps: ... - name: Login to Currents ECR if: steps.list-changed.outputs.changed == 'true' uses: aws-actions/amazon-ecr-login@v2 with: - registries: "513558712013" + registries: ${{ env.CURRENTS_ECR_ACCOUNT }} ... - name: Preload Currents images into kind run: | images=$(helm template ct-release charts/currents -f charts/currents/ci/lint-values.yaml \ - | awk '$1 == "image:" { print $2 }' | tr -d '"' | grep '513558712013' | sort -u) + | awk '$1 == "image:" { print $2 }' | tr -d '"' | grep "$CURRENTS_ECR_ACCOUNT" | sort -u)🤖 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 @.github/workflows/lint-test.yaml around lines 51 - 56, The Currents ECR account ID is duplicated in the workflow, so update the login step and the later private-image preload check in the same place to use a single shared source of truth. Refactor the duplicated `"513558712013"` value in the workflow around the `Login to Currents ECR` step and the `grep`-based check so both references stay in sync if the account changes.
117-117: 🔒 Security & Privacy | 🔵 Trivial | ⚡ Quick winStatic analysis flags template-injection on
--target-branchinterpolation.
${{ github.event.repository.default_branch }}is expanded directly into therun:script. Practically low-risk sincedefault_branchis repo-owner controlled, but the fix is cheap and matches GitHub's hardening guidance (this pattern also exists unchanged at Lines 35/42).🔒 Proposed fix
- name: Run chart-testing (install) if: steps.list-changed.outputs.changed == 'true' - run: ct install --target-branch ${{ github.event.repository.default_branch }} --namespace currents-ci --helm-extra-args "--timeout 600s" + env: + TARGET_BRANCH: ${{ github.event.repository.default_branch }} + run: ct install --target-branch "$TARGET_BRANCH" --namespace currents-ci --helm-extra-args "--timeout 600s"🤖 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 @.github/workflows/lint-test.yaml at line 117, Static analysis is flagging direct GitHub Actions expression interpolation in the `ct install` command for `--target-branch`, so update the workflow to avoid embedding `${{ github.event.repository.default_branch }}` directly in the shell script. Move the value into an environment variable in the same job/step and reference that variable in the `run` command, and apply the same hardening pattern to the matching `ct install` invocations elsewhere in this workflow.Source: Linters/SAST tools
.github/ci/dependencies.yaml (1)
21-31: 🚀 Performance & Scalability | 🔵 TrivialConsider adding resource requests/limits for CI stability.
Neither container declares resource requests/limits. On resource-constrained kind runners, this risks noisy-neighbor effects with the app pods being tested, occasionally causing readiness timeouts. Not addressing the Trivy/Checkov securityContext findings here, since this is explicitly documented as an ephemeral CI-only dependency.
Also applies to: 60-80
🤖 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 @.github/ci/dependencies.yaml around lines 21 - 31, Add resource requests and limits to the CI-only MongoDB container in the dependencies manifest so it behaves predictably on constrained kind runners. Update the mongodb container definition alongside the existing readinessProbe and the other matching container block(s) referenced by the comment so both declare explicit CPU/memory requests and limits instead of relying on defaults.
🤖 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.
Inline comments:
In @.github/workflows/lint-test.yaml:
- Around line 88-97: The replica set readiness check in the workflow does not
fail when PRIMARY election never happens, so the job continues into later steps
with a misleading error. Update the retry loop around the mongodb `mongosh`
`db.hello().isWritablePrimary` check to detect the exhausted 30 attempts and
exit the job immediately with a clear failure message before secret creation and
`ct install`. Use the existing replica-set init/wait block in the workflow to
locate the change.
---
Nitpick comments:
In @.github/ci/dependencies.yaml:
- Around line 21-31: Add resource requests and limits to the CI-only MongoDB
container in the dependencies manifest so it behaves predictably on constrained
kind runners. Update the mongodb container definition alongside the existing
readinessProbe and the other matching container block(s) referenced by the
comment so both declare explicit CPU/memory requests and limits instead of
relying on defaults.
In @.github/workflows/lint-test.yaml:
- Around line 72-73: The concurrent image pull in the workflow is unbounded
because xargs uses -P 0, which can overwhelm runner resources and trigger
registry throttling. Update the docker pull step in the lint-test workflow to
use a fixed, modest parallelism limit instead of unlimited concurrency, while
keeping the existing xargs-based batching logic intact.
- Around line 68-69: The image extraction in the lint workflow is too dependent
on rendered YAML formatting because the current `awk` pipeline only matches `$1
== "image:"`. Update the `helm template` pipeline in this workflow to use a more
robust YAML-aware extractor such as `yq` when collecting image fields, and keep
the existing filtering/sorting logic afterward so the `images` check still works
reliably if template formatting changes.
- Around line 51-56: The Currents ECR account ID is duplicated in the workflow,
so update the login step and the later private-image preload check in the same
place to use a single shared source of truth. Refactor the duplicated
`"513558712013"` value in the workflow around the `Login to Currents ECR` step
and the `grep`-based check so both references stay in sync if the account
changes.
- Line 117: Static analysis is flagging direct GitHub Actions expression
interpolation in the `ct install` command for `--target-branch`, so update the
workflow to avoid embedding `${{ github.event.repository.default_branch }}`
directly in the shell script. Move the value into an environment variable in the
same job/step and reference that variable in the `run` command, and apply the
same hardening pattern to the matching `ct install` invocations elsewhere in
this workflow.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: e991d630-1222-4111-9db1-31356634a459
📒 Files selected for processing (3)
.github/ci/dependencies.yaml.github/workflows/lint-test.yamlcharts/currents/ci/lint-values.yaml
- Introduced Redis deployment and service in dependencies.yaml - Updated lint-test.yaml to include rollout status check - Modified lint-values.yaml to reference the new Redis configuration
There was a problem hiding this comment.
🧹 Nitpick comments (1)
.github/ci/dependencies.yaml (1)
20-32: 🔒 Security & Privacy | 🔵 Trivial | 💤 Low valueMissing pod/container securityContext (CI-only, low risk).
Static analysis flags
mongodbandrediscontainers for running with default security context (root, writable rootfs, noallowPrivilegeEscalation: false). Since this manifest is explicitly CI-only ephemeral test infra (per the file's own comment), the risk is minimal, but adding a baselinesecurityContextis cheap and keeps the CI cluster consistent with production hardening practices.🔒 Optional hardening
containers: - name: mongodb image: mongo:8 + securityContext: + allowPrivilegeEscalation: false + readOnlyRootFilesystem: false + runAsNonRoot: false(similarly for the
rediscontainer)Also applies to: 101-122
🤖 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 @.github/ci/dependencies.yaml around lines 20 - 32, Add a baseline securityContext to the CI test containers so `mongodb` (and the matching `redis` container) do not run with default root-oriented settings. Update the container specs in the `spec.containers` section to set the common hardening flags (for example, disable privilege escalation and use a non-writable root filesystem where compatible), keeping the existing `readinessProbe` and args unchanged.Source: Linters/SAST tools
🤖 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 @.github/ci/dependencies.yaml:
- Around line 20-32: Add a baseline securityContext to the CI test containers so
`mongodb` (and the matching `redis` container) do not run with default
root-oriented settings. Update the container specs in the `spec.containers`
section to set the common hardening flags (for example, disable privilege
escalation and use a non-writable root filesystem where compatible), keeping the
existing `readinessProbe` and args unchanged.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 035d2e90-7db8-49ce-aa95-a85eeff8c0a2
📒 Files selected for processing (3)
.github/ci/dependencies.yaml.github/workflows/lint-test.yamlcharts/currents/ci/lint-values.yaml
🚧 Files skipped from review as they are similar to previous changes (2)
- charts/currents/ci/lint-values.yaml
- .github/workflows/lint-test.yaml
- Removed the ECR login step and preloading of images into the kind cluster from the GitHub Actions workflow. - Added a new secret for ECR credentials to allow kind to pull private images directly. - Updated lint-values.yaml to include the new imagePullSecrets configuration. - Improved error handling for MongoDB replica set initialization in the CI workflow. - Ensured required fields for SAML configuration in deployment templates are validated.
twk3
changed the title
1 participant
Summary by CodeRabbit
0.7.0and updated CI/release documentation workflow tooling (checkout to v5, plus related action updates).