Ojdbc provider hashicorp #142
Conversation
…ecrets to extract specific fields from JSON secrets.
…cret instead of application name
…Readme and javadocs
…xample-test.properties file
oracle-contributor-agreement
bot
added
the
OCA Verified
… cache, and enhanced README with documentation. Included example properties and unit tests for validation.
…e hcpvault secrets parameters, and fix README
…eld Extraction Logic in HCP Vault Dedicated
…cp vault dedicated a long with unit tests
…ix a typo in auto_detect auth method
… dedicated and hcp vault secrets providers
…riables in github actions pipeline
|
|
||
| ## Centralized Config Providers | ||
| <dl> | ||
| <dt><a href="#hcp-vault-secrets-config-provider">HashiCorp Vault Dedicated Config Provider</a></dt> |
There was a problem hiding this comment.
Title does not match description, Vault Dedicated and Vault Secrets seem to be inverted. Title does not seem to match link either
|
|
||
| <pre> | ||
| jdbc:oracle:thin:@config-hcpvaultdedicated://{secret-path}[?option1=value1&option2=value2...] | ||
| </pre> |
There was a problem hiding this comment.
What are option1, option2 .... ?
| * @param authToken The optional Bearer token for authorization. Can be null or empty. | ||
| * @param namespace The optional Vault namespace. Can be null or empty. | ||
| * @return A configured {@link HttpURLConnection}. Never null. | ||
| * @throws Exception if the connection cannot be established. |
There was a problem hiding this comment.
There are other reasons than "the connection cannot be established" for a connection to be thrown.
| * the response as a string. | ||
| * | ||
| * @param conn The configured HTTP connection. Must not be null. | ||
| * @param payload The payload to send. Must not be null. |
There was a problem hiding this comment.
Since the code assumes that it is an UTF8 String, maybe it should be documented.
| public static String sendGetRequestAndGetResponse(HttpURLConnection conn) throws Exception { | ||
| int responseCode = conn.getResponseCode(); | ||
| if (responseCode != HttpURLConnection.HTTP_OK) { | ||
| String errorResponse = ""; |
There was a problem hiding this comment.
Error handling code is duplicated, could be extracted to a method that would be used in both calls. Since the HttpURLConnection can only be used for a single request, is there a point in separating open and send ?
| * @param generator the token generator function. | ||
| * @return a {@code DedicatedVaultCredentials} instance. | ||
| */ | ||
| private static DedicatedVaultToken createScopedToken( |
There was a problem hiding this comment.
Does the token really have a scope? Or is it just a token that is cached and expires?
|
|
||
| switch (method) { | ||
| case VAULT_TOKEN: | ||
| return createTokenCredentials(parameterSet); |
There was a problem hiding this comment.
Can't this token be cached too, buy with a null expiration time ? That would avoid having a special case.
| case VAULT_TOKEN: | ||
| return createTokenCredentials(parameterSet); | ||
| case USERPASS: | ||
| return createScopedToken(parameterSet, method, DedicatedVaultTokenFactory::createUserpassToken); |
There was a problem hiding this comment.
Could the token generator function be moved to the Enum and maybe the function that generates the cache key as well?
| return cachedToken; | ||
| }); | ||
|
|
||
| if (validCachedToken.getToken() instanceof OpaqueAccessToken) { |
There was a problem hiding this comment.
If the token is declared as OpaqueAccessToken, this won't be needed.
| URL url = new URL(urlStr); | ||
| HttpURLConnection conn = (HttpURLConnection) url.openConnection(); | ||
| conn.setRequestMethod(method); | ||
| conn.setRequestProperty("Content-Type", contentType); |
There was a problem hiding this comment.
Seems strange to me to set the contentType on createConnection and not on send. How can you know the content before you send ?
| OracleJsonObject secretJsonObj = | ||
| new OracleJsonFactory() | ||
| .createJsonTextValue(inputStream) | ||
| .asJsonObject(); |
There was a problem hiding this comment.
Lines 97 - 103: isn't that what JsonUtil.parseJsonResponse does? Shouldn't it be used instead?
| .request(parameters) | ||
| .getContent(); | ||
|
|
||
| return new ByteArrayInputStream(secretString.getBytes()); |
There was a problem hiding this comment.
In other places in the code UTF8 encoding has been used but not here, why?
| } | ||
|
|
||
| @Override | ||
| public String getParserType(String arg0) { |
There was a problem hiding this comment.
Can you give the correct name to the argument (even though it is not used)?
| */ | ||
| private static String parseSecretJson(String secretJson, String secretPath) { | ||
| try (InputStream is = new ByteArrayInputStream(secretJson.getBytes(UTF_8))) { | ||
| OracleJsonObject rootObject = JSON_FACTORY.createJsonTextValue(is).asJsonObject(); |
There was a problem hiding this comment.
Here again, is there a reason for creating JsonUtil.parseJsonResponse if it is not used?
| * @throws IllegalArgumentException if the value is unrecognized. | ||
| */ | ||
| private static DedicatedVaultAuthenticationMethod parseAuthentication(String value) { | ||
| switch (value.toUpperCase()) { |
There was a problem hiding this comment.
Can you use DedicatedAuthenticationMethod.valueOf(String) instead?
| /** | ||
| * The username for Userpass authentication. Required for Userpass method. | ||
| */ | ||
| public static final Parameter<String> USERNAME = Parameter.create(REQUIRED); |
There was a problem hiding this comment.
Why are all authentication parameters declared here, if they are never used in this class?
No description provided.