Merge lp:~nacc/curtin/iscsi-lp1679222 into lp:~curtin-dev/curtin/trunk

Proposed by Nish Aravamudan
Status: Merged
Merged at revision: 482
Proposed branch: lp:~nacc/curtin/iscsi-lp1679222
Merge into: lp:~curtin-dev/curtin/trunk
Diff against target: 189 lines (+69/-21)
4 files modified
curtin/block/iscsi.py (+1/-1)
tests/unittests/test_block_iscsi.py (+48/-6)
tests/vmtests/__init__.py (+2/-2)
tools/launch (+18/-12)
To merge this branch: bzr merge lp:~nacc/curtin/iscsi-lp1679222
Reviewer Review Type Date Requested Status
Server Team CI bot continuous-integration Approve
Blake Rouse (community) Approve
Review via email: mp+321906@code.launchpad.net

Description of the change

Fixes for ':' in iscsi target names

The regular expression we have now incorrectly allows ':' in IPv4
addresses and hostnames, rather than only IPv6 addresses. This ends up
as an inability to use ':' in target names (common and supported),
because we greedily consume the prefix (host) and suffix (target).

In order to fix this, we need to require that IPv6 addresses are enclosed in [], so we can disambiguate them from hostnames and IPv4 addresses which have different validation.

Fix the regex and add a testcase for this case.

Fixes: LP: #1679222

To post a comment you must log in.
Revision history for this message
Server Team CI bot (server-team-bot) wrote :
review: Approve (continuous-integration)
lp:~nacc/curtin/iscsi-lp1679222 updated
484. By Nish Aravamudan

tests/vmtests: insert a ':' into the vmtest iSCSI disks

Revision history for this message
Server Team CI bot (server-team-bot) wrote :
review: Approve (continuous-integration)
Revision history for this message
Blake Rouse (blake-rouse) wrote :

Works on my side! Thanks for the quick fix.

review: Approve
lp:~nacc/curtin/iscsi-lp1679222 updated
485. By Nish Aravamudan

vmtests: iSCSI targets can contain ':'

So we cannot use ':' as the separator for iscsi disk specifications to
tools/launch, because it becomes ambiguous and makes the code more
complicated. Instead, use '|' and special-case the `awk` delimiter if
'iscsi' is found in the disk specification to launch.

Revision history for this message
Server Team CI bot (server-team-bot) wrote :
review: Approve (continuous-integration)
Revision history for this message
Nish Aravamudan (nacc) wrote :
Revision history for this message
Scott Moser (smoser) wrote :

I think what i'd like to do, given blake's +1 above for actual test, and your proof in the vmtest run that this *does* work, is to merge 483 and 484, and then let us work out getting vmtest sorted.

I have some comments in line below, but they're more just code cleanups.

I dont' like having the --disk delimited one way if iscsi and another if not.
i think id' rather change everything to | than to just change some.
two other options for this are:
 a.) require escaping of whatever delimiter we choose (which should probably be done anyway)
 b.) allow the user to provide the delimiter ('--disk-delim=|').

Revision history for this message
Nish Aravamudan (nacc) wrote :

On 05.04.2017 [13:49:16 -0000], Scott Moser wrote:
> I think what i'd like to do, given blake's +1 above for actual test,
> and your proof in the vmtest run that this *does* work, is to merge
> 483 and 484, and then let us work out getting vmtest sorted.

Yep, that seems reasonable to me, and let's us SRU the fix back sooner
(which seems like the priority).

> I have some comments in line below, but they're more just code cleanups.
>
> I dont' like having the --disk delimited one way if iscsi and another if not.
> i think id' rather change everything to | than to just change some.
> two other options for this are:
> a.) require escaping of whatever delimiter we choose (which should probably be done anyway)
> b.) allow the user to provide the delimiter ('--disk-delim=|').

Agreed, that is better, I just didn't want to make the changes bigger :)

> Diff comments:
>
> >
> > === modified file 'tools/launch'
> > --- tools/launch 2017-03-22 21:03:58 +0000
> > +++ tools/launch 2017-04-04 23:50:04 +0000
> > @@ -470,13 +470,19 @@
> > # 3=src:size:driver
> > # 4=src:size:driver:bsize
> > # 5=src:size:driver:bsize:devopts
> > - # 6=src:size:iscsi:bsize:target
> > - # 7=src:size:iscsi:bsize:target:user:password:iuser:ipassword
> > - src=$(echo $disk | awk -F: '{print $1}')
> > - size=$(echo $disk | awk -F: '{print $2}')
> > - driver=$(echo $disk | awk -F: '{print $3}')
> > - bsize=$(echo $disk | awk -F: '{print $4}')
> > - devopts=$(echo $disk | awk -F: '{print $5}')
> > + # because iSCSI targets can contain ':'
> > + # 6=src|size|iscsi|bsize|target
> > + # 7=src|size|iscsi|bsize|target|user|password|iuser|ipassword
> > + delim=':'
> > + echo $disk | grep -q 'iscsi'
>
> case "$disk" in
> *\|*\|iscsi\|*) delim='|';;
> esac

Sure enough. Except * will match | so it doesn't do exactly what you
want, afaict? Meaning a badly formatted disk entry with 'iscsi' in the
4th or 5th |-separated field would get caught.

> > + if [ $? == 0 ]; then
> > + delim='|'
> > + fi
> > + src=$(echo $disk | awk -F$delim '{print $1}')
>
> this is more easily done like
> oifs=$IFS; IFS="$delim"; set -- $disk; IFS=oifs
> src="$1";
> size=${2:-5G}
> driver=${3:-virtio-blk}
> if [ "$driver" = "iscsi" ]; then
> target=${4}
> user=${5}
> pass=${6}
> fi
> driver="${2:-virtio-blk}";
> bsize="${3:512}";
> devopts="${4"
>
> its been hard for me to not do that before :)

Ah nice, I might stage that as a separate change so the | is only
changing to | and not restructuring the code.

Revision history for this message
Joshua Powers (powersj) wrote :

Marking as merged in 482

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
1=== modified file 'curtin/block/iscsi.py'
2--- curtin/block/iscsi.py 2017-02-18 00:33:38 +0000
3+++ curtin/block/iscsi.py 2017-04-04 23:50:04 +0000
4@@ -37,7 +37,7 @@
5 ''', re.VERBOSE)
6
7 RFC4173_TARGET_REGEX = re.compile(r'''^
8- (?P<host>[^@]*): # greedy so ipv6 IPs are matched
9+ (?P<host>[^@:\[\]]*|\[[^@]*\]): # greedy so ipv6 IPs are matched
10 (?P<proto>[^:]*):
11 (?P<port>[^:]*):
12 (?P<lun>[^:]*):
13
14=== modified file 'tests/unittests/test_block_iscsi.py'
15--- tests/unittests/test_block_iscsi.py 2017-02-16 00:59:12 +0000
16+++ tests/unittests/test_block_iscsi.py 2017-04-04 23:50:04 +0000
17@@ -93,12 +93,12 @@
18 self.assertEquals(i.lun, 0)
19 self.assertEquals(i.target, 'target')
20
21- i = iscsi.IscsiDisk('iscsi:fe80:::::target')
22+ i = iscsi.IscsiDisk('iscsi:[fe80::]::::target')
23 self.assertEquals(i.user, None)
24 self.assertEquals(i.password, None)
25 self.assertEquals(i.iuser, None)
26 self.assertEquals(i.ipassword, None)
27- self.assertEquals(i.host, 'fe80:')
28+ self.assertEquals(i.host, 'fe80::')
29 self.assertEquals(i.proto, '6')
30 self.assertEquals(i.port, 3260)
31 self.assertEquals(i.lun, 0)
32@@ -119,7 +119,8 @@
33 with self.assertRaisesRegexp(ValueError, 'Specified iSCSI port'):
34 iscsi.IscsiDisk('iscsi:192.168.1.12::ABCD::target')
35 with self.assertRaisesRegexp(ValueError, 'Specified iSCSI port'):
36- iscsi.IscsiDisk('iscsi:fe80::a634:d9ff:fe40:768a:6::ABCD::target')
37+ iscsi.IscsiDisk('iscsi:[fe80::a634:d9ff:fe40:768a:6]::ABCD::'
38+ 'target')
39 with self.assertRaisesRegexp(ValueError, 'Specified iSCSI port'):
40 iscsi.IscsiDisk('iscsi:test.example.com::ABCD::target')
41
42@@ -127,7 +128,7 @@
43 with self.assertRaisesRegexp(ValueError, 'Both host and targetname'):
44 iscsi.IscsiDisk('iscsi:192.168.1.12::::')
45 with self.assertRaisesRegexp(ValueError, 'Both host and targetname'):
46- iscsi.IscsiDisk('iscsi:fe80::a634:d9ff:fe40:768a:6::::')
47+ iscsi.IscsiDisk('iscsi:[fe80::a634:d9ff:fe40:768a:6]::::')
48 with self.assertRaisesRegexp(ValueError, 'Both host and targetname'):
49 iscsi.IscsiDisk('iscsi:test.example.com::::')
50
51@@ -140,7 +141,8 @@
52 with self.assertRaises(ValueError):
53 iscsi.IscsiDisk('iscsi:user@192.168.1.12::::target')
54 with self.assertRaises(ValueError):
55- iscsi.IscsiDisk('iscsi:user@fe80::a634:d9ff:fe40:768a:6::::target')
56+ iscsi.IscsiDisk('iscsi:user@[fe80::a634:d9ff:fe40:768a:6]::::'
57+ 'target')
58 with self.assertRaises(ValueError):
59 iscsi.IscsiDisk('iscsi:user@test.example.com::::target')
60
61@@ -149,7 +151,7 @@
62 iscsi.IscsiDisk('iscsi:user:password:iuser@192.168.1.12::::target')
63 with self.assertRaises(ValueError):
64 iscsi.IscsiDisk('iscsi:user:password:iuser@'
65- 'fe80::a634:d9ff:fe40:768a:6::::target')
66+ '[fe80::a634:d9ff:fe40:768a:6]::::target')
67 with self.assertRaises(ValueError):
68 iscsi.IscsiDisk(
69 'iscsi:user:password:iuser@test.example.com::::target')
70@@ -432,4 +434,44 @@
71 self.assertEquals(i.lun, 1)
72 self.assertEquals(i.target, 'target')
73
74+ # LP: #1679222
75+ def test_iscsi_target_parsing(self):
76+ i = iscsi.IscsiDisk(
77+ 'iscsi:192.168.1.12::::iqn.2017-04.com.example.test:target-name')
78+ self.assertEquals(i.user, None)
79+ self.assertEquals(i.password, None)
80+ self.assertEquals(i.iuser, None)
81+ self.assertEquals(i.ipassword, None)
82+ self.assertEquals(i.host, '192.168.1.12')
83+ self.assertEquals(i.proto, '6')
84+ self.assertEquals(i.port, 3260)
85+ self.assertEquals(i.lun, 0)
86+ self.assertEquals(i.target, 'iqn.2017-04.com.example.test:target-name')
87+
88+ i = iscsi.IscsiDisk(
89+ 'iscsi:[fe80::a634:d9ff:fe40:768a:6]::::'
90+ 'iqn.2017-04.com.example.test:target-name')
91+ self.assertEquals(i.user, None)
92+ self.assertEquals(i.password, None)
93+ self.assertEquals(i.iuser, None)
94+ self.assertEquals(i.ipassword, None)
95+ self.assertEquals(i.host, 'fe80::a634:d9ff:fe40:768a:6')
96+ self.assertEquals(i.proto, '6')
97+ self.assertEquals(i.port, 3260)
98+ self.assertEquals(i.lun, 0)
99+ self.assertEquals(i.target, 'iqn.2017-04.com.example.test:target-name')
100+
101+ i = iscsi.IscsiDisk(
102+ 'iscsi:test.example.com::::'
103+ 'iqn.2017-04.com.example.test:target-name')
104+ self.assertEquals(i.user, None)
105+ self.assertEquals(i.password, None)
106+ self.assertEquals(i.iuser, None)
107+ self.assertEquals(i.ipassword, None)
108+ self.assertEquals(i.host, 'test.example.com')
109+ self.assertEquals(i.proto, '6')
110+ self.assertEquals(i.port, 3260)
111+ self.assertEquals(i.lun, 0)
112+ self.assertEquals(i.target, 'iqn.2017-04.com.example.test:target-name')
113+
114 # vi: ts=4 expandtab syntax=python
115
116=== modified file 'tests/vmtests/__init__.py'
117--- tests/vmtests/__init__.py 2017-03-22 16:41:51 +0000
118+++ tests/vmtests/__init__.py 2017-04-04 23:50:04 +0000
119@@ -454,10 +454,10 @@
120 uuid, _ = util.subp(['uuidgen'], capture=True,
121 decode='replace')
122 uuid = uuid.rstrip()
123- target = 'curtin-%s' % uuid
124+ target = 'iqn.2017-04.com.canonical:curtin-%s' % uuid
125 cls._iscsi_disks.append(target)
126 dpath = os.path.join(cls.td.disks, '%s.img' % (target))
127- iscsi_disk = '{}:{}:iscsi:{}:{}:{}:{}:{}:{}'.format(
128+ iscsi_disk = '{}|{}|iscsi|{}|{}|{}|{}|{}|{}'.format(
129 dpath, disk_sz, cls.disk_block_size, target,
130 disk_user, disk_password, disk_iuser, disk_ipassword)
131 disks.extend(['--disk', iscsi_disk])
132
133=== modified file 'tools/launch'
134--- tools/launch 2017-03-22 21:03:58 +0000
135+++ tools/launch 2017-04-04 23:50:04 +0000
136@@ -470,13 +470,19 @@
137 # 3=src:size:driver
138 # 4=src:size:driver:bsize
139 # 5=src:size:driver:bsize:devopts
140- # 6=src:size:iscsi:bsize:target
141- # 7=src:size:iscsi:bsize:target:user:password:iuser:ipassword
142- src=$(echo $disk | awk -F: '{print $1}')
143- size=$(echo $disk | awk -F: '{print $2}')
144- driver=$(echo $disk | awk -F: '{print $3}')
145- bsize=$(echo $disk | awk -F: '{print $4}')
146- devopts=$(echo $disk | awk -F: '{print $5}')
147+ # because iSCSI targets can contain ':'
148+ # 6=src|size|iscsi|bsize|target
149+ # 7=src|size|iscsi|bsize|target|user|password|iuser|ipassword
150+ delim=':'
151+ echo $disk | grep -q 'iscsi'
152+ if [ $? == 0 ]; then
153+ delim='|'
154+ fi
155+ src=$(echo $disk | awk -F$delim '{print $1}')
156+ size=$(echo $disk | awk -F$delim '{print $2}')
157+ driver=$(echo $disk | awk -F$delim '{print $3}')
158+ bsize=$(echo $disk | awk -F$delim '{print $4}')
159+ devopts=$(echo $disk | awk -F$delim '{print $5}')
160
161 if [ -z "${src}" ]; then
162 error "Failed to provide disk source"
163@@ -510,21 +516,21 @@
164 if [ "${driver}" == "iscsi" ]; then
165 local target="" tid="" user="" password=""
166 local iuser="" ipassword=""
167- target=$(echo "$disk" | awk -F: '{print $5}') &&
168+ target=$(echo "$disk" | awk -F$delim '{print $5}') &&
169 [ -n "$target" ] || {
170 error "empty target for iSCSI disk '$disk'"
171 return 1
172 }
173- user=$(echo "$disk" | awk -F: '{print $6}')
174- password=$(echo "$disk" | awk -F: '{print $7}')
175+ user=$(echo "$disk" | awk -F$delim '{print $6}')
176+ password=$(echo "$disk" | awk -F$delim '{print $7}')
177 [ -n "$user" -a -n "$password" ] || \
178 [ -z "$user" -a -z "$password" ] || {
179 error "both target user ($user) and password ($password) " \
180 "must be specified for iSCSI disk '$disk'"
181 return 1
182 }
183- iuser=$(echo "$disk" | awk -F: '{print $8}')
184- ipassword=$(echo "$disk" | awk -F: '{print $9}')
185+ iuser=$(echo "$disk" | awk -F$delim '{print $8}')
186+ ipassword=$(echo "$disk" | awk -F$delim '{print $9}')
187 [ -n "$iuser" -a -n "$ipassword" ] || \
188 [ -z "$iuser" -a -z "$ipassword" ] || {
189 error "both initiator user ($iuser) and password ($ipassword) " \

Subscribers

People subscribed via source and target branches