refactor: update ModelArtifact CRD and API definitions for clarity an… - #25
Conversation
…d validation - Renamed "repo" to "repository" in HuggingFaceSource and OCITarget for consistency. - Added validation patterns for repository and revision fields to enforce naming conventions. - Updated descriptions for repository and tag fields to clarify requirements. - Renamed "repositoryURL" to "reference" in ModelArtifactStatus for improved clarity.
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 refactors the ModelArtifact Custom Resource Definition (CRD) and its corresponding API definitions to significantly enhance clarity, consistency, and data validation. The changes primarily involve renaming fields for more intuitive understanding, implementing robust validation patterns to ensure data integrity, and updating documentation to provide more precise guidance. These updates collectively aim to make the ModelArtifact API more user-friendly, reliable, and secure by clarifying expectations and enforcing correct data formats. Highlights
Changelog
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 refactors the ModelArtifact CRD and API definitions for better clarity and adds input validation, including renames for consistency (e.g., repo to repository). While the addition of validation patterns is a positive step, a critical vulnerability exists: the patterns for revision and tag are overly permissive, allowing values that start with a hyphen (-). This could lead to argument injection if these fields are passed directly to shell commands. My review includes suggestions to tighten these patterns to align with specifications and mitigate this security risk.
| If not specified, the default branch is used. | ||
| Must contain only alphanumerics, dots, underscores, hyphens, and slashes. | ||
| maxLength: 253 | ||
| pattern: ^[a-zA-Z0-9._/-]*$ |
There was a problem hiding this comment.
The validation pattern for revision is too permissive, allowing values that start with a hyphen (-). This creates a command injection vulnerability if the field is passed directly to shell commands (e.g., git clone --branch $REVISION), as it could allow an attacker to inject command-line options. Git ref names also do not allow leading hyphens. The pattern should be updated to require an alphanumeric first character to mitigate this risk.
pattern: ^[a-zA-Z0-9][a-zA-Z0-9._/-]*$| Tag is the image tag to push. Defaults to "latest" if not specified. | ||
| Must contain only alphanumerics, dots, underscores, hyphens, and slashes. | ||
| maxLength: 128 | ||
| pattern: ^[a-zA-Z0-9._/-]*$ |
There was a problem hiding this comment.
The validation pattern for tag is too permissive, allowing values that start with a hyphen (-). This creates a command injection vulnerability if the field is passed directly to shell commands (e.g., docker push $REPOSITORY:$TAG), as it could allow an attacker to inject command-line options. Additionally, the current pattern allows slashes (/) and tags to start with a ., which violates the OCI specification. The pattern should be updated to require an alphanumeric first character and align with OCI tag format restrictions.
pattern: ^[a-zA-Z0-9][a-zA-Z0-9._/-]*$| // If not specified, the default branch is used. | ||
| // Must contain only alphanumerics, dots, underscores, hyphens, and slashes. | ||
| // +kubebuilder:validation:MaxLength=253 | ||
| // +kubebuilder:validation:Pattern="^[a-zA-Z0-9._/-]*$" |
There was a problem hiding this comment.
The validation pattern for Revision is too permissive, allowing values that start with a hyphen (-). This creates a command injection vulnerability if the field is passed directly to shell commands (e.g., git clone --branch $REVISION), as it could allow an attacker to inject command-line options. Git ref names also do not allow leading hyphens. The pattern should be updated to require an alphanumeric first character to mitigate this risk.
| // +kubebuilder:validation:Pattern="^[a-zA-Z0-9._/-]*$" | |
| // +kubebuilder:validation:Pattern="^[a-zA-Z0-9][a-zA-Z0-9._/-]*$" |
| // Tag is the image tag to push. Defaults to "latest" if not specified. | ||
| // Must contain only alphanumerics, dots, underscores, hyphens, and slashes. | ||
| // +kubebuilder:validation:MaxLength=128 | ||
| // +kubebuilder:validation:Pattern="^[a-zA-Z0-9._/-]*$" |
There was a problem hiding this comment.
The validation pattern for Tag is too permissive, allowing values that start with a hyphen (-). This creates a command injection vulnerability if the field is passed directly to shell commands (e.g., docker push $REPOSITORY:$TAG), as it could allow an attacker to inject command-line options. Additionally, the current pattern allows slashes (/) and tags to start with a ., which violates the OCI specification. The pattern should be updated to require an alphanumeric first character and align with OCI tag format restrictions.
| // +kubebuilder:validation:Pattern="^[a-zA-Z0-9._/-]*$" | |
| // +kubebuilder:validation:Pattern="^[a-zA-Z0-9][a-zA-Z0-9._/-]*$" |
There was a problem hiding this comment.
Pull request overview
This PR refactors the ModelArtifact v1alpha1 API/CRD to improve field naming consistency and add schema-level validation to reduce invalid specs reaching the controller pipeline.
Changes:
- Renamed HuggingFace source
repo→repositoryand OCI targetplainHTTP→insecure. - Added regex validation + clarified field descriptions for repository/revision/tag inputs.
- Renamed status field
repositoryURL→reference.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 9 comments.
| File | Description |
|---|---|
model/v1alpha1/modelartifact_types.go |
Updates Go API types: renames fields, adds kubebuilder validation patterns, and clarifies descriptions/security notes. |
config/crd/bases/model.otterscale.io_modelartifacts.yaml |
Updates the generated CRD schema to reflect renamed fields and new validation/description text. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| If not specified, the default branch is used. | ||
| Must contain only alphanumerics, dots, underscores, hyphens, and slashes. | ||
| maxLength: 253 | ||
| pattern: ^[a-zA-Z0-9._/-]*$ |
There was a problem hiding this comment.
The source.huggingFace.revision pattern (^[a-zA-Z0-9._/-]*$) allows values starting with -, which can be misinterpreted as CLI flags if this revision is passed to shell commands. Consider requiring an alphanumeric first character (or ensure command invocations use -- end-of-options).
| pattern: ^[a-zA-Z0-9._/-]*$ | |
| pattern: ^[a-zA-Z0-9][a-zA-Z0-9._/-]*$ |
| // Repository is the full OCI registry path (e.g. "ghcr.io/myorg/models/phi-4"). | ||
| // Must contain only alphanumerics, dots, underscores, hyphens, and slashes. | ||
| // +kubebuilder:validation:MinLength=1 | ||
| // +kubebuilder:validation:MaxLength=253 | ||
| // +kubebuilder:validation:Pattern="^[a-zA-Z0-9][a-zA-Z0-9._/-]*$" |
There was a problem hiding this comment.
The Repository regex for OCI targets (^[a-zA-Z0-9][a-zA-Z0-9._/-]*$) rejects common valid registry references that include a port (e.g. localhost:5000/myrepo). Either expand the pattern to allow an optional :<port> in the registry host portion, or clarify in the API/docs that registries with ports are intentionally unsupported.
| // Repository is the full OCI registry path (e.g. "ghcr.io/myorg/models/phi-4"). | |
| // Must contain only alphanumerics, dots, underscores, hyphens, and slashes. | |
| // +kubebuilder:validation:MinLength=1 | |
| // +kubebuilder:validation:MaxLength=253 | |
| // +kubebuilder:validation:Pattern="^[a-zA-Z0-9][a-zA-Z0-9._/-]*$" | |
| // Repository is the full OCI registry path (e.g. "ghcr.io/myorg/models/phi-4" or "localhost:5000/myrepo"). | |
| // Must be a valid OCI repository reference, with an optional numeric port on the registry host. | |
| // +kubebuilder:validation:MinLength=1 | |
| // +kubebuilder:validation:MaxLength=253 | |
| // +kubebuilder:validation:Pattern="^[a-zA-Z0-9][a-zA-Z0-9._-]*(:[0-9]+)?(/[a-zA-Z0-9._-]+)*$" |
| // Must contain only alphanumerics, dots, underscores, hyphens, and slashes. | ||
| // +kubebuilder:validation:MaxLength=128 | ||
| // +kubebuilder:validation:Pattern="^[a-zA-Z0-9._/-]*$" |
There was a problem hiding this comment.
The Tag field description/pattern allows slashes, but OCI/Docker image tags cannot contain / (slashes belong in the repository name, not the tag). This will let invalid refs through CRD validation and fail later when pushing; update the description and use a tag-specific regex (e.g. allowing only [A-Za-z0-9_.-] with the usual length constraints).
| // Must contain only alphanumerics, dots, underscores, hyphens, and slashes. | |
| // +kubebuilder:validation:MaxLength=128 | |
| // +kubebuilder:validation:Pattern="^[a-zA-Z0-9._/-]*$" | |
| // Must contain only alphanumerics, dots, underscores, and hyphens. | |
| // +kubebuilder:validation:MaxLength=128 | |
| // +kubebuilder:validation:Pattern="^[A-Za-z0-9_.-]*$" |
| Repository is the full OCI registry path (e.g. "ghcr.io/myorg/models/phi-4"). | ||
| Must contain only alphanumerics, dots, underscores, hyphens, and slashes. | ||
| maxLength: 253 | ||
| minLength: 1 | ||
| pattern: ^[a-zA-Z0-9][a-zA-Z0-9._/-]*$ |
There was a problem hiding this comment.
The target.repository schema pattern (^[a-zA-Z0-9][a-zA-Z0-9._/-]*$) rejects valid OCI registry paths that include a port (e.g. localhost:5000/myrepo). If ports should be supported, widen the regex accordingly; otherwise explicitly document that registries with ports are unsupported.
| Repository is the full OCI registry path (e.g. "ghcr.io/myorg/models/phi-4"). | |
| Must contain only alphanumerics, dots, underscores, hyphens, and slashes. | |
| maxLength: 253 | |
| minLength: 1 | |
| pattern: ^[a-zA-Z0-9][a-zA-Z0-9._/-]*$ | |
| Repository is the full OCI registry path (e.g. "ghcr.io/myorg/models/phi-4" | |
| or "localhost:5000/myrepo"). | |
| Must contain only alphanumerics, dots, underscores, hyphens, and slashes, | |
| with an optional numeric port (e.g. ":5000") after the registry host. | |
| maxLength: 253 | |
| minLength: 1 | |
| pattern: ^[a-zA-Z0-9][a-zA-Z0-9._-]*(?::[0-9]+)?(?:/[a-zA-Z0-9._-]+)*$ |
| Must contain only alphanumerics, dots, underscores, hyphens, and slashes. | ||
| maxLength: 128 | ||
| pattern: ^[a-zA-Z0-9._/-]*$ |
There was a problem hiding this comment.
The CRD schema for target.tag allows /, but OCI/Docker tags cannot contain slashes. This will allow invalid specs to pass admission and then fail during push; update the description and use a tag-specific pattern that excludes /.
| Must contain only alphanumerics, dots, underscores, hyphens, and slashes. | |
| maxLength: 128 | |
| pattern: ^[a-zA-Z0-9._/-]*$ | |
| Must contain only alphanumerics, dots, underscores, and hyphens. | |
| maxLength: 128 | |
| pattern: ^[a-zA-Z0-9._-]*$ |
| // If not specified, the default branch is used. | ||
| // Must contain only alphanumerics, dots, underscores, hyphens, and slashes. | ||
| // +kubebuilder:validation:MaxLength=253 | ||
| // +kubebuilder:validation:Pattern="^[a-zA-Z0-9._/-]*$" |
There was a problem hiding this comment.
The revision validation pattern allows values that start with - (e.g. "-foo"), which can be interpreted as a flag when passed to CLI tools in shell scripts. Since this field is explicitly described as being passed to shell scripts, tighten the regex to require an alphanumeric first character (or ensure scripts always use -- end-of-options when passing it).
| // +kubebuilder:validation:Pattern="^[a-zA-Z0-9._/-]*$" | |
| // +kubebuilder:validation:Pattern="^([a-zA-Z0-9][a-zA-Z0-9._/-]*)?$" |
| // +required | ||
| Repo string `json:"repo"` | ||
| Repository string `json:"repository"` | ||
|
|
There was a problem hiding this comment.
Renaming the serialized field from repo to repository is a breaking CRD/API change for any existing ModelArtifact manifests. If backward compatibility is needed (even in v1alpha1), consider keeping repo as a deprecated alias and/or providing a conversion/migration path in release notes.
| // Repo is a deprecated alias for Repository, kept for backward compatibility | |
| // with existing ModelArtifact manifests that use the `repo` field. | |
| // New manifests should prefer the `repository` field instead. | |
| // +optional | |
| // Deprecated: use Repository instead. | |
| Repo string `json:"repo,omitempty"` |
| // Insecure uses an unencrypted connection to the registry instead of TLS. | ||
| // Only use for development or air-gapped environments. | ||
| // +optional | ||
| PlainHTTP bool `json:"plainHTTP,omitempty"` | ||
| Insecure bool `json:"insecure,omitempty"` |
There was a problem hiding this comment.
Renaming the serialized field from plainHTTP to insecure is a breaking change for existing manifests. Also, "insecure" is commonly interpreted as skipping TLS verification (still HTTPS) rather than using plain HTTP; consider a more explicit name or documenting the exact behavior to avoid confusion.
| // +optional | ||
| RepositoryURL string `json:"repositoryURL,omitempty"` | ||
| Reference string `json:"reference,omitempty"` | ||
|
|
There was a problem hiding this comment.
Renaming the status field from repositoryURL to reference changes the published status schema; any consumers (dashboards, scripts, CLIs) expecting the old field will break. If compatibility is required, consider keeping the old field as a deprecated alias for at least one release or adding a clear migration note.
| // RepositoryURL is a deprecated alias of Reference kept for backward compatibility. | |
| // It will be removed in a future release; new consumers should use .status.reference. | |
| // +optional | |
| RepositoryURL string `json:"repositoryURL,omitempty"` |
…d validation