-
Notifications
You must be signed in to change notification settings - Fork 121
CNTRLPLANE-3762: Add support for component-scoped proxy #946
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.
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
tchap
wants to merge
9
commits into
openshift:master
Choose a base branch
from
tchap:disconnected-env-review
base: master
Could not load branches
Branch not found: {{ refName }}
Loading
Could not load tags
Nothing to show
Loading
Are you sure you want to change the base?
Some commits from the old base branch may be removed from the timeline,
and old review comments may become outdated.
Open
Changes from all commits
Commits
Show all changes
9 commits
Select commit
Hold shift + click to select a range
bfc1b89
transport: Add TransportForCARefWithProxy
tchap cd22c77
Add proxy resolution helpers
tchap 7ee06c7
Add component-scoped proxy to all controllers
tchap 6e856a3
Align integration tests
tchap ccddfe3
Add shared test helpers and improve test coverage
tchap 053f60f
Make mergeNoProxy output deterministic and remove duplicate test
tchap b52e4de
Load proxy trustedCA into endpoint accessible controller TLS config
tchap 9a6dc13
Add KUBERNETES_SERVICE_HOST to component proxy NO_PROXY entries
tchap dbef886
Centralize proxy resolution and transport construction via ProxyResolver
tchap File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,114 @@ | ||
| package common | ||
|
|
||
| import ( | ||
| "fmt" | ||
| "os" | ||
| "strings" | ||
|
|
||
| "golang.org/x/net/http/httpproxy" | ||
|
|
||
| "k8s.io/apimachinery/pkg/api/errors" | ||
| "k8s.io/apimachinery/pkg/util/sets" | ||
|
|
||
| "github.com/openshift/api/features" | ||
| operatorv1 "github.com/openshift/api/operator/v1" | ||
| operatorv1listers "github.com/openshift/client-go/operator/listers/operator/v1" | ||
| "github.com/openshift/library-go/pkg/operator/configobserver/featuregates" | ||
|
|
||
| "github.com/openshift/cluster-authentication-operator/pkg/transport" | ||
| ) | ||
|
|
||
| // ResolvedProxy holds the effective proxy configuration for authentication components. | ||
| type ResolvedProxy struct { | ||
| *httpproxy.Config | ||
| TrustedCAName string | ||
| } | ||
|
|
||
| func (p *ResolvedProxy) IsProxyConfigured() bool { | ||
| return len(p.HTTPProxy) != 0 || len(p.HTTPSProxy) != 0 | ||
| } | ||
|
|
||
| // ProxyFunc returns a function that resolves the proxy URL for a given request URL. | ||
| func (p *ResolvedProxy) ProxyFunc() transport.ProxyFunc { | ||
| return transport.ProxyFunc(p.Config.ProxyFunc()) | ||
| } | ||
|
|
||
| // ResolveProxy reads the component-scoped proxy from the Authentication | ||
| // operator CR (when the feature gate is enabled) and returns the effective proxy | ||
| // settings. When no component proxy is configured, it falls back to the process | ||
| // environment (which reflects the cluster-wide proxy). | ||
| func ResolveProxy( | ||
| featureGateAccessor featuregates.FeatureGateAccess, | ||
| operatorAuthLister operatorv1listers.AuthenticationLister, | ||
| ) (*ResolvedProxy, error) { | ||
| authProxy, err := getComponentProxyConfig(featureGateAccessor, operatorAuthLister) | ||
| if err != nil { | ||
| return nil, err | ||
| } | ||
|
|
||
| if authProxy != nil { | ||
| return &ResolvedProxy{ | ||
| TrustedCAName: authProxy.TrustedCA.Name, | ||
| Config: &httpproxy.Config{ | ||
| HTTPProxy: authProxy.HTTPProxy, | ||
| HTTPSProxy: authProxy.HTTPSProxy, | ||
| NoProxy: mergeNoProxy(authProxy.NoProxy), | ||
| }, | ||
| }, nil | ||
| } | ||
|
|
||
| return &ResolvedProxy{ | ||
| Config: httpproxy.FromEnvironment(), | ||
| }, nil | ||
| } | ||
|
|
||
| // getComponentProxyConfig returns the component-scoped proxy configuration | ||
| // from operator.openshift.io/v1 Authentication if the feature gate is enabled. | ||
| // Returns (nil, nil) when the gate is disabled or the resource is not found. | ||
| func getComponentProxyConfig( | ||
| featureGateAccessor featuregates.FeatureGateAccess, | ||
| operatorAuthLister operatorv1listers.AuthenticationLister, | ||
| ) (*operatorv1.AuthenticationProxyConfig, error) { | ||
| featureGates, err := featureGateAccessor.CurrentFeatureGates() | ||
| if err != nil { | ||
| return nil, fmt.Errorf("failed to get current feature gates: %w", err) | ||
| } | ||
| if !featureGates.Enabled(features.FeatureGateAuthenticationComponentProxy) { | ||
| return nil, nil | ||
| } | ||
|
|
||
| authOp, err := operatorAuthLister.Get("cluster") | ||
| if err != nil { | ||
| if errors.IsNotFound(err) { | ||
| return nil, nil | ||
| } | ||
| return nil, fmt.Errorf("failed to get operator.openshift.io/v1 authentication/cluster: %w", err) | ||
| } | ||
|
|
||
| proxy := authOp.Spec.Proxy | ||
| if proxy.HTTPProxy == "" && proxy.HTTPSProxy == "" { | ||
| return nil, nil | ||
| } | ||
| return &proxy, nil | ||
| } | ||
|
|
||
| // staticNoProxyEntries contains cluster-internal addresses that must bypass the | ||
| // proxy. Auth components connect to internal services via DNS names covered by | ||
| // .svc and .cluster.local. The kubernetes API service IP (KUBERNETES_SERVICE_HOST) | ||
| // is included explicitly because Go's in-cluster client connects to it by raw IP, | ||
| // which does not match the hostname-based entries above. | ||
| var staticNoProxyEntries = func() []string { | ||
| entries := []string{".cluster.local", ".svc", "127.0.0.1", "localhost"} | ||
| if host := os.Getenv("KUBERNETES_SERVICE_HOST"); host != "" { | ||
| entries = append(entries, host) | ||
| } | ||
| return entries | ||
| }() | ||
|
|
||
| // mergeNoProxy combines user-provided noProxy entries with static cluster-internal | ||
| // defaults. The result is deduplicated and sorted for deterministic output. | ||
| func mergeNoProxy(userNoProxy []string) string { | ||
| entries := sets.New[string](staticNoProxyEntries...) | ||
| entries.Insert(userNoProxy...) | ||
| return strings.Join(sets.List(entries), ",") | ||
| } | ||
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,191 @@ | ||
| package common | ||
|
|
||
| import ( | ||
| "crypto/tls" | ||
| "crypto/x509" | ||
| "fmt" | ||
| "net/http" | ||
| "net/url" | ||
|
|
||
| "golang.org/x/net/http/httpproxy" | ||
|
|
||
| knet "k8s.io/apimachinery/pkg/util/net" | ||
| corelistersv1 "k8s.io/client-go/listers/core/v1" | ||
| "k8s.io/client-go/tools/cache" | ||
|
|
||
| operatorv1informers "github.com/openshift/client-go/operator/informers/externalversions/operator/v1" | ||
| operatorv1listers "github.com/openshift/client-go/operator/listers/operator/v1" | ||
| "github.com/openshift/library-go/pkg/operator/configobserver/featuregates" | ||
|
|
||
| "github.com/openshift/cluster-authentication-operator/pkg/transport" | ||
| ) | ||
|
|
||
| // transportConfig holds the cert pool being assembled by TransportOptions. | ||
| type transportConfig struct { | ||
| pool *x509.CertPool | ||
| } | ||
|
|
||
| func (c *transportConfig) appendToPool(name string, data []byte) error { | ||
| if c.pool == nil { | ||
| c.pool = x509.NewCertPool() | ||
| } | ||
| if ok := c.pool.AppendCertsFromPEM(data); !ok { | ||
| return fmt.Errorf("unable to parse CA bundle from %s", name) | ||
| } | ||
| return nil | ||
| } | ||
|
|
||
| // TransportOption configures the trust pool for NewTransport. | ||
| type TransportOption func(cfg *transportConfig, cmLister corelistersv1.ConfigMapLister) error | ||
|
|
||
| // WithCertPool provides a pre-built certificate pool as the base trust | ||
| // pool. The proxy's trusted CA (if any) is appended to this pool. | ||
| // This avoids re-parsing CA bundles when multiple transports share the | ||
| // same trust anchors. | ||
| func WithCertPool(pool *x509.CertPool) TransportOption { | ||
| return func(cfg *transportConfig, _ corelistersv1.ConfigMapLister) error { | ||
| cfg.pool = pool | ||
| return nil | ||
| } | ||
| } | ||
|
|
||
| // WithCA appends raw CA certificate bytes to the transport's trust pool. | ||
| // The name identifies the source for error messages. | ||
| func WithCA(name string, data []byte) TransportOption { | ||
| return func(cfg *transportConfig, _ corelistersv1.ConfigMapLister) error { | ||
| if len(data) == 0 { | ||
| return nil | ||
| } | ||
| return cfg.appendToPool(name, data) | ||
| } | ||
| } | ||
|
|
||
| // WithCAFromConfigMap loads a CA bundle from a ConfigMap in the openshift-config | ||
| // namespace and appends it to the transport's trust pool. | ||
| func WithCAFromConfigMap(name, key string) TransportOption { | ||
| return func(cfg *transportConfig, cmLister corelistersv1.ConfigMapLister) error { | ||
| if len(name) == 0 { | ||
| return nil | ||
| } | ||
| data, err := transport.LoadCAData(cmLister, name, key) | ||
| if err != nil { | ||
| return err | ||
| } | ||
| return cfg.appendToPool(fmt.Sprintf("configmap %q", name), data) | ||
| } | ||
| } | ||
|
|
||
| // ProxyResolver resolves the effective proxy configuration and builds | ||
| // pre-configured HTTP transports for outbound requests. | ||
| type ProxyResolver interface { | ||
| ResolveProxy() (*ResolvedProxy, error) | ||
|
|
||
| // NewTransport returns an *http.Transport with proxy settings, the | ||
| // proxy's trusted CA, and any additional CA sources merged into the | ||
| // trust pool. The transport is initialized with knet.SetTransportDefaults. | ||
| NewTransport(opts ...TransportOption) (*http.Transport, error) | ||
| } | ||
|
|
||
| // AuthProxyResolver implements ProxyResolver by reading the component proxy | ||
| // from the Authentication operator CR (when the feature gate is enabled) and | ||
| // falling back to process-level environment variables. | ||
| type AuthProxyResolver struct { | ||
| operatorAuthInformer cache.SharedIndexInformer | ||
| operatorAuthLister operatorv1listers.AuthenticationLister | ||
| configMapLister corelistersv1.ConfigMapLister | ||
| featureGateAccessor featuregates.FeatureGateAccess | ||
| } | ||
|
|
||
| func NewAuthProxyResolver( | ||
| operatorAuth operatorv1informers.AuthenticationInformer, | ||
| configMapLister corelistersv1.ConfigMapLister, | ||
| featureGateAccessor featuregates.FeatureGateAccess, | ||
| ) AuthProxyResolver { | ||
| return AuthProxyResolver{ | ||
| operatorAuthInformer: operatorAuth.Informer(), | ||
| operatorAuthLister: operatorAuth.Lister(), | ||
| configMapLister: configMapLister, | ||
| featureGateAccessor: featureGateAccessor, | ||
| } | ||
| } | ||
|
|
||
| // NewAuthProxyResolverFromListers creates an AuthProxyResolver directly from | ||
| // listers, without requiring informers. This is intended for tests. | ||
| func NewAuthProxyResolverFromListers( | ||
| operatorAuthLister operatorv1listers.AuthenticationLister, | ||
| configMapLister corelistersv1.ConfigMapLister, | ||
| featureGateAccessor featuregates.FeatureGateAccess, | ||
| ) AuthProxyResolver { | ||
| return AuthProxyResolver{ | ||
| operatorAuthLister: operatorAuthLister, | ||
| configMapLister: configMapLister, | ||
| featureGateAccessor: featureGateAccessor, | ||
| } | ||
| } | ||
|
|
||
| func (r *AuthProxyResolver) Informer() cache.SharedIndexInformer { | ||
| return r.operatorAuthInformer | ||
| } | ||
|
|
||
| func (r *AuthProxyResolver) ResolveProxy() (*ResolvedProxy, error) { | ||
| return ResolveProxy(r.featureGateAccessor, r.operatorAuthLister) | ||
| } | ||
|
|
||
| func (r *AuthProxyResolver) NewTransport(opts ...TransportOption) (*http.Transport, error) { | ||
| proxy, err := r.ResolveProxy() | ||
| if err != nil { | ||
| return nil, err | ||
| } | ||
|
|
||
| var cfg transportConfig | ||
| for _, opt := range opts { | ||
| if err := opt(&cfg, r.configMapLister); err != nil { | ||
| return nil, err | ||
| } | ||
| } | ||
|
|
||
| if len(proxy.TrustedCAName) > 0 { | ||
| proxyCA, err := transport.LoadCAData(r.configMapLister, proxy.TrustedCAName, "ca-bundle.crt") | ||
| if err != nil { | ||
| return nil, err | ||
| } | ||
| if err := cfg.appendToPool(fmt.Sprintf("proxy trustedCA %q", proxy.TrustedCAName), proxyCA); err != nil { | ||
| return nil, err | ||
| } | ||
| } | ||
|
|
||
| tr := knet.SetTransportDefaults(&http.Transport{ | ||
| TLSClientConfig: &tls.Config{ | ||
| RootCAs: cfg.pool, | ||
| }, | ||
| }) | ||
|
|
||
| if proxy.IsProxyConfigured() { | ||
| proxyCfg := httpproxy.Config{ | ||
| HTTPProxy: proxy.HTTPProxy, | ||
| HTTPSProxy: proxy.HTTPSProxy, | ||
| NoProxy: proxy.NoProxy, | ||
| } | ||
| proxyFunc := proxyCfg.ProxyFunc() | ||
| tr.Proxy = func(req *http.Request) (*url.URL, error) { | ||
| return proxyFunc(req.URL) | ||
| } | ||
| } | ||
|
|
||
| return tr, nil | ||
| } | ||
|
|
||
| // FakeProxyResolver is a test helper implementing ProxyResolver. | ||
| type FakeProxyResolver struct { | ||
| Proxy *ResolvedProxy | ||
| Transport *http.Transport | ||
| Err error | ||
| } | ||
|
|
||
| func (f *FakeProxyResolver) ResolveProxy() (*ResolvedProxy, error) { | ||
| return f.Proxy, f.Err | ||
| } | ||
|
|
||
| func (f *FakeProxyResolver) NewTransport(opts ...TransportOption) (*http.Transport, error) { | ||
| return f.Transport, f.Err | ||
| } |
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Having gone through a lot of the code changes here, I wonder if making this a struct that contains a method equivalent to this
ResolveProxyfunction would be a better path that reduces the amount of changes a bit further?As an example, as part of the external OIDC work, we added
cluster-authentication-operator/pkg/controllers/common/external_oidc.go
Lines 23 to 49 in 8c68999
Which allowed us to minimally change function signatures to accept that new struct (or an interface that struct satisfies - which is my preference for mocking in unit tests) and avoid having to make changes to things like the listers struct to add the feature gate accessor. Instead, we instantiate it once in
starter.golikecluster-authentication-operator/pkg/operator/starter.go
Lines 138 to 144 in 8c68999
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I will look into this probably when I finish some of the e2e tests, too much context switching.
I agree this would be nice. I probably ended up with the messy approach because I was implementing things gradually and didn't get to the final refactoring.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I refactored the changes in dbef886