fix(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

Attention: Patch coverage is 28.57143% with 65 lines in your changes missing coverage. Please review.

Project coverage is 54.61%. Comparing base (f485671) to head (99f8af3).
Report is 16 commits behind head on master.

Files with missing lines	Patch %	Lines
pkg/services/opsgenie.go	0.00%	8 Missing ⚠️
pkg/services/alertmanager.go	25.00%	5 Missing and 1 partial ⚠️
pkg/services/github.go	25.00%	5 Missing and 1 partial ⚠️
pkg/services/googlechat.go	25.00%	5 Missing and 1 partial ⚠️
pkg/services/grafana.go	25.00%	5 Missing and 1 partial ⚠️
pkg/services/mattermost.go	25.00%	5 Missing and 1 partial ⚠️
pkg/services/newrelic.go	25.00%	5 Missing and 1 partial ⚠️
pkg/services/teams.go	25.00%	5 Missing and 1 partial ⚠️
pkg/services/webex.go	25.00%	5 Missing and 1 partial ⚠️
pkg/services/webhook.go	25.00%	5 Missing and 1 partial ⚠️
... and 1 more

Additional details and impacted files

@@            Coverage Diff             @@
##           master     #376      +/-   ##
==========================================
- Coverage   55.35%   54.61%   -0.74%     
==========================================
  Files          35       46      +11     
  Lines        3438     4202     +764     
==========================================
+ Hits         1903     2295     +392     
- Misses       1256     1562     +306     
- Partials      279      345      +66     

☔ 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 this looks good to me in general

Before merging this, I believe you should do the following:

1. Ask in the #argocd-contributors channel about this feature - for me this seems good, but would like to have some consensus before merging
2. Assuming you get a 👍, that we try to get in some unit tests exercising the conversions from string -> duration

[pasha-codefresh]

Looks good, but i would try to move transport creation in single place, otherwise we need to duplicate it in every provider

[blakepettersson]

@afzal442 do you have further thoughts regarding what Pasha said?

[afzal442]

sure! I think that makes sense. But I'll need to apply the suggestions it in order to check this.

[afzal442]

I tried moving the transport as a new helper function like below, but need your thoughts whether that is feasible; I'd create this inside transport.go and use it as a helper

func NewServiceHTTPClient(serviceName, webhookUrl string, maxIdleConns, maxIdleConnsPerHost, maxConnsPerHost int, idleConnTimeout time.Duration, insecureSkipVerify bool) *http.Client {
	transport := NewTransport(webhookUrl, maxIdleConns, maxIdleConnsPerHost, maxConnsPerHost, idleConnTimeout, insecureSkipVerify)
	return &http.Client{
		Transport: NewLoggingRoundTripper(transport, log.WithField("service", serviceName)),
	}
}

cc @blakepettersson @pasha-codefresh

[blakepettersson]

@afzal442 yes, something like this should work

func NewServiceHTTPClient(maxIdleConns, maxIdleConnsPerHost, maxConnsPerHost int, idleConnTimeout, serviceName string, insecureSkipVerify bool, apiURL string) (client *http.Client, err error) {
	var timeout time.Duration
	if idleConnTimeout != "" {
		timeout, err = time.ParseDuration(idleConnTimeout)
		if err != nil {
			return nil, fmt.Errorf("failed to parse idle connection timeout: %w", err)
		}
	}
	transport := httputil.NewTransport(apiURL, maxIdleConns, maxIdleConnsPerHost, maxConnsPerHost, timeout, insecureSkipVerify)
	return &http.Client{
		Transport: httputil.NewLoggingRoundTripper(transport, log.WithField("service", serviceName)),
	}, nil
}