ircbot: add shorturl plugin #94

abizer · 2019-01-28T11:40:38Z

let the bikeshedding begin

this basically does s/macro/shorturl/ on macros.py to build a database-backed, written-in-python, lives-on-marathon, accessible-through-ircbot, avoids-username-collisions simple redirect system for shorturls. as far as I remember, those conditions address every criticism of the previous approaches (ocf/puppet#253). this is designed to be used along with a formal puppet-based shorturl rule that looks something like

redirect_rule => '^s(/.*)?* https://ircbot.ocf.berkeley.edu/shorturls$1 [R]'

e.g. Apache on death returns a 302 to ircbot which returns a 302 to the real destination.

perhaps a new staffer can work on applying #56 to this along with the other plugins

dkess · 2019-01-28T18:39:07Z

Should this be an ircbot plugin? I think it would be better to have a command line or web interface for this. Or perhaps it should be part of etc.

abizer · 2019-01-28T20:33:56Z

it's a database, so we can still easily build a command line or web interface for this should someone want to. I thought about putting it in etc but that means writing more scripts to generate an Apache config on death and this was much simpler and easier to implement.

ircbot/plugin/help.py

kpengboy

Thanks for actually taking initiative on this :)

However, I would have to agree with @dkess that ircbot is not the appropriate place to host a shorturl service. If anything, I would make this a separate service running on Kubernetes. This way:

You can run more than one instance of this service
If ircbot goes down, it doesn't take the shortlink service with it

Having a plugin to manipulate shorturls from IRC sounds fine to me though.

kpengboy · 2019-01-29T05:36:03Z

ircbot/plugin/shorturls.py

+
+
+def register(bot):
+    # [!-~] is all printable ascii except spaces


Do we want shorturls to be able to contain ?, &, or =?

...Well now this won't allow hyphens or underscores. Methinks those are still useful characters.
(Also I think you need to update the comment)

abizer · 2019-01-29T08:40:33Z

I'm not particularly a fan of the redirector living in ircbot either - certainly, in the long term, we are overloading ircbot with functionality that ought to live elsewhere. However, right now, here's the nice thing about this approach: transitioning to a dedicated shorturl redirect service is trivial. I'll write it myself when Kubernetes is ready. All we'd have to do is change the rule in puppet from redirect /s/<whatever> to ircbot.ocf.berkeley.edu/shorturls/ to shorturls.ocf.berkeley.edu/<whatever> and register the new redirect bouncer at that address (then delete a few lines of code in this repo).

Considering the length of time over which we've repeatedly bikeshed the right way to engineer this dead simple system, if we ever want to actually implement this feature, I think we need to get something up that works and meets our needs and iterate on it to improve it rather than relegating it to development hell.

We already have some services working in dev on k8s - templates, mastodon. keur even has a provisional ocfweb working on k8s. It shouldn't be long until we have prod services running on k8s, at which point I'll transition the redirector. I'm willing to live with suboptimal design for a short period of time in order to get the feature up so we can make use of it this semester rather than continuing to wait and invariably ending up with the feature once again not being implemented.

cg505

I am also unsure about if this is the right approach but I'm up for giving it a shot since it is pretty minimal.

Is there a record of the DB schema anywhere? Is that something we do?

cg505 · 2019-01-29T21:35:15Z

ircbot/plugin/shorturls.py

+            'UPDATE shorturls SET slug = %s WHERE slug = %s',
+            (new_slug, old_slug),
+        )
+        msg.respond('shorturl `{}` has been renamed to `{}`'.format(old_slug, new_slug))


what happens when the old slug does not exist?

nothing, the where clause doesn't match anything so it's a no-op

cg505 · 2019-01-29T21:36:05Z

ircbot/plugin/shorturls.py

+            'UPDATE shorturls SET target = %s WHERE slug = %s',
+            (new_target, slug),
+        )
+        msg.respond('shorturl `{}` updated'.format(slug))


likewise, what happens when the slug does not exist?

kpengboy · 2019-01-30T05:34:44Z

ircbot/plugin/help.py

@@ -10,7 +10,8 @@
 def register(bot):
    threading.Thread(target=help_server, args=(bot,), daemon=True).start()
    bot.listen(r'^help$', help, require_mention=True)
-    bot.listen(r'^macros$', help_macro, require_mention=True)
+    bot.listen(r'^macros?$', help_macro, require_mention=True)
+    bot.listen(r'^shorturls?$', help_shorturls, require_mention=True)


Do we plan to have any authentication before deleting shorturls?

If possible, I would try to limit deletion to ops or half-ops (do we even have half-ops?)

same with replacement and similar non-adding commands

kpengboy · 2019-01-30T05:47:17Z

@abizer if it were up to me I'd write the dedicated shortlink service now and run it on Marathon if Kubernetes isn't ready yet. My two cents...

chriskuehl · 2019-01-30T06:07:39Z

I gotta +1 everyone else, I think serving redirects from the ircbot is just a bit too crazy.

What if you instead configure death to proxy the shorturls to some PHP script hosted in ~staff/public_html? It'll be a 5-line PHP script to query a row from the database and redirect to it, and you can worry about making it into a proper service with version control and all the other goodies later on.

jvperrin · 2019-01-30T06:23:38Z

Or just put the redirects themselves in ocfweb, which has 3 instances and should be highly available, and have the getting/setting still added here. Wouldn't be too bad to implement since it'd only have to read/write from the db to make the redirects so the ocfweb part would be pretty minimal. Honestly I think it's the cleanest solution that fits with our current setup...

abizer · 2019-01-30T06:39:54Z

putting the bouncer in ocfweb seems like a good idea

Baisang · 2019-01-30T07:34:02Z

ircbot/plugin/shorturls.py

+        return
+
+    if len(slug) > 255:
+        msg.respond('shorturl slugs must be <= 255 characters')


this unfairly discriminates against all of those massive facebook URLs with a million query parameters! how else will Big ZUCC be able to do the machine learnings to see how people are clicking the links? /s

this only limits the size of the slug itself, not the target

Baisang · 2019-01-30T07:36:57Z

Is there a record of the DB schema anywhere? Is that something we do?

I do love me some schematized data, we can at least store some random .sql file somewhere in git

dkess

I suppose this lgtm assuming we don't serve from the ircbot. I also still think there should be other ways of adding shorturls, but this is fine for now.

dkess · 2019-01-30T18:27:11Z

ircbot/plugin/shorturls.py

+    bot.listen(r'^!shorturl add ([\w/]+) (.+)$', add, require_privileged_oper=True)
+    bot.listen(r'^!shorturl delete ([\w/]+)$', delete, require_privileged_oper=True)
+    bot.listen(r'^!shorturl rename ([\w/]+) ([\w/]+)$', rename, require_privileged_oper=True)
+    bot.listen(r'^!shorturl replace ([\w/]+) (.+)$', replace, require_privileged_oper=True)


Replace and rename aren't necessary IMO, if we want to change the shorturls we should just delete and re-add

dkess · 2019-01-30T18:28:28Z

ircbot/plugin/shorturls.py

+
+
+def retrieve(bot, slug):
+    """Reusable function to retrieve a shorturl by slug from the DB."""


Does this need the bot param?

kpengboy · 2019-01-31T05:17:31Z

Thanks again for taking the initiative on this. I approve of the basic structure of this as it now stands; I still think there are minor points that still ought to be addressed, which I've commented on.

Outdated

dkess

lgtm assuming this is tested

ircbot: add shorturl plugin

83b8d95

abizer requested review from jvperrin, chriskuehl, dkess, kkuehlz, bzh-bzh and cg505 January 28, 2019 11:41

abizer mentioned this pull request Jan 28, 2019

some useful shorturls + bit of sp19 cleanup ocf/puppet#512

Merged

abizer requested a review from Baisang January 28, 2019 11:46

fawaf reviewed Jan 28, 2019

View reviewed changes

ircbot/plugin/help.py Outdated Show resolved Hide resolved

kpengboy previously requested changes Jan 29, 2019

View reviewed changes

cg505 approved these changes Jan 29, 2019

View reviewed changes

abizer added 2 commits January 29, 2019 15:00

shorturl regex to only allow alphanum and /

c066279

shorturl register helper and bugfix

477f530

kpengboy reviewed Jan 30, 2019

View reviewed changes

Baisang reviewed Jan 30, 2019

View reviewed changes

abizer added 2 commits January 30, 2019 03:50

remove shorturl bouncer from ircbot

10ab80e

move shorturls to their own db, require oper to modify

1ebed71

dkess approved these changes Jan 30, 2019

View reviewed changes

shorturls: update shorturls to use ocflib functionality

2c617b0

shorturls: update comments

0cc96ea

dkess approved these changes Feb 28, 2019

View reviewed changes

abizer merged commit 8a31401 into ocf:master Feb 28, 2019

cg505 mentioned this pull request Aug 21, 2019

Move ocf.io to kubernetes ocf/projects#9

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

ircbot: add shorturl plugin #94

ircbot: add shorturl plugin #94

abizer commented Jan 28, 2019 •

edited

Loading

dkess commented Jan 28, 2019

abizer commented Jan 28, 2019

kpengboy left a comment •

edited

Loading

kpengboy Jan 29, 2019

kpengboy Jan 30, 2019 •

edited

Loading

abizer commented Jan 29, 2019

cg505 left a comment

cg505 Jan 29, 2019

abizer Jan 29, 2019

cg505 Jan 29, 2019

abizer Jan 29, 2019

kpengboy Jan 30, 2019

Baisang Jan 30, 2019

Baisang Jan 30, 2019

kpengboy commented Jan 30, 2019

chriskuehl commented Jan 30, 2019

jvperrin commented Jan 30, 2019

abizer commented Jan 30, 2019

Baisang Jan 30, 2019

abizer Jan 30, 2019

Baisang commented Jan 30, 2019

dkess left a comment

dkess Jan 30, 2019

dkess Jan 30, 2019

kpengboy commented Jan 31, 2019

dkess left a comment



		def register(bot):
		# [!-~] is all printable ascii except spaces



		def retrieve(bot, slug):
		"""Reusable function to retrieve a shorturl by slug from the DB."""

ircbot: add shorturl plugin #94

ircbot: add shorturl plugin #94

Conversation

abizer commented Jan 28, 2019 • edited Loading

dkess commented Jan 28, 2019

abizer commented Jan 28, 2019

kpengboy left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kpengboy Jan 30, 2019 • edited Loading

Choose a reason for hiding this comment

abizer commented Jan 29, 2019

cg505 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kpengboy commented Jan 30, 2019

chriskuehl commented Jan 30, 2019

jvperrin commented Jan 30, 2019

abizer commented Jan 30, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Baisang commented Jan 30, 2019

dkess left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kpengboy commented Jan 31, 2019

dkess left a comment

Choose a reason for hiding this comment

abizer commented Jan 28, 2019 •

edited

Loading

kpengboy left a comment •

edited

Loading

kpengboy Jan 30, 2019 •

edited

Loading