MGMT-23923: Create overlay for integration environment - #86
Conversation
|
@adriengentil: This pull request references MGMT-23923 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. |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: adriengentil 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 |
|
Warning Rate limit exceeded
To continue reviewing without waiting, purchase usage credits in the billing tab. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (8)
WalkthroughAdds an Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested labels
Suggested reviewers
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
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. 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 |
![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 the current code and only fix it if needed.
Inline comments:
In `@overlays/osac-integration/kustomization.yaml`:
- Around line 133-148: The manifest currently sets OSAC_AAP_URL to use http and
injects OSAC_AAP_TOKEN while enabling OSAC_AAP_INSECURE_SKIP_VERIFY, which
weakens credential transport; update the environment entries so OSAC_AAP_URL
uses https, remove or set OSAC_AAP_INSECURE_SKIP_VERIFY to "false" (or omit it
entirely), and ensure OSAC_AAP_TOKEN continues to be supplied via secretKeyRef
(osac-aap-api-token) so TLS is enforced for AAP API calls.
- Around line 138-143: Remove the optional flag on the secret reference for
OSAC_AAP_TOKEN so the controller fails fast when credentials are missing: edit
the secretKeyRef block for name: osac-aap-api-token / key: token (the
OSAC_AAP_TOKEN env var) and either delete the optional: true line or set it to
false to ensure the secret is required when OSAC_PROVISIONING_PROVIDER=aap.
In `@overlays/osac-integration/prefixTransformer.yaml`:
- Around line 6-10: Current PrefixSuffixTransformer updates ClusterRole and
ClusterRoleBinding metadata/name but lacks a nameReference mapping for
ClusterRole→ClusterRoleBinding, so roleRef.name remains unprefixed; update
base/nameReference.yaml to add an entry mapping kind: ClusterRole with a
fieldSpecs entry pointing to ClusterRoleBinding path roleRef/name so Kustomize
will rewrite roleRef.name to match the prefixed ClusterRole metadata/name, or
alternatively remove the custom PrefixSuffixTransformer and use top-level
namePrefix in kustomization.yaml to leverage built-in nameReference handling.
🪄 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: acdaaf56-2fd6-46db-9b62-2532d90243ae
📒 Files selected for processing (6)
overlays/osac-integration/.buildfilesoverlays/osac-integration/ca-trust-bundle.yamloverlays/osac-integration/files/osac-aap-configuration.envoverlays/osac-integration/files/osac-aap-secrets.env.exampleoverlays/osac-integration/kustomization.yamloverlays/osac-integration/prefixTransformer.yaml
| name: OSAC_AAP_URL | ||
| value: "http://osac-aap/api/controller" | ||
| - op: add | ||
| path: /spec/template/spec/containers/0/env/- | ||
| value: | ||
| name: OSAC_AAP_TOKEN | ||
| valueFrom: | ||
| secretKeyRef: | ||
| name: osac-aap-api-token | ||
| key: token | ||
| optional: true | ||
| - op: add | ||
| path: /spec/template/spec/containers/0/env/- | ||
| value: | ||
| name: OSAC_AAP_INSECURE_SKIP_VERIFY | ||
| value: "true" |
There was a problem hiding this comment.
Use TLS for AAP API calls when sending credentials.
Line 134 uses http://... while Line 138 injects a token and Line 147 sets insecure verification. This weakens credential transport security and should be tightened for integration too.
Suggested hardening
- op: add
path: /spec/template/spec/containers/0/env/-
value:
name: OSAC_AAP_URL
- value: "http://osac-aap/api/controller"
+ value: "https://osac-aap/api/controller"
@@
- op: add
path: /spec/template/spec/containers/0/env/-
value:
name: OSAC_AAP_INSECURE_SKIP_VERIFY
- value: "true"
+ value: "false"🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@overlays/osac-integration/kustomization.yaml` around lines 133 - 148, The
manifest currently sets OSAC_AAP_URL to use http and injects OSAC_AAP_TOKEN
while enabling OSAC_AAP_INSECURE_SKIP_VERIFY, which weakens credential
transport; update the environment entries so OSAC_AAP_URL uses https, remove or
set OSAC_AAP_INSECURE_SKIP_VERIFY to "false" (or omit it entirely), and ensure
OSAC_AAP_TOKEN continues to be supplied via secretKeyRef (osac-aap-api-token) so
TLS is enforced for AAP API calls.
598f7c3 to
899483f
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@scripts/prepare-fulfillment-service.sh`:
- Line 19: The FULFILLMENT_API_URL assignment uses an unquoted
${INSTALLER_NAMESPACE} in the command substitution which can cause
word-splitting/globbing; update the command in the FULFILLMENT_API_URL line so
the INSTALLER_NAMESPACE variable is quoted inside the oc get route invocation
(i.e., reference ${INSTALLER_NAMESPACE} as a quoted parameter) to ensure safe
namespace handling when calling oc get route -n in that assignment.
🪄 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: 7414ca6b-de70-4706-87bf-a420ac521e21
📒 Files selected for processing (15)
base/kustomization.yamlbase/osac-aapbase/osac-fulfillment-servicebase/osac-operatoroverlays/caas-ci/kustomization.yamloverlays/development/kustomization.yamloverlays/hypershift2/kustomization.yamloverlays/osac-integration/.buildfilesoverlays/osac-integration/ca-trust-bundle.yamloverlays/osac-integration/files/osac-aap-configuration.envoverlays/osac-integration/files/osac-aap-secrets.env.exampleoverlays/osac-integration/kustomization.yamloverlays/osac-integration/prefixTransformer.yamloverlays/vmaas-ci/kustomization.yamlscripts/prepare-fulfillment-service.sh
✅ Files skipped from review due to trivial changes (5)
- base/osac-aap
- overlays/osac-integration/files/osac-aap-secrets.env.example
- overlays/osac-integration/files/osac-aap-configuration.env
- overlays/hypershift2/kustomization.yaml
- overlays/osac-integration/.buildfiles
🚧 Files skipped from review as they are similar to previous changes (3)
- overlays/osac-integration/prefixTransformer.yaml
- overlays/osac-integration/ca-trust-bundle.yaml
- overlays/osac-integration/kustomization.yaml
There was a problem hiding this comment.
♻️ Duplicate comments (2)
overlays/osac-integration/kustomization.yaml (2)
181-186:⚠️ Potential issue | 🟠 Major | ⚡ Quick winMake the AAP token secret mandatory when provider is
aap.With
OSAC_PROVISIONING_PROVIDER=aap, keeping Line [186] asoptional: trueallows startup without required credentials and shifts failure to runtime.Suggested fix
- op: add path: /spec/template/spec/containers/0/env/- value: name: OSAC_AAP_TOKEN valueFrom: secretKeyRef: name: osac-aap-api-token key: token - optional: true🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@overlays/osac-integration/kustomization.yaml` around lines 181 - 186, The OSAC_AAP_TOKEN secret reference is currently marked optional which allows the pod to start without the AAP credentials; update the secretKeyRef for name: osac-aap-api-token key: token (the OSAC_AAP_TOKEN env var) to make the secret mandatory by changing optional: true to optional: false (ensure OSAC_PROVISIONING_PROVIDER=aap expects this required secret).
176-177:⚠️ Potential issue | 🟠 Major | ⚡ Quick winEnforce TLS for AAP API calls carrying credentials.
Line [177] still uses
http://while Line [191] sets insecure verification to"true", which weakens token transport security for AAP integration.Suggested hardening patch
- op: add path: /spec/template/spec/containers/0/env/- value: name: OSAC_AAP_URL - value: "http://osac-aap/api/controller" + value: "https://osac-aap/api/controller" @@ - op: add path: /spec/template/spec/containers/0/env/- value: name: OSAC_AAP_INSECURE_SKIP_VERIFY - value: "true" + value: "false"Also applies to: 190-191
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@overlays/osac-integration/kustomization.yaml` around lines 176 - 177, The OSAC AAP URL is configured insecurely and the integration also sets insecure verification to "true": update the OSAC_AAP_URL environment value to use "https://..." instead of "http://", and change the corresponding insecure verification setting (the variable that enables skipping TLS verification) to "false" or remove it so TLS verification is enforced; ensure any related variables or references that mention insecure verification are synchronized (e.g., the env var named OSAC_AAP_URL and the insecure verification flag currently set to "true") so token transport uses TLS with certificate validation.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@overlays/osac-integration/kustomization.yaml`:
- Around line 181-186: The OSAC_AAP_TOKEN secret reference is currently marked
optional which allows the pod to start without the AAP credentials; update the
secretKeyRef for name: osac-aap-api-token key: token (the OSAC_AAP_TOKEN env
var) to make the secret mandatory by changing optional: true to optional: false
(ensure OSAC_PROVISIONING_PROVIDER=aap expects this required secret).
- Around line 176-177: The OSAC AAP URL is configured insecurely and the
integration also sets insecure verification to "true": update the OSAC_AAP_URL
environment value to use "https://..." instead of "http://", and change the
corresponding insecure verification setting (the variable that enables skipping
TLS verification) to "false" or remove it so TLS verification is enforced;
ensure any related variables or references that mention insecure verification
are synchronized (e.g., the env var named OSAC_AAP_URL and the insecure
verification flag currently set to "true") so token transport uses TLS with
certificate validation.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: fa81188c-e99d-4c8e-8b5a-74e680c4afec
📒 Files selected for processing (1)
overlays/osac-integration/kustomization.yaml
There was a problem hiding this comment.
♻️ Duplicate comments (1)
overlays/osac-integration/kustomization.yaml (1)
181-191:⚠️ Potential issue | 🟠 Major | ⚡ Quick winHarden AAP token handling and TLS verification in the operator env patch.
Line 186 keeps
OSAC_AAP_TOKENoptional, and Line 191 setsOSAC_AAP_INSECURE_SKIP_VERIFYto"true". WithOSAC_PROVISIONING_PROVIDER="aap"(Line 166), this weakens startup safety and transport security for authenticated AAP API calls.Suggested hardening
- op: add path: /spec/template/spec/containers/0/env/- value: name: OSAC_AAP_TOKEN valueFrom: secretKeyRef: name: osac-aap-api-token key: token - optional: true @@ - op: add path: /spec/template/spec/containers/0/env/- value: name: OSAC_AAP_INSECURE_SKIP_VERIFY - value: "true" + value: "false"🤖 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 `@overlays/osac-integration/kustomization.yaml` around lines 181 - 191, The env patch weakens security: change the OSAC_AAP_TOKEN secretKeyRef to be required (remove or set optional: false on osac-aap-api-token) so the operator fails fast when OSAC_PROVISIONING_PROVIDER is "aap", and remove or set OSAC_AAP_INSECURE_SKIP_VERIFY to "false" (do not set it to "true") to enforce TLS verification in the operator container; update the patch that touches OSAC_AAP_TOKEN and OSAC_AAP_INSECURE_SKIP_VERIFY accordingly so startup and transport remain hardened.
🤖 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.
Duplicate comments:
In `@overlays/osac-integration/kustomization.yaml`:
- Around line 181-191: The env patch weakens security: change the OSAC_AAP_TOKEN
secretKeyRef to be required (remove or set optional: false on
osac-aap-api-token) so the operator fails fast when OSAC_PROVISIONING_PROVIDER
is "aap", and remove or set OSAC_AAP_INSECURE_SKIP_VERIFY to "false" (do not set
it to "true") to enforce TLS verification in the operator container; update the
patch that touches OSAC_AAP_TOKEN and OSAC_AAP_INSECURE_SKIP_VERIFY accordingly
so startup and transport remain hardened.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: ce198e6c-aa92-4b9c-a7af-d9202196722e
📒 Files selected for processing (2)
overlays/osac-integration/README.mdoverlays/osac-integration/kustomization.yaml
✅ Files skipped from review due to trivial changes (1)
- overlays/osac-integration/README.md
also fuldillment-internal-api is on 8001
|
/cc @omer-vishlitzky |
|
/lgtm |
|
/retest-required |
3 participants
For the moment this environment is configured for vmaas
Summary by CodeRabbit
New Features
Documentation
Chores