Deprecate index audit output type#37671
Conversation
|
Pinging @elastic/es-security |
| static DeprecationIssue auditIndexSettingsCheck(List<NodeInfo> nodeInfos, List<NodeStats> nodeStats) { | ||
| List<String> nodesFound = nodeInfos.stream() | ||
| .filter(nodeInfo -> (nodeInfo.getSettings().getByPrefix("xpack.security.audit.outputs").isEmpty() == false) | ||
| || (nodeInfo.getSettings().getByPrefix("xpack.security.audit.index").isEmpty() == false)) |
There was a problem hiding this comment.
This seems to produce a deprecation issue if the outputs is non-empty, even if it only contains logfile.
Is that what you intended? It seems strange to deprecate any explicit configuration of the output type.
There was a problem hiding this comment.
Yes, it is intended. I plan to remove this setting altogether in 7.
There is no point of having a setting for the output type when there is only one output type.
The deprecation is there to warn that the setting will go away.
| ? Collections.emptyList() | ||
| : Collections.singletonList(LoggingAuditTrail.NAME), | ||
| Property.NodeScope); | ||
| Property.NodeScope, Property.Deprecated); |
There was a problem hiding this comment.
As above, is this really what we want?
I'm not totally against it, but the description of the PR, and the docs changes don't indicate that we're deprecating the outputs setting. Since there's only 1 non-deprecated value, and that's the default it makes some sense to just say the setting is obsolete, but the rest of this PR doesn't say that.
There was a problem hiding this comment.
Okay, I will update the description. Sorry if it came across as a "hidden" change!
| cluster or a separate cluster. | ||
| cluster or a separate cluster. deprecated[6.7.0, `logfile` will be the only audit | ||
| option available in 7.0. A separate log file ingestion component should instead | ||
| be used for indexing the audit trail.] |
There was a problem hiding this comment.
This deprecation comment confused me on my earlier review, because the text talks about "logfile" but not "output".
It probably reads OK in context, but I would have phrased it more like:
The
outputssetting will be removed in 7.0 as there will only be one supported output type (logfile). Users who wish to store their audit information in an Elasticsearch index should write to the file output, and a use a file ingestion component to index it into Elasticsearch.
Entirely up to you whether you want to update it.
There was a problem hiding this comment.
You're right, I have made the change as suggested! thanks 👍
|
https://elasticsearch-ci.elastic.co/job/elastic+elasticsearch+pull-request-1/5755/console
@elasticmachine run elasticsearch-ci/1 |
Supersedes #37301
This PR deprecates the index audit output.
In general, the problem with the index audit output is that event indexing can be slower than the rate with which audit events are generated, especially during the daily rollovers or the rolling cluster upgrades. In this situation audit events will be lost which is a terrible failure case for an audit system.
I will follow-up with the removal PR for 7.0 .
EDIT:
Besides of the settings under the
xpack.security.audit.indexnamespace, thexpack.security.audit.outputssetting has also been deprecated and will be removed in 7. Although explicitly configuring thelogfileoutput does not touch any deprecation bits, this setting is made redundant in 7 so this PR deprecates it as well.Relates #29881