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.

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

Add x-elastic-product-origin header to serverless testing in 8.x and 9.0 #16736

Closed
robbavey opened this issue Dec 2, 2024 · 7 comments
Closed
Assignees

Comments

@robbavey
Copy link
Member

robbavey commented Dec 2, 2024

Add Header to uses of the Kibana API endpoint api/logstash/pipeline/*

@robbavey robbavey closed this as completed Dec 2, 2024
@robbavey robbavey reopened this Dec 2, 2024
donoghuc added a commit to donoghuc/logstash that referenced this issue Dec 6, 2024
This commit contains an example for a clarifying question on:
elastic#16736

TODO: after confirming this is the approach, clean up commit msg
@donoghuc
Copy link
Member

donoghuc commented Dec 6, 2024

I may be missing something here... I put up a draft of how I interpreted this task #16766 but i realize i may be way off.

@donoghuc donoghuc self-assigned this Dec 6, 2024
@kaisecheng
Copy link
Contributor

The header should be capital letter X, X-elastic-product-origin

@donoghuc
Copy link
Member

donoghuc commented Dec 11, 2024

Thanks for looking. All the tickets related to this work on the LS side and corresponding work so far have had the all lower case version. I did a search in the org and see both https://github.com/search?q=org%3Aelastic+x-elastic-product-origin&type=code&p=3 Header fields are case insensitive https://www.rfc-editor.org/rfc/rfc9110.html#fields but I agree consistency is probably nice. Is there a technical reason we would want to try to update everything with the capitalized first letter?

The main question I have on this issue is that all the other tasks have been pretty clear in terms of ensuring a header is included for requests to an internal product. This has mostly consisted of finding http clients in the source code and ensuring that if it s communicating with an internal end point that it is including a header. This ticket references

the Kibana API endpoint api/logstash/pipeline/*
Which the only place in the LS source code i see using that endpoint is a shell script that appearst to be used in testing. I raised #16766 showing a change that would update those tests to send along the headers.

So my two explicit questions now are:

  1. Do we need to replace x-elastic-product-origin with X-elastic-product-origin everywhere?
  2. Is the intent of this ticket to send along the header in this script used in testing Update serverless tests to include product origin header #16766 or is there something else i'm missing?

References:
The original upstream issue https://github.com/elastic/dev/issues/2819
Issue capturing work done so far (with all lowercase versions) https://github.com/elastic/ingest-dev/issues/4279

@kaisecheng
Copy link
Contributor

@donoghuc thanks for double checking the header. I wasn't aware of lowercase x is used in our code. No need to make existing to capital letter.

This ticket is about updating the current serverless tests with header. The tests locate here. You can follow readme to setup environment. The buildkite job that run the tests daily

@robbavey
Copy link
Member Author

  1. Do we need to replace x-elastic-product-origin with X-elastic-product-origin everywhere?

We don't need to change the header to X-elastic-product-origin - x-elastic-product-origin is used in the relevant Kibana code base, and we use both, plus X-Elastic-Product-Origin in various places in the Elastic code base.

  1. Is the intent of this ticket to send along the header in this script used in testing Update serverless tests to include product origin header #16766 or is there something else i'm missing?

Yes

@donoghuc
Copy link
Member

Thank you both for the clarification! I cleaned up the commit and moved #16766 out of draft mode in to ready for review.

@donoghuc
Copy link
Member

main: #16766
8.x: #16797

Both merged!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants