Skip to content

out_http: TLS1.3 support #4859

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

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

Athishpranav2003
Copy link
Contributor

@Athishpranav2003 Athishpranav2003 commented Mar 8, 2025

Which issue(s) this PR fixes:
Fixes #4332

What this PR does / why we need it:
Changes the way we configure Net::HTTP client.

Docs Changes:
fluent/fluentd-docs-gitbook#579

Release Note:
The same as the title.

Copy link

github-actions bot commented Apr 9, 2025

This PR has been automatically marked as stale because it has been open 30 days with no activity. Remove stale label or comment or this PR will be closed in 7 days

@github-actions github-actions bot added the stale label Apr 9, 2025
@Watson1978 Watson1978 removed the stale label Apr 10, 2025
@daipom
Copy link
Contributor

daipom commented Apr 10, 2025

@Athishpranav2003 This PR is in draft status. Please let me know if there are any TODO.

@Athishpranav2003
Copy link
Contributor Author

Hey @daipom

I guess the pr is left to be tested but I didn't get time to test. Maybe I can check it next week but if someone else can do it as well it will be really helpful. Setting up the openssl thing I didn't try it

@daipom
Copy link
Contributor

daipom commented Apr 10, 2025

OK! Thanks!

Copy link

This PR has been automatically marked as stale because it has been open 30 days with no activity. Remove stale label or comment or this PR will be closed in 7 days

@github-actions github-actions bot added the stale label May 10, 2025
@ashie ashie added enhancement Feature request or improve operations and removed stale labels May 11, 2025
@Athishpranav2003
Copy link
Contributor Author

Hey guys

i am back, was busy with college. Will look at this pr this week

@Athishpranav2003
Copy link
Contributor Author

Athishpranav2003 commented May 17, 2025

Testing output

2025-05-17 17:23:47 +0530 [info]: fluent/log.rb:362:: using configuration file: <ROOT>
  <source>
    @type sample
    @label @mainstream
    sample {"hello":"world","foo":"bar"}
    tag "sample"
  </source>
  <label @mainstream>
    <match **>
      @type http
      endpoint "https://0.0.0.0:8000"
      tls_verify_mode none
      tls_version TLSv1_3
      json_array true
      <format>
        @type "json"
      </format>
      <buffer>
        flush_interval 2s
      </buffer>
    </match>
  </label>
</ROOT>
2025-05-17 17:23:47 +0530 [info]: fluent/log.rb:362:: starting fluentd-1.18.0 pid=41729 ruby="3.4.2"
2025-05-17 17:23:47 +0530 [info]: fluent/log.rb:362:: spawn command to main:  cmdline=["/usr/bin/ruby", "-Eascii-8bit:ascii-8bit", "/usr/local/sbin/fluentd", "-c", "fluentd.conf", "-v", "--under-supervisor"]
2025-05-17 17:23:48 +0530 [info]: #0 fluent/log.rb:362:: fluentd worker is now running worker=0
2025-05-17 17:23:48 +0530 [debug]: #0 fluent/log.rb:341:: flush_thread actually running
2025-05-17 17:23:48 +0530 [debug]: #0 fluent/log.rb:341:: enqueue_thread actually running
2025-05-17 17:23:52 +0530 [debug]: #0 fluent/log.rb:341:: Post data to https://0.0.0.0:8000 with chunk(635538d474b42c0c20451f2461610d3e)

@daipom
image

@Athishpranav2003 Athishpranav2003 force-pushed the tls branch 2 times, most recently from 71cb2aa to 8acba89 Compare May 17, 2025 12:00
@Athishpranav2003 Athishpranav2003 marked this pull request as ready for review May 17, 2025 12:26
@Athishpranav2003 Athishpranav2003 requested a review from kenhys May 17, 2025 12:26
@daipom
Copy link
Contributor

daipom commented May 19, 2025

Thanks! We will see this soon!

@Watson1978 Watson1978 self-requested a review May 20, 2025 01:14
@Watson1978
Copy link
Contributor

If you rebase your commits with master, It will pass tests on Ruby 3.2 / 3.3 Windows.

Signed-off-by: Athishpranav2003 <[email protected]>
Signed-off-by: Athishpranav2003 <[email protected]>
Copy link
Contributor

@Watson1978 Watson1978 left a comment

Choose a reason for hiding this comment

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

👍🏻

@daipom
Copy link
Contributor

daipom commented May 21, 2025

@Athishpranav2003 Thanks so much for this improvement!

The following would be necessary for us.

@Athishpranav2003
Could you do these if you have time? Especially the addition of tests.
This PR can be merged with TODO to update the document.

@daipom daipom changed the title TLS1.3 support for out_http out_http: TLS1.3 support May 21, 2025
@Athishpranav2003
Copy link
Contributor Author

This PR can be merged with TODO to update the document.

Sure

will do it around this weekend or next week

@Athishpranav2003
Copy link
Contributor Author

@daipom will default tls version stay as tls1.2?

@daipom
Copy link
Contributor

daipom commented May 22, 2025

@Athishpranav2003

will do it around this weekend or next week

Thanks!!

will default tls version stay as tls1.2?

Yes!
It should be Fluentd::TLS::DEFAULT_VERSION:

DEFAULT_VERSION = :'TLSv1_2'

If we change this, it should be done in a separate PR.
Let's keep the default value as is for now.

@Athishpranav2003
Copy link
Contributor Author

and @daipom what kinda tests to add to test_tls.rb?

@daipom
Copy link
Contributor

daipom commented May 22, 2025

and @daipom what kinda tests to add to test_tls.rb?

We want some tests for Fluent::TLS.set_version_to_options there.
Every feature of Fluent::TLS should be tested in some way here.

We already have tests for Fluent::TLS.set_version_to_context here.
Looks like they are very simple tests.
They don't look like enough, but it would be fine to check something like these existing tests.

@daipom daipom added this to the v1.19.0 milestone May 22, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement Feature request or improve operations
Projects
None yet
Development

Successfully merging this pull request may close these issues.

TLS 1.3 support in http output plugin
5 participants