Skip to content

OCPCLOUD-3465: feat: implement cluster-wide proxy support via new ProxyController #586

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

Open
tmshort wants to merge 1 commit into
base: main
Choose a base branch
from
Open

OCPCLOUD-3465: feat: implement cluster-wide proxy support via new ProxyController #586

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
27 changes: 27 additions & 0 deletions manifests-gen/customizations.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -162,12 +162,39 @@ func findWebhookServiceSecretName(objs []client.Object) map[string]string {
return serviceSecretNames
}

// capiOperatorDomain is the shared annotation/label domain for this operator.
// Mirrors operatorstatus.CAPIOperatorIdentifierDomain, which is not importable
// here because manifests-gen is a standalone module.
const (
capiOperatorDomain = "capi-operator.openshift.io"
proxyInjectAnnotation = capiOperatorDomain + "/inject-proxy"
)

func customizeDeployment(obj client.Object) (client.Object, error) {
deployment := &appsv1.Deployment{}
mustConvert(obj, deployment)

deployment.Spec.Template.Spec.PriorityClassName = "system-cluster-critical"

// Add the proxy injection annotation to the pod template if not already set
// by the provider. manifests-gen targets the "manager" container, which is
// the standard name for CAPI provider controller containers. Providers that
// use different container names, or need proxy injection on additional
// containers, should set this annotation in their upstream manifests.
if _, exists := deployment.Spec.Template.Annotations[proxyInjectAnnotation]; !exists {
for _, c := range deployment.Spec.Template.Spec.Containers {
if c.Name == "manager" {
if deployment.Spec.Template.Annotations == nil {
deployment.Spec.Template.Annotations = make(map[string]string)
}

deployment.Spec.Template.Annotations[proxyInjectAnnotation] = "manager"

break
}
}
}

for i := range deployment.Spec.Template.Spec.Containers {
container := &deployment.Spec.Template.Spec.Containers[i]
// Add resource requests
Expand Down
51 changes: 51 additions & 0 deletions pkg/controllers/installer/helpers_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -33,6 +33,7 @@ import (
"sigs.k8s.io/controller-runtime/pkg/client"
"sigs.k8s.io/controller-runtime/pkg/event"

proxycontroller "github.com/openshift/cluster-capi-operator/pkg/controllers/proxy"
"github.com/openshift/cluster-capi-operator/pkg/providerimages"
"github.com/openshift/cluster-capi-operator/pkg/revisiongenerator"
"github.com/openshift/cluster-capi-operator/pkg/test"
Expand All @@ -59,6 +60,9 @@ const (
providerIrregularCRD = "irregular-resource-crd"
providerAdoptExisting = "adopt-existing"
providerAdoptInvalid = "adopt-invalid"
// Proxy controller test providers.
providerProxyAnnotated = "proxy-annotated"
providerProxyNotAnnotated = "proxy-not-annotated"

coreCMName = "test-cm-core"
adoptCMName = "test-cm-adopt"
Expand Down Expand Up @@ -100,6 +104,41 @@ var (
providersByName map[string]providerimages.ProviderImageManifests
)

const proxyTestDeploymentName = "test-proxy-deployment"

// proxyDeploymentYAML returns a Deployment YAML with a "manager" container and,
// optionally, the inject-proxy annotation on the pod template.
func proxyDeploymentYAML(withAnnotation bool) string {
annotations := ""
if withAnnotation {
annotations = fmt.Sprintf(`
annotations:
%s: manager`, proxycontroller.ProxyInjectAnnotation)
}

return fmt.Sprintf(`apiVersion: apps/v1

@nrb nrb Aug 18, 2026

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.

What's the reason for using string templating for YAML rather than a builder and Go struct?

@tmshort tmshort Aug 19, 2026

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

It's following the convention of the other test utilities, specifically in https://github.com/openshift/cluster-capi-operator/blob/main/pkg/test/provider_fixtures.go, which return YAML strings.

kind: Deployment
metadata:
name: %s
namespace: default
spec:
replicas: 1
selector:
matchLabels:
app: %s
template:
metadata:
labels:
app: %s%s
spec:
containers:
- name: manager
image: registry.example.com/test:latest
- name: other
image: registry.example.com/test:latest`,
proxyTestDeploymentName, proxyTestDeploymentName, proxyTestDeploymentName, annotations)
}

// validatingAdmissionPolicyYAML generates a minimal ValidatingAdmissionPolicy YAML.
func validatingAdmissionPolicyYAML(name string) string {
return fmt.Sprintf(`apiVersion: admissionregistration.k8s.io/v1
Expand Down Expand Up @@ -228,12 +267,24 @@ func setupProviderProfiles() {
)).
Build()

// Provider "proxy-annotated": Deployment with the inject-proxy annotation on its pod template.
proxyAnnotated := test.NewProviderImageManifests(tb, providerProxyAnnotated).
WithManifests(proxyDeploymentYAML(true)).
Build()

// Provider "proxy-not-annotated": Same Deployment without the inject-proxy annotation,
// used to test that the proxy controller clears vars when the annotation is removed.
proxyNotAnnotated := test.NewProviderImageManifests(tb, providerProxyNotAnnotated).
WithManifests(proxyDeploymentYAML(false)).
Build()

allProviderProfiles = []providerimages.ProviderImageManifests{
core, infra, addon, coreV2, dupObj,
clusterScoped, clusterScoped2, crdProvider, nsProvider,
deploymentProvider, mixed, manyClusterScoped,
vapProvider, irregularCRDProvider,
adoptExisting, adoptInvalid,
proxyAnnotated, proxyNotAnnotated,
}

providersByName = make(map[string]providerimages.ProviderImageManifests, len(allProviderProfiles))
Expand Down
52 changes: 43 additions & 9 deletions pkg/controllers/installer/installer_controller.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -23,6 +23,7 @@ import (
"strings"

"github.com/go-logr/logr"
configv1 "github.com/openshift/api/config/v1"
operatorv1alpha1 "github.com/openshift/api/operator/v1alpha1"
operatorv1alpha1apply "github.com/openshift/client-go/operator/applyconfigurations/operator/v1alpha1"
apierrors "k8s.io/apimachinery/pkg/api/errors"
Expand All @@ -31,6 +32,7 @@ import (
"k8s.io/apimachinery/pkg/runtime/schema"
"k8s.io/apimachinery/pkg/selection"
"k8s.io/apimachinery/pkg/util/sets"
metav1applyconfig "k8s.io/client-go/applyconfigurations/meta/v1"
"k8s.io/client-go/discovery"
ctrl "sigs.k8s.io/controller-runtime"
"sigs.k8s.io/controller-runtime/pkg/builder"
Expand All @@ -44,6 +46,7 @@ import (
"pkg.package-operator.run/boxcutter"
"pkg.package-operator.run/boxcutter/managedcache"

"github.com/openshift/cluster-capi-operator/pkg/controllers/proxy"
"github.com/openshift/cluster-capi-operator/pkg/operatorstatus"
"github.com/openshift/cluster-capi-operator/pkg/providerimages"
"github.com/openshift/cluster-capi-operator/pkg/revisiongenerator"
Expand All @@ -66,30 +69,46 @@ type InstallerController struct {
revisionEngine *boxcutter.RevisionEngine
providerProfiles []providerimages.ProviderImageManifests
restMapper meta.RESTMapper
proxyReconciler *proxy.Controller
}

// SetupWithManager creates the boxcutter dependencies and sets up the installer
// controller with the Manager. Additional sources may be provided to trigger
// reconciliation from external events (e.g. a channel source for testing).
// controller with the Manager. The proxy sub-reconciler is wired in here so
// that both controllers share a single trackingCache.Source() registration,
// avoiding the double-registration shutdown issue. Additional sources may be
// provided to trigger reconciliation from external events (e.g. a channel
// source for testing).
func SetupWithManager(mgr ctrl.Manager, providerProfiles []providerimages.ProviderImageManifests, additionalSources ...source.Source) error {
trackingCache, err := setupTrackingCache(mgr)
if err != nil {
return fmt.Errorf("unable to setup tracking cache: %w", err)
}

revisionEngine, err := setupRevisionEngine(mgr, trackingCache)
revisionEngine, discoveryClient, err := setupRevisionEngine(mgr, trackingCache)
if err != nil {
return fmt.Errorf("unable to setup revision engine: %w", err)
}

extractor, err := metav1applyconfig.NewUnstructuredExtractor(discoveryClient)
if err != nil {
return fmt.Errorf("unable to create unstructured extractor: %w", err)
}

proxyReconciler := proxy.New(mgr.GetClient(), trackingCache, extractor)

c := &InstallerController{
client: mgr.GetClient(),
trackingCache: trackingCache,
revisionEngine: revisionEngine,
providerProfiles: providerProfiles,
restMapper: mgr.GetRESTMapper(),
proxyReconciler: proxyReconciler,
}

return setupController(mgr, c, additionalSources)
}

func setupController(mgr ctrl.Manager, c *InstallerController, additionalSources []source.Source) error {
toClusterAPI := func(_ context.Context, _ client.Object) []reconcile.Request {
return []reconcile.Request{{
NamespacedName: client.ObjectKey{Name: clusterAPIName},
Expand Down Expand Up @@ -122,7 +141,14 @@ func SetupWithManager(mgr ctrl.Manager, providerProfiles []providerimages.Provid
// predicates.
),
),
)
).
// Watch the cluster-wide Proxy CR so that proxy changes trigger
// reconciliation of proxy env vars on managed workloads.
Watches(&configv1.Proxy{},
handler.EnqueueRequestsFromMapFunc(toClusterAPI),
builder.WithPredicates(predicate.NewPredicateFuncs(func(obj client.Object) bool {
return obj.GetName() == "cluster"
})))

for _, src := range additionalSources {
b = b.WatchesRawSource(src)
Expand Down Expand Up @@ -163,10 +189,10 @@ func setupTrackingCache(mgr ctrl.Manager) (managedcache.TrackingCache, error) {
return trackingCache, nil
}

func setupRevisionEngine(mgr ctrl.Manager, trackingCache managedcache.TrackingCache) (*boxcutter.RevisionEngine, error) {
func setupRevisionEngine(mgr ctrl.Manager, trackingCache managedcache.TrackingCache) (*boxcutter.RevisionEngine, discovery.DiscoveryInterface, error) {
discoveryClient, err := discovery.NewDiscoveryClientForConfig(mgr.GetConfig())
if err != nil {
return nil, fmt.Errorf("unable to create discovery client: %w", err)
return nil, nil, fmt.Errorf("unable to create discovery client: %w", err)
}
Comment on lines 193 to 196

@mdbooth mdbooth Jul 15, 2026

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.

Re-using the discovery client is nice 👌


revisionEngine, err := boxcutter.NewRevisionEngine(boxcutter.RevisionEngineOptions{
Expand All @@ -180,13 +206,14 @@ func setupRevisionEngine(mgr ctrl.Manager, trackingCache managedcache.TrackingCa
UnfilteredReader: mgr.GetAPIReader(),
})
if err != nil {
return nil, fmt.Errorf("unable to create revision engine: %w", err)
return nil, nil, fmt.Errorf("unable to create revision engine: %w", err)
}

return revisionEngine, nil
return revisionEngine, discoveryClient, nil
}

// Reconcile handles applying and managing revisions on the cluster.
// Reconcile handles applying and managing revisions on the cluster, and calls
// the proxy sub-reconciler to keep proxy env vars on managed workloads current.
func (c *InstallerController) Reconcile(ctx context.Context, _ ctrl.Request) (ctrl.Result, error) {
log := ctrl.LoggerFrom(ctx).WithName(controllerName)
log.Info("Reconciling installer revisions")
Expand All @@ -197,6 +224,13 @@ func (c *InstallerController) Reconcile(ctx context.Context, _ ctrl.Request) (ct
return ctrl.Result{}, fmt.Errorf("failed to write conditions: %w", err)
}

// TODO: the proxy reconciler needs its own status reporting mechanism.
// For now, log errors without propagating them so that proxy failures
// do not affect the installer controller's ClusterOperator conditions.
if err := c.proxyReconciler.Reconcile(ctx); err != nil {
log.Error(err, "Failed to reconcile proxy env vars")
}

log.Info("Reconcile finished")

@mdbooth mdbooth Jul 15, 2026

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.

I need to think about this, because the proxy reconciler needs to report its status somewhere. I don't mind it being integrated with the installer controller's status, but we need to think about how to do that.


return reconcileResult.Result()
Expand Down
Loading