Differential D512

Support for pubkey
AbandonedPublic
Actions

Authored by lbrabec on Aug 19 2015, 8:57 AM.

Details

Reviewers
kparal
roshi
tflink
jskladan
Summary

This diff introduces support for pubkey, which is added through cloud-init. WIP, open for discussion.

Test Plan

manually tested with adjusted taskotron disposable-develop branch

Diff Detail

Repository
rTCLOUD testcloud
Branch
feature/pubkey
Lint
Lint OK
Unit
Unit Tests OK
Build Status
Buildable 123
Build 123: arc lint + arc unit
lbrabec retitled this revision from to Support for pubkey.Aug 19 2015, 8:57 AM
lbrabec updated this object.
lbrabec edited the test plan for this revision. (Show Details)
lbrabec added reviewers: jskladan, kparal, tflink.
jskladan added inline comments.Aug 21 2015, 9:42 AM
testcloud/config.py
94

isn't the password valid for fedora user?

tflink added a reviewer: roshi.Aug 21 2015, 7:56 PM
tflink added a subscriber: roshi.

@roshi should be added to all testcloud reviews

tflink requested changes to this revision.Aug 21 2015, 8:04 PM

Other than the root-as-default bit, it looks pretty good to me.

The other thing I'm wondering is if --username, --password and --pubkey options should be added to the CLI. If so, that could be done as another ticket.

testcloud/config.py
94

I don't think that root is an appropriate default. It's what we end up using in Taskotron but I don't see a reason to deviate from the default in the cloud WG in testcloud

This revision now requires changes to proceed.Aug 21 2015, 8:04 PM
tflink added a comment.Aug 21 2015, 8:17 PM

I almost forgot - the config changes should go into conf/settings-example.py as well with some sane defaults

lbrabec updated this revision to Diff 1381.Aug 25 2015, 10:39 AM
  • code polishing
lbrabec added a comment.Aug 25 2015, 1:25 PM
In D512#9845, @tflink wrote:

The other thing I'm wondering is if --username, --password and --pubkey options should be added to the CLI. If so, that could be done as another ticket.

This is partially covered by the --ssh user@machine option, mainly for debugging reasons. In production, I would expect the taskotron to specify username (and password) for me.

Regarding the --pubkey option, we could use --ssh-privkey option and expect the public key to be named same as privkey only with .pub suffix. I think it is the most common naming and we can always implement --pubkey later. Or do I miss something and inferring pubkey name from privkey is bad idea?

tflink requested changes to this revision.Aug 27 2015, 9:24 PM

sorry, missed this the first time around

testcloud/instance.py
117

now that I'm looking at this again, I don't like having bits that can throw exceptions in constructors

please move the file opening bits outside the constructor or wrap it in try/except and log the error.

This revision now requires changes to proceed.Aug 27 2015, 9:24 PM
kparal added inline comments.Aug 28 2015, 8:26 AM
testcloud/instance.py
117

I don't know the code much, but this comment caught my attention, so I searched a bit for Python guidelines about exceptions in constructors. I've found this:
http://stackoverflow.com/questions/1507082/python-is-it-bad-form-to-raise-exceptions-within-init

After reading it, it seems obviously OK to raise stuff like ValueError for invalid parameter values. Reading a file and potentially raising an IOError (or maybe even something else, not sure what can all happen during .read()), is something a bit different, but for very short files (like pubkeys), I find it quite similar to just an invalid parameter value. So I wouldn't mind. But the exception should be definitely documented in a docstring, so people can guard against it. Or, if it can potentially raise multiple errors, or we want to make it easy for people to handle testcloud errors, it could be wrapped in a try-except (even blank except) block and raised as a custom testcloud error. What I wouldn't do is just to log the error and fall back to pubkey=None. The user explicitly requested a particular pubkey to work, it should fail if we can't make it happen.

The other possible approach is to move the file reading code to the actual method that uses it (I guess that's _create_user_data). It will not raise during constructor init, which I find helpful (faster class initialization, no potentially intensive code run in init) and unhelpful (it's better to fail early than late, it wastes user's time and it's much harder to tear down stuff later on).

So, that was just me pondering about this.

roshi accepted this revision.Aug 28 2015, 4:11 PM

I'm still unsure about the need for this change - since you can do this yourself in the config file (it'll take any valid cloud-init data), totally side stepping the need for reading in a file. But my reservations aren't enough to say no to it. It just means there are now 2 places you can do this from which could potentially add to debug time when helping users troubleshoot.

Overall, the change looks good. I don't personally see a reason to not allow exceptions to be raised in init, and the arguments from @kparal's link were compelling.

lbrabec abandoned this revision.Jul 25 2016, 11:56 AM

Revision Contents

PathCoverage (All)Coverage (Touched)Packages
Mconf/settings-example.py (11 lines)--
Mtestcloud/config.py (13 lines)93%100%
Mtestcloud/instance.py (18 lines)40%54%

Diff 1381

View Options

conf/settings-example.py

1# Commented out default values with details are displayed below. To override 1​# Commented out default values with details are displayed below. To override
2# these default values, uncomment the values, rename the file to settings.py 2​# these default values, uncomment the values, rename the file to settings.py
3# and place it in a supported config location 3​# and place it in a supported config location
4 4​
5# Do not modify this file directly, it will not be picked up unless the filename 5​# Do not modify this file directly, it will not be picked up unless the filename
6# is changed to settings.py 6​# is changed to settings.py
7 7​
8#DOWNLOAD_PROGRESS = True 8​#DOWNLOAD_PROGRESS = True
9 9​
10# Directories for data and cached downloaded images 10​# Directories for data and cached downloaded images
11#DATA_DIR = "/var/lib/testcloud/" 11​#DATA_DIR = "/var/lib/testcloud/"
12#CACHE_DIR = "/var/lib/testcloud/cache" 12​#CACHE_DIR = "/var/lib/testcloud/cache"
13 13​
14# Data for cloud-init 14​# Data for cloud-init
15 15​
16​#USERNAME = 'fedora'
16#PASSWORD = 'passw0rd' 17​#PASSWORD = 'passw0rd'
17#HOSTNAME = 'testcloud' 18​#HOSTNAME = 'testcloud'
18 19​
19#META_DATA = """instance-id: iid-123456 20​#META_DATA = """instance-id: iid-123456
20#local-hostname: %s 21​#local-hostname: %s
21#""" 22​#"""
22#USER_DATA = """#cloud-config 23​#USER_DATA = """#cloud-config
23#password: %s 24​#users:
24#chpasswd: { expire: False } 25​#- default
25#ssh_pwauth: True 26​#- name: %s
27​# password: %s
28​# chpasswd: { expire: False }
29​# ssh-authorized-keys:
30​# - %s
26#""" 31​#"""
27#ATOMIC_USER_DATA = """#cloud-config 32​#ATOMIC_USER_DATA = """#cloud-config
28#password: %s 33​#password: %s
29#chpasswd: { expire: False } 34​#chpasswd: { expire: False }
30#ssh_pwauth: True 35​#ssh_pwauth: True
31#runcmd: 36​#runcmd:
32# - [ sh, -c, 'echo -e "ROOT_SIZE=4G\nDATA_SIZE=10G" > /etc/sysconfig/docker-storage-setup'] 37​# - [ sh, -c, 'echo -e "ROOT_SIZE=4G\nDATA_SIZE=10G" > /etc/sysconfig/docker-storage-setup']
33#""" 38​#"""
34# 39​#
35# Extra cmdline args for the qemu invocation. 40​# Extra cmdline args for the qemu invocation.
36# Customize as needed :) 41​# Customize as needed :)
37 42​
38#CMD_LINE_ARGS = [] 43​#CMD_LINE_ARGS = []
View Options

testcloud/config.py

Show First 20 Lines • Show All 85 Lines • ▼ Show 20 Line(s)80class ConfigData(object):
86 86​
87 # Directories testcloud cares about 87​ # Directories testcloud cares about
88 88​
89 DATA_DIR = "/var/lib/testcloud" 89​ DATA_DIR = "/var/lib/testcloud"
90 CACHE_DIR = "/var/lib/testcloud/cache" 90​ CACHE_DIR = "/var/lib/testcloud/cache"
91 91​
92 # Data for cloud-init 92​ # Data for cloud-init
93 93​
94​ USERNAME = 'fedora'
jskladanUnsubmitted
Not Done

isn't the password valid for fedora user?

tflinkUnsubmitted
Not Done

I don't think that root is an appropriate default. It's what we end up using in Taskotron but I don't see a reason to deviate from the default in the cloud WG in testcloud

94 PASSWORD = 'passw0rd' 95​ PASSWORD = 'passw0rd'
95 HOSTNAME = 'testcloud' 96​ HOSTNAME = 'testcloud'
96 97​
97 META_DATA = """instance-id: iid-123456 98​ META_DATA = """instance-id: iid-123456
98local-hostname: %s 99​local-hostname: %s
99 """ 100​ """
100 USER_DATA = """#cloud-config 101​ USER_DATA = """#cloud-config
101password: %s 102​users:
102chpasswd: { expire: False } 103​ - default
103ssh_pwauth: True 104​ - name: %s
104 """ 105​ password: %s
106​ chpasswd: { expire: False }
107​ ssh-authorized-keys:
108​ - %s
109​"""
105 ATOMIC_USER_DATA = """#cloud-config 110​ ATOMIC_USER_DATA = """#cloud-config
106password: %s 111​password: %s
107chpasswd: { expire: False } 112​chpasswd: { expire: False }
108ssh_pwauth: True 113​ssh_pwauth: True
109runcmd: 114​runcmd:
110 - [ sh, -c, 'echo -e "ROOT_SIZE=4G\nDATA_SIZE=10G" > /etc/sysconfig/docker-storage-setup'] 115​ - [ sh, -c, 'echo -e "ROOT_SIZE=4G\nDATA_SIZE=10G" > /etc/sysconfig/docker-storage-setup']
111 """ 116​ """
112 117​
Show All 16 Lines
View Options

testcloud/instance.py

Show First 20 Lines • Show All 100 Lines • ▼ Show 20 Line(s)84def list_instances(connection='qemu:///system'):
101 return instances 101​ return instances
102 102​
103 103​
104class Instance(object): 104​class Instance(object):
105 """Handles creating, starting, stopping and destroying virtual machines 105​ """Handles creating, starting, stopping and destroying virtual machines
106 defined on the local system, using an existing :py:class:`Image`. 106​ defined on the local system, using an existing :py:class:`Image`.
107 """ 107​ """
108 108​
109 def __init__(self, name, image=None): 109​ def __init__(self, name, image=None, user=None, password=None, pubkey=None):
110 self.name = name 110​ self.name = name
111 self.image = image 111​ self.image = image
112 self.path = "{}/instances/{}".format(config_data.DATA_DIR, self.name) 112​ self.path = "{}/instances/{}".format(config_data.DATA_DIR, self.name)
113 self.seed_path = "{}/{}-seed.img".format(self.path, self.name) 113​ self.seed_path = "{}/{}-seed.img".format(self.path, self.name)
114 self.meta_path = "{}/meta".format(self.path) 114​ self.meta_path = "{}/meta".format(self.path)
115 self.local_disk = "{}/{}-local.qcow2".format(self.path, self.name) 115​ self.local_disk = "{}/{}-local.qcow2".format(self.path, self.name)
116 116​
117​ if pubkey:
tflinkUnsubmitted
Not Done

now that I'm looking at this again, I don't like having bits that can throw exceptions in constructors

please move the file opening bits outside the constructor or wrap it in try/except and log the error.

kparalUnsubmitted
Not Done

I don't know the code much, but this comment caught my attention, so I searched a bit for Python guidelines about exceptions in constructors. I've found this:
http://stackoverflow.com/questions/1507082/python-is-it-bad-form-to-raise-exceptions-within-init

After reading it, it seems obviously OK to raise stuff like ValueError for invalid parameter values. Reading a file and potentially raising an IOError (or maybe even something else, not sure what can all happen during .read()), is something a bit different, but for very short files (like pubkeys), I find it quite similar to just an invalid parameter value. So I wouldn't mind. But the exception should be definitely documented in a docstring, so people can guard against it. Or, if it can potentially raise multiple errors, or we want to make it easy for people to handle testcloud errors, it could be wrapped in a try-except (even blank except) block and raised as a custom testcloud error. What I wouldn't do is just to log the error and fall back to pubkey=None. The user explicitly requested a particular pubkey to work, it should fail if we can't make it happen.

The other possible approach is to move the file reading code to the actual method that uses it (I guess that's _create_user_data). It will not raise during constructor init, which I find helpful (faster class initialization, no potentially intensive code run in init) and unhelpful (it's better to fail early than late, it wastes user's time and it's much harder to tear down stuff later on).

So, that was just me pondering about this.

118​ with open(pubkey) as f:
119​ self.pubkey = f.read()
120​ else:
121​ self.pubkey = ''
122​ self.user = user or config_data.USERNAME
123​ self.password = password or config_data.PASSWORD
124​
117 self.ram = 512 125​ self.ram = 512
118 self.vnc = False 126​ self.vnc = False
119 self.graphics = False 127​ self.graphics = False
120 self.atomic = False 128​ self.atomic = False
121 self.seed = None 129​ self.seed = None
122 self.kernel = None 130​ self.kernel = None
123 self.initrd = None 131​ self.initrd = None
124 132​
125 # get rid of 133​ # get rid of
126 self.backing_store = image.local_path if image else None 134​ self.backing_store = image.local_path if image else None
127 self.image_path = config_data.CACHE_DIR + self.name + ".qcow2" 135​ self.image_path = config_data.CACHE_DIR + self.name + ".qcow2"
128 136​
129 def prepare(self): 137​ def prepare(self):
130 """Create local directories and metadata needed to spawn the instance 138​ """Create local directories and metadata needed to spawn the instance
131 """ 139​ """
132 # create the dirs needed for this instance 140​ # create the dirs needed for this instance
133 self._create_dirs() 141​ self._create_dirs()
134 142​
135 # generate metadata 143​ # generate metadata
136 self._create_user_data(config_data.PASSWORD) 144​ self._create_user_data()
137 self._create_meta_data(config_data.HOSTNAME) 145​ self._create_meta_data(config_data.HOSTNAME)
138 146​
139 # generate seed image 147​ # generate seed image
140 self._generate_seed_image() 148​ self._generate_seed_image()
141 149​
142 # deal with backing store 150​ # deal with backing store
143 self._create_local_disk() 151​ self._create_local_disk()
144 152​
145 def _create_dirs(self): 153​ def _create_dirs(self):
146 if not os.path.isdir(self.path): 154​ if not os.path.isdir(self.path):
147 155​
148 log.debug("Creating instance directories") 156​ log.debug("Creating instance directories")
149 os.makedirs(self.path) 157​ os.makedirs(self.path)
150 os.makedirs(self.meta_path) 158​ os.makedirs(self.meta_path)
151 159​
152 def _create_user_data(self, password, overwrite=False, atomic=False): 160​ def _create_user_data(self, overwrite=False, atomic=False):
153 """Save the right password to the 'user-data' file needed to 161​ """Save the right password to the 'user-data' file needed to
154 emulate cloud-init. Default username on cloud images is "fedora" 162​ emulate cloud-init. Default username on cloud images is "fedora"
155 163​
156 Will not overwrite an existing user-data file unless 164​ Will not overwrite an existing user-data file unless
157 the overwrite kwarg is set to True.""" 165​ the overwrite kwarg is set to True."""
158 166​
159 if atomic: 167​ if atomic:
160 file_data = config_data.ATOMIC_USER_DATA % password 168​ file_data = config_data.ATOMIC_USER_DATA % self.password
161 169​
162 else: 170​ else:
163 file_data = config_data.USER_DATA % password 171​ file_data = config_data.USER_DATA % (self.user, self.password, self.pubkey)
164 172​
165 data_path = '{}/meta/user-data'.format(self.path) 173​ data_path = '{}/meta/user-data'.format(self.path)
166 174​
167 if (os.path.isfile(data_path) and overwrite) or not os.path.isfile(data_path): 175​ if (os.path.isfile(data_path) and overwrite) or not os.path.isfile(data_path):
168 with open(data_path, 'w') as user_file: 176​ with open(data_path, 'w') as user_file:
169 user_file.write(file_data) 177​ user_file.write(file_data)
170 log.debug("Generated user-data for instance {}".format(self.name)) 178​ log.debug("Generated user-data for instance {}".format(self.name))
171 else: 179​ else:
▲ Show 20 Lines • Show All 211 Lines • Show Last 20 Lines