feat(workloads): add plumbing to support workloads #558
Conversation
This is a preliminar patch that adds support for workloads in plan, plan manager and service manager by passing a workload.Provider instance from the daemon.New() constructor all the way down to servstate manager. This also requires splitting the plan.ReadDir() and (*Plan).Validate() methods, and passing the workload.Provider as an argument to Validate(), though this is a temporary measure to get the workloads feature working before refactoring the plan and service manager code to make services a plan section, rather than an ad-hoc member of the Plan struct. In the future, this patch needs to be refactored to store the Provider instance in a future service extension, when the services layer in the plan is refactored to be a plan section. This way, we can pass the workload.Provider from a plan extension and keep the current plan, daemon, and overlord APIs intact.
| @@ -249,7 +255,7 @@ func (m *PlanManager) updatePlanLayers(layers []*plan.Layer) (*plan.Plan, error) | |||
| LogTargets: combined.LogTargets, | |||
| Sections: combined.Sections, | |||
| } | |||
| err = p.Validate() | |||
| err = p.Validate(m.workloads) | |||
There was a problem hiding this comment.
Just a note for reviewers: Once services and checks are moved over to use the plan extension system, the workload definition can be supplied by the extension itself, and this signature change can be reverted so that the plan library is unaware of workloads.
| uid, gid, err := osutil.NormalizeUidGid(s.config.UserID, s.config.GroupID, s.config.User, s.config.Group) | ||
| if err != nil { | ||
| return err | ||
| w := s.manager.workloads[s.config.Workload] |
There was a problem hiding this comment.
Should we deal the a failure lookup here, even though in theory validation already happened ? If no workloads are supplied, the map lookup will return nothing and w will be nil? Also, what happens if the service did not define workload (empty string)?
| // ServicesField is the top-level string key used in the Pebble plan. | ||
| const ServicesField = "services" | ||
|
|
| @@ -30,7 +34,8 @@ type ServiceManager struct { | |||
| randLock sync.Mutex | |||
| rand *rand.Rand | |||
|
|
|||
| logMgr LogManager | |||
| workloads map[string]workload.Workload | |||
There was a problem hiding this comment.
Since workloads are the mechanism which will evolve over time to provide service level access permissions and confinement features, for me it makes sense that the Service Manager owns the configuration.
| if _, ok := workloads[service.Workload]; !ok { | ||
| return &FormatError{ | ||
| Message: fmt.Sprintf(`service %q cannot run in non-existing workload %q`, name, service.Workload), | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
Should we enforce the rule here that if workloads are defined, uid, user, gid and group properties of the service must be unset ?
There was a problem hiding this comment.
Perhaps, until this gets more flesh and there is a proven need (i.e. app armor and friends), this file could simply live inside servstate. However, looking at the dependency list, until we remove workloads from the plan into the servstate extension, this will not work as servstate imports plan already, and we would end up with a circular import between the plan and servstate.
| package workload | ||
|
|
||
| type Workload struct { | ||
| UID, GID *int |
There was a problem hiding this comment.
Seems to me those aren't optional, so I don't see why we'd use a *int here instead of int.
| w := s.manager.workloads[s.config.Workload] | ||
| uid, gid := w.UID, w.GID | ||
| if (uid == nil) != (gid == nil) { | ||
| panic("both uid and gid must be provided by workload") |
There was a problem hiding this comment.
This panic will be triggered if either uid or gid is nil but not both. The panic text doesn't match.
| panic("both uid and gid must be provided by workload") | ||
| } | ||
| if uid == nil { | ||
| uid, gid, err = osutil.NormalizeUidGid(s.config.UserID, s.config.GroupID, s.config.User, s.config.Group) |
There was a problem hiding this comment.
Why reassign gid here if we know we have an explicitly set value already?
Edit: nvm, in this branch we know that uid == nil && gid == nil. The code feels a bit obfuscated.
| if service.Workload != "" { | ||
| if workloads == nil { | ||
| return &FormatError{ | ||
| Message: fmt.Sprintf(`service %q cannot run in workload %q because workloads are not supported in %v`, name, service.Workload, cmd.DisplayName), |
There was a problem hiding this comment.
Is this always an accurate message in this case? What if we just didn't specify any workload but are using a program that supports them?
There was a problem hiding this comment.
I think this is worth further discussion. I can see how some wiring is necessary (the service manager needs to know the workload info when starting a service), but I wonder if there's a way to achieve it without quite so much plumbing.
Maybe we could have add a servstate.GetWorkloadFor(serviceName string) that we override for projects that need it.
Anyway, happy to discuss further on our call.
This is a preliminary patch that adds support for workloads in plan, plan manager and service manager by passing the workloads from the daemon.New() constructor all the way down to servstate manager. This also requires splitting the plan.ReadDir() and (*Plan).Validate() methods, and passing the workloads as an argument to Validate(), though this is a temporary measure to get the workloads feature working before refactoring the plan and service manager code to make services a plan section, rather than an ad-hoc member of the Plan struct.
In the future, this patch needs to be refactored to store the workloads in a future service extension, when the services layer in the plan is refactored to be a plan section. This way, we can pass the workloads from a plan extension and keep the current plan, daemon, and overlord APIs intact.