Merge lp:~lazypower/charms/precise/nrpe/fix-lp-1287393 into lp:charms/nrpe

Proposed by Charles Butler
Status: Merged
Approved by: Peter Petrakis
Approved revision: 30
Merged at revision: 28
Proposed branch: lp:~lazypower/charms/precise/nrpe/fix-lp-1287393
Merge into: lp:charms/nrpe
Diff against target: 206 lines (+189/-1)
2 files modified
hooks/nrpe-relation-joined (+1/-1)
lib/net.sh (+188/-0)
To merge this branch: bzr merge lp:~lazypower/charms/precise/nrpe/fix-lp-1287393
Reviewer Review Type Date Requested Status
Charles Butler (community) Approve
Matt Bruzek (community) Approve
Peter Petrakis Pending
Review via email: mp+209137@code.launchpad.net

Description of the change

replaces charm-helpers-sh call with nslookup

https://codereview.appspot.com/69980048/

To post a comment you must log in.
Revision history for this message
Charles Butler (lazypower) wrote :

Reviewers: mp+209137_code.launchpad.net,

Message:
Please take a look.

Description:
replaces charm-helpers-sh call with nslookup

https://code.launchpad.net/~lazypower/charms/precise/nrpe/fix-lp-1287393/+merge/209137

(do not edit description out of merge proposal)

Please review this at https://codereview.appspot.com/69980048/

Affected files (+3, -1 lines):
   A [revision details]
   M hooks/nrpe-relation-joined

Index: [revision details]
=== added file '[revision details]'
--- [revision details] 2012-01-01 00:00:00 +0000
+++ [revision details] 2012-01-01 00:00:00 +0000
@@ -0,0 +1,2 @@
+Old revision: <email address hidden>
+New revision: <email address hidden>

Index: hooks/nrpe-relation-joined
=== modified file 'hooks/nrpe-relation-joined'
--- hooks/nrpe-relation-joined 2012-07-05 22:22:37 +0000
+++ hooks/nrpe-relation-joined 2014-03-03 22:05:22 +0000
@@ -7,7 +7,7 @@
  . hooks/nrpe.common.bash

  # NRPE requires IP
-address=$(ch_get_ip $(relation-get private-address))
+address=`nslookup $(unit-get private-address) | grep Add | grep -v '#' |
cut -f 2 -d ' '`

  juju-log "Adding $address to allowed_hosts."

Revision history for this message
Matt Bruzek (mbruzek) wrote :

I have a comment/question about the new command you are using.

https://codereview.appspot.com/69980048/diff/1/hooks/nrpe-relation-joined
File hooks/nrpe-relation-joined (right):

https://codereview.appspot.com/69980048/diff/1/hooks/nrpe-relation-joined#newcode10
hooks/nrpe-relation-joined:10: address=`nslookup $(unit-get
private-address) | grep Add | grep -v '#' | cut -f 2 -d ' '`
I ran this nslookup command on my own system and I believe the two greps
are cancelling each out.

$ nslookup 192.168.168.13
Server: 127.0.1.1
Address: 127.0.1.1#53

** server can't find 13.168.168.192.in-addr.arpa.: NXDOMAIN

$ nslookup 192.168.168.13 | grep Add
Address: 127.0.1.1#53

$ nslookup 192.168.168.13 | grep Add | grep -v '#'
(no match)

Are you sure this command works in all cases? Am I using the command
incorrectly here, or should we consider refining the command to work in
this case too?

https://codereview.appspot.com/69980048/

Revision history for this message
Matt Bruzek (mbruzek) wrote :

This change needs more information.

Add a comment to make it more clear what IP address the code is trying to get. The new code gets the unit address rather than the relation address (which I realize may be the same one).

Since there appears to be a possiblity of getting an empty address. Either fall back to the old way of getting the address or log a warning, or error if the address resolves to an empty string.

review: Needs Information
29. By Charles Butler

Embed lib/net.sh instead of rewriting the world

Revision history for this message
Charles Butler (lazypower) wrote :

Embedded net.sh with an addition of returning $HOST if found.

Refactored out the cowboy nslookup that fails on local. Give it another go please

30. By Charles Butler

remove stray return 1 for debugging

Revision history for this message
Charles Butler (lazypower) wrote :
Revision history for this message
Matt Bruzek (mbruzek) wrote :

+1 LGTM

review: Approve
Revision history for this message
Charles Butler (lazypower) wrote :

Merged

review: Approve

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
=== modified file 'hooks/nrpe-relation-joined'
--- hooks/nrpe-relation-joined 2012-07-05 22:22:37 +0000
+++ hooks/nrpe-relation-joined 2014-03-19 12:23:25 +0000
@@ -3,7 +3,7 @@
33
4set -e4set -e
55
6. /usr/share/charm-helper/sh/net.sh6. lib/net.sh
7. hooks/nrpe.common.bash7. hooks/nrpe.common.bash
88
9# NRPE requires IP9# NRPE requires IP
1010
=== added directory 'lib'
=== added file 'lib/net.sh'
--- lib/net.sh 1970-01-01 00:00:00 +0000
+++ lib/net.sh 2014-03-19 12:23:25 +0000
@@ -0,0 +1,188 @@
1#!/bin/sh
2
3##
4# Copyright 2011 Marco Ceppi <marco@ceppi.net>
5#
6# This file is part of Charm Helpers.
7#
8# Charm Helpers is free software: you can redistribute it and/or modify
9# it under the terms of the GNU General Public License as published by
10# the Free Software Foundation, either version 3 of the License, or
11# (at your option) any later version.
12#
13# Charm Helpers is distributed in the hope that it will be useful,
14# but WITHOUT ANY WARRANTY; without even the implied warranty of
15# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
16# GNU General Public License for more details.
17#
18# You should have received a copy of the GNU General Public License
19# along with Charm Helpers. If not, see <http://www.gnu.org/licenses/>.
20##
21
22##
23# Globally overridable settings. Make sure to set them before sourcing
24# this file.
25CH_WGET_ARGS=${CH_WGET_ARGS:-"-q --content-disposition"}
26
27##
28# Get File
29# Retrives a file and compares the file to a hash
30#
31# param FILE URL or Path to file
32# param HASH URL, Path, or HASH string for file's hash
33#
34# return filepath|null
35##
36ch_get_file()
37{
38 local FILE=${1:-""}
39 local HASH=${2:-""}
40 local CH_DOWNLOAD_DIR=${CH_DOWNLOAD_DIR:-"`mktemp -d /tmp/ch-downloads.XXXXXX`"}
41
42 if [ `ch_is_url "$FILE"` ]; then
43 wget $CH_WGET_ARGS --directory-prefix="$CH_DOWNLOAD_DIR/" "$FILE"
44 FILE=$CH_DOWNLOAD_DIR/$(ls -tr $CH_DOWNLOAD_DIR|head -n 1)
45 fi
46
47 if [ ! -f "$FILE" ]; then
48 return 2
49 fi
50
51 if [ -z "$HASH" ];then
52 #echo "Warning, no has specified. The file will be downloaded but not cryptographically checked!" > 2
53 echo "$FILE"
54 return 0
55 elif [ `ch_is_url "$HASH"` ]; then
56 local HASHNAME=$(basename $HASH)
57 wget $CH_WGET_ARGS "$HASH" -O /tmp/$HASHNAME
58 HASH=$(cat /tmp/$HASHNAME | awk '{ print $1 }')
59 elif [ -f "$HASH" ]; then
60 HASH=$(cat "$HASH" | awk '{ print $1 }')
61 fi
62
63 local HASH_TYPE=$(ch_type_hash $HASH)
64
65 if [ -z "$HASH_TYPE" ]; then
66 return 3
67 else
68 local FILE_HASH=$(${HASH_TYPE}sum $FILE | awk '{ print $1 }')
69
70 if [ "$FILE_HASH" != "$HASH" ]; then
71 return 4
72 fi
73 fi
74
75 echo "$FILE"
76 return 0
77}
78
79##
80# Hash Type
81# Determine, using best approximation, if the hash is valid and what type
82# of hashing algorithm was used
83#
84# param HASH
85#
86# return type|false
87##
88ch_type_hash()
89{
90 local DIRTY="$1"
91
92 case $DIRTY in
93 *[![:xdigit:]]* | "" )
94 echo ""
95 return 1
96 ;;
97 * )
98 case ${#DIRTY} in
99 32 )
100 echo md5
101 ;;
102 40 )
103 echo sha1
104 ;;
105 64 )
106 echo sha256
107 ;;
108 esac
109 ;;
110 esac
111}
112
113##
114# Is URL?
115# Checks if the string passed is a valid URL (http(s), ftp)
116#
117# param URL The URL to be checked
118#
119# return boolean
120##
121ch_is_url()
122{
123 local DIRTY="$1"
124
125 case "$DIRTY" in
126 "http://"* | "https://"* | "ftp://"*)
127 echo true
128 return 0
129 ;;
130 *)
131 return 1
132 ;;
133 esac
134}
135
136##
137# Is IP?
138# Checks if the string passed is an IP address
139#
140# param IP The IP addressed to be checked
141#
142# return boolean
143##
144ch_is_ip()
145{
146 local DIRTY="$1"
147 local IP=$(echo $DIRTY | awk -F. '$1 <=255 && $2 <= 255 && $3 <= 255 && $4 <= 255 ')
148
149 if [ -z "$IP" ]; then
150 return 1
151 else
152 echo true
153 return 0
154 fi
155}
156
157##
158# Get IP
159# Returns the first IP match to the hostname provided
160#
161# param HOSTNAME Host for which to retrieve an IP
162#
163# return IP|false
164##
165ch_get_ip()
166{
167 local HOST="$1"
168 #So, if there's multiple IP addresses, just grab the first
169 # for now.
170 if [ `ch_is_ip "$HOST"` ]; then
171 echo "$HOST"
172 return 0
173 fi
174
175 local CHECK_IP=$(host -t A $HOST | awk 'NR==1{ print $4 }')
176
177 if [ ! `ch_is_ip "$CHECK_IP"` ]; then
178 # Try a dig, why not?
179 CHECK_IP=$(dig +short $HOST | awk 'NR==1{ print $1 }')
180
181 if [ ! `ch_is_ip "$CHECK_IP"` ]; then
182 return 1
183 fi
184 fi
185
186 echo $CHECK_IP
187 return 0
188}

Subscribers

People subscribed via source and target branches