Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Add IP Location APIs #3107

Open
wants to merge 13 commits into
base: main
Choose a base branch
from
Open

Add IP Location APIs #3107

wants to merge 13 commits into from

Conversation

pquentin
Copy link
Member

@pquentin pquentin commented Nov 8, 2024

I extracted the rest-api-spec JSON files from #3019. We'll need a review from the Data Management team before merging.

@pquentin pquentin requested a review from a team as a code owner November 8, 2024 09:49
@github-actions GitHub Actions

This comment was marked as outdated.

Sign in to view
joegallo

This comment was marked as resolved.

Sign in to view

@github-actions GitHub Actions

This comment was marked as outdated.

Sign in to view
@joegallo
Copy link

joegallo commented Nov 8, 2024

I think we'll still need to make a change to Database.ts -- I'll reach out to you offline.

@pquentin pquentin mentioned this pull request Nov 12, 2024
Merged
@lcawl
Copy link
Contributor

lcawl commented Nov 13, 2024

Thanks for creating this PR! I was looking at how to ensure the work from elastic/elasticsearch#116611 got carried over to the new API docs site and this PR already does a lot of the heavy lifting (we can port the examples over in a subsequent PR).

This was referenced Nov 15, 2024
Merged
Merged
@github-actions GitHub Actions

This comment was marked as outdated.

Sign in to view
@github-actions GitHub Actions

This comment was marked as outdated.

Sign in to view
@github-actions GitHub Actions

This comment was marked as outdated.

Sign in to view
@github-actions GitHub Actions

This comment was marked as outdated.

Sign in to view
pquentin
pquentin commented Nov 19, 2024
View reviewed changes
specification/ingest/_types/Database.ts Outdated Show resolved Hide resolved
specification/ingest/_types/Database.ts
/** The configuration necessary to identify which IP geolocation provider to use to download the database, as well as any provider-specific configuration necessary for such downloading.
* At present, the only supported provider is maxmind, and the maxmind provider requires that an account_id (string) is configured.
*/
maxmind: Maxmind
Copy link
Member Author

Choose a reason for hiding this comment

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

We do need the maxmind key which is gone with this approach.

Also it's OK to have some duplication, eg I'm happy to keep the previous approach but simply add an ipinfo field and type, with one less field.

Copy link

Choose a reason for hiding this comment

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

Agreed, yeah, I'll take a swing at fixing that -- it wasn't an intentional change in this second version of things, I just wasn't thoughtful enough about how I did it.

Copy link

Choose a reason for hiding this comment

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

What do you think about 31fd86a?

Copy link
Member Author

Choose a reason for hiding this comment

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

This looks great, thanks! Can you please confirm that the GeoIP APIs also accept ipinfo databases? I'm asking as DatabaseConfiguration is currently shared between the two APIs.

@github-actions GitHub Actions
Copy link
Contributor

Following you can find the validation results for the APIs you have changed.

API Status Request Response
ingest.delete_geoip_database 🟢 1/1 1/1
ingest.delete_ip_location_database 🟢 1/1 1/1
ingest.delete_pipeline 🟢 15/15 15/15
ingest.geo_ip_stats 🟢 1/1 1/1
ingest.get_geoip_database 🟢 6/6 6/6
ingest.get_ip_location_database 🟢 7/7 7/7
ingest.get_pipeline 🟢 22/22 22/22
ingest.processor_grok 🟢 1/1 1/1
ingest.put_geoip_database 🟢 3/3 3/3
ingest.put_ip_location_database 🔴 3/4 4/4
ingest.put_pipeline 🟢 60/60 60/60
ingest.simulate 🟢 10/10 10/10

You can validate these APIs yourself by using the make validate target.

@github-actions GitHub Actions
Copy link
Contributor

Following you can find the validation results for the APIs you have changed.

API Status Request Response
ingest.delete_geoip_database 🟢 1/1 1/1
ingest.delete_ip_location_database 🟢 1/1 1/1
ingest.delete_pipeline 🟢 15/15 15/15
ingest.geo_ip_stats 🟢 1/1 1/1
ingest.get_geoip_database 🟢 6/6 6/6
ingest.get_ip_location_database 🟢 7/7 7/7
ingest.get_pipeline 🟢 22/22 22/22
ingest.processor_grok 🟢 1/1 1/1
ingest.put_geoip_database 🟢 3/3 3/3
ingest.put_ip_location_database 🔴 3/4 4/4
ingest.put_pipeline 🟢 60/60 60/60
ingest.simulate 🟢 10/10 10/10

You can validate these APIs yourself by using the make validate target.

pquentin
pquentin commented Nov 20, 2024
View reviewed changes
specification/ingest/put_ip_location_database/PutIpLocationDatabaseRequest.ts Show resolved Hide resolved
specification/ingest/_types/Database.ts
/** The configuration necessary to identify which IP geolocation provider to use to download the database, as well as any provider-specific configuration necessary for such downloading.
* At present, the only supported provider is maxmind, and the maxmind provider requires that an account_id (string) is configured.
*/
maxmind: Maxmind
Copy link
Member Author

Choose a reason for hiding this comment

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

This looks great, thanks! Can you please confirm that the GeoIP APIs also accept ipinfo databases? I'm asking as DatabaseConfiguration is currently shared between the two APIs.

@github-actions GitHub Actions
Copy link
Contributor

Following you can find the validation results for the APIs you have changed.

API Status Request Response
ingest.delete_geoip_database 🟢 1/1 1/1
ingest.delete_ip_location_database 🟢 1/1 1/1
ingest.delete_pipeline 🟢 15/15 15/15
ingest.geo_ip_stats 🟢 1/1 1/1
ingest.get_geoip_database 🟢 6/6 6/6
ingest.get_ip_location_database 🟢 7/7 7/7
ingest.get_pipeline 🟢 22/22 22/22
ingest.processor_grok 🟢 1/1 1/1
ingest.put_geoip_database 🟢 3/3 3/3
ingest.put_ip_location_database 🔴 3/4 4/4
ingest.put_pipeline 🟢 60/60 60/60
ingest.simulate 🟢 10/10 10/10

You can validate these APIs yourself by using the make validate target.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@joegallo joegallo joegallo requested changes

Requested changes must be addressed to merge this pull request.

Assignees
No one assigned
Labels
backport 8.x backport 8.16 specification
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@pquentin @joegallo @lcawl