OSAC-1083: OSAC-1397: Add schema validation and fix subchart version pinning - #253
Conversation
|
@eliorerz: This pull request references OSAC-1083 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 task 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. |
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository: osac-project/coderabbit/.coderabbit.yaml Review profile: ASSERTIVE Plan: Enterprise Run ID: 📒 Files selected for processing (3)
WalkthroughThis PR enhances the OSAC Helm chart with flexible dependency versioning and strict service configuration schema validation. Dependency version constraints transition from fixed "0.0.0" to ranges, while new required service configuration fields enforce certificate issuer and authentication setup with structured validation patterns. ChangesHelm Chart Configuration and Schema Updates
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Possibly related PRs
Suggested labels
Suggested reviewers
🚥 Pre-merge checks | ✅ 10 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (10 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
d785caf to
f1fe4b4
Compare
f1fe4b4 to
045af7a
Compare
|
/lgtm |
|
/retest |
045af7a to
f678f7a
Compare
f678f7a to
270d404
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 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/publish-charts.yaml:
- Around line 52-56: After extracting CRDS_VER, OPERATOR_VER, SERVICE_VER and
AAP_VER, validate each value against a strict semver regex (e.g.
^[0-9]+\.[0-9]+\.[0-9]+(-[A-Za-z0-9.-]+)?$) and abort the workflow with a clear
error if any fail; alternatively replace the grep/awk extraction with yq-based
reads and then validate the results. Also change the subsequent yq usage that
interpolates these variables (the yq command around lines 61-70) to pass the
versions safely (e.g. via yq --arg or other safe parameter passing) rather than
directly expanding untrusted double-quoted variables, so malformed or malicious
version strings cannot break the command or inject shell code.
In `@charts/osac/Chart.yaml`:
- Line 9: The Chart.yaml currently sets the root chart and subcharts to an
overly permissive version range ">=0.0.0"; change the version constraints to a
tighter range (for example ">=0.0.0 <1.0.0") or add an explicit pinned version
for each affected chart entry instead of the unbounded ">=0.0.0" token, or
alternatively implement a CI validation that checks submodule Chart.yaml
versions against an allowlist; locate the version field that currently equals
">=0.0.0" in Chart.yaml (and the other occurrences noted) and update them to the
chosen bounded constraint or add the CI/check as appropriate.
In `@charts/osac/values.schema.json`:
- Around line 156-161: The JSON schema for issuerUrl currently allows http by
using the pattern "^https?://"; update the issuerUrl property in
values.schema.json to require HTTPS only (e.g., change the pattern to
"^https://") and ensure its description reflects HTTPS requirement; optionally
add a separate boolean flag (e.g., auth.allowInsecure with default false and
documentation/warnings) if you need to explicitly permit insecure HTTP for local
testing so production remains locked to HTTPS.
🪄 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: osac-project/coderabbit/.coderabbit.yaml
Review profile: ASSERTIVE
Plan: Enterprise
Run ID: 2cb089fb-675c-494b-b2f7-3b26c95c6b2d
📒 Files selected for processing (13)
.github/workflows/publish-charts.yamlbase/kustomization.yamlbase/osac-aapbase/osac-fulfillment-servicebase/osac-operatorcharts/osac/Chart.yamlcharts/osac/values.schema.jsoncharts/osac/values.yamloverlays/caas-ci/kustomization.yamloverlays/osac-integration/kustomization.yamloverlays/vmaas-ci/kustomization.yamlvalues/caas-ci.yamlvalues/vmaas-ci.yaml
| dependencies: | ||
| - name: osac-operator-crds | ||
| version: "0.0.0" | ||
| version: ">=0.0.0" |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial | ⚖️ Poor tradeoff
Supply chain: Extremely permissive version constraint ">=0.0.0" accepts any subchart version.
The range constraint ">=0.0.0" removes all upper bounds, allowing helm dependency build to accept any version present in the submodule directories. While this prevents breakage when submodules bump their chart versions (per issue #248), it also means local builds have no version validation.
Risk impact: If a submodule's Chart.yaml version is unexpectedly modified (accidentally or maliciously), the build will silently succeed with the new version. The published chart workflow mitigates this by pinning exact versions (lines 61-70 of publish-charts.yaml), but local development and CI validation builds remain unconstrained.
Trade-off consideration: The current approach prioritizes build reliability over strict version control. If tighter supply chain guarantees are needed, consider either (a) adding version upper bounds (e.g., ">=0.0.0 <1.0.0"), or (b) adding a CI check that validates submodule chart versions against an allowlist.
Also applies to: 13-13, 17-17, 21-21
🤖 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/osac/Chart.yaml` at line 9, The Chart.yaml currently sets the root
chart and subcharts to an overly permissive version range ">=0.0.0"; change the
version constraints to a tighter range (for example ">=0.0.0 <1.0.0") or add an
explicit pinned version for each affected chart entry instead of the unbounded
">=0.0.0" token, or alternatively implement a CI validation that checks
submodule Chart.yaml versions against an allowlist; locate the version field
that currently equals ">=0.0.0" in Chart.yaml (and the other occurrences noted)
and update them to the chosen bounded constraint or add the CI/check as
appropriate.
| "issuerUrl": { | ||
| "type": "string", | ||
| "description": "OIDC issuer URL (e.g. Keycloak realm URL)", | ||
| "minLength": 1, | ||
| "pattern": "^https?://" | ||
| }, |
There was a problem hiding this comment.
Security risk: Pattern permits insecure HTTP for OIDC issuer URLs.
The regex pattern ^https?:// accepts both http:// and https:// schemes for the OIDC issuer URL. In production environments, authentication issuer URLs must use HTTPS to prevent credential interception and token leakage. Allowing http:// degrades the security posture and creates a path for misconfiguration that could expose authentication tokens in transit.
Impact: Administrators could inadvertently configure an HTTP issuer URL, exposing OIDC tokens and user credentials to network eavesdropping. This violates security best practices for identity providers.
Severity: Major – security posture gap that permits insecure configuration.
🔒 Recommended fix: Restrict pattern to HTTPS only
"issuerUrl": {
"type": "string",
"description": "OIDC issuer URL (e.g. Keycloak realm URL)",
"minLength": 1,
- "pattern": "^https?://"
+ "pattern": "^https://"
},For local development or testing scenarios that genuinely require HTTP (e.g., kind clusters), consider adding a separate boolean flag like auth.allowInsecure: false (default false) that must be explicitly enabled, with clear warnings in documentation.
🤖 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/osac/values.schema.json` around lines 156 - 161, The JSON schema for
issuerUrl currently allows http by using the pattern "^https?://"; update the
issuerUrl property in values.schema.json to require HTTPS only (e.g., change the
pattern to "^https://") and ensure its description reflects HTTPS requirement;
optionally add a separate boolean flag (e.g., auth.allowInsecure with default
false and documentation/warnings) if you need to explicitly permit insecure HTTP
for local testing so production remains locked to HTTPS.
|
/lgtm |
|
@eliorerz: The following test failed, say
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. |
|
rebase |
…pinning - Chart.yaml: Use >=0.0.0 for subchart dependency versions so builds don't break when subcharts bump their version from 0.0.0. - values.schema.json: Add required field validation for service.auth (issuerUrl with URL pattern) and service.certs (issuerRef.name), plus database connection schema. - values.yaml: Change issuerUrl default from empty string to https://REPLACE_ME so schema validation passes and the placeholder is obvious.
270d404 to
f38551f
Compare
|
/lgtm |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: eliorerz, omer-vishlitzky 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 |
3 participants
Summary
">=0.0.0"inChart.yamlforfile://dependencies so submodule bumps don't breakhelm dependency buildwhen subcharts change their version (fixes bump submodules #248)Chart.yamlusingyqinstead of assuming a single version for allvalues.schema.jsonforservice.auth.issuerUrl(URL pattern) andservice.certs.issuerRef.nameissuerUrldefault tohttps://REPLACE_MEplaceholder sohelm lint/ct lintpasses with schema enforcementSummary by CodeRabbit