WithAppendAdditionalGroups: better /etc/group handling #9494
WithAppendAdditionalGroups: better /etc/group handling #9494estesp merged 1 commit intocontainerd:mainfrom
Conversation
oci/spec_opts.go
Outdated
| if err != nil { | ||
| return err | ||
| if !os.IsNotExist(err) { | ||
| return err | ||
| } |
There was a problem hiding this comment.
Perhaps make it a real one-line change;
if err != nil && !os.IsNotExist(err) {(could also use errors.Is(err, fs.ErrNotExist) / errors.Is(err, os.ErrNotExist))
There was a problem hiding this comment.
In other parts of the code os.IsNotExist(err) is used, I can switch though if you really want
There was a problem hiding this comment.
I'm fine with either os.IsNotExist(err) or errors.Is(); ISTR os.IsNotExist also had some code to handle some corner-cases for cgroups (which isn't handled by errors.Is), so changing it in other places would depend on what it's used for.
Was mostly looking at the "make it a one-line", as it slightly reduces cognitive overload (in already complex code). Not an absolute dealbreaker, but as we're changing it 🤷♂️
5745d20 to
ee4b602
Compare
ee4b602 to
b876a37
Compare
|
In a follow-up we should probably also look at other parts of the code that read |
b876a37 to
1cf3094
Compare
thaJeztah
left a comment
There was a problem hiding this comment.
LGTM, but left one more comment 🙈 (sorry, didn't notice that earlier)
oci/spec_opts.go
Outdated
| var ugroups []user.Group | ||
| var groupErr error | ||
| ugroups, groupErr = user.ParseGroupFile(gpath) | ||
| if groupErr != nil && !os.IsNotExist(groupErr) { |
There was a problem hiding this comment.
Not a show-stopper, but just realised we don't really need the var declarations here, and could keep the original;
| var ugroups []user.Group | |
| var groupErr error | |
| ugroups, groupErr = user.ParseGroupFile(gpath) | |
| if groupErr != nil && !os.IsNotExist(groupErr) { | |
| ugroups, groupErr := user.ParseGroupFile(gpath) | |
| if groupErr != nil && !os.IsNotExist(groupErr) { |
1cf3094 to
5a3ab59
Compare
Scratch images don't necessarily have the /etc/group file, so we shouldn't fail if opening/parsing it is not needed: if all the group to add are numeric. Signed-off-by: Djordje Lukic <djordje.lukic@docker.com>
5a3ab59 to
1398186
Compare
| return err | ||
| ugroups, groupErr := user.ParseGroupFile(gpath) | ||
| if groupErr != nil && !os.IsNotExist(groupErr) { | ||
| return groupErr |
There was a problem hiding this comment.
Thinking a bit more about this, I think we want to ignore this error completely, no matter what the error is unless the file is needed. WDYT?
There was a problem hiding this comment.
imho it would be good to keep this error check and return early in case of parsing error. looking into user.ParseGroupFile impl it does not seem to guarantee nil result on error. so the remaining logic would potentially be processing partially parsed results.
Scratch images don't necessarily have the /etc/group file, so we shouldn't fail if opening/parsing it is not needed: if all the group to add are numeric.