-
Notifications
You must be signed in to change notification settings - Fork 306
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
FISH-788 Support sub-directories for MPCONFIG SecretDirConfigSource. #5006 #5007
Conversation
This adds support for mounting your secrets in subdirectories to create scopes. The relative path is used as the property name. A secret file "SECRET_DIR/foo/bar/secret" will be available as MPCONFIG value "foo.bar.secret". You can still put a file "foo.bar.secret" in "SECRET_DIR" and have the same value available. If both are present, the topmost file will be picked. Mix'n'match isn't supported. Retrieving "foo.bar.secret" from "SECRET_DIR/foo/bar.secret" will not work reliably when added at runtime.
@AlanRoth IIRC I signed the CLA already when I contributed to the former https://github.com/payara/docker-payaraserver-full repo. |
Ah, indeed you have. My apologies. |
jenkins test please |
...ice/src/main/java/fish/payara/nucleus/microprofile/config/source/SecretsDirConfigSource.java
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks @poikilotherm, I think this is a good initiative that needs to mature a little bit more.
As it is I find it confusing to reason about since the moving parts are checked and manipulated in many places. Many of the flaws I pointed out did not start with your changes but making the essential logic more complex really highlights that this class needs some more attention.
First of all what consistency do we really want to support? Are files allowed to be updated at runtime? If so what is the observable effect? What about misses and removed entries? And finally collisions.
As far as I understood you want to handle collisions as first (on time axis) found wins until eventually that very source would no longer hold the property which would open for other files to supply it? What about if two files from the very start contain same property? Which one wins? Do we make sure we process the files in the correct order for this effect?
...ice/src/main/java/fish/payara/nucleus/microprofile/config/source/SecretsDirConfigSource.java
Outdated
Show resolved
Hide resolved
...ice/src/main/java/fish/payara/nucleus/microprofile/config/source/SecretsDirConfigSource.java
Outdated
Show resolved
Hide resolved
...ice/src/main/java/fish/payara/nucleus/microprofile/config/source/SecretsDirConfigSource.java
Outdated
Show resolved
Hide resolved
...ice/src/main/java/fish/payara/nucleus/microprofile/config/source/SecretsDirConfigSource.java
Outdated
Show resolved
Hide resolved
...ice/src/main/java/fish/payara/nucleus/microprofile/config/source/SecretsDirConfigSource.java
Outdated
Show resolved
Hide resolved
jenkins test please |
Hi @jbee,
Absolutely. For starters: should we rename this class to ConfigDirSource to be consistent with docs? In 5.184 the
Files updated at runtime definitely is a must-have.
Removed entries definitely is a problem. As the code is working now, removed entries are still cached (and served). On second thought, that might be undesireable in case someone actually tries to "move" a config value (it's not necessarily a secret, also this is the primary use case) to another source with lower ordinal.
Collisions can happen in one scenario only: someone creates a file Lines 120 to 122 in 74288cb
First match wins, so someone using the source as available since v5.183 experiences the same behaviour, maintaining backward compatibility. I'd be happy to remove this and allow for the new subdir style only (so no more files with dots in flat file hierarchy). One edge case I didn't look at so far: it might be necessary to add support for ignoring file extensions. Someone might mount a file with a Docker volume as "property.txt", which could not be found. Let me know if you think this is a thing. There is a small chance for this slippery edge case to make unexpected behaviour happen: someone adding a file to |
Small addition: we might also want to check that the file we are reading is not bigger than 512KB (which would already be quite huge for text configs) to prevent loading binary files etc by accident. |
@poikilotherm Good points. After getting more familiar with the case I think we should opt for the file watcher to update the map "in the background" while lookups are simply served from the map with no file IO checks. Map has entries with 3 fields. If it is in the map it is returned, if not we have no such property. When file watcher calls back for file changes we check all possible variations of the file for existence (dots or path variants) and always apply the same rule which one wins. This gives us "immediate update" while also always reflecting the same logic for what file wins even in case a new file is added at runtime that wins over a file existing prior to it. Does that make sense? As a side benefit we do get much clearer code where some methods only read the map and the file watcher code only updates the map (apart from the initial initialisation). |
@jbee I fully agree with you. Will update my PR accordingly. @jbee @AlanRoth as we need a background runner for the file watcher, is there any preferred way of creating one in Payara?
Found PayaraExecutorService, I'll use that.
|
...ig-service/src/main/java/fish/payara/nucleus/microprofile/config/source/DirConfigSource.java
Outdated
Show resolved
Hide resolved
Doing a review but it will take 1-2 more days before I get it done... |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Overall this is what I was thinking about, looks good. Don't be discouraged by the number of detail suggestions I made. Its mostly missing modifiers. I hope you can improve the initialisation of the source a bit. Maybe follow my suggestion to add an "init" method to add the watcher part so you can use the class with or without?
Ping again if you feel we can do a final review including tests.
...ig-service/src/main/java/fish/payara/nucleus/microprofile/config/source/DirConfigSource.java
Outdated
Show resolved
Hide resolved
...ig-service/src/main/java/fish/payara/nucleus/microprofile/config/source/DirConfigSource.java
Outdated
Show resolved
Hide resolved
...ig-service/src/main/java/fish/payara/nucleus/microprofile/config/source/DirConfigSource.java
Outdated
Show resolved
Hide resolved
...ig-service/src/main/java/fish/payara/nucleus/microprofile/config/source/DirConfigSource.java
Outdated
Show resolved
Hide resolved
...ig-service/src/main/java/fish/payara/nucleus/microprofile/config/source/DirConfigSource.java
Outdated
Show resolved
Hide resolved
...ig-service/src/main/java/fish/payara/nucleus/microprofile/config/source/DirConfigSource.java
Outdated
Show resolved
Hide resolved
...ig-service/src/main/java/fish/payara/nucleus/microprofile/config/source/DirConfigSource.java
Outdated
Show resolved
Hide resolved
...ig-service/src/main/java/fish/payara/nucleus/microprofile/config/source/DirConfigSource.java
Outdated
Show resolved
Hide resolved
...ig-service/src/main/java/fish/payara/nucleus/microprofile/config/source/DirConfigSource.java
Outdated
Show resolved
Hide resolved
...ig-service/src/main/java/fish/payara/nucleus/microprofile/config/source/DirConfigSource.java
Outdated
Show resolved
Hide resolved
Thanks @jbee for your extensive review. Glad you like what you see... Will address next week, so this might make it into 5.2020.7 😄 |
@poikilotherm, 5.2020.8 definitely 😉 5.2020.7 is ready and releasing very soon. Sorry to be a bearer of bad news, but you can take your time! |
@jbee one more question. There are a lot of occurrences of "secret dir" in class names, functions etc. Should I clean this up in the same go or is this beyond scope? |
If you are able to refactor all occurrences then sure, but I'm not too sure what you mean and what the changes would be? |
…now. This reverts commit 401555d.
I don't think we should be cutting away any extensions then (oh, I overlooked that part of code!). Windows users will manage without it, and it will be less surprising. |
…rConfigSource As requested by @pdudits, do not cut off file extensions silently. People on Windows can deal with text files without a file extension, better stay consistent. Relates to payara#5006
Jenkins test please |
So looks like this is getting somewhere. What's the way forward from here? I assume I should create another PR to the docs repo. Anything else I should try or do? |
Yes, updating the docs is the way to go next. I'll take care of merging this one in. |
Hi @poikilotherm, Congratulations on getting the PR through! 96 Comments! Our second most commented PR. Head over to our documentation, here and submit a PR. Let me know if you require any assistance. Thank you, |
…nality. This doc change aligns the docs with the new implementation of the directory config source for MicroProfile Config API. - See PR payara/Payara#5007 - Internal ticket FISH-788
Another comment (sorry) - does not pass QA:
|
Educated guess: forgot that every config source is initialized, no matter if enabled or not. So I have to add a null handler to the constructor or findDir(), to silently ignore this. What happens on configuration change? Would a user expect the source to start working when configuring via asadmin without a restart of the server? |
yes absolutely |
But that would be feature of its own, as entire Config Provider does not update its "meta-configuration" at runtime, rather at app deployment time. |
OK I'll open a new PR, as this one is merged and closed. I'll only work on the "not-configured" case, leaving the "hot config reload" for later. |
I do have one in baking actually, need to test it yet. |
Shall I prepare sth. for supporting it in this source? (Control flow, ...) |
During QA after merging payara#5007, @lprimak stumbled over a SEVERE log message during deploys, triggered by the non-existing directory of DirConfigSource. The directory config value has had a default before, also the docs say otherwise. The level issue has been fixed by checking only default config value being used, resulting in an INFO level message. The logic has been refactored not to use an exception, but Optional API. - Docs will be fixed to include the formerly hidden default value - Part of payara#5006
I misunderstood that part then. I read you worked on the hot reload stuff, but you worked on the source itself... Sorry 😐 |
…5104) * fix(mpconfig): make DirConfigSource less disrupting if not configured During QA after merging #5007, @lprimak stumbled over a SEVERE log message during deploys, triggered by the non-existing directory of DirConfigSource. The directory config value has had a default before, also the docs say otherwise. The level issue has been fixed by checking only default config value being used, resulting in an INFO level message. The logic has been refactored not to use an exception, but Optional API. - Docs will be fixed to include the formerly hidden default value - Part of #5006 * style(mpconfig): reducing log level in DirConfigSource.findDir() As requested by @pdudits, reducing the log level to FINE if the directory targeted by default value (secrets) is not present.
FISH-788 Support sub-directories for MPCONFIG SecretDirConfigSource. #5006
…5104) * fix(mpconfig): make DirConfigSource less disrupting if not configured During QA after merging #5007, @lprimak stumbled over a SEVERE log message during deploys, triggered by the non-existing directory of DirConfigSource. The directory config value has had a default before, also the docs say otherwise. The level issue has been fixed by checking only default config value being used, resulting in an INFO level message. The logic has been refactored not to use an exception, but Optional API. - Docs will be fixed to include the formerly hidden default value - Part of #5006 * style(mpconfig): reducing log level in DirConfigSource.findDir() As requested by @pdudits, reducing the log level to FINE if the directory targeted by default value (secrets) is not present.
Description
Closes #5006
This adds support for mounting your secrets (or configmapped values) in subdirectories to create scopes.
The relative path is used as the property name. A secret file
SECRET_DIR/foo/bar/secret
will be available as MPCONFIG value
foo.bar.secret
.You can still put a file
foo.bar.secret
inSECRET_DIR
and have the same value available.Any combination of dots and directories is supported, also the most specific (read: most subdirs and least dots in file and dir names) will win. Updates during runtime are respected and evaluated with longest match, too. An example:
SECRET_DIR/foo/bar.secret
or SECRET_DIR/foo.bar/secret` is fine, too.Important Info
Blockers
None.
Testing
New tests
Included.
Testing Performed
Run tests in
SecretsDirConfigSourceTest
Testing Environment
Documentation
See payara/Payara-Community-Documentation#114
Notes for Reviewers
None.