diff --git a/docs/faq.md b/docs/faq.md index 153f79ee..926cadf4 100644 --- a/docs/faq.md +++ b/docs/faq.md @@ -226,6 +226,34 @@ it and replace it with your git cloned directory. In my case, I like to work on things in `~/code/mgmt/`, so that path is a symlink that points to the long project directory. +### Why does my file resource error with `no such file or directory`? + +If you create a file resource and only specify the content like this: + +``` +file "/tmp/foo" { + content => "hello world\n", +} +``` + +Then this will attempt to set the contents of that file to the desired string, +but *only* if that file already exists. If you'd like to ensure that it also +gets created in case it is not present, then you must also specify the state: + +``` +file "/tmp/foo" { + state => "exists", + content => "hello world\n", +} +``` + +Similar logic applies for situations when you only specify the `mode` parameter. + +This all turns out to be more safe and "correct", in that it would error and +prevent masking an error for a situation when you expected a file to already be +at that location. It also turns out to simplify the internals significantly, and +remove an ambiguous scenario with the reversable file resource. + ### On startup `mgmt` hangs after: `etcd: server: starting...`. If you get an error message similar to: diff --git a/docs/resources.md b/docs/resources.md index ff48590b..2a39a87e 100644 --- a/docs/resources.md +++ b/docs/resources.md @@ -69,8 +69,8 @@ identified by a trailing slash in their path name. File have no such slash. It has the following properties: * `path`: absolute file path (directories have a trailing slash here) +* `state`: either `exists`, `absent`, or undefined * `content`: raw file content -* `state`: either `exists` (the default value) or `absent` * `mode`: octal unix file permissions * `owner`: username or uid for the file owner * `group`: group name or gid for the file group @@ -79,6 +79,16 @@ It has the following properties: The path property specifies the file or directory that we are managing. +### State + +The state property describes the action we'd like to apply for the resource. The +possible values are: `exists` and `absent`. If you do not specify either of +these, it is undefined. Without specifying this value as `exists`, another param +cannot cause a file to get implicitly created. When specifying this value as +`absent`, you should not specify any other params that would normally change the +file. For example, if you specify `content` and this param is `absent`, then you +will get an engine validation error. + ### Content The content property is a string that specifies the desired file contents. @@ -88,11 +98,6 @@ The content property is a string that specifies the desired file contents. The source property points to a source file or directory path that we wish to copy over and use as the desired contents for our resource. -### State - -The state property describes the action we'd like to apply for the resource. The -possible values are: `exists` and `absent`. - ### Recurse The recurse property limits whether file resource operations should recurse into diff --git a/engine/resources/file.go b/engine/resources/file.go index c06d6a29..e1d7c17a 100644 --- a/engine/resources/file.go +++ b/engine/resources/file.go @@ -73,19 +73,29 @@ type FileRes struct { Dirname string `lang:"dirname" yaml:"dirname"` // override the path dirname Basename string `lang:"basename" yaml:"basename"` // override the path basename + // State specifies the desired state of the file. It can be either + // `exists` or `absent`. If you do not specify this, we will not be able + // to create or remove a file if it might be logical for another + // param to require that. Instead it will error. This means that this + // field is not implied by specifying some content or a mode. + State string `lang:"state" yaml:"state"` + // Content specifies the file contents to use. If this is nil, they are // left undefined. It cannot be combined with Source. Content *string `lang:"content" yaml:"content"` // Source specifies the source contents for the file resource. It cannot // be combined with the Content parameter. Source string `lang:"source" yaml:"source"` - // State specifies the desired state of the file. It can be either - // `exists` or `absent`. If you do not specify this, it will be - // undefined, and determined based on the other parameters. - State string `lang:"state" yaml:"state"` - Owner string `lang:"owner" yaml:"owner"` - Group string `lang:"group" yaml:"group"` + // Owner specifies the file owner. You can specify either the string + // name, or a string representation of the owner integer uid. + Owner string `lang:"owner" yaml:"owner"` + // Group specifies the file group. You can specify either the string + // name, or a string representation of the group integer gid. + Group string `lang:"group" yaml:"group"` + // Mode is the mode of the file as a string representation of the octal + // form. + // TODO: add symbolic representations Mode string `lang:"mode" yaml:"mode"` Recurse bool `lang:"recurse" yaml:"recurse"` Force bool `lang:"force" yaml:"force"` @@ -94,104 +104,6 @@ type FileRes struct { recWatcher *recwatch.RecWatcher } -// Default returns some sensible defaults for this resource. -func (obj *FileRes) Default() engine.Res { - return &FileRes{ - //State: FileStateUndefined, // the default must be undefined! - } -} - -// Validate reports any problems with the struct definition. -func (obj *FileRes) Validate() error { - if obj.getPath() == "" { - return fmt.Errorf("path is empty") - } - - if obj.Dirname != "" && !strings.HasSuffix(obj.Dirname, "/") { - return fmt.Errorf("dirname must end with a slash") - } - - if strings.HasPrefix(obj.Basename, "/") { - return fmt.Errorf("basename must not start with a slash") - } - - if !strings.HasPrefix(obj.getPath(), "/") { - return fmt.Errorf("resultant path must be absolute") - } - - if obj.Content != nil && obj.Source != "" { - return fmt.Errorf("can't specify both Content and Source") - } - - if obj.isDir() && obj.Content != nil { // makes no sense - return fmt.Errorf("can't specify Content when creating a Dir") - } - - if obj.State != FileStateExists && obj.State != FileStateAbsent && obj.State != FileStateUndefined { - return fmt.Errorf("the State is invalid") - } - - if obj.State == FileStateAbsent && obj.Content != nil { - return fmt.Errorf("can't specify content for an absent file") - } - - if obj.Mode != "" { - if _, err := obj.mode(); err != nil { - return err - } - } - - if obj.Owner != "" || obj.Group != "" { - fileInfo, err := os.Stat("/") // pick root just to do this test - if err != nil { - return fmt.Errorf("can't stat root to get system information") - } - _, ok := fileInfo.Sys().(*syscall.Stat_t) - if !ok { - return fmt.Errorf("can't set Owner or Group on this platform") - } - } - if _, err := engineUtil.GetUID(obj.Owner); obj.Owner != "" && err != nil { - return err - } - - if _, err := engineUtil.GetGID(obj.Group); obj.Group != "" && err != nil { - return err - } - - // XXX: should this specify that we create an empty directory instead? - //if obj.Source == "" && obj.isDir() { - // return fmt.Errorf("can't specify an empty source when creating a Dir.") - //} - - return nil -} - -// mode returns the file permission specified on the graph. It doesn't handle -// the case where the mode is not specified. The caller should check obj.Mode is -// not empty. -func (obj *FileRes) mode() (os.FileMode, error) { - m, err := strconv.ParseInt(obj.Mode, 8, 32) - if err != nil { - return os.FileMode(0), errwrap.Wrapf(err, "mode should be an octal number (%s)", obj.Mode) - } - return os.FileMode(m), nil -} - -// Init runs some startup code for this resource. -func (obj *FileRes) Init(init *engine.Init) error { - obj.init = init // save for later - - obj.sha256sum = "" - - return nil -} - -// Close is run by the engine to clean up after the resource is done. -func (obj *FileRes) Close() error { - return nil -} - // getPath returns the actual path to use for this resource. It computes this // after analysis of the Path, Dirname and Basename values. Dirs end with slash. // TODO: memoize the result if this seems important. @@ -221,6 +133,115 @@ func (obj *FileRes) isDir() bool { return strings.HasSuffix(obj.getPath(), "/") // dirs have trailing slashes } +// mode returns the file permission specified on the graph. It doesn't handle +// the case where the mode is not specified. The caller should check obj.Mode is +// not empty. +func (obj *FileRes) mode() (os.FileMode, error) { + m, err := strconv.ParseInt(obj.Mode, 8, 32) + if err != nil { + return os.FileMode(0), errwrap.Wrapf(err, "mode should be an octal number (%s)", obj.Mode) + } + return os.FileMode(m), nil +} + +// Default returns some sensible defaults for this resource. +func (obj *FileRes) Default() engine.Res { + return &FileRes{ + //State: FileStateUndefined, // the default must be undefined! + } +} + +// Validate reports any problems with the struct definition. +func (obj *FileRes) Validate() error { + if obj.getPath() == "" { + return fmt.Errorf("path is empty") + } + + if obj.Dirname != "" && !strings.HasSuffix(obj.Dirname, "/") { + return fmt.Errorf("dirname must end with a slash") + } + + if strings.HasPrefix(obj.Basename, "/") { + return fmt.Errorf("basename must not start with a slash") + } + + if !strings.HasPrefix(obj.getPath(), "/") { + return fmt.Errorf("resultant path must be absolute") + } + + if obj.State != FileStateExists && obj.State != FileStateAbsent && obj.State != FileStateUndefined { + return fmt.Errorf("the State is invalid") + } + + if obj.State == FileStateAbsent && obj.Content != nil { + return fmt.Errorf("can't specify Content for an absent file") + } + + if obj.Content != nil && obj.Source != "" { + return fmt.Errorf("can't specify both Content and Source") + } + + if obj.isDir() && obj.Content != nil { // makes no sense + return fmt.Errorf("can't specify Content when creating a Dir") + } + + // TODO: should we silently ignore these errors or include them? + //if obj.State == FileStateAbsent && obj.Owner != "" { + // return fmt.Errorf("can't specify Owner for an absent file") + //} + //if obj.State == FileStateAbsent && obj.Group != "" { + // return fmt.Errorf("can't specify Group for an absent file") + //} + if obj.Owner != "" || obj.Group != "" { + fileInfo, err := os.Stat("/") // pick root just to do this test + if err != nil { + return fmt.Errorf("can't stat root to get system information") + } + _, ok := fileInfo.Sys().(*syscall.Stat_t) + if !ok { + return fmt.Errorf("can't set Owner or Group on this platform") + } + } + if _, err := engineUtil.GetUID(obj.Owner); obj.Owner != "" && err != nil { + return err + } + + if _, err := engineUtil.GetGID(obj.Group); obj.Group != "" && err != nil { + return err + } + + // TODO: should we silently ignore this error or include it? + //if obj.State == FileStateAbsent && obj.Mode != "" { + // return fmt.Errorf("can't specify Mode for an absent file") + //} + if obj.Mode != "" { + if _, err := obj.mode(); err != nil { + return err + } + } + + // XXX: should this specify that we create an empty directory instead? + //if obj.Source == "" && obj.isDir() { + // return fmt.Errorf("can't specify an empty source when creating a Dir.") + //} + + return nil +} + +// Init runs some startup code for this resource. +func (obj *FileRes) Init(init *engine.Init) error { + obj.init = init // save for later + + obj.sha256sum = "" + + return nil +} + +// Close is run by the engine to clean up after the resource is done. +func (obj *FileRes) Close() error { + return nil +} + // Watch is the primary listener for this resource and it outputs events. // This one is a file watcher for files and directories. // Modify with caution, it is probably important to write some test cases first! @@ -273,7 +294,7 @@ func (obj *FileRes) Watch() error { // can be a bytes Buffer struct. It can take an input sha256 hash to use instead // of computing the source data hash, and it returns the computed value if this // function reaches that stage. As usual, it respects the apply action variable, -// and it symmetry with the main CheckApply function returns checkOK and error. +// and has some symmetry with the main CheckApply function. func (obj *FileRes) fileCheckApply(apply bool, src io.ReadSeeker, dst string, sha256sum string) (string, bool, error) { // TODO: does it make sense to switch dst to an io.Writer ? // TODO: use obj.Force when dealing with symlinks and other file types! @@ -310,18 +331,25 @@ func (obj *FileRes) fileCheckApply(apply bool, src io.ReadSeeker, dst string, sh defer dstClose() dstExists := !os.IsNotExist(err) + // Optimization: we shouldn't be making the file, it happens in + // stateCheckApply, but we skip doing it there in order to do it here, + // unless we're undefined, and then we shouldn't force it! + if !dstExists && obj.State == FileStateUndefined { + return "", false, err + } + dstStat, err := dstFile.Stat() if err != nil && dstExists { return "", false, err } if dstExists && dstStat.IsDir() { // oops, dst is a dir, and we want a file... - if !apply { - return "", false, nil - } if !obj.Force { return "", false, fmt.Errorf("can't force dir into file: %s", dst) } + if !apply { + return "", false, nil + } cleanDst := path.Clean(dst) if cleanDst == "" || cleanDst == "/" { @@ -411,7 +439,7 @@ func (obj *FileRes) dirCheckApply(apply bool) (bool, error) { // check if the path exists and is a directory fileInfo, err := os.Stat(obj.getPath()) if err != nil && !os.IsNotExist(err) { - return false, errwrap.Wrapf(err, "error checking file resource existence") + return false, errwrap.Wrapf(err, "stat error on file resource") } if err == nil && fileInfo.IsDir() { @@ -524,6 +552,7 @@ func (obj *FileRes) syncCheckApply(apply bool, src, dst string) (bool, error) { relPathFile := strings.TrimSuffix(relPath, "/") if _, ok := smartDst[relPathFile]; ok { absCleanDst := path.Clean(absDst) + // TODO: can we fail this before `!apply`? if !obj.Force { return false, fmt.Errorf("can't force file into dir: %s", absCleanDst) } @@ -592,13 +621,13 @@ func (obj *FileRes) syncCheckApply(apply bool, src, dst string) (bool, error) { continue } _ = absSrc - //obj.init.Logf("syncCheckApply: Recurse rm: %s -> %s", absSrc, absDst) + //obj.init.Logf("syncCheckApply: recurse rm: %s -> %s", absSrc, absDst) //if c, err := obj.syncCheckApply(apply, absSrc, absDst); err != nil { - // return false, errwrap.Wrapf(err, "syncCheckApply: Recurse rm failed") + // return false, errwrap.Wrapf(err, "syncCheckApply: recurse rm failed") //} else if !c { // don't let subsequent passes make this true // checkOK = false //} - //obj.init.Logf("syncCheckApply: Removing: %s", absCleanDst) + //obj.init.Logf("syncCheckApply: removing: %s", absCleanDst) //if apply { // safety // if err := os.Remove(absCleanDst); err != nil { // return false, err @@ -610,7 +639,8 @@ func (obj *FileRes) syncCheckApply(apply bool, src, dst string) (bool, error) { return checkOK, nil } -// state performs a CheckApply of the file state to create an empty file. +// stateCheckApply performs a CheckApply of the file state to create or remove +// an empty file or directory. func (obj *FileRes) stateCheckApply(apply bool) (bool, error) { if obj.State == FileStateUndefined { // state is not specified return true, nil @@ -635,153 +665,107 @@ func (obj *FileRes) stateCheckApply(apply bool) (bool, error) { return false, nil } - if obj.State == FileStateAbsent { - return false, nil // defer the work to contentCheckApply - } - - if obj.Content == nil && !obj.isDir() { - // Create an empty file to ensure one exists. Don't O_TRUNC it, - // in case one is magically created right after our exists test. - // The chmod used is what is used by the os.Create function. - // TODO: is using O_EXCL okay? - f, err := os.OpenFile(obj.getPath(), os.O_WRONLY|os.O_CREATE|os.O_EXCL, 0666) - if err != nil { - return false, errwrap.Wrapf(err, "problem creating empty file") + if obj.State == FileStateAbsent { // remove + p := obj.getPath() + if p == "" { + // programming error? + return false, fmt.Errorf("can't remove empty path") // safety } - if err := f.Close(); err != nil { - return false, errwrap.Wrapf(err, "problem closing empty file") - } - } - - return false, nil // defer the Content != nil and isDir work to later... -} - -// contentCheckApply performs a CheckApply for the file existence and content. -func (obj *FileRes) contentCheckApply(apply bool) (bool, error) { - obj.init.Logf("contentCheckApply(%t)", apply) - - if obj.State == FileStateAbsent { - if _, err := os.Stat(obj.getPath()); os.IsNotExist(err) { - // no such file or directory, but - // file should be missing, phew :) - return true, nil - - } else if err != nil { // what could this error be? - return false, err - } - - // state is not okay, no work done, exit, but without error - if !apply { - return false, nil - } - - // apply portion - if obj.getPath() == "" || obj.getPath() == "/" { + if p == "/" { return false, fmt.Errorf("don't want to remove root") // safety } - obj.init.Logf("contentCheckApply: removing: %s", obj.getPath()) + obj.init.Logf("stateCheckApply: removing: %s", p) // FIXME: respect obj.Recurse here... // TODO: add recurse limit here - err := os.RemoveAll(obj.getPath()) // dangerous ;) - return false, err // either nil or not + err := os.RemoveAll(p) // dangerous ;) + return false, err // either nil or not } - if obj.isDir() && obj.Source == "" { + // we need to make a file or a directory now + + if obj.isDir() { return obj.dirCheckApply(apply) } + // Optimization: we shouldn't even look at obj.Content here, but we can + // skip this empty file creation here since we know we're going to be + // making it there anyways. This way we save the extra fopen noise. + if obj.Content != nil { + return false, nil // pretend we actually made it + } + + // Create an empty file to ensure one exists. Don't O_TRUNC it, in case + // one is magically created right after our exists test. The chmod used + // is what is used by the os.Create function. + // TODO: is using O_EXCL okay? + f, err := os.OpenFile(obj.getPath(), os.O_WRONLY|os.O_CREATE|os.O_EXCL, 0666) + if err != nil { + return false, errwrap.Wrapf(err, "problem creating empty file") + } + if err := f.Close(); err != nil { + return false, errwrap.Wrapf(err, "problem closing empty file") + } + + return false, nil // defer the Content != nil work to later... +} + +// contentCheckApply performs a CheckApply for the file content. +func (obj *FileRes) contentCheckApply(apply bool) (bool, error) { + obj.init.Logf("contentCheckApply(%t)", apply) + // content is not defined, leave it alone... - if obj.Content == nil && obj.Source == "" { + if obj.Content == nil { return true, nil } - if obj.Source == "" { // do the obj.Content checks first... - bufferSrc := bytes.NewReader([]byte(*obj.Content)) - sha256sum, checkOK, err := obj.fileCheckApply(apply, bufferSrc, obj.getPath(), obj.sha256sum) - if sha256sum != "" { // empty values mean errored or didn't hash - // this can be valid even when the whole function errors - obj.sha256sum = sha256sum // cache value - } - if err != nil { - return false, err - } - // if no err, but !ok, then... - return checkOK, nil // success + bufferSrc := bytes.NewReader([]byte(*obj.Content)) + sha256sum, checkOK, err := obj.fileCheckApply(apply, bufferSrc, obj.getPath(), obj.sha256sum) + if sha256sum != "" { // empty values mean errored or didn't hash + // this can be valid even when the whole function errors + obj.sha256sum = sha256sum // cache value + } + if err != nil { + return false, err + } + // if no err, but !ok, then... + return checkOK, nil // success +} + +// sourceCheckApply performs a CheckApply for the file source. +func (obj *FileRes) sourceCheckApply(apply bool) (bool, error) { + obj.init.Logf("sourceCheckApply(%t)", apply) + + // source is not defined, leave it alone... + if obj.Source == "" { + return true, nil } checkOK, err := obj.syncCheckApply(apply, obj.Source, obj.getPath()) if err != nil { - obj.init.Logf("syncCheckApply: Error: %v", err) + obj.init.Logf("syncCheckApply: error: %v", err) return false, err } return checkOK, nil } -// chmodCheckApply performs a CheckApply for the file permissions. -func (obj *FileRes) chmodCheckApply(apply bool) (bool, error) { - obj.init.Logf("chmodCheckApply(%t)", apply) - - if obj.State == FileStateAbsent { - // file is absent - return true, nil - } - - if obj.Mode == "" { - // no mode specified, everything is ok - return true, nil - } - - mode, err := obj.mode() - - // If the file does not exist and we are in - // noop mode, do not throw an error. - if os.IsNotExist(err) && !apply { - return false, nil - } - - if err != nil { - return false, err - } - - fileInfo, err := os.Stat(obj.getPath()) - if err != nil { - return false, err - } - - // nothing to do - if fileInfo.Mode() == mode { - return true, nil - } - - // not clean but don't apply - if !apply { - return false, nil - } - - err = os.Chmod(obj.getPath(), mode) - return false, err -} - // chownCheckApply performs a CheckApply for the file ownership. func (obj *FileRes) chownCheckApply(apply bool) (bool, error) { - var expectedUID, expectedGID int obj.init.Logf("chownCheckApply(%t)", apply) - if obj.State == FileStateAbsent { - // file is absent or no owner specified + if obj.Owner == "" && obj.Group == "" { + // no owner or group specified, everything is ok return true, nil } fileInfo, err := os.Stat(obj.getPath()) - - // If the file does not exist and we are in - // noop mode, do not throw an error. - if os.IsNotExist(err) && !apply { - return false, nil - } - - if err != nil { + // TODO: is this a sane behaviour that we want to preserve? + // If the file does not exist and we are in noop mode, do not throw an + // error. + //if os.IsNotExist(err) && !apply { + // return false, nil + //} + if err != nil { // if the file does not exist, it's correct to error! return false, err } @@ -791,6 +775,8 @@ func (obj *FileRes) chownCheckApply(apply bool) (bool, error) { return false, fmt.Errorf("can't set Owner or Group on this platform") } + var expectedUID, expectedGID int + if obj.Owner != "" { expectedUID, err = engineUtil.GetUID(obj.Owner) if err != nil { @@ -800,7 +786,6 @@ func (obj *FileRes) chownCheckApply(apply bool) (bool, error) { // nothing specified, no changes to be made, expect same as actual expectedUID = int(stUnix.Uid) } - if obj.Group != "" { expectedGID, err = engineUtil.GetGID(obj.Group) if err != nil { @@ -824,6 +809,38 @@ func (obj *FileRes) chownCheckApply(apply bool) (bool, error) { return false, os.Chown(obj.getPath(), expectedUID, expectedGID) } +// chmodCheckApply performs a CheckApply for the file permissions. +func (obj *FileRes) chmodCheckApply(apply bool) (bool, error) { + obj.init.Logf("chmodCheckApply(%t)", apply) + + if obj.Mode == "" { + // no mode specified, everything is ok + return true, nil + } + + mode, err := obj.mode() // get the desired mode + if err != nil { + return false, err + } + + fileInfo, err := os.Stat(obj.getPath()) + if err != nil { // if the file does not exist, it's correct to error! + return false, err + } + + // nothing to do + if fileInfo.Mode() == mode { + return true, nil + } + + // not clean but don't apply + if !apply { + return false, nil + } + + return false, os.Chmod(obj.getPath(), mode) +} + // CheckApply checks the resource state and applies the resource if the bool // input is true. It returns error info and if the state check passed or not. func (obj *FileRes) CheckApply(apply bool) (bool, error) { @@ -841,7 +858,7 @@ func (obj *FileRes) CheckApply(apply bool) (bool, error) { checkOK := true - // always run stateCheckApply before contentCheckApply, they go together + // run stateCheckApply before contentCheckApply and sourceCheckApply if c, err := obj.stateCheckApply(apply); err != nil { return false, err } else if !c { @@ -852,8 +869,7 @@ func (obj *FileRes) CheckApply(apply bool) (bool, error) { } else if !c { checkOK = false } - - if c, err := obj.chmodCheckApply(apply); err != nil { + if c, err := obj.sourceCheckApply(apply); err != nil { return false, err } else if !c { checkOK = false @@ -864,6 +880,11 @@ func (obj *FileRes) CheckApply(apply bool) (bool, error) { } else if !c { checkOK = false } + if c, err := obj.chmodCheckApply(apply); err != nil { + return false, err + } else if !c { + checkOK = false + } return checkOK, nil // w00t } @@ -881,6 +902,11 @@ func (obj *FileRes) Cmp(r engine.Res) error { if obj.getPath() != res.getPath() { return fmt.Errorf("the Path differs") } + + if obj.State != res.State { + return fmt.Errorf("the State differs") + } + if (obj.Content == nil) != (res.Content == nil) { // xor return fmt.Errorf("the Content differs") } @@ -892,9 +918,6 @@ func (obj *FileRes) Cmp(r engine.Res) error { if obj.Source != res.Source { return fmt.Errorf("the Source differs") } - if obj.State != res.State { - return fmt.Errorf("the State differs") - } if obj.Owner != res.Owner { return fmt.Errorf("the Owner differs") @@ -1056,9 +1079,9 @@ func (obj *FileRes) Copy() engine.CopyableRes { Path: obj.Path, Dirname: obj.Dirname, Basename: obj.Basename, + State: obj.State, // TODO: if this becomes a pointer, copy the string! Content: content, Source: obj.Source, - State: obj.State, // TODO: if this becomes a pointer, copy the string! Owner: obj.Owner, Group: obj.Group, Mode: obj.Mode, @@ -1101,12 +1124,6 @@ func (obj *FileRes) Reversed() (engine.ReversibleRes, error) { //res.Dirname = obj.Dirname //res.Basename = obj.Basename - //res.Source = "" // XXX: what should we do with this? - - // these are already copied in, and we don't need to change them... - //res.Recurse = obj.Recurse - //res.Force = obj.Force - if obj.State == FileStateExists { res.State = FileStateAbsent } @@ -1132,6 +1149,11 @@ func (obj *FileRes) Reversed() (engine.ReversibleRes, error) { res.Content = nil } + //res.Source = "" // XXX: what should we do with this? + if obj.Source != "" { + return nil, fmt.Errorf("can't reverse with Source yet") + } + // There is a race if the operating system is adding/changing/removing // the file between the ioutil.Readfile at the top and here. If there is // a discrepancy between the two, then you might get an unexpected @@ -1148,14 +1170,24 @@ func (obj *FileRes) Reversed() (engine.ReversibleRes, error) { stUnix, ok := fileInfo.Sys().(*syscall.Stat_t) // XXX: add a !ok error scenario or some alternative? if ok { // if not, this isn't unix - res.Owner = strconv.FormatInt(int64(stUnix.Uid), 10) // Uid is a uint32 - res.Group = strconv.FormatInt(int64(stUnix.Gid), 10) // Gid is a uint32 + if obj.Owner != "" { + res.Owner = strconv.FormatInt(int64(stUnix.Uid), 10) // Uid is a uint32 + } + if obj.Group != "" { + res.Group = strconv.FormatInt(int64(stUnix.Gid), 10) // Gid is a uint32 + } } // TODO: use Mode().String() when we support full rwx style mode specs! - res.Mode = fmt.Sprintf("%#o", fileInfo.Mode().Perm()) // 0400, 0777, etc. + if obj.Mode != "" { + res.Mode = fmt.Sprintf("%#o", fileInfo.Mode().Perm()) // 0400, 0777, etc. + } } + // these are already copied in, and we don't need to change them... + //res.Recurse = obj.Recurse + //res.Force = obj.Force + return res, nil } diff --git a/engine/resources/resources_test.go b/engine/resources/resources_test.go index d0feb308..0a8d9527 100644 --- a/engine/resources/resources_test.go +++ b/engine/resources/resources_test.go @@ -184,6 +184,14 @@ func FileWrite(p, s string) Step { // path & string } } +// ErrIsNotExistOK returns nil if we get an IsNotExist true result on the error. +func ErrIsNotExistOK(e error) error { + if os.IsNotExist(e) { + return nil + } + return errwrap.Wrapf(e, "unexpected error") +} + func TestResources1(t *testing.T) { type test struct { // an individual test name string @@ -217,6 +225,7 @@ func TestResources1(t *testing.T) { p := "/tmp/whatever" s := "hello, world\n" res.Path = p + res.State = "exists" contents := s res.Content = &contents @@ -601,13 +610,15 @@ func TestResources2(t *testing.T) { } } - // resCheckApply runs CheckApply with noop = false for the res. It - // errors if the returned values aren't what we were expecting. - resCheckApply := func(res engine.Res, expCheckOK bool, expErr error) func() error { + // resCheckApplyError runs CheckApply with noop = false for the res. It + // errors if the returned checkOK values isn't what we were expecting or + // if the errOK function returns an error when given a chance to inspect + // the returned error. + resCheckApplyError := func(res engine.Res, expCheckOK bool, errOK func(e error) error) func() error { return func() error { checkOK, err := res.CheckApply(true) // no noop! - if err != expErr { - return fmt.Errorf("error from CheckApply did not match expected: `%+v` != `%+v`", err, expErr) + if e := errOK(err); e != nil { + return errwrap.Wrapf(e, "error from CheckApply did not match expected") } if checkOK != expCheckOK { return fmt.Errorf("result from CheckApply did not match expected: `%t` != `%t`", checkOK, expCheckOK) @@ -615,6 +626,18 @@ func TestResources2(t *testing.T) { return nil } } + // resCheckApply runs CheckApply with noop = false for the res. It + // errors if the returned checkOK values isn't what we were expecting or + // if there was an error. + resCheckApply := func(res engine.Res, expCheckOK bool) func() error { + errOK := func(e error) error { + if e == nil { + return nil + } + return errwrap.Wrapf(e, "unexpected error from CheckApply") + } + return resCheckApplyError(res, expCheckOK, errOK) + } // resClose runs Close on the res. resClose := func(res engine.Res) func() error { // run Close @@ -702,6 +725,160 @@ func TestResources2(t *testing.T) { } testCases := []test{} + { + //file "/tmp/somefile" { + // state => "exists", + // content => "some new text\n", + //} + r1 := makeRes("file", "r1") + res := r1.(*FileRes) // if this panics, the test will panic + p := "/tmp/somefile" + res.Path = p + res.State = "exists" + content := "some new text\n" + res.Content = &content + + timeline := []func() error{ + fileWrite(p, "whatever"), + resValidate(r1), + resInit(r1), + resCheckApply(r1, false), // changed + fileExpect(p, content), + resCheckApply(r1, true), // it's already good + resClose(r1), + fileExpect(p, content), // ensure it exists + } + + testCases = append(testCases, test{ + name: "simple file", + timeline: timeline, + expect: func() error { return nil }, + startup: func() error { return nil }, + cleanup: func() error { return nil }, + }) + } + { + //file "/tmp/somefile" { + // # state is NOT specified + // content => "some new text\n", + //} + r1 := makeRes("file", "r1") + res := r1.(*FileRes) // if this panics, the test will panic + p := "/tmp/somefile" + res.Path = p + //res.State = "exists" // not specified! + content := "some new text\n" + res.Content = &content + + timeline := []func() error{ + fileWrite(p, "whatever"), + resValidate(r1), + resInit(r1), + resCheckApply(r1, false), // changed + fileExpect(p, content), + resCheckApply(r1, true), // it's already good + resClose(r1), + fileExpect(p, content), // ensure it exists + } + + testCases = append(testCases, test{ + name: "edit file only", + timeline: timeline, + expect: func() error { return nil }, + startup: func() error { return nil }, + cleanup: func() error { return nil }, + }) + } + { + //file "/tmp/somefile" { + // # state is NOT specified + // content => "some new text\n", + //} + // and no existing file exists! (therefore we want an error!) + r1 := makeRes("file", "r1") + res := r1.(*FileRes) // if this panics, the test will panic + p := "/tmp/somefile" + res.Path = p + //res.State = "exists" // not specified! + content := "some new text\n" + res.Content = &content + + timeline := []func() error{ + fileRemove(p), // nothing here + resValidate(r1), + resInit(r1), + resCheckApplyError(r1, false, ErrIsNotExistOK), // should error + resCheckApplyError(r1, false, ErrIsNotExistOK), // double check + resClose(r1), + fileAbsent(p), // ensure it's absent + } + + testCases = append(testCases, test{ + name: "strict file", + timeline: timeline, + expect: func() error { return nil }, + startup: func() error { return nil }, + cleanup: func() error { return nil }, + }) + } + { + //file "/tmp/somefile" { + // state => "absent", + //} + // and no existing file exists! + r1 := makeRes("file", "r1") + res := r1.(*FileRes) // if this panics, the test will panic + p := "/tmp/somefile" + res.Path = p + res.State = "absent" + + timeline := []func() error{ + fileRemove(p), // nothing here + resValidate(r1), + resInit(r1), + resCheckApply(r1, true), + resCheckApply(r1, true), + resClose(r1), + fileAbsent(p), // ensure it's absent + } + + testCases = append(testCases, test{ + name: "absent file", + timeline: timeline, + expect: func() error { return nil }, + startup: func() error { return nil }, + cleanup: func() error { return nil }, + }) + } + { + //file "/tmp/somefile" { + // state => "absent", + //} + // and a file already exists! + r1 := makeRes("file", "r1") + res := r1.(*FileRes) // if this panics, the test will panic + p := "/tmp/somefile" + res.Path = p + res.State = "absent" + + timeline := []func() error{ + fileWrite(p, "whatever"), + resValidate(r1), + resInit(r1), + resCheckApply(r1, false), + resCheckApply(r1, true), + resClose(r1), + fileAbsent(p), // ensure it's absent + } + + testCases = append(testCases, test{ + name: "absent file pre-existing", + timeline: timeline, + expect: func() error { return nil }, + startup: func() error { return nil }, + cleanup: func() error { return nil }, + }) + } { //file "/tmp/somefile" { // content => "some new text\n", @@ -731,9 +908,9 @@ func TestResources2(t *testing.T) { return nil }, resInit(r1), - resCheckApply(r1, false, nil), // changed + resCheckApply(r1, false), // changed fileExpect(p, content), - resCheckApply(r1, true, nil), // it's already good + resCheckApply(r1, true), // it's already good resClose(r1), //resValidate(r2), // no!!! func() error { @@ -744,10 +921,10 @@ func TestResources2(t *testing.T) { return resInit(r2)() }, func() error { - return resCheckApply(r2, false, nil)() + return resCheckApply(r2, false)() }, func() error { - return resCheckApply(r2, true, nil)() + return resCheckApply(r2, true)() }, func() error { return resClose(r2)() @@ -793,9 +970,9 @@ func TestResources2(t *testing.T) { return nil }, resInit(r1), - resCheckApply(r1, false, nil), // changed + resCheckApply(r1, false), // changed fileExpect(p, content), - resCheckApply(r1, true, nil), // it's already good + resCheckApply(r1, true), // it's already good resClose(r1), //resValidate(r2), func() error { @@ -806,10 +983,10 @@ func TestResources2(t *testing.T) { return resInit(r2)() }, func() error { - return resCheckApply(r2, false, nil)() + return resCheckApply(r2, false)() }, func() error { - return resCheckApply(r2, true, nil)() + return resCheckApply(r2, true)() }, func() error { return resClose(r2)() @@ -833,12 +1010,8 @@ func TestResources2(t *testing.T) { // Meta:reverse => true, //} //# and there's NO existing file at this path... - //# NOTE: This is a corner case subtlety... if you don't want - // this particular behaviour, then specify `state` and you won't - // get something potentially unexpected. This is probably the - // better choice, because it's otherwise hard to emulate this - // behaviour, where as specifying state gets you the other - // possibility. + //# NOTE: This used to be a corner case subtlety for reversal. + //# Now that we error in this scenario before reversal, it's ok! r1 := makeRes("file", "r1") res := r1.(*FileRes) // if this panics, the test will panic p := "/tmp/somefile" @@ -861,31 +1034,25 @@ func TestResources2(t *testing.T) { return nil }, resInit(r1), - resCheckApply(r1, false, nil), // changed - fileExpect(p, content), - resCheckApply(r1, true, nil), // it's already good + resCheckApplyError(r1, false, ErrIsNotExistOK), // changed + //fileExpect(p, content), + //resCheckApply(r1, true), // it's already good resClose(r1), - //resValidate(r2), - func() error { - // wrap it b/c it is currently nil - return r2.Validate() - }, - func() error { - return resInit(r2)() - }, //func() error { - // return resCheckApply(r2, false, nil)() + // // wrap it b/c it is currently nil + // return r2.Validate() //}, - func() error { // it's already in the correct state - return resCheckApply(r2, true, nil)() - }, - func() error { - return resClose(r2)() - }, - // TODO: instead should r2 have removed the file? - // TODO: the subtlety is that state wasn't specified :/ - fileExpect(p, content), // we never changed it back... - fileRemove(p), // cleanup + //func() error { + // return resInit(r2)() + //}, + //func() error { // it's already in the correct state + // return resCheckApply(r2, true)() + //}, + //func() error { + // return resClose(r2)() + //}, + //fileExpect(p, content), // we never changed it back... + //fileRemove(p), // cleanup } testCases = append(testCases, test{ @@ -922,9 +1089,9 @@ func TestResources2(t *testing.T) { return nil }, resInit(r1), - resCheckApply(r1, false, nil), // changed - fileAbsent(p), // ensure it got removed - resCheckApply(r1, true, nil), // it's already good + resCheckApply(r1, false), // changed + fileAbsent(p), // ensure it got removed + resCheckApply(r1, true), // it's already good resClose(r1), //resValidate(r2), // no!!! func() error { @@ -935,10 +1102,10 @@ func TestResources2(t *testing.T) { return resInit(r2)() }, func() error { - return resCheckApply(r2, false, nil)() + return resCheckApply(r2, false)() }, func() error { - return resCheckApply(r2, true, nil)() + return resCheckApply(r2, true)() }, func() error { return resClose(r2)() diff --git a/examples/lang/contains0.mcl b/examples/lang/contains0.mcl index 374f8014..58d94b66 100644 --- a/examples/lang/contains0.mcl +++ b/examples/lang/contains0.mcl @@ -11,6 +11,7 @@ $c4 = "b" in $set $s = fmt.printf("1: %t, 2: %t, 3: %t, 4: %t\n", $c1, $c2, $c3, $c4) file "/tmp/mgmt/contains" { + state => "exists", content => $s, } @@ -21,5 +22,6 @@ $x = if sys.hostname() in ["h1", "h3",] { } file "/tmp/mgmt/hello-${sys.hostname()}" { + state => "exists", content => $x, } diff --git a/examples/lang/cron2.mcl b/examples/lang/cron2.mcl index 980a27c1..5e0fd660 100644 --- a/examples/lang/cron2.mcl +++ b/examples/lang/cron2.mcl @@ -5,4 +5,5 @@ cron "purpleidea-oneshot" { svc "purpleidea-oneshot" {} +# TODO: do we need a state => "exists" specified here? file "/etc/systemd/system/purpleidea-oneshot.service" {} diff --git a/examples/lang/cron3.mcl b/examples/lang/cron3.mcl index 978cf700..ea3f3850 100644 --- a/examples/lang/cron3.mcl +++ b/examples/lang/cron3.mcl @@ -10,4 +10,5 @@ svc "purpleidea-oneshot" { session => true, } +# TODO: do we need a state => "exists" specified here? file printf("%s/.config/systemd/user/purpleidea-oneshot.service", $home) {} diff --git a/examples/lang/datetime1.mcl b/examples/lang/datetime1.mcl index 2d7f737b..9dc5b7ac 100644 --- a/examples/lang/datetime1.mcl +++ b/examples/lang/datetime1.mcl @@ -2,5 +2,6 @@ import "datetime" $d = datetime.now() file "/tmp/mgmt/datetime" { + state => "exists", content => template("Hello! It is now: {{ datetime_print . }}\n", $d), } diff --git a/examples/lang/datetime2.mcl b/examples/lang/datetime2.mcl index 9ca66dbe..735a71ba 100644 --- a/examples/lang/datetime2.mcl +++ b/examples/lang/datetime2.mcl @@ -12,6 +12,7 @@ $theload = structlookup(sys.load(), "x1") if 5 > 3 { file "/tmp/mgmt/datetime" { + state => "exists", content => template("Now + 1 year is: {{ .year }} seconds, aka: {{ datetime_print .year }}\n\nload average: {{ .load }}\n", $tmplvalues), } } diff --git a/examples/lang/datetime3.mcl b/examples/lang/datetime3.mcl index ff5acfe5..2b9fab00 100644 --- a/examples/lang/datetime3.mcl +++ b/examples/lang/datetime3.mcl @@ -14,5 +14,6 @@ $theload = structlookup(sys.load(), "x1") $vumeter = example.vumeter("====", 10, 0.9) file "/tmp/mgmt/datetime" { + state => "exists", content => template("Now + 1 year is: {{ .year }} seconds, aka: {{ datetime_print .year }}\n\nload average: {{ .load }}\n\nvu: {{ .vumeter }}\n", $tmplvalues), } diff --git a/examples/lang/exchange0.mcl b/examples/lang/exchange0.mcl index 7c020ed8..933886e9 100644 --- a/examples/lang/exchange0.mcl +++ b/examples/lang/exchange0.mcl @@ -13,5 +13,6 @@ $rand = random1(8) $exchanged = world.exchange("keyns", $rand) file "/tmp/mgmt/exchange-${sys.hostname()}" { + state => "exists", content => template("Found: {{ . }}\n", $exchanged), } diff --git a/examples/lang/history1.mcl b/examples/lang/history1.mcl index 66c77fd9..3ce8bcb8 100644 --- a/examples/lang/history1.mcl +++ b/examples/lang/history1.mcl @@ -5,5 +5,6 @@ $dt = datetime.now() $hystvalues = {"ix0" => $dt, "ix1" => $dt{1}, "ix2" => $dt{2}, "ix3" => $dt{3},} file "/tmp/mgmt/history" { + state => "exists", content => template("Index(0) {{.ix0}}: {{ datetime_print .ix0 }}\nIndex(1) {{.ix1}}: {{ datetime_print .ix1 }}\nIndex(2) {{.ix2}}: {{ datetime_print .ix2 }}\nIndex(3) {{.ix3}}: {{ datetime_print .ix3 }}\n", $hystvalues), } diff --git a/examples/lang/hysteresis1.mcl b/examples/lang/hysteresis1.mcl index 90e40da3..4e8ff119 100644 --- a/examples/lang/hysteresis1.mcl +++ b/examples/lang/hysteresis1.mcl @@ -1,6 +1,7 @@ import "sys" file "/tmp/mgmt/systemload" { + state => "exists", content => template("load average: {{ .load }} threshold: {{ .threshold }}\n", $tmplvalues), } diff --git a/examples/lang/password0.mcl b/examples/lang/password0.mcl index 11a02fdc..8f3990fb 100644 --- a/examples/lang/password0.mcl +++ b/examples/lang/password0.mcl @@ -3,6 +3,7 @@ password "pass0" { } file "/tmp/mgmt/password" { + state => "exists", } Password["pass0"].password -> File["/tmp/mgmt/password"].content diff --git a/examples/lang/readfile1.mcl b/examples/lang/readfile1.mcl index 1610de48..37ae2701 100644 --- a/examples/lang/readfile1.mcl +++ b/examples/lang/readfile1.mcl @@ -2,5 +2,6 @@ import "os" # this copies the contents from /tmp/input and puts them in /tmp/output file "/tmp/output" { + state => "exists", content => os.readfile("/tmp/input"), } diff --git a/examples/lang/reverse3.mcl b/examples/lang/reverse3.mcl index cc1176ee..8e571236 100644 --- a/examples/lang/reverse3.mcl +++ b/examples/lang/reverse3.mcl @@ -18,6 +18,7 @@ file "/tmp/mgmt/" { # editing the file contents at anytime is allowed if $mod { file "/tmp/mgmt/hello" { + state => "exists", mode => "0777", Meta:reverse => true, diff --git a/examples/lang/schedule0.mcl b/examples/lang/schedule0.mcl index a8fbd8a8..576c1161 100644 --- a/examples/lang/schedule0.mcl +++ b/examples/lang/schedule0.mcl @@ -17,5 +17,6 @@ $set = world.schedule("xsched", $opts) #$set = world.schedule("xsched") file "/tmp/mgmt/scheduled-${sys.hostname()}" { + state => "exists", content => template("set: {{ . }}\n", $set), } diff --git a/examples/lang/sendrecv1.mcl b/examples/lang/sendrecv1.mcl index 1a791d1f..47652f75 100644 --- a/examples/lang/sendrecv1.mcl +++ b/examples/lang/sendrecv1.mcl @@ -19,6 +19,7 @@ Exec["exec0"].output -> Kv["kv0"].value if $state != "default" { file "/tmp/mgmt/state" { + state => "exists", content => fmt.printf("state: %s\n", $state), } } diff --git a/examples/lang/states0.mcl b/examples/lang/states0.mcl index 84aacf4e..81dcc529 100644 --- a/examples/lang/states0.mcl +++ b/examples/lang/states0.mcl @@ -7,6 +7,7 @@ $state = maplookup($exchanged, $hostname, "default") if $state == "one" || $state == "default" { file "/tmp/mgmt/state" { + state => "exists", content => "state: one\n", } @@ -22,6 +23,7 @@ if $state == "one" || $state == "default" { if $state == "two" { file "/tmp/mgmt/state" { + state => "exists", content => "state: two\n", } @@ -37,6 +39,7 @@ if $state == "two" { if $state == "three" { file "/tmp/mgmt/state" { + state => "exists", content => "state: three\n", } diff --git a/examples/lang/unicode.mcl b/examples/lang/unicode.mcl index ae33ac33..0381891b 100644 --- a/examples/lang/unicode.mcl +++ b/examples/lang/unicode.mcl @@ -3,5 +3,6 @@ print "unicode" { msg => $unicode, } file "/tmp/unicode" { + state => "exists", content => $unicode + "\n", } diff --git a/examples/lang/virt2.mcl b/examples/lang/virt2.mcl index fc2beeb9..18352e90 100644 --- a/examples/lang/virt2.mcl +++ b/examples/lang/virt2.mcl @@ -17,6 +17,7 @@ $count = if $input > 8 { } file "/tmp/output" { + state => "exists", content => fmt.printf("requesting: %d cpus\n", $count), } diff --git a/test/shell/env0.mcl b/test/shell/env0.mcl index baa22dfc..c05e09f6 100644 --- a/test/shell/env0.mcl +++ b/test/shell/env0.mcl @@ -18,5 +18,6 @@ $env = sys.env() $m = maplookup($env, "TEST", "321") file "${tmpdir}/environ" { + state => "exists", content => fmt.printf("%s,%s,%s:%s,%s,%s:%t,%t:%s", $x, $y, $z, $a, $b, $c, $t, $f, $m), } diff --git a/test/shell/exchange0.mcl b/test/shell/exchange0.mcl index 7c020ed8..933886e9 100644 --- a/test/shell/exchange0.mcl +++ b/test/shell/exchange0.mcl @@ -13,5 +13,6 @@ $rand = random1(8) $exchanged = world.exchange("keyns", $rand) file "/tmp/mgmt/exchange-${sys.hostname()}" { + state => "exists", content => template("Found: {{ . }}\n", $exchanged), }