Decouple API key version from node version#104156
Decouple API key version from node version#104156elasticsearchmachine merged 7 commits intoelastic:mainfrom
Conversation
This commit decouples the version stored in API key documents from the node version, as part of the broader effort to stop relying on node version for product logic. The actual data format does not change - the version is stored as an integer before and after this commit, it's just determined by a manually set `ApiKey.Version` class rather than being derived from the node version.
|
Pinging @elastic/es-security (Team:Security) |
# Conflicts: # x-pack/plugin/security/src/test/java/org/elasticsearch/xpack/security/authc/ApiKeyServiceTests.java
jfreden
left a comment
There was a problem hiding this comment.
Code looks good! Just a comment on the overall use of version for API keys.
The purpose of the version field in production looks like debugging? So if there is a version change of an API key and we try to update it with an older version (could this happen in mixed clusters or after downgrades?) a debug log will happen but the update is successful.
After this is introduced, the process of bumping the version number for API Keys when breaking changes are introduced will be manual, which makes it easy to forget about and maybe not that useful?
If the case for the: ApiKey.Version(currentVersionedDoc.doc().version); assert currentDocVersion.onOrBefore(targetDocVersion) : "current API key doc version must be on or before target version"; for CI purposes is strong enough, maybe that's enough to keep it around though, but after decoupling that one will also be less useful I think.
| } | ||
| } | ||
|
|
||
| public record Version(int version) { |
There was a problem hiding this comment.
Not sure if implementing VersionId would be helpful here. Maybe as a marker?
|
As its introducing a new version number here, it may be helpful to report it as a ComponentVersionNumber for diagnostic purposes. It also needs some docs indicating how you work with the version number - how to increment it, any dependencies, etc |
|
@thecoop Thanks, I wasn't aware of those things and I will implement them posthaste! Some thoughts on the purpose of this/docs indicating how you work with it (which I will add): This is really mostly a precautionary field, because eventually we'll want to make a change to the API key format and we'll need some way to track the version. As far as I can tell, it hasn't been used for anything and we'll know what needs to happen with it when we need to use it. I'm usually on the side of "if we don't need it, remove it instead of fixing it" but version info is one of those things I consider kind of standard metadata for this kind of document these days, like createdAt/modifiedAt timestamp fields are just assumed for CRUD-type software - we will need it at some point. |
|
Sure - these extra things aren't necessary for it to work here, but it helps to have some diagnostics and documented procedures in place around handling the new version numbers |
|
@elasticmachine test this please |
|
Failure was a network failure during the build. |
|
@elasticmachine update branch |
This commit decouples the version stored in API key documents from the node version, as part of the broader effort to stop relying on node version for product logic. The actual data format does not change - the version is stored as an integer before and after this commit, it's just determined by a manually set
ApiKey.Versionclass rather than being derived from the node version.