Skip to content

Conversation

@amcasey
Copy link
Member

@amcasey amcasey commented Aug 15, 2023

Also watch resolved targets of symlinks.

There are still some rough edges around directory symlinks, since non-polling watchers tend not to notify us when they change. I think there might be something we can do, but I have yet to investigate.

This may or may not work for the k8s case, depending on what events are produced when the ..data symlink is updated (and whether polling is enabled).

Also watch resolved targets of symlinks.

There are still some rough edges around directory symlinks, since non-polling watchers tend not to notify us when they change.  I think there might be something we can do, but I have yet to investigate.

This may or may not work for the k8s case, depending on what events are produced when the `..data` symlink is updated (and whether polling is enabled).
Specifically, move the file watcher up one level so that we can also see changes to the parent directory of a cert, even if it's a symlink.

Note that this approach still won't handle changes to directories further up the hierarchy.  It also does nothing for files directly in the root directory which, presumably, can't be modified anyway.

The tests are failing because they're testing for things that are no longer intended to be true.
@davidfowl
Copy link
Member

Why not do this in the runtime?

@amcasey
Copy link
Member Author

amcasey commented Aug 17, 2023

Why not do this in the runtime?

My plan was to port (some of) this code to runtime for 9.0. I understood Eric to say it wasn't on the table for 8.0.

@davidfowl
Copy link
Member

This feels like way too much specialized code in kestrel for what is supposed to be file watching.

@danmoseley
Copy link
Member

Cc @jozkee

@amcasey
Copy link
Member Author

amcasey commented Aug 18, 2023

This feels like way too much specialized code in kestrel for what is supposed to be file watching.

I generally agree, but deciding how much symlink resolution to do when watching files feels somewhat app-specific, so there would need to be knobs. (File symlinks are pretty straightforward, but walking up the directory hierarchy to find directory symlinks and junctions and then monitoring all of the paths indirectly referred to - probably with a watcher at the drive route - seems like more of an investment.)

I 100% agree that IFileProvider should at least expose information about directories and symlinks. That would be new API surface area (though mirroring a well-established API) but very little new code.

@jozkee
Copy link
Member

jozkee commented Aug 18, 2023

I feel like this should've been fixed by dotnet/runtime#55664. Do I understand correctly from #32351 (comment) that polling doesn't handle symlinks correctly? If so, that's a bug in our runtime libraries.

@amcasey
Copy link
Member Author

amcasey commented Aug 18, 2023

I haven't confirmed @aelij's result - I'm still having some trouble with the deploy script - but it makes sense to me. I wouldn't expect the poller's symlink code to see the ..data symlink change (unless I've missed something).

@jozkee
Copy link
Member

jozkee commented Aug 18, 2023

I wouldn't expect the poller's symlink code to see the ..data symlink change (unless I've missed something).

If we watch recursively, i.e: we are watching ..data/tls.key, then that file symlink can be resolved and the updated LastWriteTime would trigger the change event.

I don't remember if we watch recursively.

@amcasey
Copy link
Member Author

amcasey commented Aug 18, 2023

I wouldn't expect the poller's symlink code to see the ..data symlink change (unless I've missed something).

If we watch recursively, i.e: we are watching ..data/tls.key, then that file symlink can be resolved and the updated LastWriteTime would trigger the change event.

I don't remember if we watch recursively.

I wouldn't have guessed the filter "tls.key" would match "..data/tls.key", regardless of whether the watcher is aware of the change.

@jozkee
Copy link
Member

jozkee commented Aug 18, 2023

PhysicalFileProvider.Watch supports globing so it should be able to watch recursively and in that case I would expect it to pick up the change.

@amcasey
Copy link
Member Author

amcasey commented Aug 18, 2023

But it's effectively a coincidence that the symlink key.tls points to another file with the same name? It could just as easily be key.date.tls. We're not presently using globbing, since that seemed like the most straightforward way to be notified of changes to particular files (esp since the change notification does not, AFAIK, indicate which file was changed).

@jozkee
Copy link
Member

jozkee commented Aug 18, 2023

@amcasey can you please try using Polling with a * or */** pattern according to your needs e.g: dirMetadata.FileProvider.Watch(Path.GetFileName(path/*)).

Perhaps this PR is not needed if that works.

Here's the list of supported patterns supported by the underlying Matcher:
https://learn.microsoft.com/dotnet/api/microsoft.extensions.filesystemglobbing.matcher#remarks

@amcasey
Copy link
Member Author

amcasey commented Aug 18, 2023

@jozkee My concern with globbing is that two files can be related but have different names or be unrelated and have the same name. Am I missing something?

@jozkee
Copy link
Member

jozkee commented Aug 18, 2023

What does related mean in this context?

I somehow thought you wanted to watch the whole ..data folder, you are right you don't need globbing if you want to identify specific files. Looking at #49979 what you have there, creating one IChangeToken per file, lgtm.

@amcasey amcasey added area-networking Includes servers, yarp, json patch, bedrock, websockets, http client factory, and http abstractions and removed area-runtime labels Aug 25, 2023
@amcasey
Copy link
Member Author

amcasey commented Sep 6, 2023

This solution was partial, at best - it only looked up one level of the directory hierarchy.

@amcasey amcasey closed this Sep 6, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

area-networking Includes servers, yarp, json patch, bedrock, websockets, http client factory, and http abstractions

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants