Page MenuHomePhabricator

Rewrite /usr/local/bin/crontab in python; fix bugs
Closed, ResolvedPublic

Description

The /usr/local/bin/crontab wrapper script has a bug which presents a good opportunity to port it to python. The bug is that when the ssh connection to fetch an existing crontab from the cron host fails it doesn't abort. Instead it just acts like there is no cron file on the remote and stumbles on.

See T156168: Tool labs crontab host tools-cron-01 cannot be ssh-ed into within a service user for the config failure that caused this to be noticed

Event Timeline

Restricted Application added a subscriber: Aklapper. · View Herald Transcript

It would be nice to split out the "template" bit of it; i. e., let Puppet write @cron_host to some file in /etc and make /usr/local/bin/crontab read from that instead of changing it in the source code. That should make testing it more easy.

That seems like a good idea. We could move the actual script into the labs/toollabs deb with become, jsub, etc with that bit of config moved outside the script.

How about something similar to this:

1#! /usr/bin/python -Es
2# -*- coding: UTF-8 -*-
3#
4# Copyright © 2013 Marc-André Pelletier <mpelletier@wikimedia.org>
5# Copyright © 2017 Zhuyifei1999 <zhuyifei1999@gmail.com>
6#
7# Permission to use, copy, modify, and/or distribute this software for any
8# purpose with or without fee is hereby granted, provided that the above
9# copyright notice and this permission notice appear in all copies.
10#
11# THE SOFTWARE IS PROVIDED "AS IS" AND THE AUTHOR DISCLAIMS ALL WARRANTIES
12# WITH REGARD TO THIS SOFTWARE INCLUDING ALL IMPLIED WARRANTIES OF
13# MERCHANTABILITY AND FITNESS. IN NO EVENT SHALL THE AUTHOR BE LIABLE FOR
14# ANY SPECIAL, DIRECT, INDIRECT, OR CONSEQUENTIAL DAMAGES OR ANY DAMAGES
15# WHATSOEVER RESULTING FROM LOSS OF USE, DATA OR PROFITS, WHETHER IN AN
16# ACTION OF CONTRACT, NEGLIGENCE OR OTHER TORTIOUS ACTION, ARISING OUT OF
17# OR IN CONNECTION WITH THE USE OR PERFORMANCE OF THIS SOFTWARE.
18#
19
20# This goes in /usr/local/bin to override the system crontab (which will
21# have its permissions restricted).
22
23# THIS FILE IS MANAGED BY PUPPET
24#
25# This goes in /usr/local/bin to override the system crontab (which will
26# have its permissions restricted).
27#
28# Source: modules/toollabs/templates/crontab.erb
29# From: toollabs::submit
30
31from __future__ import print_function
32
33import argparse
34import pwd
35import os
36import re
37import subprocess
38import sys
39import tempfile
40
41# TODO: read from somewhere like /etc/toollabs-cronhost
42CRON_HOST = 'tools-cron-01.tools.eqiad.wmflabs' # FIXME: <%= @cron_host %>
43JSUB_MODIFIED = '''
44NOTE: some crontab entries have been modified to grid submissions.
45 You may want to examine the result with 'crontab -e'.
46'''
47DEFAULT_CRONTAB = '''\
48# Edit this file to introduce tasks to be run by cron.
49#
50# Each task to run has to be defined through a single line
51# indicating with different fields when the task will be run
52# and what command to run for the task
53#
54# To define the time you can provide concrete values for
55# minute (m), hour (h), day of month (dom), month (mon),
56# and day of week (dow) or use '*' in these fields (for 'any').#
57# Notice that tasks will be started based on the cron's system
58# daemon's notion of time and timezones.
59#
60# Output of the crontab jobs (including errors) is sent through
61# email to the user the crontab file belongs to (unless redirected).
62#
63# For example, you can run a backup of all your user accounts
64# at 5 a.m every week with:
65# 0 5 * * 1 tar -zcf /var/backups/home.tgz /home/
66#
67# For more information see the manual pages of crontab(5) and cron(8)
68#
69# Wikimedia Tool Labs specific note:
70# Please be aware that *only* jsub and jstart are acceptable
71# commands to schedule via cron. Any command specified here will
72# be modified to be invoked through jsub unless it is one of
73# the two.
74#
75# m h dom mon dow command
76'''
77
78parser = argparse.ArgumentParser()
79parser.add_argument('-u', help=argparse.SUPPRESS,
80 dest='user')
81parser.add_argument('-i', help='prompt before deleting crontab',
82 action='store_true')
83group = parser.add_mutually_exclusive_group()
84group.add_argument('file', help='replace crontab with file (default)',
85 default=sys.stdin, nargs='?', type=argparse.FileType('r'))
86subgroup = group.add_mutually_exclusive_group()
87subgroup.add_argument('-e', help='edit crontab',
88 action='store_const', const='e', dest='operation')
89subgroup.add_argument('-l', help='list crontab',
90 action='store_const', const='l', dest='operation')
91subgroup.add_argument('-r', help='delete crontab',
92 action='store_const', const='r', dest='operation')
93
94
95class NoCrontab(Exception):
96 pass
97
98
99class Crontab(object):
100 def __init__(self, user):
101 self.user = user
102
103 def _remote(self, args, stdin=None):
104 """Execute remote crontab command and returns stdout."""
105 args = ['/usr/bin/ssh', CRON_HOST, '/usr/bin/crontab'] + args
106 if self.user.pw_uid != os.getuid():
107 args += ['-u', self.user.pw_name]
108
109 ssh = subprocess.Popen(args,
110 stdin=subprocess.PIPE,
111 stdout=subprocess.PIPE,
112 stderr=subprocess.PIPE)
113
114 stdoutdata, stderrdata = ssh.communicate(input=stdin)
115
116 if ssh.returncode:
117 if stderrdata.lower().startswith('no crontab for '):
118 raise NoCrontab
119 else:
120 print(stderrdata, end='', file=sys.stderr)
121 err('unable to execute remote crontab command')
122 sys.exit(ssh.returncode)
123
124 return stdoutdata
125
126 @staticmethod
127 def _add_jsub(text):
128 # TODO: regex insanity here
129 return text
130
131 def load(self):
132 return self._remote(['-l'])
133
134 def save(self, text):
135 jsub_text = self._add_jsub(text)
136 if jsub_text != text:
137 print(JSUB_MODIFIED, file=sys.stderr)
138 self._remote([], stdin=jsub_text)
139
140 def remove(self):
141 self._remote(['-r'])
142
143
144def editor(text):
145 with tempfile.NamedTemporaryFile() as f:
146 f.write(text)
147 f.flush()
148
149 subprocess.check_call(['/usr/bin/sensible-editor', f.name])
150
151 f.seek(0)
152 return f.read()
153
154
155def err(message):
156 print('{}: {}'.format(sys.argv[0], message), file=sys.stderr)
157
158
159def main():
160 args = parser.parse_args()
161
162 if args.i and args.operation != 'r':
163 parser.error('argument -i: only applicable with -r')
164
165 target = pwd.getpwuid(os.getuid())
166 if args.user is not None:
167 if os.getuid():
168 parser.error('argument -u: must be privileged')
169
170 try:
171 target = pwd.getpwnam(args.user)
172 except KeyError:
173 parser.error('argument -u: unknown user "{}"'.format(args.user))
174
175 if target.pw_uid < 500:
176 # If the target user is not managed in LDAP and thus likely
177 # a system user, invoke the original crontab instead.
178 os.execv('/usr/bin/crontab', sys.argv[1:])
179 elif target.pw_uid < 40000 and os.getuid():
180 err('only tools are allowed crontabs')
181 sys.exit(1)
182 else:
183 try:
184 crontab = Crontab(target)
185
186 if args.operation is None:
187 # Replace
188 crontab.save(args.file.read())
189 elif args.operation == 'e':
190 # Edit
191 try:
192 crontab_text = crontab.load()
193 except NoCrontab:
194 crontab_text = DEFAULT_CRONTAB
195 new_crontab_text = editor(crontab_text)
196
197 if new_crontab_text == crontab_text:
198 print('No modification made', file=sys.stderr)
199 elif not new_crontab_text.strip():
200 err('cowardly refusing to install empty crontab')
201 err('use `{} -r` if you want to remove the crontab'.format(
202 sys.srgv[0]))
203 sys.exit(1)
204 else:
205 crontab.save(new_crontab_text)
206 elif args.operation == 'l':
207 # List
208 try:
209 print(crontab.load(), end='') # crontab already has lf
210 except NoCrontab:
211 print('no crontab for {}'.format(target.pw_name),
212 file=sys.stderr)
213 sys.exit(1)
214 elif args.operation == 'r':
215 # Delete
216 try:
217 crontab.load()
218 except NoCrontab:
219 print('no crontab for {}'.format(target.pw_name),
220 file=sys.stderr)
221 sys.exit(1)
222 if args.i:
223 prompt = "{}: really delete {}'s crontab? (y/n) ".format(
224 sys.argv[0], target.pw_name)
225 while True:
226 try:
227 stdin = raw_input(prompt).lower()
228 except EOFError:
229 stdin = ''
230 except KeyboardInterrupt:
231 print(file=sys.stderr)
232 raise
233
234 if stdin == 'y':
235 crontab.remove()
236 break
237 elif stdin == 'n':
238 break
239 else:
240 prompt = 'Please enter Y or N: '
241
242 else:
243 crontab.remove()
244 except KeyboardInterrupt:
245 pass
246
247if __name__ == '__main__':
248 main()

The regex insanity has not been written yet, though.

@zhuyifei1999 Looks like a good start. I think we should move the script from Puppet to https://gerrit.wikimedia.org/r/#/admin/projects/labs/toollabs and make the change in Puppet to populate /etc/toollabs-cronhost (name can be bikesheded). Do you want to take a shot at the patches for that?

Yeah sure. Part of this task anyways. :)

Change 336990 had a related patch set uploaded (by Zhuyifei1999):
toollabs: Preparing to move /usr/local/bin/crontab to labs/toollabs

https://gerrit.wikimedia.org/r/336990

Change 336998 had a related patch set uploaded (by Zhuyifei1999):
Add rewritten crontab in Python

https://gerrit.wikimedia.org/r/336998

scfc triaged this task as Medium priority.Feb 16 2017, 11:22 PM
scfc moved this task from Triage to Waiting for code review on the Toolforge board.

Change 336990 merged by Dzahn:
[operations/puppet] toollabs: Preparing to move /usr/local/bin/crontab to labs/toollabs

https://gerrit.wikimedia.org/r/336990

Change 336998 merged by jenkins-bot:
[labs/toollabs@master] Add rewritten crontab in Python

https://gerrit.wikimedia.org/r/336998

Change 382629 had a related patch set uploaded (by BryanDavis; owner: Bryan Davis):
[labs/toollabs@master] crontab: Fix python3 syntax problems

https://gerrit.wikimedia.org/r/382629

Change 382629 merged by jenkins-bot:
[labs/toollabs@master] crontab: Fix python3 syntax problems

https://gerrit.wikimedia.org/r/382629

The new script is available on Toolforge hosts as /usr/bin/oge-crontab. The final step needed is to replace the current /usr/local/bin/crontab script provisioned by Puppet with a symlink to /usr/bin/oge-crontab.

tools.yifeibot@tools-bastion-02:~$ oge-crontab -e
Traceback (most recent call last):
  File "/usr/bin/oge-crontab", line 337, in <module>
    main()
  File "/usr/bin/oge-crontab", line 283, in main
    new_crontab_text = editor(crontab_text)
  File "/usr/bin/oge-crontab", line 209, in editor
    f.write(text.encode('latin1'))
UnicodeEncodeError: 'latin-1' codec can't encode characters in position 1522-1534: ordinal not in range(256)

Umm... shouldn't the encode be utf-8 instead of latin-1? Alternatively can we open this temp file in text mode?

Umm... shouldn't the encode be utf-8 instead of latin-1? Alternatively can we open this temp file in text mode?

The latin-1 encoding should be 8-bit clean, but apparently its not? This was part of the code I messed with to port it to python3. I touched as little as possible to do that, but maybe that was the wrong approach.

Change 383770 had a related patch set uploaded (by Zhuyifei1999; owner: Zhuyifei1999):
[labs/toollabs@master] crontab: make tempfile use utf-8 encoding

https://gerrit.wikimedia.org/r/383770

Change 383770 merged by jenkins-bot:
[labs/toollabs@master] crontab: make tempfile use utf-8 encoding

https://gerrit.wikimedia.org/r/383770

Mentioned in SAL (#wikimedia-cloud) [2017-11-03T21:19:31Z] <bd808> Deployed misctools 1.26 (T156174)

tools.yifeibot@tools-bastion-02:~$ oge-crontab -e
Traceback (most recent call last):
  File "/usr/bin/oge-crontab", line 337, in <module>
    main()
  File "/usr/bin/oge-crontab", line 283, in main
    new_crontab_text = editor(crontab_text)
  File "/usr/bin/oge-crontab", line 209, in editor
    f.write(text.encode('latin1'))
UnicodeEncodeError: 'latin-1' codec can't encode characters in position 1522-1534: ordinal not in range(256)

This test case works now.

Change 391979 had a related patch set uploaded (by BryanDavis; owner: Bryan Davis):
[operations/puppet@production] toolforge: Replace /usr/local/bin/crontab with oge-crontab

https://gerrit.wikimedia.org/r/391979

Change 391979 merged by Rush:
[operations/puppet@production] toolforge: Replace /usr/local/bin/crontab with oge-crontab

https://gerrit.wikimedia.org/r/391979

Note: crons backed up on tools-cron-01 prior to merge at tools-cron-01:/var/spool/cron/crontabs# ls /root/20112017/crontabs/. Old crontab script is backed up locally on tools-bastion-03 at tools-bastion-03:~# cp /usr/local/bin/crontab /home/rush

Post merge of 391979 status for this issue:

  • Adding crons on a tool without them seems to add the correct header
  • Adding new lines to an existing crontab worked and preserved old content
  • Puppet still errrors for Prometheus crontabs but I believe the errors are different now
Notice: /Stage[main]/Prometheus::Node_puppet_agent/Cron[prometheus_puppet_agent_stats]/ensure: created

NOTE: some crontab entries have been modified to grid submissions.
      You may want to examine the result with 'crontab -e'.

Permission denied (publickey,hostbased).
/usr/local/bin/crontab: unable to execute remote crontab command
Notice: Finished catalog run in 103.34 seconds

vs debug output (roughly the same)

Notice: /Stage[main]/Prometheus::Node_puppet_agent/Cron[prometheus_puppet_agent_stats]/ensure: created
Debug: Flushing cron provider target prometheus

NOTE: some crontab entries have been modified to grid submissions.
      You may want to examine the result with 'crontab -e'.

Permission denied (publickey,hostbased).
/usr/local/bin/crontab: unable to execute remote crontab command
Debug: /Stage[main]/Prometheus::Node_puppet_agent/Cron[prometheus_puppet_agent_stats]: The container Class[Prometheus::Node_puppet_agent] will propagate my refresh event

  • We are still executing the user crontab replacement as root
root@tools-bastion-05:~# which crontab
/usr/local/bin/crontab
root@tools-bastion-05:~# echo $PATH
/usr/local/sbin:/usr/local/bin:/usr/sbin:/usr/bin:/sbin:/bin
  • The portion of of oge-crontab that is being trigged for prometheus is
stdoutdata, stderrdata = ssh.communicate(input=stdin)

if ssh.returncode:
    if stderrdata.lower().startswith('no crontab for '):
        raise NoCrontab
    else:
        print(stderrdata, end='', file=sys.stderr)
        err('unable to execute remote crontab command')
        sys.exit(ssh.returncode)

I can't find a loss of functionality since prometheus was already tripping over this special cron behavior. I believe we can easily add an exclusion now based on UID if we want? That's not the correct long term solution but will work. The issue seems to show consistently on Puppet runs now and is reproduceable.

Thanks for all of your work on this @zhuyifei1999!