Skip to content

WIP 🐛 OCPBUGS-61082: Add HTTP proxy support for operator deployments#2501

Open
tmshort wants to merge 1 commit intooperator-framework:mainfrom
tmshort:fix-OCPBUGS-61082
Open

WIP 🐛 OCPBUGS-61082: Add HTTP proxy support for operator deployments#2501
tmshort wants to merge 1 commit intooperator-framework:mainfrom
tmshort:fix-OCPBUGS-61082

Conversation

@tmshort
Copy link
Contributor

@tmshort tmshort commented Feb 10, 2026

Fixes: OCPBUGS-61082

Add support for HTTP_PROXY, HTTPS_PROXY, and NO_PROXY environment variables in operator deployments. Proxy configuration is read from operator-controller's environment at startup and injected into all containers in deployed operator Deployments.

When operator-controller restarts with changed proxy configuration, existing operator deployments are automatically updated via triggered reconciliation.

Supports both helm and boxcutter appliers.
Includes unit and e2e tests.

Assisted-By: Claude

Description

Reviewer Checklist

  • API Go Documentation
  • Tests: Unit Tests (and E2E Tests, if appropriate)
  • Comprehensive Commit Messages
  • Links to related GitHub Issue(s)

Copilot AI review requested due to automatic review settings February 10, 2026 20:07
@netlify
Copy link

netlify bot commented Feb 10, 2026

Deploy Preview for olmv1 ready!

Name Link
🔨 Latest commit 1154717
🔍 Latest deploy log https://app.netlify.com/projects/olmv1/deploys/698f41a37b96c20008de2b31
😎 Deploy Preview https://deploy-preview-2501--olmv1.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

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

@openshift-ci
Copy link

openshift-ci bot commented Feb 10, 2026

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by:
Once this PR has been reviewed and has the lgtm label, please assign perdasilva for approval. For more information see the Code Review Process.

The full list of commands accepted by this bot can be found here.

Details Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

Copy link
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull request overview

This pull request adds HTTP proxy support for operator deployments in OLMv1. The implementation reads proxy configuration (HTTP_PROXY, HTTPS_PROXY, NO_PROXY) from the operator-controller's environment at startup and injects these values into all containers of deployed operator Deployments. When proxy configuration changes and operator-controller restarts, existing operator deployments are automatically updated through a startup reconciliation trigger that annotates all ClusterExtensions. The feature supports both Helm and Boxcutter appliers through a shared manifest provider architecture.

Changes:

  • Added proxy environment variable injection into operator deployment containers
  • Implemented proxy configuration fingerprinting to trigger new ClusterExtensionRevisions when proxy settings change
  • Added startup hook to trigger reconciliation of existing ClusterExtensions when proxy config changes
  • Extended E2E tests with proxy configuration scenarios and increased timeouts for deployment updates

Reviewed changes

Copilot reviewed 13 out of 13 changed files in this pull request and generated 9 comments.

Show a summary per file
File Description
test/e2e/steps/steps.go Added E2E test step functions for proxy environment variable verification, increased timeout constants for Boxcutter deployments, added CRD cleanup wait logic
test/e2e/features/proxy.feature New E2E test scenarios covering proxy configuration inheritance and removal
internal/operator-controller/rukpak/render/render.go Added Proxy struct type and WithProxy option for render configuration
internal/operator-controller/rukpak/render/registryv1/generators/resources.go Implemented WithProxy function to inject/remove proxy env vars from deployment containers
internal/operator-controller/rukpak/render/registryv1/generators/resources_test.go Added unit test for proxy environment variable injection
internal/operator-controller/rukpak/render/registryv1/generators/generators.go Integrated proxy options into deployment generation with always-call pattern for proper removal
internal/operator-controller/labels/labels.go Added ProxyConfigHashKey annotation for tracking proxy configuration changes
internal/operator-controller/controllers/boxcutter_reconcile_steps_apply_test.go Updated test to provide empty proxy fingerprint function
internal/operator-controller/controllers/boxcutter_reconcile_steps.go Modified ApplyBundleWithBoxcutter to accept proxy fingerprint function and add hash to revision annotations
internal/operator-controller/applier/provider.go Added Proxy field to RegistryV1ManifestProvider and ProxyFingerprint method for change detection
internal/operator-controller/applier/boxcutter.go Enhanced revision creation logic to detect proxy changes via hash comparison and create new revisions accordingly
cmd/operator-controller/main.go Read proxy environment variables at startup, added runnable to trigger reconciliation on all ClusterExtensions when proxy config is detected
Makefile Added 30-minute timeout and GODOG_TAGS support for selective E2E test execution

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

@tmshort tmshort changed the title 🐛 OCPBUGS-61082: Add HTTP proxy support for operator deployments WIP 🐛 OCPBUGS-61082: Add HTTP proxy support for operator deployments Feb 10, 2026
@openshift-ci openshift-ci bot added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Feb 10, 2026
Copilot AI review requested due to automatic review settings February 10, 2026 21:42
Copy link
Contributor

Copilot AI left a comment

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 14 out of 14 changed files in this pull request and generated 9 comments.


💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

@codecov
Copy link

codecov bot commented Feb 11, 2026

Codecov Report

❌ Patch coverage is 77.02703% with 34 lines in your changes missing coverage. Please review.
✅ Project coverage is 73.29%. Comparing base (10c2841) to head (1154717).

Files with missing lines Patch % Lines
cmd/operator-controller/main.go 47.50% 17 Missing and 4 partials ⚠️
internal/operator-controller/proxy/proxy.go 78.94% 5 Missing and 3 partials ⚠️
...r/rukpak/render/registryv1/generators/resources.go 91.66% 2 Missing and 1 partial ⚠️
internal/operator-controller/applier/boxcutter.go 86.66% 2 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #2501      +/-   ##
==========================================
+ Coverage   73.23%   73.29%   +0.05%     
==========================================
  Files         102      103       +1     
  Lines        8504     8641     +137     
==========================================
+ Hits         6228     6333     +105     
- Misses       1801     1825      +24     
- Partials      475      483       +8     
Flag Coverage Δ
e2e 45.89% <46.62%> (+0.05%) ⬆️
experimental-e2e 53.59% <60.81%> (+0.18%) ⬆️
unit 57.79% <54.05%> (-0.10%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

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

Copilot AI review requested due to automatic review settings February 12, 2026 01:54
Copy link
Contributor

Copilot AI left a comment

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 14 out of 14 changed files in this pull request and generated 4 comments.


💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Copilot AI review requested due to automatic review settings February 12, 2026 18:50
Copy link
Contributor

Copilot AI left a comment

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 14 out of 14 changed files in this pull request and generated 3 comments.


💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Copilot AI review requested due to automatic review settings February 12, 2026 19:50
Copy link
Contributor

Copilot AI left a comment

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 14 out of 14 changed files in this pull request and generated 2 comments.


💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

@camilamacedo86
Copy link
Contributor

We need to solve: Error: Failed to connect to Docker: Error response from daemon: client version 1.43 is too old. Minimum supported API version is 1.44, please upgrade your client to a newer version

To move forward :-(

Fixes: OCPBUGS-61082

Add support for HTTP_PROXY, HTTPS_PROXY, and NO_PROXY environment
variables in operator deployments. Proxy configuration is read from
operator-controller's environment at startup and injected into all
containers in deployed operator Deployments.

When operator-controller restarts with changed proxy configuration,
existing operator deployments are automatically updated via triggered
reconciliation.

Supports both helm and boxcutter appliers.
Includes unit and e2e tests.

Signed-off-by: Todd Short <tshort@redhat.com>
Assisted-By: Claude
Copilot AI review requested due to automatic review settings February 13, 2026 15:22
@tmshort
Copy link
Contributor Author

tmshort commented Feb 13, 2026

We need to solve: Error: Failed to connect to Docker: Error response from daemon: client version 1.43 is too old. Minimum supported API version is 1.44, please upgrade your client to a newer version

To move forward :-(

See: #2506

Copy link
Contributor

Copilot AI left a comment

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 14 out of 14 changed files in this pull request and generated 2 comments.


💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

// ProxyFingerprint returns a stable hash of the proxy configuration.
// This is used to detect when proxy settings change and a new revision is needed.
// The hash is calculated once during proxy construction and cached in the Proxy itself.
func (r *RegistryV1ManifestProvider) ProxyFingerprint() string {
Copy link

Copilot AI Feb 13, 2026

Choose a reason for hiding this comment

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

The ProxyFingerprint method will panic with a nil pointer dereference if r.Proxy is nil. According to the proxy.NewFromEnv() function, it returns nil when no proxy environment variables are set. The method should handle the nil case safely.

Add a nil check before calling Fingerprint():

func (r *RegistryV1ManifestProvider) ProxyFingerprint() string {
    if r.Proxy == nil {
        return ""
    }
    return r.Proxy.Fingerprint()
}
Suggested change
func (r *RegistryV1ManifestProvider) ProxyFingerprint() string {
func (r *RegistryV1ManifestProvider) ProxyFingerprint() string {
if r.Proxy == nil {
return ""
}

Copilot uses AI. Check for mistakes.
Comment on lines +91 to +103
// Always call WithProxy to ensure proxy env vars are managed correctly
// When proxy is nil, this will remove any existing proxy env vars
var httpProxy, httpsProxy, noProxy string
if opts.Proxy != nil {
httpProxy = opts.Proxy.HTTPProxy
httpsProxy = opts.Proxy.HTTPSProxy
noProxy = opts.Proxy.NoProxy
}
deploymentOpts := []ResourceCreatorOption{
WithDeploymentSpec(depSpec.Spec),
WithLabels(depSpec.Label),
WithProxy(httpProxy, httpsProxy, noProxy),
}
Copy link

Copilot AI Feb 13, 2026

Choose a reason for hiding this comment

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

The order of operations creates a potential issue where custom deployment configuration can override proxy environment variables. The proxy env vars are set on line 102 via WithProxy, but after the deployment resource is created, applyCustomConfigToDeployment is called (line 116 - outside the diff). The applyEnvironmentConfig function can override environment variables with the same name.

If a user's deployment config includes HTTP_PROXY, HTTPS_PROXY, or NO_PROXY in the env array, those values will override the proxy configuration from operator-controller. This creates inconsistent behavior where the proxy configuration might not be applied as expected.

Consider either:

  1. Applying proxy env vars after custom config (though this would prevent users from overriding if needed)
  2. Documenting this behavior clearly
  3. Checking if proxy vars exist in custom config and warning/erroring if there's a conflict

The current implementation silently allows overrides which could lead to confusing proxy configuration issues.

Copilot uses AI. Check for mistakes.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants