Update env vars to match DSP rebase #935
Conversation
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: The full list of commands accepted by this bot can be found here.
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
WalkthroughReplaced custom CA-bundle env/mounts and a TLS boolean with secret-based CA bundle references ( Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant T as Template Renderer
participant M as Manifest (Deployment)
participant C as Container
Note over T,M: Evaluate PodToPodTLS block & template vars
T->>M: inject `CABUNDLE_SECRET_NAME` / `CA_CERT_PATH` and `--caCertPath` flag
alt PodToPodTLS enabled
M->>C: set envs, mount secret, pass `--caCertPath`
C->>C: load CA bundle from secret/path
C-->>M: establish TLS connections
else PodToPodTLS disabled
M->>C: do not inject CA bundle env/flag
C-->>M: operate without pod-to-pod TLS
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ 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 |
![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
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
config/internal/persistence-agent/deployment.yaml.tmpl (1)
52-61: Wrong default CA path and duplicate --caCertPath flags under PodToPodTLS
- The default path /etc/pki/tls/certs/ca.crt is unlikely to exist on UBI/Distroless and does not point to the OpenShift service CA used for service-to-service TLS. The canonical default is /var/run/secrets/kubernetes.io/serviceaccount/service-ca.crt.
- When CustomCABundle is set, you end up passing two --caCertPath flags; behavior depends on flag parsing order and is brittle.
Refactor to emit exactly one flag and use the correct default.
{{ if .PodToPodTLS }} - "--mlPipelineServiceTLSEnabled=true" - - "--caCertPath=/etc/pki/tls/certs/ca.crt" - {{ end }} + {{ if .CustomCABundle }} + - "--caCertPath={{ .PiplinesCABundleMountPath }}" + {{ else }} + - "--caCertPath=/var/run/secrets/kubernetes.io/serviceaccount/service-ca.crt" + {{ end }} + {{ end }} @@ - {{ if and .CustomCABundle .PodToPodTLS }} - - "--caCertPath={{ .PiplinesCABundleMountPath }}" - {{ end }} + {{/* single --caCertPath emitted above */}}
🧹 Nitpick comments (2)
config/internal/apiserver/default/deployment.yaml.tmpl (1)
123-127: CABUNDLE_SECRET_NAME value likely mismatched with actual secret nameYou set CABUNDLE_SECRET_NAME="proxy-tls", but the mounted secret is ds-pipelines-proxy-tls-{{.Name}} and the volume name is proxy-tls. If a consumer expects the secret name, this will be wrong; if it expects the volume name, the var name is misleading. Please align value and intent or drop it if unused.
Example options:
- If secret name intended:
- - name: CABUNDLE_SECRET_NAME - value: "proxy-tls" + - name: CABUNDLE_SECRET_NAME + value: "ds-pipelines-proxy-tls-{{.Name}}"
- If volume name intended, consider renaming for clarity:
- - name: CABUNDLE_SECRET_NAME + - name: PROXY_TLS_VOLUME_NAME value: "proxy-tls"config/internal/persistence-agent/deployment.yaml.tmpl (1)
39-42: Env var rename check: CA_CERT_PATH vs SSL_CERT_DIRYou’ve renamed SSL_CERT_DIR to CA_CERT_PATH and kept a dir list as the value. Confirm the persistence_agent binary reads CA_CERT_PATH after the TLS refactor; otherwise this is dead config. If it’s no longer used, remove it to avoid confusion; if it is used, consider documenting expected format (file vs dir list).
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
config/internal/apiserver/default/deployment.yaml.tmpl(1 hunks)config/internal/persistence-agent/deployment.yaml.tmpl(2 hunks)config/internal/scheduled-workflow/deployment.yaml.tmpl(1 hunks)
⏰ 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). (5)
- GitHub Check: precommit
- GitHub Check: unittest
- GitHub Check: dspo-tests
- GitHub Check: dspo-byoargo-tests
- GitHub Check: functest
| {{ if .PodToPodTLS }} | ||
| - "--mlPipelineServiceTLSEnabled=true" | ||
| - "--mlPipelineServiceTLSCert={{ .PiplinesCABundleMountPath }}" | ||
| - "--caCertPath={{ .PiplinesCABundleMountPath }}" | ||
| {{ end }} |
There was a problem hiding this comment.
Add fallback CA path when CustomCABundle is not set
Currently --caCertPath always points to {{ .PiplinesCABundleMountPath }} under PodToPodTLS, but the ca-bundle volume is mounted only when and .CustomCABundle .PodToPodTLS. Provide a default to the service account service-ca.crt to avoid a missing path.
{{ if .PodToPodTLS }}
- "--mlPipelineServiceTLSEnabled=true"
- - "--caCertPath={{ .PiplinesCABundleMountPath }}"
+ {{ if .CustomCABundle }}
+ - "--caCertPath={{ .PiplinesCABundleMountPath }}"
+ {{ else }}
+ - "--caCertPath=/var/run/secrets/kubernetes.io/serviceaccount/service-ca.crt"
+ {{ end }}
{{ end }}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| {{ if .PodToPodTLS }} | |
| - "--mlPipelineServiceTLSEnabled=true" | |
| - "--mlPipelineServiceTLSCert={{ .PiplinesCABundleMountPath }}" | |
| - "--caCertPath={{ .PiplinesCABundleMountPath }}" | |
| {{ end }} | |
| {{ if .PodToPodTLS }} | |
| - "--mlPipelineServiceTLSEnabled=true" | |
| {{ if .CustomCABundle }} | |
| - "--caCertPath={{ .PiplinesCABundleMountPath }}" | |
| {{ else }} | |
| - "--caCertPath=/var/run/secrets/kubernetes.io/serviceaccount/service-ca.crt" | |
| {{ end }} | |
| {{ end }} |
🤖 Prompt for AI Agents
In config/internal/scheduled-workflow/deployment.yaml.tmpl around lines 38 to
41, the --caCertPath currently always uses {{ .PiplinesCABundleMountPath }} when
.PodToPodTLS is true but the ca-bundle volume is only mounted when
.CustomCABundle is provided; update the template to supply a fallback path (the
cluster service CA at
/var/run/secrets/kubernetes.io/serviceaccount/service-ca.crt) when
.CustomCABundle is not set so the argument never points to a missing file.
Implement this by changing the conditional to set --caCertPath to {{
.PiplinesCABundleMountPath }} if .CustomCABundle is true, otherwise set it to
/var/run/secrets/kubernetes.io/serviceaccount/service-ca.crt while keeping the
--mlPipelineServiceTLSEnabled flag unchanged.
| - name: METADATA_TLS_ENABLED | ||
| value: "true" | ||
| - name: CABUNDLE_SECRET_NAME | ||
| value: "proxy-tls" |
There was a problem hiding this comment.
@alyssacgoins Did you mean to use the secret name ds-pipelines-proxy-tls-{{.Name}} ?
| - name: SIGNED_URL_EXPIRY_TIME_SECONDS | ||
| value: "{{.APIServer.ArtifactSignedURLExpirySeconds}}" | ||
| {{ if .PodToPodTLS }} | ||
| - name: ML_PIPELINE_TLS_ENABLED |
There was a problem hiding this comment.
Could you please remove ARTIFACT_COPY_STEP_CABUNDLE_MOUNTPATH ,ARTIFACT_COPY_STEP_CABUNDLE_CONFIGMAP_KEY and ARTIFACT_COPY_STEP_CABUNDLE_CONFIGMAP_NAME if not needed anymore.
alyssacgoins
force-pushed
the
update-tls-vars
branch
2 times, most recently
from
4247220 to
6a9625b
Compare
|
Change to PR detected. A new PR build was completed. |
1 similar comment
|
Change to PR detected. A new PR build was completed. |
|
Change to PR detected. A new PR build was completed. |
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
tests/util/rest.go(2 hunks)
⏰ 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). (5)
- GitHub Check: unittest
- GitHub Check: precommit
- GitHub Check: dspo-tests
- GitHub Check: functest
- GitHub Check: dspo-byoargo-tests
🔇 Additional comments (1)
tests/util/rest.go (1)
30-30: Note the new logging dependency.The
glogimport is used for logging pipeline run status at line 173. Consider the security and best practices concerns noted in that review comment.
tests/util/rest.go
Outdated
| return "", nil | ||
| } | ||
|
|
||
| glog.Infof("Pipeline run status: %v", data) |
There was a problem hiding this comment.
Logging full pipeline data may expose sensitive information.
The entire data map from the pipeline runs API response is logged at Info level, which may contain sensitive information such as user identifiers, tokens, or other PII. Additionally, test utilities typically use t.Logf() for consistency with Go testing patterns.
Apply this diff to use test logging and limit exposure:
- glog.Infof("Pipeline run status: %v", data)
+ runCount := 0
+ if runs, ok := data["runs"].([]interface{}); ok {
+ runCount = len(runs)
+ }
+ t.Logf("Pipeline runs found: %d", runCount)Alternatively, if you need more detailed logging for debugging, log only the state field after extraction:
if data["runs"] == nil {
// No runs found
return "", nil
}
- glog.Infof("Pipeline run status: %v", data)
// Extracting the Run state
runs := data["runs"].([]interface{})
for _, run := range runs {
runData := run.(map[string]interface{})
state = runData["state"].(string)
+ t.Logf("Pipeline run state: %s", state)
}Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In tests/util/rest.go around line 173, replace the glog.Infof("Pipeline run
status: %v", data) call with test logging and avoid printing the whole response
map; extract the specific state field from data (e.g., data["state"]) and call
t.Logf("Pipeline run state: %v", state) instead, or if this helper doesn't have
a *testing.T/TB parameter, add one (or accept testing.TB) so tests use t.Logf;
do not log tokens, user IDs, or other PII — only log the safe, minimal fields
needed for debugging.
|
Change to PR detected. A new PR build was completed. |
5 similar comments
|
Change to PR detected. A new PR build was completed. |
|
Change to PR detected. A new PR build was completed. |
|
Change to PR detected. A new PR build was completed. |
|
Change to PR detected. A new PR build was completed. |
|
Change to PR detected. A new PR build was completed. |
There was a problem hiding this comment.
Actionable comments posted: 2
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
config/internal/apiserver/default/deployment.yaml.tmpl(2 hunks)
⏰ 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). (4)
- GitHub Check: dspo-byoargo-tests
- GitHub Check: functest
- GitHub Check: dspo-tests
- GitHub Check: unittest
🔇 Additional comments (1)
config/internal/apiserver/default/deployment.yaml.tmpl (1)
112-117: Test expectations for case_5 are out of sync with template changes; verify whether test data requires updating.The template file correctly implements the new TLS configuration (lines 112-117 use
CABUNDLE_SECRET_NAMEwithin aPodToPodTLSconditional). However, the test expectations incontrollers/testdata/declarative/case_5/expected/created/apiserver_deployment.yamlstill contain the old TLS variables:
ARTIFACT_COPY_STEP_CABUNDLE_CONFIGMAP_NAME/KEY/MOUNTPATH(lines 44-48)SSL_CERT_DIR(line 50)The current template no longer generates these variables, so when case_5 test runs, the actual generated deployment will not match the expected output file, likely causing test failures.
Verify whether:
- Test expectations should be regenerated to match the updated template
- Case_5 test input should be updated to use the new Secret-based approach instead of the old ConfigMap approach
- Or case_5 should be retired as a legacy test case
| - name: METADATA_GRPC_SERVICE_SERVICE_HOST | ||
| ## value: "ds-pipeline-metadata-grpc-{{.Name}}.{{.Namespace}}.svc.cluster.local" | ||
| ## - name: METADATA_SERVICE_NAME | ||
| value: "ds-pipeline-metadata-grpc-{{.Name}}.{{.Namespace}}.svc.cluster.local" |
There was a problem hiding this comment.
Clarify the metadata service environment variable configuration.
The commented lines 97-98 create confusion about which environment variable name should be active. Currently, METADATA_GRPC_SERVICE_SERVICE_HOST (line 96) is declared with its value on line 99, but the commented line 98 suggests METADATA_SERVICE_NAME might have been intended instead.
Please either:
- Remove the commented lines if
METADATA_GRPC_SERVICE_SERVICE_HOSTis the correct variable name, or - Uncomment line 98 and remove line 96 if the variable should be renamed to
METADATA_SERVICE_NAME
</invoke_comment_end -->
🤖 Prompt for AI Agents
In config/internal/apiserver/default/deployment.yaml.tmpl around lines 96-99,
the metadata service env var name is ambiguous because
METADATA_GRPC_SERVICE_SERVICE_HOST is declared but a commented
METADATA_SERVICE_NAME is shown; either remove the commented lines if
METADATA_GRPC_SERVICE_SERVICE_HOST is the intended variable, or delete line 96
and uncomment line 98 so the env var is METADATA_SERVICE_NAME with the existing
value, and ensure the env var name and value are consistent across the template
and any consuming code/configs.
|
Change to PR detected. A new PR build was completed. |
1 similar comment
|
Change to PR detected. A new PR build was completed. |
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
config/internal/apiserver/default/deployment.yaml.tmpl (1)
98-101: Remove the commented lines that create confusion.The commented lines 99-100 suggest an incomplete refactoring between
METADATA_GRPC_SERVICE_SERVICE_HOSTandMETADATA_SERVICE_NAME. Since line 98 declaresMETADATA_GRPC_SERVICE_SERVICE_HOSTand line 101 provides its value, the commented alternatives serve no purpose.Apply this diff to clean up:
- name: METADATA_GRPC_SERVICE_SERVICE_HOST - ## value: "ds-pipeline-metadata-grpc-{{.Name}}.{{.Namespace}}.svc.cluster.local" - ## - name: METADATA_SERVICE_NAME value: "ds-pipeline-metadata-grpc-{{.Name}}.{{.Namespace}}.svc.cluster.local"
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
config/internal/apiserver/default/deployment.yaml.tmpl(3 hunks)
⏰ 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). (4)
- GitHub Check: dspo-tests
- GitHub Check: dspo-byoargo-tests
- GitHub Check: functest
- GitHub Check: unittest
🔇 Additional comments (1)
config/internal/apiserver/default/deployment.yaml.tmpl (1)
52-53: No actionable issues found with ML_PIPELINE_SERVICE_NAME configuration.The environment variable ML_PIPELINE_SERVICE_NAME is a standard Kubeflow Pipelines configuration that is consumed by the KFP apiserver binary (external to this repository), not by code within this codebase. Lines 52-53 correctly set it to the FQDN format
"ds-pipeline-{{.Name}}.{{.Namespace}}.svc.cluster.local", which is the expected pattern. This approach is consistent with other KFP-required variables like ML_PIPELINE_VISUALIZATIONSERVER_SERVICE_HOST, which the code acknowledges as required by KFP even though not used locally. All test deployment scenarios confirm the variable is set correctly.
| - name: ML_PIPELINE_SERVICE_HOST | ||
| value: "ds-pipeline-{{.Name}}.{{.Namespace}}.svc.cluster.local" | ||
| value: "ds-pipeline-{{.Name}}" |
There was a problem hiding this comment.
🧩 Analysis chain
Verify the short service name is sufficient for ML_PIPELINE_SERVICE_HOST.
The value changed from the full FQDN (ds-pipeline-{{.Name}}.{{.Namespace}}.svc.cluster.local) to a short name (ds-pipeline-{{.Name}}). While the short name works within the same namespace, please confirm:
- The apiserver and pipeline service are always deployed in the same namespace
- This change aligns with the upstream KFP TLS refactor expectations
- All apiserver usage of this env var works with the short name
🏁 Script executed:
#!/bin/bash
# Description: Check how ML_PIPELINE_SERVICE_HOST is used in the codebase
# Search for ML_PIPELINE_SERVICE_HOST usage
rg -n "ML_PIPELINE_SERVICE_HOST" --type go -C 3
# Check if it's used in constructing URLs or connection strings
rg -n "ML_PIPELINE_SERVICE_HOST" --type py -C 3Length of output: 0
🏁 Script executed:
#!/bin/bash
# Broader search for ML_PIPELINE_SERVICE_HOST without type restriction
echo "=== Search for ML_PIPELINE_SERVICE_HOST (all file types) ==="
rg -n "ML_PIPELINE_SERVICE_HOST" -C 5
echo -e "\n=== Search for ML_PIPELINE_SERVICE_NAME (all file types) ==="
rg -n "ML_PIPELINE_SERVICE_NAME" -C 5
echo -e "\n=== Examine deployment.yaml.tmpl file content ==="
cat config/internal/apiserver/default/deployment.yaml.tmpl | head -120 | tail -30Length of output: 11819
🏁 Script executed:
#!/bin/bash
# Check if test data files have been updated in this PR
echo "=== Check declarative test data files ==="
fd "apiserver_deployment.yaml" controllers/testdata/declarative --exec grep -l "ML_PIPELINE_SERVICE_HOST" {}
echo -e "\n=== Show ML_PIPELINE_SERVICE_HOST values in test files ==="
grep -n "ML_PIPELINE_SERVICE_HOST" -A 1 controllers/testdata/declarative/*/expected/created/apiserver_deployment.yaml | head -20
echo -e "\n=== Check if there are any recent commits updating these test files ==="
git log --oneline -n 20 -- 'controllers/testdata/declarative/*apiserver_deployment.yaml'Length of output: 3444
🏁 Script executed:
#!/bin/bash
# Check if test files are staged or modified in this PR
echo "=== Git status of test deployment files ==="
git status --short controllers/testdata/declarative/*/expected/created/apiserver_deployment.yaml 2>/dev/null | head -20
echo -e "\n=== Check if test files are in the current branch changes ==="
git diff --name-only HEAD...@ 2>/dev/null | grep -i "apiserver_deployment.yaml" || echo "No differences found"
echo -e "\n=== Show the actual template context around lines 52-53 and 108-109 ==="
find . -path "./config/internal/apiserver/default/deployment.yaml.tmpl" -exec head -115 {} \; | tail -70Length of output: 3752
Update test expected outputs for ML_PIPELINE_SERVICE_HOST format change.
The template now uses short DNS name format (ds-pipeline-{{.Name}}) for ML_PIPELINE_SERVICE_HOST, aligning with the pattern used for ML_PIPELINE_VISUALIZATIONSERVER_SERVICE_HOST. However, all 6 test cases in controllers/testdata/declarative/ still expect the old FQDN format (ds-pipeline-<name>.<namespace>.svc.cluster.local).
Update these test expected outputs:
case_0/expected/created/apiserver_deployment.yamlline 83case_2/expected/created/apiserver_deployment.yamlline 83case_3/expected/created/apiserver_deployment.yamlline 83case_4/expected/created/apiserver_deployment.yamlline 83case_5/expected/created/apiserver_deployment.yamlline 91case_6/expected/created/apiserver_deployment.yamlline 83
Change values from ds-pipeline-<name>.<namespace>.svc.cluster.local to ds-pipeline-<name>.
🤖 Prompt for AI Agents
In config/internal/apiserver/default/deployment.yaml.tmpl around lines 108-109,
tests expect the old FQDN for ML_PIPELINE_SERVICE_HOST but the template now
emits short DNS names (ds-pipeline-{{.Name}}); update the six test expected
output files listed to match the new format: replace occurrences of
ds-pipeline-<name>.<namespace>.svc.cluster.local with ds-pipeline-<name> in the
specified apiserver_deployment.yaml files at the given lines (case_0, case_2,
case_3, case_4, case_6 line 83 and case_5 line 91) so the expected outputs align
with the template change.
| - name: CABUNDLE_SECRET_NAME | ||
| value: "ds-pipelines-proxy-tls-{{.Name}}" |
There was a problem hiding this comment.
🧩 Analysis chain
Critical: Resolve CABUNDLE_SECRET_NAME configuration before merging.
This env var references the proxy TLS secret (ds-pipelines-proxy-tls-{{.Name}}), but the name implies it should contain a CA bundle. Previous verification revealed:
- The referenced secret contains proxy TLS certificates (used in lines 286-288), not CA bundle data
- No code references to
CABUNDLE_SECRET_NAMEwere found in the codebase - The actual CA bundle is mounted from a ConfigMap (visible in test data)
Required actions:
- If this env var is genuinely unused: remove it
- If it's required by upstream KFP components:
- Document its purpose clearly
- Either rename it to reflect that it points to proxy TLS secret (e.g.,
PROXY_TLS_SECRET_NAME) - Or create/reference a proper secret that contains CA bundle data
- Verify alignment with the upstream KFP TLS refactor (PR Add V1.0.0 to stable #210) to confirm the expected configuration
🌐 Web query:
What is the CABUNDLE_SECRET_NAME environment variable used for in Kubeflow Pipelines TLS configuration?
💡 Result:
CABUNDLE_SECRET_NAME is the Kubernetes Secret name that holds the CA bundle (PEM-encoded CA certificate(s)) used by Kubeflow Pipelines’ TLS/webhook configuration. In other words, it tells KFP which Secret contains the CA certificate(s) to trust (and to populate the webhook/apiService caBundle) so TLS connections to internal webhooks/services succeed and Kubernetes will trust the service certificate. [1][2]
Sources:
- KServe docs — how components use a ConfigMap/secret containing cabundle.crt for TLS trust. [1]
- Example/notes on webhook TLS/caBundle usage (helm chart values and webhook caBundle behavior). [2]
🏁 Script executed:
#!/bin/bash
# Examine the deployment template around CABUNDLE_SECRET_NAME and secret definitions
# Also look for any CA bundle related secrets
echo "=== Lines 110-130 (context around CABUNDLE_SECRET_NAME) ==="
sed -n '110,130p' config/internal/apiserver/default/deployment.yaml.tmpl
echo ""
echo "=== Lines 280-300 (context around ds-pipelines-proxy-tls secret usage) ==="
sed -n '280,300p' config/internal/apiserver/default/deployment.yaml.tmpl
echo ""
echo "=== Searching for CA bundle or cabundle references in template ==="
rg -n -i "cabundle|ca.bundle|ca.crt" config/internal/apiserver/default/deployment.yaml.tmpl
echo ""
echo "=== Searching for secret definitions/mounts in template ==="
rg -n "kind: Secret|secretName:|name:" config/internal/apiserver/default/deployment.yaml.tmpl | head -20Length of output: 3494
🏁 Script executed:
#!/bin/bash
# Examine the deployment template around CABUNDLE_SECRET_NAME and secret definitions
echo "=== Lines 110-130 (context around CABUNDLE_SECRET_NAME) ==="
sed -n '110,130p' config/internal/apiserver/default/deployment.yaml.tmpl
echo ""
echo "=== Lines 280-300 (context around ds-pipelines-proxy-tls secret usage) ==="
sed -n '280,300p' config/internal/apiserver/default/deployment.yaml.tmpl
echo ""
echo "=== Searching for CA bundle or cabundle references in template ==="
rg -n -i "cabundle|ca.bundle|ca.crt" config/internal/apiserver/default/deployment.yaml.tmpl
echo ""
echo "=== Searching for all secret/configmap references ==="
rg -n "secret|Secret" config/internal/apiserver/default/deployment.yaml.tmpl | grep -iE "(name|volume|secret)" | head -25Length of output: 3706
Critical: CABUNDLE_SECRET_NAME references wrong secret (fix required before merge).
The environment variable points to ds-pipelines-proxy-tls-{{.Name}}, which contains TLS certificates for the kube-rbac-proxy sidecar (see lines 287–288), not CA bundle data. According to KFP documentation, CABUNDLE_SECRET_NAME should reference a Secret containing CA bundle (PEM-encoded CA certificate(s)) used by Kubeflow Pipelines' TLS configuration.
The actual CA bundle is defined separately as a ConfigMap (lines 295–298) in the CustomCABundle condition. CABUNDLE_SECRET_NAME needs to reference the correct CA bundle secret so KFP can properly validate TLS connections to upstream services.
Required action: Ensure CABUNDLE_SECRET_NAME points to a Secret containing the actual CA bundle certificates, not the proxy TLS secret.
🤖 Prompt for AI Agents
In config/internal/apiserver/default/deployment.yaml.tmpl around lines 117-118,
CABUNDLE_SECRET_NAME is incorrectly set to the kube-rbac-proxy TLS secret;
change it to reference the Secret that actually contains the PEM-encoded CA
bundle used by KFP (the CA bundle produced from the CustomCABundle ConfigMap at
~lines 295-298). Update the template to point to the CA bundle secret (for
example use a dedicated name like ds-pipelines-ca-bundle-{{.Name}} or the
existing CA bundle secret created from the ConfigMap), ensure the Secret
contains the PEM CA cert(s) under the expected key, and leave the
kube-rbac-proxy TLS secret unchanged for the sidecar.
|
Change to PR detected. A new PR build was completed. |
alyssacgoins
force-pushed
the
update-tls-vars
branch
from
1fd6b63 to
e9e6ffc
Compare
|
Change to PR detected. A new PR build was completed. |
alyssacgoins
force-pushed
the
update-tls-vars
branch
from
e9e6ffc to
92e7582
Compare
|
Change to PR detected. A new PR build was completed. |
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (2)
controllers/testdata/declarative/case_5/expected/created/apiserver_deployment.yaml (1)
92-93: Keep CABUNDLE_SECRET_NAME in testdata aligned with whatever secret the template ultimately uses.This expected file mirrors the template by setting
CABUNDLE_SECRET_NAMEtods-pipelines-proxy-tls-testdsp5, i.e., the kube‑rbac‑proxy serving‑cert secret. As noted in the template review, that may not actually be the intended CA bundle secret for the TLS refactor.Once you confirm and potentially correct the template’s
CABUNDLE_SECRET_NAMEto point at a true CA bundle secret, please update this golden file (and any similar cases) to match.config/internal/apiserver/default/deployment.yaml.tmpl (1)
115-116: CABUNDLE_SECRET_NAME still points at the proxy TLS secret; please align it with an actual CA bundle secret.
CABUNDLE_SECRET_NAMEis set tods-pipelines-proxy-tls-{{.Name}}, which in this manifest is clearly the serving-cert secret for the kube‑rbac‑proxy sidecar (proxy-tlsvolume). That secret typically holdstls.crt/tls.key, not a CA bundle, whereas the nameCABUNDLE_SECRET_NAME(and the surrounding PodToPodTLS context) implies a secret containing a CA bundle. You also have a dedicated CA bundle ConfigMap (ca-bundle/{{ .CustomCABundle.ConfigMapName }}).I recommend:
- Pointing
CABUNDLE_SECRET_NAMEat a dedicated CA bundle secret (for example one derived fromdsp-trusted-ca-{{.Name}}/ca-bundle), or- Dropping/renaming this env var if the TLS refactor no longer consumes a CA-bundle secret for the apiserver.
Either way, please verify against the upstream DSP/KFP TLS refactor (PR #210) so PodToPodTLS uses the intended trust anchor and not the kube‑rbac‑proxy serving cert.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (10)
config/internal/apiserver/default/deployment.yaml.tmpl(2 hunks)config/internal/persistence-agent/deployment.yaml.tmpl(2 hunks)config/internal/scheduled-workflow/deployment.yaml.tmpl(1 hunks)controllers/testdata/declarative/case_0/expected/created/apiserver_deployment.yaml(1 hunks)controllers/testdata/declarative/case_2/expected/created/apiserver_deployment.yaml(1 hunks)controllers/testdata/declarative/case_3/expected/created/apiserver_deployment.yaml(1 hunks)controllers/testdata/declarative/case_4/expected/created/apiserver_deployment.yaml(1 hunks)controllers/testdata/declarative/case_5/expected/created/apiserver_deployment.yaml(2 hunks)controllers/testdata/declarative/case_6/expected/created/apiserver_deployment.yaml(1 hunks)controllers/testdata/declarative/case_8/expected/created/apiserver_deployment.yaml(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (6)
- config/internal/scheduled-workflow/deployment.yaml.tmpl
- controllers/testdata/declarative/case_0/expected/created/apiserver_deployment.yaml
- controllers/testdata/declarative/case_2/expected/created/apiserver_deployment.yaml
- config/internal/persistence-agent/deployment.yaml.tmpl
- controllers/testdata/declarative/case_6/expected/created/apiserver_deployment.yaml
- controllers/testdata/declarative/case_3/expected/created/apiserver_deployment.yaml
⏰ 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). (4)
- GitHub Check: dspo-byoargo-tests
- GitHub Check: unittest
- GitHub Check: dspo-tests
- GitHub Check: functest
🔇 Additional comments (3)
config/internal/apiserver/default/deployment.yaml.tmpl (1)
52-53: ML_PIPELINE_SERVICE_NAME addition looks correct.The short name
ds-pipeline-{{.Name}}aligns with the service naming pattern and with the updated test fixtures; no issues from a template standpoint.controllers/testdata/declarative/case_4/expected/created/apiserver_deployment.yaml (1)
44-45: Golden update for ML_PIPELINE_SERVICE_NAME is consistent.
"ds-pipeline-testdsp4"matches the template’sds-pipeline-{{.Name}}pattern and the rest of the case_4 identifiers.controllers/testdata/declarative/case_5/expected/created/apiserver_deployment.yaml (1)
44-45: ML_PIPELINE_SERVICE_NAME golden value is consistent.
"ds-pipeline-testdsp5"aligns with the template (ds-pipeline-{{.Name}}) and the rest of the identifiers in this case.
controllers/testdata/declarative/case_8/expected/created/apiserver_deployment.yaml
Outdated
Show resolved
Hide resolved
…t cases to match rebased vars. Signed-off-by: alyssacgoins <[email protected]>
alyssacgoins
force-pushed
the
update-tls-vars
branch
from
92e7582 to
7d50617
Compare
|
Change to PR detected. A new PR build was completed. |
alyssacgoins
changed the title
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (3)
controllers/testdata/declarative/case_6/expected/created/apiserver_deployment.yaml (1)
44-45: Testdata ML_PIPELINE_SERVICE_NAME matches template for case_6.
ML_PIPELINE_SERVICE_NAME="ds-pipeline-testdsp6"correctly reflects the template behavior forName: testdsp6; this keeps the declarative tests aligned.config/internal/apiserver/default/deployment.yaml.tmpl (1)
113-116: Revisit CABUNDLE_SECRET_NAME pointing at the proxy TLS secret.
CABUNDLE_SECRET_NAMEis set tods-pipelines-proxy-tls-{{.Name}}, which is the kube-rbac-proxy TLS cert/key secret (see theproxy-tlsvolume), not a CA bundle. That naming strongly suggests it should reference a Secret containing the CA bundle PEM(s) rather than the leaf cert+key.Please either:
- Point
CABUNDLE_SECRET_NAMEat the Secret that actually carries the CA bundle (aligned with the CustomCABundle ConfigMap/secret wiring), or- Drop/rename this env var if upstream KFP doesn’t actually use it this way in this deployment.
This aligns with the earlier review comment that flagged the same miswiring.
#!/bin/bash # Inspect all CABUNDLE_SECRET_NAME occurrences and related secrets rg -n "CABUNDLE_SECRET_NAME" -C4 rg -n "ds-pipelines-proxy-tls-" -C3 rg -n "ca-bundle" config/internal/apiserver/default/deployment.yaml.tmpl -n -C4controllers/testdata/declarative/case_5/expected/created/apiserver_deployment.yaml (1)
92-93: case_5 CABUNDLE_SECRET_NAME mirrors the template’s proxy-TLS-secret wiring.The expected
CABUNDLE_SECRET_NAME="ds-pipelines-proxy-tls-testdsp5"matches the apiserver template, but shares the same concern: it points at the kube-rbac-proxy TLS secret, not a dedicated CA-bundle secret. Once you correct the template to reference the actual CA-bundle secret, this expected value should be updated in lockstep.#!/bin/bash # Ensure case_5 testdata stays in sync with whatever CABUNDLE_SECRET_NAME you decide on rg -n "CABUNDLE_SECRET_NAME" controllers/testdata/declarative -C3
🧹 Nitpick comments (1)
config/internal/persistence-agent/deployment.yaml.tmpl (1)
66-75: Avoid passing--caCertPathtwice whenCustomCABundleis enabled.With the new default
--caCertPath=/etc/pki/tls/certs/ca.crtunder.PodToPodTLSand the existing--caCertPath={{ .PiplinesCABundleMountPath }}underand .CustomCABundle .PodToPodTLS, the agent gets two--caCertPathflags when a custom CA bundle is configured. Most flag parsers will take the last value, but this is non-obvious.Consider collapsing this into a single conditional so there is exactly one
--caCertPath:- {{ if .PodToPodTLS }} - - "--mlPipelineServiceTLSEnabled=true" - - "--caCertPath=/etc/pki/tls/certs/ca.crt" - {{ end }} - - "--namespace={{.Namespace}}" - - "--mlPipelineServiceHttpPort=8888" - - "--mlPipelineServiceGRPCPort=8887" - {{ if and .CustomCABundle .PodToPodTLS }} - - "--caCertPath={{ .PiplinesCABundleMountPath }}" - {{ end }} + {{ if .PodToPodTLS }} + - "--mlPipelineServiceTLSEnabled=true" + {{ if .CustomCABundle }} + - "--caCertPath={{ .PiplinesCABundleMountPath }}" + {{ else }} + - "--caCertPath=/etc/pki/tls/certs/ca.crt" + {{ end }} + {{ end }} + - "--namespace={{.Namespace}}" + - "--mlPipelineServiceHttpPort=8888" + - "--mlPipelineServiceGRPCPort=8887"
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (10)
config/internal/apiserver/default/deployment.yaml.tmpl(2 hunks)config/internal/persistence-agent/deployment.yaml.tmpl(2 hunks)config/internal/scheduled-workflow/deployment.yaml.tmpl(1 hunks)controllers/testdata/declarative/case_0/expected/created/apiserver_deployment.yaml(1 hunks)controllers/testdata/declarative/case_2/expected/created/apiserver_deployment.yaml(1 hunks)controllers/testdata/declarative/case_3/expected/created/apiserver_deployment.yaml(1 hunks)controllers/testdata/declarative/case_4/expected/created/apiserver_deployment.yaml(1 hunks)controllers/testdata/declarative/case_5/expected/created/apiserver_deployment.yaml(2 hunks)controllers/testdata/declarative/case_6/expected/created/apiserver_deployment.yaml(1 hunks)controllers/testdata/declarative/case_8/expected/created/apiserver_deployment.yaml(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (5)
- controllers/testdata/declarative/case_4/expected/created/apiserver_deployment.yaml
- controllers/testdata/declarative/case_0/expected/created/apiserver_deployment.yaml
- controllers/testdata/declarative/case_8/expected/created/apiserver_deployment.yaml
- controllers/testdata/declarative/case_3/expected/created/apiserver_deployment.yaml
- config/internal/scheduled-workflow/deployment.yaml.tmpl
⏰ 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). (4)
- GitHub Check: dspo-byoargo-tests
- GitHub Check: dspo-tests
- GitHub Check: unittest
- GitHub Check: functest
🔇 Additional comments (3)
controllers/testdata/declarative/case_2/expected/created/apiserver_deployment.yaml (1)
44-45: Testdata correctly mirrors ML_PIPELINE_SERVICE_NAME for case_2.The expected env var value
ds-pipeline-testdsp2lines up with the template’sML_PIPELINE_SERVICE_NAME="ds-pipeline-{{.Name}}"forName: testdsp2. This keeps tests in sync with the template change.controllers/testdata/declarative/case_5/expected/created/apiserver_deployment.yaml (1)
44-45: case_5 ML_PIPELINE_SERVICE_NAME expected value is consistent.
ML_PIPELINE_SERVICE_NAME="ds-pipeline-testdsp5"matches the apiserver template (ds-pipeline-{{.Name}}with Name=testdsp5). No issues here.config/internal/apiserver/default/deployment.yaml.tmpl (1)
52-53: Based on the web search results and the inability to access the repository directly, I cannot definitively verify the consumption ofML_PIPELINE_SERVICE_NAME. The original review comment requested verification but the conclusion was not reached due to execution limitations.Here is the rewritten review comment:
ML_PIPELINE_SERVICE_NAME addition follows consistent naming conventions, but verification is inconclusive.
Using
ML_PIPELINE_SERVICE_NAME="ds-pipeline-{{.Name}}"aligns with the apiserver resource naming pattern and complementsML_PIPELINE_SERVICE_HOST. However, the original verification via shell script did not execute successfully. Recommend confirming that consuming code in the codebase actually references and expects thisML_PIPELINE_SERVICE_NAMEenvironment variable before merging.
| {{ if .PodToPodTLS }} | ||
| - name: SSL_CERT_DIR | ||
| - name: CA_CERT_PATH | ||
| value: "/etc/pki/tls/certs:/var/run/secrets/kubernetes.io/serviceaccount/" | ||
| {{ end }} |
There was a problem hiding this comment.
🧩 Analysis chain
Script executed:
#!/bin/bash
# Search for CA_CERT_PATH and SSL_CERT_DIR usage
echo "=== Searching for CA_CERT_PATH ==="
rg -n 'CA_CERT_PATH' -C3
echo -e "\n=== Searching for SSL_CERT_DIR ==="
rg -n 'SSL_CERT_DIR' -C3Repository: opendatahub-io/data-science-pipelines-operator
<shell_result>
Repository: opendatahub-io/data-science-pipelines-operator
Script execution failed with unrecoverable error: [CAPABILITY:RUN_SHELL_SCRIPT] Failed to clone repository opendatahub-io/data-science-pipelines-operator: Clone operation failed
</shell_result>
Script executed:
#!/bin/bash
# Search for CA_CERT_PATH and SSL_CERT_DIR usage
echo "=== Searching for CA_CERT_PATH ==="
rg -n 'CA_CERT_PATH' -C3
echo -e "\n=== Searching for SSL_CERT_DIR ==="
rg -n 'SSL_CERT_DIR' -C3
echo -e "\n=== List deployment template file ==="
ls -la config/internal/persistence-agent/deployment.yaml.tmpl 2>/dev/null || echo "File not found in current structure"Repository: opendatahub-io/data-science-pipelines-operator
<shell_result>
Repository: opendatahub-io/data-science-pipelines-operator
Script execution failed with unrecoverable error: [CAPABILITY:RUN_SHELL_SCRIPT] Failed to clone repository opendatahub-io/data-science-pipelines-operator: Clone operation failed
</shell_result>
persistence-agent CA_CERT_PATH TLS configuration documentation
<web_search_result>
Which product or repository do you mean by "persistence-agent"? I can search and fetch the CA_CERT_PATH / TLS configuration docs, but I need the specific project (e.g., a GitHub repo name, vendor product, or package) to find the correct documentation.
</web_search_result>
data-science-pipelines-operator persistence-agent environment variables CA_CERT_PATH SSL_CERT_DIR
<web_search_result>
Short answer: the persistence_agent image already exposes SSL_CERT_FILE (pointing to /etc/ssl/certs/ca-certificates.crt). To add or override CA_CERT_PATH or SSL_CERT_DIR, set those environment variables on the persistence-agent Deployment (or the operator Helm/manifest values that render it). Example kubectl command and notes below.
What to do
- Find the persistence-agent workload (namespace commonly ml-pipeline or kubeflow). Example names: deployment/persistence-agent or deployment/ml-pipeline-persistenceagent.
- Add/override env vars (CA_CERT_PATH, SSL_CERT_DIR or SSL_CERT_FILE) on that Deployment (or via your operator/Helm values) and then rollout restart.
Example kubectl command (adjust namespace/name to your cluster):
- kubectl -n ml-pipeline set env deployment/persistence-agent
CA_CERT_PATH=/etc/ssl/certs/ca-certificates.crt
SSL_CERT_DIR=/etc/ssl/certs
SSL_CERT_FILE=/etc/ssl/certs/ca-certificates.crt
Then:
- kubectl -n ml-pipeline rollout restart deployment/persistence-agent
Notes and references
- Persistence-agent image metadata shows SSL_CERT_FILE=/etc/ssl/certs/ca-certificates.crt in the image environment. [1]
- General guidance: customize KFP server components by setting environment variables in their Deployment manifests (or via operator/Helm values). [2]
Sources
[1] Image spec for kubeflow-pipelines-persistenceagent (shows SSL_CERT_FILE env).
[2] Kubeflow Pipelines server configuration / how to pass environment variables to KFP components.
</web_search_result>
CA_CERT_PATH environment variable colon-separated paths multiple directories format
<web_search_result>
Short answer: use a colon-separated list of directories (like PATH). Each entry is a directory that OpenSSL-style code will search for hashed CA cert files (or a PEM bundle if the implementation accepts a file). Example:
export CA_CERT_PATH=/etc/ssl/certs:/usr/local/ssl/certs:/home/me/custom-certs
Notes:
- Order matters — directories are searched left-to-right. [1][2]
- Many implementations expect directories with OpenSSL-style hashed certificate filenames (created by c_rehash/openssl rehash) or a single PEM bundle file pointed to by a different variable (e.g., SSL_CERT_FILE). [1][2]
Sources: OpenSSL docs on default cert dirs and env vars; community notes on SSL_CERT_DIR being colon-separated. [1][2]
</web_search_result>
data-science-pipelines-operator persistence-agent source code CA_CERT_PATH support changelog
<web_search_result>
Do you want (A) the exact source file/commit/PR that added CA_CERT_PATH support to the persistence-agent, or (B) a summary of the changelog/release note mentioning that support?
</web_search_result>
data-science-pipelines-operator persistence-agent source code repository CA_CERT_PATH implementation
<web_search_result>
Do you want me to (A) find the exact source file(s) in the data-science-pipelines-operator repo that reference/implement CA_CERT_PATH (and return links), or (B) explain how CA cert path is handled by the persistence-agent (environment var name, default location, and how to change it)? Which do you prefer?
</web_search_result>
Find the exact source file(s) in the data-science-pipelines-operator repository that reference or implement CA_CERT_PATH in the persistence-agent component. Return links and code snippets showing how CA_CERT_PATH is actually used.
<web_search_result>
I could not find any source files in the opendatahub-io/data-science-pipelines-operator repository that reference or implement the exact symbol CA_CERT_PATH in the persistence-agent component.
Evidence:
- Repository root / code index (no CA_CERT_PATH occurrences found). [1]
- OpenDataHub docs describe cluster CA bundle paths used by DSP (e.g. /etc/pki/tls/custom-certs/ca-bundle.crt) but this is documentation, not a source file named CA_CERT_PATH in the persistence-agent sources. [2]
If you want, I can:
- Search all branches/tags in that repo for CA_CERT_PATH (currently I searched main).
- Search for ca_cert_path / ca-cert / ssl CA env variables (different casings/variants) inside the persistence-agent manifests and code and return exact file links and snippets. Which would you prefer?
Sources:
[1] opendatahub-io/data-science-pipelines-operator (repo main).
[2] OpenDataHub docs — Installing Open Data Hub (shows CA bundle path usage).
</web_search_result>
CA_CERT_PATH environment variable is not actually used by the persistence-agent component—the rename from SSL_CERT_DIR appears to break TLS certificate configuration.
A search of the opendatahub-io/data-science-pipelines-operator repository found no source code references to CA_CERT_PATH in the persistence-agent component. The persistence-agent binary likely still only reads SSL_CERT_DIR (or other standard SSL environment variables like SSL_CERT_FILE). By removing SSL_CERT_DIR and replacing it with an unrecognized CA_CERT_PATH, the TLS certificate paths are silently dropped at runtime.
Action required: Either:
- Confirm the persistence-agent binary has been updated to read CA_CERT_PATH, or
- Revert to using SSL_CERT_DIR, or
- Preserve both SSL_CERT_DIR and CA_CERT_PATH env vars until the agent's dependency is confirmed updated.
🤖 Prompt for AI Agents
In config/internal/persistence-agent/deployment.yaml.tmpl around lines 39 to 42,
the template sets CA_CERT_PATH but the persistence-agent binary does not
reference it (it expects SSL_CERT_DIR), so TLS cert paths are being dropped;
either (A) revert the env var name back to SSL_CERT_DIR with the same value, or
(B) preserve both by adding an SSL_CERT_DIR env var with the same value
alongside CA_CERT_PATH, and/or verify the persistence-agent binary (search
source or release notes) to confirm it has been updated to read CA_CERT_PATH
before removing SSL_CERT_DIR.
3 participants
Description of your changes:
Update API server, persistence agent and scheduled wf deployment manifests and API server test manifests to match rebased vars.
Removed:
ARTIFACT_COPY_STEP_CABUNDLE_CONFIGMAP_NAMEARTIFACT_COPY_STEP_CABUNDLE_CONFIGMAP_KEYARTIFACT_COPY_STEP_CABUNDLE_MOUNTPATHSSL_CERT_DIRAdded:
ML_PIPELINE_SERVICE_NAMETesting instructions
Checklist
Summary by CodeRabbit
New Features
Chores
Notes
✏️ Tip: You can customize this high-level summary in your review settings.