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

[Fleet] Logstash Output - being compliant to RFC-952 #176298

Merged
merged 5 commits into from
Feb 6, 2024

Conversation

Terilia
Copy link
Member

@Terilia Terilia commented Feb 6, 2024

Summary

The logstash output in fleet was not following the RFC-952 correctly, only accepting lowercase url's for the hostname, even though according to the standard, this should be irrelevant.

Additionally added two new unit tests, to check that this behaviour is compliant in the future as well.

Fix explanation

The issue was in the past that the URL Type in nodejs is automatically lowercase, and then the value of the form is or the string provided is checked against this url type. The types behaviour is explained here: https://nodejs.org/api/url.html#the-whatwg-url-api

As we would like to be case insensitive, in the check, the value is also lowerecased with: val.toLowerCase() to pass the check

Checklist

Delete any items that are not applicable to this PR.

Adjusted form of the hostname, to be accepted if non lowercase.
@Terilia Terilia requested a review from a team as a code owner February 6, 2024 14:26
@botelastic botelastic bot added the Team:Fleet Team label for Observability Data Collection Fleet team label Feb 6, 2024
@elasticmachine
Copy link
Contributor

Pinging @elastic/fleet (Team:Fleet)

@juliaElastic juliaElastic added release_note:fix backport:prev-minor Backport to (8.x) the previous minor version (i.e. one version back from main) labels Feb 6, 2024
@@ -34,6 +34,12 @@ describe('Output form validation', () => {
expect(res).toBeUndefined();
});

it('should work with hostnames using uppercase letters', () => {
const res = validateKafkaHosts(['tEsT.fr:9200', 'TEST2.fr:9200', 'teSt.fR:9999']);
Copy link
Contributor

Choose a reason for hiding this comment

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

did you mean to test validateLogstashHosts, since the change is there?

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes, yes I did thank you :D

Copy link
Contributor

@juliaElastic juliaElastic left a comment

Choose a reason for hiding this comment

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

LGTM, thanks for the fix! 🚀

@Terilia Terilia enabled auto-merge (squash) February 6, 2024 14:49
@kibana-ci
Copy link
Collaborator

💚 Build Succeeded

Metrics [docs]

Async chunks

Total size of all lazy-loaded chunks that will be downloaded as the user navigates the app

id before after diff
fleet 1.2MB 1.2MB +14.0B

To update your PR or re-run it, just comment with:
@elasticmachine merge upstream

@Terilia Terilia merged commit 5542917 into elastic:main Feb 6, 2024
20 checks passed
kibanamachine pushed a commit to kibanamachine/kibana that referenced this pull request Feb 6, 2024
## Summary

The logstash output in fleet was not following the
[RFC-952](https://www.rfc-editor.org/rfc/rfc952) correctly, only
accepting lowercase url's for the hostname, even though according to the
standard, this should be irrelevant.

Additionally added two new unit tests, to check that this behaviour is
compliant in the future as well.

## Fix explanation

The issue was in the past that the URL Type in nodejs is automatically
lowercase, and then the value of the form is or the string provided is
checked against this url type. The types behaviour is explained here:
https://nodejs.org/api/url.html#the-whatwg-url-api

As we would like to be case insensitive, in the check, the value is also
lowerecased with: val.toLowerCase() to pass the check

### Checklist

Delete any items that are not applicable to this PR.

- [X] [Unit or functional
tests](https://www.elastic.co/guide/en/kibana/master/development-tests.html)
were updated or added to match the most common scenarios

(cherry picked from commit 5542917)
@kibanamachine
Copy link
Contributor

💚 All backports created successfully

Status Branch Result
8.12

Note: Successful backport PRs will be merged automatically after passing CI.

Questions ?

Please refer to the Backport tool documentation

kibanamachine added a commit that referenced this pull request Feb 6, 2024
…#176323)

# Backport

This will backport the following commits from `main` to `8.12`:
- [[Fleet] Logstash Output - being compliant to RFC-952
(#176298)](#176298)

<!--- Backport version: 9.4.3 -->

### Questions ?
Please refer to the [Backport tool
documentation](https://github.com/sqren/backport)

<!--BACKPORT [{"author":{"name":"Alex
S","email":"Terilia@users.noreply.github.com"},"sourceCommit":{"committedDate":"2024-02-06T16:33:56Z","message":"[Fleet]
Logstash Output - being compliant to RFC-952 (#176298)\n\n##
Summary\r\n\r\nThe logstash output in fleet was not following
the\r\n[RFC-952](https://www.rfc-editor.org/rfc/rfc952) correctly,
only\r\naccepting lowercase url's for the hostname, even though
according to the\r\nstandard, this should be
irrelevant.\r\n\r\nAdditionally added two new unit tests, to check that
this behaviour is\r\ncompliant in the future as well.\r\n\r\n\r\n## Fix
explanation\r\n\r\nThe issue was in the past that the URL Type in nodejs
is automatically\r\nlowercase, and then the value of the form is or the
string provided is\r\nchecked against this url type. The types behaviour
is explained
here:\r\nhttps://nodejs.org/api/url.html#the-whatwg-url-api\r\n\r\nAs we
would like to be case insensitive, in the check, the value is
also\r\nlowerecased with: val.toLowerCase() to pass the check\r\n\r\n###
Checklist\r\n\r\nDelete any items that are not applicable to this
PR.\r\n\r\n- [X] [Unit or
functional\r\ntests](https://www.elastic.co/guide/en/kibana/master/development-tests.html)\r\nwere
updated or added to match the most common
scenarios","sha":"55429179ba30ca3a922f6daf44ce17f8d5f19507","branchLabelMapping":{"^v8.13.0$":"main","^v(\\d+).(\\d+).\\d+$":"$1.$2"}},"sourcePullRequest":{"labels":["release_note:fix","Team:Fleet","backport:prev-minor","v8.13.0"],"title":"[Fleet]
Logstash Output - being compliant to
RFC-952","number":176298,"url":"https://github.com/elastic/kibana/pull/176298","mergeCommit":{"message":"[Fleet]
Logstash Output - being compliant to RFC-952 (#176298)\n\n##
Summary\r\n\r\nThe logstash output in fleet was not following
the\r\n[RFC-952](https://www.rfc-editor.org/rfc/rfc952) correctly,
only\r\naccepting lowercase url's for the hostname, even though
according to the\r\nstandard, this should be
irrelevant.\r\n\r\nAdditionally added two new unit tests, to check that
this behaviour is\r\ncompliant in the future as well.\r\n\r\n\r\n## Fix
explanation\r\n\r\nThe issue was in the past that the URL Type in nodejs
is automatically\r\nlowercase, and then the value of the form is or the
string provided is\r\nchecked against this url type. The types behaviour
is explained
here:\r\nhttps://nodejs.org/api/url.html#the-whatwg-url-api\r\n\r\nAs we
would like to be case insensitive, in the check, the value is
also\r\nlowerecased with: val.toLowerCase() to pass the check\r\n\r\n###
Checklist\r\n\r\nDelete any items that are not applicable to this
PR.\r\n\r\n- [X] [Unit or
functional\r\ntests](https://www.elastic.co/guide/en/kibana/master/development-tests.html)\r\nwere
updated or added to match the most common
scenarios","sha":"55429179ba30ca3a922f6daf44ce17f8d5f19507"}},"sourceBranch":"main","suggestedTargetBranches":[],"targetPullRequestStates":[{"branch":"main","label":"v8.13.0","branchLabelMappingKey":"^v8.13.0$","isSourceBranch":true,"state":"MERGED","url":"https://github.com/elastic/kibana/pull/176298","number":176298,"mergeCommit":{"message":"[Fleet]
Logstash Output - being compliant to RFC-952 (#176298)\n\n##
Summary\r\n\r\nThe logstash output in fleet was not following
the\r\n[RFC-952](https://www.rfc-editor.org/rfc/rfc952) correctly,
only\r\naccepting lowercase url's for the hostname, even though
according to the\r\nstandard, this should be
irrelevant.\r\n\r\nAdditionally added two new unit tests, to check that
this behaviour is\r\ncompliant in the future as well.\r\n\r\n\r\n## Fix
explanation\r\n\r\nThe issue was in the past that the URL Type in nodejs
is automatically\r\nlowercase, and then the value of the form is or the
string provided is\r\nchecked against this url type. The types behaviour
is explained
here:\r\nhttps://nodejs.org/api/url.html#the-whatwg-url-api\r\n\r\nAs we
would like to be case insensitive, in the check, the value is
also\r\nlowerecased with: val.toLowerCase() to pass the check\r\n\r\n###
Checklist\r\n\r\nDelete any items that are not applicable to this
PR.\r\n\r\n- [X] [Unit or
functional\r\ntests](https://www.elastic.co/guide/en/kibana/master/development-tests.html)\r\nwere
updated or added to match the most common
scenarios","sha":"55429179ba30ca3a922f6daf44ce17f8d5f19507"}}]}]
BACKPORT-->

Co-authored-by: Alex S <Terilia@users.noreply.github.com>
@mistic
Copy link
Member

mistic commented Feb 7, 2024

This PR didn't make it on time to the latest build candidate of v8.12.1. Updating the labels.

@mistic mistic added v8.12.2 and removed v8.12.1 labels Feb 7, 2024
jloleysens added a commit that referenced this pull request Feb 7, 2024
* main: (224 commits)
  [Http] Replace `buildNr` with `buildSha` in static asset paths (#175898)
  [Ops] Fix GCS bucket access for future buildkite agents (#174756)
  [api-docs] 2024-02-07 Daily api_docs build (#176362)
  skip flaky suite (#176002)
  skip failing es promotion suite (#176359)
  [Cloud Security] [Grouping] Add URL Params support to the grouping components (#175749)
  chore(NA): update versions after v8.12.2 bump (#176309)
  chore(NA): update versions after v7.17.19 bump (#176313)
  skip failing test suite (#176352)
  [SLO] Enable burn rate alert by default during creation via UI (#176317)
  [Fleet] Add the uptime capability to observability projects (#176285)
  [Security Solution][Endpoint] Fix Manifest Manger so that it works with large (>10k) (#174411)
  [ResponseOps] Alert creation delay based on user definition (#175851)
  [data views] Default field formatters based on field meta values (#174973)
  [Cloud Security]Detection Rules counter on Rules Flyout (#176041)
  [Security Solution] Data Quality Dashboard persistence (#175673)
  [Ent Search] Connector client copy cleanup (#176290)
  [ML] Anomaly Detection: Adds actions menu to anomaly markers in Single Metric Viewer chart. (#175556)
  [ML] Anomaly Detection: Fix `values-dots` colors (#176303)
  [Fleet] Logstash Output - being compliant to RFC-952 (#176298)
  ...
fkanout pushed a commit to fkanout/kibana that referenced this pull request Feb 7, 2024
## Summary

The logstash output in fleet was not following the
[RFC-952](https://www.rfc-editor.org/rfc/rfc952) correctly, only
accepting lowercase url's for the hostname, even though according to the
standard, this should be irrelevant.

Additionally added two new unit tests, to check that this behaviour is
compliant in the future as well.


## Fix explanation

The issue was in the past that the URL Type in nodejs is automatically
lowercase, and then the value of the form is or the string provided is
checked against this url type. The types behaviour is explained here:
https://nodejs.org/api/url.html#the-whatwg-url-api

As we would like to be case insensitive, in the check, the value is also
lowerecased with: val.toLowerCase() to pass the check

### Checklist

Delete any items that are not applicable to this PR.

- [X] [Unit or functional
tests](https://www.elastic.co/guide/en/kibana/master/development-tests.html)
were updated or added to match the most common scenarios
CoenWarmer pushed a commit to CoenWarmer/kibana that referenced this pull request Feb 15, 2024
## Summary

The logstash output in fleet was not following the
[RFC-952](https://www.rfc-editor.org/rfc/rfc952) correctly, only
accepting lowercase url's for the hostname, even though according to the
standard, this should be irrelevant.

Additionally added two new unit tests, to check that this behaviour is
compliant in the future as well.


## Fix explanation

The issue was in the past that the URL Type in nodejs is automatically
lowercase, and then the value of the form is or the string provided is
checked against this url type. The types behaviour is explained here:
https://nodejs.org/api/url.html#the-whatwg-url-api

As we would like to be case insensitive, in the check, the value is also
lowerecased with: val.toLowerCase() to pass the check

### Checklist

Delete any items that are not applicable to this PR.

- [X] [Unit or functional
tests](https://www.elastic.co/guide/en/kibana/master/development-tests.html)
were updated or added to match the most common scenarios
CoenWarmer pushed a commit to CoenWarmer/kibana that referenced this pull request Feb 15, 2024
## Summary

The logstash output in fleet was not following the
[RFC-952](https://www.rfc-editor.org/rfc/rfc952) correctly, only
accepting lowercase url's for the hostname, even though according to the
standard, this should be irrelevant.

Additionally added two new unit tests, to check that this behaviour is
compliant in the future as well.


## Fix explanation

The issue was in the past that the URL Type in nodejs is automatically
lowercase, and then the value of the form is or the string provided is
checked against this url type. The types behaviour is explained here:
https://nodejs.org/api/url.html#the-whatwg-url-api

As we would like to be case insensitive, in the check, the value is also
lowerecased with: val.toLowerCase() to pass the check

### Checklist

Delete any items that are not applicable to this PR.

- [X] [Unit or functional
tests](https://www.elastic.co/guide/en/kibana/master/development-tests.html)
were updated or added to match the most common scenarios
fkanout pushed a commit to fkanout/kibana that referenced this pull request Mar 4, 2024
## Summary

The logstash output in fleet was not following the
[RFC-952](https://www.rfc-editor.org/rfc/rfc952) correctly, only
accepting lowercase url's for the hostname, even though according to the
standard, this should be irrelevant.

Additionally added two new unit tests, to check that this behaviour is
compliant in the future as well.


## Fix explanation

The issue was in the past that the URL Type in nodejs is automatically
lowercase, and then the value of the form is or the string provided is
checked against this url type. The types behaviour is explained here:
https://nodejs.org/api/url.html#the-whatwg-url-api

As we would like to be case insensitive, in the check, the value is also
lowerecased with: val.toLowerCase() to pass the check

### Checklist

Delete any items that are not applicable to this PR.

- [X] [Unit or functional
tests](https://www.elastic.co/guide/en/kibana/master/development-tests.html)
were updated or added to match the most common scenarios
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backport:prev-minor Backport to (8.x) the previous minor version (i.e. one version back from main) release_note:fix Team:Fleet Team label for Observability Data Collection Fleet team v8.12.2 v8.13.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants