Conversation
1f5bf16 to
04da10b
Compare
There was a problem hiding this comment.
Pull request overview
This PR updates image templates for EMT PTL and Ubuntu PTL RC2 and introduces RPM “package pinning” support via an allowlist so only selected RPMs from a user repository are considered during metadata parsing.
Changes:
- Extend RPM repo metadata parsing to accept an optional package allowlist and plumb it through RPM user-repo download flow.
- Add
AllowPackagesto the template schema/config to express per-repository RPM allowlists. - Update Ubuntu 24 PTL template and add new EMT3 PTL EMF raw/rt-raw templates plus netplan additional files.
Reviewed changes
Copilot reviewed 10 out of 10 changed files in this pull request and generated 6 comments.
Show a summary per file
| File | Description |
|---|---|
| internal/ospackage/rpmutils/resolver.go | Adds allowlist filtering during RPM primary.xml parsing. |
| internal/ospackage/rpmutils/download.go | Threads per-repo allowlist from config into RPM metadata parsing. |
| internal/ospackage/rpmutils/resolver_test.go | Updates test call-site for the new ParseRepositoryMetadata signature. |
| internal/config/config.go | Adds AllowPackages to PackageRepository YAML config struct. |
| internal/config/schema/os-image-template.schema.json | Adds AllowPackages to template schema for repositories. |
| image-templates/ubuntu24-x86_64-minimal-ptl.yml | Updates Ubuntu PTL repo URL, package set, kernel config, and adds netplan additional files. |
| image-templates/emt3-x86_64-ptl-emf-raw.yml | New EMT3 PTL EMF raw template with RPM allowlist pinning. |
| image-templates/emt3-x86_64-ptl-emf-rt-raw.yml | New EMT3 PTL EMF-RT raw template with RPM allowlist pinning. |
| config/osv/ubuntu/ubuntu24/imageconfigs/additionalfiles/ptl/01-network-manager-all.yaml | Adds netplan config to use NetworkManager renderer. |
| config/osv/ubuntu/ubuntu24/imageconfigs/additionalfiles/ptl/50-cloud-init.yaml | Adds netplan config for DHCP on ens3 for cloud-init use. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| "AllowPackages": { | ||
| "type": "array", | ||
| "description": "Optional: specific packages to include from this repository (package pinning)", | ||
| "items": { | ||
| "type": "string", | ||
| "minLength": 1 | ||
| } | ||
| } |
There was a problem hiding this comment.
AllowPackages is the only property in PackageRepository that starts with an uppercase letter; the rest of the schema uses lower-case keys (e.g., codename, url, pkey). This inconsistency makes templates harder to author and is easy to miss. Consider renaming the schema property to allowPackages (and updating the Go yaml tag + templates accordingly) to match the established naming pattern.
| PKey string `yaml:"pkey"` // Public GPG key URL for verification | ||
| Component string `yaml:"component,omitempty"` // Repository component (e.g., "main", "restricted") | ||
| Priority int `yaml:"priority,omitempty"` // Repository priority (higher numbers = higher priority) | ||
| AllowPackages []string `yaml:"AllowPackages,omitempty"` // Optional: specific packages to include from this repo (pinning) |
There was a problem hiding this comment.
The YAML tag for AllowPackages is capitalized (yaml:"AllowPackages,omitempty") while all other fields in PackageRepository use lower-case keys (e.g., codename, url, pkey). To avoid surprising config behavior and keep templates consistent, rename the YAML tag (and corresponding schema/templates) to a lower-camel key like allowPackages.
| AllowPackages []string `yaml:"AllowPackages,omitempty"` // Optional: specific packages to include from this repo (pinning) | |
| AllowPackages []string `yaml:"allowPackages,omitempty"` // Optional: specific packages to include from this repo (pinning) |
| // Prefix match with version (e.g., "kernel-drivers-gpu-6.17.11" matches "kernel-drivers-gpu-6.17.11-1.emt3.x86_64") | ||
| if strings.HasPrefix(pkgName, pattern+"-") || strings.HasPrefix(pkgName, pattern) { | ||
| return true |
There was a problem hiding this comment.
matchesPackageFilter currently treats any string prefix as a match (strings.HasPrefix(pkgName, pattern)), which is broader than the comment implies (exact match or version-specific match). For example, a pattern glibc would also match glibc-common..., potentially letting in unintended packages when pinning. Consider enforcing a boundary after the pattern (e.g., exact match, or prefix followed by -/.) and/or updating the comment to match the actual behavior.
| // Prefix match with version (e.g., "kernel-drivers-gpu-6.17.11" matches "kernel-drivers-gpu-6.17.11-1.emt3.x86_64") | |
| if strings.HasPrefix(pkgName, pattern+"-") || strings.HasPrefix(pkgName, pattern) { | |
| return true | |
| // Prefix match with version, enforcing a boundary after the pattern. | |
| // For example, "kernel-drivers-gpu-6.17.11" matches "kernel-drivers-gpu-6.17.11-1.emt3.x86_64", | |
| // but "glibc" does not match "glibc-common-2.28-151.el8.x86_64". | |
| if strings.HasPrefix(pkgName, pattern) { | |
| if len(pkgName) == len(pattern) { | |
| // Already handled by exact match above, but kept for clarity. | |
| return true | |
| } | |
| next := pkgName[len(pattern)] | |
| if next == '-' || next == '.' { | |
| return true | |
| } |
|
|
||
| // Test ParseRepositoryMetadata | ||
| packages, err := ParseRepositoryMetadata(server.URL+"/", tt.filename) | ||
| packages, err := ParseRepositoryMetadata(server.URL+"/", tt.filename, nil) |
There was a problem hiding this comment.
A package filter was added to ParseRepositoryMetadata, but the tests only update the call-site and don't validate the new filtering behavior. Add a test case that passes a non-empty filter and asserts that only matching RPMs are returned (and non-matching ones are excluded).
| - local: ../additionalfiles/ptl/01-network-manager-all.yaml | ||
| final: /etc/netplan/01-network-manager-all.yaml | ||
| - local: ../additionalfiles/ptl/50-cloud-init.yaml |
There was a problem hiding this comment.
The additionalFiles.local paths resolve relative to the template directory; ../additionalfiles/ptl/... points to <repo>/additionalfiles/..., but the added netplan files are under config/osv/ubuntu/ubuntu24/imageconfigs/additionalfiles/ptl/. As written, these entries will be ignored at runtime (non-existent local path). Update the local: paths to the actual location, or move/copy the files into an additionalfiles/ptl/ directory adjacent to the templates.
| - local: ../additionalfiles/ptl/01-network-manager-all.yaml | |
| final: /etc/netplan/01-network-manager-all.yaml | |
| - local: ../additionalfiles/ptl/50-cloud-init.yaml | |
| - local: ../imageconfigs/additionalfiles/ptl/01-network-manager-all.yaml | |
| final: /etc/netplan/01-network-manager-all.yaml | |
| - local: ../imageconfigs/additionalfiles/ptl/50-cloud-init.yaml |
| version: "6.12" | ||
| cmdline: "root=/dev/mapper/rootfs_verity console=ttyS0,115200 console=tty0 loglevel=7 sysctl.vm.overcommit_memory=1 sysctl.kernel.panic=10 sysctl.kernel.panic_on_oops=1 sysctl.fs.inotify.max_user_instances=8192 rd.parallel=1 rd.shell=0 rd.timeout=200 rd.emergency=reboot" | ||
| enableExtraModules: "intel_vpu uas" | ||
| packages: | ||
| - kernel-rt-drivers-gpu No newline at end of file |
There was a problem hiding this comment.
The RT PTL template pins AllowPackages to kernel-rt-6.17.11 / kernel-rt-drivers-gpu-6.17.11, but the systemConfig.kernel.version is set to 6.12. With repo filtering enabled, this version mismatch is likely to exclude the intended kernel RPMs or make kernel selection ambiguous. Align kernel.version, kernel.packages, and AllowPackages to the same kernel series/version.
| version: "6.12" | |
| cmdline: "root=/dev/mapper/rootfs_verity console=ttyS0,115200 console=tty0 loglevel=7 sysctl.vm.overcommit_memory=1 sysctl.kernel.panic=10 sysctl.kernel.panic_on_oops=1 sysctl.fs.inotify.max_user_instances=8192 rd.parallel=1 rd.shell=0 rd.timeout=200 rd.emergency=reboot" | |
| enableExtraModules: "intel_vpu uas" | |
| packages: | |
| - kernel-rt-drivers-gpu | |
| version: "6.17.11" | |
| cmdline: "root=/dev/mapper/rootfs_verity console=ttyS0,115200 console=tty0 loglevel=7 sysctl.vm.overcommit_memory=1 sysctl.kernel.panic=10 sysctl.kernel.panic_on_oops=1 sysctl.fs.inotify.max_user_instances=8192 rd.parallel=1 rd.shell=0 rd.timeout=200 rd.emergency=reboot" | |
| enableExtraModules: "intel_vpu uas" | |
| packages: | |
| - kernel-rt-drivers-gpu-6.17.11 |
Merge Checklist
All boxes should be checked before merging the PR
Description
Any Newly Introduced Dependencies
How Has This Been Tested?