Skip to content

Commit 78b5407

Browse files
danehansdanehans
committed
Addresses bparees and dcarr feedback
1 parent 488b7fb commit 78b5407

4 files changed

Lines changed: 72 additions & 75 deletions

File tree

‎pkg/controller/proxyconfig/controller.go‎

Lines changed: 42 additions & 37 deletions
Original file line numberDiff line numberDiff line change
@@ -55,30 +55,30 @@ func add(mgr manager.Manager, r reconcile.Reconciler) error {
5555
// so filter events before they are provided to the controller event handlers.
5656
pred := predicate.Funcs{
5757
UpdateFunc: func(e event.UpdateEvent) bool {
58-
return e.MetaOld.GetName() == names.MERGED_TRUST_BUNDLE_CONFIGMAP &&
58+
return e.MetaOld.GetName() == names.TRUST_BUNDLE_CONFIGMAP &&
5959
e.MetaOld.GetNamespace() == names.TRUST_BUNDLE_CONFIGMAP_NS
6060
},
6161
DeleteFunc: func(e event.DeleteEvent) bool {
62-
return e.Meta.GetName() == names.MERGED_TRUST_BUNDLE_CONFIGMAP &&
62+
return e.Meta.GetName() == names.TRUST_BUNDLE_CONFIGMAP &&
6363
e.Meta.GetNamespace() == names.TRUST_BUNDLE_CONFIGMAP_NS
6464
},
6565
CreateFunc: func(e event.CreateEvent) bool {
66-
return e.Meta.GetName() == names.MERGED_TRUST_BUNDLE_CONFIGMAP &&
66+
return e.Meta.GetName() == names.TRUST_BUNDLE_CONFIGMAP &&
6767
e.Meta.GetNamespace() == names.TRUST_BUNDLE_CONFIGMAP_NS
6868
},
6969
GenericFunc: func(e event.GenericEvent) bool {
70-
return e.Meta.GetName() == names.MERGED_TRUST_BUNDLE_CONFIGMAP &&
70+
return e.Meta.GetName() == names.TRUST_BUNDLE_CONFIGMAP &&
7171
e.Meta.GetNamespace() == names.TRUST_BUNDLE_CONFIGMAP_NS
7272
},
7373
}
7474

75-
// Watch for changes to the user/system merged configmap
75+
// Watch for changes to the trust bundle configmap.
7676
err = c.Watch(&source.Kind{Type: &corev1.ConfigMap{}}, &handler.EnqueueRequestForObject{}, pred)
7777
if err != nil {
7878
return err
7979
}
8080

81-
// Watch for changes to resource config.openshift.io/v1/Proxy
81+
// Watch for changes to the proxy resource.
8282
err = c.Watch(&source.Kind{Type: &configv1.Proxy{}}, &handler.EnqueueRequestForObject{})
8383
if err != nil {
8484
return err
@@ -87,8 +87,6 @@ func add(mgr manager.Manager, r reconcile.Reconciler) error {
8787
return nil
8888
}
8989

90-
//var _ reconcile.Reconciler = &ReconcileProxyConfig{}
91-
9290
// ReconcileProxyConfig reconciles a Proxy object
9391
type ReconcileProxyConfig struct {
9492
// This client, initialized using mgr.Client() above, is a split client
@@ -98,16 +96,19 @@ type ReconcileProxyConfig struct {
9896
status *statusmanager.StatusManager
9997
}
10098

101-
// Reconcile expects request to refer to a proxy object named "cluster" in the
102-
// default namespace, and will ensure proxy is in the desired state.
99+
// Reconcile expects request to refer to a proxy object named "cluster"
100+
// in the default namespace or to a configmap object named
101+
// "trusted-ca-bundle" in namespace "openshift-config-managed", and will
102+
// ensure the proxy object is in the desired state.
103103
func (r *ReconcileProxyConfig) Reconcile(request reconcile.Request) (reconcile.Result, error) {
104-
// Collect required config objects for proxy reconciliation.
105-
proxyConfig := &configv1.Proxy{}
106-
infraConfig := &configv1.Infrastructure{}
107-
netConfig := &configv1.Network{}
108-
clusterConfig := &corev1.ConfigMap{}
109104
switch {
110-
case request.NamespacedName == names.ProxyName():
105+
case request.NamespacedName == names.Proxy():
106+
// Collect required config objects for proxy reconciliation.
107+
proxyConfig := &configv1.Proxy{}
108+
infraConfig := &configv1.Infrastructure{}
109+
netConfig := &configv1.Network{}
110+
clusterConfig := &corev1.ConfigMap{}
111+
111112
log.Printf("Reconciling proxy: %s\n", request.Name)
112113
err := r.client.Get(context.TODO(), request.NamespacedName, proxyConfig)
113114
if err != nil {
@@ -122,39 +123,43 @@ func (r *ReconcileProxyConfig) Reconcile(request reconcile.Request) (reconcile.R
122123
}
123124

124125
// A nil proxy is generated by upgrades and installs not requiring a proxy.
126+
validate := true
125127
if !isSpecHTTPProxySet(&proxyConfig.Spec) && !isSpecHTTPSProxySet(&proxyConfig.Spec) {
126-
log.Printf("httpProxy and httpsProxy not defined; reconciliation will be skipped for proxy: %s\n",
128+
log.Printf("httpProxy and httpsProxy not defined; validation will be skipped for proxy: %s\n",
127129
request.Name)
128-
return reconcile.Result{}, nil
130+
validate = false
129131
}
130132

131133
// Only proceed if the required config objects can be collected.
132-
if err := r.client.Get(context.TODO(), types.NamespacedName{Name: "cluster"}, infraConfig); err != nil {
133-
return reconcile.Result{}, fmt.Errorf("failed to get infrastructure config 'cluster': %v", err)
134-
}
135-
if err := r.client.Get(context.TODO(), types.NamespacedName{Name: "cluster"}, netConfig); err != nil {
136-
log.Printf("failed to get network config 'cluster': %v", err)
137-
return reconcile.Result{}, err
138-
}
139-
if err := r.client.Get(context.TODO(), types.NamespacedName{Name: "cluster-config-v1", Namespace: "kube-system"}, clusterConfig); err != nil {
140-
log.Printf("failed to get configmap 'cluster': %v", err)
141-
return reconcile.Result{}, err
142-
}
143-
if err := r.ValidateProxyConfig(&proxyConfig.Spec); err != nil {
144-
log.Printf("Failed to validate Proxy.Spec: %v", err)
145-
r.status.SetDegraded(statusmanager.ProxyConfig, "InvalidProxyConfig",
146-
fmt.Sprintf("The proxy configuration is invalid (%v). Use 'oc edit proxy.config.openshift.io cluster' to fix.", err))
147-
return reconcile.Result{}, nil
134+
if validate {
135+
if err := r.client.Get(context.TODO(), types.NamespacedName{Name: "cluster"}, infraConfig); err != nil {
136+
return reconcile.Result{}, fmt.Errorf("failed to get infrastructure config 'cluster': %v", err)
137+
}
138+
if err := r.client.Get(context.TODO(), types.NamespacedName{Name: "cluster"}, netConfig); err != nil {
139+
log.Printf("failed to get network config 'cluster': %v", err)
140+
return reconcile.Result{}, err
141+
}
142+
if err := r.client.Get(context.TODO(), types.NamespacedName{Name: "cluster-config-v1", Namespace: "kube-system"}, clusterConfig); err != nil {
143+
log.Printf("failed to get configmap 'cluster': %v", err)
144+
return reconcile.Result{}, err
145+
}
146+
if err := r.ValidateProxyConfig(&proxyConfig.Spec); err != nil {
147+
log.Printf("Failed to validate Proxy.Spec: %v", err)
148+
r.status.SetDegraded(statusmanager.ProxyConfig, "InvalidProxyConfig",
149+
fmt.Sprintf("The proxy configuration is invalid (%v). Use 'oc edit proxy.config.openshift.io cluster' to fix.", err))
150+
return reconcile.Result{}, nil
151+
}
148152
}
149-
// Update Proxy.config.openshift.io.Status
153+
154+
// Update proxy status.
150155
if err := r.syncProxyStatus(proxyConfig, infraConfig, netConfig, clusterConfig); err != nil {
151156
log.Printf("Could not sync proxy status: %v", err)
152157
r.status.SetDegraded(statusmanager.ProxyConfig, "StatusError",
153158
fmt.Sprintf("Could not update proxy configuration status: %v", err))
154159
return reconcile.Result{}, err
155160
}
156161
log.Printf("Reconciling proxy: %s complete\n", request.Name)
157-
case request.NamespacedName == names.MergedTrustBundleName():
162+
case request.NamespacedName == names.TrustBundleConfigMap():
158163
cfgMap := &corev1.ConfigMap{}
159164
log.Printf("Reconciling configmap: %s/%s\n", request.Namespace, request.Name)
160165
err := r.client.Get(context.TODO(), request.NamespacedName, cfgMap)
@@ -210,7 +215,7 @@ func isSpecTrustedCASet(proxyConfig *configv1.ProxySpec) bool {
210215
// isTrustedCAConfigMap returns true if the ConfigMap name in
211216
// spec.trustedCA is "proxy-ca-bundle".
212217
func isTrustedCAConfigMap(proxyConfig *configv1.ProxySpec) bool {
213-
return proxyConfig.TrustedCA.Name == names.MERGED_TRUST_BUNDLE_CONFIGMAP
218+
return proxyConfig.TrustedCA.Name == names.TRUST_BUNDLE_CONFIGMAP
214219
}
215220

216221
// isSpecReadinessEndpoints returns true if spec.readinessEndpoints of

‎pkg/controller/proxyconfig/status.go‎

Lines changed: 6 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -46,8 +46,7 @@ func (r *ReconcileProxyConfig) syncProxyStatus(proxy *configv1.Proxy, infra *con
4646
return nil
4747
}
4848

49-
// mergeUserSystemNoProxy returns noProxyWildcard if it supplied as a noProxy setting.
50-
// Otherwise, mergeUserSystemNoProxy merges user-supplied noProxy settings from proxy
49+
// mergeUserSystemNoProxy merges user-supplied noProxy settings from proxy
5150
// with cluster-wide noProxy settings, returning a merged, comma-separated
5251
// string of noProxy settings.
5352
func mergeUserSystemNoProxy(proxy *configv1.Proxy, infra *configv1.Infrastructure, network *configv1.Network, cluster *corev1.ConfigMap) (string, error) {
@@ -68,9 +67,8 @@ func mergeUserSystemNoProxy(proxy *configv1.Proxy, infra *configv1.Infrastructur
6867
apiServerURL.Hostname(),
6968
internalAPIServer.Hostname(),
7069
)
71-
platform := infra.Status.PlatformStatus.Type
7270

73-
// TODO: Does a better way exist to get machineCIDR and control-plane replicas?
71+
// TODO: This will be flexible when master machine management is more dynamic.
7472
type installConfig struct {
7573
ControlPlane struct {
7674
Replicas string `json:"replicas"`
@@ -88,10 +86,11 @@ func mergeUserSystemNoProxy(proxy *configv1.Proxy, infra *configv1.Infrastructur
8886
return "", fmt.Errorf("invalid install-config: %v\njson:\n%s", err, data)
8987
}
9088

91-
if platform != configv1.VSpherePlatformType &&
92-
platform != configv1.NonePlatformType &&
93-
platform != configv1.BareMetalPlatformType {
89+
switch infra.Status.PlatformStatus.Type {
90+
case configv1.AWSPlatformType:
9491
set.Insert("169.254.169.254", ic.Networking.MachineCIDR)
92+
default:
93+
return "", fmt.Errorf("unsupported infrastructure provider: %s", infra.Status.PlatformStatus.Type)
9594
}
9695

9796
replicas, err := strconv.Atoi(ic.ControlPlane.Replicas)

‎pkg/controller/proxyconfig/validation.go‎

Lines changed: 16 additions & 22 deletions
Original file line numberDiff line numberDiff line change
@@ -4,7 +4,6 @@ import (
44
"context"
55
"crypto/tls"
66
"crypto/x509"
7-
"errors"
87
"fmt"
98
"net"
109
"net/http"
@@ -32,6 +31,7 @@ const (
3231

3332
// ValidateProxyConfig ensures that proxyConfig is valid.
3433
func (r *ReconcileProxyConfig) ValidateProxyConfig(proxyConfig *configv1.ProxySpec) error {
34+
var err error
3535
if isSpecHTTPProxySet(proxyConfig) {
3636
scheme, err := cnovalidation.URI(proxyConfig.HTTPProxy)
3737
if err != nil {
@@ -42,26 +42,11 @@ func (r *ReconcileProxyConfig) ValidateProxyConfig(proxyConfig *configv1.ProxySp
4242
}
4343
}
4444

45-
readinessCerts := []*x509.Certificate{}
4645
if isSpecHTTPSProxySet(proxyConfig) {
47-
scheme, err := cnovalidation.URI(proxyConfig.HTTPSProxy)
46+
_, err := cnovalidation.URI(proxyConfig.HTTPSProxy)
4847
if err != nil {
4948
return fmt.Errorf("invalid httpsProxy URI: %v", err)
5049
}
51-
if scheme == proxyHTTPSScheme {
52-
// A trusted CA bundle must be provided when using https
53-
// between client and proxy.
54-
if !isSpecTrustedCASet(proxyConfig) {
55-
return errors.New("trustedCA is required when using an https scheme with httpsProxy")
56-
}
57-
certBundle, err := r.validateTrustedCA(proxyConfig)
58-
if err != nil {
59-
return fmt.Errorf("failed validating TrustedCA %q: %v", proxyConfig.TrustedCA.Name, err)
60-
}
61-
for _, cert := range certBundle {
62-
readinessCerts = append(readinessCerts, cert)
63-
}
64-
}
6550
}
6651

6752
if isSpecNoProxySet(proxyConfig) {
@@ -77,6 +62,14 @@ func (r *ReconcileProxyConfig) ValidateProxyConfig(proxyConfig *configv1.ProxySp
7762
}
7863
}
7964

65+
var certBundle []*x509.Certificate
66+
if isSpecTrustedCASet(proxyConfig) {
67+
certBundle, err = r.validateTrustedCA(proxyConfig)
68+
if err != nil {
69+
return fmt.Errorf("failed to validate TrustedCA %q: %v", proxyConfig.TrustedCA.Name, err)
70+
}
71+
}
72+
8073
if isSpecReadinessEndpoints(proxyConfig) {
8174
for _, endpoint := range proxyConfig.ReadinessEndpoints {
8275
scheme, err := cnovalidation.URI(endpoint)
@@ -92,7 +85,7 @@ func (r *ReconcileProxyConfig) ValidateProxyConfig(proxyConfig *configv1.ProxySp
9285
if !isSpecTrustedCASet(proxyConfig) {
9386
return fmt.Errorf("readinessEndpoint with an %q scheme requires trustedCA to be set", proxyHTTPSScheme)
9487
}
95-
if err := validateHTTPSReadinessEndpoint(readinessCerts, endpoint); err != nil {
88+
if err := validateHTTPSReadinessEndpoint(certBundle, endpoint); err != nil {
9689
return fmt.Errorf("readinessEndpoint probe failed for endpoint %s", endpoint)
9790
}
9891
default:
@@ -156,13 +149,14 @@ func validateHTTPSReadinessEndpoint(certBundle []*x509.Certificate, endpoint str
156149
// by using certBundle as trusted CAs to create a TLS connection using a
157150
// finite loop based on retries, returning an error if it never succeeds.
158151
func validateHTTPSReadinessEndpointWithRetries(certBundle []*x509.Certificate, endpoint string, retries int) error {
152+
var err error
159153
for i := 0; i < retries; i++ {
160-
if err := runHTTPSReadinessProbe(certBundle, endpoint); err != nil {
161-
return err
154+
if err := runHTTPSReadinessProbe(certBundle, endpoint); err == nil {
155+
return nil
162156
}
163157
}
164158

165-
return nil
159+
return err
166160
}
167161

168162
// runHTTPSReadinessProbe tries connecting to endpoint by using certBundle as
@@ -216,7 +210,7 @@ func (r *ReconcileProxyConfig) validateTrustedCAConfigMap(proxyConfig *configv1.
216210
return nil, fmt.Errorf("invalid ConfigMap reference for TrustedCA: %s", proxyConfig.TrustedCA.Name)
217211
}
218212
cfgMap := &corev1.ConfigMap{}
219-
if err := r.client.Get(context.TODO(), names.MergedTrustBundleName(), cfgMap); err != nil {
213+
if err := r.client.Get(context.TODO(), names.TrustBundleConfigMap(), cfgMap); err != nil {
220214
return nil, err
221215
}
222216

‎pkg/names/names.go‎

Lines changed: 8 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -41,29 +41,28 @@ const MULTUS_VALIDATING_WEBHOOK = "multus.openshift.io"
4141
// the PEM encoded trust bundle.
4242
const TRUST_BUNDLE_CONFIGMAP_KEY = "ca-bundle.crt"
4343

44-
// MERGED_TRUST_BUNDLE_CONFIGMAP is the name of the ConfigMap
44+
// TRUST_BUNDLE_CONFIGMAP is the name of the ConfigMap
4545
// containing the combined user/system trust bundle.
46-
// TODO: The bundle can be used for more than proxy, change name.
47-
const MERGED_TRUST_BUNDLE_CONFIGMAP = "proxy-ca-bundle"
46+
const TRUST_BUNDLE_CONFIGMAP = "trusted-ca-bundle"
4847

4948
// TRUST_BUNDLE_CONFIGMAP_NS is the namespace that hosts the
50-
// ADDL_TRUST_BUNDLE_CONFIGMAP and MERGED_TRUST_BUNDLE_CONFIGMAP
49+
// ADDL_TRUST_BUNDLE_CONFIGMAP and TRUST_BUNDLE_CONFIGMAP
5150
// ConfigMaps.
5251
const TRUST_BUNDLE_CONFIGMAP_NS = "openshift-config-managed"
5352

54-
// ProxyName returns the namespaced name of the proxy
53+
// Proxy returns the namespaced name of the proxy
5554
// object named "cluster" in namespace "openshift-config-managed".
56-
func ProxyName() types.NamespacedName {
55+
func Proxy() types.NamespacedName {
5756
return types.NamespacedName{
5857
Name: PROXY_CONFIG,
5958
}
6059
}
6160

62-
// MergedTrustBundleName returns the namespaced name of the ConfigMap
61+
// TrustBundleConfigMap returns the namespaced name of the ConfigMap
6362
// containing the merged user/system trust bundle.
64-
func MergedTrustBundleName() types.NamespacedName {
63+
func TrustBundleConfigMap() types.NamespacedName {
6564
return types.NamespacedName{
6665
Namespace: TRUST_BUNDLE_CONFIGMAP_NS,
67-
Name: MERGED_TRUST_BUNDLE_CONFIGMAP,
66+
Name: TRUST_BUNDLE_CONFIGMAP,
6867
}
6968
}

0 commit comments

Comments
 (0)