feat: TLS config option availability using the client options

[ssi-anik]

Description

This PR allows developers to provide the TLS config during the client creation via the client options.

Issues

• extends: Make SSL Certificate Verification Optional #2

[aldy505]

Curious, wouldn't it possible to set the *tls.Config from ClientOptions.HTTPClient?

[ssi-anik]

@aldy505 yes, it could be done. But setting the value using that HttpClient will require more lines. Also, allowing the package to choose the defaults rather than having the task done by the developer.

[giortzisg]

I'm a bit doubtful with the actual usefulness of this and don't really want to populate the clientOptions with so many conflicting options. Currently TlsConfig completely overrides CaCerts, which might be confusing to users, so even if we want to add the option we need to:

• either merge those two
• deprecate CaCerts and only use TlsConfig

In general the plan at some point is to deprecate the current transport and use the new one, when it would make sense to simplify the httpConfig to actually make sense and make all the breaking changes then.

[ssi-anik]

@giortzisg Thanks for your concern.

Firstly, in my case, the use case of TlsConfig is that I can provide the value of InsecureSkipVerify with minimal effort. There are scenarios where fixing the SSL of the domain might take longer than expected. Without merging this change, someone will have to use HTTPTransport. Rather, I want to let the module decide the instantiation of HTTPTransport during the init process.

As the CaCerts config is a part of the TlsConfig, after merging this PR, the existing implementation will continue to work. As TlsConfig's default is nil, it will fall back to the CaCerts section and will use that value. Otherwise, whoever needs to set the TlsConfig, just like I need, will set the value, and TlsConfig will take precedence over the CaCerts.

In general the plan at some point is to deprecate the current transport and use the new one, when it would make sense to simplify the httpConfig to actually make sense and make all the breaking changes then.

Yes, when trying to include breaking changes, you might do things like that. But IMO, it was the least change someone has to add to get their application running as soon as they figure it out, just like I did.

[giortzisg]

TlsConfig will take precedence over the CaCerts.

Let's at least try and merge the two in case that TlsConfig exists without any CaCerts

[ssi-anik]

Let's at least try and merge the two in case that TlsConfig exists without any CaCerts

Did you mean to say that if the developer provides both the TlsConfig and CaCerts, then CaCerts will be injected into the TlsConfig even if the developer intentionally sets any value in CaCerts inside the TlsConfig? Did I get your point right?

However, my thought was that if the developer is adding TlsConfig, they already know that they can skip the value of CaCerts in the root config, and add it directly to the TlsConfig. For me, it made more sense.

[giortzisg]

then CaCerts will be injected into the TlsConfig

if TlsConfig already specifies CaCerts then the other option gets ignored, but if TlsConfig doesn't and CaCerts does, we need to merge them. So the precedence would go from:
Client -> Transport -> TlsConfig -> CaCerts

[ssi-anik]

then CaCerts will be injected into the TlsConfig

if TlsConfig already specifies CaCerts then the other option gets ignored, but if TlsConfig doesn't and CaCerts does, we need to merge them. So the precedence would go from: Client -> Transport -> TlsConfig -> CaCerts

Added those changes mention in your comment.

[cursor]

Cursor Bugbot has reviewed your changes and found 1 potential issue.

^{Bugbot Autofix is OFF. To automatically fix reported issues with Cloud Agents, enable Autofix in the Cursor dashboard.}

[codecov]

Codecov Report

❌ Patch coverage is 27.27273% with 8 lines in your changes missing coverage. Please review.
✅ Project coverage is 86.05%. Comparing base (1ce3436) to head (a2fa698).
⚠️ Report is 17 commits behind head on master.

Files with missing lines	Patch %	Lines
util.go	27.27%	8 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##           master    #1168      +/-   ##
==========================================
+ Coverage   86.04%   86.05%   +0.01%     
==========================================
  Files          62       63       +1     
  Lines        6090     6169      +79     
==========================================
+ Hits         5240     5309      +69     
- Misses        635      644       +9     
- Partials      215      216       +1     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

[giortzisg]

Let's fix the lint issues and extract getTLSConfig under utils and we're good to go.

[github-actions]

Semver Impact of This PR

🟡 Minor (new features)

📋 Changelog Preview

This is how your changes will appear in the changelog.
Entries from this PR are highlighted with a left border (blockquote style).

---

New Features ✨

• TLS config option availability using the client options by ssi-anik in #1168

Build / dependencies / internal 🔧

• (release) Switch from action-prepare-release to Craft by BYK in #1167
• (repo) Add Claude Code settings with basic permissions by philipphofmann in #1175
• Update release and changelog-preview workflows by giortzisg in #1177
• Bump echo to 4.10.1 by giortzisg in #1174

---

_{🤖 This preview updates automatically when you update the PR.}

[ssi-anik]

@giortzisg is there any update required?

[giortzisg]

Hey @ssi-anik, sorry for not specifying on the previous comment, but the internal transport also uses getProxyConfig and getTLSConfig here, so we should move the proxy and tls utils under internal/utils. But after that it's good to go.