WIP 🐛 OCPBUGS-61082: Add HTTP proxy support for operator deployments #2501
Conversation
✅ Deploy Preview for olmv1 ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: The full list of commands accepted by this bot can be found here. DetailsNeeds approval from an approver in each of these files:Approvers can indicate their approval by writing |
There was a problem hiding this comment.
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.
internal/operator-controller/rukpak/render/registryv1/generators/resources.go
Show resolved
Hide resolved
tmshort
changed the title
openshift-ci
bot
added
the
do-not-merge/work-in-progress
tmshort
force-pushed
the
fix-OCPBUGS-61082
branch
from
3959ccf to
80e0d7e
Compare
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
tmshort
force-pushed
the
fix-OCPBUGS-61082
branch
from
80e0d7e to
9e92a12
Compare
There was a problem hiding this comment.
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.
| } | ||
|
|
||
| // Use strategic merge patch to add/update the env var | ||
| patch := fmt.Sprintf(`{"spec":{"template":{"spec":{"containers":[{"name":"manager","env":[{"name":"%s","value":"%s"}]}]}}}}`, envName, envValue) |
There was a problem hiding this comment.
The patch payload is built with fmt.Sprintf and directly interpolates envName/envValue into JSON. If either contains quotes/backslashes, the JSON becomes invalid and the patch will fail. Consider building the patch as a Go struct/map and json.Marshal it (or otherwise escape values).
| patch := fmt.Sprintf(`{"spec":{"template":{"spec":{"containers":[{"name":"manager","env":[{"name":"%s","value":"%s"}]}]}}}}`, envName, envValue) | |
| patchObj := map[string]interface{}{ | |
| "spec": map[string]interface{}{ | |
| "template": map[string]interface{}{ | |
| "spec": map[string]interface{}{ | |
| "containers": []map[string]interface{}{ | |
| { | |
| "name": "manager", | |
| "env": []map[string]interface{}{ | |
| { | |
| "name": envName, | |
| "value": envValue, | |
| }, | |
| }, | |
| }, | |
| }, | |
| }, | |
| }, | |
| }, | |
| } | |
| patchBytes, err := json.Marshal(patchObj) | |
| if err != nil { | |
| return fmt.Errorf("failed to marshal deployment patch: %w", err) | |
| } | |
| patch := string(patchBytes) |
| // Get the current proxy-reconcile annotation timestamp before waiting | ||
| initialProxyTime, err := k8sClient("get", "clusterextension", sc.clusterExtensionName, "-o", "jsonpath={.metadata.annotations.olm\\.operatorframework\\.io/proxy-reconcile}") | ||
| if err != nil { | ||
| // ClusterExtension might not exist yet, that's okay | ||
| initialProxyTime = "" | ||
| } | ||
| initialProxyTime = strings.TrimSpace(initialProxyTime) | ||
|
|
||
| // Wait for the startup hook to update the proxy-reconcile annotation to a new timestamp | ||
| // This indicates that operator-controller has restarted and triggered reconciliation | ||
| var proxyReconcileTime string | ||
| waitFor(ctx, func() bool { | ||
| raw, err := k8sClient("get", "clusterextension", sc.clusterExtensionName, "-o", "jsonpath={.metadata.annotations.olm\\.operatorframework\\.io/proxy-reconcile}") | ||
| if err != nil { | ||
| return false | ||
| } | ||
| newTime := strings.TrimSpace(raw) | ||
| if newTime == "" { | ||
| return false | ||
| } | ||
| // The annotation must be different from the initial value (updated to a new timestamp) | ||
| if newTime != initialProxyTime { | ||
| proxyReconcileTime = newTime |
There was a problem hiding this comment.
OperatorControllerEnvVarRemoved captures initialProxyTime after waiting for the operator-controller Deployment rollout and Pod readiness. The startup hook may have already updated the olm.operatorframework.io/proxy-reconcile annotation by then, so the subsequent wait for it to change can hang/flap. Capture the initial annotation value before triggering the restart, then wait for it to differ afterwards.
| // Check that ALL containers have the env var with the correct value | ||
| if len(dep.Spec.Template.Spec.Containers) == 0 { | ||
| return false | ||
| } | ||
| for _, container := range dep.Spec.Template.Spec.Containers { | ||
| found := false | ||
| for _, env := range container.Env { | ||
| if env.Name == envName && env.Value == envValue { | ||
| found = true | ||
| break | ||
| } | ||
| } | ||
| if !found { | ||
| return false | ||
| } | ||
| } |
There was a problem hiding this comment.
ResourceHasEnvVar asserts the env var exists in all regular containers, but it does not check InitContainers. If the implementation regresses and stops injecting proxy env vars into init containers, this step would still pass.
| if ext.Annotations == nil { | ||
| ext.Annotations = make(map[string]string) | ||
| } | ||
| ext.Annotations["olm.operatorframework.io/proxy-reconcile"] = time.Now().Format(time.RFC3339) |
There was a problem hiding this comment.
The startup reconciliation trigger sets olm.operatorframework.io/proxy-reconcile using time.RFC3339 (seconds resolution). If the controller restarts multiple times within the same second, the annotation value may not change and reconciliation may not be triggered. Using time.RFC3339Nano or a monotonic value like UnixNano() would avoid this edge case.
| ext.Annotations["olm.operatorframework.io/proxy-reconcile"] = time.Now().Format(time.RFC3339) | |
| ext.Annotations["olm.operatorframework.io/proxy-reconcile"] = time.Now().Format(time.RFC3339Nano) |
| phases, ok := rev["spec"].(map[string]interface{})["phases"].([]interface{}) | ||
| if !ok { | ||
| return fmt.Errorf("no phases found in revision") | ||
| } | ||
|
|
||
| for _, phase := range phases { | ||
| phaseMap := phase.(map[string]interface{}) | ||
| if phaseMap["kind"] != "Deployment" { | ||
| continue | ||
| } | ||
|
|
||
| // Get manifests from this phase | ||
| manifests, ok := phaseMap["manifests"].([]interface{}) |
There was a problem hiding this comment.
ClusterExtensionRevisionHasNoProxyEnvVars uses unchecked type assertions (e.g., rev["spec"].(map[string]interface{}) and phase.(map[string]interface{})). If the JSON shape differs (or fields are missing), this will panic and crash the e2e run. Please add ok checks for each assertion and return a helpful error when the expected structure is not present.
| phases, ok := rev["spec"].(map[string]interface{})["phases"].([]interface{}) | |
| if !ok { | |
| return fmt.Errorf("no phases found in revision") | |
| } | |
| for _, phase := range phases { | |
| phaseMap := phase.(map[string]interface{}) | |
| if phaseMap["kind"] != "Deployment" { | |
| continue | |
| } | |
| // Get manifests from this phase | |
| manifests, ok := phaseMap["manifests"].([]interface{}) | |
| specVal, ok := rev["spec"] | |
| if !ok { | |
| return fmt.Errorf("revision %s has no spec field", revisionName) | |
| } | |
| specMap, ok := specVal.(map[string]interface{}) | |
| if !ok { | |
| return fmt.Errorf("revision %s has unexpected spec structure (got %T)", revisionName, specVal) | |
| } | |
| phasesVal, found := specMap["phases"] | |
| if !found || phasesVal == nil { | |
| return fmt.Errorf("no phases found in revision %s", revisionName) | |
| } | |
| phases, ok := phasesVal.([]interface{}) | |
| if !ok { | |
| return fmt.Errorf("revision %s has phases field with unexpected type (got %T)", revisionName, phasesVal) | |
| } | |
| for _, phase := range phases { | |
| phaseMap, ok := phase.(map[string]interface{}) | |
| if !ok { | |
| return fmt.Errorf("revision %s has phase with unexpected structure (got %T)", revisionName, phase) | |
| } | |
| kindVal, ok := phaseMap["kind"].(string) | |
| if !ok || kindVal != "Deployment" { | |
| continue | |
| } | |
| // Get manifests from this phase | |
| manifestsVal, exists := phaseMap["manifests"] | |
| if !exists || manifestsVal == nil { | |
| continue | |
| } | |
| manifests, ok := manifestsVal.([]interface{}) |
| continue | ||
| } | ||
|
|
||
| // Decode base64 | ||
| decoded, err := base64.StdEncoding.DecodeString(manifestStr) | ||
| if err != nil { | ||
| continue |
There was a problem hiding this comment.
This step silently continues on unexpected manifest data types and base64 decode failures. That can produce false passes (skipping manifests entirely) and hide real problems. Consider returning an error when a manifest entry can't be decoded/processed so the test fails loudly.
| continue | |
| } | |
| // Decode base64 | |
| decoded, err := base64.StdEncoding.DecodeString(manifestStr) | |
| if err != nil { | |
| continue | |
| return fmt.Errorf("unexpected manifest data type %T in revision %s", manifestData, revisionName) | |
| } | |
| // Decode base64 | |
| decoded, err := base64.StdEncoding.DecodeString(manifestStr) | |
| if err != nil { | |
| return fmt.Errorf("failed to base64 decode manifest in revision %s: %w", revisionName, err) |
| .PHONY: e2e | ||
| e2e: #EXHELP Run the e2e tests. | ||
| go test -count=1 -v ./test/e2e/features_test.go | ||
| $(if $(GODOG_TAGS),go test -timeout=30m -count=1 -v ./test/e2e/features_test.go --godog.tags="$(GODOG_TAGS)",go test -timeout=30m -count=1 -v ./test/e2e/features_test.go) |
There was a problem hiding this comment.
The e2e target passes --godog.tags directly to go test, but go test will error on unknown flags unless they are passed after -args. This likely breaks make e2e whenever GODOG_TAGS is set.
| $(if $(GODOG_TAGS),go test -timeout=30m -count=1 -v ./test/e2e/features_test.go --godog.tags="$(GODOG_TAGS)",go test -timeout=30m -count=1 -v ./test/e2e/features_test.go) | |
| $(if $(GODOG_TAGS),go test -timeout=30m -count=1 -v ./test/e2e/features_test.go -args --godog.tags="$(GODOG_TAGS)",go test -timeout=30m -count=1 -v ./test/e2e/features_test.go) |
| return false | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
ResourceHasNoProxyEnvVars only checks dep.Spec.Template.Spec.Containers and ignores InitContainers, but proxy env vars are applied to init containers too. This can allow the step to pass even when init containers still have proxy vars.
| } | |
| } | |
| // Also ensure that NO init containers have proxy env vars | |
| for _, container := range dep.Spec.Template.Spec.InitContainers { | |
| for _, env := range container.Env { | |
| if env.Name == "HTTP_PROXY" || env.Name == "HTTPS_PROXY" || env.Name == "NO_PROXY" { | |
| return false | |
| } | |
| } | |
| } |
| // Check that NO containers have the env var | ||
| for _, container := range dep.Spec.Template.Spec.Containers { | ||
| for _, env := range container.Env { | ||
| if env.Name == envName { | ||
| return false | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
ResourceHasNoEnvVar only checks regular containers and ignores InitContainers. If the env var is present on init containers, this step will incorrectly succeed.
1 participant
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