Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions manifests/03-rbac-role-cluster.yaml
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,7 @@ rules:
- config.openshift.io
resources:
- infrastructures
- ingresses
verbs:
- get
- list
Expand Down
1 change: 1 addition & 0 deletions manifests/03-rbac-role-ns-console.yaml
Original file line number Diff line number Diff line change
Expand Up @@ -63,6 +63,7 @@ rules:
- route.openshift.io
resources:
- routes
- routes/custom-host
Copy link
Member Author

Choose a reason for hiding this comment

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

This is needed to set the route.spec.tls fields

verbs:
- get
- list
Expand Down
1 change: 1 addition & 0 deletions manifests/03-rbac-role-ns-openshift-config.yaml
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,7 @@ rules:
- ""
resources:
- configmaps
- secrets
Copy link
Contributor

Choose a reason for hiding this comment

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

Too bad we have to add this, I liked that we didn't read secrets from here.

Copy link
Contributor

Choose a reason for hiding this comment

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

Not that we ARE reading them all, just that now we can. but we are only doing a GET on the one that matters to us.

Copy link
Member Author

Choose a reason for hiding this comment

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

Other solution would be to have the secret name as API, then we could specify only that single one... but not sure if thats worth it.

verbs:
- get
- list
Expand Down
1 change: 1 addition & 0 deletions pkg/api/api.go
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,7 @@ const (
OpenShiftConsoleOperator = "console-operator"
OpenShiftConsoleConfigMapName = "console-config"
OpenShiftConsolePublicConfigMapName = "console-public"
OpenshiftConsoleCustomRouteName = "console-custom"
ServiceCAConfigMapName = "service-ca"
DefaultIngressCertConfigMapName = "default-ingress-cert"
OpenShiftConsoleDeploymentName = OpenShiftConsoleName
Expand Down
237 changes: 188 additions & 49 deletions pkg/console/controllers/route/controller.go
Original file line number Diff line number Diff line change
Expand Up @@ -4,12 +4,16 @@ import (
"context"
"crypto/tls"
"crypto/x509"
"encoding/pem"
"errors"
"fmt"
"net/http"
"strings"
"time"

// k8s
corev1 "k8s.io/api/core/v1"
apierrors "k8s.io/apimachinery/pkg/api/errors"
metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
"k8s.io/apimachinery/pkg/util/runtime"
"k8s.io/apimachinery/pkg/util/wait"
Expand All @@ -21,6 +25,7 @@ import (
// openshift
operatorsv1 "github.com/openshift/api/operator/v1"
routev1 "github.com/openshift/api/route/v1"
configclientv1 "github.com/openshift/client-go/config/clientset/versioned/typed/config/v1"
operatorclientv1 "github.com/openshift/client-go/operator/clientset/versioned/typed/operator/v1"
v1 "github.com/openshift/client-go/operator/informers/externalversions/operator/v1"
routeclientv1 "github.com/openshift/client-go/route/clientset/versioned/typed/route/v1"
Expand All @@ -31,7 +36,6 @@ import (
"github.com/openshift/console-operator/pkg/api"
customerrors "github.com/openshift/console-operator/pkg/console/errors"
"github.com/openshift/console-operator/pkg/console/status"
"github.com/openshift/console-operator/pkg/console/subresource/route"
routesub "github.com/openshift/console-operator/pkg/console/subresource/route"
)

Expand All @@ -43,8 +47,10 @@ const (
type RouteSyncController struct {
// clients
operatorConfigClient operatorclientv1.ConsoleInterface
ingressClient configclientv1.IngressInterface
routeClient routeclientv1.RoutesGetter
configMapClient coreclientv1.ConfigMapsGetter
secretClient coreclientv1.SecretsGetter
// names
targetNamespace string
routeName string
Expand All @@ -57,10 +63,13 @@ type RouteSyncController struct {
}

func NewRouteSyncController(
// top level config
configClient configclientv1.ConfigV1Interface,
// clients
operatorConfigClient operatorclientv1.ConsoleInterface,
routev1Client routeclientv1.RoutesGetter,
configMapClient coreclientv1.ConfigMapsGetter,
secretClient coreclientv1.SecretsGetter,
Copy link
Contributor

Choose a reason for hiding this comment

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

Nit: We probably could fix this so that NewRouteSyncController() accepts the KubeV1Client once, but then separates them out when we create the controller:

ctrl := &RouteSyncController{
   configMapClient:   KubeV1Client.ConfigMaps()
   secretsClient:  KubeV1Client.Secrets()

But I see our other controllers have the same issue. It would be a tad cleaner if:

// We are a little incorrect as the secrets client is 1 step into the KubeV1Client as .Secrets
c.secretClient.Secrets(api.OpenShiftConfigNamespace).Get(c.ctx, operatorConfig.Spec.Route.Secret.Name, metav1.GetOptions{})
// This would be ideal
c.secretClient(api.OpenShiftConfigNamespace).Get(c.ctx, operatorConfig.Spec.Route.Secret.Name, metav1.GetOptions{})

Copy link
Contributor

Choose a reason for hiding this comment

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

So we can chip away at it, or just fix them all as a separate PR.

Copy link
Member Author

Choose a reason for hiding this comment

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

would rather deal with it in one PR, if thats ok :)

// informers
operatorConfigInformer v1.ConsoleInformer,
routeInformer routesinformersv1.RouteInformer,
Expand All @@ -74,8 +83,10 @@ func NewRouteSyncController(
) *RouteSyncController {
ctrl := &RouteSyncController{
operatorConfigClient: operatorConfigClient,
ingressClient: configClient.Ingresses(),
routeClient: routev1Client,
configMapClient: configMapClient,
secretClient: secretClient,
targetNamespace: targetNamespace,
routeName: routeName,
// events
Expand Down Expand Up @@ -110,59 +121,184 @@ func (c *RouteSyncController) sync() error {
return nil
case operatorsv1.Removed:
klog.V(4).Infoln("console is in a removed state: deleting route")
return c.removeRoute()
if err = c.removeRoute(api.OpenshiftConsoleCustomRouteName); err != nil {
return err
}
return c.removeRoute(api.OpenShiftConsoleName)
default:
return fmt.Errorf("unknown state: %v", operatorConfig.Spec.ManagementState)
}

updatedOperatorConfig := operatorConfig.DeepCopy()
_, _, errReason, err := c.SyncRoute(updatedOperatorConfig)

status.HandleProgressingOrDegraded(updatedOperatorConfig, "RouteSync", errReason, err)
status.SyncStatus(c.ctx, c.operatorConfigClient, updatedOperatorConfig)
defaultRouteErrReason, defaultRouteErr := c.SyncDefaultRoute(updatedOperatorConfig)
status.HandleProgressingOrDegraded(updatedOperatorConfig, "DefaultRouteSync", defaultRouteErrReason, defaultRouteErr)
if defaultRouteErr != nil {
return defaultRouteErr
}

customRouteErrReason, customRouteErr := c.SyncCustomRoute(updatedOperatorConfig)
status.HandleProgressingOrDegraded(updatedOperatorConfig, "CustomRouteSync", customRouteErrReason, customRouteErr)

return err
}

func (c *RouteSyncController) removeRoute(routeName string) error {
err := c.routeClient.Routes(c.targetNamespace).Delete(c.ctx, routeName, metav1.DeleteOptions{})
if apierrors.IsNotFound(err) {
return nil
}
return err
}

func (c *RouteSyncController) removeRoute() error {
klog.V(2).Info("deleting console route")
defer klog.V(2).Info("finished deleting console route")
return c.routeClient.Routes(c.targetNamespace).Delete(c.ctx, route.Stub().Name, metav1.DeleteOptions{})
func (c *RouteSyncController) SyncDefaultRoute(operatorConfig *operatorsv1.Console) (string, error) {
requiredDefaultRoute := routesub.DefaultRoute(operatorConfig)

defaultRoute, _, defaultRouteError := routesub.ApplyRoute(c.routeClient, c.recorder, requiredDefaultRoute)
if defaultRouteError != nil {
return "FailedDefaultRouteApply", defaultRouteError
}

if len(routesub.GetCanonicalHost(defaultRoute)) == 0 {
return "FailedDefaultRouteHost", customerrors.NewSyncError(fmt.Sprintf("default route is not available at canonical host %s", defaultRoute.Status.Ingress))
}

if !routesub.IsCustomRouteSet(operatorConfig) {
c.CheckRouteHealth(operatorConfig, defaultRoute)
}

return "", defaultRouteError
}

// Custom route sync needs to:
// 1. validate if the reference for secret with TLS certificate and key is defined in operator config(in case a non-openshift cluster domain is used)
// 2. if secret is defined, verify the TLS certificate and key
// 4. create the custom console route, if custom TLS certificate and key are defined use them
// 5. apply the custom route
func (c *RouteSyncController) SyncCustomRoute(operatorConfig *operatorsv1.Console) (string, error) {
if !routesub.IsCustomRouteSet(operatorConfig) {
return "", nil
}

customSecret, configErr := c.ValidateCustomRouteConfig(operatorConfig)
if configErr != nil {
return "InvalidCustomRouteConfig", configErr
}

customTLSCert, secretValidationErr := ValidateCustomCertSecret(customSecret)
if secretValidationErr != nil {
return "InvalidCustomTLSSecret", secretValidationErr
}

requiredCustomRoute := routesub.CustomRoute(operatorConfig, customTLSCert)
customRoute, _, customRouteError := routesub.ApplyRoute(c.routeClient, c.recorder, requiredCustomRoute)
if customRouteError != nil {
return "FailedCustomRouteApply", customRouteError
}

if len(routesub.GetCanonicalHost(customRoute)) == 0 {
return "FailedCustomRouteHost", customerrors.NewSyncError(fmt.Sprintf("custom route is not available at canonical host %s", customRoute.Status.Ingress))
}

if routesub.IsCustomRouteSet(operatorConfig) {
c.CheckRouteHealth(operatorConfig, customRoute)
}

return "", customRouteError
}

// apply route
// - be sure to test that we don't trigger an infinite loop by stomping on the
// default host name set by the server, or any other values. The ApplyRoute()
// logic will have to be sound.
// - update to ApplyRoute() once the logic is settled
func (c *RouteSyncController) SyncRoute(operatorConfig *operatorsv1.Console) (consoleRoute *routev1.Route, isNew bool, reason string, err error) {
// ensure we have a route. any error returned is a non-404 error

rt, rtIsNew, rtErr := routesub.GetOrCreate(c.ctx, c.routeClient, routesub.DefaultRoute(operatorConfig))
if rtErr != nil {
return nil, false, "FailedCreate", rtErr
}
// Check if the console is reachable
c.CheckRouteHealth(operatorConfig, rt)

// we will not proceed until the route is valid. this eliminates complexity with the
// configmap, secret & oauth client as they can be certain they have a host if we pass this point.
host := routesub.GetCanonicalHost(rt)
if len(host) == 0 {
return nil, false, "FailedHost", customerrors.NewSyncError(fmt.Sprintf("route is not available at canonical host %s", rt.Status.Ingress))
}

if validatedRoute, changed := routesub.Validate(rt); changed {
// if validation changed the route, issue an update
if _, err := c.routeClient.Routes(api.TargetNamespace).Update(c.ctx, validatedRoute, metav1.UpdateOptions{}); err != nil {
// error is unexpected, this is a real error
return nil, false, "InvalidRouteCorrection", err
func (c *RouteSyncController) ValidateCustomRouteConfig(operatorConfig *operatorsv1.Console) (*corev1.Secret, error) {
// get ingress
ingress, err := c.ingressClient.Get(c.ctx, api.ConfigResourceName, metav1.GetOptions{})
if err != nil {
return nil, err
}
// Check if the custom hostname has cluster domain suffix, which indicates
// if a secret that contains TLS certificate and key needs to exist in the
// `openshift-config` namespace and referenced in the operator config.
// If the suffix matches the cluster domain, then the secret is optional.
// If the suffix doesn't matches the cluster domain, then the secret is mandatory.
if !routesub.IsCustomRouteSecretSet(operatorConfig) {
if !strings.HasSuffix(operatorConfig.Spec.Route.Hostname, ingress.Spec.Domain) {
return nil, fmt.Errorf("secret reference for custom route TLS secret is not defined")
}
// abort sync, route changed, let it settle & retry
return nil, true, "InvalidRoute", customerrors.NewSyncError("route is invalid, correcting route state")
return nil, nil
Copy link
Contributor

Choose a reason for hiding this comment

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

like I said - I configured my custom route within the cluster domain and configured a custom cert I want it to use, why don't you honor it?

}

return c.secretClient.Secrets(api.OpenShiftConfigNamespace).Get(c.ctx, operatorConfig.Spec.Route.Secret.Name, metav1.GetOptions{})
}

// Validate secret that holds custom TLS certificate and key.
// Secret has to contain `tls.crt` and `tls.key` data keys
// where the certificate and key are stored and both need
// to be in valid format.
// Return the custom TLS certificate and key
func ValidateCustomCertSecret(customCertSecret *corev1.Secret) (*routesub.CustomTLSCert, error) {
if customCertSecret == nil {
return nil, nil
}
if customCertSecret.Type != corev1.SecretTypeTLS {
return nil, fmt.Errorf("custom cert secret is not in %q type, instead uses %q type", corev1.SecretTypeTLS, customCertSecret.Type)
}

customTLS := &routesub.CustomTLSCert{}
cert, certExist := customCertSecret.Data["tls.crt"]
if !certExist {
return nil, fmt.Errorf("custom cert secret data doesn't contain 'tls.crt' entry")
}
// only return the route if it is valid with a host
return rt, rtIsNew, "", rtErr

certificateVerifyErr := certificateVerifier(cert)
if certificateVerifyErr != nil {
return nil, fmt.Errorf("failed to verify custom certificate PEM: " + certificateVerifyErr.Error())
}
customTLS.Certificate = string(cert)

key, keyExist := customCertSecret.Data["tls.key"]
if !keyExist {
return nil, fmt.Errorf("custom cert secret data doesn't contain 'tls.key' entry")
}

privateKeyVerifyErr := privateKeyVerifier(key)
if privateKeyVerifyErr != nil {
return nil, fmt.Errorf("failed to verify custom key PEM: " + privateKeyVerifyErr.Error())
}
customTLS.Key = string(key)

return customTLS, nil
}

func certificateVerifier(customCert []byte) error {
block, _ := pem.Decode([]byte(customCert))
if block == nil {
return fmt.Errorf("failed to decode certificate PEM")
}
certificate, err := x509.ParseCertificate(block.Bytes)
if err != nil {
return err
}
now := time.Now()
if now.After(certificate.NotAfter) {
return fmt.Errorf("custom TLS certificate is expired")
}
if now.Before(certificate.NotBefore) {
return fmt.Errorf("custom TLS certificate is not valid yet")
}
return nil
}

func privateKeyVerifier(customKey []byte) error {
block, _ := pem.Decode([]byte(customKey))
if block == nil {
return fmt.Errorf("failed to decode key PEM")
}
if _, err := x509.ParsePKCS8PrivateKey(block.Bytes); err != nil {
if _, err = x509.ParsePKCS1PrivateKey(block.Bytes); err != nil {
if _, err = x509.ParseECPrivateKey(block.Bytes); err != nil {
return fmt.Errorf("block %s is not valid key PEM", block.Type)
}
}
}
return nil
}

func (c *RouteSyncController) Run(workers int, stopCh <-chan struct{}) {
Expand Down Expand Up @@ -209,36 +345,39 @@ func (c *RouteSyncController) newEventHandler() cache.ResourceEventHandler {
}
}

func (c *RouteSyncController) CheckRouteHealth(opConfig *operatorsv1.Console, rt *routev1.Route) {
func (c *RouteSyncController) CheckRouteHealth(operatorConfig *operatorsv1.Console, route *routev1.Route) {
status.HandleDegraded(func() (conf *operatorsv1.Console, prefix string, reason string, err error) {
prefix = "RouteHealth"

caPool, err := c.getCA()
if err != nil {
return opConfig, prefix, "FailedLoadCA", fmt.Errorf("failed to read CA to check route health: %v", err)
return operatorConfig, prefix, "FailedLoadCA", fmt.Errorf("failed to read CA to check route health: %v", err)
}
client := clientWithCA(caPool)

url := "https://" + rt.Spec.Host + "/health"
if len(route.Spec.Host) == 0 {
return operatorConfig, prefix, "RouteHostError", fmt.Errorf("route does not have host specified")
}
url := "https://" + route.Spec.Host + "/health"
req, err := http.NewRequest(http.MethodGet, url, nil)
if err != nil {
return opConfig, prefix, "FailedRequest", fmt.Errorf("failed to build request to route (%s): %v", url, err)
return operatorConfig, prefix, "FailedRequest", fmt.Errorf("failed to build request to route (%s): %v", url, err)
}
resp, err := client.Do(req)
if err != nil {
return opConfig, prefix, "FailedGet", fmt.Errorf("failed to GET route (%s): %v", url, err)
return operatorConfig, prefix, "FailedGet", fmt.Errorf("failed to GET route (%s): %v", url, err)
}
defer resp.Body.Close()

if resp.StatusCode != http.StatusOK {
return opConfig, prefix, "StatusError", fmt.Errorf("route not yet available, %s returns '%s'", url, resp.Status)
return operatorConfig, prefix, "StatusError", fmt.Errorf("route not yet available, %s returns '%s'", url, resp.Status)

}
return opConfig, prefix, "", nil
return operatorConfig, prefix, "", nil
}())

status.HandleAvailable(opConfig, "Route", "FailedAdmittedIngress", func() error {
if !routesub.IsAdmitted(rt) {
status.HandleAvailable(operatorConfig, "Route", "FailedAdmittedIngress", func() error {
if !routesub.IsAdmitted(route) {
return errors.New("console route is not admitted")
}
return nil
Expand Down
Loading