-
Notifications
You must be signed in to change notification settings - Fork 59
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
feat: add workloads to the plan #571
base: master
Are you sure you want to change the base?
Conversation
} else if s.workload != nil { | ||
// Take user/group config from workload | ||
uid, gid, err = osutil.NormalizeUidGid(s.workload.UserID, s.workload.GroupID, s.workload.User, s.workload.Group) | ||
} |
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.
What about the else
case?
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.
uid
, gid
are nil
, and setCmdCredential()
is not called, resulting in the services being run as the current user
@@ -378,6 +405,12 @@ func (s *serviceData) startInternal() error { | |||
} | |||
} | |||
|
|||
if s.workload != nil && len(s.workload.Environment) != 0 { | |||
for k, v := range s.workload.Environment { | |||
environment[k] = v |
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.
Do we allow every environment variable to be customized here? Just thought about this. I guess yes.
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.
Yes, we decided nothing against that when discussing the spec, but I'm happy to explore this. Personally, I don't think there's anything wrong per se with allowing every environment variable to be changed, but if you have arguments against that I'm happy to make them part of the spec discussion
Entries map[string]*Workload `yaml:",inline"` | ||
} | ||
|
||
func (ws *WorkloadsSection) IsZero() bool { |
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.
Suggestion to rename to IsEmpty
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.
Feels more fitting, however IsZero()
is already part of the Section
interface in the plan:
Lines 53 to 59 in d8a630a
type Section interface { | |
// Validate checks whether the section is valid, returning an error if not. | |
Validate() error | |
// IsZero reports whether the section is empty. | |
IsZero() bool | |
} |
} | ||
|
||
func (ws *WorkloadsSection) combine(other *WorkloadsSection) error { | ||
if len(other.Entries) != 0 && ws.Entries == nil { |
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.
!other.IsEmpty() && ws.Entries == nil
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.
This would potentially result in a nil pointer dereference in sneaky ways, since it's up to the implementation of IsZero()
to dereference or not the pointer receiver. len()
is special in that len(nil) == 0
for slices and maps.
if w.Name == "" { | ||
return errors.New("cannot have an empty name") | ||
} | ||
// Value of Override is checked in the (*WorkloadSection).combine() method |
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.
My intuition is that validate
should do that check regardless.
return errors.New("cannot have an empty name") | ||
} | ||
// Value of Override is checked in the (*WorkloadSection).combine() method | ||
return nil |
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.
Additionally, there could be a lot of invalid stuff in the Environment
map. Would it make sense to check here?
UserID *int `yaml:"user-id,omitempty"` | ||
User string `yaml:"user,omitempty"` | ||
GroupID *int `yaml:"group-id,omitempty"` | ||
Group string `yaml:"group,omitempty"` |
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.
Considering we allow specifying both user-id and username, or both group-id and group name, there's a possibility of a workload where the group-id doesn't actually match the group name and same for users. Where would we deal with that? I expect this will only get detected for sure in kernos bootstrap. Correct?
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.
This is detected in servstate
when calling osutil.NormalizeUidGid()
. An error may be returned if user
/user-id
or group
/group-id
does not match, as well as when specifying a user
/group
that does not exist in the system.
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.
Except for a couple of minor details this looks good to me. I'm still a bit fuzzy on how this will actually play out in bootstrap.
Workloads are a way to optionally group services and apply shared run configuration (user, group, environment) to services, in a way that may be extended in the future to support confinement.