Skip to content

Conversation

@ldematte
Copy link
Contributor

@ldematte ldematte commented Mar 1, 2025

Based on #123503

Relates to ES-10994

@ldematte ldematte added v8.18.1 v8.19.0 v9.0.1 >non-issue auto-backport Automatically create backport pull requests when merged :Core/Infra/Entitlements Entitlements infrastructure labels Mar 3, 2025
@ldematte ldematte marked this pull request as ready for review March 3, 2025 07:55
@ldematte ldematte requested a review from a team as a code owner March 3, 2025 07:55
@elasticsearchmachine
Copy link
Collaborator

Pinging @elastic/es-core-infra (Team:Core/Infra)

@elasticsearchmachine elasticsearchmachine added the Team:Core/Infra Meta label for core/infra team label Mar 3, 2025
@ldematte ldematte changed the title [Entitlements] FtpURLConnection and HttpURLConnection instrumentation [Entitlements] Add URLConnection instrumentation for ftp, http and https protocols Mar 3, 2025
java.net.HttpURLConnection that,
String name
) {
policyManager.checkOutboundNetworkAccess(callerClass);
Copy link
Contributor

Choose a reason for hiding this comment

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

(Wow, this kind of sucks. Requiring a check for every individual property of a connection seems fishy. But we really have no alternative right now unless we consider some form of "deep" check.)

void check$sun_net_www_protocol_http_HttpURLConnection$$openConnectionCheckRedirects(Class<?> callerClass, java.net.URLConnection c);

void check$sun_net_www_protocol_http_HttpURLConnection$connect(Class<?> callerClass, java.net.HttpURLConnection that);

Copy link
Contributor

Choose a reason for hiding this comment

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

what about doTunneling?

Copy link
Contributor

Choose a reason for hiding this comment

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

and looks like this is yet another case we haven't handled yet :/

protected HttpClient getNewHttpClient(URL url, Proxy p, int connectTimeout)
        throws IOException {
        return HttpClient.New(url, p, connectTimeout, this);
    }

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I added those and then removed them because they are "internal" - they are public but in non-exported classes.
(I realized because I was not able to test them :D )

Copy link
Contributor

Choose a reason for hiding this comment

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

I was just about to say, this is probably not exported 👍

Copy link
Contributor

@mosche mosche left a comment

Choose a reason for hiding this comment

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

lgtm
but I fear there's just more and more coming up that requires instrumentation

@ldematte
Copy link
Contributor Author

ldematte commented Mar 3, 2025

but I fear there's just more and more coming up that requires instrumentation

Agreed, this is best effort/what seems to be a sensible compromise

@ldematte ldematte merged commit 4f2f1bc into elastic:main Mar 4, 2025
17 checks passed
@ldematte ldematte deleted the entitlements/missing-url-connection-2 branch March 4, 2025 07:35
ldematte added a commit to ldematte/elasticsearch that referenced this pull request Mar 4, 2025
@elasticsearchmachine
Copy link
Collaborator

💔 Backport failed

Status Branch Result
8.18 Commit could not be cherrypicked due to conflicts
8.x Commit could not be cherrypicked due to conflicts
9.0

You can use sqren/backport to manually backport by running backport --upstream elastic/elasticsearch --pr 123802

ldematte added a commit to ldematte/elasticsearch that referenced this pull request Mar 4, 2025
ldematte added a commit to ldematte/elasticsearch that referenced this pull request Mar 4, 2025
elasticsearchmachine pushed a commit that referenced this pull request Mar 4, 2025
* [Entitlements] Add URLConnection instrumentation (#123503)

* [Entitlements] Add URLConnection instrumentation for ftp, http and https protocols (#123802)
elasticsearchmachine pushed a commit that referenced this pull request Mar 4, 2025
* [Entitlements] Add URLConnection instrumentation (#123503)

* [Entitlements] Add URLConnection instrumentation for ftp, http and https protocols (#123802)
georgewallace pushed a commit to georgewallace/elasticsearch that referenced this pull request Mar 11, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

auto-backport Automatically create backport pull requests when merged :Core/Infra/Entitlements Entitlements infrastructure >non-issue Team:Core/Infra Meta label for core/infra team v8.18.1 v8.19.0 v9.0.1 v9.1.0

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants