SDS filesystem watches operate on file parent rather than grandparent

[htuch]

#10163 which resolved #9359 added support for inotify watching of directories to catch symlink rotation. I think this might have a bug. Specifically, if we have a cert in /foo/bar/cert.pem and perform a symlink rotation on bar, we don't see any SDS filesystem action.

The reason is that the watch is added on bar, rather than foo, see

envoy/source/common/secret/sds_api.cc

Line 68 in f95f539

watcher_->addWatch(absl::StrCat(result.directory_, "/"),

. The watcher test in the original PR actually does that, see https://github.com/envoyproxy/envoy/pull/10163/files#diff-c2abed8cf8fe469d48e4c17a0cfc0a35ef532c8c578c90f1e675a82d8200a01cR168.

@tsaarni can you confirm that this is an issue? I'm surprised it worked in your Contour example, but maybe some other feature was triggering the inotify, e.g. the file deletes?

I think the solution is to watch the grandparent, rather than the parent directory.

[htuch]

This is basically a dupe of #10979, except I claim the solution here is to watch grandparent, rather than document existing behavior. I think this should work for k8s.

[tsaarni]

If /foo/bar/cert.pem was from Kubernetes Secret or ConfigMap, the directory would have been set in following way:

/foo/bar/cert.pem                # symlink to ..data/cert.pem
/foo/bar/..data                  # symlink to ..timestamp-1
/foo/bar/..timestamp-1/cert.pem  # actual certificate file

after symlink swap it would look like

/foo/bar/cert.pem                # symlink to ..data/cert.pem
/foo/bar/..data                  # symlink to ..timestamp-2 (updated)
/foo/bar/..timestamp-2/cert.pem  # actual certificate file (updated)

If watching grantparent /foo I think it would not work in Kubernetes - the symlink swap is done within /foo/bar and no changes were made in /foo.

[htuch]

In this case, I'd argue we should be watching /foo/bar/..data/cert.pem, rather than /foo/bar/cert.pem. This makes this compatible with other key rotation schemes which work like described ^^.

[htuch]

We need to have a consistent directory-level symlink to follow to solve #13370 as well I think.

[tsaarni]

Inotify watch will get deleted automatically when target gets deleted:

(a) If watching /foo/bar/..data/cert.pem the watch gets deleted during swap when the old version of the file /foo/bar/..timestamp-1/cert.pem is deleted.

(b) If watching /foo/bar/..data the watch gets deleted during swap when the old symlink /foo/bar/..data --> /foo/bar/..timestamp-1/ is deleted.

and then further notifications are not received anymore. Case (a) is described in #9359 (comment) and (b) in #9359 (comment).

[htuch]

In my example, the inotify watch is places on /foo/bar, so it shouldn't be deleted; if you're saying this is what the implementation does because the underlying file has been deleted, I think we can fix the Envoy implementation.

[tsaarni]

Sorry, I think I then missed what you meant. If I set inotify watch for file /foo/bar/..data/cert.pem, it did not work after underlying file and therefore also watch got deleted at first rotate.

The current behavior is that when SDS config is as following:

resources:
  - "@type": "type.googleapis.com/envoy.extensions.transport_sockets.tls.v3.Secret"
    validation_context:
      trusted_ca:
        filename: /foo/bar/cert.pem

inotify is armed to watch all changes on directory /foo/bar. If comparing to runtime config, I think this is roughly equivalent to setting symlink root to /foo/bar which Kubernetes has as symlink root (the symlink being /foo/bar/..data)?

[htuch]

I'm suggesting we:

1. Set a watch on /foo/bar/..data/cert.pem.
2. Fix the existing behavior to watch on grandparent, i.e. on /foo/bar.
   The existing behavior relies on very specific k8s behavior where the cert is present both in the parent directory as well as in the symlinked directory. I think the general case would only be in the symlinked directory.

[tsaarni]

I think that could work. Some further thoughts:

(a) How to do this without breaking current users that rely on K8s Secret updates working with the normal path?

(b) The ConfigSource doc says "Envoy will only watch the file path for moves". Should this also be changed to grandparent as well?

This statement in the doc predated Envoy 1.14. It does not match runtime config description, but I thought this was intentional and it matched (by coincidence) with Kubernetes symlink implementation.

ConfigSource filesystem subscription seemed to be implemented according to this doc, i.e. watching the parent, not grandparent:

1. Full ConfigSource.path is passed to FileSystemSubscriptionImpl here
2. addWatch() is called with full ConfigSource.path here
3. inotify_add_watch() is called for parent directory here

(c) I doubt ..data is part of any K8s specification. Relying kubelet to use that exact name for the symlink may break, though maybe it is not too likely scenario.

[htuch]

(a) this would be a config/runtime option
(b) yes, this would be updated conditional on (a)
(c) it might not be in the k8s specification, but any sensible key rotation system is going to need to be doing directory swapping and have some canonical directory containing the cert/key.