From 0830ac3b34dd500b7461104b832aa53096dabba2 Mon Sep 17 00:00:00 2001 From: Daneyon Hansen Date: Tue, 16 Jul 2019 16:26:33 -0700 Subject: [PATCH 01/11] WIP: Adds proxy controller. --- pkg/controller/add_networkconfig.go | 2 + .../proxyconfig/proxyconfig_controller.go | 91 +++++++++++++++++++ .../statusmanager/status_manager.go | 1 + pkg/names/names.go | 3 + pkg/network/{proxy.go => kube_proxy.go} | 0 .../{proxy_test.go => kube_proxy_test.go} | 0 pkg/proxy/proxy_config.go | 10 ++ pkg/proxy/proxy_config_test.go | 3 + 8 files changed, 110 insertions(+) create mode 100644 pkg/controller/proxyconfig/proxyconfig_controller.go rename pkg/network/{proxy.go => kube_proxy.go} (100%) rename pkg/network/{proxy_test.go => kube_proxy_test.go} (100%) create mode 100644 pkg/proxy/proxy_config.go create mode 100644 pkg/proxy/proxy_config_test.go diff --git a/pkg/controller/add_networkconfig.go b/pkg/controller/add_networkconfig.go index dd25ef145c..b1cfe0d34f 100644 --- a/pkg/controller/add_networkconfig.go +++ b/pkg/controller/add_networkconfig.go @@ -3,11 +3,13 @@ package controller import ( "github.com/openshift/cluster-network-operator/pkg/controller/clusterconfig" "github.com/openshift/cluster-network-operator/pkg/controller/operconfig" + "github.com/openshift/cluster-network-operator/pkg/controller/proxyconfig" ) func init() { // AddToManagerFuncs is a list of functions to create controllers and add them to a manager. AddToManagerFuncs = append(AddToManagerFuncs, + proxyconfig.Add, operconfig.Add, clusterconfig.Add, operconfig.AddConfigMapReconciler, diff --git a/pkg/controller/proxyconfig/proxyconfig_controller.go b/pkg/controller/proxyconfig/proxyconfig_controller.go new file mode 100644 index 0000000000..826fb563be --- /dev/null +++ b/pkg/controller/proxyconfig/proxyconfig_controller.go @@ -0,0 +1,91 @@ +package proxyconfig + +import ( + "context" + "fmt" + "log" + + configv1 "github.com/openshift/api/config/v1" + "github.com/openshift/cluster-network-operator/pkg/controller/statusmanager" + "github.com/openshift/cluster-network-operator/pkg/names" + + apierrors "k8s.io/apimachinery/pkg/api/errors" + "k8s.io/apimachinery/pkg/runtime" + "sigs.k8s.io/controller-runtime/pkg/client" + "sigs.k8s.io/controller-runtime/pkg/controller" + "sigs.k8s.io/controller-runtime/pkg/handler" + "sigs.k8s.io/controller-runtime/pkg/manager" + "sigs.k8s.io/controller-runtime/pkg/reconcile" + "sigs.k8s.io/controller-runtime/pkg/source" +) + +// and Start it when the Manager is Started. +func Add(mgr manager.Manager, status *statusmanager.StatusManager) error { + return add(mgr, newReconciler(mgr, status)) +} + +// newReconciler returns a new reconcile.Reconciler +func newReconciler(mgr manager.Manager, status *statusmanager.StatusManager) reconcile.Reconciler { + configv1.Install(mgr.GetScheme()) + return &ReconcileProxyConfig{client: mgr.GetClient(), scheme: mgr.GetScheme(), status: status} +} + +// add adds a new Controller to mgr with r as the reconcile.Reconciler +func add(mgr manager.Manager, r reconcile.Reconciler) error { + // Create a new controller + c, err := controller.New("proxyconfig-controller", mgr, controller.Options{Reconciler: r}) + if err != nil { + return err + } + + // Watch for changes to primary resource config.openshift.io/v1/Proxy + err = c.Watch(&source.Kind{Type: &configv1.Proxy{}}, &handler.EnqueueRequestForObject{}) + if err != nil { + return err + } + + return nil +} + +var _ reconcile.Reconciler = &ReconcileProxyConfig{} + +// ReconcileProxyConfig reconciles a Proxy object +type ReconcileProxyConfig struct { + // This client, initialized using mgr.Client() above, is a split client + // that reads objects from the cache and writes to the apiserver + client client.Client + scheme *runtime.Scheme + status *statusmanager.StatusManager +} + +// Reconcile expects request to refer to a proxy object named "cluster" in the +// default namespace, and will ensure proxy is in the desired state. +func (r *ReconcileProxyConfig) Reconcile(request reconcile.Request) (reconcile.Result, error) { + log.Printf("Reconciling Proxy.config.openshift.io %s\n", request.Name) + + // Only reconcile the "cluster" proxy. + if request.Name != names.PROXY_CONFIG { + log.Printf("Ignoring Proxy without default name " + names.PROXY_CONFIG) + return reconcile.Result{}, nil + } + + // Fetch the proxy config + proxyConfig := &configv1.Proxy{} + err := r.client.Get(context.TODO(), request.NamespacedName, proxyConfig) + if err != nil { + if apierrors.IsNotFound(err) { + // Request object not found, could have been deleted after reconcile request. + // Return and don't requeue + log.Println("proxy not found; reconciliation will be skipped", "request", request) + return reconcile.Result{}, nil + } + // Error reading the object - requeue the request. + return reconcile.Result{}, fmt.Errorf("failed to get proxy %q: %v", request, err) + } + + // TODO: Validate the proxy spec and write status. + // TODO: How should proxy reconciliation be reflected in clusteroperator/network status? + + r.status.SetNotDegraded(statusmanager.ProxyConfig) + return reconcile.Result{}, nil +} diff --git a/pkg/controller/statusmanager/status_manager.go b/pkg/controller/statusmanager/status_manager.go index 00fcc930da..9786dc470d 100644 --- a/pkg/controller/statusmanager/status_manager.go +++ b/pkg/controller/statusmanager/status_manager.go @@ -26,6 +26,7 @@ type StatusLevel int const ( ClusterConfig StatusLevel = iota OperatorConfig StatusLevel = iota + ProxyConfig StatusLevel = iota PodDeployment StatusLevel = iota maxStatusLevel StatusLevel = iota ) diff --git a/pkg/names/names.go b/pkg/names/names.go index bd68838958..d2ee8ddfe7 100644 --- a/pkg/names/names.go +++ b/pkg/names/names.go @@ -10,6 +10,9 @@ const OPERATOR_CONFIG = "cluster" // and status object. const CLUSTER_CONFIG = "cluster" +// PROXY_CONFIG is the name of the default proxy object. +const PROXY_CONFIG = "cluster" + // APPLIED_PREFIX is the prefix applied to the config maps // where we store previously applied configuration const APPLIED_PREFIX = "applied-" diff --git a/pkg/network/proxy.go b/pkg/network/kube_proxy.go similarity index 100% rename from pkg/network/proxy.go rename to pkg/network/kube_proxy.go diff --git a/pkg/network/proxy_test.go b/pkg/network/kube_proxy_test.go similarity index 100% rename from pkg/network/proxy_test.go rename to pkg/network/kube_proxy_test.go diff --git a/pkg/proxy/proxy_config.go b/pkg/proxy/proxy_config.go new file mode 100644 index 0000000000..40e3421628 --- /dev/null +++ b/pkg/proxy/proxy_config.go @@ -0,0 +1,10 @@ +package proxy + +import ( + configv1 "github.com/openshift/api/config/v1" +) + +// ValidateProxyConfig ensures the proxy config is valid. +func ValidateProxyConfig(proxyConfig configv1.ProxySpec) error { + // TODO: Validate proxy spec according to design and write status. +} diff --git a/pkg/proxy/proxy_config_test.go b/pkg/proxy/proxy_config_test.go new file mode 100644 index 0000000000..24ff69e9dd --- /dev/null +++ b/pkg/proxy/proxy_config_test.go @@ -0,0 +1,3 @@ +package proxy + +// TODO: Create proxy controller unit tests. \ No newline at end of file From 095cd157b3a2f8764f7ee090d09a3a92cf177ef8 Mon Sep 17 00:00:00 2001 From: Daneyon Hansen Date: Mon, 22 Jul 2019 13:32:23 -0700 Subject: [PATCH 02/11] Adds initial proxy.spec validation --- .../proxyconfig/proxyconfig_controller.go | 125 ++++++- pkg/proxy/proxy_config.go | 309 +++++++++++++++++- 2 files changed, 430 insertions(+), 4 deletions(-) diff --git a/pkg/controller/proxyconfig/proxyconfig_controller.go b/pkg/controller/proxyconfig/proxyconfig_controller.go index 826fb563be..e20f6604ef 100644 --- a/pkg/controller/proxyconfig/proxyconfig_controller.go +++ b/pkg/controller/proxyconfig/proxyconfig_controller.go @@ -3,14 +3,24 @@ package proxyconfig import ( "context" "fmt" + "k8s.io/apimachinery/pkg/types" "log" + "net/url" + "strconv" + "strings" + + "github.com/ghodss/yaml" configv1 "github.com/openshift/api/config/v1" "github.com/openshift/cluster-network-operator/pkg/controller/statusmanager" "github.com/openshift/cluster-network-operator/pkg/names" + "github.com/openshift/cluster-network-operator/pkg/proxy" + + corev1 "k8s.io/api/core/v1" apierrors "k8s.io/apimachinery/pkg/api/errors" "k8s.io/apimachinery/pkg/runtime" + "k8s.io/apimachinery/pkg/util/sets" "sigs.k8s.io/controller-runtime/pkg/client" "sigs.k8s.io/controller-runtime/pkg/controller" "sigs.k8s.io/controller-runtime/pkg/handler" @@ -52,7 +62,7 @@ var _ reconcile.Reconciler = &ReconcileProxyConfig{} // ReconcileProxyConfig reconciles a Proxy object type ReconcileProxyConfig struct { // This client, initialized using mgr.Client() above, is a split client - // that reads objects from the cache and writes to the apiserver + // that reads objects from the cache and writes to the apiserver. client client.Client scheme *runtime.Scheme status *statusmanager.StatusManager @@ -83,9 +93,120 @@ func (r *ReconcileProxyConfig) Reconcile(request reconcile.Request) (reconcile.R return reconcile.Result{}, fmt.Errorf("failed to get proxy %q: %v", request, err) } - // TODO: Validate the proxy spec and write status. + // Only proceed if we can collect cluster config. + infraConfig := &configv1.Infrastructure{} + if err := r.client.Get(context.TODO(), types.NamespacedName{Name: "cluster"}, infraConfig); err != nil { + return reconcile.Result{}, fmt.Errorf("failed to get infrastructure config 'cluster': %v", err) + } + netConfig := &configv1.Network{} + if err := r.client.Get(context.TODO(), types.NamespacedName{Name: "cluster"}, infraConfig); err != nil { + return reconcile.Result{}, fmt.Errorf("failed to get network config 'cluster': %v", err) + } + clusterConfig := &corev1.ConfigMap{} + if err := r.client.Get(context.TODO(), types.NamespacedName{Name: "cluster-config-v1"}, clusterConfig); err != nil { + return reconcile.Result{}, fmt.Errorf("failed to get network config 'cluster': %v", err) + } + + if err := proxy.ValidateProxyConfig(r.client, proxyConfig.Spec); err != nil { + log.Printf("Failed to validate Proxy.Spec: %v", err) + r.status.SetDegraded(statusmanager.ProxyConfig, "InvalidProxyConfig", + fmt.Sprintf("The proxy configuration is invalid (%v). Use 'oc edit proxy.config.openshift.io cluster' to fix.", err)) + return reconcile.Result{}, err + } + + if err := r.syncProxyStatus(proxyConfig, infraConfig, netConfig, clusterConfig); err != nil { + log.Printf("Failed to enforce NoProxy default values: %v", err) + r.status.SetDegraded(statusmanager.ProxyConfig, "DefaultNoProxyFailedEnforcement", + fmt.Sprintf("Failed to enforce system default NoProxy values: %v", err)) + return reconcile.Result{}, err + } + // TODO: How should proxy reconciliation be reflected in clusteroperator/network status? r.status.SetNotDegraded(statusmanager.ProxyConfig) return reconcile.Result{}, nil } + +// syncProxyStatus... +func (r *ReconcileProxyConfig) syncProxyStatus(proxy *configv1.Proxy, infra *configv1.Infrastructure, network *configv1.Network, cluster *corev1.ConfigMap) error { + updated := proxy.DeepCopy() + + apiServerURL, err := url.Parse(infra.Status.APIServerURL) + if err != nil { + return fmt.Errorf("failed to parse API server URL") + } + internalAPIServer, err := url.Parse(infra.Status.APIServerInternalURL) + if err != nil { + return fmt.Errorf("failed to parse API server internal URL") + } + + set := sets.NewString( + "127.0.0.1", + "localhost", + network.Status.ServiceNetwork[0], + apiServerURL.Hostname(), + internalAPIServer.Hostname(), + ) + platform := infra.Status.PlatformStatus.Type + + // TODO: Does a better way exist to get machineCIDR and controlplane replicas? + type installConfig struct { + ControlPlane struct { + Replicas string `json:"replicas"` + } `json:"controlPlane"` + Networking struct { + MachineCIDR string `json:"machineCIDR"` + } `json:"networking"` + } + var ic installConfig + data, ok := cluster.Data["install-config"] + if !ok { + return fmt.Errorf("missing install-config in configmap") + } + if err := yaml.Unmarshal([]byte(data), &ic); err != nil { + return fmt.Errorf("invalid install-config: %v\njson:\n%s", err, data) + } + + if platform != configv1.VSpherePlatformType && platform != configv1.NonePlatformType { + set.Insert("169.254.169.254", ic.Networking.MachineCIDR) + } + + replicas, err := strconv.Atoi(ic.ControlPlane.Replicas) + if err != nil { + return fmt.Errorf("failed to parse install config replicas: %v", err) + } + + for i := int64(0); i < int64(replicas); i++ { + etcdHost := fmt.Sprintf("etcd-%d.%s", i, infra.Status.EtcdDiscoveryDomain) + set.Insert(etcdHost) + } + + for _, clusterNetwork := range network.Status.ClusterNetwork { + set.Insert(clusterNetwork.CIDR) + } + + for _, userValue := range strings.Split(proxy.Spec.NoProxy, ",") { + set.Insert(userValue) + } + + updated.Status.NoProxy = strings.Join(set.List(), ",") + + if !proxyStatusesEqual(proxy.Status, updated.Status) { + if err := r.client.Status().Update(context.TODO(), updated); err != nil { + return fmt.Errorf("failed to update proxy status: %v", err) + } + } + + return nil +} + +// proxyStatusesEqual compares two ProxyStatus values. Returns true if the +// provided values should be considered equal for the purpose of determining +// whether an update is necessary, false otherwise. +func proxyStatusesEqual(a, b configv1.ProxyStatus) bool { + if a.HTTPProxy != b.HTTPProxy || a.HTTPSProxy != b.HTTPSProxy || a.NoProxy != b.NoProxy { + return false + } + + return true +} diff --git a/pkg/proxy/proxy_config.go b/pkg/proxy/proxy_config.go index 40e3421628..b12a527c99 100644 --- a/pkg/proxy/proxy_config.go +++ b/pkg/proxy/proxy_config.go @@ -1,10 +1,315 @@ package proxy import ( + "context" + "crypto/tls" + "crypto/x509" + "errors" + "fmt" + "net" + "net/http" + "net/url" + "strconv" + "strings" + configv1 "github.com/openshift/api/config/v1" + + corev1 "k8s.io/api/core/v1" + "k8s.io/apimachinery/pkg/types" + k8serrors "k8s.io/apimachinery/pkg/util/errors" + "k8s.io/apimachinery/pkg/util/validation" + "sigs.k8s.io/controller-runtime/pkg/client" +) + +const ( + // The number of times the controller will attempt to issue an http GET + // to the endpoint specified in readinessEndpoints. + proxyProbeMaxRetries = 3 + // clusterConfigMapName is the name of the proxy.spec.trustedCA + // ConfigMap that contains the CA bundle certificate. + clusterConfigMapName = "proxy-ca" + // clusterConfigMapNamespace is the name of the namespace that hosts + // the proxy.spec.trustedCA ConfigMap. + clusterConfigMapNamespace = "openshift-config-managed" + // clusterConfigMapKey is the name of the data key containing the PEM encoded + // CA certificate trust bundle in clusterConfigMapName. + clusterConfigMapKey = "ca-bundle.crt" + // installerConfigMapName is the name of the ConfigMap generated + // by the installer containing the user-provided CA certificate bundle. + installerConfigMapName = "proxy-ca-bundle" + // installerConfigMapNamespace is the name of the namespace that hosts the + // ConfigMap containing the user-provided CA certificate bundle. + installerConfigMapNamespace = "openshift-config" + + proxyHTTPScheme = "http" + proxyHTTPSScheme = "https" ) // ValidateProxyConfig ensures the proxy config is valid. -func ValidateProxyConfig(proxyConfig configv1.ProxySpec) error { - // TODO: Validate proxy spec according to design and write status. +func ValidateProxyConfig(cli client.Client, proxyConfig configv1.ProxySpec) error { + if len(proxyConfig.HTTPProxy) != 0 { + scheme, err := validateURI(proxyConfig.HTTPProxy) + if err != nil { + return fmt.Errorf("invalid httpProxy URI: %v", err) + } + if scheme != proxyHTTPScheme { + return fmt.Errorf("httpProxy requires a %q URI scheme", proxyHTTPScheme) + } + } + if len(proxyConfig.HTTPSProxy) != 0 { + if len(proxyConfig.TrustedCA.Name) != 0 { + return errors.New("trustedCA is required when using httpsProxy") + } + scheme, err := validateURI(proxyConfig.HTTPSProxy) + if err != nil { + return fmt.Errorf("invalid httpsProxy URI: %v", err) + } + if scheme != proxyHTTPSScheme { + return fmt.Errorf("httpsProxy requires a %q URI scheme", proxyHTTPSScheme) + } + } + if len(proxyConfig.NoProxy) != 0 { + for _, v := range strings.Split(proxyConfig.NoProxy, ",") { + v = strings.TrimSpace(v) + errDomain := validateDomainName(v, false) + _, _, errCIDR := net.ParseCIDR(v) + if errDomain != nil && errCIDR != nil { + return fmt.Errorf("invalid noProxy: %v", v) + } + } + } + if len(proxyConfig.TrustedCA.Name) != 0 { + if proxyConfig.TrustedCA.Name != clusterConfigMapName { + return fmt.Errorf("invalid ConfigMap reference for TrustedCA: %s", proxyConfig.TrustedCA.Name) + } + cfgMap := &corev1.ConfigMap{} + if err := cli.Get(context.TODO(), clusterCAConfigMapName(), cfgMap); err != nil { + return err + } + // TODO: Have validateClusterCABundle return certBundle []byte containing the validated ca bundle. + if err := validateClusterCABundle(cfgMap); err != nil { + return fmt.Errorf("validation failed for trustedCA %s: %v", proxyConfig.TrustedCA.Name, err) + } + } + if proxyConfig.ReadinessEndpoints != nil { + for _, endpoint := range proxyConfig.ReadinessEndpoints { + scheme, err := validateURI(endpoint) + if err != nil { + return fmt.Errorf("invalid URI for endpoint %s: %v", endpoint, err) + } + switch { + case scheme == proxyHTTPScheme: + if err := validateHTTPReadinessEndpoint(endpoint); err != nil { + return fmt.Errorf("readinessEndpoint probe failed for endpoint %s", endpoint) + } + // TODO: Uncomment after validateClusterCABundle() returns a validated ca bundle. + /*case scheme == proxyHTTPSScheme: + if err := validateHTTPSReadinessEndpoint(caBundle, endpoint); err != nil { + return fmt.Errorf("readinessEndpoint probe failed for endpoint %s", endpoint) + }*/ + default: + return fmt.Errorf("readiness endpoints requires a %q or %q URI sheme", proxyHTTPScheme, proxyHTTPSScheme) + } + } + } + + return nil +} + +// validateURI validates if url is a valid absolute URI and returns +// the url scheme. +func validateURI(uri string) (string, error) { + parsed, err := url.Parse(uri) + if err != nil { + return "", err + } + if !parsed.IsAbs() { + return "", fmt.Errorf("failed validating URI, no scheme for URI %q", uri) + } + host := parsed.Hostname() + if err := validateHost(host); err != nil { + return "", fmt.Errorf("failed validating URI %q: %v", uri, err) + } + if port := parsed.Port(); len(port) != 0 { + intPort, err := strconv.Atoi(port) + if err != nil { + return "", fmt.Errorf("failed converting port to integer for URI %q: %v", uri, err) + } + if err := validatePort(intPort); err != nil { + return "", fmt.Errorf("failed to validate port for URL %q: %v", uri, err) + } + } + + return parsed.Scheme, nil +} + +// validateHost validates if host is a valid IP address or subdomain in DNS (RFC 1123). +func validateHost(host string) error { + errDomain := validateDomainName(host, false) + errIP := validation.IsValidIP(host) + if errDomain != nil && errIP != nil { + return fmt.Errorf("invalid host: %s", host) + } + + return nil +} + +// validatePort validates if port is a valid port number between 1-65535. +func validatePort(port int) error { + invalidPorts := validation.IsValidPortNum(port) + if invalidPorts != nil { + return fmt.Errorf("invalid port number: %d", port) + } + + return nil +} + +// validateHTTPReadinessEndpoint validates an http readinessEndpoint endpoint. +func validateHTTPReadinessEndpoint(endpoint string) error { + if err := validateHTTPReadinessEndpointWithRetries(endpoint, proxyProbeMaxRetries); err != nil { + return err + } + + return nil +} + +// validateHTTPReadinessEndpointWithRetries tries to validate an http +// endpoint in a finite loop based on the scheme type, it returns the +// last result if it never succeeds. +func validateHTTPReadinessEndpointWithRetries(endpoint string, retries int) error { + for i := 0; i < retries; i++ { + if err := runHTTPReadinessProbe(endpoint); err != nil { + return err + } + } + + return nil +} + +// runHTTPReadinessProbe issues an http GET request to an http endpoint +// and returns an error if a 2XX or 3XX http status code is not returned. +func runHTTPReadinessProbe(endpoint string) error { + resp, err := http.Get(endpoint) + if err != nil { + return err + } + defer func() { + if err := resp.Body.Close(); err != nil { + fmt.Errorf("failed to close connection: %v", err) + } + }() + + if resp.StatusCode >= http.StatusOK && resp.StatusCode < http.StatusBadRequest { + return nil + } + + return fmt.Errorf("HTTP probe failed with statuscode: %d", resp.StatusCode) +} + +// validateHTTPSReadinessEndpoint validates an https readinessEndpoint endpoint. +func validateHTTPSReadinessEndpoint(certBundle []byte, endpoint string) error { + if err := validateHTTPSReadinessEndpointWithRetries(certBundle, endpoint, proxyProbeMaxRetries); err != nil { + return err + } + + return nil +} + +// validateHTTPSReadinessEndpointWithRetries tries to validate an endpoint +// by using certBundle to attempt a TLS handshake in a finite loop returning +// the last result if it never succeeds. +func validateHTTPSReadinessEndpointWithRetries(certBundle []byte, endpoint string, retries int) error { + for i := 0; i < retries; i++ { + if err := runHTTPSReadinessProbe(certBundle, endpoint); err != nil { + return err + } + } + + return nil +} + +// runHTTPSReadinessProbe tries connecting to endpoint by using certBundle +// to attempt a TLS handshake. +func runHTTPSReadinessProbe(certBundle []byte, endpoint string) error { + parsedURL, err := url.Parse(endpoint) + if err != nil { + return fmt.Errorf("failed parsing URL for endpoint: %s", endpoint) + } + certPool := x509.NewCertPool() + if !certPool.AppendCertsFromPEM(certBundle) { + return fmt.Errorf("failed to parse CA certificate bundle") + } + port := parsedURL.Port() + if len(port) == 0 { + parsedURL.Host += ":" + port + } + conn, err := tls.Dial("tcp", parsedURL.String(), &tls.Config{ + RootCAs: certPool, + ServerName: endpoint, + }) + if err != nil { + return fmt.Errorf("failed to connect to endpoint %q: %v", endpoint, err) + } + defer func() { + if err := conn.Close(); err != nil { + fmt.Errorf("failed to close connection: %v", err) + } + }() + + return nil +} + +// validateDomainName checks if the given string is a valid domain name and returns an error if not. +func validateDomainName(v string, acceptTrailingDot bool) error { + if acceptTrailingDot { + v = strings.TrimSuffix(v, ".") + } + return validateSubdomain(v) +} + +// validateSubdomain checks if the given string is a valid subdomain name and returns an error if not. +func validateSubdomain(v string) error { + validationMessages := validation.IsDNS1123Subdomain(v) + if len(validationMessages) == 0 { + return nil + } + + errs := make([]error, len(validationMessages)) + for i, m := range validationMessages { + errs[i] = errors.New(m) + } + return k8serrors.NewAggregate(errs) +} + +// validateClusterCABundle validates that configMap contains a +// CA certificate bundle named clusterConfigMapKey and that +// clusterConfigMapKey contains a valid x.509 certificate. +func validateClusterCABundle(configMap *corev1.ConfigMap) error { + if _, ok := configMap.Data[clusterConfigMapKey]; !ok { + return fmt.Errorf("ConfigMap %q is missing %q", clusterConfigMapName, clusterConfigMapKey) + } + _, err := x509.ParseCertificates([]byte(configMap.Data[clusterConfigMapKey])) + if err != nil { + return err + } + + return nil +} + +// installerCAConfigMapName returns the namespaced name of the ConfigMap +// containing the installer-generated CA certificate bundle. +func installerCAConfigMapName() types.NamespacedName { + return types.NamespacedName{ + Namespace: installerConfigMapNamespace, + Name: installerConfigMapName, + } +} + +// clusterCAConfigMapName returns the namespaced name of the ConfigMap +// containing the cluster CA certificate bundle. +func clusterCAConfigMapName() types.NamespacedName { + return types.NamespacedName{ + Namespace: clusterConfigMapNamespace, + Name: clusterConfigMapName, + } } From d577466cfb1b050bf3c0deb94a6f9ee6b046243c Mon Sep 17 00:00:00 2001 From: Daneyon Hansen Date: Thu, 25 Jul 2019 15:30:43 -0700 Subject: [PATCH 03/11] Adds support for creating default proxy --- cmd/cluster-network-operator/main.go | 63 ++++++++++++++++++- .../proxyconfig/proxyconfig_controller.go | 11 +++- 2 files changed, 69 insertions(+), 5 deletions(-) diff --git a/cmd/cluster-network-operator/main.go b/cmd/cluster-network-operator/main.go index dd924cad63..8a174fac18 100644 --- a/cmd/cluster-network-operator/main.go +++ b/cmd/cluster-network-operator/main.go @@ -7,6 +7,7 @@ import ( "net/url" "os" "runtime" + "time" configv1 "github.com/openshift/api/config/v1" operv1 "github.com/openshift/api/operator/v1" @@ -19,6 +20,10 @@ import ( "sigs.k8s.io/controller-runtime/pkg/client/config" "sigs.k8s.io/controller-runtime/pkg/manager" "sigs.k8s.io/controller-runtime/pkg/runtime/signals" + "k8s.io/apimachinery/pkg/api/errors" + metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" + "k8s.io/apimachinery/pkg/types" + "k8s.io/apimachinery/pkg/util/wait" ) func printVersion() { @@ -108,8 +113,62 @@ func main() { log.Print("Starting the Cmd.") - // Start the Cmd - if err := mgr.Start(signals.SetupSignalHandler()); err != nil { + // Create the stop channel and start the operator + stop := signals.SetupSignalHandler() + if err := start(mgr, stop); err != nil { log.Fatal(err) } } + +// start creates the default Proxy if it does not exist and then +// starts the operator synchronously until a message is received +// on the stop channel. +func start(mgr manager.Manager, stop <-chan struct{}) error { + // Periodically ensure the default proxy exists. + go wait.Until(func() { + if !mgr.GetCache().WaitForCacheSync(stop) { + log.Print("failed to sync cache before ensuring default proxy") + return + } + err := ensureDefaultProxy(mgr) + if err != nil { + log.Print(err, "failed to ensure default proxy") + } + }, 1*time.Minute, stop) + + errChan := make(chan error) + go func() { + errChan <- mgr.Start(stop) + }() + + // Wait for the manager to exit or an explicit stop. + select { + case <-stop: + return nil + case err := <-errChan: + return err + } +} + +// ensureDefaultProxy creates the default proxy if it doesn't exist. +func ensureDefaultProxy(mgr manager.Manager) error { + proxy := &configv1.Proxy{ + ObjectMeta: metav1.ObjectMeta{ + Name: "cluster", + }, + } + client := mgr.GetClient() + err := client.Get(context.TODO(), types.NamespacedName{Name: proxy.Name}, proxy) + if err == nil { + return nil + } + if !errors.IsNotFound(err) { + return err + } + err = client.Create(context.TODO(), proxy) + if err != nil { + return err + } + log.Printf("created default proxy %q", proxy.Name) + return nil +} diff --git a/pkg/controller/proxyconfig/proxyconfig_controller.go b/pkg/controller/proxyconfig/proxyconfig_controller.go index e20f6604ef..525fe464f4 100644 --- a/pkg/controller/proxyconfig/proxyconfig_controller.go +++ b/pkg/controller/proxyconfig/proxyconfig_controller.go @@ -3,7 +3,6 @@ package proxyconfig import ( "context" "fmt" - "k8s.io/apimachinery/pkg/types" "log" "net/url" "strconv" @@ -20,6 +19,7 @@ import ( apierrors "k8s.io/apimachinery/pkg/api/errors" "k8s.io/apimachinery/pkg/runtime" + "k8s.io/apimachinery/pkg/types" "k8s.io/apimachinery/pkg/util/sets" "sigs.k8s.io/controller-runtime/pkg/client" "sigs.k8s.io/controller-runtime/pkg/controller" @@ -93,6 +93,13 @@ func (r *ReconcileProxyConfig) Reconcile(request reconcile.Request) (reconcile.R return reconcile.Result{}, fmt.Errorf("failed to get proxy %q: %v", request, err) } + // A nil proxy is generated by installs and not requiring a proxy + // and upgrades to 4.2, skip reconciliation. + if len(proxyConfig.Spec.HTTPProxy) == 0 && len(proxyConfig.Spec.HTTPSProxy) == 0 { + log.Println("httpProxy or httpsProxy not defined; reconciliation will be skipped", "request", request) + return reconcile.Result{}, nil + } + // Only proceed if we can collect cluster config. infraConfig := &configv1.Infrastructure{} if err := r.client.Get(context.TODO(), types.NamespacedName{Name: "cluster"}, infraConfig); err != nil { @@ -121,8 +128,6 @@ func (r *ReconcileProxyConfig) Reconcile(request reconcile.Request) (reconcile.R return reconcile.Result{}, err } - // TODO: How should proxy reconciliation be reflected in clusteroperator/network status? - r.status.SetNotDegraded(statusmanager.ProxyConfig) return reconcile.Result{}, nil } From 4443f4c1f6b279efa2ca0e492a1762a671f82a44 Mon Sep 17 00:00:00 2001 From: Daneyon Hansen Date: Thu, 25 Jul 2019 16:59:58 -0700 Subject: [PATCH 04/11] Refactors proxy controller --- cmd/cluster-network-operator/main.go | 10 +- pkg/controller/proxyconfig/config_test.go | 3 + ...roxyconfig_controller.go => controller.go} | 154 ++++++------- pkg/controller/proxyconfig/status.go | 117 ++++++++++ .../proxyconfig/validation.go} | 217 ++++++++++-------- pkg/names/names.go | 20 ++ pkg/proxy/proxy_config_test.go | 3 - 7 files changed, 330 insertions(+), 194 deletions(-) create mode 100644 pkg/controller/proxyconfig/config_test.go rename pkg/controller/proxyconfig/{proxyconfig_controller.go => controller.go} (51%) create mode 100644 pkg/controller/proxyconfig/status.go rename pkg/{proxy/proxy_config.go => controller/proxyconfig/validation.go} (52%) delete mode 100644 pkg/proxy/proxy_config_test.go diff --git a/cmd/cluster-network-operator/main.go b/cmd/cluster-network-operator/main.go index 8a174fac18..6f373043eb 100644 --- a/cmd/cluster-network-operator/main.go +++ b/cmd/cluster-network-operator/main.go @@ -15,15 +15,15 @@ import ( k8sutil "github.com/openshift/cluster-network-operator/pkg/util/k8s" "github.com/operator-framework/operator-sdk/pkg/leader" sdkVersion "github.com/operator-framework/operator-sdk/version" + "k8s.io/apimachinery/pkg/api/errors" + metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" + "k8s.io/apimachinery/pkg/types" + "k8s.io/apimachinery/pkg/util/wait" _ "k8s.io/client-go/plugin/pkg/client/auth/gcp" "k8s.io/client-go/tools/clientcmd" "sigs.k8s.io/controller-runtime/pkg/client/config" "sigs.k8s.io/controller-runtime/pkg/manager" "sigs.k8s.io/controller-runtime/pkg/runtime/signals" - "k8s.io/apimachinery/pkg/api/errors" - metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" - "k8s.io/apimachinery/pkg/types" - "k8s.io/apimachinery/pkg/util/wait" ) func printVersion() { @@ -154,7 +154,7 @@ func start(mgr manager.Manager, stop <-chan struct{}) error { func ensureDefaultProxy(mgr manager.Manager) error { proxy := &configv1.Proxy{ ObjectMeta: metav1.ObjectMeta{ - Name: "cluster", + Name: "cluster", }, } client := mgr.GetClient() diff --git a/pkg/controller/proxyconfig/config_test.go b/pkg/controller/proxyconfig/config_test.go new file mode 100644 index 0000000000..a18ea81f99 --- /dev/null +++ b/pkg/controller/proxyconfig/config_test.go @@ -0,0 +1,3 @@ +package proxyconfig + +// TODO: Create proxy controller unit tests. diff --git a/pkg/controller/proxyconfig/proxyconfig_controller.go b/pkg/controller/proxyconfig/controller.go similarity index 51% rename from pkg/controller/proxyconfig/proxyconfig_controller.go rename to pkg/controller/proxyconfig/controller.go index 525fe464f4..bd8e389100 100644 --- a/pkg/controller/proxyconfig/proxyconfig_controller.go +++ b/pkg/controller/proxyconfig/controller.go @@ -4,23 +4,15 @@ import ( "context" "fmt" "log" - "net/url" - "strconv" - "strings" - - "github.com/ghodss/yaml" configv1 "github.com/openshift/api/config/v1" "github.com/openshift/cluster-network-operator/pkg/controller/statusmanager" "github.com/openshift/cluster-network-operator/pkg/names" - "github.com/openshift/cluster-network-operator/pkg/proxy" corev1 "k8s.io/api/core/v1" - apierrors "k8s.io/apimachinery/pkg/api/errors" "k8s.io/apimachinery/pkg/runtime" "k8s.io/apimachinery/pkg/types" - "k8s.io/apimachinery/pkg/util/sets" "sigs.k8s.io/controller-runtime/pkg/client" "sigs.k8s.io/controller-runtime/pkg/controller" "sigs.k8s.io/controller-runtime/pkg/handler" @@ -31,12 +23,20 @@ import ( // and Start it when the Manager is Started. func Add(mgr manager.Manager, status *statusmanager.StatusManager) error { - return add(mgr, newReconciler(mgr, status)) + reconciler := newReconciler(mgr, status) + if reconciler == nil { + return fmt.Errorf("failed to create reconciler") + } + + return add(mgr, reconciler) } // newReconciler returns a new reconcile.Reconciler func newReconciler(mgr manager.Manager, status *statusmanager.StatusManager) reconcile.Reconciler { - configv1.Install(mgr.GetScheme()) + if err := configv1.Install(mgr.GetScheme()); err != nil { + return &ReconcileProxyConfig{} + } + return &ReconcileProxyConfig{client: mgr.GetClient(), scheme: mgr.GetScheme(), status: status} } @@ -73,7 +73,7 @@ type ReconcileProxyConfig struct { func (r *ReconcileProxyConfig) Reconcile(request reconcile.Request) (reconcile.Result, error) { log.Printf("Reconciling Proxy.config.openshift.io %s\n", request.Name) - // Only reconcile the "cluster" proxy. + // Only reconcile a proxy object named "cluster". if request.Name != names.PROXY_CONFIG { log.Printf("Ignoring Proxy without default name " + names.PROXY_CONFIG) return reconcile.Result{}, nil @@ -93,123 +93,103 @@ func (r *ReconcileProxyConfig) Reconcile(request reconcile.Request) (reconcile.R return reconcile.Result{}, fmt.Errorf("failed to get proxy %q: %v", request, err) } - // A nil proxy is generated by installs and not requiring a proxy - // and upgrades to 4.2, skip reconciliation. - if len(proxyConfig.Spec.HTTPProxy) == 0 && len(proxyConfig.Spec.HTTPSProxy) == 0 { - log.Println("httpProxy or httpsProxy not defined; reconciliation will be skipped", "request", request) + // A nil proxy is generated by upgrades and installs not requiring a proxy. + if !isSpecHTTPProxySet(&proxyConfig.Spec) && !isSpecHTTPSProxySet(&proxyConfig.Spec) { + log.Println("httpProxy and httpsProxy not defined; reconciliation will be skipped", "request", request) return reconcile.Result{}, nil } - // Only proceed if we can collect cluster config. + // Only proceed if the required config objects can be collected. infraConfig := &configv1.Infrastructure{} if err := r.client.Get(context.TODO(), types.NamespacedName{Name: "cluster"}, infraConfig); err != nil { return reconcile.Result{}, fmt.Errorf("failed to get infrastructure config 'cluster': %v", err) } netConfig := &configv1.Network{} - if err := r.client.Get(context.TODO(), types.NamespacedName{Name: "cluster"}, infraConfig); err != nil { - return reconcile.Result{}, fmt.Errorf("failed to get network config 'cluster': %v", err) + if err := r.client.Get(context.TODO(), types.NamespacedName{Name: "cluster"}, netConfig); err != nil { + log.Printf("failed to get network config 'cluster': %v", err) + return reconcile.Result{}, err } clusterConfig := &corev1.ConfigMap{} - if err := r.client.Get(context.TODO(), types.NamespacedName{Name: "cluster-config-v1"}, clusterConfig); err != nil { - return reconcile.Result{}, fmt.Errorf("failed to get network config 'cluster': %v", err) + if err := r.client.Get(context.TODO(), types.NamespacedName{Name: "cluster-config-v1", Namespace: "kube-system"}, clusterConfig); err != nil { + log.Printf("failed to get configmap 'cluster': %v", err) + return reconcile.Result{}, err } - if err := proxy.ValidateProxyConfig(r.client, proxyConfig.Spec); err != nil { + if err := r.ValidateProxyConfig(&proxyConfig.Spec); err != nil { log.Printf("Failed to validate Proxy.Spec: %v", err) r.status.SetDegraded(statusmanager.ProxyConfig, "InvalidProxyConfig", fmt.Sprintf("The proxy configuration is invalid (%v). Use 'oc edit proxy.config.openshift.io cluster' to fix.", err)) - return reconcile.Result{}, err } + // Update Proxy.config.openshift.io.Status if err := r.syncProxyStatus(proxyConfig, infraConfig, netConfig, clusterConfig); err != nil { - log.Printf("Failed to enforce NoProxy default values: %v", err) - r.status.SetDegraded(statusmanager.ProxyConfig, "DefaultNoProxyFailedEnforcement", - fmt.Sprintf("Failed to enforce system default NoProxy values: %v", err)) + log.Printf("Could not sync proxy status: %v", err) + r.status.SetDegraded(statusmanager.ProxyConfig, "StatusError", + fmt.Sprintf("Could not update proxy configuration status: %v", err)) return reconcile.Result{}, err } - r.status.SetNotDegraded(statusmanager.ProxyConfig) + log.Printf("Reconciling Proxy.config.openshift.io %s complete\n", request.Name) + return reconcile.Result{}, nil } -// syncProxyStatus... -func (r *ReconcileProxyConfig) syncProxyStatus(proxy *configv1.Proxy, infra *configv1.Infrastructure, network *configv1.Network, cluster *corev1.ConfigMap) error { - updated := proxy.DeepCopy() +// isSpecHTTPAndHTTPSProxySet checks whether spec.httpProxy and spec.httpsProxy +// of proxy is set. +func isSpecHTTPAndHTTPSProxySet(proxyConfig *configv1.ProxySpec) bool { + return !isSpecHTTPProxySet(proxyConfig) && !isSpecHTTPSProxySet(proxyConfig) +} - apiServerURL, err := url.Parse(infra.Status.APIServerURL) - if err != nil { - return fmt.Errorf("failed to parse API server URL") - } - internalAPIServer, err := url.Parse(infra.Status.APIServerInternalURL) - if err != nil { - return fmt.Errorf("failed to parse API server internal URL") +// isSpecHTTPProxySet checks whether spec.httpProxy of proxy is set. +func isSpecHTTPProxySet(proxyConfig *configv1.ProxySpec) bool { + if len(proxyConfig.HTTPProxy) == 0 { + return false } - set := sets.NewString( - "127.0.0.1", - "localhost", - network.Status.ServiceNetwork[0], - apiServerURL.Hostname(), - internalAPIServer.Hostname(), - ) - platform := infra.Status.PlatformStatus.Type - - // TODO: Does a better way exist to get machineCIDR and controlplane replicas? - type installConfig struct { - ControlPlane struct { - Replicas string `json:"replicas"` - } `json:"controlPlane"` - Networking struct { - MachineCIDR string `json:"machineCIDR"` - } `json:"networking"` - } - var ic installConfig - data, ok := cluster.Data["install-config"] - if !ok { - return fmt.Errorf("missing install-config in configmap") - } - if err := yaml.Unmarshal([]byte(data), &ic); err != nil { - return fmt.Errorf("invalid install-config: %v\njson:\n%s", err, data) - } + return true +} - if platform != configv1.VSpherePlatformType && platform != configv1.NonePlatformType { - set.Insert("169.254.169.254", ic.Networking.MachineCIDR) +// isSpecHTTPSProxySet checks whether spec.httpsProxy of proxy is set. +func isSpecHTTPSProxySet(proxyConfig *configv1.ProxySpec) bool { + if len(proxyConfig.HTTPSProxy) == 0 { + return false } - replicas, err := strconv.Atoi(ic.ControlPlane.Replicas) - if err != nil { - return fmt.Errorf("failed to parse install config replicas: %v", err) - } + return true +} - for i := int64(0); i < int64(replicas); i++ { - etcdHost := fmt.Sprintf("etcd-%d.%s", i, infra.Status.EtcdDiscoveryDomain) - set.Insert(etcdHost) +// isSpecNoProxySet checks whether spec.NoProxy of proxy is set. +func isSpecNoProxySet(proxyConfig *configv1.ProxySpec) bool { + if len(proxyConfig.NoProxy) == 0 { + return false } - for _, clusterNetwork := range network.Status.ClusterNetwork { - set.Insert(clusterNetwork.CIDR) - } + return true +} - for _, userValue := range strings.Split(proxy.Spec.NoProxy, ",") { - set.Insert(userValue) +// isSpecTrustedCASet checks whether spec.trustedCA of proxy is set. +func isSpecTrustedCASet(proxyConfig *configv1.ProxySpec) bool { + if len(proxyConfig.TrustedCA.Name) == 0 { + return false } - updated.Status.NoProxy = strings.Join(set.List(), ",") + return true +} - if !proxyStatusesEqual(proxy.Status, updated.Status) { - if err := r.client.Status().Update(context.TODO(), updated); err != nil { - return fmt.Errorf("failed to update proxy status: %v", err) - } +// isExpectedProxyConfigMap checks whether the ConfigMap name in +// spec.trustedCA is "user-ca-bundle". +func isExpectedProxyConfigMap(proxyConfig *configv1.ProxySpec) bool { + if proxyConfig.TrustedCA.Name != names.PROXY_TRUSTED_CA_CONFIGMAP { + return false } - return nil + return true } -// proxyStatusesEqual compares two ProxyStatus values. Returns true if the -// provided values should be considered equal for the purpose of determining -// whether an update is necessary, false otherwise. -func proxyStatusesEqual(a, b configv1.ProxyStatus) bool { - if a.HTTPProxy != b.HTTPProxy || a.HTTPSProxy != b.HTTPSProxy || a.NoProxy != b.NoProxy { +// isSpecReadinessEndpoints checks whether spec.readinessEndpoints of +// proxy is set. +func isSpecReadinessEndpoints(proxyConfig *configv1.ProxySpec) bool { + if len(proxyConfig.ReadinessEndpoints) == 0 { return false } diff --git a/pkg/controller/proxyconfig/status.go b/pkg/controller/proxyconfig/status.go new file mode 100644 index 0000000000..de9ac9cd4d --- /dev/null +++ b/pkg/controller/proxyconfig/status.go @@ -0,0 +1,117 @@ +package proxyconfig + +import ( + "context" + "fmt" + "net/url" + "strconv" + "strings" + + "github.com/ghodss/yaml" + + configv1 "github.com/openshift/api/config/v1" + + corev1 "k8s.io/api/core/v1" + "k8s.io/apimachinery/pkg/util/sets" +) + +// syncProxyStatus computes the current status of proxy and +// updates status if any changes since last sync. +func (r *ReconcileProxyConfig) syncProxyStatus(proxy *configv1.Proxy, infra *configv1.Infrastructure, network *configv1.Network, cluster *corev1.ConfigMap) error { + updated := proxy.DeepCopy() + noProxy, err := mergeUserSystemNoProxy(proxy, infra, network, cluster) + if err != nil { + return fmt.Errorf("failed to merge user/system noProxy settings: %v", err) + } + + updated.Status.NoProxy = noProxy + updated.Status.HTTPProxy = proxy.Spec.HTTPProxy + updated.Status.HTTPSProxy = proxy.Spec.HTTPSProxy + + if !proxyStatusesEqual(proxy.Status, updated.Status) { + if err := r.client.Status().Update(context.TODO(), updated); err != nil { + return fmt.Errorf("failed to update proxy status: %v", err) + } + } + + return nil +} + +// mergeUserSystemNoProxy merges user-supplied noProxy settings from proxy +// with cluster-wide noProxy settings, returning a merged, comma-separated +// string of noProxy settings. +func mergeUserSystemNoProxy(proxy *configv1.Proxy, infra *configv1.Infrastructure, network *configv1.Network, cluster *corev1.ConfigMap) (string, error) { + apiServerURL, err := url.Parse(infra.Status.APIServerURL) + if err != nil { + return "", fmt.Errorf("failed to parse API server URL") + } + + internalAPIServer, err := url.Parse(infra.Status.APIServerInternalURL) + if err != nil { + return "", fmt.Errorf("failed to parse API server internal URL") + } + + set := sets.NewString( + "127.0.0.1", + "localhost", + network.Status.ServiceNetwork[0], + apiServerURL.Hostname(), + internalAPIServer.Hostname(), + ) + platform := infra.Status.PlatformStatus.Type + + // TODO: Does a better way exist to get machineCIDR and control-plane replicas? + type installConfig struct { + ControlPlane struct { + Replicas string `json:"replicas"` + } `json:"controlPlane"` + Networking struct { + MachineCIDR string `json:"machineCIDR"` + } `json:"networking"` + } + var ic installConfig + data, ok := cluster.Data["install-config"] + if !ok { + return "", fmt.Errorf("missing install-config in configmap") + } + if err := yaml.Unmarshal([]byte(data), &ic); err != nil { + return "", fmt.Errorf("invalid install-config: %v\njson:\n%s", err, data) + } + + if platform != configv1.VSpherePlatformType && + platform != configv1.NonePlatformType && + platform != configv1.BareMetalPlatformType { + set.Insert("169.254.169.254", ic.Networking.MachineCIDR) + } + + replicas, err := strconv.Atoi(ic.ControlPlane.Replicas) + if err != nil { + return "", fmt.Errorf("failed to parse install config replicas: %v", err) + } + + for i := int64(0); i < int64(replicas); i++ { + etcdHost := fmt.Sprintf("etcd-%d.%s", i, infra.Status.EtcdDiscoveryDomain) + set.Insert(etcdHost) + } + + for _, clusterNetwork := range network.Status.ClusterNetwork { + set.Insert(clusterNetwork.CIDR) + } + + for _, userValue := range strings.Split(proxy.Spec.NoProxy, ",") { + set.Insert(userValue) + } + + return strings.Join(set.List(), ","), nil +} + +// proxyStatusesEqual compares two ProxyStatus values. Returns true if the +// provided values should be considered equal for the purpose of determining +// whether an update is necessary, false otherwise. +func proxyStatusesEqual(a, b configv1.ProxyStatus) bool { + if a.HTTPProxy != b.HTTPProxy || a.HTTPSProxy != b.HTTPSProxy || a.NoProxy != b.NoProxy { + return false + } + + return true +} diff --git a/pkg/proxy/proxy_config.go b/pkg/controller/proxyconfig/validation.go similarity index 52% rename from pkg/proxy/proxy_config.go rename to pkg/controller/proxyconfig/validation.go index b12a527c99..6ed6cf3f22 100644 --- a/pkg/proxy/proxy_config.go +++ b/pkg/controller/proxyconfig/validation.go @@ -1,9 +1,10 @@ -package proxy +package proxyconfig import ( "context" "crypto/tls" "crypto/x509" + "encoding/pem" "errors" "fmt" "net" @@ -13,51 +14,41 @@ import ( "strings" configv1 "github.com/openshift/api/config/v1" + "github.com/openshift/cluster-network-operator/pkg/names" corev1 "k8s.io/api/core/v1" - "k8s.io/apimachinery/pkg/types" k8serrors "k8s.io/apimachinery/pkg/util/errors" "k8s.io/apimachinery/pkg/util/validation" - "sigs.k8s.io/controller-runtime/pkg/client" ) const ( - // The number of times the controller will attempt to issue an http GET - // to the endpoint specified in readinessEndpoints. + // proxyProbeMaxRetries is the number of times to attempt an http GET + // to a readinessEndpoints endpoint. proxyProbeMaxRetries = 3 - // clusterConfigMapName is the name of the proxy.spec.trustedCA - // ConfigMap that contains the CA bundle certificate. - clusterConfigMapName = "proxy-ca" - // clusterConfigMapNamespace is the name of the namespace that hosts - // the proxy.spec.trustedCA ConfigMap. - clusterConfigMapNamespace = "openshift-config-managed" - // clusterConfigMapKey is the name of the data key containing the PEM encoded - // CA certificate trust bundle in clusterConfigMapName. + // clusterConfigMapKey is the name of the data key containing the + // PEM-encoded CA trust bundle. clusterConfigMapKey = "ca-bundle.crt" - // installerConfigMapName is the name of the ConfigMap generated - // by the installer containing the user-provided CA certificate bundle. - installerConfigMapName = "proxy-ca-bundle" - // installerConfigMapNamespace is the name of the namespace that hosts the - // ConfigMap containing the user-provided CA certificate bundle. - installerConfigMapNamespace = "openshift-config" - - proxyHTTPScheme = "http" - proxyHTTPSScheme = "https" + proxyHTTPScheme = "http" + proxyHTTPSScheme = "https" + // certPEMBlock is the type taken from the preamble of a PEM-encoded structure. + certPEMBlock = "CERTIFICATE" ) -// ValidateProxyConfig ensures the proxy config is valid. -func ValidateProxyConfig(cli client.Client, proxyConfig configv1.ProxySpec) error { - if len(proxyConfig.HTTPProxy) != 0 { +// ValidateProxyConfig ensures that proxyConfig is valid. +func (r *ReconcileProxyConfig) ValidateProxyConfig(proxyConfig *configv1.ProxySpec) error { + if isSpecHTTPProxySet(proxyConfig) { scheme, err := validateURI(proxyConfig.HTTPProxy) if err != nil { return fmt.Errorf("invalid httpProxy URI: %v", err) } if scheme != proxyHTTPScheme { - return fmt.Errorf("httpProxy requires a %q URI scheme", proxyHTTPScheme) + return fmt.Errorf("httpProxy requires an %q URI scheme", proxyHTTPScheme) } } - if len(proxyConfig.HTTPSProxy) != 0 { - if len(proxyConfig.TrustedCA.Name) != 0 { + + var readinessCerts []*x509.Certificate + if isSpecHTTPSProxySet(proxyConfig) { + if !isSpecTrustedCASet(proxyConfig) { return errors.New("trustedCA is required when using httpsProxy") } scheme, err := validateURI(proxyConfig.HTTPSProxy) @@ -65,10 +56,15 @@ func ValidateProxyConfig(cli client.Client, proxyConfig configv1.ProxySpec) erro return fmt.Errorf("invalid httpsProxy URI: %v", err) } if scheme != proxyHTTPSScheme { - return fmt.Errorf("httpsProxy requires a %q URI scheme", proxyHTTPSScheme) + certBundle, err := r.validateTrustedCA(proxyConfig) + if err != nil { + return fmt.Errorf("failed validating TrustedCA %q: %v", proxyConfig.TrustedCA.Name, err) + } + copy(certBundle, readinessCerts[0:]) } } - if len(proxyConfig.NoProxy) != 0 { + + if isSpecNoProxySet(proxyConfig) { for _, v := range strings.Split(proxyConfig.NoProxy, ",") { v = strings.TrimSpace(v) errDomain := validateDomainName(v, false) @@ -78,20 +74,8 @@ func ValidateProxyConfig(cli client.Client, proxyConfig configv1.ProxySpec) erro } } } - if len(proxyConfig.TrustedCA.Name) != 0 { - if proxyConfig.TrustedCA.Name != clusterConfigMapName { - return fmt.Errorf("invalid ConfigMap reference for TrustedCA: %s", proxyConfig.TrustedCA.Name) - } - cfgMap := &corev1.ConfigMap{} - if err := cli.Get(context.TODO(), clusterCAConfigMapName(), cfgMap); err != nil { - return err - } - // TODO: Have validateClusterCABundle return certBundle []byte containing the validated ca bundle. - if err := validateClusterCABundle(cfgMap); err != nil { - return fmt.Errorf("validation failed for trustedCA %s: %v", proxyConfig.TrustedCA.Name, err) - } - } - if proxyConfig.ReadinessEndpoints != nil { + + if isSpecReadinessEndpoints(proxyConfig) { for _, endpoint := range proxyConfig.ReadinessEndpoints { scheme, err := validateURI(endpoint) if err != nil { @@ -102,11 +86,13 @@ func ValidateProxyConfig(cli client.Client, proxyConfig configv1.ProxySpec) erro if err := validateHTTPReadinessEndpoint(endpoint); err != nil { return fmt.Errorf("readinessEndpoint probe failed for endpoint %s", endpoint) } - // TODO: Uncomment after validateClusterCABundle() returns a validated ca bundle. - /*case scheme == proxyHTTPSScheme: - if err := validateHTTPSReadinessEndpoint(caBundle, endpoint); err != nil { + case scheme == proxyHTTPSScheme: + if !isSpecTrustedCASet(proxyConfig) { + return fmt.Errorf("readinessEndpoint with an %q scheme requires trustedCA to be set", proxyHTTPSScheme) + } + if err := validateHTTPSReadinessEndpoint(readinessCerts, endpoint); err != nil { return fmt.Errorf("readinessEndpoint probe failed for endpoint %s", endpoint) - }*/ + } default: return fmt.Errorf("readiness endpoints requires a %q or %q URI sheme", proxyHTTPScheme, proxyHTTPSScheme) } @@ -116,8 +102,7 @@ func ValidateProxyConfig(cli client.Client, proxyConfig configv1.ProxySpec) erro return nil } -// validateURI validates if url is a valid absolute URI and returns -// the url scheme. +// validateURI validates uri as being a valid url and returns the url scheme. func validateURI(uri string) (string, error) { parsed, err := url.Parse(uri) if err != nil { @@ -174,8 +159,7 @@ func validateHTTPReadinessEndpoint(endpoint string) error { } // validateHTTPReadinessEndpointWithRetries tries to validate an http -// endpoint in a finite loop based on the scheme type, it returns the -// last result if it never succeeds. +// endpoint in a finite loop and returns the last result if it never succeeds. func validateHTTPReadinessEndpointWithRetries(endpoint string, retries int) error { for i := 0; i < retries; i++ { if err := runHTTPReadinessProbe(endpoint); err != nil { @@ -186,18 +170,14 @@ func validateHTTPReadinessEndpointWithRetries(endpoint string, retries int) erro return nil } -// runHTTPReadinessProbe issues an http GET request to an http endpoint -// and returns an error if a 2XX or 3XX http status code is not returned. +// runHTTPReadinessProbe issues an http GET request to endpoint and returns +// an error if a 2XX or 3XX http status code is not returned. func runHTTPReadinessProbe(endpoint string) error { resp, err := http.Get(endpoint) if err != nil { return err } - defer func() { - if err := resp.Body.Close(); err != nil { - fmt.Errorf("failed to close connection: %v", err) - } - }() + defer resp.Body.Close() if resp.StatusCode >= http.StatusOK && resp.StatusCode < http.StatusBadRequest { return nil @@ -206,8 +186,8 @@ func runHTTPReadinessProbe(endpoint string) error { return fmt.Errorf("HTTP probe failed with statuscode: %d", resp.StatusCode) } -// validateHTTPSReadinessEndpoint validates an https readinessEndpoint endpoint. -func validateHTTPSReadinessEndpoint(certBundle []byte, endpoint string) error { +// validateHTTPSReadinessEndpoint validates endpoint using certBundle as trusted CAs. +func validateHTTPSReadinessEndpoint(certBundle []*x509.Certificate, endpoint string) error { if err := validateHTTPSReadinessEndpointWithRetries(certBundle, endpoint, proxyProbeMaxRetries); err != nil { return err } @@ -215,10 +195,10 @@ func validateHTTPSReadinessEndpoint(certBundle []byte, endpoint string) error { return nil } -// validateHTTPSReadinessEndpointWithRetries tries to validate an endpoint -// by using certBundle to attempt a TLS handshake in a finite loop returning -// the last result if it never succeeds. -func validateHTTPSReadinessEndpointWithRetries(certBundle []byte, endpoint string, retries int) error { +// validateHTTPSReadinessEndpointWithRetries tries to validate endpoint +// by using certBundle as trusted CAs to create a TLS connection using a +// finite loop based on retries, returning an error if it never succeeds. +func validateHTTPSReadinessEndpointWithRetries(certBundle []*x509.Certificate, endpoint string, retries int) error { for i := 0; i < retries; i++ { if err := runHTTPSReadinessProbe(certBundle, endpoint); err != nil { return err @@ -228,16 +208,16 @@ func validateHTTPSReadinessEndpointWithRetries(certBundle []byte, endpoint strin return nil } -// runHTTPSReadinessProbe tries connecting to endpoint by using certBundle -// to attempt a TLS handshake. -func runHTTPSReadinessProbe(certBundle []byte, endpoint string) error { +// runHTTPSReadinessProbe tries connecting to endpoint by using certBundle as +// trusted CAs to create a TLS connection, returning an error if it never succeeds. +func runHTTPSReadinessProbe(certBundle []*x509.Certificate, endpoint string) error { parsedURL, err := url.Parse(endpoint) if err != nil { return fmt.Errorf("failed parsing URL for endpoint: %s", endpoint) } certPool := x509.NewCertPool() - if !certPool.AppendCertsFromPEM(certBundle) { - return fmt.Errorf("failed to parse CA certificate bundle") + for _, cert := range certBundle { + certPool.AddCert(cert) } port := parsedURL.Port() if len(port) == 0 { @@ -250,24 +230,20 @@ func runHTTPSReadinessProbe(certBundle []byte, endpoint string) error { if err != nil { return fmt.Errorf("failed to connect to endpoint %q: %v", endpoint, err) } - defer func() { - if err := conn.Close(); err != nil { - fmt.Errorf("failed to close connection: %v", err) - } - }() - return nil + return conn.Close() } -// validateDomainName checks if the given string is a valid domain name and returns an error if not. +// validateDomainName checks if the given string is a valid domain name. func validateDomainName(v string, acceptTrailingDot bool) error { + v = strings.TrimPrefix(v, ".") if acceptTrailingDot { v = strings.TrimSuffix(v, ".") } return validateSubdomain(v) } -// validateSubdomain checks if the given string is a valid subdomain name and returns an error if not. +// validateSubdomain checks if the given string is a valid subdomain name. func validateSubdomain(v string) error { validationMessages := validation.IsDNS1123Subdomain(v) if len(validationMessages) == 0 { @@ -281,35 +257,78 @@ func validateSubdomain(v string) error { return k8serrors.NewAggregate(errs) } -// validateClusterCABundle validates that configMap contains a -// CA certificate bundle named clusterConfigMapKey and that -// clusterConfigMapKey contains a valid x.509 certificate. -func validateClusterCABundle(configMap *corev1.ConfigMap) error { - if _, ok := configMap.Data[clusterConfigMapKey]; !ok { - return fmt.Errorf("ConfigMap %q is missing %q", clusterConfigMapName, clusterConfigMapKey) +// validateTrustedCA validates that trustedCA of proxyConfig is a +// valid ConfigMap reference and that the configmap contains a +// valid CA trust bundle. +func (r *ReconcileProxyConfig) validateTrustedCA(proxyConfig *configv1.ProxySpec) ([]*x509.Certificate, error) { + cfgMap, err := r.validateTrustedCAConfigMap(proxyConfig) + if err != nil { + return nil, err } - _, err := x509.ParseCertificates([]byte(configMap.Data[clusterConfigMapKey])) + + caBundle, err := validateTrustedCABundle(cfgMap) if err != nil { - return err + return nil, err } - return nil + return caBundle, nil } -// installerCAConfigMapName returns the namespaced name of the ConfigMap -// containing the installer-generated CA certificate bundle. -func installerCAConfigMapName() types.NamespacedName { - return types.NamespacedName{ - Namespace: installerConfigMapNamespace, - Name: installerConfigMapName, +// validateTrustedCAConfigMap validates that proxyConfig is a +// valid ConfigMap reference +func (r *ReconcileProxyConfig) validateTrustedCAConfigMap(proxyConfig *configv1.ProxySpec) (*corev1.ConfigMap, error) { + if !isExpectedProxyConfigMap(proxyConfig) { + return nil, fmt.Errorf("invalid ConfigMap reference for TrustedCA: %s", proxyConfig.TrustedCA.Name) + } + cfgMap := &corev1.ConfigMap{} + if err := r.client.Get(context.TODO(), names.ProxyTrustedCAConfigMap(), cfgMap); err != nil { + return nil, err } + + return cfgMap, nil } -// clusterCAConfigMapName returns the namespaced name of the ConfigMap -// containing the cluster CA certificate bundle. -func clusterCAConfigMapName() types.NamespacedName { - return types.NamespacedName{ - Namespace: clusterConfigMapNamespace, - Name: clusterConfigMapName, +// validateTrustedCABundle validates that ConfigMap contains a +// CA certificate bundle named clusterConfigMapKey and that +// clusterConfigMapKey contains a valid PEM encoded certificate. +func validateTrustedCABundle(configMap *corev1.ConfigMap) ([]*x509.Certificate, error) { + if _, ok := configMap.Data[clusterConfigMapKey]; !ok { + return nil, fmt.Errorf("ConfigMap %q is missing %q", names.PROXY_TRUSTED_CA_CONFIGMAP, clusterConfigMapKey) } + certData := []byte(configMap.Data[clusterConfigMapKey]) + if len(certData) == 0 { + return nil, fmt.Errorf("data key %q is empty from ConfigMap %q", clusterConfigMapKey, names.PROXY_TRUSTED_CA_CONFIGMAP) + } + certBundle, err := parseCertificates(certData) + if err != nil { + return nil, fmt.Errorf("failed parsing certificate data from ConfigMap %q: %v", configMap.Name, err) + } + + return certBundle, nil +} + +// parseCertificates decodes certData, ensure the PEM block type is +// certPEMBlock and that the PEM certificate block can be parsed, returning +// a slice of parsed certificates. +func parseCertificates(certData []byte) ([]*x509.Certificate, error) { + var certs []*x509.Certificate + var block *pem.Block + for len(certData) != 0 { + block, certData = pem.Decode(certData) + if block == nil { + return nil, fmt.Errorf("failed to parse certificate PEM") + } + if block.Type != certPEMBlock { + return nil, fmt.Errorf("invalid certificate PEM, must be of type %q", certPEMBlock) + + } + + cert, err := x509.ParseCertificate(block.Bytes) + if err != nil { + return nil, fmt.Errorf("failed to parse certificate: %v", err) + } + certs = append(certs, cert) + } + + return certs, nil } diff --git a/pkg/names/names.go b/pkg/names/names.go index d2ee8ddfe7..61bafb6d34 100644 --- a/pkg/names/names.go +++ b/pkg/names/names.go @@ -1,5 +1,7 @@ package names +import "k8s.io/apimachinery/pkg/types" + // some names // OperatorConfig is the name of the CRD that defines the complete @@ -34,3 +36,21 @@ const SERVICE_CA_CONFIGMAP = "openshift-service-ca" // MULTUS_VALIDATING_WEBHOOK is the name of the ValidatingWebhookConfiguration for multus-admission-controller // that is used in multus admission controller deployment const MULTUS_VALIDATING_WEBHOOK = "multus.openshift.io" + +// PROXY_TRUSTED_CA_CONFIGMAP is the name of the proxy.spec.trustedCA +// ConfigMap that contains the trusted CA certificate bundle used for +// proxying HTTPS connections. +const PROXY_TRUSTED_CA_CONFIGMAP = "user-ca-bundle" + +// PROXY_TRUSTED_CA_CONFIGMAP_NS is the namespace that hosts the +// PROXY_TRUSTED_CA_CONFIGMAP ConfigMap. +const PROXY_TRUSTED_CA_CONFIGMAP_NS = "openshift-config-managed" + +// ProxyTrustedCAConfigMap returns the namespaced name of the ConfigMap +// containing the proxy trusted CA certificate bundle. +func ProxyTrustedCAConfigMap() types.NamespacedName { + return types.NamespacedName{ + Namespace: PROXY_TRUSTED_CA_CONFIGMAP_NS, + Name: PROXY_TRUSTED_CA_CONFIGMAP, + } +} diff --git a/pkg/proxy/proxy_config_test.go b/pkg/proxy/proxy_config_test.go deleted file mode 100644 index 24ff69e9dd..0000000000 --- a/pkg/proxy/proxy_config_test.go +++ /dev/null @@ -1,3 +0,0 @@ -package proxy - -// TODO: Create proxy controller unit tests. \ No newline at end of file From eaf115833c98143b16c9cc44bd42738902762dc6 Mon Sep 17 00:00:00 2001 From: Daneyon Hansen Date: Tue, 30 Jul 2019 14:59:20 -0700 Subject: [PATCH 05/11] Adds apimachinery deps --- Gopkg.lock | 2 ++ 1 file changed, 2 insertions(+) diff --git a/Gopkg.lock b/Gopkg.lock index 6ccada9bd4..99f92f232e 100644 --- a/Gopkg.lock +++ b/Gopkg.lock @@ -1008,6 +1008,8 @@ "k8s.io/apimachinery/pkg/util/errors", "k8s.io/apimachinery/pkg/util/net", "k8s.io/apimachinery/pkg/util/runtime", + "k8s.io/apimachinery/pkg/util/sets", + "k8s.io/apimachinery/pkg/util/validation", "k8s.io/apimachinery/pkg/util/yaml", "k8s.io/client-go/discovery", "k8s.io/client-go/kubernetes/scheme", From 5689933d8f1d037ee91f7aa90c489a612651c5c2 Mon Sep 17 00:00:00 2001 From: Daneyon Hansen Date: Tue, 30 Jul 2019 16:05:54 -0700 Subject: [PATCH 06/11] Removes proxy recreation from main and fixes func comments --- cmd/cluster-network-operator/main.go | 63 +---------------------- pkg/controller/proxyconfig/config_test.go | 3 -- pkg/controller/proxyconfig/controller.go | 8 +-- pkg/controller/proxyconfig/validation.go | 4 +- 4 files changed, 6 insertions(+), 72 deletions(-) delete mode 100644 pkg/controller/proxyconfig/config_test.go diff --git a/cmd/cluster-network-operator/main.go b/cmd/cluster-network-operator/main.go index 6f373043eb..dd924cad63 100644 --- a/cmd/cluster-network-operator/main.go +++ b/cmd/cluster-network-operator/main.go @@ -7,7 +7,6 @@ import ( "net/url" "os" "runtime" - "time" configv1 "github.com/openshift/api/config/v1" operv1 "github.com/openshift/api/operator/v1" @@ -15,10 +14,6 @@ import ( k8sutil "github.com/openshift/cluster-network-operator/pkg/util/k8s" "github.com/operator-framework/operator-sdk/pkg/leader" sdkVersion "github.com/operator-framework/operator-sdk/version" - "k8s.io/apimachinery/pkg/api/errors" - metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" - "k8s.io/apimachinery/pkg/types" - "k8s.io/apimachinery/pkg/util/wait" _ "k8s.io/client-go/plugin/pkg/client/auth/gcp" "k8s.io/client-go/tools/clientcmd" "sigs.k8s.io/controller-runtime/pkg/client/config" @@ -113,62 +108,8 @@ func main() { log.Print("Starting the Cmd.") - // Create the stop channel and start the operator - stop := signals.SetupSignalHandler() - if err := start(mgr, stop); err != nil { + // Start the Cmd + if err := mgr.Start(signals.SetupSignalHandler()); err != nil { log.Fatal(err) } } - -// start creates the default Proxy if it does not exist and then -// starts the operator synchronously until a message is received -// on the stop channel. -func start(mgr manager.Manager, stop <-chan struct{}) error { - // Periodically ensure the default proxy exists. - go wait.Until(func() { - if !mgr.GetCache().WaitForCacheSync(stop) { - log.Print("failed to sync cache before ensuring default proxy") - return - } - err := ensureDefaultProxy(mgr) - if err != nil { - log.Print(err, "failed to ensure default proxy") - } - }, 1*time.Minute, stop) - - errChan := make(chan error) - go func() { - errChan <- mgr.Start(stop) - }() - - // Wait for the manager to exit or an explicit stop. - select { - case <-stop: - return nil - case err := <-errChan: - return err - } -} - -// ensureDefaultProxy creates the default proxy if it doesn't exist. -func ensureDefaultProxy(mgr manager.Manager) error { - proxy := &configv1.Proxy{ - ObjectMeta: metav1.ObjectMeta{ - Name: "cluster", - }, - } - client := mgr.GetClient() - err := client.Get(context.TODO(), types.NamespacedName{Name: proxy.Name}, proxy) - if err == nil { - return nil - } - if !errors.IsNotFound(err) { - return err - } - err = client.Create(context.TODO(), proxy) - if err != nil { - return err - } - log.Printf("created default proxy %q", proxy.Name) - return nil -} diff --git a/pkg/controller/proxyconfig/config_test.go b/pkg/controller/proxyconfig/config_test.go deleted file mode 100644 index a18ea81f99..0000000000 --- a/pkg/controller/proxyconfig/config_test.go +++ /dev/null @@ -1,3 +0,0 @@ -package proxyconfig - -// TODO: Create proxy controller unit tests. diff --git a/pkg/controller/proxyconfig/controller.go b/pkg/controller/proxyconfig/controller.go index bd8e389100..205d61f399 100644 --- a/pkg/controller/proxyconfig/controller.go +++ b/pkg/controller/proxyconfig/controller.go @@ -57,7 +57,7 @@ func add(mgr manager.Manager, r reconcile.Reconciler) error { return nil } -var _ reconcile.Reconciler = &ReconcileProxyConfig{} +//var _ reconcile.Reconciler = &ReconcileProxyConfig{} // ReconcileProxyConfig reconciles a Proxy object type ReconcileProxyConfig struct { @@ -134,12 +134,6 @@ func (r *ReconcileProxyConfig) Reconcile(request reconcile.Request) (reconcile.R return reconcile.Result{}, nil } -// isSpecHTTPAndHTTPSProxySet checks whether spec.httpProxy and spec.httpsProxy -// of proxy is set. -func isSpecHTTPAndHTTPSProxySet(proxyConfig *configv1.ProxySpec) bool { - return !isSpecHTTPProxySet(proxyConfig) && !isSpecHTTPSProxySet(proxyConfig) -} - // isSpecHTTPProxySet checks whether spec.httpProxy of proxy is set. func isSpecHTTPProxySet(proxyConfig *configv1.ProxySpec) bool { if len(proxyConfig.HTTPProxy) == 0 { diff --git a/pkg/controller/proxyconfig/validation.go b/pkg/controller/proxyconfig/validation.go index 6ed6cf3f22..8d9875e649 100644 --- a/pkg/controller/proxyconfig/validation.go +++ b/pkg/controller/proxyconfig/validation.go @@ -275,7 +275,7 @@ func (r *ReconcileProxyConfig) validateTrustedCA(proxyConfig *configv1.ProxySpec } // validateTrustedCAConfigMap validates that proxyConfig is a -// valid ConfigMap reference +// valid ConfigMap reference. func (r *ReconcileProxyConfig) validateTrustedCAConfigMap(proxyConfig *configv1.ProxySpec) (*corev1.ConfigMap, error) { if !isExpectedProxyConfigMap(proxyConfig) { return nil, fmt.Errorf("invalid ConfigMap reference for TrustedCA: %s", proxyConfig.TrustedCA.Name) @@ -292,6 +292,8 @@ func (r *ReconcileProxyConfig) validateTrustedCAConfigMap(proxyConfig *configv1. // CA certificate bundle named clusterConfigMapKey and that // clusterConfigMapKey contains a valid PEM encoded certificate. func validateTrustedCABundle(configMap *corev1.ConfigMap) ([]*x509.Certificate, error) { + // TODO: A separate controller is needed to watch for changes to the proxy + // ca bundle configmap. if _, ok := configMap.Data[clusterConfigMapKey]; !ok { return nil, fmt.Errorf("ConfigMap %q is missing %q", names.PROXY_TRUSTED_CA_CONFIGMAP, clusterConfigMapKey) } From 9e17a4345334dcf9cd425d89d5e69da857354a1c Mon Sep 17 00:00:00 2001 From: Daneyon Hansen Date: Wed, 31 Jul 2019 11:11:13 -0700 Subject: [PATCH 07/11] fixes probe bug and simplifies *Set funcs --- pkg/controller/proxyconfig/controller.go | 36 ++++-------------------- pkg/controller/proxyconfig/validation.go | 8 ++++-- 2 files changed, 11 insertions(+), 33 deletions(-) diff --git a/pkg/controller/proxyconfig/controller.go b/pkg/controller/proxyconfig/controller.go index 205d61f399..57ddc467d3 100644 --- a/pkg/controller/proxyconfig/controller.go +++ b/pkg/controller/proxyconfig/controller.go @@ -136,56 +136,32 @@ func (r *ReconcileProxyConfig) Reconcile(request reconcile.Request) (reconcile.R // isSpecHTTPProxySet checks whether spec.httpProxy of proxy is set. func isSpecHTTPProxySet(proxyConfig *configv1.ProxySpec) bool { - if len(proxyConfig.HTTPProxy) == 0 { - return false - } - - return true + return len(proxyConfig.HTTPProxy) == 0 } // isSpecHTTPSProxySet checks whether spec.httpsProxy of proxy is set. func isSpecHTTPSProxySet(proxyConfig *configv1.ProxySpec) bool { - if len(proxyConfig.HTTPSProxy) == 0 { - return false - } - - return true + return len(proxyConfig.HTTPSProxy) == 0 } // isSpecNoProxySet checks whether spec.NoProxy of proxy is set. func isSpecNoProxySet(proxyConfig *configv1.ProxySpec) bool { - if len(proxyConfig.NoProxy) == 0 { - return false - } - - return true + return len(proxyConfig.NoProxy) == 0 } // isSpecTrustedCASet checks whether spec.trustedCA of proxy is set. func isSpecTrustedCASet(proxyConfig *configv1.ProxySpec) bool { - if len(proxyConfig.TrustedCA.Name) == 0 { - return false - } - - return true + return len(proxyConfig.TrustedCA.Name) == 0 } // isExpectedProxyConfigMap checks whether the ConfigMap name in // spec.trustedCA is "user-ca-bundle". func isExpectedProxyConfigMap(proxyConfig *configv1.ProxySpec) bool { - if proxyConfig.TrustedCA.Name != names.PROXY_TRUSTED_CA_CONFIGMAP { - return false - } - - return true + return proxyConfig.TrustedCA.Name != names.PROXY_TRUSTED_CA_CONFIGMAP } // isSpecReadinessEndpoints checks whether spec.readinessEndpoints of // proxy is set. func isSpecReadinessEndpoints(proxyConfig *configv1.ProxySpec) bool { - if len(proxyConfig.ReadinessEndpoints) == 0 { - return false - } - - return true + return len(proxyConfig.ReadinessEndpoints) == 0 } diff --git a/pkg/controller/proxyconfig/validation.go b/pkg/controller/proxyconfig/validation.go index 8d9875e649..7c463b7796 100644 --- a/pkg/controller/proxyconfig/validation.go +++ b/pkg/controller/proxyconfig/validation.go @@ -161,13 +161,15 @@ func validateHTTPReadinessEndpoint(endpoint string) error { // validateHTTPReadinessEndpointWithRetries tries to validate an http // endpoint in a finite loop and returns the last result if it never succeeds. func validateHTTPReadinessEndpointWithRetries(endpoint string, retries int) error { + var err error for i := 0; i < retries; i++ { - if err := runHTTPReadinessProbe(endpoint); err != nil { - return err + err = runHTTPReadinessProbe(endpoint) + if err == nil { + return nil } } - return nil + return err } // runHTTPReadinessProbe issues an http GET request to endpoint and returns From 48855060c36b4161fa0bbf1e7af50721c588dff0 Mon Sep 17 00:00:00 2001 From: Daneyon Hansen Date: Wed, 31 Jul 2019 12:35:47 -0700 Subject: [PATCH 08/11] refactors proxy status sync --- pkg/controller/proxyconfig/controller.go | 187 +++++++++++++++-------- pkg/controller/proxyconfig/status.go | 20 ++- pkg/controller/proxyconfig/validation.go | 182 +++++----------------- pkg/names/names.go | 41 +++-- pkg/util/validation/network.go | 84 ++++++++++ pkg/util/validation/trustbundle.go | 62 ++++++++ 6 files changed, 342 insertions(+), 234 deletions(-) create mode 100644 pkg/util/validation/network.go create mode 100644 pkg/util/validation/trustbundle.go diff --git a/pkg/controller/proxyconfig/controller.go b/pkg/controller/proxyconfig/controller.go index 57ddc467d3..378ea4635d 100644 --- a/pkg/controller/proxyconfig/controller.go +++ b/pkg/controller/proxyconfig/controller.go @@ -8,6 +8,7 @@ import ( configv1 "github.com/openshift/api/config/v1" "github.com/openshift/cluster-network-operator/pkg/controller/statusmanager" "github.com/openshift/cluster-network-operator/pkg/names" + cnovalidation "github.com/openshift/cluster-network-operator/pkg/util/validation" corev1 "k8s.io/api/core/v1" apierrors "k8s.io/apimachinery/pkg/api/errors" @@ -15,8 +16,10 @@ import ( "k8s.io/apimachinery/pkg/types" "sigs.k8s.io/controller-runtime/pkg/client" "sigs.k8s.io/controller-runtime/pkg/controller" + "sigs.k8s.io/controller-runtime/pkg/event" "sigs.k8s.io/controller-runtime/pkg/handler" "sigs.k8s.io/controller-runtime/pkg/manager" + "sigs.k8s.io/controller-runtime/pkg/predicate" "sigs.k8s.io/controller-runtime/pkg/reconcile" "sigs.k8s.io/controller-runtime/pkg/source" ) @@ -48,7 +51,34 @@ func add(mgr manager.Manager, r reconcile.Reconciler) error { return err } - // Watch for changes to primary resource config.openshift.io/v1/Proxy + // We only care about a configmap source with a specific name/namespace, + // so filter events before they are provided to the controller event handlers. + pred := predicate.Funcs{ + UpdateFunc: func(e event.UpdateEvent) bool { + return e.MetaOld.GetName() == names.MERGED_TRUST_BUNDLE_CONFIGMAP && + e.MetaOld.GetNamespace() == names.TRUST_BUNDLE_CONFIGMAP_NS + }, + DeleteFunc: func(e event.DeleteEvent) bool { + return e.Meta.GetName() == names.MERGED_TRUST_BUNDLE_CONFIGMAP && + e.Meta.GetNamespace() == names.TRUST_BUNDLE_CONFIGMAP_NS + }, + CreateFunc: func(e event.CreateEvent) bool { + return e.Meta.GetName() == names.MERGED_TRUST_BUNDLE_CONFIGMAP && + e.Meta.GetNamespace() == names.TRUST_BUNDLE_CONFIGMAP_NS + }, + GenericFunc: func(e event.GenericEvent) bool { + return e.Meta.GetName() == names.MERGED_TRUST_BUNDLE_CONFIGMAP && + e.Meta.GetNamespace() == names.TRUST_BUNDLE_CONFIGMAP_NS + }, + } + + // Watch for changes to the user/system merged configmap + err = c.Watch(&source.Kind{Type: &corev1.ConfigMap{}}, &handler.EnqueueRequestForObject{}, pred) + if err != nil { + return err + } + + // Watch for changes to resource config.openshift.io/v1/Proxy err = c.Watch(&source.Kind{Type: &configv1.Proxy{}}, &handler.EnqueueRequestForObject{}) if err != nil { return err @@ -71,97 +101,120 @@ type ReconcileProxyConfig struct { // Reconcile expects request to refer to a proxy object named "cluster" in the // default namespace, and will ensure proxy is in the desired state. func (r *ReconcileProxyConfig) Reconcile(request reconcile.Request) (reconcile.Result, error) { - log.Printf("Reconciling Proxy.config.openshift.io %s\n", request.Name) - - // Only reconcile a proxy object named "cluster". - if request.Name != names.PROXY_CONFIG { - log.Printf("Ignoring Proxy without default name " + names.PROXY_CONFIG) - return reconcile.Result{}, nil - } - - // Fetch the proxy config + // Collect required config objects for proxy reconciliation. proxyConfig := &configv1.Proxy{} - err := r.client.Get(context.TODO(), request.NamespacedName, proxyConfig) - if err != nil { - if apierrors.IsNotFound(err) { - // Request object not found, could have been deleted after reconcile request. - // Return and don't requeue - log.Println("proxy not found; reconciliation will be skipped", "request", request) - return reconcile.Result{}, nil - } - // Error reading the object - requeue the request. - return reconcile.Result{}, fmt.Errorf("failed to get proxy %q: %v", request, err) - } - - // A nil proxy is generated by upgrades and installs not requiring a proxy. - if !isSpecHTTPProxySet(&proxyConfig.Spec) && !isSpecHTTPSProxySet(&proxyConfig.Spec) { - log.Println("httpProxy and httpsProxy not defined; reconciliation will be skipped", "request", request) - return reconcile.Result{}, nil - } - - // Only proceed if the required config objects can be collected. infraConfig := &configv1.Infrastructure{} - if err := r.client.Get(context.TODO(), types.NamespacedName{Name: "cluster"}, infraConfig); err != nil { - return reconcile.Result{}, fmt.Errorf("failed to get infrastructure config 'cluster': %v", err) - } netConfig := &configv1.Network{} - if err := r.client.Get(context.TODO(), types.NamespacedName{Name: "cluster"}, netConfig); err != nil { - log.Printf("failed to get network config 'cluster': %v", err) - return reconcile.Result{}, err - } clusterConfig := &corev1.ConfigMap{} - if err := r.client.Get(context.TODO(), types.NamespacedName{Name: "cluster-config-v1", Namespace: "kube-system"}, clusterConfig); err != nil { - log.Printf("failed to get configmap 'cluster': %v", err) - return reconcile.Result{}, err - } + switch { + case request.NamespacedName == names.ProxyName(): + log.Printf("Reconciling proxy: %s\n", request.Name) + err := r.client.Get(context.TODO(), request.NamespacedName, proxyConfig) + if err != nil { + if apierrors.IsNotFound(err) { + // Request object not found, could have been deleted after reconcile request. + // Return and don't requeue + log.Println("proxy not found; reconciliation will be skipped", "request", request) + return reconcile.Result{}, nil + } + // Error reading the object - requeue the request. + return reconcile.Result{}, fmt.Errorf("failed to get proxy %q: %v", request, err) + } - if err := r.ValidateProxyConfig(&proxyConfig.Spec); err != nil { - log.Printf("Failed to validate Proxy.Spec: %v", err) - r.status.SetDegraded(statusmanager.ProxyConfig, "InvalidProxyConfig", - fmt.Sprintf("The proxy configuration is invalid (%v). Use 'oc edit proxy.config.openshift.io cluster' to fix.", err)) - } + // A nil proxy is generated by upgrades and installs not requiring a proxy. + if !isSpecHTTPProxySet(&proxyConfig.Spec) && !isSpecHTTPSProxySet(&proxyConfig.Spec) { + log.Printf("httpProxy and httpsProxy not defined; reconciliation will be skipped for proxy: %s\n", + request.Name) + return reconcile.Result{}, nil + } - // Update Proxy.config.openshift.io.Status - if err := r.syncProxyStatus(proxyConfig, infraConfig, netConfig, clusterConfig); err != nil { - log.Printf("Could not sync proxy status: %v", err) - r.status.SetDegraded(statusmanager.ProxyConfig, "StatusError", - fmt.Sprintf("Could not update proxy configuration status: %v", err)) - return reconcile.Result{}, err + // Only proceed if the required config objects can be collected. + if err := r.client.Get(context.TODO(), types.NamespacedName{Name: "cluster"}, infraConfig); err != nil { + return reconcile.Result{}, fmt.Errorf("failed to get infrastructure config 'cluster': %v", err) + } + if err := r.client.Get(context.TODO(), types.NamespacedName{Name: "cluster"}, netConfig); err != nil { + log.Printf("failed to get network config 'cluster': %v", err) + return reconcile.Result{}, err + } + if err := r.client.Get(context.TODO(), types.NamespacedName{Name: "cluster-config-v1", Namespace: "kube-system"}, clusterConfig); err != nil { + log.Printf("failed to get configmap 'cluster': %v", err) + return reconcile.Result{}, err + } + if err := r.ValidateProxyConfig(&proxyConfig.Spec); err != nil { + log.Printf("Failed to validate Proxy.Spec: %v", err) + r.status.SetDegraded(statusmanager.ProxyConfig, "InvalidProxyConfig", + fmt.Sprintf("The proxy configuration is invalid (%v). Use 'oc edit proxy.config.openshift.io cluster' to fix.", err)) + return reconcile.Result{}, nil + } + // Update Proxy.config.openshift.io.Status + if err := r.syncProxyStatus(proxyConfig, infraConfig, netConfig, clusterConfig); err != nil { + log.Printf("Could not sync proxy status: %v", err) + r.status.SetDegraded(statusmanager.ProxyConfig, "StatusError", + fmt.Sprintf("Could not update proxy configuration status: %v", err)) + return reconcile.Result{}, err + } + log.Printf("Reconciling proxy: %s complete\n", request.Name) + case request.NamespacedName == names.MergedTrustBundleName(): + cfgMap := &corev1.ConfigMap{} + log.Printf("Reconciling configmap: %s/%s\n", request.Namespace, request.Name) + err := r.client.Get(context.TODO(), request.NamespacedName, cfgMap) + if err != nil { + if apierrors.IsNotFound(err) { + // Request object not found, could have been deleted after reconcile request. + // Return and don't requeue + log.Println("configmap not found; reconciliation will be skipped", "request", request) + return reconcile.Result{}, nil + } + // Error reading the object - requeue the request. + return reconcile.Result{}, fmt.Errorf("failed to get configmap %q: %v", request, err) + } + if _, _, err := cnovalidation.TrustBundleConfigMap(cfgMap); err != nil { + log.Printf("Failed to validate configmap: %v", err) + r.status.SetDegraded(statusmanager.ProxyConfig, "InvalidTrustedCAConfigMap", + fmt.Sprintf("The configmap is invalid (%v). Use 'oc edit configmap %s -n %s' to fix.", err, + request.Name, request.Namespace)) + return reconcile.Result{}, nil + } + log.Printf("Reconciling configmap: %s/%s complete\n", request.Namespace, request.Name) + default: + // unknown object + log.Println("Ignoring unknown object, reconciliation will be skipped", "request", request) } r.status.SetNotDegraded(statusmanager.ProxyConfig) - log.Printf("Reconciling Proxy.config.openshift.io %s complete\n", request.Name) return reconcile.Result{}, nil } -// isSpecHTTPProxySet checks whether spec.httpProxy of proxy is set. +// isSpecHTTPProxySet returns true if spec.httpProxy of +// proxyConfig is set. func isSpecHTTPProxySet(proxyConfig *configv1.ProxySpec) bool { - return len(proxyConfig.HTTPProxy) == 0 + return len(proxyConfig.HTTPProxy) > 0 } -// isSpecHTTPSProxySet checks whether spec.httpsProxy of proxy is set. +// isSpecHTTPSProxySet returns true if spec.httpsProxy of +// proxyConfig is set. func isSpecHTTPSProxySet(proxyConfig *configv1.ProxySpec) bool { - return len(proxyConfig.HTTPSProxy) == 0 + return len(proxyConfig.HTTPSProxy) > 0 } -// isSpecNoProxySet checks whether spec.NoProxy of proxy is set. +// isSpecNoProxySet returns true if spec.NoProxy of proxyConfig is set. func isSpecNoProxySet(proxyConfig *configv1.ProxySpec) bool { - return len(proxyConfig.NoProxy) == 0 + return len(proxyConfig.NoProxy) > 0 } -// isSpecTrustedCASet checks whether spec.trustedCA of proxy is set. +// isSpecTrustedCASet returns true if spec.trustedCA of proxyConfig is set. func isSpecTrustedCASet(proxyConfig *configv1.ProxySpec) bool { - return len(proxyConfig.TrustedCA.Name) == 0 + return len(proxyConfig.TrustedCA.Name) > 0 } -// isExpectedProxyConfigMap checks whether the ConfigMap name in -// spec.trustedCA is "user-ca-bundle". -func isExpectedProxyConfigMap(proxyConfig *configv1.ProxySpec) bool { - return proxyConfig.TrustedCA.Name != names.PROXY_TRUSTED_CA_CONFIGMAP +// isTrustedCAConfigMap returns true if the ConfigMap name in +// spec.trustedCA is "proxy-ca-bundle". +func isTrustedCAConfigMap(proxyConfig *configv1.ProxySpec) bool { + return proxyConfig.TrustedCA.Name == names.MERGED_TRUST_BUNDLE_CONFIGMAP } -// isSpecReadinessEndpoints checks whether spec.readinessEndpoints of -// proxy is set. +// isSpecReadinessEndpoints returns true if spec.readinessEndpoints of +// proxyConfig is set. func isSpecReadinessEndpoints(proxyConfig *configv1.ProxySpec) bool { - return len(proxyConfig.ReadinessEndpoints) == 0 + return len(proxyConfig.ReadinessEndpoints) > 0 } diff --git a/pkg/controller/proxyconfig/status.go b/pkg/controller/proxyconfig/status.go index de9ac9cd4d..c35be8bd45 100644 --- a/pkg/controller/proxyconfig/status.go +++ b/pkg/controller/proxyconfig/status.go @@ -18,15 +18,24 @@ import ( // syncProxyStatus computes the current status of proxy and // updates status if any changes since last sync. func (r *ReconcileProxyConfig) syncProxyStatus(proxy *configv1.Proxy, infra *configv1.Infrastructure, network *configv1.Network, cluster *corev1.ConfigMap) error { + var err error + var noProxy string updated := proxy.DeepCopy() - noProxy, err := mergeUserSystemNoProxy(proxy, infra, network, cluster) - if err != nil { - return fmt.Errorf("failed to merge user/system noProxy settings: %v", err) + + if isSpecNoProxySet(&proxy.Spec) { + if proxy.Spec.NoProxy == noProxyWildcard { + noProxy = proxy.Spec.NoProxy + } else { + noProxy, err = mergeUserSystemNoProxy(proxy, infra, network, cluster) + if err != nil { + return fmt.Errorf("failed to merge user/system noProxy settings: %v", err) + } + } } - updated.Status.NoProxy = noProxy updated.Status.HTTPProxy = proxy.Spec.HTTPProxy updated.Status.HTTPSProxy = proxy.Spec.HTTPSProxy + updated.Status.NoProxy = noProxy if !proxyStatusesEqual(proxy.Status, updated.Status) { if err := r.client.Status().Update(context.TODO(), updated); err != nil { @@ -37,7 +46,8 @@ func (r *ReconcileProxyConfig) syncProxyStatus(proxy *configv1.Proxy, infra *con return nil } -// mergeUserSystemNoProxy merges user-supplied noProxy settings from proxy +// mergeUserSystemNoProxy returns noProxyWildcard if it supplied as a noProxy setting. +// Otherwise, mergeUserSystemNoProxy merges user-supplied noProxy settings from proxy // with cluster-wide noProxy settings, returning a merged, comma-separated // string of noProxy settings. func mergeUserSystemNoProxy(proxy *configv1.Proxy, infra *configv1.Infrastructure, network *configv1.Network, cluster *corev1.ConfigMap) (string, error) { diff --git a/pkg/controller/proxyconfig/validation.go b/pkg/controller/proxyconfig/validation.go index 7c463b7796..5780b1170d 100644 --- a/pkg/controller/proxyconfig/validation.go +++ b/pkg/controller/proxyconfig/validation.go @@ -4,40 +4,36 @@ import ( "context" "crypto/tls" "crypto/x509" - "encoding/pem" "errors" "fmt" "net" "net/http" "net/url" - "strconv" "strings" configv1 "github.com/openshift/api/config/v1" "github.com/openshift/cluster-network-operator/pkg/names" + cnovalidation "github.com/openshift/cluster-network-operator/pkg/util/validation" corev1 "k8s.io/api/core/v1" - k8serrors "k8s.io/apimachinery/pkg/util/errors" - "k8s.io/apimachinery/pkg/util/validation" ) const ( // proxyProbeMaxRetries is the number of times to attempt an http GET // to a readinessEndpoints endpoint. proxyProbeMaxRetries = 3 - // clusterConfigMapKey is the name of the data key containing the - // PEM-encoded CA trust bundle. - clusterConfigMapKey = "ca-bundle.crt" - proxyHTTPScheme = "http" - proxyHTTPSScheme = "https" - // certPEMBlock is the type taken from the preamble of a PEM-encoded structure. - certPEMBlock = "CERTIFICATE" + proxyHTTPScheme = "http" + proxyHTTPSScheme = "https" + // noProxyWildcard is the string used to as a wildcard attached to a + // domain suffix in proxy.spec.noProxy to bypass proxy for httpProxy + // and httpsProxy. + noProxyWildcard = "*" ) // ValidateProxyConfig ensures that proxyConfig is valid. func (r *ReconcileProxyConfig) ValidateProxyConfig(proxyConfig *configv1.ProxySpec) error { if isSpecHTTPProxySet(proxyConfig) { - scheme, err := validateURI(proxyConfig.HTTPProxy) + scheme, err := cnovalidation.URI(proxyConfig.HTTPProxy) if err != nil { return fmt.Errorf("invalid httpProxy URI: %v", err) } @@ -46,38 +42,44 @@ func (r *ReconcileProxyConfig) ValidateProxyConfig(proxyConfig *configv1.ProxySp } } - var readinessCerts []*x509.Certificate + readinessCerts := []*x509.Certificate{} if isSpecHTTPSProxySet(proxyConfig) { - if !isSpecTrustedCASet(proxyConfig) { - return errors.New("trustedCA is required when using httpsProxy") - } - scheme, err := validateURI(proxyConfig.HTTPSProxy) + scheme, err := cnovalidation.URI(proxyConfig.HTTPSProxy) if err != nil { return fmt.Errorf("invalid httpsProxy URI: %v", err) } - if scheme != proxyHTTPSScheme { + if scheme == proxyHTTPSScheme { + // A trusted CA bundle must be provided when using https + // between client and proxy. + if !isSpecTrustedCASet(proxyConfig) { + return errors.New("trustedCA is required when using an https scheme with httpsProxy") + } certBundle, err := r.validateTrustedCA(proxyConfig) if err != nil { return fmt.Errorf("failed validating TrustedCA %q: %v", proxyConfig.TrustedCA.Name, err) } - copy(certBundle, readinessCerts[0:]) + for _, cert := range certBundle { + readinessCerts = append(readinessCerts, cert) + } } } if isSpecNoProxySet(proxyConfig) { - for _, v := range strings.Split(proxyConfig.NoProxy, ",") { - v = strings.TrimSpace(v) - errDomain := validateDomainName(v, false) - _, _, errCIDR := net.ParseCIDR(v) - if errDomain != nil && errCIDR != nil { - return fmt.Errorf("invalid noProxy: %v", v) + if proxyConfig.NoProxy != noProxyWildcard { + for _, v := range strings.Split(proxyConfig.NoProxy, ",") { + v = strings.TrimSpace(v) + errDomain := cnovalidation.DomainName(v, false) + _, _, errCIDR := net.ParseCIDR(v) + if errDomain != nil && errCIDR != nil { + return fmt.Errorf("invalid noProxy: %v", v) + } } } } if isSpecReadinessEndpoints(proxyConfig) { for _, endpoint := range proxyConfig.ReadinessEndpoints { - scheme, err := validateURI(endpoint) + scheme, err := cnovalidation.URI(endpoint) if err != nil { return fmt.Errorf("invalid URI for endpoint %s: %v", endpoint, err) } @@ -102,53 +104,6 @@ func (r *ReconcileProxyConfig) ValidateProxyConfig(proxyConfig *configv1.ProxySp return nil } -// validateURI validates uri as being a valid url and returns the url scheme. -func validateURI(uri string) (string, error) { - parsed, err := url.Parse(uri) - if err != nil { - return "", err - } - if !parsed.IsAbs() { - return "", fmt.Errorf("failed validating URI, no scheme for URI %q", uri) - } - host := parsed.Hostname() - if err := validateHost(host); err != nil { - return "", fmt.Errorf("failed validating URI %q: %v", uri, err) - } - if port := parsed.Port(); len(port) != 0 { - intPort, err := strconv.Atoi(port) - if err != nil { - return "", fmt.Errorf("failed converting port to integer for URI %q: %v", uri, err) - } - if err := validatePort(intPort); err != nil { - return "", fmt.Errorf("failed to validate port for URL %q: %v", uri, err) - } - } - - return parsed.Scheme, nil -} - -// validateHost validates if host is a valid IP address or subdomain in DNS (RFC 1123). -func validateHost(host string) error { - errDomain := validateDomainName(host, false) - errIP := validation.IsValidIP(host) - if errDomain != nil && errIP != nil { - return fmt.Errorf("invalid host: %s", host) - } - - return nil -} - -// validatePort validates if port is a valid port number between 1-65535. -func validatePort(port int) error { - invalidPorts := validation.IsValidPortNum(port) - if invalidPorts != nil { - return fmt.Errorf("invalid port number: %d", port) - } - - return nil -} - // validateHTTPReadinessEndpoint validates an http readinessEndpoint endpoint. func validateHTTPReadinessEndpoint(endpoint string) error { if err := validateHTTPReadinessEndpointWithRetries(endpoint, proxyProbeMaxRetries); err != nil { @@ -236,103 +191,34 @@ func runHTTPSReadinessProbe(certBundle []*x509.Certificate, endpoint string) err return conn.Close() } -// validateDomainName checks if the given string is a valid domain name. -func validateDomainName(v string, acceptTrailingDot bool) error { - v = strings.TrimPrefix(v, ".") - if acceptTrailingDot { - v = strings.TrimSuffix(v, ".") - } - return validateSubdomain(v) -} - -// validateSubdomain checks if the given string is a valid subdomain name. -func validateSubdomain(v string) error { - validationMessages := validation.IsDNS1123Subdomain(v) - if len(validationMessages) == 0 { - return nil - } - - errs := make([]error, len(validationMessages)) - for i, m := range validationMessages { - errs[i] = errors.New(m) - } - return k8serrors.NewAggregate(errs) -} - // validateTrustedCA validates that trustedCA of proxyConfig is a // valid ConfigMap reference and that the configmap contains a -// valid CA trust bundle. +// valid trust bundle, returning a byte[] of the trust bundle +// data upon success. func (r *ReconcileProxyConfig) validateTrustedCA(proxyConfig *configv1.ProxySpec) ([]*x509.Certificate, error) { cfgMap, err := r.validateTrustedCAConfigMap(proxyConfig) if err != nil { return nil, err } - caBundle, err := validateTrustedCABundle(cfgMap) + certBundle, _, err := cnovalidation.TrustBundleConfigMap(cfgMap) if err != nil { return nil, err } - return caBundle, nil + return certBundle, nil } // validateTrustedCAConfigMap validates that proxyConfig is a // valid ConfigMap reference. func (r *ReconcileProxyConfig) validateTrustedCAConfigMap(proxyConfig *configv1.ProxySpec) (*corev1.ConfigMap, error) { - if !isExpectedProxyConfigMap(proxyConfig) { + if !isTrustedCAConfigMap(proxyConfig) { return nil, fmt.Errorf("invalid ConfigMap reference for TrustedCA: %s", proxyConfig.TrustedCA.Name) } cfgMap := &corev1.ConfigMap{} - if err := r.client.Get(context.TODO(), names.ProxyTrustedCAConfigMap(), cfgMap); err != nil { + if err := r.client.Get(context.TODO(), names.MergedTrustBundleName(), cfgMap); err != nil { return nil, err } return cfgMap, nil } - -// validateTrustedCABundle validates that ConfigMap contains a -// CA certificate bundle named clusterConfigMapKey and that -// clusterConfigMapKey contains a valid PEM encoded certificate. -func validateTrustedCABundle(configMap *corev1.ConfigMap) ([]*x509.Certificate, error) { - // TODO: A separate controller is needed to watch for changes to the proxy - // ca bundle configmap. - if _, ok := configMap.Data[clusterConfigMapKey]; !ok { - return nil, fmt.Errorf("ConfigMap %q is missing %q", names.PROXY_TRUSTED_CA_CONFIGMAP, clusterConfigMapKey) - } - certData := []byte(configMap.Data[clusterConfigMapKey]) - if len(certData) == 0 { - return nil, fmt.Errorf("data key %q is empty from ConfigMap %q", clusterConfigMapKey, names.PROXY_TRUSTED_CA_CONFIGMAP) - } - certBundle, err := parseCertificates(certData) - if err != nil { - return nil, fmt.Errorf("failed parsing certificate data from ConfigMap %q: %v", configMap.Name, err) - } - - return certBundle, nil -} - -// parseCertificates decodes certData, ensure the PEM block type is -// certPEMBlock and that the PEM certificate block can be parsed, returning -// a slice of parsed certificates. -func parseCertificates(certData []byte) ([]*x509.Certificate, error) { - var certs []*x509.Certificate - var block *pem.Block - for len(certData) != 0 { - block, certData = pem.Decode(certData) - if block == nil { - return nil, fmt.Errorf("failed to parse certificate PEM") - } - if block.Type != certPEMBlock { - return nil, fmt.Errorf("invalid certificate PEM, must be of type %q", certPEMBlock) - - } - - cert, err := x509.ParseCertificate(block.Bytes) - if err != nil { - return nil, fmt.Errorf("failed to parse certificate: %v", err) - } - certs = append(certs, cert) - } - - return certs, nil -} diff --git a/pkg/names/names.go b/pkg/names/names.go index 61bafb6d34..8a78f71626 100644 --- a/pkg/names/names.go +++ b/pkg/names/names.go @@ -37,20 +37,33 @@ const SERVICE_CA_CONFIGMAP = "openshift-service-ca" // that is used in multus admission controller deployment const MULTUS_VALIDATING_WEBHOOK = "multus.openshift.io" -// PROXY_TRUSTED_CA_CONFIGMAP is the name of the proxy.spec.trustedCA -// ConfigMap that contains the trusted CA certificate bundle used for -// proxying HTTPS connections. -const PROXY_TRUSTED_CA_CONFIGMAP = "user-ca-bundle" - -// PROXY_TRUSTED_CA_CONFIGMAP_NS is the namespace that hosts the -// PROXY_TRUSTED_CA_CONFIGMAP ConfigMap. -const PROXY_TRUSTED_CA_CONFIGMAP_NS = "openshift-config-managed" - -// ProxyTrustedCAConfigMap returns the namespaced name of the ConfigMap -// containing the proxy trusted CA certificate bundle. -func ProxyTrustedCAConfigMap() types.NamespacedName { +// TRUST_BUNDLE_CONFIGMAP_KEY is the name of the data key containing +// the PEM encoded trust bundle. +const TRUST_BUNDLE_CONFIGMAP_KEY = "ca-bundle.crt" + +// MERGED_TRUST_BUNDLE_CONFIGMAP is the name of the ConfigMap +// containing the combined user/system trust bundle. +// TODO: The bundle can be used for more than proxy, change name. +const MERGED_TRUST_BUNDLE_CONFIGMAP = "proxy-ca-bundle" + +// TRUST_BUNDLE_CONFIGMAP_NS is the namespace that hosts the +// ADDL_TRUST_BUNDLE_CONFIGMAP and MERGED_TRUST_BUNDLE_CONFIGMAP +// ConfigMaps. +const TRUST_BUNDLE_CONFIGMAP_NS = "openshift-config-managed" + +// ProxyName returns the namespaced name of the proxy +// object named "cluster" in namespace "openshift-config-managed". +func ProxyName() types.NamespacedName { + return types.NamespacedName{ + Name: PROXY_CONFIG, + } +} + +// MergedTrustBundleName returns the namespaced name of the ConfigMap +// containing the merged user/system trust bundle. +func MergedTrustBundleName() types.NamespacedName { return types.NamespacedName{ - Namespace: PROXY_TRUSTED_CA_CONFIGMAP_NS, - Name: PROXY_TRUSTED_CA_CONFIGMAP, + Namespace: TRUST_BUNDLE_CONFIGMAP_NS, + Name: MERGED_TRUST_BUNDLE_CONFIGMAP, } } diff --git a/pkg/util/validation/network.go b/pkg/util/validation/network.go new file mode 100644 index 0000000000..92de1684b7 --- /dev/null +++ b/pkg/util/validation/network.go @@ -0,0 +1,84 @@ +package validation + +import ( + "errors" + "fmt" + "net/url" + "strconv" + "strings" + + k8serrors "k8s.io/apimachinery/pkg/util/errors" + "k8s.io/apimachinery/pkg/util/validation" +) + +// DomainName checks if the given string is a valid domain name. +func DomainName(v string, acceptTrailingDot bool) error { + v = strings.TrimPrefix(v, ".") + if acceptTrailingDot { + v = strings.TrimSuffix(v, ".") + } + + return Subdomain(v) +} + +// Subdomain checks if the given string is a valid subdomain name. +func Subdomain(v string) error { + validationMessages := validation.IsDNS1123Subdomain(v) + if len(validationMessages) == 0 { + return nil + } + + errs := make([]error, len(validationMessages)) + for i, m := range validationMessages { + errs[i] = errors.New(m) + } + + return k8serrors.NewAggregate(errs) +} + +// Host validates if host is a valid IP address or subdomain in DNS (RFC 1123). +func Host(host string) error { + errDomain := DomainName(host, false) + errIP := validation.IsValidIP(host) + if errDomain != nil && errIP != nil { + return fmt.Errorf("invalid host: %s", host) + } + + return nil +} + +// Port validates if port is a valid port number between 1-65535. +func Port(port int) error { + invalidPorts := validation.IsValidPortNum(port) + if invalidPorts != nil { + return fmt.Errorf("invalid port number: %d", port) + } + + return nil +} + +// URI validates uri as being a valid url and returns the url scheme. +func URI(uri string) (string, error) { + parsed, err := url.Parse(uri) + if err != nil { + return "", err + } + if !parsed.IsAbs() { + return "", fmt.Errorf("failed validating URI, no scheme for URI %q", uri) + } + host := parsed.Hostname() + if err := Host(host); err != nil { + return "", fmt.Errorf("failed validating URI %q: %v", uri, err) + } + if port := parsed.Port(); len(port) != 0 { + intPort, err := strconv.Atoi(port) + if err != nil { + return "", fmt.Errorf("failed converting port to integer for URI %q: %v", uri, err) + } + if err := Port(intPort); err != nil { + return "", fmt.Errorf("failed to validate port for URL %q: %v", uri, err) + } + } + + return parsed.Scheme, nil +} diff --git a/pkg/util/validation/trustbundle.go b/pkg/util/validation/trustbundle.go new file mode 100644 index 0000000000..9b2e28a2a8 --- /dev/null +++ b/pkg/util/validation/trustbundle.go @@ -0,0 +1,62 @@ +package validation + +import ( + "crypto/x509" + "encoding/pem" + "fmt" + + "github.com/openshift/cluster-network-operator/pkg/names" + + corev1 "k8s.io/api/core/v1" +) + +const ( + // certPEMBlock is the type taken from the preamble of a PEM-encoded structure. + certPEMBlock = "CERTIFICATE" +) + +// TrustBundleConfigMap validates that ConfigMap contains a +// trust bundle named "ca-bundle.crt" and that "ca-bundle.crt" +// contains one or more valid PEM encoded certificates, returning +// a byte slice of "ca-bundle.crt" contents upon success. +func TrustBundleConfigMap(cfgMap *corev1.ConfigMap) ([]*x509.Certificate, []byte, error) { + if _, ok := cfgMap.Data[names.TRUST_BUNDLE_CONFIGMAP_KEY]; !ok { + return nil, nil, fmt.Errorf("ConfigMap %q is missing %q", cfgMap.Name, names.TRUST_BUNDLE_CONFIGMAP_KEY) + } + trustBundleData := []byte(cfgMap.Data[names.TRUST_BUNDLE_CONFIGMAP_KEY]) + if len(trustBundleData) == 0 { + return nil, nil, fmt.Errorf("data key %q is empty from ConfigMap %q", names.TRUST_BUNDLE_CONFIGMAP_KEY, cfgMap.Name) + } + certBundle, _, err := CertificateData(trustBundleData) + if err != nil { + return nil, nil, fmt.Errorf("failed parsing certificate data from ConfigMap %q: %v", cfgMap.Name, err) + } + + return certBundle, trustBundleData, nil +} + +// CertificateData decodes certData, ensuring each PEM block is type +// "CERTIFICATE" and the block can be parsed as an x509 certificate, +// returning slices of parsed certificates and parsed certificate data. +func CertificateData(certData []byte) ([]*x509.Certificate, []byte, error) { + var block *pem.Block + certBundle := []*x509.Certificate{} + for len(certData) != 0 { + block, certData = pem.Decode(certData) + if block == nil { + return nil, nil, fmt.Errorf("failed to parse certificate PEM") + } + if block.Type != certPEMBlock { + return nil, nil, fmt.Errorf("invalid certificate PEM, must be of type %q", certPEMBlock) + + } + + cert, err := x509.ParseCertificate(block.Bytes) + if err != nil { + return nil, nil, fmt.Errorf("failed to parse certificate: %v", err) + } + certBundle = append(certBundle, cert) + } + + return certBundle, certData, nil +} From 488b7fb08e0d90dbd12447d8c8e9b8e161104dc6 Mon Sep 17 00:00:00 2001 From: Daneyon Hansen Date: Mon, 5 Aug 2019 08:43:58 -0700 Subject: [PATCH 09/11] Adds controller-runtime event and predicate pkgs to Gopkg.lock --- Gopkg.lock | 2 ++ 1 file changed, 2 insertions(+) diff --git a/Gopkg.lock b/Gopkg.lock index 99f92f232e..0aa516d574 100644 --- a/Gopkg.lock +++ b/Gopkg.lock @@ -1031,8 +1031,10 @@ "sigs.k8s.io/controller-runtime/pkg/client/fake", "sigs.k8s.io/controller-runtime/pkg/controller", "sigs.k8s.io/controller-runtime/pkg/controller/controllerutil", + "sigs.k8s.io/controller-runtime/pkg/event", "sigs.k8s.io/controller-runtime/pkg/handler", "sigs.k8s.io/controller-runtime/pkg/manager", + "sigs.k8s.io/controller-runtime/pkg/predicate", "sigs.k8s.io/controller-runtime/pkg/reconcile", "sigs.k8s.io/controller-runtime/pkg/runtime/signals", "sigs.k8s.io/controller-runtime/pkg/source", From 6b189c84f3cdf869496fc2d7f6118065415fd35e Mon Sep 17 00:00:00 2001 From: Daneyon Hansen Date: Mon, 5 Aug 2019 10:08:31 -0700 Subject: [PATCH 10/11] Addresses bparees and dcarr feedback --- pkg/controller/proxyconfig/controller.go | 83 +++++++++++++----------- pkg/controller/proxyconfig/status.go | 13 ++-- pkg/controller/proxyconfig/validation.go | 48 ++++++-------- pkg/names/names.go | 17 +++-- 4 files changed, 79 insertions(+), 82 deletions(-) diff --git a/pkg/controller/proxyconfig/controller.go b/pkg/controller/proxyconfig/controller.go index 378ea4635d..dc9ca25079 100644 --- a/pkg/controller/proxyconfig/controller.go +++ b/pkg/controller/proxyconfig/controller.go @@ -8,7 +8,7 @@ import ( configv1 "github.com/openshift/api/config/v1" "github.com/openshift/cluster-network-operator/pkg/controller/statusmanager" "github.com/openshift/cluster-network-operator/pkg/names" - cnovalidation "github.com/openshift/cluster-network-operator/pkg/util/validation" + "github.com/openshift/cluster-network-operator/pkg/util/validation" corev1 "k8s.io/api/core/v1" apierrors "k8s.io/apimachinery/pkg/api/errors" @@ -55,30 +55,30 @@ func add(mgr manager.Manager, r reconcile.Reconciler) error { // so filter events before they are provided to the controller event handlers. pred := predicate.Funcs{ UpdateFunc: func(e event.UpdateEvent) bool { - return e.MetaOld.GetName() == names.MERGED_TRUST_BUNDLE_CONFIGMAP && + return e.MetaOld.GetName() == names.TRUST_BUNDLE_CONFIGMAP && e.MetaOld.GetNamespace() == names.TRUST_BUNDLE_CONFIGMAP_NS }, DeleteFunc: func(e event.DeleteEvent) bool { - return e.Meta.GetName() == names.MERGED_TRUST_BUNDLE_CONFIGMAP && + return e.Meta.GetName() == names.TRUST_BUNDLE_CONFIGMAP && e.Meta.GetNamespace() == names.TRUST_BUNDLE_CONFIGMAP_NS }, CreateFunc: func(e event.CreateEvent) bool { - return e.Meta.GetName() == names.MERGED_TRUST_BUNDLE_CONFIGMAP && + return e.Meta.GetName() == names.TRUST_BUNDLE_CONFIGMAP && e.Meta.GetNamespace() == names.TRUST_BUNDLE_CONFIGMAP_NS }, GenericFunc: func(e event.GenericEvent) bool { - return e.Meta.GetName() == names.MERGED_TRUST_BUNDLE_CONFIGMAP && + return e.Meta.GetName() == names.TRUST_BUNDLE_CONFIGMAP && e.Meta.GetNamespace() == names.TRUST_BUNDLE_CONFIGMAP_NS }, } - // Watch for changes to the user/system merged configmap + // Watch for changes to the trust bundle configmap. err = c.Watch(&source.Kind{Type: &corev1.ConfigMap{}}, &handler.EnqueueRequestForObject{}, pred) if err != nil { return err } - // Watch for changes to resource config.openshift.io/v1/Proxy + // Watch for changes to the proxy resource. err = c.Watch(&source.Kind{Type: &configv1.Proxy{}}, &handler.EnqueueRequestForObject{}) if err != nil { return err @@ -87,8 +87,6 @@ func add(mgr manager.Manager, r reconcile.Reconciler) error { return nil } -//var _ reconcile.Reconciler = &ReconcileProxyConfig{} - // ReconcileProxyConfig reconciles a Proxy object type ReconcileProxyConfig struct { // This client, initialized using mgr.Client() above, is a split client @@ -98,16 +96,19 @@ type ReconcileProxyConfig struct { status *statusmanager.StatusManager } -// Reconcile expects request to refer to a proxy object named "cluster" in the -// default namespace, and will ensure proxy is in the desired state. +// Reconcile expects request to refer to a proxy object named "cluster" +// in the default namespace or to a configmap object named +// "trusted-ca-bundle" in namespace "openshift-config-managed", and will +// ensure the proxy object is in the desired state. func (r *ReconcileProxyConfig) Reconcile(request reconcile.Request) (reconcile.Result, error) { - // Collect required config objects for proxy reconciliation. - proxyConfig := &configv1.Proxy{} - infraConfig := &configv1.Infrastructure{} - netConfig := &configv1.Network{} - clusterConfig := &corev1.ConfigMap{} switch { - case request.NamespacedName == names.ProxyName(): + case request.NamespacedName == names.Proxy(): + // Collect required config objects for proxy reconciliation. + proxyConfig := &configv1.Proxy{} + infraConfig := &configv1.Infrastructure{} + netConfig := &configv1.Network{} + clusterConfig := &corev1.ConfigMap{} + log.Printf("Reconciling proxy: %s\n", request.Name) err := r.client.Get(context.TODO(), request.NamespacedName, proxyConfig) if err != nil { @@ -122,31 +123,35 @@ func (r *ReconcileProxyConfig) Reconcile(request reconcile.Request) (reconcile.R } // A nil proxy is generated by upgrades and installs not requiring a proxy. + validate := true if !isSpecHTTPProxySet(&proxyConfig.Spec) && !isSpecHTTPSProxySet(&proxyConfig.Spec) { - log.Printf("httpProxy and httpsProxy not defined; reconciliation will be skipped for proxy: %s\n", + log.Printf("httpProxy and httpsProxy not defined; validation will be skipped for proxy: %s\n", request.Name) - return reconcile.Result{}, nil + validate = false } // Only proceed if the required config objects can be collected. - if err := r.client.Get(context.TODO(), types.NamespacedName{Name: "cluster"}, infraConfig); err != nil { - return reconcile.Result{}, fmt.Errorf("failed to get infrastructure config 'cluster': %v", err) - } - if err := r.client.Get(context.TODO(), types.NamespacedName{Name: "cluster"}, netConfig); err != nil { - log.Printf("failed to get network config 'cluster': %v", err) - return reconcile.Result{}, err - } - if err := r.client.Get(context.TODO(), types.NamespacedName{Name: "cluster-config-v1", Namespace: "kube-system"}, clusterConfig); err != nil { - log.Printf("failed to get configmap 'cluster': %v", err) - return reconcile.Result{}, err - } - if err := r.ValidateProxyConfig(&proxyConfig.Spec); err != nil { - log.Printf("Failed to validate Proxy.Spec: %v", err) - r.status.SetDegraded(statusmanager.ProxyConfig, "InvalidProxyConfig", - fmt.Sprintf("The proxy configuration is invalid (%v). Use 'oc edit proxy.config.openshift.io cluster' to fix.", err)) - return reconcile.Result{}, nil + if validate { + if err := r.client.Get(context.TODO(), types.NamespacedName{Name: "cluster"}, infraConfig); err != nil { + return reconcile.Result{}, fmt.Errorf("failed to get infrastructure config 'cluster': %v", err) + } + if err := r.client.Get(context.TODO(), types.NamespacedName{Name: "cluster"}, netConfig); err != nil { + log.Printf("failed to get network config 'cluster': %v", err) + return reconcile.Result{}, err + } + if err := r.client.Get(context.TODO(), types.NamespacedName{Name: "cluster-config-v1", Namespace: "kube-system"}, clusterConfig); err != nil { + log.Printf("failed to get configmap 'cluster': %v", err) + return reconcile.Result{}, err + } + if err := r.ValidateProxyConfig(&proxyConfig.Spec); err != nil { + log.Printf("Failed to validate Proxy.Spec: %v", err) + r.status.SetDegraded(statusmanager.ProxyConfig, "InvalidProxyConfig", + fmt.Sprintf("The proxy configuration is invalid (%v). Use 'oc edit proxy.config.openshift.io cluster' to fix.", err)) + return reconcile.Result{}, nil + } } - // Update Proxy.config.openshift.io.Status + + // Update proxy status. if err := r.syncProxyStatus(proxyConfig, infraConfig, netConfig, clusterConfig); err != nil { log.Printf("Could not sync proxy status: %v", err) r.status.SetDegraded(statusmanager.ProxyConfig, "StatusError", @@ -154,7 +159,7 @@ func (r *ReconcileProxyConfig) Reconcile(request reconcile.Request) (reconcile.R return reconcile.Result{}, err } log.Printf("Reconciling proxy: %s complete\n", request.Name) - case request.NamespacedName == names.MergedTrustBundleName(): + case request.NamespacedName == names.TrustBundleConfigMap(): cfgMap := &corev1.ConfigMap{} log.Printf("Reconciling configmap: %s/%s\n", request.Namespace, request.Name) err := r.client.Get(context.TODO(), request.NamespacedName, cfgMap) @@ -168,7 +173,7 @@ func (r *ReconcileProxyConfig) Reconcile(request reconcile.Request) (reconcile.R // Error reading the object - requeue the request. return reconcile.Result{}, fmt.Errorf("failed to get configmap %q: %v", request, err) } - if _, _, err := cnovalidation.TrustBundleConfigMap(cfgMap); err != nil { + if _, _, err := validation.TrustBundleConfigMap(cfgMap); err != nil { log.Printf("Failed to validate configmap: %v", err) r.status.SetDegraded(statusmanager.ProxyConfig, "InvalidTrustedCAConfigMap", fmt.Sprintf("The configmap is invalid (%v). Use 'oc edit configmap %s -n %s' to fix.", err, @@ -210,7 +215,7 @@ func isSpecTrustedCASet(proxyConfig *configv1.ProxySpec) bool { // isTrustedCAConfigMap returns true if the ConfigMap name in // spec.trustedCA is "proxy-ca-bundle". func isTrustedCAConfigMap(proxyConfig *configv1.ProxySpec) bool { - return proxyConfig.TrustedCA.Name == names.MERGED_TRUST_BUNDLE_CONFIGMAP + return proxyConfig.TrustedCA.Name == names.TRUST_BUNDLE_CONFIGMAP } // isSpecReadinessEndpoints returns true if spec.readinessEndpoints of diff --git a/pkg/controller/proxyconfig/status.go b/pkg/controller/proxyconfig/status.go index c35be8bd45..b03b9f5546 100644 --- a/pkg/controller/proxyconfig/status.go +++ b/pkg/controller/proxyconfig/status.go @@ -46,8 +46,7 @@ func (r *ReconcileProxyConfig) syncProxyStatus(proxy *configv1.Proxy, infra *con return nil } -// mergeUserSystemNoProxy returns noProxyWildcard if it supplied as a noProxy setting. -// Otherwise, mergeUserSystemNoProxy merges user-supplied noProxy settings from proxy +// mergeUserSystemNoProxy merges user-supplied noProxy settings from proxy // with cluster-wide noProxy settings, returning a merged, comma-separated // string of noProxy settings. 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 apiServerURL.Hostname(), internalAPIServer.Hostname(), ) - platform := infra.Status.PlatformStatus.Type - // TODO: Does a better way exist to get machineCIDR and control-plane replicas? + // TODO: This will be flexible when master machine management is more dynamic. type installConfig struct { ControlPlane struct { Replicas string `json:"replicas"` @@ -88,10 +86,11 @@ func mergeUserSystemNoProxy(proxy *configv1.Proxy, infra *configv1.Infrastructur return "", fmt.Errorf("invalid install-config: %v\njson:\n%s", err, data) } - if platform != configv1.VSpherePlatformType && - platform != configv1.NonePlatformType && - platform != configv1.BareMetalPlatformType { + switch infra.Status.PlatformStatus.Type { + case configv1.AWSPlatformType: set.Insert("169.254.169.254", ic.Networking.MachineCIDR) + default: + return "", fmt.Errorf("unsupported infrastructure provider: %s", infra.Status.PlatformStatus.Type) } replicas, err := strconv.Atoi(ic.ControlPlane.Replicas) diff --git a/pkg/controller/proxyconfig/validation.go b/pkg/controller/proxyconfig/validation.go index 5780b1170d..d880d1d6e0 100644 --- a/pkg/controller/proxyconfig/validation.go +++ b/pkg/controller/proxyconfig/validation.go @@ -4,7 +4,6 @@ import ( "context" "crypto/tls" "crypto/x509" - "errors" "fmt" "net" "net/http" @@ -13,7 +12,7 @@ import ( configv1 "github.com/openshift/api/config/v1" "github.com/openshift/cluster-network-operator/pkg/names" - cnovalidation "github.com/openshift/cluster-network-operator/pkg/util/validation" + "github.com/openshift/cluster-network-operator/pkg/util/validation" corev1 "k8s.io/api/core/v1" ) @@ -32,8 +31,9 @@ const ( // ValidateProxyConfig ensures that proxyConfig is valid. func (r *ReconcileProxyConfig) ValidateProxyConfig(proxyConfig *configv1.ProxySpec) error { + var err error if isSpecHTTPProxySet(proxyConfig) { - scheme, err := cnovalidation.URI(proxyConfig.HTTPProxy) + scheme, err := validation.URI(proxyConfig.HTTPProxy) if err != nil { return fmt.Errorf("invalid httpProxy URI: %v", err) } @@ -42,33 +42,18 @@ func (r *ReconcileProxyConfig) ValidateProxyConfig(proxyConfig *configv1.ProxySp } } - readinessCerts := []*x509.Certificate{} if isSpecHTTPSProxySet(proxyConfig) { - scheme, err := cnovalidation.URI(proxyConfig.HTTPSProxy) + _, err := validation.URI(proxyConfig.HTTPSProxy) if err != nil { return fmt.Errorf("invalid httpsProxy URI: %v", err) } - if scheme == proxyHTTPSScheme { - // A trusted CA bundle must be provided when using https - // between client and proxy. - if !isSpecTrustedCASet(proxyConfig) { - return errors.New("trustedCA is required when using an https scheme with httpsProxy") - } - certBundle, err := r.validateTrustedCA(proxyConfig) - if err != nil { - return fmt.Errorf("failed validating TrustedCA %q: %v", proxyConfig.TrustedCA.Name, err) - } - for _, cert := range certBundle { - readinessCerts = append(readinessCerts, cert) - } - } } if isSpecNoProxySet(proxyConfig) { if proxyConfig.NoProxy != noProxyWildcard { for _, v := range strings.Split(proxyConfig.NoProxy, ",") { v = strings.TrimSpace(v) - errDomain := cnovalidation.DomainName(v, false) + errDomain := validation.DomainName(v, false) _, _, errCIDR := net.ParseCIDR(v) if errDomain != nil && errCIDR != nil { return fmt.Errorf("invalid noProxy: %v", v) @@ -77,9 +62,17 @@ func (r *ReconcileProxyConfig) ValidateProxyConfig(proxyConfig *configv1.ProxySp } } + var certBundle []*x509.Certificate + if isSpecTrustedCASet(proxyConfig) { + certBundle, err = r.validateTrustedCA(proxyConfig) + if err != nil { + return fmt.Errorf("failed to validate TrustedCA %q: %v", proxyConfig.TrustedCA.Name, err) + } + } + if isSpecReadinessEndpoints(proxyConfig) { for _, endpoint := range proxyConfig.ReadinessEndpoints { - scheme, err := cnovalidation.URI(endpoint) + scheme, err := validation.URI(endpoint) if err != nil { return fmt.Errorf("invalid URI for endpoint %s: %v", endpoint, err) } @@ -92,7 +85,7 @@ func (r *ReconcileProxyConfig) ValidateProxyConfig(proxyConfig *configv1.ProxySp if !isSpecTrustedCASet(proxyConfig) { return fmt.Errorf("readinessEndpoint with an %q scheme requires trustedCA to be set", proxyHTTPSScheme) } - if err := validateHTTPSReadinessEndpoint(readinessCerts, endpoint); err != nil { + if err := validateHTTPSReadinessEndpoint(certBundle, endpoint); err != nil { return fmt.Errorf("readinessEndpoint probe failed for endpoint %s", endpoint) } default: @@ -156,13 +149,14 @@ func validateHTTPSReadinessEndpoint(certBundle []*x509.Certificate, endpoint str // by using certBundle as trusted CAs to create a TLS connection using a // finite loop based on retries, returning an error if it never succeeds. func validateHTTPSReadinessEndpointWithRetries(certBundle []*x509.Certificate, endpoint string, retries int) error { + var err error for i := 0; i < retries; i++ { - if err := runHTTPSReadinessProbe(certBundle, endpoint); err != nil { - return err + if err := runHTTPSReadinessProbe(certBundle, endpoint); err == nil { + return nil } } - return nil + return err } // runHTTPSReadinessProbe tries connecting to endpoint by using certBundle as @@ -201,7 +195,7 @@ func (r *ReconcileProxyConfig) validateTrustedCA(proxyConfig *configv1.ProxySpec return nil, err } - certBundle, _, err := cnovalidation.TrustBundleConfigMap(cfgMap) + certBundle, _, err := validation.TrustBundleConfigMap(cfgMap) if err != nil { return nil, err } @@ -216,7 +210,7 @@ func (r *ReconcileProxyConfig) validateTrustedCAConfigMap(proxyConfig *configv1. return nil, fmt.Errorf("invalid ConfigMap reference for TrustedCA: %s", proxyConfig.TrustedCA.Name) } cfgMap := &corev1.ConfigMap{} - if err := r.client.Get(context.TODO(), names.MergedTrustBundleName(), cfgMap); err != nil { + if err := r.client.Get(context.TODO(), names.TrustBundleConfigMap(), cfgMap); err != nil { return nil, err } diff --git a/pkg/names/names.go b/pkg/names/names.go index 8a78f71626..dd4cf1bffb 100644 --- a/pkg/names/names.go +++ b/pkg/names/names.go @@ -41,29 +41,28 @@ const MULTUS_VALIDATING_WEBHOOK = "multus.openshift.io" // the PEM encoded trust bundle. const TRUST_BUNDLE_CONFIGMAP_KEY = "ca-bundle.crt" -// MERGED_TRUST_BUNDLE_CONFIGMAP is the name of the ConfigMap +// TRUST_BUNDLE_CONFIGMAP is the name of the ConfigMap // containing the combined user/system trust bundle. -// TODO: The bundle can be used for more than proxy, change name. -const MERGED_TRUST_BUNDLE_CONFIGMAP = "proxy-ca-bundle" +const TRUST_BUNDLE_CONFIGMAP = "trusted-ca-bundle" // TRUST_BUNDLE_CONFIGMAP_NS is the namespace that hosts the -// ADDL_TRUST_BUNDLE_CONFIGMAP and MERGED_TRUST_BUNDLE_CONFIGMAP +// ADDL_TRUST_BUNDLE_CONFIGMAP and TRUST_BUNDLE_CONFIGMAP // ConfigMaps. const TRUST_BUNDLE_CONFIGMAP_NS = "openshift-config-managed" -// ProxyName returns the namespaced name of the proxy +// Proxy returns the namespaced name of the proxy // object named "cluster" in namespace "openshift-config-managed". -func ProxyName() types.NamespacedName { +func Proxy() types.NamespacedName { return types.NamespacedName{ Name: PROXY_CONFIG, } } -// MergedTrustBundleName returns the namespaced name of the ConfigMap +// TrustBundleConfigMap returns the namespaced name of the ConfigMap // containing the merged user/system trust bundle. -func MergedTrustBundleName() types.NamespacedName { +func TrustBundleConfigMap() types.NamespacedName { return types.NamespacedName{ Namespace: TRUST_BUNDLE_CONFIGMAP_NS, - Name: MERGED_TRUST_BUNDLE_CONFIGMAP, + Name: TRUST_BUNDLE_CONFIGMAP, } } From 42dbcf89559c543b656c3f3eb72e46a4e954b16d Mon Sep 17 00:00:00 2001 From: Daneyon Hansen Date: Tue, 6 Aug 2019 13:21:07 -0700 Subject: [PATCH 11/11] Refactors PR to focus on http/https/no proxy reconciliation --- Gopkg.lock | 2 - pkg/controller/proxyconfig/controller.go | 189 +++++++---------------- pkg/controller/proxyconfig/status.go | 6 +- pkg/controller/proxyconfig/validation.go | 176 +-------------------- pkg/names/names.go | 26 +--- pkg/util/validation/trustbundle.go | 62 -------- 6 files changed, 71 insertions(+), 390 deletions(-) delete mode 100644 pkg/util/validation/trustbundle.go diff --git a/Gopkg.lock b/Gopkg.lock index 0aa516d574..99f92f232e 100644 --- a/Gopkg.lock +++ b/Gopkg.lock @@ -1031,10 +1031,8 @@ "sigs.k8s.io/controller-runtime/pkg/client/fake", "sigs.k8s.io/controller-runtime/pkg/controller", "sigs.k8s.io/controller-runtime/pkg/controller/controllerutil", - "sigs.k8s.io/controller-runtime/pkg/event", "sigs.k8s.io/controller-runtime/pkg/handler", "sigs.k8s.io/controller-runtime/pkg/manager", - "sigs.k8s.io/controller-runtime/pkg/predicate", "sigs.k8s.io/controller-runtime/pkg/reconcile", "sigs.k8s.io/controller-runtime/pkg/runtime/signals", "sigs.k8s.io/controller-runtime/pkg/source", diff --git a/pkg/controller/proxyconfig/controller.go b/pkg/controller/proxyconfig/controller.go index dc9ca25079..8c7e7ec8e8 100644 --- a/pkg/controller/proxyconfig/controller.go +++ b/pkg/controller/proxyconfig/controller.go @@ -8,7 +8,6 @@ import ( configv1 "github.com/openshift/api/config/v1" "github.com/openshift/cluster-network-operator/pkg/controller/statusmanager" "github.com/openshift/cluster-network-operator/pkg/names" - "github.com/openshift/cluster-network-operator/pkg/util/validation" corev1 "k8s.io/api/core/v1" apierrors "k8s.io/apimachinery/pkg/api/errors" @@ -16,10 +15,8 @@ import ( "k8s.io/apimachinery/pkg/types" "sigs.k8s.io/controller-runtime/pkg/client" "sigs.k8s.io/controller-runtime/pkg/controller" - "sigs.k8s.io/controller-runtime/pkg/event" "sigs.k8s.io/controller-runtime/pkg/handler" "sigs.k8s.io/controller-runtime/pkg/manager" - "sigs.k8s.io/controller-runtime/pkg/predicate" "sigs.k8s.io/controller-runtime/pkg/reconcile" "sigs.k8s.io/controller-runtime/pkg/source" ) @@ -51,33 +48,6 @@ func add(mgr manager.Manager, r reconcile.Reconciler) error { return err } - // We only care about a configmap source with a specific name/namespace, - // so filter events before they are provided to the controller event handlers. - pred := predicate.Funcs{ - UpdateFunc: func(e event.UpdateEvent) bool { - return e.MetaOld.GetName() == names.TRUST_BUNDLE_CONFIGMAP && - e.MetaOld.GetNamespace() == names.TRUST_BUNDLE_CONFIGMAP_NS - }, - DeleteFunc: func(e event.DeleteEvent) bool { - return e.Meta.GetName() == names.TRUST_BUNDLE_CONFIGMAP && - e.Meta.GetNamespace() == names.TRUST_BUNDLE_CONFIGMAP_NS - }, - CreateFunc: func(e event.CreateEvent) bool { - return e.Meta.GetName() == names.TRUST_BUNDLE_CONFIGMAP && - e.Meta.GetNamespace() == names.TRUST_BUNDLE_CONFIGMAP_NS - }, - GenericFunc: func(e event.GenericEvent) bool { - return e.Meta.GetName() == names.TRUST_BUNDLE_CONFIGMAP && - e.Meta.GetNamespace() == names.TRUST_BUNDLE_CONFIGMAP_NS - }, - } - - // Watch for changes to the trust bundle configmap. - err = c.Watch(&source.Kind{Type: &corev1.ConfigMap{}}, &handler.EnqueueRequestForObject{}, pred) - if err != nil { - return err - } - // Watch for changes to the proxy resource. err = c.Watch(&source.Kind{Type: &configv1.Proxy{}}, &handler.EnqueueRequestForObject{}) if err != nil { @@ -97,94 +67,72 @@ type ReconcileProxyConfig struct { } // Reconcile expects request to refer to a proxy object named "cluster" -// in the default namespace or to a configmap object named -// "trusted-ca-bundle" in namespace "openshift-config-managed", and will -// ensure the proxy object is in the desired state. +// in the default namespace and will ensure the proxy object is in the +// desired state. func (r *ReconcileProxyConfig) Reconcile(request reconcile.Request) (reconcile.Result, error) { - switch { - case request.NamespacedName == names.Proxy(): - // Collect required config objects for proxy reconciliation. - proxyConfig := &configv1.Proxy{} - infraConfig := &configv1.Infrastructure{} - netConfig := &configv1.Network{} - clusterConfig := &corev1.ConfigMap{} - - log.Printf("Reconciling proxy: %s\n", request.Name) - err := r.client.Get(context.TODO(), request.NamespacedName, proxyConfig) - if err != nil { - if apierrors.IsNotFound(err) { - // Request object not found, could have been deleted after reconcile request. - // Return and don't requeue - log.Println("proxy not found; reconciliation will be skipped", "request", request) - return reconcile.Result{}, nil - } - // Error reading the object - requeue the request. - return reconcile.Result{}, fmt.Errorf("failed to get proxy %q: %v", request, err) + log.Printf("Reconciling proxy '%s'", request.Name) + proxyConfig := &configv1.Proxy{} + err := r.client.Get(context.TODO(), request.NamespacedName, proxyConfig) + if err != nil { + if apierrors.IsNotFound(err) { + // Request object not found, could have been deleted after reconcile request. + // Return and don't requeue + log.Printf("proxy '%s' not found; reconciliation will be skipped", request.Name) + return reconcile.Result{}, nil } + // Error reading the object - requeue the request. + return reconcile.Result{}, fmt.Errorf("failed to get proxy '%s': %v", request.Name, err) + } - // A nil proxy is generated by upgrades and installs not requiring a proxy. - validate := true - if !isSpecHTTPProxySet(&proxyConfig.Spec) && !isSpecHTTPSProxySet(&proxyConfig.Spec) { - log.Printf("httpProxy and httpsProxy not defined; validation will be skipped for proxy: %s\n", - request.Name) - validate = false - } + // A nil proxy is generated by upgrades and installs not requiring a proxy. + if !isSpecHTTPProxySet(&proxyConfig.Spec) && + !isSpecHTTPSProxySet(&proxyConfig.Spec) && + !isSpecNoProxySet(&proxyConfig.Spec) { + log.Printf("httpProxy, httpsProxy and noProxy not defined for proxy '%s'; validation will be skipped", + request.Name) + } - // Only proceed if the required config objects can be collected. - if validate { - if err := r.client.Get(context.TODO(), types.NamespacedName{Name: "cluster"}, infraConfig); err != nil { - return reconcile.Result{}, fmt.Errorf("failed to get infrastructure config 'cluster': %v", err) - } - if err := r.client.Get(context.TODO(), types.NamespacedName{Name: "cluster"}, netConfig); err != nil { - log.Printf("failed to get network config 'cluster': %v", err) - return reconcile.Result{}, err - } - if err := r.client.Get(context.TODO(), types.NamespacedName{Name: "cluster-config-v1", Namespace: "kube-system"}, clusterConfig); err != nil { - log.Printf("failed to get configmap 'cluster': %v", err) - return reconcile.Result{}, err - } - if err := r.ValidateProxyConfig(&proxyConfig.Spec); err != nil { - log.Printf("Failed to validate Proxy.Spec: %v", err) - r.status.SetDegraded(statusmanager.ProxyConfig, "InvalidProxyConfig", - fmt.Sprintf("The proxy configuration is invalid (%v). Use 'oc edit proxy.config.openshift.io cluster' to fix.", err)) - return reconcile.Result{}, nil - } - } + // Only proceed if the required config objects can be collected. + infraConfig := &configv1.Infrastructure{} + netConfig := &configv1.Network{} + clusterCfgMap := &corev1.ConfigMap{} + if err := r.client.Get(context.TODO(), types.NamespacedName{Name: names.CLUSTER_CONFIG}, infraConfig); err != nil { + log.Printf("failed to get infrastructure config '%s': %v", names.CLUSTER_CONFIG, err) + r.status.SetDegraded(statusmanager.ProxyConfig, "InfraConfigError", + fmt.Sprintf("Error getting infrastructure config %s: %v.", names.CLUSTER_CONFIG, err)) + return reconcile.Result{}, nil + } + if err := r.client.Get(context.TODO(), types.NamespacedName{Name: names.CLUSTER_CONFIG}, netConfig); err != nil { + log.Printf("failed to get network config '%s': %v", names.CLUSTER_CONFIG, err) + r.status.SetDegraded(statusmanager.ProxyConfig, "NetworkConfigError", + fmt.Sprintf("Error getting network config '%s': %v.", names.CLUSTER_CONFIG, err)) + return reconcile.Result{}, nil + } + if err := r.client.Get(context.TODO(), types.NamespacedName{Name: "cluster-config-v1", Namespace: "kube-system"}, + clusterCfgMap); err != nil { + log.Printf("failed to get configmap '%s/%s': %v", clusterCfgMap.Namespace, clusterCfgMap.Name, err) + r.status.SetDegraded(statusmanager.ProxyConfig, "ClusterConfigError", + fmt.Sprintf("Error getting cluster config configmap '%s/%s': %v.", clusterCfgMap.Namespace, + clusterCfgMap.Name, err)) + return reconcile.Result{}, nil + } + if err := r.ValidateProxyConfig(&proxyConfig.Spec); err != nil { + log.Printf("Failed to validate proxy '%s': %v", proxyConfig.Name, err) + r.status.SetDegraded(statusmanager.ProxyConfig, "InvalidProxyConfig", + fmt.Sprintf("The configuration is invalid for proxy '%s' (%v). "+ + "Use 'oc edit proxy.config.openshift.io %s' to fix.", proxyConfig.Name, proxyConfig.Name, err)) + return reconcile.Result{}, nil + } - // Update proxy status. - if err := r.syncProxyStatus(proxyConfig, infraConfig, netConfig, clusterConfig); err != nil { - log.Printf("Could not sync proxy status: %v", err) - r.status.SetDegraded(statusmanager.ProxyConfig, "StatusError", - fmt.Sprintf("Could not update proxy configuration status: %v", err)) - return reconcile.Result{}, err - } - log.Printf("Reconciling proxy: %s complete\n", request.Name) - case request.NamespacedName == names.TrustBundleConfigMap(): - cfgMap := &corev1.ConfigMap{} - log.Printf("Reconciling configmap: %s/%s\n", request.Namespace, request.Name) - err := r.client.Get(context.TODO(), request.NamespacedName, cfgMap) - if err != nil { - if apierrors.IsNotFound(err) { - // Request object not found, could have been deleted after reconcile request. - // Return and don't requeue - log.Println("configmap not found; reconciliation will be skipped", "request", request) - return reconcile.Result{}, nil - } - // Error reading the object - requeue the request. - return reconcile.Result{}, fmt.Errorf("failed to get configmap %q: %v", request, err) - } - if _, _, err := validation.TrustBundleConfigMap(cfgMap); err != nil { - log.Printf("Failed to validate configmap: %v", err) - r.status.SetDegraded(statusmanager.ProxyConfig, "InvalidTrustedCAConfigMap", - fmt.Sprintf("The configmap is invalid (%v). Use 'oc edit configmap %s -n %s' to fix.", err, - request.Name, request.Namespace)) - return reconcile.Result{}, nil - } - log.Printf("Reconciling configmap: %s/%s complete\n", request.Namespace, request.Name) - default: - // unknown object - log.Println("Ignoring unknown object, reconciliation will be skipped", "request", request) + // Update proxy status. + if err := r.syncProxyStatus(proxyConfig, infraConfig, netConfig, clusterCfgMap); err != nil { + log.Printf("Could not sync proxy '%s' status: %v", proxyConfig.Name, err) + r.status.SetDegraded(statusmanager.ProxyConfig, "StatusError", + fmt.Sprintf("Could not update proxy '%s' status: %v", proxyConfig.Name, err)) + return reconcile.Result{}, err } + log.Printf("Reconciling proxy '%s' complete", request.Name) + r.status.SetNotDegraded(statusmanager.ProxyConfig) return reconcile.Result{}, nil @@ -206,20 +154,3 @@ func isSpecHTTPSProxySet(proxyConfig *configv1.ProxySpec) bool { func isSpecNoProxySet(proxyConfig *configv1.ProxySpec) bool { return len(proxyConfig.NoProxy) > 0 } - -// isSpecTrustedCASet returns true if spec.trustedCA of proxyConfig is set. -func isSpecTrustedCASet(proxyConfig *configv1.ProxySpec) bool { - return len(proxyConfig.TrustedCA.Name) > 0 -} - -// isTrustedCAConfigMap returns true if the ConfigMap name in -// spec.trustedCA is "proxy-ca-bundle". -func isTrustedCAConfigMap(proxyConfig *configv1.ProxySpec) bool { - return proxyConfig.TrustedCA.Name == names.TRUST_BUNDLE_CONFIGMAP -} - -// isSpecReadinessEndpoints returns true if spec.readinessEndpoints of -// proxyConfig is set. -func isSpecReadinessEndpoints(proxyConfig *configv1.ProxySpec) bool { - return len(proxyConfig.ReadinessEndpoints) > 0 -} diff --git a/pkg/controller/proxyconfig/status.go b/pkg/controller/proxyconfig/status.go index b03b9f5546..8e21679489 100644 --- a/pkg/controller/proxyconfig/status.go +++ b/pkg/controller/proxyconfig/status.go @@ -16,7 +16,7 @@ import ( ) // syncProxyStatus computes the current status of proxy and -// updates status if any changes since last sync. +// updates status of any changes since last sync. func (r *ReconcileProxyConfig) syncProxyStatus(proxy *configv1.Proxy, infra *configv1.Infrastructure, network *configv1.Network, cluster *corev1.ConfigMap) error { var err error var noProxy string @@ -87,10 +87,8 @@ func mergeUserSystemNoProxy(proxy *configv1.Proxy, infra *configv1.Infrastructur } switch infra.Status.PlatformStatus.Type { - case configv1.AWSPlatformType: + case configv1.AWSPlatformType, configv1.GCPPlatformType, configv1.AzurePlatformType, configv1.OpenStackPlatformType: set.Insert("169.254.169.254", ic.Networking.MachineCIDR) - default: - return "", fmt.Errorf("unsupported infrastructure provider: %s", infra.Status.PlatformStatus.Type) } replicas, err := strconv.Atoi(ic.ControlPlane.Replicas) diff --git a/pkg/controller/proxyconfig/validation.go b/pkg/controller/proxyconfig/validation.go index d880d1d6e0..8548aa7a79 100644 --- a/pkg/controller/proxyconfig/validation.go +++ b/pkg/controller/proxyconfig/validation.go @@ -1,37 +1,24 @@ package proxyconfig import ( - "context" - "crypto/tls" - "crypto/x509" "fmt" "net" - "net/http" - "net/url" "strings" configv1 "github.com/openshift/api/config/v1" - "github.com/openshift/cluster-network-operator/pkg/names" "github.com/openshift/cluster-network-operator/pkg/util/validation" - - corev1 "k8s.io/api/core/v1" ) const ( - // proxyProbeMaxRetries is the number of times to attempt an http GET - // to a readinessEndpoints endpoint. - proxyProbeMaxRetries = 3 - proxyHTTPScheme = "http" - proxyHTTPSScheme = "https" + proxyHTTPScheme = "http" + proxyHTTPSScheme = "https" // noProxyWildcard is the string used to as a wildcard attached to a - // domain suffix in proxy.spec.noProxy to bypass proxy for httpProxy - // and httpsProxy. + // domain suffix in proxy.spec.noProxy to bypass proxying. noProxyWildcard = "*" ) // ValidateProxyConfig ensures that proxyConfig is valid. func (r *ReconcileProxyConfig) ValidateProxyConfig(proxyConfig *configv1.ProxySpec) error { - var err error if isSpecHTTPProxySet(proxyConfig) { scheme, err := validation.URI(proxyConfig.HTTPProxy) if err != nil { @@ -43,10 +30,13 @@ func (r *ReconcileProxyConfig) ValidateProxyConfig(proxyConfig *configv1.ProxySp } if isSpecHTTPSProxySet(proxyConfig) { - _, err := validation.URI(proxyConfig.HTTPSProxy) + scheme, err := validation.URI(proxyConfig.HTTPSProxy) if err != nil { return fmt.Errorf("invalid httpsProxy URI: %v", err) } + if scheme != proxyHTTPScheme && scheme != proxyHTTPSScheme { + return fmt.Errorf("httpsProxy requires a %q or %s URI scheme", proxyHTTPScheme, proxyHTTPSScheme) + } } if isSpecNoProxySet(proxyConfig) { @@ -62,157 +52,5 @@ func (r *ReconcileProxyConfig) ValidateProxyConfig(proxyConfig *configv1.ProxySp } } - var certBundle []*x509.Certificate - if isSpecTrustedCASet(proxyConfig) { - certBundle, err = r.validateTrustedCA(proxyConfig) - if err != nil { - return fmt.Errorf("failed to validate TrustedCA %q: %v", proxyConfig.TrustedCA.Name, err) - } - } - - if isSpecReadinessEndpoints(proxyConfig) { - for _, endpoint := range proxyConfig.ReadinessEndpoints { - scheme, err := validation.URI(endpoint) - if err != nil { - return fmt.Errorf("invalid URI for endpoint %s: %v", endpoint, err) - } - switch { - case scheme == proxyHTTPScheme: - if err := validateHTTPReadinessEndpoint(endpoint); err != nil { - return fmt.Errorf("readinessEndpoint probe failed for endpoint %s", endpoint) - } - case scheme == proxyHTTPSScheme: - if !isSpecTrustedCASet(proxyConfig) { - return fmt.Errorf("readinessEndpoint with an %q scheme requires trustedCA to be set", proxyHTTPSScheme) - } - if err := validateHTTPSReadinessEndpoint(certBundle, endpoint); err != nil { - return fmt.Errorf("readinessEndpoint probe failed for endpoint %s", endpoint) - } - default: - return fmt.Errorf("readiness endpoints requires a %q or %q URI sheme", proxyHTTPScheme, proxyHTTPSScheme) - } - } - } - - return nil -} - -// validateHTTPReadinessEndpoint validates an http readinessEndpoint endpoint. -func validateHTTPReadinessEndpoint(endpoint string) error { - if err := validateHTTPReadinessEndpointWithRetries(endpoint, proxyProbeMaxRetries); err != nil { - return err - } - - return nil -} - -// validateHTTPReadinessEndpointWithRetries tries to validate an http -// endpoint in a finite loop and returns the last result if it never succeeds. -func validateHTTPReadinessEndpointWithRetries(endpoint string, retries int) error { - var err error - for i := 0; i < retries; i++ { - err = runHTTPReadinessProbe(endpoint) - if err == nil { - return nil - } - } - - return err -} - -// runHTTPReadinessProbe issues an http GET request to endpoint and returns -// an error if a 2XX or 3XX http status code is not returned. -func runHTTPReadinessProbe(endpoint string) error { - resp, err := http.Get(endpoint) - if err != nil { - return err - } - defer resp.Body.Close() - - if resp.StatusCode >= http.StatusOK && resp.StatusCode < http.StatusBadRequest { - return nil - } - - return fmt.Errorf("HTTP probe failed with statuscode: %d", resp.StatusCode) -} - -// validateHTTPSReadinessEndpoint validates endpoint using certBundle as trusted CAs. -func validateHTTPSReadinessEndpoint(certBundle []*x509.Certificate, endpoint string) error { - if err := validateHTTPSReadinessEndpointWithRetries(certBundle, endpoint, proxyProbeMaxRetries); err != nil { - return err - } - return nil } - -// validateHTTPSReadinessEndpointWithRetries tries to validate endpoint -// by using certBundle as trusted CAs to create a TLS connection using a -// finite loop based on retries, returning an error if it never succeeds. -func validateHTTPSReadinessEndpointWithRetries(certBundle []*x509.Certificate, endpoint string, retries int) error { - var err error - for i := 0; i < retries; i++ { - if err := runHTTPSReadinessProbe(certBundle, endpoint); err == nil { - return nil - } - } - - return err -} - -// runHTTPSReadinessProbe tries connecting to endpoint by using certBundle as -// trusted CAs to create a TLS connection, returning an error if it never succeeds. -func runHTTPSReadinessProbe(certBundle []*x509.Certificate, endpoint string) error { - parsedURL, err := url.Parse(endpoint) - if err != nil { - return fmt.Errorf("failed parsing URL for endpoint: %s", endpoint) - } - certPool := x509.NewCertPool() - for _, cert := range certBundle { - certPool.AddCert(cert) - } - port := parsedURL.Port() - if len(port) == 0 { - parsedURL.Host += ":" + port - } - conn, err := tls.Dial("tcp", parsedURL.String(), &tls.Config{ - RootCAs: certPool, - ServerName: endpoint, - }) - if err != nil { - return fmt.Errorf("failed to connect to endpoint %q: %v", endpoint, err) - } - - return conn.Close() -} - -// validateTrustedCA validates that trustedCA of proxyConfig is a -// valid ConfigMap reference and that the configmap contains a -// valid trust bundle, returning a byte[] of the trust bundle -// data upon success. -func (r *ReconcileProxyConfig) validateTrustedCA(proxyConfig *configv1.ProxySpec) ([]*x509.Certificate, error) { - cfgMap, err := r.validateTrustedCAConfigMap(proxyConfig) - if err != nil { - return nil, err - } - - certBundle, _, err := validation.TrustBundleConfigMap(cfgMap) - if err != nil { - return nil, err - } - - return certBundle, nil -} - -// validateTrustedCAConfigMap validates that proxyConfig is a -// valid ConfigMap reference. -func (r *ReconcileProxyConfig) validateTrustedCAConfigMap(proxyConfig *configv1.ProxySpec) (*corev1.ConfigMap, error) { - if !isTrustedCAConfigMap(proxyConfig) { - return nil, fmt.Errorf("invalid ConfigMap reference for TrustedCA: %s", proxyConfig.TrustedCA.Name) - } - cfgMap := &corev1.ConfigMap{} - if err := r.client.Get(context.TODO(), names.TrustBundleConfigMap(), cfgMap); err != nil { - return nil, err - } - - return cfgMap, nil -} diff --git a/pkg/names/names.go b/pkg/names/names.go index dd4cf1bffb..e278639099 100644 --- a/pkg/names/names.go +++ b/pkg/names/names.go @@ -37,32 +37,10 @@ const SERVICE_CA_CONFIGMAP = "openshift-service-ca" // that is used in multus admission controller deployment const MULTUS_VALIDATING_WEBHOOK = "multus.openshift.io" -// TRUST_BUNDLE_CONFIGMAP_KEY is the name of the data key containing -// the PEM encoded trust bundle. -const TRUST_BUNDLE_CONFIGMAP_KEY = "ca-bundle.crt" - -// TRUST_BUNDLE_CONFIGMAP is the name of the ConfigMap -// containing the combined user/system trust bundle. -const TRUST_BUNDLE_CONFIGMAP = "trusted-ca-bundle" - -// TRUST_BUNDLE_CONFIGMAP_NS is the namespace that hosts the -// ADDL_TRUST_BUNDLE_CONFIGMAP and TRUST_BUNDLE_CONFIGMAP -// ConfigMaps. -const TRUST_BUNDLE_CONFIGMAP_NS = "openshift-config-managed" - -// Proxy returns the namespaced name of the proxy -// object named "cluster" in namespace "openshift-config-managed". +// Proxy returns the namespaced name "cluster" in the +// default namespace. func Proxy() types.NamespacedName { return types.NamespacedName{ Name: PROXY_CONFIG, } } - -// TrustBundleConfigMap returns the namespaced name of the ConfigMap -// containing the merged user/system trust bundle. -func TrustBundleConfigMap() types.NamespacedName { - return types.NamespacedName{ - Namespace: TRUST_BUNDLE_CONFIGMAP_NS, - Name: TRUST_BUNDLE_CONFIGMAP, - } -} diff --git a/pkg/util/validation/trustbundle.go b/pkg/util/validation/trustbundle.go deleted file mode 100644 index 9b2e28a2a8..0000000000 --- a/pkg/util/validation/trustbundle.go +++ /dev/null @@ -1,62 +0,0 @@ -package validation - -import ( - "crypto/x509" - "encoding/pem" - "fmt" - - "github.com/openshift/cluster-network-operator/pkg/names" - - corev1 "k8s.io/api/core/v1" -) - -const ( - // certPEMBlock is the type taken from the preamble of a PEM-encoded structure. - certPEMBlock = "CERTIFICATE" -) - -// TrustBundleConfigMap validates that ConfigMap contains a -// trust bundle named "ca-bundle.crt" and that "ca-bundle.crt" -// contains one or more valid PEM encoded certificates, returning -// a byte slice of "ca-bundle.crt" contents upon success. -func TrustBundleConfigMap(cfgMap *corev1.ConfigMap) ([]*x509.Certificate, []byte, error) { - if _, ok := cfgMap.Data[names.TRUST_BUNDLE_CONFIGMAP_KEY]; !ok { - return nil, nil, fmt.Errorf("ConfigMap %q is missing %q", cfgMap.Name, names.TRUST_BUNDLE_CONFIGMAP_KEY) - } - trustBundleData := []byte(cfgMap.Data[names.TRUST_BUNDLE_CONFIGMAP_KEY]) - if len(trustBundleData) == 0 { - return nil, nil, fmt.Errorf("data key %q is empty from ConfigMap %q", names.TRUST_BUNDLE_CONFIGMAP_KEY, cfgMap.Name) - } - certBundle, _, err := CertificateData(trustBundleData) - if err != nil { - return nil, nil, fmt.Errorf("failed parsing certificate data from ConfigMap %q: %v", cfgMap.Name, err) - } - - return certBundle, trustBundleData, nil -} - -// CertificateData decodes certData, ensuring each PEM block is type -// "CERTIFICATE" and the block can be parsed as an x509 certificate, -// returning slices of parsed certificates and parsed certificate data. -func CertificateData(certData []byte) ([]*x509.Certificate, []byte, error) { - var block *pem.Block - certBundle := []*x509.Certificate{} - for len(certData) != 0 { - block, certData = pem.Decode(certData) - if block == nil { - return nil, nil, fmt.Errorf("failed to parse certificate PEM") - } - if block.Type != certPEMBlock { - return nil, nil, fmt.Errorf("invalid certificate PEM, must be of type %q", certPEMBlock) - - } - - cert, err := x509.ParseCertificate(block.Bytes) - if err != nil { - return nil, nil, fmt.Errorf("failed to parse certificate: %v", err) - } - certBundle = append(certBundle, cert) - } - - return certBundle, certData, nil -}