Add '--adb-keys-file $HOME/.android/*.pub' to all bot provision_devices calls.

Add '--adb-keys-file $HOME/.android/*.pub' to all bot provision_devices calls.

This will add --adb-key-files $HOME/.android/*.pub argument by default to all
Android bots on recipes.

Here use of the argument has been changed to not fail if any of the adb key
files the file does not exist: https://codereview.chromium.org/975963002/

BUG=

[navabi, 2015-03-04 00:19:22]

navabi@google.com changed reviewers:
+ friedman@chromium.org, jbudorick@chromium.org, luqui@chromium.org

[navabi, 2015-03-04 00:19:23]

Please review along with: https://codereview.chromium.org/975963002/

https://codereview.chromium.org/976913002/diff/1/scripts/slave/recipe_modules...
File scripts/slave/recipe_modules/chromium_android/api.py (right):

https://codereview.chromium.org/976913002/diff/1/scripts/slave/recipe_modules...
scripts/slave/recipe_modules/chromium_android/api.py:367: if not
no_adb_keys_files:
This option is for if in the future some bots may not want to write adb keys. We
can also take this out as the provision_devices script will not fail if a path
that does not exist is specified.

[jbudorick, 2015-03-04 00:23:50]

https://codereview.chromium.org/976913002/diff/1/scripts/slave/recipe_modules...
File scripts/slave/recipe_modules/chromium_android/api.py (right):

https://codereview.chromium.org/976913002/diff/1/scripts/slave/recipe_modules...
scripts/slave/recipe_modules/chromium_android/api.py:367: if not
no_adb_keys_files:
On 2015/03/04 00:19:23, navabi wrote:
> This option is for if in the future some bots may not want to write adb keys.
We
> can also take this out as the provision_devices script will not fail if a path
> that does not exist is specified.

I don't really have a problem with having an option, but the double negative
makes it _really_ clunky. Why not just something like "write_adb_keys_files"
that defaults to True? I think "if write_adb_keys_files" reads more cleanly than
"if not no_adb_keys_files".

[navabi, 2015-03-04 00:56:38]

https://codereview.chromium.org/976913002/diff/1/scripts/slave/recipe_modules...
File scripts/slave/recipe_modules/chromium_android/api.py (right):

https://codereview.chromium.org/976913002/diff/1/scripts/slave/recipe_modules...
scripts/slave/recipe_modules/chromium_android/api.py:367: if not
no_adb_keys_files:
On 2015/03/04 00:23:50, jbudorick wrote:
> On 2015/03/04 00:19:23, navabi wrote:
> > This option is for if in the future some bots may not want to write adb
keys.
> We
> > can also take this out as the provision_devices script will not fail if a
path
> > that does not exist is specified.
> 
> I don't really have a problem with having an option, but the double negative
> makes it _really_ clunky. Why not just something like "write_adb_keys_files"
> that defaults to True? I think "if write_adb_keys_files" reads more cleanly
than
> "if not no_adb_keys_files".

I agree. Done.

[luqui, 2015-03-04 20:19:44]

https://codereview.chromium.org/976913002/diff/40001/scripts/slave/recipe_mod...
File scripts/slave/recipe_modules/chromium_android/api.py (right):

https://codereview.chromium.org/976913002/diff/40001/scripts/slave/recipe_mod...
scripts/slave/recipe_modules/chromium_android/api.py:367: '.android', '*.pub')):
If we told it to write the adb keys file, and there is no adb keys file, that
means something is wrong, no?  We should die.

Also you should not use glob directly in recipes -- it will not interact
correctly with expectations.  If you can't write this with existing recipe
module functionality, we need to make that possible.

https://codereview.chromium.org/976913002/diff/40001/scripts/slave/recipe_mod...
File
scripts/slave/recipe_modules/chromium_android/example.expected/basic_builder_basic.json
(right):

https://codereview.chromium.org/976913002/diff/40001/scripts/slave/recipe_mod...
scripts/slave/recipe_modules/chromium_android/example.expected/basic_builder_basic.json:343:
"--adb-key-files /usr/local/google/home/navabi/.android/*.pub"
This will not be expanded by the shell.  Will provision_devices handle a glob?

[jbudorick, 2015-03-04 20:22:02]

https://codereview.chromium.org/976913002/diff/40001/scripts/slave/recipe_mod...
File
scripts/slave/recipe_modules/chromium_android/example.expected/basic_builder_basic.json
(right):

https://codereview.chromium.org/976913002/diff/40001/scripts/slave/recipe_mod...
scripts/slave/recipe_modules/chromium_android/example.expected/basic_builder_basic.json:343:
"--adb-key-files /usr/local/google/home/navabi/.android/*.pub"
On 2015/03/04 20:19:44, luqui wrote:
> This will not be expanded by the shell.  Will provision_devices handle a glob?

I don't think it will.

[friedman1, 2015-03-04 20:49:33]

friedman@google.com changed reviewers:
+ friedman@google.com

[friedman1, 2015-03-04 20:49:34]

https://codereview.chromium.org/976913002/diff/40001/scripts/slave/recipe_mod...
File scripts/slave/recipe_modules/chromium_android/api.py (right):

https://codereview.chromium.org/976913002/diff/40001/scripts/slave/recipe_mod...
scripts/slave/recipe_modules/chromium_android/api.py:367: '.android', '*.pub')):
On 2015/03/04 20:19:44, luqui wrote:
> If we told it to write the adb keys file, and there is no adb keys file, that
> means something is wrong, no?  We should die.
> 
> Also you should not use glob directly in recipes -- it will not interact
> correctly with expectations.  If you can't write this with existing recipe
> module functionality, we need to make that possible.

Why not just [key for key in os.listdir(os.path.join(os.environ['HOME'],
'.android')) if key.endswith('.pub')] and get rid of the glob?

[luqui, 2015-03-04 21:53:46]

On 2015/03/04 20:49:34, friedman1 wrote:
>
https://codereview.chromium.org/976913002/diff/40001/scripts/slave/recipe_mod...
> File scripts/slave/recipe_modules/chromium_android/api.py (right):
> 
>
https://codereview.chromium.org/976913002/diff/40001/scripts/slave/recipe_mod...
> scripts/slave/recipe_modules/chromium_android/api.py:367: '.android',
'*.pub')):
> On 2015/03/04 20:19:44, luqui wrote:
> > If we told it to write the adb keys file, and there is no adb keys file,
that
> > means something is wrong, no?  We should die.
> > 
> > Also you should not use glob directly in recipes -- it will not interact
> > correctly with expectations.  If you can't write this with existing recipe
> > module functionality, we need to make that possible.
> 
> Why not just [key for key in os.listdir(os.path.join(os.environ['HOME'],
> '.android')) if key.endswith('.pub')] and get rid of the glob?

This is not sufficient for the same reason.  recipe_lint should be catching your
'import os' and yelling about it, but I guess I never implemented it for recipe
modules.

[friedman1, 2015-03-04 21:59:11]

On 2015/03/04 21:53:46, luqui wrote:
> On 2015/03/04 20:49:34, friedman1 wrote:
> >
>
https://codereview.chromium.org/976913002/diff/40001/scripts/slave/recipe_mod...
> > File scripts/slave/recipe_modules/chromium_android/api.py (right):
> > 
> >
>
https://codereview.chromium.org/976913002/diff/40001/scripts/slave/recipe_mod...
> > scripts/slave/recipe_modules/chromium_android/api.py:367: '.android',
> '*.pub')):
> > On 2015/03/04 20:19:44, luqui wrote:
> > > If we told it to write the adb keys file, and there is no adb keys file,
> that
> > > means something is wrong, no?  We should die.
> > > 
> > > Also you should not use glob directly in recipes -- it will not interact
> > > correctly with expectations.  If you can't write this with existing recipe
> > > module functionality, we need to make that possible.
> > 
> > Why not just [key for key in os.listdir(os.path.join(os.environ['HOME'],
> > '.android')) if key.endswith('.pub')] and get rid of the glob?
> 
> This is not sufficient for the same reason.  recipe_lint should be catching
your
> 'import os' and yelling about it, but I guess I never implemented it for
recipe
> modules.

The problem is that the adb binary needs to be spawned with an
ADB_VENDOR_KEYS=x:y:z variable
(ADB_VENDOR_KEYS=/home/chrome-bot/.android/chrome_infrastructure_adbkey.pub:/home/chrome-bot/.android/adb_key.pub
adb start-server).  We are trying to use 1 default key on all devices and bots
so that we don't have to hack adb out of secure mode with custom android builds
anymore.

What's the best way to do this?

Armand: We could just have puppet write the chrome_infrastructure_adbkey{,.pub}
to all bots as the default key, and everything will be fine... WDYT?

[luqui, 2015-03-04 22:02:56]

That puppet solution sounds fine to me, there might be larger forces at play
that make it a bad idea, dunno.

If you want to do it with recipes, I would suggest adding a support to the adb
module for finding the keys (as a step, maybe inline python) and adding
accessors.  Or maybe the chromium_android module if you're feeling cheeky /
depending on the scope of this expectation.  Then explicitly put it in the
environment where you need it.  We want to keep env variables tightly contained
and not use them where they aren't needed.

[navabi, 2015-03-04 22:06:02]

On 2015/03/04 22:02:56, luqui wrote:
> That puppet solution sounds fine to me, there might be larger forces at play
> that make it a bad idea, dunno.
> 
> If you want to do it with recipes, I would suggest adding a support to the adb
> module for finding the keys (as a step, maybe inline python) and adding
> accessors.  Or maybe the chromium_android module if you're feeling cheeky /
> depending on the scope of this expectation.  Then explicitly put it in the
> environment where you need it.  We want to keep env variables tightly
contained
> and not use them where they aren't needed.

Elliot, the issue that Luke is talking about is different than what you are
describing. I'm currently adding an authorize_adb_keys step. This step will be
called from all recipes and will use ADB_VENDOR_KEYS to push the host specific
public adb key to the devices. We can do that in the recipe or puppet.

[navabi, 2015-03-04 23:22:12]

> Elliot, the issue that Luke is talking about is different than what you are
> describing. I'm currently adding an authorize_adb_keys step. This step will be
> called from all recipes and will use ADB_VENDOR_KEYS to push the host specific
> public adb key to the devices. We can do that in the recipe or puppet.

Sorry, I should have been more clear.

As I understand it, the problem here is that the HOME directory (which contains
the adb keys) is done in the recipe, not in a script called by the recipe. Thus,
regardless of if we use glob or os.listdir, the problem Luke explained is the
same.

The authorize_adb_keys solution I propose, is different in that there will be a
script in build/scripts/slave/android/authorize_adb_keys.py that does all the
work. The home path will be specified in the script as opposed to the recipe,
and thus the expectations will only show a call to this script.

I'll upload that as a next patch to this, because I don't think we want to do it
this way.

[navabi, 2015-03-05 23:35:23]

Closing this issue. The better solution is implemented here:
https://codereview.chromium.org/979013002/