Skip to content
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

Implement a new standalone client #215

Merged
merged 66 commits into from
Mar 1, 2024

Conversation

kdubb
Copy link
Contributor

@kdubb kdubb commented Dec 19, 2023

TL;DR
This implements a new standalone client as contemplated in #214. It aims to solve as many of the raised issues as possible while making the development easier as things move forward.

To see how it's used and some basic capabilities look at the very limited tests. There's some simple but good examples of its usage.

But... here's the headline...

try (var httpClient = new JDKVaultHttpClient(HttpClient.newHttpClient())) {

    var client = VaultClient.builder()
            .executor(httpClient)
            .baseUrl(vault.getHttpHostAddress())
            .build();

    // read a secret

    client.secrets().kv2().readSecret("some-secret")
      .await()

    // read a secret from a specific mount
    
    client.secrets().kv2("my-kv").readSecret("some-secret")
      .await()

}

Below is an overview of the implementation as it currently stands.

VaultRequest & VaultReqeuestExecutor

The client is built around a VaultRequest. It is a class that models every feature of a Vault request like authentication tokens, namespaces, mounts, paths, etc; it ignores HTTP although there is some similarity due to the way Vault defines it's API.

VaultRequests are processed by a simple single function interface the VaultRequestExecutor. The interface is simple to implement and allows layering and progressively configuring the request as it's passed to the final executor, which will pass it to a Vault instance via an HTTP client implementation.

This completely decouples all of the client from any specific HTTP implementation. Currently there are Vert.x and JDK implementations which are fairly small. I plan on breaking the Vertx client out into its own separate module. In the future modules for other HTTP clients can be added as requested (e.g. the Apache HttpClient).

This allows neat things like simplified mocking or adding a tracing logger to the chain.

The classes are located in io.quarkus.vault.common.

Code Generation

I wrote a code generator using JavaPoet that reads from a YAML "spec" file. It's an all original YAML schema and associated generator designed to make it to generate Vault style APIs.

Vault does self report an OpenAPI schema but it is woefully underspecified and seemingly leaves properties un-types. Reading the documentation you get a much more complete picture of the apis. This is the reason for this dedicated generator. The positives are that it generates classes in the same style as we currently implement by hand and it is very easy to look at the Vault docs and transcribe a new API from them.

In the future we can look at generating the YAML files from some other source like scraping the web docs or if the OpenAPI schema starts to fill in the gaps.

I'll spare you the details in this PR of the YAML schema. I intend to write a document that fully explains it, until then you can see the specs in src/main/specs of the client module. Check out secerets/kv1.yaml!

Generated APIs

For Each API spec two main classes are generated, a "Request Factory" where each method returns a partially configured VaultRequest and matching API where where each method returns an actual result from calling the API.

This decoupling comes in very handy when implementing complicated authentication like using a wrapping token to retrieve a password to use in "auth/userpass" authentication.

As an example for the KV1 secrets engine an API class named VaultSecretsKV1 is generated and a factory class named VaultSecretsKV1RequestFactory is generated.

Each method in both these interfaces is named for the vault endpoint and takes the parameters appropriate to the method; and possibly a generated options class for methods with a lot of parameters.

These classes are vended by the vault client and it's "accessor" classes that allow user code to mimic the Vault API and docs.

See VaultSecretsKV1 and VaultSecretsKV1RequestFactory.

Authentication via Token Providers

Authentication methods are implemented via VaultTokenProvider which simply vends a (possibly time limited) VaultToken. Again these can be layered, for example token caching is implemented in VaultCachingTokenProvider and simply wraps another VaultTokenProvider.

If the implementation needs to support unwrapping, it can be written to use a VaultUnwrappedTokenProvider for the unwrapped piece.

Importantly, both VaultTokenProvider and VaultUnwrappedTokenProvider are passed a VaultRequestExecutor (via VaultAuthRequest) so they can execute Vault requests as needed.

Altogether, this allows the token providers, using the generated request factories, to build and execute the required requests without needing a "client" or event the generated API object itself.

For example in the VaultUserPassTokenProvider here's how it calls Vault to login:

executor.execute(VaultAuthUserPass.FACTORY.login(mountPath, username, password))

@kdubb kdubb requested review from vsevel and gsmet December 19, 2023 03:00
@kdubb
Copy link
Contributor Author

kdubb commented Dec 19, 2023

The see the generated APIs you need to checkout the PR and at least run mvn compile. This should generate the APIs in the client/target/generated-sources/vault directory.

@kdubb kdubb force-pushed the feature/standalone-client branch 3 times, most recently from 59014c1 to 267af37 Compare January 3, 2024 02:44
@kdubb kdubb marked this pull request as ready for review January 3, 2024 06:48
@kdubb
Copy link
Contributor Author

kdubb commented Jan 3, 2024

@vsevel The client now implements all the secret & auth engines that are currently supported by the current client. It also supports all the sys apis supported/used by the client and a few extra. Additionally, all the implemented apis are complete up to Vault version 1.15; making many of the much more complete than previously.

The apis follow the documentation pretty close and methods are named after the documented title (e.g. sys/leases/lookup is accessed via sys().leases().read(...) because the doc title is "Read Lease"). All of them include complete testing and you can look at the ample tests to see how each api works.

I'm interested to see what changes are suggested for generated code before this is merged and the apis become somewhat "locked".

Here's the complete list of supported apis:

auth

  • approle
  • kubernetes
  • token
  • userpass

secrets

  • database
  • kv1
  • kv2
  • pki
  • rabbitmq
  • totp
  • transit

sys

  • auth
  • health
  • init
  • leases
  • mounts
  • plugins
  • policies
  • remount
  • seal
  • tools
  • wrapping

@kdubb
Copy link
Contributor Author

kdubb commented Jan 3, 2024

@vsevel We should probably do a beta release (e.g 3.5.0-beta) that has nothing but the new client added to allow people to use it with easy access. Once we're happy with the apis and usage of the client. After that we can switch the Quarkus implementation to use the new client.

@aaronz-vipaso
Copy link

@kdubb How does this change affect the backward compatibility of the extension API? Will I have to update my code that uses the extension after updating the extension to the version with the new client?

@kdubb
Copy link
Contributor Author

kdubb commented Jan 4, 2024

@aaronz-vipaso Currently this PR is purely additive. It doesn't touch the extension in any fashion.

When integrating it we can choose to re-implement the current "engines" with the new client or we can choose to remove them and have people use the new client instead.

Personally, I'm in favor or doing a major release (e.g. 4.0) and removing the current engine implementations and having the extension use the new client to implement Quarkus specific features like static configuration, configuration sourcing, and dynamic credentials.

For most users this will not be a source breaking change as the most common use seems to be to provide configuration values and/or dynamic credentials, not to interact directly with Vault via APIs.

For those of us interacting with Vault directly, the current engines are very limited and statically configured (except those that have been updated to allow dynamic mounting). Switching to the new client should only be a positive change and should be fairly simple.

For example, previously you might inject the VaultPKIManagerFactory to manage a PKI engine mount like so:

@Inject VaultPKIManagerFactory pkiFactory;

void issueCertFromMount(String pkiMount) {
  var pki = pkiFactory.engine(pkiMount);
  pki.issue(...);
}

// or using the Quarkus "configured" PKI mount

@Inject VaultPKIManager pki;

void issueCert() {
  pki.issue(...);
}

With the new client the code would look like:

@Inject VaultClient client;

void issueCertFromMount(String pkiMount) {
  client.secrets().pki(pkiMount).issue(...);
}

// or using a configured mount

@ConfigProperty("app.pki.mount") String pkiMount;
@Inject VaultCleint client;

void issueCert() {
  client.secrets().pki(pkiMount).issue(...);
}

While this will be a source breaking change, having multiple methods to do something is a maintenance nightmare and the new client is simpler to use for these tasks, has a uniform api, and naturally supports dynamic usage.

@kdubb kdubb force-pushed the feature/standalone-client branch 3 times, most recently from 55b2972 to fc24917 Compare January 6, 2024 20:37
@kdubb
Copy link
Contributor Author

kdubb commented Jan 7, 2024

@aaronz-vipaso I've completed this PR by replacing the extension's client with the standalone client, leaving in place all the current engines. It does remove all the "internal" classes, but as long as your code uses the public classes it shouldn't require any changes.

@kdubb
Copy link
Contributor Author

kdubb commented Jan 8, 2024

@vsevel My suggested plan for this is as follows.

  1. When ready, merge this PR.
  2. Release 3.5.0 based on this PR as it only alters (by removal) "internal" classes this shouldn't be a source breaking change.
    • Maybe aligned with Quarkus 3.7 as well.
  3. Deprecate the current Quarkus extension "engines" for KV, Transit, PKI and TOTP
    • Pointing users to their equivalent usage in the new client
    • Includes deprecating any related Quarkus configuration for old engines
  4. Release 3.6.0 with deprecation.
  5. When we hit 4.0.0, remove all deprecations.

Copy link
Contributor

@vsevel vsevel left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In the future we can look at generating the YAML files from some other source like scraping the web docs or if the OpenAPI schema starts to fill in the gaps.

are they not using a similar approach?
can't we use the same source?

import io.quarkus.vault.client.json.JsonMapping;
import io.smallrye.mutiny.Uni;

public abstract class VaultHttpClient implements VaultRequestExecutor, AutoCloseable {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

a VaultHttpClient is not a VaultClient?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No. VaultRequestExecutor is the central type. VaultClient is just a member of the request processing chain; the root of it.

There are a few reasons why this is but it ensures a VaultClient can't be anything but the root because it does special things (e.g. token unwrapping) that need access to a request executor which cannot be a VaultClient.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

would there be a different name, avoiding the confusion?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not sure what you are suggesting we rename. Currently (I believe) they are named for exactly what they do. Examples?

@@ -27,7 +27,7 @@

import io.quarkus.test.QuarkusUnitTest;
import io.quarkus.test.common.QuarkusTestResource;
import io.quarkus.vault.runtime.client.VaultClientException;
import io.quarkus.vault.client.VaultClientException;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

that would be an api change?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah. I couldn't get around this without duplicating the name. Obviously, we need VaultClientException to be in the client module. The only solution I could think of was to derive a copy of in the runtime but then I realized it would never be thrown by the client itself.

When I was thinking would be 2 PR's (one for adding the standalone client and one for replacing the in-extension client) we could do a 3.5 and then 4.0 release to match "breaking" changes. Currently it looks like we need to release a 4.0 client and have end-users deal with the minor changes.

@kdubb
Copy link
Contributor Author

kdubb commented Jan 16, 2024

are they not using a similar approach?
can't we use the same source?

I look at all the current methods in the Vault ecosystem for getting a good source for the api request/response types and always found two problems...

  1. Underspecification.
    They don't specify response types; e.g. OpenAPI only gives you the request parameters and specifies the response as a freeform object. Additionally a lot of the request parameters are freeform and/or missing.

  2. Java Centricity
    To make a "nice" Java API I made the generator support a lot of "Java" stuff (e.g. generics, nested types, etc.). Reading from other languages would not allow this without manually editing the generated YAML or other structures.

@vsevel
Copy link
Contributor

vsevel commented Jan 16, 2024

one approach could have been to complement their specification. you do not repeat everything, you just extend their specification. just like SB does for kotlin, adding metadata when needed.
I am not advocating to change it now that you have done it. just one thing to keep in mind.

@kdubb
Copy link
Contributor Author

kdubb commented Jan 16, 2024

you just extend their specification

My thought after looking at everything was a one-shot copy to start you off (it generates a basic YAML in spec format) and the we customize it. This would make re-running it for updates problematic but it seemed like the way to go to ensure we don't end up with a non-native feeling Java client.

@vsevel
Copy link
Contributor

vsevel commented Feb 14, 2024

why did you have to move io.quarkus.vault.runtime.client.VaultClientException?

@kdubb
Copy link
Contributor Author

kdubb commented Feb 14, 2024

The client couldn't depend on a Quarkus package to be standalone.

Removes `VaultIOException` in favor of more informative `VaultClientException`. Also ensures all exceptions thrown by the client are `VaultClientException` instances.

`VaultException` is retained for other uses, e.g. exceptions thrown/derived in the Vault extension.
Some outside uses of Jackson seem to miss then JsonValue/JsonCreator format for enumerations. Doubling up ensures easy access for users (using `getValue()` and `from`) as well as all uses with Jackson.
This does not currently support the “non-proxy hosts” configuration that the Vert.x client does
Wraps the default `ProxySelector` to support non-proxy hosts configuration.
@kdubb
Copy link
Contributor Author

kdubb commented Mar 1, 2024

@vsevel Just FYI, I removed all the references to Java version in the POMs; mostly because I had setup the client to be 17 but target 11 before the changeover. It now relies on the parent quarkiverse POM for this.

I verified in the logs that it compiles with Java 17.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants