f5devcentral / f5go Goto Github PK

When deleting a link in the edit pane that has multiple keywords you can unintentionally delete multiple keywords if you don't know what you are doing.

If you hit delete on this one, all keywords shown would get deleted.

title says it all, create the travis-ci build job which will run the unit tests on pull requests for verification.

We just need to get coherent docstrings into the main classes within the script. Right now, there aren't many indicators of the links between the different classes. It makes it tough to read through.

The important bit of information would be the relationships between all of them.

The way f5go is imlemented it currently uses GET for everything including updating records. While not an issue it is something that shouldn't be done, here's my logs showing a link being added via GET.

::ffff:10.0.0.10 - - [10/Jun/2016:23:53:25] "GET /add?title=&url=&otherlists=http%3A%2F%2F10.0.0.99%3A5000%2F&lists=nas&returnto=nas&error=URL+required HTTP/1.1" 500 1493 "http://go/_add_/nas" "Mozilla/5.0 (Windows NT 10.0; WOW64) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/50.0.2661.102 Safari/537.36"
::ffff:10.0.0.10 - - [10/Jun/2016:23:53:25] "GET /favicon.ico HTTP/1.1" 301 - "http://go/_add_?title=&url=&otherlists=http%3A%2F%2F10.0.0.99%3A5000%2F&lists=nas&returnto=nas&error=URL+required" "Mozilla/5.0 (Windows NT 10.0; WOW64) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/50.0.2661.102 Safari/537.36"

The current implementation of the Go Redirector uses a pickle file to store it's application data. This has been known to cause corruption (see Issue #4). The redirector should be refactorred to use a proper database system like postgres, or something similar or perhaps redis. A straight forward way to migrate the data from the pickle file to a database should be included.

One advantage this could have if implemented correctly is that the redirector would not have to keep the state in memory and could query the database allowing for multiple instances to be spun up as part of an HA pair.

During some testing of resiliency to power losses on the system, I noticed that the godb.pickle serialized data can get clobbered pretty bad and in very, very strange ways. In this particular failure, a 'pkill' on the process ID was all it took. In about half of my tries, I could corrupt the data. Then it wouldn't start back up and I had lost lots of edits and new lists which were added while it was up. This leads me to think about two possible improvements.

1. Go to a proper database, with atomic operations and cleaner startup/shutdown.
2. Get a proper database backup interval in place, whatever data source we end up using.

This is something to think about, for sure. As more and more people use this, while it works for most of the cases, it isn't as fault-tolerant as we'd like.

The list of links used within the last 30 days shows all links. Either the title should be updated or there should be this list and then a separate expandable list to show all links.

We need to return a better error here for the user. They're not told why an underscore is wrong, nor do I really know why it's wrong. Should we allow them?

Traceback (most recent call last):
  File "/usr/local/lib/python2.7/site-packages/cherrypy/_cprequest.py", line 670, in respond
    response.body = self.handler()
  File "/usr/local/lib/python2.7/site-packages/cherrypy/lib/encoding.py", line 217, in __call__
    self.body = self.oldhandler(*args, **kwargs)
  File "/usr/local/lib/python2.7/site-packages/cherrypy/_cpdispatch.py", line 61, in __call__
    return self.callable(*self.args, **self.kwargs)
  File "./go.py", line 757, in default
    ll = g_db.getList(keyword, create=False)
  File "/home/lab/tools/general/f5go/go.py", line 591, in getList
    raise InvalidKeyword("keyword '%s' not sanitary" % listname)
InvalidKeyword: keyword 'with_underscores' not sanitary

When having a URL that is duplicated across multiple redirect keywords, (for example a floor map) the UI is unintuitive

The variables page was disabled by me a long time back because we were having issues sanitizing input into the variables themselves. IIRC, someone had an asterisk within their variable name in some weird way and it broke the entire redirector. I couldn't figure it out at the moment, so I disabled the 'add a new variable' section on the variables.html section.

Eventually, once we figure out what is safe and unsafe for this field, we can get a unit test in place.

Currently the logs are only output to the stdout. They should be written to files (one for access logs, and one for the error logs). CherryPy does support this natively, http://docs.cherrypy.org/en/latest/basics.html#logging

Likely we'd want to use the native logging package so we could deal with log rotation and not fill the disk, but I need to do more research on if there are config options with cherrypy.config.update() that I can limit the file size.

This is just a placeholder issue for me to work on adding some unit tests.