Consolidate path setting files entitlements to config #123649
Conversation
The setting based paths could be either absolute or relative, and they are always relative to the config dir. This commit renames the path_setting to make it clear it is related to config, and removes the relative variant.
rjernst
added
>refactoring
auto-backport
|
Pinging @elastic/es-core-infra (Team:Core/Infra) |
| if (platform == platform()) { | ||
| return this; | ||
| Stream<String> result; | ||
| if (setting.contains("*")) { |
There was a problem hiding this comment.
I'm wondering why we need this asterisk check. Does the settingGlobResolver path do the wrong thing if the setting contains no asterisk?
There was a problem hiding this comment.
Yes, the setting glob resolver only knows how to deal with glob.
There was a problem hiding this comment.
But a glob with no asterisks is still a glob right? Even if it's trivial.
There was a problem hiding this comment.
It's just a way to understand if we want to call the "regular" Settings resolved (which maps to Settings#get) or the glob resolver (which maps to Settings#getGlobValues). The latter throws if you don't pass down something that contains a ".*."
Btw I think we should match that here, and look for ".*.", not just *.
...rc/main/java/org/elasticsearch/entitlement/runtime/policy/entitlements/FilesEntitlement.java
Show resolved
Hide resolved
| mode: read | ||
| - relative_path_setting: foo.bar | ||
| relative_to: config | ||
| - config_path_setting: foo.bar |
There was a problem hiding this comment.
Hmm. This is the first entitlement whose meaning is not immediately obvious to me. It could be particularly confusing if the path has nothing to do with the config dir (ie. if it's an absolute path). path_setting made sense to me, because it's a setting representing a path.
Anyway I don't feel strongly about it. If you think plugin authors will understand this, that's what matters. And assuming we want all three words "config", "path", and "setting" in the name, I think you've picked the best possible order. 😂
There was a problem hiding this comment.
Yeah, I find it a bit awkward too. I also tend to think path_setting makes sense but it doesn't convey the implicit handling paths relative to config. But, to be honest, I'm not sure prefixing config to the name makes things much clearer.
This also keeps the door open to introducing other "path setting" entitlements in the future that have slightly different semantics.
There was a problem hiding this comment.
I'm not sure there's a need to convey that relative paths relate to config in the name of this field. That could be just how path_setting works, and mentioned in documentation. The first time someone says "huh, a relative path; I wonder what it's relative to?" the first place they'd look would be the docs for path_setting.
...st/java/org/elasticsearch/entitlement/runtime/policy/entitlements/FilesEntitlementTests.java
Show resolved
Hide resolved
The setting based paths could be either absolute or relative, and they are always relative to the config dir. This commit renames the path_setting to make it clear it is related to config, and removes the relative variant.
The setting based paths could be either absolute or relative, and they are always relative to the config dir. This commit renames the path_setting to make it clear it is related to config, and removes the relative variant.
The setting based paths could be either absolute or relative, and they are always relative to the config dir. This commit renames the path_setting to make it clear it is related to config, and removes the relative variant.
|
I see this has been already merged and I like it overall, but I agree that naming does not make it obvious. |
The setting based paths could be either absolute or relative, and they are always relative to the config dir. This commit removes the relative_path_setting property, and adds a mandatory basedir_if_relative property.