Fix ScrollID used for retrieval of APM Agents configuration

[up2neck]

• Fixed broken APM Agents configuration cache

Motivation/summary

APM Agents configuration cache appears to be broken, resulting frequent invalid requests made by APM Server to Elasticsearch cluster.

Checklist

• Update CHANGELOG.asciidoc
• Documentation has been updated

For functional changes, consider:

• Is it observable through the addition of either logging or metrics?
• Is its use being published in telemetry to enable product improvement?
• Have system tests been added to avoid regression?

How to test these changes

1. Deploy Elastic Stack: Elasticsearch, Kibana, APM Server.
2. Configure self-instrumentation for APM Server
3. Open Kibana/APM
4. Check APM Server has no failed operations of type "Elasticsearch: POST .apm-agent-configuration/_search" for Elasticsearch dependency

Related issues

closes #13957

[cla-checker-service]

💚 CLA has been signed

[mergify]

This pull request does not have a backport label. Could you fix it @up2neck? 🙏
To fixup this pull request, you need to add the backport labels for the needed
branches, such as:

• backport-7.17 is the label to automatically backport to the 7.17 branch.
• backport-8./d is the label to automatically backport to the 8./d branch. /d is the digit.

NOTE: backport-skip has been added to this pull request.

[carsonip]

Thanks for the PR. Nice catch!

Ideally this PR also updates the test to assert scroll id payload in elasticsearch_test.go, but it may not be trivial. I'm happy to accept this PR as is, and I can add a test in a follow up PR.

[up2neck]

@carsonip
Alongside this, what do you think about adding check of "scroll_id" is not empty after cache renewal?
It might require of new error type for such case.

[carsonip]

Alongside this, what do you think about adding check of "scroll_id" is not empty after cache renewal?

It doesn't hurt, but personally I do not think there is a need in handler to validate anything returned by Elasticsearch. We rely on Elasticsearch for a correct scroll ID, as well as correct search results. What's needed here is a better test, which I'm working on.

Either way the error will be surfaced, just more explicitly if an additional check is in place, or a little more obscurely in a form of HTTP 400 like this case.

[up2neck]

Alongside this, what do you think about adding check of "scroll_id" is not empty after cache renewal?

It doesn't hurt, but personally I do not think there is a need in handler to validate anything returned by Elasticsearch. We rely on Elasticsearch for a correct scroll ID, as well as correct search results. What's needed here is a better test, which I'm working on.

Either way the error will be surfaced, just more explicitly if an additional check is in place, or a little more obscurely in a form of HTTP 400 like this case.

But original code contains HTTP validation, but only for the first search request:

apm-server/internal/agentcfg/elasticsearch.go

Lines 255 to 278 in a5aaf07

	if scrollID == "" {
	resp, err := esapi.SearchRequest{
	Index: []string{ElasticsearchIndexName},
	Size: &f.searchSize,
	Scroll: f.cacheDuration,
	}.Do(ctx, f.client)
	if err != nil {
	return result, err
	}
	defer resp.Body.Close()
	if resp.StatusCode >= http.StatusBadRequest {
	// Elasticsearch returns 401 on unauthorized requests and 403 on insufficient permission
	if resp.StatusCode == http.StatusUnauthorized || resp.StatusCode == http.StatusForbidden {
	f.invalidESCfg.Store(true)
	}
	bodyBytes, err := io.ReadAll(resp.Body)
	if err == nil {
	f.logger.Debugf("refresh cache elasticsearch returned status %d: %s", resp.StatusCode, string(bodyBytes))
	}
	return result, fmt.Errorf("refresh cache elasticsearch returned status %d", resp.StatusCode)
	}
	return result, json.NewDecoder(resp.Body).Decode(&result)
	}

Probably we can modify it to handle errors in both requests:

func (f *ElasticsearchFetcher) singlePageRefresh(ctx context.Context, scrollID string) (cacheResult, error) {
	var result cacheResult
	var err error
	var resp *esapi.Response

	if scrollID == "" {
		resp, err = esapi.SearchRequest{
			Index:  []string{ElasticsearchIndexName},
			Size:   &f.searchSize,
			Scroll: f.cacheDuration,
		}.Do(ctx, f.client)
	} else {
		resp, err = esapi.ScrollRequest{
			ScrollID: scrollID,
			Scroll:   f.cacheDuration,
		}.Do(ctx, f.client)
	}
	if err != nil {
		return result, err
	}
	defer resp.Body.Close()

	if resp.StatusCode >= http.StatusBadRequest {
		// Elasticsearch returns 401 on unauthorized requests and 403 on insufficient permission
		if resp.StatusCode == http.StatusUnauthorized || resp.StatusCode == http.StatusForbidden {
			f.invalidESCfg.Store(true)
		}
		bodyBytes, err := io.ReadAll(resp.Body)
		if err == nil {
			f.logger.Debugf("refresh cache elasticsearch returned status %d: %s", resp.StatusCode, string(bodyBytes))
		}
		return result, fmt.Errorf("refresh cache elasticsearch returned status %d", resp.StatusCode)
	}
	return result, json.NewDecoder(resp.Body).Decode(&result)
}

[carsonip]

But original code contains HTTP validation, but only for the first search request

Correct. There is no handling for bad HTTP response codes in scroll requests. In retrospect, there should be. Thanks for adding it to the PR.

However, that increases the scope of this PR a little bit, and it will need to be tested on our end to avoid introducing regression (I don't expect any).

[carsonip]

lgtm, thanks for the PR. I can confirm this fixes the scrolling bug by my follow-up PR to add tests: #13959 . The refactoring to handle http errors should be fine, as expired scroll requests returns 404.

[carsonip]

run docs-build

[carsonip]

@up2neck do you mind signing the commits and force push? See https://docs.github.com/authentication/managing-commit-signature-verification/about-commit-signature-verification

[up2neck]

But original code contains HTTP validation, but only for the first search request

Correct. There is no handling for bad HTTP response codes in scroll requests. In retrospect, there should be. Thanks for adding it to the PR.

However, that increases the scope of this PR a little bit, and it will need to be tested on our end to avoid introducing regression (I don't expect any).

Made it

[carsonip]

run docs-build

[up2neck]

@carsonip unfortunately, Github lost my commit signature after rebase in web UI. Have restored it.

[carsonip]

run docs-build

[carsonip]

run docs-build