Multi level pfx path #189
Multi level pfx path #189
Conversation
|
@steve-perkins Can we push this forward? The issue is a blocker since prefixed KV v2 secrets are not usable. |
|
We'd love to have this merged too :) |
| * | ||
| * @param prefixPathDepth integer number of path elements in the prefix path | ||
| */ | ||
| public VaultConfig prefixPathDepth(int pathLength) { |
There was a problem hiding this comment.
Maybe rename this method setPrefixPathDepth ?
| * @param prefixPath string prefix path, with or without initial or | ||
| * final forward slashes | ||
| */ | ||
| public VaultConfig prefixPath(String prefixPath) { |
There was a problem hiding this comment.
I suggest setPrefixPathDepthFromString because this method does not actually set a prefixPath attribute
| throw new IllegalArgumentException("can't use an empty path"); | ||
| } | ||
|
|
||
| while ((orig < pathLen) && |
There was a problem hiding this comment.
You could maybe simply call StringUtils.split(prefixPath, '/').length ?
-> https://commons.apache.org/proper/commons-lang/apidocs/org/apache/commons/lang3/StringUtils.html#split-java.lang.String-char-
| @@ -11,6 +11,20 @@ alike, without causing conflicts with any other dependency. | |||
| NOTE: Although the binary artifact produced by the project is backwards-compatible with Java 8, you do need | |||
| JDK 9 or higher to modify or build the source code of this library itself. | |||
|
|
|||
| This Change | |||
There was a problem hiding this comment.
👍
This is a great explanation, but it should probably be moved below in this README.md
Maybe as a sub-section of Key Value Secret Engine Config
There was a problem hiding this comment.
Definitely agree with this. The placement of this text here is pretty bizarre, and future people reading the README will have no context of which PR/change this paragraph is talking about.
I'll re-work this myself, post-merge.
There was a problem hiding this comment.
Yes, I intended for you to move it to release notes once a release number had been assigned.
| */ | ||
| public VaultConfig prefixPathDepth(int pathLength) { | ||
| if (pathLength < 1) { | ||
| throw new IllegalArgumentException("pathLength must be > 1"); |
|
Any chance this can get pushed forward any time soon? been blocked waiting for this for a while now. |
|
Sure, thanks! I have a less-insane-than-usual day today, so I'll take a look at the other open PR's and hopefully cut a binary release if everything goes smooth. |
|
Eeesh... not only are there no tests with this PR, but it breaks the existing tests. This code didn't even compile before it was submitted. It'll take me a bit longer to clean this up and make it shippable. |
|
Thanks for taking the time to improve & merge this ! |
|
In updating the unit test suite to address broken tests, I've discovered that this PR accidentally fixes a bug with the previous versions. Previously, when a secret path had only ONE segment, then the implementation would result in that path having a trailing slash character appended. When a secret path had more than one segments, then the resulting path would only have a trailing slash if the original did. That was awkward and had no good reason for being that way, and appears to have simply been an accidental quirk. Regardless, this PR "fixes" that. As implemented here, the modified secret paths with ALWAYS match the original in terms of having a trailing slash or not. I don't THINK this is a breaking change. But it worries me somewhat, and I'd love to hear any second opinions from anyone else following this thread. |
|
Any ETA on when this will be in a release? |
|
Steve, |
|
Oh, also -- do you want a fix for the issue you describe above ("In updating the unit test suite to address ..."), or would you rather I change the tests so they pass with the way I originally coded LogicalUtilities.addQualifierToPath ? IOW, do you want to keep the original trailing-slash behavior, or keep the "accidental bugfix". |
|
Sorry for the delay here. I've resolved the issues with the integration test suite, and bumped the version up to 5.1.0. I just merged an unrelated pull request (#194), which is causing two broken unit tests. I'd like to give that author a chance to respond to my ping, and/or take a look to see if I can resolve that issue myself. If not, then I'll revert the #194 change and push a new 5.1.0 binary artifact to Maven Central this week. Not taking in any additional new features for this release. |
|
Unfortunately, I'm having to roll back that proposed change for now. There are other changes waiting to go out in a new release, and the automated tests are in a failing state due to this change. If any contributor wants to look at the #194 PR changes, and take a crack at them in a way that doesn't break test coverage, then we can get them out in the next release. |
The vault driver lib has a issue with vault resource pathing ref: BetterCloud/vault-java-driver#189 Forcing use of version 1, which is the version on the resources nada-kafka-conenct uses atm, seems to solve the issue.
Please merge the following change into the mainline if you approve. Our admins insist on the flexibility to allow multiple path elements in path prefixes; this apparently also is consistent with Hashicorp Vault documentation. The change should be fully backward-compatible, causing no issues for users who don't want to take advantage of it. See the (branched) README.md for further information.