Reworking proxy configuration#96
Conversation
| For the filesystem-based snapshot, passwords are stored in a `passwords.yaml` file in the | ||
| configuration directory. This file maps resource names to their passwords: | ||
|
|
||
| ```yaml | ||
| # passwords.yaml | ||
| my-keystore: changeit | ||
| ``` |
There was a problem hiding this comment.
This is a bit weak. It precludes the possibility of different secret content coming from different trust zones, because someone has to assemble a single file where all the passwords are in the clear.
It would be better if this allows people to add their key store and their password each as its own file. That way FS permissions could be used so that Bob can add his keystore, but not decrypt Alice's keystore when he does so.
| The keystore format need not be user-specified. | ||
| Common formats have distinctive magic bytes (JKS: `0xFEEDFEED`, PKCS12: ASN.1 `0x30`, PEM: `-----BEGIN`). | ||
| The deserialiser probes the bytes and selects the appropriate format. | ||
| PKCS12 is the default keystore type since Java 9; JKS is legacy. |
There was a problem hiding this comment.
If we support file extensions other than just .data then maybe the file extension could also be used. It's not clear whether the consumer of the bytes gets passed just the file content (bytes) or can also see the extension.
Are the magic bytes and trial deserialization hard coded to just the common stores? Should probably explain in a bit more detail how this might work for other keystore types, like EJKS.
| format: | ||
|
|
||
| ``` | ||
| kroxylicious migrate-config -i legacy-config.yaml -o output-dir/ |
There was a problem hiding this comment.
Do we really want to bolt keywords onto the existing kroxylicious app that's the entry point to running the proxy? It could be the top of a slippery slope, where we add utility functionality which ends up bloating the container image for the proxy, and including functionality which we might not want to be available to the proxy.
As an alternative, we could have a separate kroxyctl which had this functionality (and other similar code that's not needed for the proxy process).
Initial draft proposing a Kubernetes-inspired multi-file configuration system with explicit plugin references, dependency graph validation, JSON Schema validation, and a migration tool. Assisted-by: Claude Opus 4.6 <noreply@anthropic.com> Signed-off-by: Tom Bentley <tbentley@redhat.com>
The five numbered motivations now match the original problem statement: per-plugin schemas, uniformity, dependency understanding for dynamic reloading, individual plugin identity for control plane, and principled config versioning. Assisted-by: Claude Opus 4.6 <noreply@anthropic.com> Signed-off-by: Tom Bentley <tbentley@redhat.com>
Plugin instance files must include a `name` field matching the filename. This makes files self-describing for debugging and non-filesystem sources. Assisted-by: Claude Opus 4.6 <noreply@anthropic.com> Signed-off-by: Tom Bentley <tbentley@redhat.com>
Explain why HasPluginReferences exists (explicit > introspection), why PluginReference is not a breaking change (config versioning), and why we enforce referential integrity unlike Kubernetes. Clarify which types are public API (kroxylicious-api) vs internal. Assisted-by: Claude Opus 4.6 <noreply@anthropic.com> Signed-off-by: Tom Bentley <tbentley@redhat.com>
Signed-off-by: Tom Bentley <tbentley@redhat.com>
This reverts commit 83938a7. Signed-off-by: Tom Bentley <tbentley@redhat.com>
Plugin instance files must use FQCNs for the type field, making them self-describing without depending on which plugins are loadable. JSON Schema enum constraints provide editor code completion. Assisted-by: Claude Opus 4.6 <noreply@anthropic.com> Signed-off-by: Tom Bentley <tbentley@redhat.com>
…serialisation Plugin references in YAML are bare instance name strings. The config type constructs PluginReference internally via HasPluginReferences, combining the statically-known interface type with the instance name from YAML. Assisted-by: Claude Opus 4.6 <noreply@anthropic.com> Signed-off-by: Tom Bentley <tbentley@redhat.com>
…onfig2 proposal Describe the full Snapshot interface (metadata, bytes, passwords, generation numbers) once in the Snapshot abstraction section instead of partially describing it and then re-describing it in Non-YAML resources. Add @ResourceType annotation, TLS plugin interfaces, and filesystem layout for binary resources. Assisted-by: Claude Opus 4.6 <noreply@anthropic.com> Signed-off-by: Tom Bentley <tbentley@redhat.com>
Signed-off-by: Tom Bentley <tbentley@redhat.com>
Signed-off-by: Tom Bentley <tbentley@redhat.com>
- Document version dispatch via instanceof rather than passing version string - Mandate K8s CRD-compatible JSON Schema dialect for plugin schemas - Require schemas for versioned plugins - Add HasPluginReferences validation rule for versioned config types - Note that unreachable plugins are not an error - Expand @stateless with YAML example and lifecycle detail - Document ResolvedPluginRegistry as public API with full code context - Add dynamic reloading integration subsection - Fix dangling "these resources" reference in non-YAML section - Allow flexible sidecar file extensions (.p12, .jks) instead of .data - Add ANTLR line number caveat for inline resources - Add ResourceSerializer/ResourceDeserializer interface declarations - Document @ResourceType placement and package - Add trust zone limitation and K8s/Vault password sourcing detail - Note ResourceDeserializer receives bytes only, not file extension - Add concrete VaultKmsConfigV1 TLS material example - Add open question on migration tool packaging Assisted-by: Claude Opus 4.6 <noreply@anthropic.com> Signed-off-by: Tom Bentley <tbentley@redhat.com>
8e0e721 to
e7f8b3b
Compare
|
Discussed at Community Call 23rd April. Tom is looking for feedback. |
|
|
||
| The configuration model is tree-like. If two filters need the same KMS | ||
| then each must embed a copy of the KMS configuration. There is no way to define a plugin | ||
| instance once and reference it from multiple consumers. |
There was a problem hiding this comment.
YAML does have anchors and aliases
There was a problem hiding this comment.
It does, but that's more of a syntax level thing. The API we have for the config file (Jackson) would essentially duplicate those JsonNode or mapped POJOs, so we would see two objects which were .equals, but not ==. You can use @JsonIdentityInfo to make the references first class. But if you're seriously proposing that as an alternative then I think you should elaborate how you'd expect it to work.
| Similarly, external tools (editors, config linters, CI validators, Kubernetes operators) | ||
| cannot inspect or validate plugin configurations (see [2436](https://github.com/kroxylicious/kroxylicious/issues/2436)), | ||
| and there is no declarative way to validate plugin configurations before | ||
| the proxy starts (somewhat related: [3267](https://github.com/kroxylicious/kroxylicious/issues/3267#issuecomment-4042031788)). |
There was a problem hiding this comment.
there is no declarative way to validate plugin configurations before the proxy starts
that one aspect is solvable without redesigning the config model. Tools such as httpd and squid have config-check command line parameter.
There was a problem hiding this comment.
I think you're commenting on part of one sentence, and not really accounting for the full context in the paragraph as a whole. Sure, we can provide a tool. But that won't be built into anyone's editor, and won't provide anything like code completion, and doesn't address the documentation part.
| Plugin developers can ship a JSON Schema for each config version at: | ||
|
|
||
| ``` | ||
| META-INF/kroxylicious/schemas/{PluginSimpleName}/{version}.schema.yaml |
There was a problem hiding this comment.
puzzled why you chose simple name, when you made the case for FQCN above. Why the discrepancy?
There was a problem hiding this comment.
Claude chose that and I was too tired to argue. I agree with you.
|
|
||
| Discovery of schemas by external tooling depends on plugin developers publishing the schema so that it can be discovered by tools. | ||
| One way to do that is to publish the schema on a public webserver, allowing end users to make use of JSON Schema's `$schema` keyword. | ||
| Editors often support downloading such schemas. |
There was a problem hiding this comment.
I must confess, I've actually rarely done this myself. Maybe I'm an outlier? Is this something others commonly do?
There was a problem hiding this comment.
You might find, if you're using intellij for instance, that you're getting the benefits of schemas published to schemastore without even knowing it. It is, admittedly, more painful to configure an unpublished schema manually.
Personally I find the code completion and documentation part of using schemas at least as useful as the validation part.
| As a plugin evolves, the plugin implementation either accepts the YAML or it does not. | ||
| If a newer version of a plugin removes support for a configuration property the user will likely be confused, because the configuration used to work — but the plugin has no effective way to communicate that the user is speaking the old language. | ||
| Explicit version strings (following Kubernetes conventions: v1alpha1, v1beta1, v1) would give plugin developers a principled mechanism for config schema evolution, including support for multiple concurrent versions during migration periods. | ||
|
|
There was a problem hiding this comment.
One place I see the configuration schemas being very useful is validating the documentation. As our documentation gets larger, keeping the configuration snippets accurate will become increasing hard. Tool support will help greatly.
There was a problem hiding this comment.
That's what I said on line 52.
| } | ||
| ``` | ||
|
|
||
| ### Snapshot abstraction |
There was a problem hiding this comment.
to self: haven't followed this properly eyt
| 5. Writes `proxy.yaml` by passing through the raw YAML (stripping `filterDefinitions`, replacing | ||
| `micrometer` with instance name references). | ||
|
|
||
| **Open question:** whether the migration tool should be a subcommand of the proxy entrypoint or |
| encrypt.yaml # metadata + YAML config (no sidecar needed) | ||
| ``` | ||
|
|
||
| For a plugin instance named `foo`, the metadata is always in `foo.yaml`. If a sidecar data file exists, it must be the only other file in the directory whose name without extension is `foo` (e.g. `foo.p12`, `foo.jks`, `foo.pem`). Any extension is permitted (except `.yaml` which is taken), which allows filesystem tools that depend on extensions to work naturally. The runtime validates that at most one sidecar file exists per plugin instance. |
There was a problem hiding this comment.
What about plugins that need more than one data file? Limiting to a single file seems quite restrictive.
There was a problem hiding this comment.
It is restrictive to some extent. Maybe we should make it more flexible, but can you provide some more concrete use cases where it would be a benefit?
| For text-based resources like ACL rules, YAML multi-line syntax (`|`) is simplest. | ||
| The versioned config type can use a `String` field instead of a file path: | ||
|
|
||
| ```yaml |
There was a problem hiding this comment.
I believe it is. What makes you say that?
| * @return the password as a char array, or {@code null} if no password is configured | ||
| */ | ||
| @Nullable | ||
| char[] resourcePassword(String resourceName); |
There was a problem hiding this comment.
resources like keystores have a store password and key password, which maybe distinct.
There was a problem hiding this comment.
Correct, but look at the direction of travel: JKS is not recommented for new applications (since Java 9).
What became the new default, PKCS#12, in its Java implementation, more or less expects the passwords to be the same. Quoting gemini, the "best practice":
If you are using PKCS12 in Java, you should keep the store password and the key password identical. If you provide a different password for the key, many non-Java tools (and even some older Java application servers like Tomcat or JBoss) may fail to read the private key because they expect the passwords to match. If you need distinct security layers, it is better to use file-system-level permissions rather than relying on different PKCS12 passwords.
|
I see the attractions of the proposals - the modularity, re-use, schema validation are all desirable features. But this already looks like a significant piece of work and it'll take significant effort. There's bound to be issues that crop up during the implementation. That's all opportunity cost where we don't get to work on features (that are more directly applicable to use-cases). Do we really need it? Components like Httpd and Tomcat seem to have done fine without something analogous. I'm floating around +0.5 |
tombentley
left a comment
There was a problem hiding this comment.
Thanks for taking a look @k-wall.
I see the attractions of the proposals - the modularity, re-use, schema validation are all desirable features. But this already looks like a significant piece of work and it'll take significant effort. There's bound to be issues that crop up during the implementation. That's all opportunity cost where we don't get to work on features (that are more directly applicable to use-cases). Do we really need it? Components like Httpd and Tomcat seem to have done fine without something analogous.
One thing you've not pointed to there is the configuration versioning and schema evolution part. Our conversations about Kroxylicious 1.0 have landed on that as an important thing to resolve. This proposal addresses that, but you're saying we don't need it, or at least are unwilling to commit to the effort needed to implement this proposal. So I'm wondering whether you see a conflict in holding both those positions, or whether you think we can solve them in some other way, or just that 1.0 is not something we need.
|
|
||
| The configuration model is tree-like. If two filters need the same KMS | ||
| then each must embed a copy of the KMS configuration. There is no way to define a plugin | ||
| instance once and reference it from multiple consumers. |
There was a problem hiding this comment.
It does, but that's more of a syntax level thing. The API we have for the config file (Jackson) would essentially duplicate those JsonNode or mapped POJOs, so we would see two objects which were .equals, but not ==. You can use @JsonIdentityInfo to make the references first class. But if you're seriously proposing that as an alternative then I think you should elaborate how you'd expect it to work.
| Similarly, external tools (editors, config linters, CI validators, Kubernetes operators) | ||
| cannot inspect or validate plugin configurations (see [2436](https://github.com/kroxylicious/kroxylicious/issues/2436)), | ||
| and there is no declarative way to validate plugin configurations before | ||
| the proxy starts (somewhat related: [3267](https://github.com/kroxylicious/kroxylicious/issues/3267#issuecomment-4042031788)). |
There was a problem hiding this comment.
I think you're commenting on part of one sentence, and not really accounting for the full context in the paragraph as a whole. Sure, we can provide a tool. But that won't be built into anyone's editor, and won't provide anything like code completion, and doesn't address the documentation part.
| As a plugin evolves, the plugin implementation either accepts the YAML or it does not. | ||
| If a newer version of a plugin removes support for a configuration property the user will likely be confused, because the configuration used to work — but the plugin has no effective way to communicate that the user is speaking the old language. | ||
| Explicit version strings (following Kubernetes conventions: v1alpha1, v1beta1, v1) would give plugin developers a principled mechanism for config schema evolution, including support for multiple concurrent versions during migration periods. | ||
|
|
There was a problem hiding this comment.
That's what I said on line 52.
| Plugin developers can ship a JSON Schema for each config version at: | ||
|
|
||
| ``` | ||
| META-INF/kroxylicious/schemas/{PluginSimpleName}/{version}.schema.yaml |
There was a problem hiding this comment.
Claude chose that and I was too tired to argue. I agree with you.
|
|
||
| Discovery of schemas by external tooling depends on plugin developers publishing the schema so that it can be discovered by tools. | ||
| One way to do that is to publish the schema on a public webserver, allowing end users to make use of JSON Schema's `$schema` keyword. | ||
| Editors often support downloading such schemas. |
There was a problem hiding this comment.
You might find, if you're using intellij for instance, that you're getting the benefits of schemas published to schemastore without even knowing it. It is, admittedly, more painful to configure an unpublished schema manually.
Personally I find the code completion and documentation part of using schemas at least as useful as the validation part.
| encrypt.yaml # metadata + YAML config (no sidecar needed) | ||
| ``` | ||
|
|
||
| For a plugin instance named `foo`, the metadata is always in `foo.yaml`. If a sidecar data file exists, it must be the only other file in the directory whose name without extension is `foo` (e.g. `foo.p12`, `foo.jks`, `foo.pem`). Any extension is permitted (except `.yaml` which is taken), which allows filesystem tools that depend on extensions to work naturally. The runtime validates that at most one sidecar file exists per plugin instance. |
There was a problem hiding this comment.
It is restrictive to some extent. Maybe we should make it more flexible, but can you provide some more concrete use cases where it would be a benefit?
| For text-based resources like ACL rules, YAML multi-line syntax (`|`) is simplest. | ||
| The versioned config type can use a `String` field instead of a file path: | ||
|
|
||
| ```yaml |
There was a problem hiding this comment.
I believe it is. What makes you say that?
| * @return the password as a char array, or {@code null} if no password is configured | ||
| */ | ||
| @Nullable | ||
| char[] resourcePassword(String resourceName); |
There was a problem hiding this comment.
Correct, but look at the direction of travel: JKS is not recommented for new applications (since Java 9).
What became the new default, PKCS#12, in its Java implementation, more or less expects the passwords to be the same. Quoting gemini, the "best practice":
If you are using PKCS12 in Java, you should keep the store password and the key password identical. If you provide a different password for the key, many non-Java tools (and even some older Java application servers like Tomcat or JBoss) may fail to read the private key because they expect the passwords to match. If you need distinct security layers, it is better to use file-system-level permissions rather than relying on different PKCS12 passwords.
Add PRs kroxylicious#96, kroxylicious#98, kroxylicious#99, kroxylicious#100, kroxylicious#101, and kroxylicious#103 which were opened after the initial script was created. Note: PR kroxylicious#100 is already correctly named (100-sidecar-injection-webhook.md). Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com>
This change simplifies the proposal numbering system by using PR numbers as proposal identifiers, eliminating number collisions and removing the need for a separate allocation process. Changes: - Simplified proposals/README.md to focus on author workflow - Removed index tables (directory listing serves as the index) - Streamlined instructions for creating and renaming proposals - Updated proposal template with workflow instructions - Require PR number in title format: # <PR#> - <Title> - Moved workflow instructions into comment block - Added GitHub workflow to automatically check proposal numbering - Validates both filename and title format - Updates PR description when proposal files don't match PR number - Provides exact commands to fix naming issues - Removes warning once corrected - Handles both added and renamed files - Runs on all PRs (ready for mandatory status check) - Added notification script for existing open PRs - After merge, run notify-open-prs.sh to ask authors to rebase - Workflow will automatically guide them through renaming - Updated with all current open proposal PRs (kroxylicious#70, kroxylicious#82, kroxylicious#83, kroxylicious#85, kroxylicious#88, kroxylicious#93, kroxylicious#94, kroxylicious#96, kroxylicious#98, kroxylicious#99, kroxylicious#100, kroxylicious#101, kroxylicious#103) Proposals 001-019 retain their original numbers. Assisted-By: Claude Sonnet 4.5 <noreply@anthropic.com> Signed-off-by: Keith Wall <kwall@apache.org>
This change simplifies the proposal numbering system by using PR numbers as proposal identifiers, eliminating number collisions and removing the need for a separate allocation process. Changes: - Simplified proposals/README.md to focus on author workflow - Removed index tables (directory listing serves as the index) - Streamlined instructions for creating and renaming proposals - Updated proposal template with workflow instructions - Require PR number in title format: # <PR#> - <Title> - Moved workflow instructions into comment block - Added GitHub workflow to automatically check proposal numbering - Validates both filename and title format - Updates PR description when proposal files don't match PR number - Provides exact commands to fix naming issues - Removes warning once corrected - Handles both added and renamed files - Runs on all PRs (ready for mandatory status check) - Added notification script for existing open PRs - After merge, run notify-open-prs.sh to ask authors to rebase - Workflow will automatically guide them through renaming - Updated with all current open proposal PRs (#70, #82, #83, #85, #88, #93, #94, #96, #98, #99, #100, #101, #103) Proposals 001-019 retain their original numbers. Assisted-By: Claude Sonnet 4.5 <noreply@anthropic.com> Signed-off-by: Keith Wall <kwall@apache.org>
|
Hi! We've updated the proposal numbering system to use PR numbers as proposal identifiers. Action required: Please rebase your PR on Once you rebase, you'll need to rename your proposal file and update the title: git mv proposals/015-config2-multi-file-plugin-configuration.md proposals/096-config2-multi-file-plugin-configuration.md
# Update title: remove any old number prefix and add PR number
sed -i.bak '0,/^# /{s/^# \([0-9]\{3\}\|xxx\|nnn\|000\) - /# 96 - /; t; s/^# /# 96 - /}' proposals/096-config2-multi-file-plugin-configuration.md && rm proposals/096-config2-multi-file-plugin-configuration.md.bak
git add proposals/096-config2-multi-file-plugin-configuration.md
git commit -m "Rename proposal to use PR number"
git pushThe GitHub workflow will automatically check your proposal file naming after you push and update this PR description if any corrections are still needed. See proposals/README.md for the updated workflow. |
This proposal covers a significant reworking of the Kroxylicious proxy's configuration system.