Masterfiles stage rewrite #2465

mikeweilgart · 2016-01-07T07:08:45Z

masterfiles-stage.sh and its supporting files common.sh and options.sh have been completely refactored and largely rewritten. Many potential bugs and edge cases have been handled, including removal of insecure use of eval in option parsing.

The script is now 100% a drop-in replacement for the version of the code that ships with CFEngine.

This overhaul was actually done to allow incorporating the GIT_POLICY_CHANNELS vcs_type, and this has been done.

Some major functional changes should be noted:

GIT_TAG_OR_COMMIT and GIT vcs_types now use a mirrored git repository rather than a normal git clone, thus avoiding many edge cases related to git repos getting out of sync. These two VCS_TYPEs are now in fact identical except for variable names (see the main case switch in masterfiles-stage.sh to see how the difference is implemented).

The rollout method for policy has been changed from rsync to mv as an atomic operation for the git-based VCS_TYPEs. (The rollout method for the svn_branch function hasn't been changed.)

Add double quoting to variables so script won't break on spaces.

Clean up comments (remove obsolete comment, add comment with section header). Make error_exit function def consistent with other defs syntax.

avoid_triggering_unneeded_policy_updates, and rollout_staged_policy_to_masterdir

in masterfiles-stage.sh. It calls git_stage_policy_channels_from_mirror.

git_tag_or_commit_masterstage, and git_branch_masterstage, have been modified to use the recently added rollout functions: avoid_triggering_unneeded_policy_updates, and rollout_staged_policy_to_masterdir. Also rollout_staged_policy_to_masterdir was modified slightly in its comments, and using dirname instead of bash string handling.

git_tag_or_commit_masterstage and git_branch_masterstage have both been modified to call the functions git_setup_local_mirrored_repo and git_stage_refspec. The only distinction between these functions now is that one uses GIT_TAG_OR_COMMIT where the other uses GIT_BRANCH.

Moved setting of PARAMS and MASTERDIR to within options.sh Combined git_tag_or_commit_masterstage and git_branch_masterstage into a single function, git_masterstage, which accepts as an argument either GIT_TAG_OR_COMMIT or GIT_BRANCH. Add comments clarifying where certain variables are set. Add failure catch in case options.sh or common.sh not found.

Document options.sh and new VCS_TYPE.

since it's redundant; now ALL the git stage functions use a mirror. Change from GIT_MIRROR_POLICY_CHANNELS to GIT_POLICY_CHANNELS.

to be set in the params.sh file for the GIT_POLICY_CHANNELS vcs type. Updated readme.org with this info also. Add mkdir command for channel_deployment_dir so we don't fail easily. Add -x flag to git clean command to prevent any issue with a .gitignore file in the staging dir causing the deployed code to differ from the intended code from the git repo.

cfengine-review-bot · 2016-01-07T07:10:03Z

Can one of the admins verify this patch?

mikeweilgart · 2016-01-07T07:11:53Z

There is one undocumented feature which was present before and is still present: if you set a value for MASTERDIR in your params.sh file, it will override the value set on the command line. I found this at the last moment during testing, and actually found it useful in testing.

We should keep the default setting for MASTERDIR so the script can still be a drop-in replacement for the version which ships with CFEngine. And we should allow MASTERDIR to be set from the command line, so anyone currently using the script can go on using it that way. But I would recommend for most users that they set MASTERDIR directly in the params file for simplicity. ;)

mikeweilgart · 2016-01-07T07:26:06Z

@nickanderson here is the pull request!

nickanderson · 2016-01-07T14:09:39Z

ping @tzz if you don't mind taking a look, you did much of the original.

tzz · 2016-01-13T10:56:29Z

contrib/masterfiles-stage/README.org

@@ -86,7 +90,7 @@ The following parameters are in use, be sure to avoid their collision
 when developing new staging methods.

 - VCS_TYPE - The staging method to use
-  - Currently supported: (GIT|GIT_TAG_OR_COMMIT|SVN)
+  - Currently supported: (GIT|GIT_TAG_OR_COMMIT|GIT_POLICY_CHANNELS|SVN)


I'd add a note here that GIT_TAG_OR_COMMIT is legacy and equivalent to the other one.

I'll hold off on that until Nick and I come to an agreement regarding mirrored repos and how cf_promises_release_id gets populated.

tzz · 2016-01-13T11:05:14Z

I think this is a great rewrite. The only thing I'd consider changing is to use arrays inside the options instead of separate channels files, but that may not be possible if we want compatibility with older shells (I don't!).

mikeweilgart · 2016-01-22T03:24:16Z

contrib/masterfiles-stage/common.sh

+    error_exit "The channel_config array must have at least two elements."
+
+  check_git_installed
+  git_setup_local_mirrored_repo "$dir_to_hold_mirror"


Note the new parameter $dir_to_hold_mirror to be set in the PARAMS file. This is so the policy channels framework doesn't use ROOT in any way.

(This is only used by GIT_POLICY_CHANNELS but could easily be extended to git_masterstage() if you want.)

nickanderson · 2016-01-22T18:34:03Z

I have proposed some changes to the Design Center tooling to add in the direct sourcing of the PARAMS file.

Also I think we can also drop the GIT_TAG_OR_COMMIT VCS type. It was never publicly used, and git_masterstage works the same now with your refspec modifications.

Another thing that I think we should consider doing is fixing the permissions on the staging directory files before deploying to the final destination. It's not uncommon for things to get checked into git with bad permissions, and that leads to repairs on each deploy.

For example:

[root@hub dc-scripts]# cf-agent -KIf update.cf --define cfengine_internal_masterfiles_update
    info: Executing 'no timeout' ... '/var/cfengine/httpd/htdocs/api/dc-scripts/masterfiles-stage.sh'
    info: Command related to promiser '/var/cfengine/httpd/htdocs/api/dc-scripts/masterfiles-stage.sh' returned code defined as promise kept 0
    info: Completed execution of '/var/cfengine/httpd/htdocs/api/dc-scripts/masterfiles-stage.sh'
    info: Object '/var/cfengine/masterfiles/./sketches/sketch_template/scripts/sample.sh' had permission 0700, changed it to 0600
    info: Object '/var/cfengine/masterfiles/./sketches/sketch_template/modules/mymodule' had permission 0700, changed it to 0600
    info: Setting restart class 'no_vacuumdb' for promise '/var/cfengine/bin/vacuumdb'
[root@hub dc-scripts]# cf-agent -KIf update.cf --define cfengine_internal_masterfiles_update
    info: Executing 'no timeout' ... '/var/cfengine/httpd/htdocs/api/dc-scripts/masterfiles-stage.sh'
    info: Command related to promiser '/var/cfengine/httpd/htdocs/api/dc-scripts/masterfiles-stage.sh' returned code defined as promise kept 0
    info: Completed execution of '/var/cfengine/httpd/htdocs/api/dc-scripts/masterfiles-stage.sh'
    info: Object '/var/cfengine/masterfiles/./sketches/sketch_template/scripts/sample.sh' had permission 0700, changed it to 0600
    info: Object '/var/cfengine/masterfiles/./sketches/sketch_template/modules/mymodule' had permission 0700, changed it to 0600
    info: Setting restart class 'no_vacuumdb' for promise '/var/cfengine/bin/vacuumdb'

nickanderson · 2016-01-22T18:51:42Z

contrib/masterfiles-stage/common.sh

+  ########################## 3. SET PERMISSIONS ON POLICY SET
+  chown -R root:root "${temp_stage}" || error_exit "Unable to chown '${temp_stage}'"
+  chmod -R go-rwx    "${temp_stage}" || error_exit "Unable to chmod '${temp_stage}'"
+


Files should be 600, and directories should be 700

find "${temp_stage}" -type f | xargs chmod 600 find "${temp_stage}" -type d | xargs chmod 700

Maybe there is a better way than using find?

Not better than using find, but better than using xargs, which will cause issues when filenames contain whitespace. Instead I'll fix this to do the whole action with find (using the -exec flag). Thanks for pointing out the permissions issue; I agree it will be good to get rid of those warning messages.

nickanderson · 2016-01-22T19:57:33Z

@mikeweilgart my proposed changes to upstream tooling were accepted, so this will work with Design Center in future releases as a drop in replacement. Should probably note that its only drop-in replacement on 3.7.3+ and 3.8.2+ (not sure if it will make 3.8.1 or not)

nickanderson · 2016-01-22T20:02:34Z

@mikeweilgart also thinking that we should switch to using logger so that things goto syslog instead of a custom log file.

Found this interesting:
http://urbanautomaton.com/blog/2014/09/09/redirecting-bash-script-output-to-syslog/

Thoughts?

nickanderson · 2016-01-22T20:44:53Z

Here is a nitpick.

Staging twice in a row logs that masterfiles were successfully deployed even though nothing was changed in the deployment directory.

Perhaps instead we can log why deployment was skipped.

mikeweilgart · 2016-01-22T20:50:18Z

Right now my code never ever skips deployment. It only will skip triggering an update on hosts. But yes, if we were logging to syslog, that would be excessive logging.

This VCS_TYPE was never publicly used and is now unneeded, as the GIT vcs type will handle git tags or commit hashes as well as git branches. README was brought in sync with recent code changes.

mikeweilgart · 2016-01-22T20:56:49Z

contrib/masterfiles-stage/README.org

    - 88335d36b48c8808b12b48667a463182dc8d0338
-    - mytag
+    - cb375d0f


@nickanderson, is the GIT vcs type publicly used, or can I go ahead and change GIT_BRANCH to GIT_REFSPEC?

No, GIT_BRANCH is publicly used, and can be set by issuing an api call to mission portal. So we have to keep that one the way it is for now. I would aim to clean that up if/when we formalize policy channels in the API.

Oh and so is GIT as a VCS type. Its the only one supported by Mission Portal for Design Center currently.

nickanderson · 2016-01-22T21:13:37Z

I do think that it would be good if we avoided the additional churn caused by deploy fresh each time. Do you have a good idea in mind to prevent the unnecessary replacement?

nickanderson · 2016-01-22T21:36:57Z

@mikeweilgart regarding population of .git/HEAD I think its good that it always contains a commit sha. The point is to be able to easily associate that back to a specific state in the repo and commit shas are what we want for that. So I think what you have done will give us what we need there.

mikeweilgart · 2016-01-22T22:04:40Z

Re .git/HEAD, great, glad it's useful! :)

Now the extra temp dir will get removed if the script error exits, which is particularly important if the code validation fails (since otherwise the temp staging dirs would build up every time the script runs\!)

nickanderson · 2016-06-02T19:19:55Z

@mikeweilgart I've worked my way back around to this again.

I have gone through some simple tests with this code as a drop in replacement on top of 3.7.3.

Without doing any thing special, I am able to use version control integration as expected from Mission Portal.
Of note (probably already discussed previously in the thread) the target deployment directory gets replaced each time this runs. However the special cf_promises_validated file has special handling to preserve the previous content if cf_promises_release_id has not changed. This handling is necessary since successful validation when tagging with cf-promises -T DIR will always re-write the contents of cf_promises_validated and when the contnet of cf_promises_validated changes, remote agents will re-scan masterfiles for changes.

mikeweilgart · 2016-06-03T22:54:33Z

Perhaps I should add the policy files showing how the policy channel assignment gets pulled in by the hosts and used?

I also wrote a script to summarize the channel assignments for the hosts. Of course, you can use policy channels just for test directories that you run directly on the Hub, so the other pieces aren't necessarily a crucial ingredient to the masterfiles stage script upgrade, but it seems that all the pieces put together could be quite valuable to others.

I may just create a dedicate repository and include all examples and supporting scripts there.

nickanderson · 2016-07-27T18:55:48Z

contrib/masterfiles-stage/common.sh

  else
-    error_exit "The staged policies in ${STAGING_DIR} could not be validated, aborting."
+    if /usr/bin/cmp -s "${temp_stage}/cf_promises_release_id" \


Here we find that the release id is unchanged. And we preserve cf_promises_validated to avoid unecessry client updates. But I think that it would be useful to perform a secondary check at this point to see if there are actually any differences from the staged policy release to the currently deployed policy release and then only re-deploy if a change is found. A simple check could use diff -r. The main benefit of this would be to only deploy when necessary, instead of just doing it anyway. It's the small difference between being idempotent (same action repeatedly results in same final state) vs convergence which trys to limit unnecessary changes (at the expense of additional checking).

But I won't block the merge based on this.

nickanderson · 2016-10-05T14:26:25Z

Merged in #2707

Mike Weilgart added 21 commits January 5, 2016 13:42

Change DIRNAME to mydirname as it's not an environment variable.

18cca43

Add double quoting to variables so script won't break on spaces.

Fix error_exit calls for consistency

3bf4eb9

Add comment specifying env vars available

18f844c

Refactor STAGING_DIR setting to within function calls.

42da03f

Refactor duplicate code to function set_staging_dir_from_params

01e7822

Clean up comments (remove obsolete comment, add comment with section header). Make error_exit function def consistent with other defs syntax.

Refactor out check_git_installed function

96a849e

Add and incorporate validate_staged_policy function

c016016

Add functions for handling rollout:

6d499c5

avoid_triggering_unneeded_policy_updates, and rollout_staged_policy_to_masterdir

Add function git_setup_local_mirrored_repo

facdcdb

Add function git_stage_refspec

f11edd9

Add function git_stage_policy_channels_from_mirror

5ffc134

Add GIT_MIRROR_POLICY_CHANNELS as VCS_TYPE

ae11575

in masterfiles-stage.sh. It calls git_stage_policy_channels_from_mirror.

Add success message to git_stage_policy_channels_from_mirror

b1c4168

Add fully rewritten options.sh

53edd2d

Remove optparse library.

5764aa3

Update readme.org:

924da17

Document options.sh and new VCS_TYPE.

Remove 'mirror' from name of VCS_TYPE

a332eb9

since it's redundant; now ALL the git stage functions use a mirror. Change from GIT_MIRROR_POLICY_CHANNELS to GIT_POLICY_CHANNELS.

kacf assigned nickanderson Jan 7, 2016

tzz reviewed Jan 13, 2016
View reviewed changes

Remove obsolete comment line

986aa45

mikeweilgart reviewed Jan 22, 2016
View reviewed changes

nickanderson reviewed Jan 22, 2016
View reviewed changes

Improve permissions handling to prevent warning messages

537f3a9

Mike Weilgart added 2 commits January 22, 2016 12:51

Removed GIT_TAG_OR_COMMIT and document it.

19c6ddb

This VCS_TYPE was never publicly used and is now unneeded, as the GIT vcs type will handle git tags or commit hashes as well as git branches. README was brought in sync with recent code changes.

Remove readme reference to options.sh that doesn't now exist.

387b32c

mikeweilgart reviewed Jan 22, 2016
View reviewed changes

Add trap handling to common.sh

a57c700

Now the extra temp dir will get removed if the script error exits, which is particularly important if the code validation fails (since otherwise the temp staging dirs would build up every time the script runs\!)

nickanderson reviewed Jul 27, 2016
View reviewed changes

nickanderson merged commit a57c700 into cfengine:master Oct 5, 2016

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Masterfiles stage rewrite #2465

Masterfiles stage rewrite #2465

mikeweilgart commented Jan 7, 2016

cfengine-review-bot commented Jan 7, 2016

mikeweilgart commented Jan 7, 2016

mikeweilgart commented Jan 7, 2016

nickanderson commented Jan 7, 2016

tzz Jan 13, 2016

mikeweilgart Jan 15, 2016

tzz commented Jan 13, 2016

mikeweilgart Jan 22, 2016

nickanderson commented Jan 22, 2016

nickanderson Jan 22, 2016

mikeweilgart Jan 22, 2016

nickanderson commented Jan 22, 2016

nickanderson commented Jan 22, 2016

nickanderson commented Jan 22, 2016

mikeweilgart commented Jan 22, 2016

mikeweilgart Jan 22, 2016

nickanderson Jan 22, 2016

nickanderson Jan 22, 2016

nickanderson commented Jan 22, 2016

nickanderson commented Jan 22, 2016

mikeweilgart commented Jan 22, 2016

nickanderson commented Jun 2, 2016 •

edited

mikeweilgart commented Jun 3, 2016

nickanderson Jul 27, 2016

nickanderson commented Oct 5, 2016

Masterfiles stage rewrite #2465

Masterfiles stage rewrite #2465

Conversation

mikeweilgart commented Jan 7, 2016

cfengine-review-bot commented Jan 7, 2016

mikeweilgart commented Jan 7, 2016

mikeweilgart commented Jan 7, 2016

nickanderson commented Jan 7, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tzz commented Jan 13, 2016

Choose a reason for hiding this comment

nickanderson commented Jan 22, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nickanderson commented Jan 22, 2016

nickanderson commented Jan 22, 2016

nickanderson commented Jan 22, 2016

mikeweilgart commented Jan 22, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nickanderson commented Jan 22, 2016

nickanderson commented Jan 22, 2016

mikeweilgart commented Jan 22, 2016

nickanderson commented Jun 2, 2016 • edited

mikeweilgart commented Jun 3, 2016

Choose a reason for hiding this comment

nickanderson commented Oct 5, 2016

nickanderson commented Jun 2, 2016 •

edited