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.

Sign up for GitHub

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

resolve proxy to inject using http package #6675

Merged
merged 14 commits into from
Feb 7, 2025
Merged

resolve proxy to inject using http package #6675

Show file tree
Hide file tree
Changes from 7 commits
Commits
Show all changes
14 commits
Select commit Hold shift + click to select a range
264215a
resolve proxy to inject using http package
intxgo Jan 31, 2025
c38c7ed
unit tests
intxgo Jan 31, 2025
d3bedfc
golint
intxgo Jan 31, 2025
5949aa0
golint
intxgo Jan 31, 2025
d0339f2
changelog fragment
intxgo Jan 31, 2025
fb8b21b
changelog fragment
intxgo Jan 31, 2025
96dc6cc
move environment manipulation, trying to fix test run in CI
intxgo Jan 31, 2025
3fb97d6
fix unit test, localhost never goes through environment proxy
intxgo Jan 31, 2025
04d509d
use internal function to be able to unit test the behavior
intxgo Feb 5, 2025
ec6d43b
make lint happy
intxgo Feb 5, 2025
62a411b
Merge branch 'main' into environment-proxy-fix
intxgo Feb 5, 2025
92bd58d
fix build
intxgo Feb 6, 2025
d01f049
update NOTICE
intxgo Feb 6, 2025
04eccf9
Merge branch 'main' into environment-proxy-fix
intxgo Feb 7, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
32 changes: 32 additions & 0 deletions changelog/fragments/1738329703-environment-proxy-injection-fix.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,32 @@
# Kind can be one of:
# - breaking-change: a change to previously-documented behavior
# - deprecation: functionality that is being removed in a later release
# - bug-fix: fixes a problem in a previous version
# - enhancement: extends functionality but does not break or fix existing behavior
# - feature: new functionality
# - known-issue: problems that we are aware of in a given version
# - security: impacts on the security of a product or a user’s deployment.
# - upgrade: important information for someone upgrading from a prior version
# - other: does not fit into any of the other categories
kind: bug-fix

# Change summary; a 80ish characters long description of the change.
summary: environment-proxy-injection-fix

# Long description; in case the summary is not enough to describe the change
# this field accommodate a description without length limits.
# NOTE: This field will be rendered only for breaking-change and known-issue kinds at the moment.
description: When injecting environment proxy into Elastic Defend config the Agent should use http library to resolve the proxy settings to ensure consistency across all components.

# Affected component; usually one of "elastic-agent", "fleet-server", "filebeat", "metricbeat", "auditbeat", "all", etc.
component: elastic-agent

# PR URL; optional; the PR number that added the changeset.
# If not present is automatically filled by the tooling finding the PR where this changelog fragment has been added.
# NOTE: the tooling supports backports, so it's able to fill the original PR number instead of the backport PR number.
# Please provide it if you are adding a fragment for a different PR.
pr: https://github.com/elastic/elastic-agent/pull/6675

# Issue URL; optional; the GitHub issue related to this changeset (either closes or is part of).
# If not present is automatically filled by the tooling with the issue linked to the PR number.
issue: https://github.com/elastic/elastic-agent/issues/6209
42 changes: 16 additions & 26 deletions internal/pkg/agent/application/inject_proxy_component_modifier.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,8 +5,7 @@
package application

import (
"os"
"strings"
"net/http"

"github.com/elastic/elastic-agent-client/v7/pkg/client"
"github.com/elastic/elastic-agent/internal/pkg/agent/application/coordinator"
Expand Down Expand Up @@ -59,9 +58,8 @@ func InjectProxyEndpointModifier() coordinator.ComponentsModifier {

// injectProxyURL will inject the a proxy_url into the passed map if there is no existing key and there is an appropriate proxy through defined as an env var.
//
// The 1st item of the passed hosts list is checked to see if it starts with https or http and the corresponding proxy var is used.
// Nothing is injected if the *_PROXY env var is empty, the map contains proxy_url: "", or the map has proxy_disable: true.
// If no hosts are passed, then the HTTPS_PROXY value is used over the HTTP_PROXY value if it's defined.
// Go http client is used to determine the proxy URL, to ensure consistent behavior across all components.
// Traffic through proxy is preferred if the proxy is defined for any of the hosts.
func injectProxyURL(m map[string]interface{}, hosts []string) {
if m == nil {
return
Expand All @@ -79,28 +77,20 @@ func injectProxyURL(m map[string]interface{}, hosts []string) {
}
}

var proxyURL string
matched := false
// If hosts are specified, check the 1st to see if HTTPS or HTTP is used to determine proxy
if len(hosts) > 0 {
if strings.HasPrefix(hosts[0], "https://") {
matched = true
proxyURL = os.Getenv("HTTPS_PROXY")
} else if strings.HasPrefix(hosts[0], "http://") {
matched = true
proxyURL = os.Getenv("HTTP_PROXY")
// Check if a proxy is defined for the hosts
for _, host := range hosts {
//nolint:noctx // this request will not be executed
request, err := http.NewRequest("GET", host, nil)
if err != nil {
continue
}
}
// if no hosts are specified, or it could not match a host prefix prefer HTTPS_PROXY over HTTP_PROXY
if proxyURL == "" && !matched {
proxyURL = os.Getenv("HTTPS_PROXY")
if proxyURL == "" {
proxyURL = os.Getenv("HTTP_PROXY")
proxyURL, err := http.ProxyFromEnvironment(request)
Copy link
Contributor

Choose a reason for hiding this comment

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

👍 didn't know this existed!

Copy link
Member

@cmacknz cmacknz Feb 4, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

I think this function in its current form is the problem in the tests. If you look at the implementation it is just calling https://cs.opensource.google/go/go/+/master:src/net/http/transport.go?q=FromEnvironment&ss=go%2Fgo

func ProxyFromEnvironment(req *Request) (*url.URL, error) {
	return envProxyFunc()(req.URL)
}


// envProxyFunc returns a function that reads the
// environment variable to determine the proxy address.
func envProxyFunc() func(*url.URL) (*url.URL, error) {
	envProxyOnce.Do(func() {
		envProxyFuncValue = httpproxy.FromEnvironment().ProxyFunc()
	})
	return envProxyFuncValue
}

If you directly get a reference to httpproxy.FromEnvironment().ProxyFunc() yourself I think the problem goes away as you skip the do.Once. The following fixes the tests. You could maybe be smarter and not re-read from the environment on every configuration change if you wanted to, I'm not sure that efficiency matters here.

diff --git a/internal/pkg/agent/application/inject_proxy_component_modifier.go b/internal/pkg/agent/application/inject_proxy_component_modifier.go
index d8eb9f1e98..2f68e7452c 100644
--- a/internal/pkg/agent/application/inject_proxy_component_modifier.go
+++ b/internal/pkg/agent/application/inject_proxy_component_modifier.go
@@ -10,6 +10,7 @@ import (
        "github.com/elastic/elastic-agent-client/v7/pkg/client"
        "github.com/elastic/elastic-agent/internal/pkg/agent/application/coordinator"
        "github.com/elastic/elastic-agent/pkg/component"
+       "golang.org/x/net/http/httpproxy"
 )

 // InjectProxyEndpointModifier injects a proxy_url value into endpoint's output config if one is not set.
@@ -84,7 +85,7 @@ func injectProxyURL(m map[string]interface{}, hosts []string) {
                if err != nil {
                        continue
                }
-               proxyURL, err := http.ProxyFromEnvironment(request)
+               proxyURL, err := httpproxy.FromEnvironment().ProxyFunc()(request.URL)
                if err != nil {
                        continue
                }

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks. This will theoretically departure a tiny bit from the desired goal to have Endpoint behaving exactly as Agent, because Agent will read the environment only once for it's http operations. However since the environment on a process can be only modified from within the process, by debugger, etc, but not just by modifying system configuration LGTM.

Copy link
Contributor Author

@intxgo intxgo Feb 5, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

updated 04d509d

if err != nil {
continue
}
if proxyURL != nil && proxyURL.String() != "" {
m["proxy_url"] = proxyURL.String()
return
}
}
// No proxy defined
if proxyURL == "" {
return
}
m["proxy_url"] = proxyURL
}
39 changes: 13 additions & 26 deletions internal/pkg/agent/application/inject_proxy_component_modifier_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -318,9 +318,6 @@ func TestInjectProxyEndpointModifier(t *testing.T) {
}

func Test_injectProxyURL(t *testing.T) {
t.Setenv("HTTPS_PROXY", "https://localhost:8080")
t.Setenv("HTTP_PROXY", "http://localhost:8081")

tests := []struct {
name string
m map[string]interface{}
Expand All @@ -341,41 +338,31 @@ func Test_injectProxyURL(t *testing.T) {
hosts: nil,
expect: map[string]interface{}{"key": "value", "proxy_disable": true},
}, {
name: "no hosts uses HTTPS_PROXY",
name: "http hosts uses HTTP_PROXY",
m: map[string]interface{}{"key": "value"},
hosts: nil,
expect: map[string]interface{}{"key": "value", "proxy_url": "https://localhost:8080"},
hosts: []string{"http://example:80"},
expect: map[string]interface{}{"key": "value", "proxy_url": "http://localhost:8081"},
}, {
name: "https hosts uses HTTPS_PROXY",
m: map[string]interface{}{"key": "value"},
hosts: []string{"https://example:443"},
expect: map[string]interface{}{"key": "value", "proxy_url": "https://localhost:8080"},
}, {
name: "http host uses HTTP_PROXY",
},
{
name: "host skipped by NO_PROXY",
m: map[string]interface{}{"key": "value"},
hosts: []string{"http://example:80"},
expect: map[string]interface{}{"key": "value", "proxy_url": "http://localhost:8081"},
hosts: []string{"https://do.not.inject.proxy.for.me", "https://do.not.inject.proxy.for.me:8080", "really.do.not.inject.proxy.for.me"},
expect: map[string]interface{}{"key": "value"},
}}

for _, tc := range tests {
t.Run(tc.name, func(t *testing.T) {
t.Setenv("HTTPS_PROXY", "https://localhost:8080")
t.Setenv("HTTP_PROXY", "http://localhost:8081")
t.Setenv("NO_PROXY", "do.not.inject.proxy.for.me")

injectProxyURL(tc.m, tc.hosts)
require.Equal(t, tc.expect, tc.m)
})
}

t.Run("no hosts or HTTPS_PROXY uses HTTP_PROXY", func(t *testing.T) {
t.Setenv("HTTPS_PROXY", "")
t.Setenv("HTTP_PROXY", "http://localhost:8081")

m := map[string]interface{}{"key": "value"}
injectProxyURL(m, nil)
require.Equal(t, map[string]interface{}{"key": "value", "proxy_url": "http://localhost:8081"}, m)
})
t.Run("no env vars", func(t *testing.T) {
t.Setenv("HTTPS_PROXY", "")
t.Setenv("HTTP_PROXY", "")
m := map[string]interface{}{"key": "value"}
injectProxyURL(m, nil)
require.Equal(t, map[string]interface{}{"key": "value"}, m)
})
}
Loading