Skip to content

feat(skills-init): add securityContext + resources configuration to i…#1539

Merged
EItanya merged 3 commits intokagent-dev:mainfrom
jholt96:fix-skills
Apr 6, 2026
Merged

feat(skills-init): add securityContext + resources configuration to i…#1539
EItanya merged 3 commits intokagent-dev:mainfrom
jholt96:fix-skills

Conversation

@jholt96
Copy link
Copy Markdown
Contributor

@jholt96 jholt96 commented Mar 24, 2026

…nitContainer

there are issues with the initContainer where if a limitRanger is set in the namespace then it can add a limit that is too low for the skills to be added. There is also an issue where the securityContext is not set for enterprise controls and slows adoption

Copilot AI review requested due to automatic review settings March 24, 2026 22:30
Copy link
Copy Markdown
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

This PR adds user-configurable resources and securityContext for the skills-init init container so agents can (a) avoid too-low defaults injected by namespace LimitRanger and (b) satisfy enterprise security controls without slowing adoption.

Changes:

  • Extends the Agent CRD schema to support spec.skills.initContainer.{resources,securityContext}.
  • Adds new API type SkillsInitContainer (plus deepcopy generation) to represent the init container config.
  • Updates the agent translator to apply the init container’s resources/securityContext (and to pass env/resources into the init container).

Reviewed changes

Copilot reviewed 5 out of 5 changed files in this pull request and generated 3 comments.

Show a summary per file
File Description
helm/kagent-crds/templates/kagent.dev_agents.yaml Adds CRD OpenAPI schema for skills.initContainer in the Helm-distributed CRDs.
go/api/config/crd/bases/kagent.dev_agents.yaml Adds the same CRD schema in the controller-generated CRD bases.
go/api/v1alpha2/agent_types.go Introduces SkillsInitContainer and wires it into SkillForAgent.
go/api/v1alpha2/zz_generated.deepcopy.go Adds deepcopy support for the new init container config type.
go/core/internal/controller/translator/agent/adk_api_translator.go Applies init container resources/securityContext and passes env/resources into skills-init.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment thread go/core/internal/controller/translator/agent/adk_api_translator.go Outdated
Comment thread go/core/internal/controller/translator/agent/adk_api_translator.go Outdated

insecure := agent.Spec.Skills != nil && agent.Spec.Skills.InsecureSkipVerify
container, skillsVolumes, err := buildSkillsInitContainer(gitRefs, gitAuthSecretRef, skills, insecure, dep.SecurityContext)
initEnv := append(dep.Env, sharedEnv...)
Copy link

Copilot AI Mar 24, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

initEnv := append(dep.Env, sharedEnv...) propagates all user-specified deployment env vars (including potential SecretKeyRefs) into the skills-init init container. Since init containers often run with different permissions/attack surface, consider limiting init container env to only what it needs (e.g., shared/proxy-related env) or adding an explicit skills.initContainer.env field rather than inheriting dep.Env wholesale.

Suggested change
initEnv := append(dep.Env, sharedEnv...)
initEnv := make([]corev1.EnvVar, 0, len(sharedEnv))
initEnv = append(initEnv, sharedEnv...)

Copilot uses AI. Check for mistakes.
@jholt96 jholt96 force-pushed the fix-skills branch 2 times, most recently from bd8e049 to dca4fd4 Compare March 25, 2026 13:10
@jholt96
Copy link
Copy Markdown
Contributor Author

jholt96 commented Mar 27, 2026

Just to add more context on this, We have strict securityContext requirements for all containers and this was blocking us from being able to properly deploy this out for our clusters. The same for the resources block being a requirement. Lmk if there are any changes!

Copy link
Copy Markdown
Contributor

@EItanya EItanya left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So I think a much simpler fix here could just use a restricted security context and small resources as the default rather than making this configurable. What do you think?

@jholt96
Copy link
Copy Markdown
Contributor Author

jholt96 commented Mar 30, 2026

So I think a much simpler fix here could just use a restricted security context and small resources as the default rather than making this configurable. What do you think?

I think a standard securityContext would be totally fine with me if thats the direction we want. I was trying to keep it configurable as that was the behavior before. I can update the PR with that!

For the resources, I actually saw the initContainer go OOM from the limit ranger setting too low of limits. We were having it pull about 10 skills folders which is when we were seeing that happen. Granted they were unnecessarily bloated folders but I think it might make sense to keep this configurable because eventually someone will hit the limits we set.

The other change in here is populating the envs for the initContainer. I can break that out into a separate PR if you want but we have github access behind corporate proxy so we needed to set those envs.

Lmk what you think and I can adjust the PR from there!

@EItanya
Copy link
Copy Markdown
Contributor

EItanya commented Mar 30, 2026

So I think a much simpler fix here could just use a restricted security context and small resources as the default rather than making this configurable. What do you think?

I think a standard securityContext would be totally fine with me if thats the direction we want. I was trying to keep it configurable as that was the behavior before. I can update the PR with that!

For the resources, I actually saw the initContainer go OOM from the limit ranger setting too low of limits. We were having it pull about 10 skills folders which is when we were seeing that happen. Granted they were unnecessarily bloated folders but I think it might make sense to keep this configurable because eventually someone will hit the limits we set.

The other change in here is populating the envs for the initContainer. I can break that out into a separate PR if you want but we have github access behind corporate proxy so we needed to set those envs.

Lmk what you think and I can adjust the PR from there!

Ok, I buy that, but I do think we should probably split the env vars, and maybe have separate vars because I don't think we necessarily need to set all of the env vars from the main container, what do you think?

@jholt96
Copy link
Copy Markdown
Contributor Author

jholt96 commented Mar 31, 2026

So I think a much simpler fix here could just use a restricted security context and small resources as the default rather than making this configurable. What do you think?

I think a standard securityContext would be totally fine with me if thats the direction we want. I was trying to keep it configurable as that was the behavior before. I can update the PR with that!
For the resources, I actually saw the initContainer go OOM from the limit ranger setting too low of limits. We were having it pull about 10 skills folders which is when we were seeing that happen. Granted they were unnecessarily bloated folders but I think it might make sense to keep this configurable because eventually someone will hit the limits we set.
The other change in here is populating the envs for the initContainer. I can break that out into a separate PR if you want but we have github access behind corporate proxy so we needed to set those envs.
Lmk what you think and I can adjust the PR from there!

Ok, I buy that, but I do think we should probably split the env vars, and maybe have separate vars because I don't think we necessarily need to set all of the env vars from the main container, what do you think?

Makes sense to me will get the PR updated

…nitContainer

there are issues with the initContainer where if a limitRanger is set in the namespace then it can add a limit that is too low for the skills to be added. There is also an issue where the securityContext is not set for enterprise controls and slows adoption

Signed-off-by: Josh Holt <jholt96@live.com>
…or the initContainer

Signed-off-by: Josh Holt <jholt96@live.com>
Signed-off-by: Josh Holt <jholt96@live.com>
@jholt96
Copy link
Copy Markdown
Contributor Author

jholt96 commented Apr 5, 2026

So I think a much simpler fix here could just use a restricted security context and small resources as the default rather than making this configurable. What do you think?

I think a standard securityContext would be totally fine with me if thats the direction we want. I was trying to keep it configurable as that was the behavior before. I can update the PR with that!
For the resources, I actually saw the initContainer go OOM from the limit ranger setting too low of limits. We were having it pull about 10 skills folders which is when we were seeing that happen. Granted they were unnecessarily bloated folders but I think it might make sense to keep this configurable because eventually someone will hit the limits we set.
The other change in here is populating the envs for the initContainer. I can break that out into a separate PR if you want but we have github access behind corporate proxy so we needed to set those envs.
Lmk what you think and I can adjust the PR from there!

Ok, I buy that, but I do think we should probably split the env vars, and maybe have separate vars because I don't think we necessarily need to set all of the env vars from the main container, what do you think?

Sorry for the delay, but the PR is updated. The changes are now:

  1. add Resource Config
  2. add Env Config
  3. revert the securityContext as I think this PR actually fixes the issues seen.

@jholt96 jholt96 requested a review from EItanya April 5, 2026 17:43
@EItanya EItanya merged commit bf4640a into kagent-dev:main Apr 6, 2026
22 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants