Namespace migrations are not applied on DSN memory

[zepatrik]

Describe the bug

It is not possible to dynamically change the namespaces when DSN is memory.

Reproducing the bug

1. start Keto with dsn: memory
2. trigger namespace change
3. observe no migrations applied

Expected behavior

At least the new namespaces should be migrated up, but the old ones should probably be migrated down as well.
This needs some thought on whether fs changes are reliable enough.

[aravindarc]

I need more details about this issue, after preliminary analysis. I came across this line in config/provider.go

configx.AttachWatcher(func(watcherx.Event, error) {
	// TODO this can be optimized to run only on changes related to namespace config
	kp.resetNamespaceManager()
})

and inside resetNamespaceManager function I found this:

// cancel and delete old namespace manager
// the next read request will result in a new one being created
k.cancelNamespaceManager()
k.nm = nil

1. Can you help me understand this claim that in the next read request, a new namespacemanager will be created.

2. In NamespaceManager function in config/provider.go there is a switch block
   if namespaces key in keto.yaml is a string (a url or file) the first case creates a NewNamespaceWatcher/Manager. If the namespaces key in keto.yaml is totally missing then there is a default "file://./keto_namespaces", which will error out in parseNamespace function in namespace_watcher.go, because this function expects a yaml, json or toml.

3. Further even if a proper yaml file is given to override the default file, the parsing happens against a namespace.Namespace{} object, meaning the file should contain something like this

id: 0
name: default

is this the intended behaviour, shouldn't it be an array like in keto.yaml

4. I found some issues in the underlying library fsnotify, which is reproducible even outside keto project, the issue is when we use fsnotify to watch a directory for changes (which for some reason is the way watcherx uses by default instead of watching individual file, watcherx/file.go at lines 19, 20), when there is a change in one of the file, the events generated sometimes contain a typo of the file name, there is a ~ symbol attached to the Name field in the event object, like this "keto_namespaces.yaml~", this happens randomly. This in turn is causing issue because in watcherx/file.go, in streamFileEvents function there is a line if filepath.Clean(e.Name) == watchedFile, this is not matching sometimes, because the Name field in the event object has a ~ attached to it sometimes.

[aravindarc]

Code to reproduce the fsnotify error:

func main() {
	// creates a new file watcher
	watcher, err := fsnotify.NewWatcher()
	if err != nil {
		fmt.Println("ERROR", err)
	}
	defer watcher.Close()

	//
	done := make(chan bool)

	//
	go func() {
		for {
			select {
			// watch for events
			case event := <-watcher.Events:
				//if !ok {
				//	return
				//}
				fmt.Printf("EVENT! %#v\n", filepath.Clean(event.Name))

				// watch for errors
			case err := <-watcher.Errors:
				fmt.Println("ERROR", err)
			}
		}
	}()

	dir := filepath.Dir("./keto_namespaces.yaml")
	// out of the box fsnotify can watch a single file, or a single directory
	if err := watcher.Add(dir); err != nil {
		fmt.Println("ERROR", err)
	}

	<-done
}

The output I get:

EVENT! "keto_namespaces.yaml~"
EVENT! "keto_namespaces.yaml~"
EVENT! "keto_namespaces.yaml"
EVENT! "keto_namespaces.yaml~"
EVENT! "keto_namespaces.yaml~"
EVENT! "keto_namespaces.yaml"
EVENT! "keto_namespaces.yaml"
EVENT! "keto_namespaces.yaml~"

My system configuration:
OS: Linux Mint 20.1 x86_64
Host: Latitude 3410
Kernel: 5.4.0-72-generic
Uptime: 1 day, 5 hours, 13 mins
Packages: 2860 (dpkg), 6 (snap)
Shell: zsh 5.8
Resolution: 1366x768
DE: Cinnamon
WM: Mutter (Muffin)
WM Theme: Mint-Y-Grey (Mint-Y)
Theme: Mint-Y [GTK2/3]
Icons: Mint-Y [GTK2/3]
Terminal: gnome-terminal
CPU: Intel i7-10510U (8) @ 4.900GHz
GPU: Intel UHD Graphics
Memory: 10375MiB / 15771MiB

[zepatrik]

1. This refers to the namespace manager getter function. It will create a new manager if it is nil:

keto/internal/driver/config/provider.go

Lines 154 to 155 in e5b76b3

	func (k *Provider) NamespaceManager() (namespace.Manager, error) {
	if k.nm == nil {

2. and 3. The namespace config is currently quite small, but as you can see in the zanzibar paper §2.3, it will get more complex eventually. That's why we wanted to go with either inline (for simple setups), or a path to a directory containing multiple namespace files. Therefore we don't watch a single file but a whole directory. You can see it in action in the corresponding test: https://github.com/ory/keto/blob/e5b76b35c495add911b6c2c61c4f51716ac64a3f/internal/driver/config/namespace_watcher_test.go

4. This is not a bug, but a feature 😉 No, seriously, you are probably using some editor to edit the files, and it creates a new temporary file with the appended ~ instead of overriding the old file. Then it deletes the old file and renames the temp file, or something along those lines. On linux, fsnotify uses inotify, which you can also directly invoke.
This is e.g. the output of deleting the file foo, then creating it again and applying changes using VSCode:

$ inotifywait -m .
Setting up watches.
Watches established.
./ OPEN foo
./ ACCESS foo
./ CLOSE_NOWRITE,CLOSE foo
./ MOVED_FROM foo
./ OPEN,ISDIR 
./ ACCESS,ISDIR 
./ CLOSE_NOWRITE,CLOSE,ISDIR 
./ OPEN,ISDIR 
./ ACCESS,ISDIR 
./ CLOSE_NOWRITE,CLOSE,ISDIR 
./ OPEN,ISDIR 
./ ACCESS,ISDIR 
./ CLOSE_NOWRITE,CLOSE,ISDIR 
./ CREATE foo
./ OPEN foo
./ OPEN,ISDIR 
./ ACCESS,ISDIR 
./ CLOSE_NOWRITE,CLOSE,ISDIR 
./ CLOSE_WRITE,CLOSE foo
./ OPEN foo
./ CLOSE_NOWRITE,CLOSE foo
./ MODIFY foo
./ OPEN foo
./ OPEN,ISDIR 
./ ACCESS,ISDIR 
./ CLOSE_NOWRITE,CLOSE,ISDIR 
./ MODIFY foo
./ OPEN,ISDIR 
./ ACCESS,ISDIR 
./ CLOSE_NOWRITE,CLOSE,ISDIR 
./ CLOSE_WRITE,CLOSE foo

As opposed to doing the same thing with rm, touch, and nano:

$ inotifywait -m .
Setting up watches.
Watches established.
./ DELETE foo
./ OPEN,ISDIR 
./ ACCESS,ISDIR 
./ CLOSE_NOWRITE,CLOSE,ISDIR 
./ OPEN,ISDIR 
./ ACCESS,ISDIR 
./ CLOSE_NOWRITE,CLOSE,ISDIR 
./ OPEN,ISDIR 
./ ACCESS,ISDIR 
./ CLOSE_NOWRITE,CLOSE,ISDIR 
./ OPEN,ISDIR 
./ ACCESS,ISDIR 
./ CLOSE_NOWRITE,CLOSE,ISDIR 
./ CREATE foo
./ OPEN foo
./ ATTRIB foo
./ CLOSE_WRITE,CLOSE foo
./ OPEN,ISDIR 
./ ACCESS,ISDIR 
./ ACCESS,ISDIR 
./ CLOSE_NOWRITE,CLOSE,ISDIR 
./ OPEN,ISDIR 
./ ACCESS,ISDIR 
./ CLOSE_NOWRITE,CLOSE,ISDIR 
./ OPEN foo
./ CLOSE_NOWRITE,CLOSE foo
./ OPEN,ISDIR 
./ ACCESS,ISDIR 
./ CLOSE_NOWRITE,CLOSE,ISDIR 
./ OPEN foo
./ CLOSE_NOWRITE,CLOSE foo
./ MODIFY foo
./ OPEN foo
./ MODIFY foo
./ OPEN,ISDIR 
./ ACCESS,ISDIR 
./ CLOSE_NOWRITE,CLOSE,ISDIR 
./ OPEN,ISDIR 
./ ACCESS,ISDIR 
./ CLOSE_NOWRITE,CLOSE,ISDIR 
./ CLOSE_WRITE,CLOSE foo
./ OPEN foo
./ ACCESS foo
./ CLOSE_NOWRITE,CLOSE foo

TL;DR editors do all kinds of fancy things to make your life as a developer harder.

Did this answer your questions?

[aravindarc]

Awesome, can I look into this issue?
I have already gone through the code, but still if you have any pointers on how to approach this issue, it would be helpful

[zepatrik]

In fact, it does not make much sense to fix this at the moment. In #613 we are currently figuring out a new architecture that will most likely not have a table per namespace, but just one table for all namespaces. My goal is to have an architecture drafted this week, so this issue will be obsolete very soon. But if you want to look into some other issue, I can give you pointers there.

[aravindarc]

Sure, would love to contribute, I just picked this up since it was marked as a good first issue, I have already did some debugging over #608, if you can recommend any other issues, will be able to contribute

[zepatrik]

I will close this as it will be obsoleted by #613