Inject pull secret for Kserve components#15
Inject pull secret for Kserve components#15pierDipi wants to merge 5 commits intoopendatahub-io:mainfrom
Conversation
Signed-off-by: Pierangelo Di Pilato <pierdipi@redhat.com>
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: pierDipi 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 |
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Central YAML (base), Organization UI (inherited) Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (4)
💤 Files with no reviewable changes (3)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughAdds pull-secret support by introducing a Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant Helmfile
participant Helm
participant TemplateProcessor
participant KubernetesAPI
User->>Helmfile: deploy (values include pullSecret config)
Helmfile->>Helm: render release with set pullSecret.dockerConfigJson
Helm->>TemplateProcessor: render charts/kserve/templates/resources.yaml with values
TemplateProcessor->>TemplateProcessor: split resources.yaml into documents
alt doc is ServiceAccount and pullSecret provided
TemplateProcessor->>KubernetesAPI: emit Secret (dockerconfigjson) [once per namespace]
TemplateProcessor->>KubernetesAPI: emit ServiceAccount with imagePullSecrets referencing Secret
else other doc
TemplateProcessor->>KubernetesAPI: emit resource as-is
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~22 minutes 🚥 Pre-merge checks | ✅ 2✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. 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: 2
🧹 Nitpick comments (1)
charts/kserve/templates/resources.yaml (1)
22-31: ServiceAccountannotationsare silently dropped during reconstruction.The template only preserves
labels,name, andnamespacefrom the original ServiceAccount's metadata.annotationsare not copied. On AKS (azure.workload.identity/client-id), EKS (eks.amazonaws.com/role-arn), and GKE (workload identity), ServiceAccount annotations are essential for cloud IAM integration. Iffiles/resources.yamlever carries such annotations — or if the file is extended in the future — they will be silently lost.The same applies to
automountServiceAccountTokenand any pre-existingimagePullSecretson the original ServiceAccount.♻️ Proposed fix — forward metadata annotations and merge imagePullSecrets
--- apiVersion: v1 kind: ServiceAccount metadata: {{- with $resource.metadata.labels }} labels: {{- range $key, $value := . }} {{ $key }}: {{ $value | quote }} {{- end }} {{- end }} + {{- with $resource.metadata.annotations }} + annotations: + {{- range $key, $value := . }} + {{ $key }}: {{ $value | quote }} + {{- end }} + {{- end }} name: {{ $resource.metadata.name }} namespace: {{ $ns }} +{{- if $resource.automountServiceAccountToken }} +automountServiceAccountToken: {{ $resource.automountServiceAccountToken }} +{{- end }} +{{- $existingPullSecrets := $resource.imagePullSecrets | default list }} +{{- if $.Values.pullSecret.dockerConfigJson }} imagePullSecrets: - name: {{ $.Values.pullSecret.name }} + {{- range $existingPullSecrets }} + - name: {{ .name }} + {{- end }} +{{- else if $existingPullSecrets }} +imagePullSecrets: + {{- range $existingPullSecrets }} + - name: {{ .name }} + {{- end }} +{{- end }}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@charts/kserve/templates/resources.yaml` around lines 22 - 31, The ServiceAccount reconstruction in the template currently only preserves labels, name, and namespace and drops annotations, automountServiceAccountToken, and imagePullSecrets; update the ServiceAccount resource block to also forward {{ $resource.metadata.annotations }} into metadata.annotations, copy automountServiceAccountToken from {{ $resource.automountServiceAccountToken }}, and merge existing imagePullSecrets by iterating/including {{ $resource.imagePullSecrets }} so pre-existing pull secrets are retained rather than lost during template rendering.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@charts/kserve/templates/resources.yaml`:
- Around line 8-33: The ServiceAccount always receives imagePullSecrets even
when $.Values.pullSecret.dockerConfigJson is empty; wrap or move the
imagePullSecrets block so it is emitted only when the same condition that
creates the Secret is true (the if and set that reference
$.Values.pullSecret.dockerConfigJson and $pullSecretEmitted for $ns).
Specifically, ensure the imagePullSecrets: - name: {{ $.Values.pullSecret.name
}} line is inside the existing {{- if and $.Values.pullSecret.dockerConfigJson
(not (hasKey $pullSecretEmitted $ns)) }} ... {{- end }} guard (or duplicated
guarded with the same condition) so ServiceAccount (resource.metadata.name in
namespace $ns) only gets the pullSecret when the Secret is actually created.
In `@helmfile.yaml.gotmpl`:
- Around line 54-61: The condition checking for system Podman auth in
helmfile.yaml.gotmpl is using .Values.useSystemPodmanAuth directly and should
apply the same default as other releases; update the if/else test to use the
templating default (i.e., replace references to .Values.useSystemPodmanAuth in
the conditional around pullSecret.dockerConfigJson with
.Values.useSystemPodmanAuth | default true) so the kserve release follows the
same default-true behavior as the other child helmfiles.
---
Nitpick comments:
In `@charts/kserve/templates/resources.yaml`:
- Around line 22-31: The ServiceAccount reconstruction in the template currently
only preserves labels, name, and namespace and drops annotations,
automountServiceAccountToken, and imagePullSecrets; update the ServiceAccount
resource block to also forward {{ $resource.metadata.annotations }} into
metadata.annotations, copy automountServiceAccountToken from {{
$resource.automountServiceAccountToken }}, and merge existing imagePullSecrets
by iterating/including {{ $resource.imagePullSecrets }} so pre-existing pull
secrets are retained rather than lost during template rendering.
There was a problem hiding this comment.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@charts/kserve/templates/resources.yaml`:
- Around line 41-52: Ensure the ServiceAccount template only injects
imagePullSecrets when appropriate: add the pull-secret name when
$.Values.pullSecret.dockerConfigJson is true, preserve any existing
$resource.imagePullSecrets, and avoid duplicate entries; update the logic around
imagePullSecrets, $.Values.pullSecret.dockerConfigJson,
$.Values.pullSecret.name, and $resource.imagePullSecrets to merge the
pull-secret and existing secrets deterministically (dedupe by .name) so the
final imagePullSecrets list contains each name exactly once.
| # Pull secret for registry.redhat.io | ||
| # Auth is configured in the root values.yaml and passed via helmfile | ||
| pullSecret: | ||
| name: redhat-pull-secret |
There was a problem hiding this comment.
any reason to have this name configable?
can be just hardcoded in the template for 'secret', right?
| {{- if (.Values.useSystemPodmanAuth | default true) }} | ||
| - name: pullSecret.dockerConfigJson | ||
| file: {{ env "HOME" }}/.config/containers/auth.json |
There was a problem hiding this comment.
if we wanna default to true (when not set) maybe we do not need it values.yaml
or have it as
# uncomment belonw line if use dockerConfigJson
# useSystemPodmanAuth: false uncomment if when use dockerConfigJson
There was a problem hiding this comment.
This is following the existing pattern in other charts
|
@pierDipi Look at lines in make file - deploy-opendatahub-prerequisites: This copies the redhat-pull-secret from istio-system into the KServe namespace (opendatahub). docs/deploying-llm-d-on-managed-kubernetes.md - instruct users to manually copy the pull secret and patch ServiceAccounts with imagePullSecrets |
|
@aneeshkp I will clean it up now in this PR |
|
Thanks, also look in for docs - two places . |
Signed-off-by: Pierangelo Di Pilato <pierdipi@redhat.com>
pierDipi
force-pushed
the
kserve-image-pull-secret
branch
from
bf3b179 to
ef76c90
Compare
|
@pierDipi pull secret needs to be created in the Makefile that used to copy The gateway SA still has |
|
are you installing Kserve chart before setting up the Gateway? |
|
Also note that the current helmfile uses the previous released Kserve chart and not the local chart (we will need a new release after merging + update helmfile) |
|
Yes, I am installing kserver before running script/setup-gateway.sh. |
this is the problem, |
|
@pierDipi I get the same as Aneesh while trying this out. What do you mean by "previous helm chart"? |
|
I mean that helmfile points to the released dev chart
it doesn't use the local one because the local one uses unreleased images (registry.redhat.io) until 3.4 is out/released |
|
the current flow is:
|
There was a problem hiding this comment.
🧹 Nitpick comments (1)
charts/kserve/templates/resources.yaml (1)
3-4: The regex split pattern is fragile but currently safe. All document separators infiles/resources.yamlare properly placed at boundaries between complete YAML documents, and no bare---lines exist inside block scalars or data fields. However, the approach remains vulnerable to future changes: if any resource adds content like a PEM certificate or multi-line configuration containing a bare---, the split will fail. Consider adopting a proper YAML parser for more robust document separation.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@charts/kserve/templates/resources.yaml` around lines 3 - 4, The current regexSplit("(?m)^---$",$resourcesFile) approach is fragile; replace the manual split with the YAML-aware parser by using fromYamlAll to parse all documents at once (e.g. range over $resource := fromYamlAll $resourcesFile) instead of regexSplit + fromYaml so multi-document YAML with embedded '---' inside scalars (PEM blocks, multi-line configs) is handled correctly.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@charts/kserve/templates/resources.yaml`:
- Around line 3-4: The current regexSplit("(?m)^---$",$resourcesFile) approach
is fragile; replace the manual split with the YAML-aware parser by using
fromYamlAll to parse all documents at once (e.g. range over $resource :=
fromYamlAll $resourcesFile) instead of regexSplit + fromYaml so multi-document
YAML with embedded '---' inside scalars (PEM blocks, multi-line configs) is
handled correctly.
ℹ️ Review info
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Cache: Disabled due to data retention organization setting
Knowledge base: Disabled due to data retention organization setting
📒 Files selected for processing (7)
.gitignoreMakefilecharts/kserve/templates/resources.yamlcharts/kserve/values.yamldocs/deploying-llm-d-on-managed-kubernetes.mddocs/gateway-setup-for-kserve.mdscripts/setup-gateway.sh
💤 Files with no reviewable changes (3)
- Makefile
- scripts/setup-gateway.sh
- docs/deploying-llm-d-on-managed-kubernetes.md
✅ Files skipped from review due to trivial changes (1)
- docs/gateway-setup-for-kserve.md
|
PR needs rebase. 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. |
5 participants
Inject pull secret for Kserve components, inline with the other charts configurations
Summary by CodeRabbit
New Features
Bug Fixes
Chores
Documentation