Skip to content

fix: support multiple versions of Elasticsearch logger #12364

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 18 commits into from
Jun 26, 2025

Conversation

Revolyssup
Copy link
Contributor

@Revolyssup Revolyssup commented Jun 23, 2025

Fixes
Error comes when using elasticsearch version 9+
Screenshot From 2025-06-23 12-29-02

We want to remove the compat header and have a general support for all major versions.

For elasticsearch logger plugin comptability check. Following are the observations and conclusion based on reading docs and self testing.

The type field issue:
Before version 7, the type field was required in json schema https://github.com/elastic/elasticsearch/blob/8453f7701de25c467ce08088ddddac185cb8028c/rest-api-spec/src/main/resources/rest-api-spec/api/create.json#L21.
The current tests without type fail on version 6(here it's 6.7) with the following error

Batch Processor[elasticsearch-logger] failed to process entries: elasticsearch server returned status: 400, body: {"error":{"root_cause":[{"type":"action_request_validation_exception","reason":"Validation Failed: 1: type is missing;"}]

Adding a _type in the code fixes the above test.
Conclusion
As suggested in the migration guide, we can hardcode the type to "_doc" in versions prior to 7 and on 7+ we can omit type completely
Indices created in 6.x only allow a single-type per index. Any name can be used for the type, but there can be only one. The preferred type name is _doc, so that index APIs have the same path as they will have in 7.0: PUT {index}/_doc/{id} and POST {index}/_doc

The id field issue:
Another problem was detected while running tests on older version(6), when _id was not passed along with _index,. the following error was reported:

Batch Processor[elasticsearch-logger] failed to process entries: elasticsearch server returned status: 400, body: {"error":{"root_cause":[{"type":"action_request_validation_exception","reason":"Validation Failed: 1: an id must be provided if version type or value are set;"}],"

In older version, _id is also required https://github.com/elastic/elasticsearch/blob/8453f7701de25c467ce08088ddddac185cb8028c/rest-api-spec/src/main/resources/rest-api-spec/api/create.json#L9
But I when I used index API instead of create API, request passed even without _id for all versions 6,7,8

  •        create = {
    
  •        index = {
               _index = conf.field.index,
    
  •            _type = conf.field.type
    
  •            _type = "_doc",
           }
       }) .. "\n" ..
    

The doc says
A create action fails if a document with the same ID already exists in the target An index action adds or replaces a document as necessary.
An answer in the discuss forum says
Neither is used to create an index (although they may do so as a side-effect of inserting the first document into a new index). "index" indexes a document. "create" only indexes a document if it doesn't already exist. It used to be that "create" was faster if you knew that your docs didn't already exist. But now, since versioning was added, "index" performs as well as "create"
Another answer suggests
you use create if a doc with that id already exists, if you use index it will create a document or update it if xisting.
Conclusion: To me it looks okay to replace create with index

Conclusion:
We can remove type from plugin configuration and for detected version below 7, hardcode it as _doc
We can replace Create with Index in the request body.
We run all tests on version 8 and my PR adds a comptability test with version 9. We can add two more compatibilty tests with 6 and 7 to confirm our changes.

Result: We will be able to support ES versions 6,7,8,9

References and other relevant information:
https://www.elastic.co/guide/en/elasticsearch/reference/6.7/release-notes-6.7.0.html. Search "Deprecate types" in this doc.
From reading this issue: elastic/elasticsearch#35190
Though a temporary parameter was added include_type_name which defaults to true between 6.7 upto 7 meaning by default type was being sent but it could be turned off.
Then between 7 and 8 this field was default to false meaning by default the type will no longer be sent and a deprecated warning will come.
Then after 8+ this include_type_name was removed and type was no longer supported.
References:
https://www.elastic.co/docs/reference/elasticsearch/rest-apis/compatibility
https://www.elastic.co/guide/en/elasticsearch/reference/8.18/rest-api-compatibility.html
elastic/elasticsearch-ruby#2665

Checklist

  • I have explained the need for this PR and the problem it solves
  • I have explained the changes or the new features added to this PR
  • I have added tests corresponding to this change
  • I have updated the documentation to reflect this change
  • I have verified that this change is backward compatible (If not, please discuss on the APISIX mailing list first)

@dosubot dosubot bot added the size:L This PR changes 100-499 lines, ignoring generated files. label Jun 23, 2025
@Revolyssup Revolyssup marked this pull request as draft June 23, 2025 07:00
@dosubot dosubot bot added enhancement New feature or request plugin labels Jun 23, 2025
@Revolyssup Revolyssup marked this pull request as ready for review June 23, 2025 13:01
@dosubot dosubot bot added the bug Something isn't working label Jun 23, 2025
@Revolyssup Revolyssup requested a review from bzp2010 June 23, 2025 14:06
headers["Content-Type"] = headers["Content-Type"] .. compat_header_8
headers["Accept"] = headers["Accept"] .. compat_header_8
if conf.field.type then
core.log.warn("type is not supported in Elasticsearch 9, removing `type`")
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we should use error log level

@Revolyssup Revolyssup requested review from membphis and nic-6443 June 24, 2025 17:18
@@ -195,6 +264,11 @@ function _M.body_filter(conf, ctx)
log_util.collect_body(conf, ctx)
end

function _M.access(conf)
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

set_version calls ES server the first time and this http connection is not allowed in log phase so I had no other way but to add this here because version information is needed at the start of log phase when entries are being created.

}
if conf._version == "8" then
headers["Content-Type"] = headers["Content-Type"] .. compat_header_7
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

can we remove compat_header_7 and compat_header_8?

If yes, then we can move if conf._version == "8" then and elseif conf._version == "9" then.

just use the default header, pls confirm if it is ok:

local headers = {
        ["Content-Type"] = "application/x-ndjson",
        ["Accept"] = "application/vnd.elasticsearch+json"
}

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I do not like if conf._version == "8" then ... elseif conf._version == "9" then ... end style

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

removed it. not needed anymore

function _M.access(conf)
-- set_version will call ES server only the first time
-- so this should not amount to considerable overhead
set_version(conf)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

how about fetch_and_update_es_version?

core.json.encode(entry) .. "\n"
end

local function set_version(conf)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

local function set_version(conf)
    if conf._version then
        return
    end

        local selected_endpoint_addr
        if conf.endpoint_addr then
            selected_endpoint_addr = conf.endpoint_addr
        else
            selected_endpoint_addr = conf.endpoint_addrs[math_random(#conf.endpoint_addrs)]
        end
        local major_version, err = get_es_major_version(selected_endpoint_addr, conf)
        if err then
            return false, str_format("failed to get Elasticsearch version: %s", err)
        end
        conf._version = major_version
end

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done

end
local major_version, err = get_es_major_version(selected_endpoint_addr, conf)
if err then
return false, str_format("failed to get Elasticsearch version: %s", err)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

we should write error log here

we do not capture the error msg in access phase

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done

xpack.security.enabled: 'true'

elasticsearch-auth-4:
image: docker.elastic.co/elasticsearch/elasticsearch:6.7.0
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

we started more instances of es

the github action, is it power enough to support this? I am little worried

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

i removed two of these containers(not needed)

Copy link
Member

@membphis membphis left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do we need to remove type field in this PR?

@Revolyssup
Copy link
Contributor Author

Do we need to remove type field in this PR?

Yes. If we don't remove type field then we will need the compat headers that I removed

@membphis
Copy link
Member

Do we need to remove type field in this PR?

Yes. If we don't remove type field then we will need the compat headers that I removed

pls update the doc too

@Revolyssup
Copy link
Contributor Author

Do we need to remove type field in this PR?

Yes. If we don't remove type field then we will need the compat headers that I removed

pls update the doc too

Removed type from docs

membphis
membphis previously approved these changes Jun 26, 2025
Copy link
Member

@membphis membphis left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

nic-6443
nic-6443 previously approved these changes Jun 26, 2025
@Revolyssup Revolyssup dismissed stale reviews from nic-6443 and membphis via 2f7421d June 26, 2025 07:45
@nic-6443 nic-6443 requested review from ronething and AlinsRan June 26, 2025 11:10
@Revolyssup Revolyssup merged commit 78e8fc0 into apache:master Jun 26, 2025
27 checks passed
@Revolyssup Revolyssup deleted the revolyssup/es9 branch June 26, 2025 12:05
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working enhancement New feature or request plugin size:L This PR changes 100-499 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants