CPM handle 404 response gracefully with user-friendly log

[kaisecheng]

Starting from es-output 12.0.2, a 404 response is treated as an error. Previously, central pipeline management considered 404 as an empty pipeline, not an error.

This commit restores the expected behavior by handling 404 gracefully and logs a user-friendly message.
It also removes the redundant cache of pipeline in CPM

Fixes: #17035

[kaisecheng]

es-output 12.0.2 leaves the caller to make the decision on what to consider a bad response code error. Although this PR allow CPM to handle 404 response gracefully with user-friendly log, the released versions with es-output 12.0.2 are still getting the stack trace in log

An alternative solution is to restore 404 behaviour in es-output.

[donoghuc]

Out of curiousity... Why not handle the 404 in fetch_config?

logstash/x-pack/lib/config_management/elasticsearch_source.rb

Line 274 in 5573b5a

def fetch_config(es_version, pipeline_ids, client)

or whatever class is using the client directly?

[kaisecheng]

Why not handle the 404 in fetch_config?

To handle in fetcher, it requires changes on SystemIndicesFetcher, LegacyHiddenIndicesFetcher, the internal state of @pipelines and unwrap the BadResponseCodeError. As we need to maintain logstash core to work with all supported es-output versions, keeping the minimum change in this case would be easier for maintainer

Thanks for review. The pipelines should be Array

[donoghuc]

I'm not quite sure I'm following your comment here #17052 (comment).

I think that the behavior we are trying to preserve is that fetch_config will just be an "empty" pipeline if there is a 404.

logstash/x-pack/lib/config_management/elasticsearch_source.rb

Lines 213 to 224 in 5573b5a

	def fetch_config(es_version, pipeline_ids, client)
	es_supports_pipeline_wildcard_search = es_supports_pipeline_wildcard_search?(es_version)
	retry_handler = ::LogStash::Helpers::LoggableTry.new(logger, 'fetch pipelines from Central Management')
	response = retry_handler.try(10.times, ::LogStash::Outputs::ElasticSearch::HttpClient::Pool::HostUnreachableError) {
	path = es_supports_pipeline_wildcard_search ?
	"#{SYSTEM_INDICES_API_PATH}?id=#{ERB::Util.url_encode(pipeline_ids.join(","))}" :
	"#{SYSTEM_INDICES_API_PATH}/"
	client.get(path)
	}
	if response["error"]
	raise ElasticsearchSource::RemoteConfigError, "Cannot find find configuration for pipeline_id: #{pipeline_ids}, server returned status: `#{response["status"]}`, message: `#{response["error"]}`"

( i had the wrong fetch_config linked in #17052 (comment) ). It seems to me like the responsibility of that is fully in the fetch_config method as it is the consumer of the plugin's client. Moving the required knowledge of an error type defined in logstash-output-elasticsearch to the source loader seems to perpetuate the underlying tech debt issue of using the plugin's client rather than our own.

It seems to me that the path forward would be to for now fix up instances of fetch_config to handle 404 (a rescue there should be forward/backward compatible) and then when we move away from using the plugin's client we just have one file to refactor?

[kaisecheng]

I see what you mean. We have two fetchers using client to get data, SystemIndicesFetcher for 7.10+ and LegacyHiddenIndicesFetcher for 6 & 7. The code you linked is SystemIndicesFetcher. I would avoid touching any part of LegacyHiddenIndicesFetcher, as it requires me to test against ES 6 & 7 and I know the returned value is neither an empty pipeline {} nor []

agree your point that it is better to put try catch block close to the caller class. In this case, it should be inside xpack.
I moved the block to xpack and certainly need to more tests to verify as my memory about system indices is very weak and not up-to-date.

when we move away from using the plugin's client we just have one file to refactor?

probably not, we have another use case licence checker which is also in xpack

[donoghuc]

Will this early return cause a difference in behavior whereby the @cached_pipelines are not updated on 404 now? Does that matter?

[kaisecheng]

It doesn't matter. The pipeline is empty anyway and the cache is purely optional

[kaisecheng]

Reviewing the flow, I decide to remove the cache because the existing pipelines can keep running without cache when error is raised. It was there to prevent cleaning up the running pipelines.

[donoghuc]

Thanks for the explanation. Given this is a short term solution anyway with the intent to get a dedicated client I think its OK if we are concerned about the risks/burdens you mentioned.

[elastic-sonarqube]

Quality Gate passed

Issues
0 New issues
0 Fixed issues
0 Accepted issues

Measures
0 Security Hotspots
No data about Coverage
No data about Duplication

See analysis details on SonarQube

[elasticmachine]

💛 Build succeeded, but was flaky

• Buildkite Build
• Commit: 5fb9c74

Failed CI Steps

• 🥼 IT Persistent Queues / part 1

History

• 💚 Build #2270 succeeded 419fa8c
• 💚 Build #2267 succeeded a0dee8e
• 💚 Build #2262 succeeded 7960a97

[donoghuc]

I couldn't quite trace back the git history for the addition of the cache (dead ends with a generic "migrate x-pack" bulk commit 93cad10.

Its kind of hard to trace through exactly what all calls this (or how many times), but i dont see anything egregiously consuming it.

[kaisecheng]

@logstashmachine backport 9.0

[kaisecheng]

@logstashmachine backport 8.18

[kaisecheng]

@logstashmachine backport 8.17

[kaisecheng]

@logstashmachine backport 8.16