Add wsgi_autodetect.py to autodetect repositories with low complexity #267

wsldankers · 2021-05-02T15:18:56Z

Alternative take on the "automatically discovered repositories" concept that requires no threads, polling or inotify. Instead the filesystem is consulted whenever a repository name is looked up.

Since os.path.exists() and os.listdir() are fairly quick filesystem operations, performance should be good for small to medium sites. FancyRepo() objects are cached.

Repositories are identified (by default) by the existence of a <reponame>/git-daemon-export-ok file (for compatibility with gitweb).

Usage is similar to wsgi_autoreload.py:

uwsgi -w klaus.contrib.wsgi_autodetect \
      --env KLAUS_SITE_NAME="Klaus Demo" \
      --env KLAUS_REPOS_ROOT=/path/to/repos \
      ...

To configure certain aspects you can add these flags:

      --env KLAUS_DETECT_REMOVALS=1 \
      --env KLAUS_EXPORT_OK_PATH=git-daemon-export-ok \

Setting KLAUS_DETECT_REMOVALS to 0 will improve repository listing performance at the cost of no longer being able to detect if a repository was removed.

You can set KLAUS_EXPORT_OK_PATH to . if you do not care about gitweb conventions.

jonashaag · 2021-05-03T09:36:52Z

I like the idea, adding a few code comments.

jonashaag · 2021-05-03T09:37:45Z

klaus/contrib/auto_klaus.py

+"""
+Alternative take on the "automatically discovered repositories" concept
+that requires no threads, polling or inotify. Instead the filesystem is
+consulted whenever a repository name is looked up.


I feel like it's important to mention here that each time the repo list is viewed by someone, the following code is run:

1x listdir() in the root

Nx os.path.exists in each of the folders

Added a blurb to that effect to the wsgi_autodetecting.py docstring.

jonashaag · 2021-05-03T09:39:53Z

klaus/contrib/auto_klaus.py

+
+Example usage:
+
+    from klaus.contrib.auto_klaus import Klaus, SlashDynamicRepos


Maybe we can find a more descriptive name than auto_klaus

Yeah this was just the first name I came up with as I was throwing this together for my own server, heh. I'm open to suggestions. The two hardest problems in computer science: naming things, cache invalidation and off-by-one errors. 😉

Renamed it to wsgi_autodetecting.py. Still a bit generic but hopefully more descriptive.

jonashaag · 2021-05-03T09:43:22Z

klaus/contrib/auto_klaus.py

+
+    application = Klaus('/srv/git', "My git repositories", False)
+
+    application.wsgi_app = httpauth.AlwaysFailingAuthMiddleware(


Most people using klaus won't be able to understand how to actually run a klaus server with this code. We should have something as simple as this: https://github.com/jonashaag/klaus/wiki/Autoreloader

Sure, I'll look into that.

Added a wsgi_autodetect.py in the style of wsgi_autoreload.py.

jonashaag · 2021-05-03T09:46:00Z

klaus/contrib/auto_klaus.py

+        self._repos = {}
+
+    def __getitem__(self, name):
+        if not name or name[0] == '.' or '/' in name or '\0' in name:


I think we should also check for os.pardir here?

It's included in name[0] == '.' for most common operating systems but we could include os.pardir os.curdir, os.sep and os.altsep explicitly, yes. Better to be safe than sorry.

Added all of these to the validation expression.

jonashaag · 2021-05-03T09:46:22Z

klaus/contrib/auto_klaus.py

+        if not name or name[0] == '.' or '/' in name or '\0' in name:
+            raise KeyError(name)
+
+        repos = self._repos


That alias is confusing, please don't do it.

Removed (and one other instance of the same pattern as well).

Not a fan of the repeated subexpressions it causes, though.

jonashaag · 2021-05-03T09:46:54Z

klaus/contrib/auto_klaus.py

+        root = self._root
+        return (
+            repo for repo in os.listdir(root)
+                if os.path.exists(root / repo / 'git-daemon-export-ok')


Could we save a few exists() checks here by checking self._repos for a positive match first?

That's what I had at first, in fact. But then the admin cannot dynamically retract access from a repository. I suppose we could make that optional.

Right but if we keep the safety check + .pop() in __getitem__ that's an acceptable compromise IMO.

I don't follow. The “safety check + .pop()” is what we have now. The alternative is to remove the safety check and just rely on the presence of self._repos[repo]. I don't see the compromise you're trying to get at. Edit: ahh, you meant safety for new repos.

Added the detect_removal option as a compromise between performance and correctness.

jonashaag · 2021-05-03T09:49:44Z

klaus/contrib/auto_klaus.py

+    def __len__(self):
+        return len(self._base)
+
+class Klaus(klaus.Klaus):


I don't think it's wise to create a subclass of klaus.Klaus because it will potentially break anytime we make changes to klaus.Klaus. Can you think of another way of solving this that doesn't require using a "private" API? (I'm happy with doing changes to klaus.Klaus if it's necessary.)

Well we could change these lines in klaus/__init__.py to always use the SlashDynamicRepos proxy (perhaps under a different name):

dulwich_backend = dulwich.server.DictBackend( { "/" + namespaced_name: repo for namespaced_name, repo in app.valid_repos.items() } )

Then add an optional named parameter so we can pass a constructor that determines how to handle the first argument to Klaus(), dependency-injection style. load_repos() would be moved to a default implementation for that.

One thing I didn't get though, is why invalid_repos exists, it seems like it could be an information leak.

Implemented: Klaus now has a repo_container_factory (apologies for the Java-esque naming) that receives the repo_paths parameter as its argument and returns a repository container. Such a container implements the python dict interface and additionally has an invalid property to store any invalid repositories (exposed as invalid_repos on Klaus).

jelmer · 2021-05-03T17:17:23Z

klaus/contrib/auto_klaus.py

+
+        repos = self._repos
+        path = self._root / name
+        if not os.path.exists(path / 'git-daemon-export-ok'):


what about non-bare repositories?

I suppose we could make the path to check configurable, so you can configure it to be .git/git-daemon-export-ok or even something completely different if you don't care about gitweb conventions.

Checking for git-daemon-export-ok is quite specific to being compatible with gitweb, and a change from the behaviour that currently exists in klaus. For example, most of my repositories that are visible in klaus don't have a git-daemon-export-ok file and wouldn't work with this script. Perhaps it would make sense to call that out more explicitly?

I agree that it's kind of annoying that we have multiple ways to deal with autodiscovery/autoreloading. It long overdue someone contributes a well thought out solution into Klaus proper.

I wonder if we should make this "flag file" approach part of the existing autoreloading code? To me it seems the only user-visible difference between this implementation and the existing one the git-daemon-export-ok file.

ldrumm · 2021-05-13T21:32:18Z

This is a great idea. I'm going to close my 5 year old inotify PR that I was one day going to get round to in favour of this one.

ldrumm

I've tested this on my server and it's performing well modulo the one issue described above. I like the git-daemon-export-ok idea too as I'd otherwise create a public directory and a bunch of symlinks which I've never entirely conveniced by.

ldrumm · 2021-05-15T18:16:20Z

klaus/contrib/auto_klaus.py

+
+        repos = self._repos
+        path = self._root / name
+        if not os.path.exists(path / 'git-daemon-export-ok'):


I think there's a bug here WRT .git suffix handling

If I have a repo named repo.git with repo.git/git-daemon-export-ok, then it's properly listed on the index page, but the link generated resolves to /repo which then 404s because ./repo/git-daemon-export-ok doesn't exist. klaus.contrib.autoreload exhibits the same suffix removal, but correctly resolves the shortened name.

I'm not sure what the best approach to resolve this is, but I think this mapping might need a minor rethink

- Rename auto_klaus.py to wsgi_autodetect(ing).py and make its usage more like the existing wsgi_autoreload(ing).py scripts. - Factor out the repository container functionality from the Klaus object into its own class hierarchy (RepoContainer). - Make certain aspects of the automatic detection configurable (specifically, the path that determines whether a subdirectory is a valid repo, and whether it should detect removed repos).

wsldankers force-pushed the master branch from cd0b0d3 to 33c287f Compare May 2, 2021 20:26

jonashaag reviewed May 3, 2021

View reviewed changes

jelmer reviewed May 3, 2021

View reviewed changes

wsldankers changed the title ~~Add auto_klaus.py to autodetect repositories with low complexity~~ Add wsgi_autodetect.py to autodetect repositories with low complexity May 5, 2021

wsldankers force-pushed the master branch 2 times, most recently from c30a04a to c968f61 Compare May 5, 2021 21:06

ldrumm mentioned this pull request May 13, 2021

Added a wsgi application that automatically discovers all git repositori... #112

Closed

ldrumm reviewed May 15, 2021

View reviewed changes

jonashaag added Feature Ready to work on labels Jul 14, 2021

wsldankers added 2 commits November 15, 2021 16:02

Add auto_klaus.py to autodetect repositories with low complexity

fea8fd0

wsldankers force-pushed the master branch from c968f61 to 1f4a734 Compare November 15, 2021 15:03

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add wsgi_autodetect.py to autodetect repositories with low complexity #267

Add wsgi_autodetect.py to autodetect repositories with low complexity #267

wsldankers commented May 2, 2021 •

edited

jonashaag commented May 3, 2021

jonashaag May 3, 2021

wsldankers May 5, 2021

jonashaag May 3, 2021

wsldankers May 3, 2021

wsldankers May 5, 2021

jonashaag May 3, 2021

wsldankers May 3, 2021

wsldankers May 5, 2021

jonashaag May 3, 2021

wsldankers May 3, 2021 •

edited

wsldankers May 5, 2021

jonashaag May 3, 2021

wsldankers May 5, 2021

jonashaag May 3, 2021

wsldankers May 3, 2021 •

edited

jonashaag May 4, 2021

wsldankers May 4, 2021 •

edited

wsldankers May 5, 2021

jonashaag May 3, 2021

wsldankers May 3, 2021

wsldankers May 5, 2021

jelmer May 3, 2021

wsldankers May 3, 2021

jelmer May 3, 2021

jonashaag May 4, 2021

ldrumm commented May 13, 2021

ldrumm left a comment

ldrumm May 15, 2021 •

edited


		Example usage:

		from klaus.contrib.auto_klaus import Klaus, SlashDynamicRepos


		application = Klaus('/srv/git', "My git repositories", False)

		application.wsgi_app = httpauth.AlwaysFailingAuthMiddleware(

Add wsgi_autodetect.py to autodetect repositories with low complexity #267

Are you sure you want to change the base?

Add wsgi_autodetect.py to autodetect repositories with low complexity #267

Conversation

wsldankers commented May 2, 2021 • edited

jonashaag commented May 3, 2021

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

Choose a reason for hiding this comment

wsldankers May 3, 2021 • edited

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

wsldankers May 3, 2021 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

wsldankers May 4, 2021 • edited

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

Choose a reason for hiding this comment

ldrumm commented May 13, 2021

ldrumm left a comment

Choose a reason for hiding this comment

ldrumm May 15, 2021 • edited

Choose a reason for hiding this comment

wsldankers commented May 2, 2021 •

edited

wsldankers May 3, 2021 •

edited

wsldankers May 3, 2021 •

edited

wsldankers May 4, 2021 •

edited

ldrumm May 15, 2021 •

edited