Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add crash reporter service #1005

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

paulrouget
Copy link
Contributor

Flask: https://github.com/paulrouget/crash-reporter - this will need a review, and be migrated under github.com/servo. Tested and appears to work as expected.

Saltfs configuration is blindly copy/pasted from the intermittent-tracker code.

@jdm can you give a quick a look at the flask code? And do we have any mechanism in place to prevent abuse of the different build.servo.org services?

@jdm
Copy link
Member

jdm commented Aug 5, 2020

In general our build.servo.org services rely on secrets that need to be provided as part of the submission process (eg: https://github.com/servo/standups/blob/415c2fe2a399575740cd9ce585eeff0b11fafec1/standups/flask_server.py#L90). The crash reporter would need to embed the secret in the binary, which is unavoidable, but it's Good Enough for casual attempts to abuse the server.

We would be able to store the secret in taskcluster rather than the repository, so it would only be added to nightly builds, and we could add a simple #define override mechanism to make it easy to add to local builds when necessary.

- upgrade: True
- require:
- virtualenv: crash-reporter
{% if grains.get('virtual_subtype', '') != 'Docker' %}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This conditional (and the corresponding endif) can be removed like in #1006.


/home/servo/crash-reporter/config.json:
file.managed:
- source: salt://{{ tpldir }}/files/config.json
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For the shared secret, we will want to add:

- context:
    secret: {{ pillars['crash-reporter']['secret'] }}

We will also need to add /srv/pillar/crash-reporter.sls on the main machine, and add the following fake crash-reporter.sls to .travis/test_pillars/:

'crash-reporter':
  'secret': 'TEST-CRASH-REPORTER-SECRET'

@@ -0,0 +1 @@
{"port": 5004, "crash_dir": "./crashes/"}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This will need a "secret": "{{ pillar['crash-reporter']['secret'] }}" entry as well.

Comment on lines 44 to 46
/lib/systemd/system/tracker.service:
file.managed:
- source: salt://{{ tpldir }}/files/tracker.service
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We'll need a different name for this file or it will overwrite the existing tracker.service. Let's call it crash-reporter.service instead.

- pip: crash-reporter
- watch:
- file: /home/servo/crash-reporter/config.json
- file: /lib/systemd/system/tracker.service
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This will need to change if we change the .service file name.

@paulrouget paulrouget force-pushed the crashreporter branch 2 times, most recently from 9bf1981 to d6bc531 Compare August 11, 2020 10:34
@paulrouget
Copy link
Contributor Author

paulrouget commented Aug 11, 2020

Note to self: this hasn't been addressed yet.

@bors-servo
Copy link
Contributor

☔ The latest upstream changes (presumably #1012) made this pull request unmergeable. Please resolve the merge conflicts.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants