-
Notifications
You must be signed in to change notification settings - Fork 56
Remove dependency on github.com/opencontainers/runtime-tools
#316
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -25,7 +25,6 @@ import ( | |
| "strings" | ||
|
|
||
| oci "github.com/opencontainers/runtime-spec/specs-go" | ||
| ocigen "github.com/opencontainers/runtime-tools/generate" | ||
| cdi "tags.cncf.io/container-device-interface/specs-go" | ||
| ) | ||
|
|
||
|
|
@@ -80,9 +79,11 @@ func (e *ContainerEdits) Apply(spec *oci.Spec) error { | |
| return nil | ||
| } | ||
|
|
||
| specgen := ocigen.NewFromSpec(spec) | ||
| if len(e.Env) > 0 { | ||
| specgen.AddMultipleProcessEnv(e.Env) | ||
| if spec.Process == nil { | ||
| spec.Process = &oci.Process{} | ||
| } | ||
| addMultipleProcessEnv(spec.Process, e.Env) | ||
| } | ||
|
|
||
| for _, d := range e.DeviceNodes { | ||
|
|
@@ -104,8 +105,11 @@ func (e *ContainerEdits) Apply(spec *oci.Spec) error { | |
| } | ||
| } | ||
|
|
||
| specgen.RemoveDevice(dev.Path) | ||
| specgen.AddDevice(dev) | ||
| if spec.Linux == nil { | ||
| spec.Linux = &oci.Linux{} | ||
| } | ||
| removeDevice(spec, dev.Path) | ||
| spec.Linux.Devices = append(spec.Linux.Devices, dev) | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Another inconsistency is that
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Is that something you'd like me to update the PR for/with, or just something you want to note? 🙇
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. It was initially just a note from my side, but what about adding an
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. (similarly for |
||
|
|
||
| if dev.Type == "b" || dev.Type == "c" { | ||
| access := d.Permissions | ||
|
|
@@ -115,80 +119,131 @@ func (e *ContainerEdits) Apply(spec *oci.Spec) error { | |
| case NoPermissions: | ||
| access = "" | ||
| } | ||
| specgen.AddLinuxResourcesDevice(true, dev.Type, &dev.Major, &dev.Minor, access) | ||
| if spec.Linux.Resources == nil { | ||
| spec.Linux.Resources = &oci.LinuxResources{} | ||
| } | ||
| spec.Linux.Resources.Devices = append(spec.Linux.Resources.Devices, oci.LinuxDeviceCgroup{ | ||
| Allow: true, | ||
| Type: dev.Type, | ||
| Major: &dev.Major, | ||
| Minor: &dev.Minor, | ||
| Access: access, | ||
| }) | ||
| } | ||
| } | ||
|
|
||
| if len(e.NetDevices) > 0 { | ||
| // specgen is currently missing functionality to set Linux NetDevices, | ||
| // so we use a locally rolled function for now. | ||
| for _, dev := range e.NetDevices { | ||
| specgenAddLinuxNetDevice(&specgen, dev.HostInterfaceName, (&LinuxNetDevice{dev}).toOCI()) | ||
| } | ||
| for _, dev := range e.NetDevices { | ||
| ensureLinuxNetDevices(spec) | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. If we kept the length check it would be more consistent and we could call |
||
| spec.Linux.NetDevices[dev.HostInterfaceName] = *(&LinuxNetDevice{dev}).toOCI() | ||
| } | ||
|
|
||
| if len(e.Mounts) > 0 { | ||
| for _, m := range e.Mounts { | ||
| mnt := &Mount{m} | ||
|
|
||
| specgen.RemoveMount(m.ContainerPath) | ||
| removeMount(spec, m.ContainerPath) | ||
|
|
||
| if !specHasUserNamespace(spec) { | ||
| specgen.AddMount(mnt.toOCI()) | ||
| spec.Mounts = append(spec.Mounts, mnt.toOCI()) | ||
| } else { | ||
| specgen.AddMount(mnt.toOCI(withIDMapForBindMount())) | ||
| spec.Mounts = append(spec.Mounts, mnt.toOCI(withIDMapForBindMount())) | ||
| } | ||
| } | ||
| sortMounts(&specgen) | ||
| sort.Stable(orderedMounts(spec.Mounts)) | ||
| } | ||
|
|
||
| for _, h := range e.Hooks { | ||
| ociHook := (&Hook{h}).toOCI() | ||
| ensureOCIHooks(spec) | ||
| switch h.HookName { | ||
| case PrestartHook: | ||
| specgen.AddPreStartHook(ociHook) | ||
| spec.Hooks.Prestart = append(spec.Hooks.Prestart, ociHook) //nolint:staticcheck | ||
| case PoststartHook: | ||
| specgen.AddPostStartHook(ociHook) | ||
| spec.Hooks.Poststart = append(spec.Hooks.Poststart, ociHook) | ||
| case PoststopHook: | ||
| specgen.AddPostStopHook(ociHook) | ||
| // TODO: Maybe runtime-tools/generate should be updated with these... | ||
| spec.Hooks.Poststop = append(spec.Hooks.Poststop, ociHook) | ||
| case CreateRuntimeHook: | ||
| ensureOCIHooks(spec) | ||
| spec.Hooks.CreateRuntime = append(spec.Hooks.CreateRuntime, ociHook) | ||
| case CreateContainerHook: | ||
| ensureOCIHooks(spec) | ||
| spec.Hooks.CreateContainer = append(spec.Hooks.CreateContainer, ociHook) | ||
| case StartContainerHook: | ||
| ensureOCIHooks(spec) | ||
| spec.Hooks.StartContainer = append(spec.Hooks.StartContainer, ociHook) | ||
| default: | ||
| return fmt.Errorf("unknown hook name %q", h.HookName) | ||
| } | ||
| } | ||
|
|
||
| if e.IntelRdt != nil { | ||
| // The specgen is missing functionality to set all parameters so we | ||
| // just piggy-back on it to initialize all structs and the copy over. | ||
| specgen.SetLinuxIntelRdtClosID(e.IntelRdt.ClosID) | ||
| if spec.Linux == nil { | ||
| spec.Linux = &oci.Linux{} | ||
| } | ||
| spec.Linux.IntelRdt = (&IntelRdt{e.IntelRdt}).toOCI() | ||
| } | ||
|
|
||
| for _, additionalGID := range e.AdditionalGIDs { | ||
| if additionalGID == 0 { | ||
| continue | ||
| } | ||
| specgen.AddProcessAdditionalGid(additionalGID) | ||
| if spec.Process == nil { | ||
| spec.Process = &oci.Process{} | ||
| } | ||
| addProcessAdditionalGid(spec, additionalGID) | ||
| } | ||
|
|
||
| return nil | ||
| } | ||
|
|
||
| func specgenAddLinuxNetDevice(specgen *ocigen.Generator, hostIf string, netDev *oci.LinuxNetDevice) { | ||
| if specgen == nil || netDev == nil { | ||
| // addMultipleProcessEnv adds or replaces environment variables on the process, | ||
| // deduplicating by key. | ||
| func addMultipleProcessEnv(process *oci.Process, envs []string) { | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. One thing that I want to call out (thanks Codex). There is a subtle difference between the new code and the original Note that there is also a slight mismatch to the wording of the CDI specification which states that envvars are appended to the environment. I do, however, think that the new behaviour is correct and should also remove ambiguity around dealing with multiple envvars. We may want to update the wording around evvars for the spec though.
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. See also opencontainers/runtime-tools#809 |
||
| EnvLoop: | ||
| for _, env := range envs { | ||
| parts := strings.SplitN(env, "=", 2) | ||
| if len(parts) < 2 { | ||
| continue | ||
| } | ||
| prefix := parts[0] + "=" | ||
| for i, e := range process.Env { | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Implementing a simplified generator type locally will allow us to cache the envvars to improve performance for containers with many envvars. (Using envvars in CDI is not common, so it may not be worth it though). |
||
| if strings.HasPrefix(e, prefix) { | ||
| process.Env[i] = env | ||
| continue EnvLoop | ||
| } | ||
| } | ||
| process.Env = append(process.Env, env) | ||
| } | ||
| } | ||
|
|
||
| // removeDevice removes the device at path from spec.Linux.Devices. | ||
| func removeDevice(spec *oci.Spec, path string) { | ||
| if spec.Linux == nil { | ||
| return | ||
| } | ||
| ensureLinuxNetDevices(specgen.Config) | ||
| specgen.Config.Linux.NetDevices[hostIf] = *netDev | ||
| for i, d := range spec.Linux.Devices { | ||
| if d.Path == path { | ||
| spec.Linux.Devices = append(spec.Linux.Devices[:i], spec.Linux.Devices[i+1:]...) | ||
| return | ||
| } | ||
| } | ||
| } | ||
|
|
||
| // removeMount removes the mount with the given destination from spec.Mounts. | ||
| func removeMount(spec *oci.Spec, dest string) { | ||
| for i, m := range spec.Mounts { | ||
| if m.Destination == dest { | ||
| spec.Mounts = append(spec.Mounts[:i], spec.Mounts[i+1:]...) | ||
| return | ||
| } | ||
| } | ||
| } | ||
|
|
||
| // addProcessAdditionalGid appends gid to spec.Process.User.AdditionalGids if not already present. | ||
| func addProcessAdditionalGid(spec *oci.Spec, gid uint32) { | ||
| for _, g := range spec.Process.User.AdditionalGids { | ||
| if g == gid { | ||
| return | ||
| } | ||
| } | ||
| spec.Process.User.AdditionalGids = append(spec.Process.User.AdditionalGids, gid) | ||
| } | ||
|
|
||
| // Ensure OCI Spec Linux NetDevices map is not nil. | ||
|
|
@@ -444,14 +499,6 @@ func ensureOCIHooks(spec *oci.Spec) { | |
| } | ||
| } | ||
|
|
||
| // sortMounts sorts the mounts in the given OCI Spec. | ||
| func sortMounts(specgen *ocigen.Generator) { | ||
| mounts := specgen.Mounts() | ||
| specgen.ClearMounts() | ||
| sort.Stable(orderedMounts(mounts)) | ||
| specgen.Config.Mounts = mounts | ||
| } | ||
|
|
||
| // orderedMounts defines how to sort an OCI Spec Mount slice. | ||
| // This is the almost the same implementation sa used by CRI-O and Docker, | ||
| // with a minor tweak for stable sorting order (easier to test): | ||
|
|
||
Uh oh!
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe this could be an
ensureProcess()for consistency.