Conversation
- Changed import and unpack commands to utilize a consistent model tag "model:latest" instead of dynamic tags. - Updated tagging logic to reflect the new model tag for improved clarity and maintainability.
…, coordination, authentication, authorization, inference, and metrics endpoint.
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
Summary of ChangesHello, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request updates the Highlights
Changelog
Activity
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for GitHub and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces several important updates to the ModelService controller, expanding RBAC permissions and refining model tagging logic in internal/modelartifact/job.go for consistent model handling. However, a critical security vulnerability was identified in the shell script within the model artifact job. The script is susceptible to argument injection due to unquoted shell expansions of variables derived from external sources (Secrets), and sensitive tokens are being passed as command-line arguments, which can lead to information leakage. It is strongly recommended to use safer shell scripting patterns and avoid passing secrets as CLI arguments to mitigate these risks.
| ${HF_TOKEN:+--token "$HF_TOKEN"} \ | ||
| --tag model:latest |
There was a problem hiding this comment.
The shell script in pipelineScript uses an unquoted parameter expansion ${HF_TOKEN:+--token "$HF_TOKEN"} on line 44. This pattern is vulnerable to Argument Injection because the shell performs word splitting on the result of the expansion. If the HF_TOKEN variable (sourced from a Kubernetes Secret) contains shell metacharacters, it can inject arbitrary flags into the kit import command. Additionally, passing sensitive tokens like HF_TOKEN as command-line arguments is a security risk as they are visible to other processes. It is recommended to pass secrets via environment variables or files. While the change to explicitly tag the imported model with --tag model:latest and consistently use model:latest is a good improvement for clarity and consistency, this security vulnerability needs to be addressed first. To fix the argument injection in a POSIX-compliant way, consider using the set -- pattern to build the argument list safely: set --; [ -n "$HF_REVISION" ] && set -- "$@" --ref "$HF_REVISION"; [ -n "$HF_TOKEN" ] && set -- "$@" --token "$HF_TOKEN"; kit import "$HF_REPO" "$@" --tag model:latest
There was a problem hiding this comment.
Pull request overview
Updates the model artifact import/pack/push job to use a consistent intermediate OCI tag, and refreshes controller/manifest RBAC plus Go dependencies.
Changes:
- Tag the imported model consistently as
model:latestand reference it forunpack/tagsteps. - Expand kubebuilder RBAC annotations and generated ClusterRole rules (pods/events/leases/metrics auth delegation, inference extension reads).
- Bump
github.com/otterscale/apitov0.9.0-alpha.3and remove theModelServicesample manifest.
Reviewed changes
Copilot reviewed 5 out of 6 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| internal/modelartifact/job.go | Switches job pipeline to a fixed intermediate tag (model:latest) for subsequent steps. |
| internal/controller/modelservice_controller.go | Adds additional kubebuilder RBAC annotations that feed generated RBAC. |
| config/rbac/role.yaml | Updates manager ClusterRole rules to match new/added RBAC requirements. |
| go.mod | Bumps github.com/otterscale/api dependency to an alpha release. |
| go.sum | Updates checksums for the dependency bump. |
| config/samples/model_v1alpha1_modelservice.yaml | Removes the sample manifest. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| github.com/onsi/ginkgo/v2 v2.28.1 | ||
| github.com/onsi/gomega v1.39.1 | ||
| github.com/otterscale/api v0.8.23 | ||
| github.com/otterscale/api v0.9.0-alpha.3 |
There was a problem hiding this comment.
This PR is titled as a job-handling/tagging fix, but it also bumps github.com/otterscale/api to a new alpha version and introduces broad RBAC/manifest changes. Please either (a) explain in the PR description why the API upgrade and RBAC changes are required for the tagging fix, or (b) split these into separate PRs to keep the scope and risk easier to review.
| kit unpack -o ${PLAIN_HTTP:+--plain-http} model:latest -d /workspace | ||
| kit pack . --use-model-pack -t "$OCI_TARGET" | ||
| else | ||
| kit tag "$import_tag" "$OCI_TARGET" | ||
| kit tag model:latest "$OCI_TARGET" | ||
| fi |
There was a problem hiding this comment.
The pipeline script now uses a fixed intermediate tag (model:latest) in both branches, but the header comment above still says the ModelKit path “import tags as OCI_TARGET directly”. Please update that comment (or adjust the script) so the documented flow matches the actual commands.
No description provided.