Skip to content

Remove overwriting the default HTTP transport - #1112

Open
mozesl-nokia wants to merge 8 commits into
kptdev:mainfrom
nokia:default-http-transport
Open

Remove overwriting the default HTTP transport#1112
mozesl-nokia wants to merge 8 commits into
kptdev:mainfrom
nokia:default-http-transport

Conversation

@mozesl-nokia

Copy link
Copy Markdown
Contributor

Remove overwriting the default HTTP transport


Description

  • What changed: Removed overwriting the default HTTP transport and the tag resolution from podevaluator.
  • Why it’s needed: Overwriting the default transport breaks regclient and is generally bad practice. The tag resolution is already done in the runtime before the evaluator is called.
  • How it works:

Related Issue(s)

  • None

Type of Change

  • Bug fix
  • Refactor

Checklist

  • Code follows project style guidelines
  • Self-reviewed changes
  • Tests added/updated
  • Documentation added/updated
  • All tests and gating checks pass

Additional Notes (Optional)

  • Known issues: This way, we are not strictly tracking all http traffic going in and out of porch, but most metrics are already exposed by the apiserver framework.
  • Further improvements: Add E2E tests for tag resolution and TLS handling
  • Review notes:

AI Disclosure

  • I have used AI in the creation of this PR.

If so, please describe how:

  • The new httpclient package and the refactor of the TLS handling in podevaluator was done by composer-2.5

Signed-off-by: Mózes László Máté <laszlo.mozes@nokia.com>
Signed-off-by: Mózes László Máté <laszlo.mozes@nokia.com>
@mozesl-nokia
mozesl-nokia requested review from a team July 13, 2026 08:49
@mozesl-nokia mozesl-nokia added the bug Something isn't working label Jul 13, 2026
@mozesl-nokia
mozesl-nokia requested a review from efiacor as a code owner July 13, 2026 08:49
@mozesl-nokia mozesl-nokia added the go Pull requests that update go code label Jul 13, 2026
@netlify

netlify Bot commented Jul 13, 2026

Copy link
Copy Markdown

Deploy Preview for kpt-porch ready!

Name Link
🔨 Latest commit 2b04dd2
🔍 Latest deploy log https://app.netlify.com/projects/kpt-porch/deploys/6a579e75c027470008d6cdeb
😎 Deploy Preview https://deploy-preview-1112--kpt-porch.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.
🤖 Make changes Run an agent on this branch

To edit notification comments on pull requests, go to your Netlify project configuration.

@dosubot dosubot Bot added the size:L This PR changes 100-499 lines, ignoring generated files. label Jul 13, 2026
@github-actions
github-actions Bot requested a review from Copilot July 13, 2026 08:49

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Pull request overview

This PR removes global mutation of http.DefaultTransport (previously used for OpenTelemetry wrapping) and introduces a dedicated pkg/httpclient helper for constructing registry-focused HTTP transports/clients. It also refactors TLS handling for private registry pulls and removes tag-resolution logic from the pod evaluator path.

Changes:

  • Added pkg/httpclient with a snapshot-based base *http.Transport and helpers to create registry transports/clients with optional TLS config.
  • Removed OpenTelemetry wrapping of http.DefaultTransport in telemetry setup to avoid breaking downstream clients (e.g., regclient).
  • Refactored private-registry TLS CA discovery/transport creation in podmanager and removed pod-evaluator tag resolution + related tests.

Reviewed changes

Copilot reviewed 10 out of 10 changed files in this pull request and generated 2 comments.

Show a summary per file
File Description
pkg/httpclient/registry.go New shared HTTP transport/client helpers for registry outbound calls without mutating global defaults.
pkg/httpclient/registry_test.go Adds unit tests for the new httpclient helpers.
internal/telemetry/otel.go Removes global HTTP default transport/client wrapping during OTel setup.
go.mod Reclassifies sigs.k8s.io/cli-utils as indirect after refactor.
func/internal/podmanager.go Uses new registry transport helper and factors CA path discovery into a helper.
func/internal/podmanager_unit_test.go Removes tests tied to the deleted in-file transport constructor.
func/internal/podevaluator.go Refactors construction wiring and removes tag resolution prior to evaluation.
func/internal/podevaluator_unit_test.go Updates evaluator unit tests to match removed tag-resolver wiring.
func/internal/podevaluator_tag_resolution_test.go Deletes tag resolution unit tests (tag resolution no longer done here).
func/internal/executableevaluator_test.go Minor test constant addition for shared test image name.

Comment thread pkg/httpclient/registry_test.go Outdated
Comment thread func/internal/podmanager.go Outdated
liamfallon
liamfallon previously approved these changes Jul 13, 2026
@dosubot dosubot Bot added the lgtm #ededed label Jul 13, 2026
Signed-off-by: Mózes László Máté <laszlo.mozes@nokia.com>
Signed-off-by: Mózes László Máté <laszlo.mozes@nokia.com>
Assisted-by: Cursor:composer-2.5
Signed-off-by: Mózes László Máté <laszlo.mozes@nokia.com>

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Pull request overview

Copilot reviewed 12 out of 12 changed files in this pull request and generated 2 comments.

Comment thread func/internal/podmanager.go
Comment thread func/internal/podevaluator.go
Signed-off-by: Mózes László Máté <laszlo.mozes@nokia.com>
Signed-off-by: Mózes László Máté <laszlo.mozes@nokia.com>
liamfallon
liamfallon previously approved these changes Jul 14, 2026
@liamfallon
liamfallon requested review from a team and nagygergo July 15, 2026 13:53
@sonarqubecloud

Copy link
Copy Markdown

@mozesl-nokia
mozesl-nokia requested a review from liamfallon July 16, 2026 08:30
return nil, err
}

http.DefaultTransport = otelhttp.NewTransport(http.DefaultTransport)

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

My problem with this one is that it will disable monitoring http metrics for the following paths:
external -> porch server
porch-server -> kube-apiserver
porch-server -> git
function-runner -> oci
function-runner -> kube-apiserver

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

We can add these individually, probably

How can an external service call porch server directly btw? Shouldn't it just be through the kube apiserver?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

I will add these individual wrappers after #1111 is merged and this is rebased.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

bug Something isn't working do-not-merge/hold #ededed go Pull requests that update go code lgtm #ededed size:L This PR changes 100-499 lines, ignoring generated files.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants