osutil: add EnsureDirState and FileState

[zyga]

This branch adds the EnsureDirState() function -- a flexible concept for maintaining a
subset of files in a directory. The function ensures that all the files
in the directory are exactly as they are expected to be and reports on
the changes made.

SyncDir() can be used to manage on-disk apparmor, seccomp, udev and
dbus configuration files. Reports about changes made can be used to
trigger udev configuration reload, load or unload apparmor profiles to
the kernel.

Signed-off-by: Zygmunt Krynicki zygmunt.krynicki@canonical.com

[niemeyer]

The abstraction is quite nice, but Janitor, Tidy, Oracle.. these are cool terms but not very helpful in understanding what's going on.

I suggest moving all of this functionality into a single function into the osutil package:

func SyncDir(dir, glob string, content map[string]*FileState) (created, corrected, removed []string, err error)

[niemeyer]

Note the rename of fixed => corrected. "Fixed" also means "not moving", which is confusing in this context as the first two terms imply "moving".

[niemeyer]

One more note: changed the order of parameters to be slightly more expected (what we want first, then what we don't want).

[niemeyer]

UID and GID or Uid and Gid.

[zyga]

golint complains and wants UID but Gid :-(

[niemeyer]

The point of a linter is to make the code nicer. If it's doing the opposite, we need to either fix it or remove it.

[zyga]

Thanks for the review @niemeyer I'll get right to it.

[niemeyer]

Per the earlier note, this should be a slightly more down-to-earth explanation.

The description should also cover what the keys in the map means.

[mvo5]

I'm feeling a bit uneasy that osutil becomes the new "helpers", i.e. collecting functionality that is only very loosely related. Maybe its own package?

[niemeyer]

osutil is still growing indeed, but under a common theme.. What I suggest we do at some point is to split half of it into fsutil

[mvo5]

I would prefer a less generic name than "Janitor" here, especially since it is a generic package like osutil. I don't have a great suggestion myself, maybe DirectoryEnforcer, DirContentEnforcer or somesuch.

[mvo5]

I just noticed that Gustavo commented earlier and suggested to move it here. Its fine (I think) however I would prefer more specific names/functions (EnforceDirectory() or SyncDirectory (but its more like a force instead of a sync but I won't argue over this, just my personal feeling).

[mvo5]

Same concern as above, this is in osutil but File is very generic, I'm concerned that we use a osutil.File for something that looks like its mostly tailored to the Janitor.

[zyga]

This was renamed to FileState

[niemeyer]

We don't need to mention functions that use the given type. This is used everywhere it is used.

[niemeyer]

All of that documentation is extremely boring and unnecessary, and it's not the first time we go through that.

Can we get go lint to stop complaining about such cases?

[niemeyer]

s/corrupted/differing/

[niemeyer]

It's much better and safer to have all err returns being explicitly made, rather than having to instantiate every single value returned ahead of time, and being careful to not use := by mistake. Even if you get all of these right today, the next person (which may be you and me) will come here and miss those details, creating very unfortunate and hard to debug issues.

[niemeyer]

Rather than having such long-spanning branches and having to carefully look back to identify which conditional this is talking about, it's much better to shortcut this quickly by saying:

data, ok := content[base]
if !ok {
    err := os.RemoveAll(name)
    ...
    continue
}

... continue unintended ...

[niemeyer]

Too much content for the loop. The number of indentations is a good hint.. the filesystem related ops inside this branch is another one. This is a very nice self-contained problem: write the file out.

[niemeyer]

I suspect the code will end up cleaner if you do that first: create all content desired, and then remove all content undesired. The delta between creating something on top of existing or anew is smaller.

[niemeyer]

Hmm.. why are we doing this non-atomically? Is there a particular system that depends on this behavior?

[zyga]

No, good catch!

[niemeyer]

Again, a bit concerned about the atomicity here. Doesn't feel good to create a file in the right place with bad content and bad file mode.

[zyga]

FYI, AFAIK we are not creating any files here, just opening existing files (we don't pass O_CREAT. I'll use AtomicWriteFile() elsewhere but I think the reading parts are correct.

[niemeyer]

@mvo5 suggested EnsureDirState to avoid the rsync feeling of DirSync.. sounds good.

[niemeyer]

It's better to have the result value names in their place, instead of repeating the exact same data in the doc.

[niemeyer]

Please note the underlying pattern: if the type is self-descriptive, we don't need a name to understand what it means. If the type is generic (string), it's better with the result names.

[niemeyer]

foo, bar := baz[blah]
if !bar {

Rather than

var foo
var bar
if foo, bar = baz[blah]; !bar {

[zyga]

I think this goes back to golint giving us exactly the opposite advice. Personally I find it tedious that we have to make golint happy this way when it flies in the face of what is more natural. I don't quite know how to convince golint to do better or start the process to evict it from having a say in our mergability.

[niemeyer]

Same..

[niemeyer]

Same..

[niemeyer]

This is a serious bug.. the call site broke the contract of the function, which means it cannot possibly work. I suggest panicking for the time being:

panic(fmt.Sprintf("EnsureDirState got a filename which is actually a file path: %q", baseName))

[niemeyer]

panic(fmt.Sprintf("EnsureDirState got filename %q which doesn't match the glob pattern %q", baseName, glob))

[chipaca]

or logger.Panicf(...) :-)

[niemeyer]

Where's ownership being handled?

[zyga]

Good catch, it isn't after the re-arrange! I'll correct this.

[niemeyer]

These are file operations, so filepath rather than path everywhere.

[zyga]

Thanks for pointing this out, I wasn't aware of the difference!

[niemeyer]

@zyga I still feel like the logic in this branch could be refactored to be harder to get wrong. So, a couple of concrete suggestions.

The first one is that I don't yet see a meaningful distinction between "this file already existed and was incorrect" and "this file did not exist". In both of these situations the file was bogus, and needs to be reloaded, so we may as well return both cases in a single changed list.

Then, the second one is based on a point I've made in the prior review. It's much easier to get logic right with isolated functions that are trying to do less.

Here is a candidate way to organize the function:

var errSameState = fmt.Errorf("file state has not changed")
...
for baseName, fileState := range content {
        filePath := filepath.Join(dir, baseName)
        err := writeFile(filePath, fileState)
        if err == errSameState {
                continue
        }
        if err != nil {
                return nil, nil, err
        }
        changed = append(changed, baseName)
}
for _, filePath := range matches {
        baseName := filepath.Base(filePath)
        if content[baseName] != nil {
                continue
        }
        err := os.Remove(filePath)
        if err != nil {
                return nil, nil, err
        }
        removed = append(removed, baseName) 
}

[niemeyer]

(improved sample with a minor tweak)

[mvo5]

Tiny nitpitck, maybe we can rename it to EnsureFileStateSuite or similar?

[zyga]

Thanks for the review everyone. I'll pick up where we left off yesterday and iterate.

[zyga]

This has a suble bug that I've introduced after switching to atomic writes. stat and file are not longer related to the newly-renamed file that was created by AtomicWrite()

[jdstrand]

"Sub-directories in the name are not allowed."

Where is this enforced? Should we have a test for this?

[niemeyer]

Yes, let's have a validation pass on the input (both content and glob) before we even start processing anything.

Let's panic if there's invalid data there.. it'd mean we not only broke the contract of the function, but we've used extremely dangerous data by mistake.

[jdstrand]

"Yes, let's have a validation pass on the input (both content and glob) before we even start processing anything.

Let's panic if there's invalid data there.. it'd mean we not only broke the contract of the function, but we've used extremely dangerous data by mistake."

This isn't implemented yet.

[jdstrand]

Actually I see the sub-directory validation was implemented somewhere else. Ignore my previous comment.

[jdstrand]

Perhaps be explicit here that the caller needs to handle when an error occurs since only some of the changes will have occurred.

[niemeyer]

This is a good point. It'll be hard to manage such errors cases in a useful way. I'd prefer to go all the way to the opposite end, and into the safety side as far as security is concerned:

• Define var firstErr error to track the first error we observe. Then:
• On a write error
  • Track in firstErr
  • Set content = nil
  • Break out of the write loop so removals are attempted on every known file.
• On a remove error
  • Track in firstErr
  • Continue
• If firstErr != nil { return nil, nil, firstErr }

[niemeyer]

(minor fix on the last line: return firstErr, not err)

[jdstrand]

"

• On a write error
  • Track in firstErr
  • Set content = nil
  • Break out of the write loop so removals are attempted on every known file.
    "

I like where you are going here since it seems EnsureDirState has everything it needs to do something right, but if file 1 is ok, file 2 has a write error and file 3 would be ok, file 1 is still applied, but files 2 and 3 are deleted (assuming I understand what you are suggesting). It almost seems better to go all through the loop, tracking all the errors so that 1 and 3 are processed and 2 is deleted. This is admittedly more complicated and I'm not sure how this function is intended to be used in the grand scheme of things, so may not be appropriate.

[niemeyer]

That's not what I meant, and it sounds very dangerous to do that.

What I meant is all or nothing. On any errors, we bail out and remove all known content. This is the only sane thing to do here, IMO, because nobody has ever pondered about the consequences of partially applying an arbitrary subset of security profiles. It would be basically closing our eyes and hoping for the best, which is not a sane strategy with security.

By backing out and removing all known content we ensure that either what we asked for is accomplished, or nothing else is.

[jdstrand]

EnsureDirState has the potential to operate on a full directory and indeed, that was how it was initially envisioned. However, we are now saying that EnsureDirState() should only be used per application. That suggests to me:

• the name should change, perhaps to EnsureAppState
• we should do input verification on the glob so it must match an app name regex
• we should update the comments for the functions for the above two points accordingly

[niemeyer]

• the name should change, perhaps to EnsureAppState

No, this is a helper function. It's not per app, or per snap, or per anything really. The call site is what defines what it is. As such, the name is currently fine.

• we should do input verification on the glob so it must match an app name regex

Again, separation of concerns. This is a helper which doesn't know about snaps. It's the call site that will define what the glob is.

[jdstrand]

I actually meant to say basename here instead of glob.

[jdstrand]

That said, with this function so general without a concrete example in the documentation at the top of the function, I'm finding it difficult to follow. Ie, I know what we are trying to achieve (I think! :), and so I am looking at how this function can fail, etc with bad arguments, but with things being generalized I see lots of place where a bad invocation can cause issues. Put another way-- we want to implement a specific thing for keeping files in sync, and we know what we want to use it for, but we are making it general for use cases that I don't know about.

If you want to keep it this way, fine, but I think it worth noting that I found it harder to follow and prove to myself that it was correct than I feel it should be.

[niemeyer]

Understood, and sorry for making your life harder here. It'd have been easier for you to review this after the call site is in place. We can do another round once we have that.

[jdstrand]

Since EnsureDirState is a loop, I think it would be wise to add a few tests for when the map is > 1. Eg: 2 files with no error, 1st file with error and 2nd ok, 1st file ok and 2nd error, etc.

[jdstrand]

Another test to add would be one where the size/contents and perms are both different.

[niemeyer]

This needs to be a more strict test:

if os.IsNotExist(err) {
        ... write file ...
}
if err != nil {
        return err
}

[jdstrand]

This error check says that if there is an error, go ahead and write the file, but this only makes sense if the error is for the basename of the filePath does not exist. What if another path component doesn't exist? What if the error is EPERM (which can happen with more than just DAC perms). 'man fstatat' gives a lot of possible failure conditions. Looking at https://golang.org/src/os/file_unix.go?s=4060:4099#L150 it appears go itself is not being very helpful here, simply returning PathError if anything goes wrong.

[niemeyer]

Beat you to it. ;)

[jdstrand]

Ownership and xattrs are not included here and I understand that is intentional. There should be a comment for EnsureDirState that calls these out explicitly so that developers are aware.

Xattrs aren't something that we currently use in snappy, but it is possible in the longer term future that we will incorporate them if/when AppArmor supports storing information there. It seems fine to not implement xattr syncing for now and to simply add it if/when needed (but again, please call this out in a comment for EnsureDirState).

Lack of ownership checks seems like a bug waiting to happen though-- I realize EnsureDirState currently only will run under a root-owned process, but it seems plausible that since this is a library function it will be asked to look at a directory with non-root owned files and this will clobber ownership. As mentioned, this should be mentioned in the comments for EnsureDirState. Supporting ownership syncs in a future commit seems wise though.

[zyga]

I've added a comment about unsupported features.

File ownership was there earlier but we don't need this and cannot easily write test code for it so I'd rather just not check or support this yet. Xattrs fall under the same umbrella. I agree that in principle it would be nice to support it but we'd never use this feature in current codebase.

[jdstrand]

Minor nitpick that is not a blocker-- it might be nice since you are specifying "expected.snap", "expected" and "0600" three time each to put each of these in variables: eg, expected_name, expected_contents and expected_perms.

[niemeyer]

Disagree a bit. Such trivial constants in place make the test more readable compared to a variable we need to look for elsewhere.

An exception is when we have logic that is interdependent but not obviously so. For example, we have two loops that need to have the same number of iterations.

We should fix the inconsistent use of quotes, though ("expected").

[jdstrand]

Nitpick: "// NOTE: the existing file is currently wide-open for everyone"

[niemeyer]

For the future: comparing string(content) is better, as []byte will end up not very readable.

[niemeyer]

These tests are all almost exactly the same. For the future, these are much better spelled out as a slice of test cases which define the parameters for a single loop that runs the tests.

var tests = []testCases{{
    existing: ...
    glob:      ...
    content: ...
}

We can even use FileState itself for existing.

But please don't worry about this right now. We've spent way too much time on this functionality already. We'll fix that some day we need to touch this again.

[niemeyer]

Note that EnsureDirState only ... (no parenthesis on method or function names, and no comma there)

[niemeyer]

One trivial, and LGTM

[niemeyer]

@jdstrand Can you please have another look?

[jdstrand]

Is this debugging code that was left in?

[zyga]

No, this is sanity validation.

EDIT: foo is there so that the function does something, it has to be any non-empty value.

[jdstrand]

This check is non-obvious perhaps since it seems Go isn't providing a way to compile a pattern. It would perhaps be more clear if you checked for ErrBadPattern which according to https://golang.org/pkg/path/filepath/#Match is the only thing it can error on. I would have expected the bad pattern check to happen below with something like:

if ok, err := filepath.Match(glob, baseName); !ok {
    ...
}
if err == ErrBadPattern {
    panic()
}

If you don't like this, can you at least add a comment to make it clear what you are trying to achieve and why you are doing it where you are?

[zyga]

I can add a comment. The intent of that check up front, with "foo", was to do it before we actually mess with the files. This would panic mid-way which is not what we want, I think.

[jdstrand]

But if you do it in the manner I described, the pattern will be wrong on the first check and you panic which shouldn't remove the files?

[niemeyer]

Based on the latest comments I assume that the important pieces of this work are sorted out. I'm merging this so we can move forward.

[jdstrand]

this should be snap.* now, no?

[niemeyer]

I'd just replace it by "*.foo" instead, to drop the direct implication that it needs to be inline with what the real call site is using.

[jdstrand]

I think it would be incredibly useful to give a concrete example of what the arguments are supposed to look like for EnsureDirState here. I'm having to look at the test suite to see how it is intended to be invoked and I think in the future this comment will help people unfamiliar with the code.

[niemeyer]

@jdstrand Once more, this is a helper. Any part of snappy can call it for its particular needs, with any glob and content. The documentation seems clear about how those call sites can use it. It is the particular call site that is oriented towards writing security profiles which will define how the glob and filenames will look like for that particular purpose.

[jdstrand]

I'm trying to understand the intent of this check. What is filepath.Match(glob, baseName) going to return that doesn't match filepath.Match("snap.*", "snap..")? Shouldn't we instead verify the basename matches a particular snap name.app name regex?

[niemeyer]

@jdstrand Are you reading my comments? :-)