feat: add git-based skill fetching with shared auth and lightweight init image#1365
feat: add git-based skill fetching with shared auth and lightweight init image#1365
Conversation
Add support for cloning skills from Git repositories in agent init containers. A single gitAuthSecretRef on SkillForAgent applies to all gitRefs entries, matching the pattern used by InsecureSkipVerify for OCI skill refs. - Add GitRepo type and GitAuthSecretRef field to SkillForAgent CRD - Add git-init.sh.tmpl script template for cloning repos - Add buildGitSkillsInitContainer to create the init container - Support branch, tag, commit SHA refs, subdirectory paths, and custom skill names - Add configurable git init image (--git-init-image flag) - Add unit and golden tests Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> Signed-off-by: Eitan Yarmush <eitan.yarmush@solo.io>
f5a19ac to
19173b7
Compare
There was a problem hiding this comment.
Pull request overview
This PR adds support for fetching agent skills from Git repositories with optional shared authentication, in addition to the existing OCI registry support. The implementation introduces a new unified skills-init container that handles both Git and OCI skill sources, replacing the previous approach that used the agent image itself for skill pulling.
Changes:
- Adds
gitRefsandgitAuthSecretReffields to theSkillForAgentCRD to support Git-based skills with optional authentication - Creates a new
skills-initDocker image with Git and krane (OCI tool) for fetching skills - Implements a unified init container that handles both Git clones and OCI pulls via a shell script template
- Removes the
minItems: 1constraint from therefsfield to allow Git-only skill configurations
Reviewed changes
Copilot reviewed 40 out of 41 changed files in this pull request and generated 13 comments.
Show a summary per file
| File | Description |
|---|---|
| go/api/v1alpha2/agent_types.go | Adds GitRepo type and new fields (gitRefs, gitAuthSecretRef) to SkillForAgent |
| go/config/crd/bases/kagent.dev_agents.yaml | Updates CRD schema with new git skill fields and removes minItems constraint |
| helm/kagent-crds/templates/kagent.dev_agents.yaml | Mirrors CRD schema changes |
| go/internal/controller/translator/agent/adk_api_translator.go | Implements unified skills init container with git and OCI support |
| go/internal/controller/translator/agent/skills-init.sh.tmpl | Shell script template for fetching skills from Git and OCI |
| docker/skills-init/Dockerfile | New Docker image with git and krane tools |
| go/pkg/app/app.go | Adds command-line flags for skills-init image configuration |
| helm/kagent/values.yaml | Adds skillsInitImage configuration section |
| helm/kagent/templates/controller-configmap.yaml | Adds environment variables for skills-init image configuration |
| Makefile | Adds build target for skills-init image and updates push target |
| go/internal/controller/translator/agent/git_skills_test.go | Comprehensive test coverage for git skills functionality |
| go/internal/controller/translator/agent/testdata/* | Test fixtures and golden files for git skills |
| .gitignore | Changes env/ pattern to .env/ |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Replace two separate init containers (git-skills-init using alpine/git and skills-init using the full ~1GB kagent runtime image) with a single skills-init container using a new lightweight image (~30MB) containing only git and krane. OCI skill pulling now uses krane directly in a shell script instead of shelling out through Python, eliminating the need to pull the full runtime image just to run a Go binary. - Add docker/skills-init/Dockerfile (alpine + git + krane) - Add unified skills-init.sh.tmpl handling both git and OCI skills - Merge buildGitSkillsInitContainer and OCI init into buildSkillsInitContainer - Add ImageConfig.Image() method and DefaultSkillsInitImageConfig - Add ociSkillName() helper for extracting skill names from OCI refs - Rename git-init-image flag/config to structured skills-init-image-* - Add build-skills-init target to Makefile - Regenerate CRDs and golden test files Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> Signed-off-by: Eitan Yarmush <eitan.yarmush@solo.io>
19173b7 to
1403147
Compare
Signed-off-by: Eitan Yarmush <eitan.yarmush@solo.io>
- Use heredoc variable assignments in shell template instead of inline single-quoted values to prevent shell injection via values containing single quotes - Add echo logging before git clone and krane export operations - Stop suppressing ssh-keyscan stderr (remove 2>/dev/null) - Validate SubPath: reject absolute paths and ".." traversal segments - Detect duplicate skill directory names across git and OCI refs - Use url.Parse in gitSkillName to strip query params and fragments - Add unit tests for ociSkillName, gitSkillName, validateSubPath, and duplicate name detection - Remove unused Makefile push target Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> Signed-off-by: Eitan Yarmush <eitan.yarmush@solo.io>
Without this, users could create a Skills object with neither OCI refs nor git refs, resulting in unnecessary init containers and volumes. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> Signed-off-by: Eitan Yarmush <eitan.yarmush@solo.io>
go/api/v1alpha2/agent_types.go
Outdated
| AllowedNamespaces *AllowedNamespaces `json:"allowedNamespaces,omitempty"` | ||
| } | ||
|
|
||
| // +kubebuilder:validation:XValidation:rule="size(self.refs) > 0 || size(self.gitRefs) > 0",message="at least one of refs or gitRefs must be specified" |
There was a problem hiding this comment.
nit: You can use AtLeastOneOf here.
|
|
||
| RUN CGO_ENABLED=0 go build -trimpath -ldflags="-s -w" -o /build/krane . | ||
|
|
||
| ### Stage 1: runtime |
There was a problem hiding this comment.
nit: No need for comments here. They're implicit based on FROM structuring.
| InsecureSkipVerify bool `json:"insecureSkipVerify,omitempty"` | ||
|
|
||
| // The list of skill images to fetch. | ||
| // +kubebuilder:validation:MinItems=1 |
There was a problem hiding this comment.
Why remove this? If it's +optional, you can still enforce "at least one item should be present".
| // Git reference: branch name, tag, or commit SHA. | ||
| // +optional | ||
| // +kubebuilder:default="main" | ||
| Ref string `json:"ref,omitempty"` | ||
|
|
||
| // Subdirectory within the repo to use as the skill root. | ||
| // +optional | ||
| Path string `json:"path,omitempty"` | ||
|
|
||
| // Name for the skill directory under /skills. Defaults to the repo name. | ||
| // +optional | ||
| Name string `json:"name,omitempty"` |
There was a problem hiding this comment.
Take a peek at https://github.com/kubernetes-sigs/kube-api-linter/. All +optional fields should be a pointer to a string. We can also take this opportunity to define simple validation checks too, e.g. minLength, patterns, etc.
| GitAuthSecretRef *corev1.LocalObjectReference `json:"gitAuthSecretRef,omitempty"` | ||
|
|
||
| // Git repositories to fetch skills from. | ||
| // +kubebuilder:validation:MaxItems=20 |
There was a problem hiding this comment.
No, because it can be either gitRefs or refs
| // Reference to a Secret containing git credentials. | ||
| // Applied to all gitRefs entries. | ||
| // The secret should contain a `token` key for HTTPS auth, | ||
| // or `ssh-privatekey` for SSH auth. | ||
| // +optional | ||
| GitAuthSecretRef *corev1.LocalObjectReference `json:"gitAuthSecretRef,omitempty"` |
There was a problem hiding this comment.
This is slightly awkward to me. The intention is that this field decorates the gitRefs field? Couldn't we achieve the same thing with a top-level field where we decorate references with auth instead?
There was a problem hiding this comment.
I agree that this API is awkward. I think unfortunately this will be a v1alpha3 type of thing. Ideally we'd have a list of items which have a oneof. Either OCI refs or GitHub refs. The issue is that the auth mechanisms are different so I don't think we can realistically use the same secret for both which is very annoying
| if hasSkills { | ||
| insecure := agent.Spec.Skills != nil && agent.Spec.Skills.InsecureSkipVerify | ||
| container, skillsVolumes, err := buildSkillsInitContainer(gitRefs, gitAuthSecretRef, skills, insecure, dep.SecurityContext) | ||
| if err != nil { | ||
| return nil, fmt.Errorf("failed to build skills init container: %w", err) | ||
| } | ||
| initContainers = append(initContainers, container) | ||
| volumes = append(volumes, skillsVolumes...) | ||
| } |
There was a problem hiding this comment.
nit: can't we combine these two if hasSkills conditionals?
| func validateSubPath(p string) error { | ||
| if p == "" { | ||
| return nil | ||
| } | ||
| if path.IsAbs(p) { | ||
| return fmt.Errorf("skill subPath must be relative, got %q", p) | ||
| } | ||
| if slices.Contains(strings.Split(p, "/"), "..") { | ||
| return fmt.Errorf("skill subPath must not contain '..', got %q", p) | ||
| } | ||
| return nil | ||
| } |
There was a problem hiding this comment.
I feel like this is the sort of thing that if we use kube validation for on the API would be much more explicit and reduce the need to maintain and test such code.
There was a problem hiding this comment.
I agree in theory, but the logic here is a bit too involved for cel I think. Too much string manipulation
| name := gitSkillName(ref) | ||
| if seen[name] { | ||
| return skillsInitData{}, fmt.Errorf("duplicate skill directory name %q", name) | ||
| } |
There was a problem hiding this comment.
Would it be possible to use kube validation here to ensure that the individual gitRef items are unique? This should allow us to never have to worry about duplicate skill directory names and simplify this code.
There was a problem hiding this comment.
I think combining data from 2 lists and then checking uniqueness is a bit too complex for CEL. Most likely we should do best effort here, but I think we can start with this. What do you think?
There was a problem hiding this comment.
Do the directory names need to be defined by the user? What if we just generated them for the user in a similar way to how go mod downloaded libraries work. Splitting the ref into a folder structure like: /skills/<org>/<repo>@<tag>?
There was a problem hiding this comment.
It needs to be skills/<skill-name> because of how skills are organized on the filesystem
Signed-off-by: Eitan Yarmush <eitan.yarmush@solo.io>
Summary
gitAuthSecretRefonSkillForAgentthat applies credentials to allgitRefsentries (supports both HTTPS token and SSH key auth)skills-initimage (~30MB) containing onlygitandkranesubPathtraversal and duplicate skill namesNew CRD Fields
Auth Secret Format
The secret referenced by
gitAuthSecretRefshould contain either:tokenkey — for HTTPS credential helper (x-access-token)ssh-privatekeykey — for SSH-based cloningChanges by Area
CRD (
go/api/v1alpha2/agent_types.go)GitRepotype withurl,ref,path, andnamefieldsgitAuthSecretRefandgitRefsfields toSkillForAgentController / Translator (
go/internal/controller/translator/agent/)buildSkillsInitContainer— unified init container for both git and OCI skillsskills-init.sh.tmpl— shell script template using heredoc variables to prevent injectionvalidateSubPath()— rejects absolute paths and..traversalgitSkillName()withurl.Parse()to strip query params/fragmentsociSkillName()for extracting names from OCI refsDefaultSkillsInitImageConfigandImageConfig.Image()methodLightweight Init Image (
docker/skills-init/)kranefrom source, produces a minimal Alpine image with justgit+kraneHelm
skillsInitImageconfig (registry,repository,tag,pullPolicy) tovalues.yamlTests
agent_with_git_skills(git + OCI + auth + subpath + commit SHA)ociSkillName,gitSkillName,validateSubPath, and duplicate name detectionTest plan
cd go && go build ./...compilescd go && go test ./internal/controller/translator/agent/... -count=1— all tests passUPDATE_GOLDEN=trueto regenerate)gitRefsand verify skills are cloned🤖 Generated with Claude Code