feat(http): instantiate the http.Transport for connection limits

[afzal442]

For #22800

• ArgoCD Notifications Controller spawns hundreds of connections per instance without ever closing them argo-cd#22800

[afzal442]

@blakepettersson if I try to pass in those numbers as parameters to the NewTransport function, there will be a lot of services to be changed/ get impacted. Any input plz.

[blakepettersson]

@afzal442 yes indeed, but I don't think it's a bad idea to be allowed to set those http settings for any service that make use of it.

So we can expand this task to allow all affected services to set these http.Transport settings. You'll need to add relevant fields for all the *Options structs that could make use of these settings.

[codecov]

Codecov Report

❌ Patch coverage is 52.56410% with 37 lines in your changes missing coverage. Please review.
✅ Project coverage is 54.64%. Comparing base (da04400) to head (0cc1347).
⚠️ Report is 2 commits behind head on master.

Files with missing lines	Patch %	Lines
pkg/services/opsgenie.go	14.28%	6 Missing ⚠️
pkg/services/slack.go	50.00%	3 Missing and 2 partials ⚠️
pkg/util/http/transport.go	73.68%	4 Missing and 1 partial ⚠️
pkg/services/github.go	55.55%	3 Missing and 1 partial ⚠️
pkg/services/webhook.go	40.00%	2 Missing and 1 partial ⚠️
pkg/services/alertmanager.go	50.00%	1 Missing and 1 partial ⚠️
pkg/services/googlechat.go	50.00%	1 Missing and 1 partial ⚠️
pkg/services/grafana.go	50.00%	1 Missing and 1 partial ⚠️
pkg/services/mattermost.go	50.00%	1 Missing and 1 partial ⚠️
pkg/services/newrelic.go	50.00%	1 Missing and 1 partial ⚠️
... and 2 more

Additional details and impacted files

@@            Coverage Diff             @@
##           master     #376      +/-   ##
==========================================
- Coverage   55.41%   54.64%   -0.77%     
==========================================
  Files          46       46              
  Lines        4125     4141      +16     
==========================================
- Hits         2286     2263      -23     
- Misses       1511     1538      +27     
- Partials      328      340      +12     

☔ 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.

[blakepettersson]

@afzal442 can you

1. Update the docs for all of these options
2. Verify that this works from Argo CD?

[afzal442]

To verify this further, do I need to create instances and check against it using argocd CLI locally on this branch?

[blakepettersson]

So you'll need to setup a local copy of Argo CD.

You'll need to configure go.mod to replace notifications-engine with your own version, e.g.

github.com/argoproj/gitops-engine => github.com/afzal442/notifications-engine <your latest commit on this PR>

then run go mod tidy. Then run your local version of Argo CD and verify that this works as intended.

Ideally we should check that setting up notifications both work without setting the new fields as well as by setting the new fields (to ensure we haven't broken anything).

[afzal442]

go mod tidy

go: downloading github.com/argoproj/notifications-engine v0.4.1-0.20250309174002-87bf0576a872
go: github.com/argoproj/argo-cd/v3/cmd/argocd/commands imports
        github.com/argoproj/gitops-engine/pkg/health: github.com/afzal442/[email protected]: parsing go.mod:
        module declares its path as: github.com/argoproj/notifications-engine
                but was required as: github.com/afzal442/notifications-engine

[blakepettersson]

@afzal442 what does your go.mod look like?

You should have a go.mod which looks something like this (look at the very bottom of go.mod to see where all the replaces happen)

module github.com/argoproj/argo-cd/v3

go 1.24.3

require (
	github.com/argoproj/notifications-engine v0.4.1-0.20250309174002-87bf0576a872
        // Other requires snipped for brevity
)

// Other requires snipped for brevity

replace (
	github.com/argoproj/notifications-engine => github.com/afzal442/notifications-engine d3dfddfec1fb8b2e9ce7af27317d42e880aa676a
        // Other replaces snipped for brevity
)

Once it looks like that, you can run go mod tidy which will fix the dependency tree for you.

[afzal442]

Ideally we should check that setting up notifications both work without setting the new fields as well as by setting the new fields

Thanks! but any help on how I can set up notifications with setting the new fields. Do I need to setup Github notification svc? any further test?

[blakepettersson]

Hey @afzal442, sorry for the (very) late review 😞

I gave this a spin and it generally LGTM, with the exception that duration strings cannot be directly parsed with Golang's JSON parser (that's on me, I thought that worked). So basically you will need to use strings and handle the conversions manually.

There's also a minor section where the wrong parameter is used.

Beyond that, could we get some docs documenting these values where applicable? Otherwise LGTM!

[afzal442]

hey @blakepettersson any idea why this test is failing here https://github.com/argoproj/notifications-engine/actions/runs/16178565279/job/45669510709?pr=376#step:7:17?

[blakepettersson]

@afzal442 yes, before parsing the conn timeouts you need to check if the conn timeout is an empty string. If it's empty don't parse

[afzal442]

could we get some docs documenting these values where applicable?

How about making a separate PR for this?

[blakepettersson]

How about making a separate PR for this?

@afzal442, no these changes need to be documented in this PR, given that there are a bunch of parameters which are important to know about.

[blakepettersson]

@afzal442 there shouldn't be any need to construct new struct instances with the opts.TransportOptions refactor. You can just pass it along.

[alexmt]

LGTM

[blakepettersson]

@afzal442 you will need to create a PR in argocd to get the latest notifications-engine version