charm: Fix bug #864164 - non-exec hooks

charm: Fix bug #864164 - non-exec hooks

Changed ExpandTo() to force executable permissions to hooks
which are lacking these permissions (setting --x--x--x only).
Added a check in BundleTo() to issue warnings if any hooks
are found to lack executable permissions, telling the user 
juju will set --x--x--x for these hooks.
Also added tests for both changes. Only files inside hooks/
are checked, if they are named like hooks.

https://code.launchpad.net/~dimitern/juju-core/bug-864164-check-charm-hooks-are-executable/+merge/148071

(do not edit description out of merge proposal)

[dimitern, 2013-02-13 01:31:54]

Please take a look.

[jameinel, 2013-02-13 04:06:16]

LGTM

https://codereview.appspot.com/7301085/diff/1/charm/bundle.go
File charm/bundle.go (right):

https://codereview.appspot.com/7301085/diff/1/charm/bundle.go#newcode231
charm/bundle.go:231: }
Offhand, I wonder if 0110 or even 0100 be better. I'll go with whatever the bug
was suggesting, but I don't see a specific need to make files other and group
executable.

https://codereview.appspot.com/7301085/diff/1/charm/dir.go
File charm/dir.go (right):

https://codereview.appspot.com/7301085/diff/1/charm/dir.go#newcode146
charm/dir.go:146: }
isn't this the same as:
return perm & 0111 == 0111
?

I find it harder to sort through the three clauses above, though maybe some
people feel differently?

https://codereview.appspot.com/7301085/diff/1/charm/dir.go#newcode180
charm/dir.go:180: }
Your warning here is saying it is forcing the executable bit, but you aren't
actually changing them here.

[fwereade, 2013-02-13 10:31:47]

Looking good -- some comments, some of which may be best addressed by adding a
bug and proposing a new CL.

https://codereview.appspot.com/7301085/diff/1/charm/bundle.go
File charm/bundle.go (right):

https://codereview.appspot.com/7301085/diff/1/charm/bundle.go#newcode187
charm/bundle.go:187: var hookNameRegexp =
regexp.MustCompile("^(hooks/)?(install|start|config-changed|upgrade-charm|stop)|([^-]+-relation-(joined|changed|departed|broken))$")
I think this bit should be improved a little.

1) The list of valid hook names duplicates stuff in worker/unit/hook -- if you
can see a clean way to avoid this, I think we should, otherwise as we add new
hooks we'll end up diverging.

2) I don't think we should touch *-relation-* hooks that don't correspond to a
relation named in the metadata.

https://codereview.appspot.com/7301085/diff/1/charm/bundle.go#newcode231
charm/bundle.go:231: }
On 2013/02/13 04:06:17, jameinel wrote:
> Offhand, I wonder if 0110 or even 0100 be better. I'll go with whatever the
bug
> was suggesting, but I don't see a specific need to make files other and group
> executable.

+1

https://codereview.appspot.com/7301085/diff/1/charm/bundle_test.go
File charm/bundle_test.go (right):

https://codereview.appspot.com/7301085/diff/1/charm/bundle_test.go#newcode10
charm/bundle_test.go:10: "launchpad.net/juju-core/worker/uniter/hook"
Hmm... even though it's a test, I'd really rather not use these here. I think
the constants should maybe come from closer to home...maybe move them to a
trivial new charm/hook package?

https://codereview.appspot.com/7301085/diff/1/charm/bundle_test.go#newcode112
charm/bundle_test.go:112: }
Please add some checks for (1) random names and (2) *-relation-* names that
don't match the metadata.

https://codereview.appspot.com/7301085/diff/1/charm/dir.go
File charm/dir.go (right):

https://codereview.appspot.com/7301085/diff/1/charm/dir.go#newcode146
charm/dir.go:146: }
On 2013/02/13 04:06:17, jameinel wrote:
> isn't this the same as:
> return perm & 0111 == 0111
> ?
> 
> I find it harder to sort through the three clauses above, though maybe some
> people feel differently?

+1

https://codereview.appspot.com/7301085/diff/1/charm/dir.go#newcode179
charm/dir.go:179: log.Printf("charm: WARNING: forcing hook %q in charm %q to
executable", hookName, cname)
Please don't use (only?) log.Printf for warnings we want users to see: (1) they
won't and (2) they'll be marred by ugly badges.

I am uncomfortably aware that this request is basically "please fix our
logging"... maybe chat online?

https://codereview.appspot.com/7301085/diff/1/charm/dir.go#newcode180
charm/dir.go:180: }
On 2013/02/13 04:06:17, jameinel wrote:
> Your warning here is saying it is forcing the executable bit, but you aren't
> actually changing them here.

+1

https://codereview.appspot.com/7301085/diff/1/charm/dir_test.go
File charm/dir_test.go (right):

https://codereview.appspot.com/7301085/diff/1/charm/dir_test.go#newcode172
charm/dir_test.go:172: c.Assert(tlog, Matches, `(?m)`+expectHooks)
I'd like to see at least one test that we *don't* log executable hooks. This
interacts with the earlier comment about precisely how we log them though...

[dimitern, 2013-02-13 11:28:19]

Thanks for the reviews, here are my answers.

https://codereview.appspot.com/7301085/diff/1/charm/bundle.go
File charm/bundle.go (right):

https://codereview.appspot.com/7301085/diff/1/charm/bundle.go#newcode187
charm/bundle.go:187: var hookNameRegexp =
regexp.MustCompile("^(hooks/)?(install|start|config-changed|upgrade-charm|stop)|([^-]+-relation-(joined|changed|departed|broken))$")
On 2013/02/13 10:31:48, fwereade wrote:
> I think this bit should be improved a little.
> 
> 1) The list of valid hook names duplicates stuff in worker/unit/hook -- if you
> can see a clean way to avoid this, I think we should, otherwise as we add new
> hooks we'll end up diverging.
> 
> 2) I don't think we should touch *-relation-* hooks that don't correspond to a
> relation named in the metadata.

At first I started using strings.* and a few nested ifs, so I can reuse the
hook.* relation constants, but it got ugly quickly and with a single regexp I
can test all cases. But it's not ideal, I agree.

And good point about the metadata - I forgot to check for existing relations
there.

https://codereview.appspot.com/7301085/diff/1/charm/bundle.go#newcode231
charm/bundle.go:231: }
On 2013/02/13 10:31:48, fwereade wrote:
> On 2013/02/13 04:06:17, jameinel wrote:
> > Offhand, I wonder if 0110 or even 0100 be better. I'll go with whatever the
> bug
> > was suggesting, but I don't see a specific need to make files other and
group
> > executable.
> 
> +1

So 0110, 0100 or what?

https://codereview.appspot.com/7301085/diff/1/charm/bundle_test.go
File charm/bundle_test.go (right):

https://codereview.appspot.com/7301085/diff/1/charm/bundle_test.go#newcode10
charm/bundle_test.go:10: "launchpad.net/juju-core/worker/uniter/hook"
On 2013/02/13 10:31:48, fwereade wrote:
> Hmm... even though it's a test, I'd really rather not use these here. I think
> the constants should maybe come from closer to home...maybe move them to a
> trivial new charm/hook package?

Good point - I think having all hook consts and a few slices to iterate over
them easily. Maybe I can leave this and follow-up with the new module?

https://codereview.appspot.com/7301085/diff/1/charm/bundle_test.go#newcode112
charm/bundle_test.go:112: }
On 2013/02/13 10:31:48, fwereade wrote:
> Please add some checks for (1) random names and (2) *-relation-* names that
> don't match the metadata.

I'll do that, thanks for pointing out. That's why you shouldn't do stuff in 2AM
:)

https://codereview.appspot.com/7301085/diff/1/charm/dir.go
File charm/dir.go (right):

https://codereview.appspot.com/7301085/diff/1/charm/dir.go#newcode146
charm/dir.go:146: }
On 2013/02/13 10:31:48, fwereade wrote:
> On 2013/02/13 04:06:17, jameinel wrote:
> > isn't this the same as:
> > return perm & 0111 == 0111
> > ?
> > 
> > I find it harder to sort through the three clauses above, though maybe some
> > people feel differently?
> 
> +1

Wasn't even sure I should check uga rather than just u or ug. Probably 0110 is
enough?

https://codereview.appspot.com/7301085/diff/1/charm/dir.go#newcode180
charm/dir.go:180: }
On 2013/02/13 10:31:48, fwereade wrote:
> On 2013/02/13 04:06:17, jameinel wrote:
> > Your warning here is saying it is forcing the executable bit, but you aren't
> > actually changing them here.
> 
> +1
So the warning was one of the points of the bug, but the forcing will happen at
expand time. Should it happen here as well?

https://codereview.appspot.com/7301085/diff/1/charm/dir_test.go
File charm/dir_test.go (right):

https://codereview.appspot.com/7301085/diff/1/charm/dir_test.go#newcode172
charm/dir_test.go:172: c.Assert(tlog, Matches, `(?m)`+expectHooks)
On 2013/02/13 10:31:48, fwereade wrote:
> I'd like to see at least one test that we *don't* log executable hooks. This
> interacts with the earlier comment about precisely how we log them though...

I'll do that, ok.

[jameinel, 2013-02-13 12:01:33]

https://codereview.appspot.com/7301085/diff/1/charm/bundle.go
File charm/bundle.go (right):

https://codereview.appspot.com/7301085/diff/1/charm/bundle.go#newcode187
charm/bundle.go:187: var hookNameRegexp =
regexp.MustCompile("^(hooks/)?(install|start|config-changed|upgrade-charm|stop)|([^-]+-relation-(joined|changed|departed|broken))$")
...

> 2) I don't think we should touch *-relation-* hooks that don't correspond to a
> relation named in the metadata.

I think that in theory it is good to not touch extra stuff. In practice, I don't
think it really matters vs the effort of trying to make sure you don't get one
wrong.
If we just set everything in hooks to user-executable I don't think we'll
actually open up any security holes, and it means we are future proof against
new hooks being added.

https://codereview.appspot.com/7301085/diff/1/charm/bundle.go#newcode231
charm/bundle.go:231: }
0100 is my preference

[dimitern, 2013-02-14 12:32:02]

As discussed, I'll close this and propose a simpler solution, addressing what
was suggested, having https://codereview.appspot.com/7303091 as a prerequisite.