merge new ansible playbooks for autoland in ops aws to vct repo

RESOLVED FIXED

Status

MozReview
Autoland
RESOLVED FIXED
2 years ago
2 years ago

People

(Reporter: dividehex, Assigned: dividehex)

Tracking

(Blocks: 1 bug)

Details

MozReview Requests

Submitter Diff Changes Open Issues Last Updated
Loading...
Error loading review requests:

Attachments

(1 attachment, 8 obsolete attachments)

(Assignee)

Description

2 years ago
Opening a new bug since the other ones are confidential.

I'd like to merge the new ansible playbooks into the vct repos to keep things consistent with where the current playbooks are stored.
Comment hidden (mozreview-request)
(Assignee)

Updated

2 years ago
Assignee: nobody → jwatkins

Comment 2

2 years ago
mozreview-review
Comment on attachment 8782941 [details]
bug 1296641 - import new ansible playbooks and roles for autoland;

https://reviewboard.mozilla.org/r/72936/#review70728

This is a great improvement from the current wild west!

Is there any way to integrate this into the existing ansible/ directory? If we do that, it is easier to work this provisioning into our Docker testing environment. That means our test environment will test basically the same config we're running in production. This tends to do a good job of finding deployment bugs.

::: autoland-ansible/roles/apache/files/httpd.conf:353
(Diff revision 1)
> +EnableSendfile on
> +
> +# Supplemental configuration
> +#
> +# Load config files in the "/etc/httpd/conf.d" directory, if any.
> +IncludeOptional conf.d/*.conf

I'm not a fan of wildcard includes because the distro can install new files with new package updates.

I like when httpd.conf is self-contained and can't be "contaminated" by processes out of our control.

::: autoland-ansible/roles/autoland/files/peep.py:1
(Diff revision 1)
> +#!/usr/bin/env python

peep's functionality has been rolled into pip and peep is therefore deprecated.

Unfortunately, the pip shipping with CentOS 7 doesn't yet have this functionality.

We have a standalone ansible task in `ansible/tasks/virtualenv.yml` that creates a virtualenv using a modern version of pip with built-in peep support.

I would prefer we use pip instead of needlessly vendoring peep.

::: autoland-ansible/roles/autoland/tasks/main.yml:23
(Diff revision 1)
> +  hg: repo={{ vct_repo }}
> +      dest=/home/autoland/version-control-tools
> +      revision={{ rev }}

An issue with ansible's `hg` module is it doesn't clean/purge the working directory properly. We've hacked around this in various places in ansible/.

::: autoland-ansible/roles/autoland/tasks/main.yml:28
(Diff revision 1)
> +  hg: repo={{ vct_repo }}
> +      dest=/home/autoland/version-control-tools
> +      revision={{ rev }}
> +
> +- name: install autoland dependencies with peep
> +  command: /home/autoland/peep install -r /home/autoland/version-control-tools/autoland/requirements.txt

This will (likely) install Python packages globally in the system install. It is a best practice to run Python programs out of virtualenvs, which are self-contained and isolated from other "random" packages present in the system Python installation. This is the most reproducible and deterministic way to run Python.

::: autoland-ansible/roles/autoland/tasks/main.yml:69
(Diff revision 1)
> +  notify:
> +    - restart apache
> +
> +- name: install config.json
> +  template: src=config.json.j2
> +            dest=/home/autoland/version-control-tools/autoland/autoland/config.json

Not sure if this is scope bloat, but can we not run directly out of a source checkout?

::: autoland-ansible/roles/autoland/tasks/packages.yml:6
(Diff revision 1)
> +---
> +- name: install packages required by autoland
> +  yum: name={{ item }} state=present update_cache=yes
> +  with_items:
> +    - gcc
> +    - mercurial

We have our own Mercurial RPMs we run since the distros run a bit behind. This is relevant because we have a custom Mercurial extension and we try to track upstream Mercurial releases pretty closely so we don't have to support ancient versions.
(Assignee)

Comment 3

2 years ago
mozreview-review-reply
Comment on attachment 8782941 [details]
bug 1296641 - import new ansible playbooks and roles for autoland;

https://reviewboard.mozilla.org/r/72936/#review70728

Absolutely,  I was unsure on whether intergrating it would muck up the waters of the existing ansible/ directory.  But since it would make the testing environment work easier, I'll intergrate it.

> I'm not a fan of wildcard includes because the distro can install new files with new package updates.
> 
> I like when httpd.conf is self-contained and can't be "contaminated" by processes out of our control.

Very good point; I agree.  httpd.conf needs to be cleaned up anyway, I might as well roll it all up into a single config (or an explicit include)

> peep's functionality has been rolled into pip and peep is therefore deprecated.
> 
> Unfortunately, the pip shipping with CentOS 7 doesn't yet have this functionality.
> 
> We have a standalone ansible task in `ansible/tasks/virtualenv.yml` that creates a virtualenv using a modern version of pip with built-in peep support.
> 
> I would prefer we use pip instead of needlessly vendoring peep.

gtk.  I'll make use of this.

> An issue with ansible's `hg` module is it doesn't clean/purge the working directory properly. We've hacked around this in various places in ansible/.

Good catch. Could you point me to where you hacked around this?

> This will (likely) install Python packages globally in the system install. It is a best practice to run Python programs out of virtualenvs, which are self-contained and isolated from other "random" packages present in the system Python installation. This is the most reproducible and deterministic way to run Python.

Agreed.

> Not sure if this is scope bloat, but can we not run directly out of a source checkout?

This file contains lots of secrets so it can't be a public repo.  We could source this from a private repo with secrets encrypted with ansible-vault but that would require a shared vault password and would put a bump in the deployment process for devs.

I would prefer to seperate the process of doing an application deployment and having to decrypt secrests.  In this current design, secrets are (will be) sourced (encrypted) from a private repo and seeded (decrypted) onto the instance for the application deployment playbook to be picked up.  This should make application deployment/updates easier.

This isn't optimal but it is a compromise.  I'm all ears if you have a better suggestion (even if it is not done now due to being out of scope).

> We have our own Mercurial RPMs we run since the distros run a bit behind. This is relevant because we have a custom Mercurial extension and we try to track upstream Mercurial releases pretty closely so we don't have to support ancient versions.

I'm all for updated packages AND keeping things consistent.  I'll make sure to pull in the updated Mercurial RPMs

Comment 4

2 years ago
mozreview-review
Comment on attachment 8782941 [details]
bug 1296641 - import new ansible playbooks and roles for autoland;

https://reviewboard.mozilla.org/r/72936/#review72408

::: autoland-ansible/autoland-seed-secrets.yml:2
(Diff revision 1)
> +---
> +- name: seed server with secerts

s/secerts/secrets/

::: autoland-ansible/roles/autoland/tasks/main.yml:13
(Diff revision 1)
> +  user: name=autoland
> +        shell=/bin/bash
> +        system=yes
> +        state=present
> +
> +- name: copy peep.py

as gps mentioned, use a recent version of pip instead

::: autoland-ansible/roles/autoland/tasks/main.yml:52
(Diff revision 1)
> +        src=/opt/secrets/autoland/ssl/{{ inventory_hostname }}.crt
> +        remote_src=yes
> +  notify:
> +    - restart apache
> +
> +- name: copy tls key

would it be easier to let AWS handle this on the ELB than managing the secret ourselves?

::: autoland-ansible/roles/autoland/tasks/ssh.yml:8
(Diff revision 1)
> +- name: setup ssh dir
> +  file: path=/home/autoland/.ssh
> +        state=directory
> +        owner=autoland
> +        group=autoland
> +        mode=0755

Is this world-readable for a reason? ssh will often be unhappy doing certain things if it's 755, instead of 700.

::: autoland-ansible/roles/common/tasks/custom_packages.yml:1
(Diff revision 1)
> +---

roles/common/defaults/main.yml has an empty list for custom_packages; is this needed?

::: autoland-ansible/roles/nrpe/files/nrpe.cfg:1
(Diff revision 1)
> +#############################################################################

much like httpd.conf, I think I'd like to see this pared down to what's actually required rather than a lot of comments to sift through

Comment 5

2 years ago
mozreview-review-reply
Comment on attachment 8782941 [details]
bug 1296641 - import new ansible playbooks and roles for autoland;

https://reviewboard.mozilla.org/r/72936/#review70728

> Good catch. Could you point me to where you hacked around this?

https://hg.mozilla.org/hgcustom/version-control-tools/file/70aeab089f3c/ansible/roles/hg-web/tasks/main.yml#l172 is what we do in various places.

Comment 6

2 years ago
mozreview-review-reply
Comment on attachment 8782941 [details]
bug 1296641 - import new ansible playbooks and roles for autoland;

https://reviewboard.mozilla.org/r/72936/#review70728

> This file contains lots of secrets so it can't be a public repo.  We could source this from a private repo with secrets encrypted with ansible-vault but that would require a shared vault password and would put a bump in the deployment process for devs.
> 
> I would prefer to seperate the process of doing an application deployment and having to decrypt secrests.  In this current design, secrets are (will be) sourced (encrypted) from a private repo and seeded (decrypted) onto the instance for the application deployment playbook to be picked up.  This should make application deployment/updates easier.
> 
> This isn't optimal but it is a compromise.  I'm all ears if you have a better suggestion (even if it is not done now due to being out of scope).

My comment was more along the lines of "why are we copying untracked files into a repository checkout directory." Copying things to a separate directory and running from there is OK. I just don't think adding random files to the source checkout is a great idea. Then again, the `hg purge all` will delete untracked files, so I suppose this is tolerable.
(Assignee)

Comment 7

2 years ago
mozreview-review-reply
Comment on attachment 8782941 [details]
bug 1296641 - import new ansible playbooks and roles for autoland;

https://reviewboard.mozilla.org/r/72936/#review72408

> s/secerts/secrets/

typing is hard

> as gps mentioned, use a recent version of pip instead

agreed

> would it be easier to let AWS handle this on the ELB than managing the secret ourselves?

Ultimately, yes but for right now let's just finish the migration.  I'd like to not even use apache at all for this.  I think gunicorn or even nginx might be more light weight for this application but since I'm way off schedule, I'd prefer to just keep it as it is.

> Is this world-readable for a reason? ssh will often be unhappy doing certain things if it's 755, instead of 700.

No, it should be 700. gc

> roles/common/defaults/main.yml has an empty list for custom_packages; is this needed?

Yes, it is meant to be overridden.  If it goes undefined, ansible will scream.

> much like httpd.conf, I think I'd like to see this pared down to what's actually required rather than a lot of comments to sift through

Totally agree

Comment 8

2 years ago
mozreview-review-reply
Comment on attachment 8782941 [details]
bug 1296641 - import new ansible playbooks and roles for autoland;

https://reviewboard.mozilla.org/r/72936/#review70728

> Very good point; I agree.  httpd.conf needs to be cleaned up anyway, I might as well roll it all up into a single config (or an explicit include)

Concur. v-c-t/ansible/roles/hg-web/templates/httpd.conf.j2 might be a good role model; I hate the enourmous bloat that comes from all of the "documentation" inside httpd.conf.

> https://hg.mozilla.org/hgcustom/version-control-tools/file/70aeab089f3c/ansible/roles/hg-web/tasks/main.yml#l172 is what we do in various places.

ansible/roles/hg-reviewboard/tasks/main.yml
ansible/roles/hg-web/tasks/main.yml
ansible/tasks/install-mozreview.yml

Look for 'purge=yes'
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)

Comment 17

2 years ago
mozreview-review
Comment on attachment 8791712 [details]
bug 1296641 - move new autoland ansible files to ansible/;

https://reviewboard.mozilla.org/r/79024/#review77670

Can you roll this into the previous commit? If you update to the most recent commit in the series, run `hg histedit`. Then for the line corresponding for this commit, replace "pick" with "roll". Save and exit. Mercurial will collapse this commit into the one before it automatically. Make sure you are running Mercurial 3.8 or newer, as `hg histedit` requires an argument on older versions.
Attachment #8791712 - Flags: review?(gps)
Comment hidden (mozreview-request)
(Assignee)

Updated

2 years ago
Attachment #8791712 - Attachment is obsolete: true
(Assignee)

Updated

2 years ago
Attachment #8791713 - Attachment is obsolete: true
Attachment #8791713 - Flags: review?(gps)
(Assignee)

Updated

2 years ago
Attachment #8791714 - Attachment is obsolete: true
Attachment #8791714 - Flags: review?(gps)
(Assignee)

Updated

2 years ago
Attachment #8791715 - Attachment is obsolete: true
Attachment #8791715 - Flags: review?(gps)
(Assignee)

Updated

2 years ago
Attachment #8791716 - Attachment is obsolete: true
Attachment #8791716 - Flags: review?(gps)
(Assignee)

Updated

2 years ago
Attachment #8791717 - Attachment is obsolete: true
Attachment #8791717 - Flags: review?(gps)
(Assignee)

Updated

2 years ago
Attachment #8791718 - Attachment is obsolete: true
Attachment #8791718 - Flags: review?(gps)
(Assignee)

Updated

2 years ago
Attachment #8791719 - Attachment is obsolete: true
Attachment #8791719 - Flags: review?(gps)

Comment 19

2 years ago
mozreview-review
Comment on attachment 8782941 [details]
bug 1296641 - import new ansible playbooks and roles for autoland;

https://reviewboard.mozilla.org/r/72936/#review77688

Publishing what I have before I take off for my long weekend...

::: .hgignore:18
(Diff revision 2)
>  ~$
>  \.vagrant/*
>  \.dockerstate$
>  \.vctnode$
>  \.testtimes$
> +ansible/secrets/

Something to watch out for is there are a number of places where we effectively (or literally) rsync the v-c-t checkout to places. If secrets are stored in the repo, we could find ourselves accidentally rsyncing them.

I want to say we never rsync from the local machine to a remote machine. But I'd have to audit the code. It might be safer to require end-users to set the path to the secrets in some other directory.

::: ansible/roles/apache/files/httpd.conf:3
(Diff revision 2)
> +ServerRoot "/etc/httpd"
> +
> +Include conf.modules.d/*.conf

I'd just inline the content. I don't trust the distro packaging to not sneak updates in.

::: ansible/roles/apache/files/httpd.conf:34
(Diff revision 2)
> +    AddType text/html .shtml
> +    AddOutputFilter INCLUDES .shtml

shtml is so 90s. Kill it.

::: ansible/roles/apache/files/httpd.conf:47
(Diff revision 2)
> +    MIMEMagicFile conf/magic
> +</IfModule>
> +
> +EnableSendfile on
> +
> +IncludeOptional conf.d/autoland.conf

Is this role specific to autoland? Should we name it autoland_web or something instead?

::: ansible/roles/autoland/files/requirements.txt:10
(Diff revision 2)
> +mercurial==3.6.2 \
> +    --hash=sha256:09c567049c3e30f791db0cf5937346c7ff3568deadf4eb1d4e2f7c80001cb3d6

I hope we're not running 3.6.2 in production because everything before 3.7.3 has a CVE that can lead to remote code execution. Fortunately, the autoland repo should only pull from reviewboard-hg.mozilla.org and that server will reject data that could trigger the vuln.

The CI should be verifying Mercurial 3.9.1 (what we're running elsewhere) works. So let's switch to that.

::: ansible/roles/autoland/tasks/packages.yml:21
(Diff revision 2)
> +    - { path: mercurial-3.7.3-1.x86_64.rpm, sha256: 7cdd06e8fb5266fe9bd726c79db6040b68053a601daecb2418820c1d3e4f56a2 }
> +
> +- name: install Mozilla rpms
> +  command: yum localinstall -y /var/tmp/mercurial-3.7.3-1.x86_64.rpm

This should be Mercurial 3.9.1, like the rest of v-c-t.
Attachment #8782941 - Flags: review?(gps)
(Assignee)

Comment 20

2 years ago
mozreview-review-reply
Comment on attachment 8782941 [details]
bug 1296641 - import new ansible playbooks and roles for autoland;

https://reviewboard.mozilla.org/r/72936/#review77688

> Something to watch out for is there are a number of places where we effectively (or literally) rsync the v-c-t checkout to places. If secrets are stored in the repo, we could find ourselves accidentally rsyncing them.
> 
> I want to say we never rsync from the local machine to a remote machine. But I'd have to audit the code. It might be safer to require end-users to set the path to the secrets in some other directory.

Yes, I'll move this dir out of the repo.  It really doesn't belong here.

> I'd just inline the content. I don't trust the distro packaging to not sneak updates in.

Agreed and all onboard for bringing this inline.  We can probably trim down default modules anyway.  Looking at 00-base, it sure looks bloated with loaded (probably unused) modules.

> shtml is so 90s. Kill it.

It alluded my line deletes in vim. Consider it dead.

> Is this role specific to autoland? Should we name it autoland_web or something instead?

Thinking about it now, the entire apache role should be merged into the autoland role.  When I first started, I was thinking the apache role would be far more generic and resuable but as is, it is in no way shape or form in the state now.  Would that be acceptable as opposed to just renaming it?

> I hope we're not running 3.6.2 in production because everything before 3.7.3 has a CVE that can lead to remote code execution. Fortunately, the autoland repo should only pull from reviewboard-hg.mozilla.org and that server will reject data that could trigger the vuln.
> 
> The CI should be verifying Mercurial 3.9.1 (what we're running elsewhere) works. So let's switch to that.

Will do.

> This should be Mercurial 3.9.1, like the rest of v-c-t.

Agreed.
Comment hidden (mozreview-request)
(Assignee)

Comment 22

2 years ago
mozreview-review
Comment on attachment 8782941 [details]
bug 1296641 - import new ansible playbooks and roles for autoland;

https://reviewboard.mozilla.org/r/72936/#review78810

::: ansible/roles/seed_secrets/tasks/main.yml:10
(Diff revisions 2 - 3)
>          owner=root
>          group=root
>          mode=0700
>  
>  - name: seed secrets dir
> -  synchronize: src=secrets/{{ inventory_hostname }}/{{ item }}
> +  copy: src={{ secrets_repo_path }}/ansible-vault/{{ inventory_hostname }}/{{ item }}

The synchronize module doesn't detect and automatically decrypt ansible vault secrets. Hence then change to a recursive copy.

Comment 23

2 years ago
mozreview-review
Comment on attachment 8782941 [details]
bug 1296641 - import new ansible playbooks and roles for autoland;

https://reviewboard.mozilla.org/r/72936/#review79786

This is looking much, much better. I still left a lot of comments. But the bark is worse than the bite.

::: ansible/host_vars/autoland-dev.allizom.org.yml:2
(Diff revision 3)
> +---
> +vct_repo: "https://hg.mozilla.org/users/jwatkins_mozilla.com/version-control-tools/"

Is this what you intended?

::: ansible/host_vars/autoland-dev.allizom.org.yml:6
(Diff revision 3)
> +    gecko: "/repos/mozreview-gecko"
> +    mozilla-central: "/repos/mozreview-gecko"
> +    mozilla-central-review: "/repos/mozreview-gecko"
> +    mozreview: "/repos/mozreview-gecko"

I'm not sure why we have 3 of the same repo and 4 values all pointing to the same thing. This is probably an artifact of an ancient era.

::: ansible/roles/autoland/files/httpd.conf:3
(Diff revision 3)
> +LoadModule access_compat_module modules/mod_access_compat.so
> +LoadModule actions_module modules/mod_actions.so
> +LoadModule alias_module modules/mod_alias.so
> +LoadModule allowmethods_module modules/mod_allowmethods.so
> +LoadModule auth_basic_module modules/mod_auth_basic.so
> +LoadModule auth_digest_module modules/mod_auth_digest.so
> +LoadModule authn_anon_module modules/mod_authn_anon.so
> +LoadModule authn_core_module modules/mod_authn_core.so
> +LoadModule authn_dbd_module modules/mod_authn_dbd.so
> +LoadModule authn_dbm_module modules/mod_authn_dbm.so
> +LoadModule authn_file_module modules/mod_authn_file.so
> +LoadModule authn_socache_module modules/mod_authn_socache.so
> +LoadModule authz_core_module modules/mod_authz_core.so
> +LoadModule authz_dbd_module modules/mod_authz_dbd.so
> +LoadModule authz_dbm_module modules/mod_authz_dbm.so
> +LoadModule authz_groupfile_module modules/mod_authz_groupfile.so
> +LoadModule authz_host_module modules/mod_authz_host.so
> +LoadModule authz_owner_module modules/mod_authz_owner.so
> +LoadModule authz_user_module modules/mod_authz_user.so
> +LoadModule autoindex_module modules/mod_autoindex.so
> +LoadModule cache_module modules/mod_cache.so
> +LoadModule cache_disk_module modules/mod_cache_disk.so
> +LoadModule data_module modules/mod_data.so
> +LoadModule dbd_module modules/mod_dbd.so
> +LoadModule deflate_module modules/mod_deflate.so
> +LoadModule dir_module modules/mod_dir.so
> +LoadModule dumpio_module modules/mod_dumpio.so
> +LoadModule echo_module modules/mod_echo.so
> +LoadModule env_module modules/mod_env.so
> +LoadModule expires_module modules/mod_expires.so
> +LoadModule ext_filter_module modules/mod_ext_filter.so

We could aggressively prune modules if we want. (In the name of reducing attack surface area, of course.) File a follow-up bug?

::: ansible/roles/autoland/files/httpd.conf:67
(Diff revision 3)
> +<IfModule mpm_worker_module>
> +   LoadModule cgid_module modules/mod_cgid.so
> +</IfModule>
> +<IfModule mpm_event_module>
> +   LoadModule cgid_module modules/mod_cgid.so
> +</IfModule>

These can be removed since the config is hard-coded to load mpm_prefork_module.

::: ansible/roles/autoland/files/httpd.conf:73
(Diff revision 3)
> +<IfModule mpm_prefork_module>
> +   LoadModule cgi_module modules/mod_cgi.so
> +</IfModule>

And we can remove this block entirely because we shouldn't need CGI support on this server.

::: ansible/roles/autoland/files/httpd.conf:93
(Diff revision 3)
> +<IfModule log_config_module>
> +    LogFormat "%h %l %u %t \"%r\" %>s %b \"%{Referer}i\" \"%{User-Agent}i\"" combined
> +    LogFormat "%h %l %u %t \"%r\" %>s %b" common
> +
> +    <IfModule logio_module>
> +      LogFormat "%h %l %u %t \"%r\" %>s %b \"%{Referer}i\" \"%{User-Agent}i\" %I %O" combinedio
> +    </IfModule>
> +    CustomLog "logs/access_log" combined
> +</IfModule>

You can remove the <IfModule>'s because we know the modules are loaded.

I'm a big fan of httpd config files not having <IfModule> because nothing in the config should be optional and I believe configs should fail fast when things are broken. The existence of <IfModule> means you could remove a module and a config behind <IfModule> that you are relying on wouldn't load and things may subtly break. Could you imagine what would happen if you had server auth behind an <IfModule> and stopped loading the module?

::: ansible/roles/autoland/files/httpd.conf:104
(Diff revision 3)
> +<IfModule mime_module>
> +    TypesConfig /etc/mime.types
> +    AddType application/x-compress .Z
> +    AddType application/x-gzip .gz .tgz
> +</IfModule>

Drop the <IfModule>

::: ansible/roles/autoland/files/httpd.conf:113
(Diff revision 3)
> +<IfModule mime_magic_module>
> +    MIMEMagicFile conf/magic
> +</IfModule>

Drop the <IfModule>

::: ansible/roles/autoland/files/httpd.conf:119
(Diff revision 3)
> +    MIMEMagicFile conf/magic
> +</IfModule>
> +
> +EnableSendfile on
> +
> +IncludeOptional conf.d/autoland.conf

autoconf.conf should not be optional. Use Include.

::: ansible/roles/autoland/tasks/main.yml:73
(Diff revision 3)
> +           state=started
> +           enabled=yes
> +
> +- name: setup main conf file
> +  copy: src=httpd.conf
> +        dest=/etc/httpd/conf/

I know the behavior is the same, but please specify the full path for dest.

::: ansible/roles/autoland/tasks/main.yml:77
(Diff revision 3)
> +- name: find all non-managed conf files
> +  find: path=/etc/httpd/conf.d/
> +        patterns="*.conf"
> +  register: conf_files
> +
> +- name: remove apache default sites
> +  file: path="{{ item.path }}"
> +        state=absent
> +  with_items: "{{ conf_files.files }}"
> +  when: item.path != "/etc/httpd/conf.d/autoland.conf"
> +  notify:
> +    - restart apache

I'd just nuke /etc/httpd/conf.d and put autoland.conf in /etc/httpd/conf/autoland.conf.

::: ansible/roles/autoland/tasks/ssh.yml:24
(Diff revision 3)
> +        dest=/home/autoland/.ssh/
> +        owner=autoland
> +        group=autoland
> +        mode=0600
> +        remote_src=yes
> +        

Nit: trailing whitespace

::: ansible/roles/autoland/tasks/ssh.yml:33
(Diff revision 3)
> +        dest=/home/autoland/.ssh/
> +        owner=autoland
> +        group=autoland
> +        mode=0644
> +        remote_src=yes
> +        

Nit: trailing whitespace

::: ansible/roles/common/tasks/custom_packages.yml:7
(Diff revision 3)
> +    - name: Download custom packages from s3
> +      command: aws s3 cp s3://moz-mozreview/repos/yum/centos7/x86_64/{{ item }} /tmp/custom_packages/
> +      with_items: "{{ custom_packages }}"

Nice playbook!

Could we have this accept the hash of the downloaded file? I'm a big stickler for pinning things as a mitigation for MitM attacks and other unwanted changes. Sometimes pinning hashes makes you realize that things change when they shouldn't.

Also, is there any advantage to using `aws s3 cp` instead of get_url (like we do for Mercurial?

::: ansible/roles/common/tasks/yum_epel_update.yml:7
(Diff revision 3)
> +
> +- name: Install epel-release
> +  yum: name=epel-release state=latest
> +
> +- name: Update all packages
> +  yum: name=* state=latest

Is this ansible magic for `yum update`?
Attachment #8782941 - Flags: review-
(Assignee)

Comment 24

2 years ago
mozreview-review-reply
Comment on attachment 8782941 [details]
bug 1296641 - import new ansible playbooks and roles for autoland;

https://reviewboard.mozilla.org/r/72936/#review79786

> Is this what you intended?

No.  Used for debugging. Good catch

> I'm not sure why we have 3 of the same repo and 4 values all pointing to the same thing. This is probably an artifact of an ancient era.

This was copied over from the current production server.  I'll touch base with the mozreview devs to see what values these should be.

> We could aggressively prune modules if we want. (In the name of reducing attack surface area, of course.) File a follow-up bug?

Follow up bug filed under 1306741.

> These can be removed since the config is hard-coded to load mpm_prefork_module.

Agreed.

> And we can remove this block entirely because we shouldn't need CGI support on this server.

Agreed.

> You can remove the <IfModule>'s because we know the modules are loaded.
> 
> I'm a big fan of httpd config files not having <IfModule> because nothing in the config should be optional and I believe configs should fail fast when things are broken. The existence of <IfModule> means you could remove a module and a config behind <IfModule> that you are relying on wouldn't load and things may subtly break. Could you imagine what would happen if you had server auth behind an <IfModule> and stopped loading the module?

Very good point!

> Drop the <IfModule>

Agreed.

> Drop the <IfModule>

Agreed.

> autoconf.conf should not be optional. Use Include.

Agreed.

> I know the behavior is the same, but please specify the full path for dest.

Agreed.

> I'd just nuke /etc/httpd/conf.d and put autoland.conf in /etc/httpd/conf/autoland.conf.

Agreed.

> Nit: trailing whitespace

Not sure how that slipped in. But I'll clean it up.

> Nit: trailing whitespace

Same here. It will be removed.

> Nice playbook!
> 
> Could we have this accept the hash of the downloaded file? I'm a big stickler for pinning things as a mitigation for MitM attacks and other unwanted changes. Sometimes pinning hashes makes you realize that things change when they shouldn't.
> 
> Also, is there any advantage to using `aws s3 cp` instead of get_url (like we do for Mercurial?

Thanks! Yes, we can have this accept hashes.  I don't think there is any advantage to have using aws s3 cp over get_url.  I'll change this for consistency sake.

> Is this ansible magic for `yum update`?

Yes, it is.  It's listed as an example on the yum module.
http://docs.ansible.com/ansible/yum_module.html
(Assignee)

Comment 25

2 years ago
RE: dict of repo paths in config.json

:dminor provided insight into this via email.  I'll proceed with removing the repos from the dict and properly naming them on the new autoland instances as suggested.

The relevant bit of code is here [1]. If the value is missing in the repo dict, it defaults to '/repos' + the name of the repo received from ReviewBoard. So this is only necessary if the path on the Autoland server does not correspond to the repo names received from Reviewboard.

If you're setting up something new, I'd suggest removing this dict and just making the paths on the Autoland server match what you get from ReviewBoard to avoid confusion in the future :)

[1] https://dxr.mozilla.org/hgcustom_version-control-tools/rev/2b020dbda0eb22dddfa220954dad65d5208dca16/autoland/autoland/transplant.py#13
Comment hidden (mozreview-request)

Comment 27

2 years ago
mozreview-review
Comment on attachment 8782941 [details]
bug 1296641 - import new ansible playbooks and roles for autoland;

https://reviewboard.mozilla.org/r/72936/#review82806

lgtm. did you still want an r? on gps? looks like it got dropped after the last round.
Attachment #8782941 - Flags: review?(klibby) → review+

Comment 28

2 years ago
mozreview-review
Comment on attachment 8782941 [details]
bug 1296641 - import new ansible playbooks and roles for autoland;

https://reviewboard.mozilla.org/r/72936/#review84588

Comment 29

2 years ago
mozreview-review
Comment on attachment 8782941 [details]
bug 1296641 - import new ansible playbooks and roles for autoland;

https://reviewboard.mozilla.org/r/72936/#review84596

Rubber stamping to allow autoland.
Attachment #8782941 - Flags: review+

Comment 30

2 years ago
Pushed by smacleod@mozilla.com:
https://hg.mozilla.org/hgcustom/version-control-tools/rev/24e764dea420
import new ansible playbooks and roles for autoland; r=fubar,smacleod
Status: NEW → RESOLVED
Last Resolved: 2 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.