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

Document Logstash connection to external Elasticsearch #6895

Merged
merged 7 commits into from
Jun 22, 2023
Merged

Document Logstash connection to external Elasticsearch #6895

merged 7 commits into from
Jun 22, 2023

Conversation

kaisecheng
Copy link
Contributor

@kaisecheng kaisecheng commented Jun 11, 2023 •
edited by thbkrkr
Loading

This PR adds doc for connecting external Elasticsearch.

kaisecheng added 2 commits June 8, 2023 23:14
@kaisecheng
Logstash add port to Elasticsearch URL
802df0b 
if the URL is HTTPS and does not specify the port
@kaisecheng
add doc
181d31f
@botelastic botelastic bot added the triage label Jun 11, 2023
@kaisecheng kaisecheng marked this pull request as ready for review June 12, 2023 16:21
@kaisecheng kaisecheng requested a review from robbavey June 12, 2023 16:21
@barkbay barkbay added the >bug Something isn't working label Jun 13, 2023
@botelastic botelastic bot removed the triage label Jun 13, 2023
@botelastic botelastic bot removed the triage label Jun 13, 2023
robbavey
robbavey reviewed Jun 15, 2023
View reviewed changes
Copy link
Member

@robbavey robbavey left a comment

Choose a reason for hiding this comment

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

LGTM - I have a documentation suggestion, and a nit on test naming

pkg/controller/logstash/env_test.go Outdated Show resolved Hide resolved
docs/orchestrating-elastic-stack-applications/logstash.asciidoc Outdated
metadata:
name: external-es-ref
stringData:
url: https://abcd-42.xyz.elastic-cloud.com <1>
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 add a note somewhere that we add the default port of 443 to https URLs, as this is different behavior to standard logstash plugins, and different to how non-cloud Elasticsearch works out of the box

pebrc
pebrc reviewed Jun 16, 2023
View reviewed changes
Copy link
Collaborator

@pebrc pebrc left a comment

Choose a reason for hiding this comment

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

I think I am slightly -1 on the port appending. As explained in my comment it does fix the problem only for Logstash and not for any of the other stack applications e.g. the Beats used for monitoring. I think I would favour calling out explicitly in the doc that the default port expected is 9200 and any other port has to be configured explicitly via the url.

docs/orchestrating-elastic-stack-applications/logstash.asciidoc
monitoring:
metrics:
elasticsearchRefs:
- secretName: external-es-ref <5>
Copy link
Collaborator

Choose a reason for hiding this comment

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

Assuming a user specifies a URL without explicit port in the secret, then the changes in this PR append 443 by default for the environment. However Beats also assume 9200 by default and while the Logstash side is fixed, the monitoring will still not work until the user fixes the url field in the secret to include a port.

kaisecheng and others added 2 commits June 20, 2023 11:49
@kaisecheng @robbavey
Update pkg/controller/logstash/env_test.go
4e9d013 
Co-authored-by: Rob Bavey <rob.bavey@elastic.co>
@kaisecheng
add note to port
5c18ce6
@kaisecheng
Copy link
Contributor Author

Kibana assumes 443. Beats and Logstah assume 9200. Not sure what the default port is for others. The stack monitoring is not consistent.
+1 for calling out explicitly in doc. Instead of explaining the assumption of default port, having a note to ask for always add the port in URL may be simpler.

@kaisecheng
Revert "Logstash add port to Elasticsearch URL"
ed8064a 
This reverts commit 802df0b.
This reverts commit 47bd790.
This reverts commit 4e9d013.
@kaisecheng
Copy link
Contributor Author

@robbavey @pebrc I have removed the port appending. Now the PR only updates the doc. It is ready for review.

@kaisecheng kaisecheng requested review from pebrc and robbavey June 20, 2023 18:43
robbavey
robbavey approved these changes Jun 20, 2023
View reviewed changes
Copy link
Member

@robbavey robbavey left a comment

Choose a reason for hiding this comment

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

LGTM

@kaisecheng @pebrc
Update docs/orchestrating-elastic-stack-applications/logstash.asciidoc
30de0e2 
Co-authored-by: Peter Brachwitz <peter.brachwitz@gmail.com>
@kaisecheng kaisecheng added >docs Documentation and removed >bug Something isn't working labels Jun 20, 2023
@pebrc
Copy link
Collaborator

pebrc commented Jun 22, 2023

@kaisecheng should I merge this?

@kaisecheng
Copy link
Contributor Author

@pebrc Yes please!

@pebrc pebrc merged commit 5422e73 into elastic:main Jun 22, 2023
@pebrc pebrc changed the title Logstash connect to external Elasticsearch Document Logstash connection to external Elasticsearch Jul 10, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@robbavey robbavey robbavey approved these changes

@pebrc pebrc pebrc approved these changes

Assignees
No one assigned
Labels
>docs Documentation :logstash v2.9.0
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@kaisecheng @pebrc @robbavey @thbkrkr @barkbay