Custom console route #402

jhadvig · 2020-03-26T18:21:14Z

Adding option do specify the custom hostname for console.
When when custom hostname is specified a new console-custom route is created. In case the new hostname doesnt contain the cluster domain as a suffix, secret reference needs to be specified, that points to the secret in the openshift-config namespace, where it needs to be created manually and contain tls cert and its key, which will serve as a serving-cert for the console pod. Also we will maintain the default route so it stays reserved for us.

/assign @benjaminapetersen

API changes - openshift/api#594

pkg/console/controllers/route/controller.go

benjaminapetersen · 2020-03-26T20:45:40Z

pkg/console/controllers/route/controller.go

+		return nil, true, "InvalidCustomRouteConfig", configErr
+	}
+
+	if scscErr := c.SyncCustomServigCertSecret(operatorConfig); scscErr != nil {


nit: SyncCustomServigCertSecret has Servig, but should be Serving.

benjaminapetersen · 2020-03-26T20:46:49Z

pkg/console/controllers/route/controller.go

+	c.CheckRouteHealth(operatorConfig, cr)
+
+	if _, changed := routesub.Validate(cr); changed {
+		return nil, true, "InvalidCustomRoute", customerrors.NewSyncError("custom route is invalid, correcting route state")


So this deviates a little bit from the Validate() in SyncRoute(). Looking into this a bit to make sure its ok.

thought that since the Validate() is just checking the To, Port, TLS, WildcardPolicy then it cant hurt to check those on the custom route, since these should be the same

pkg/console/controllers/route/controller.go

benjaminapetersen · 2020-03-26T20:49:08Z

pkg/console/controllers/route/controller.go

+	}
+	// if no secret with serving-cert provided, sync an empty source to delete
+	return c.resourceSyncer.SyncSecret(
+		resourcesynccontroller.ResourceLocation{Namespace: api.OpenShiftConsoleNamespace, Name: api.OpenShiftCustomLogoConfigMapName},


Prob should update api.OpenShiftCustomLogoConfigMapName to api.CustomRouteServingCertName.
I dont think we want to call this secret "custom-logo".

yep ! will update !

benjaminapetersen · 2020-03-26T20:54:29Z

pkg/console/subresource/route/route.go

+	return false
+}
+
+func ValidateCustomRoute(operatorConfig *operatorv1.Console, client routeclient.RoutesGetter) error {


I'm not sure I follow the logic entirely here.
Its not really validating the custom route its.... checking if the custom is still on the default ingress domain?

yeah, probably bad naming hare. Maybe isDefaultIngressDomain ?

benjaminapetersen · 2020-03-26T20:57:31Z

pkg/console/subresource/route/route.go

+		return err
+	}
+
+	if is := isClusterDomain(operatorConfig.Spec.Route.Hostname, defaultRoute.Spec.Host); !is {


The question here is, do we trust the default route, or should we pull in the Ingress Config?

So I think we can trust the default route, since we will be maintaining it anyways. Also when creating we are using the default as the router name. Otherwise we are raising a sync error.
Should be ok then ?

benjaminapetersen · 2020-03-26T20:58:16Z

pkg/console/subresource/route/route.go

+	return err
+}
+
+func isClusterDomain(customRouteHost, defaultRouteHost string) bool {


So this is answering "Is the custom host still on the default ingress domain"?

benjaminapetersen · 2020-03-26T20:59:42Z

pkg/console/subresource/route/route.go

 	return route, changed
 }

+func IsCustomRouteSet(operatorConfig *operatorv1.Console) bool {


I think this will work:

func IsCustomRouteSet(operatorConfig *operatorv1.Console) bool { return operatorConfig.Spec.Route.Hostname) != 0 }

definitely 👍

benjaminapetersen · 2020-03-26T21:00:11Z

pkg/console/subresource/route/route.go

+}
+
+// Check if reference for secret holding custom serving-cert is set
+func IsCustomRouteSecretSet(operatorConfig *operatorv1.Console) bool {


func IsCustomRouteSecretSet(operatorConfig *operatorv1.Console) bool { return len(operatorConfig.Spec.Route.Secret.Name) != 0 }

benjaminapetersen · 2020-03-26T21:02:38Z

pkg/console/subresource/deployment/deployment.go

 		path:        "/var/logo/",
 		isConfigMap: true}
 }
+


This is to overwrite the existing serving cert with the custom, so we don't have to change the impl in the console to look at a different dir, correct?

yes
Thought that we should not need both serving-certs

stlaz

I couldn't find an enhancement for this, but I don't understand why you should keep two routes instead of just one. Are both going to be functional at the same time?

stlaz · 2020-03-27T07:54:59Z

pkg/console/controllers/route/controller.go

+func (c *RouteSyncController) removeRoute(routeName string) error {
+	klog.V(2).Infof("deleting %s route", routeName)
+	defer klog.V(2).Infof("finished deleting %s route", routeName)
+	return c.routeClient.Routes(c.targetNamespace).Delete(routeName, &metav1.DeleteOptions{})
+}


you can keep just this function instead of having two, just

func (c *RouteSyncController) removeRoute(routeName string) error { klog.V(2).Infof("deleting %s route", routeName) err := c.routeClient.Routes(c.targetNamespace).Delete(routeName, &metav1.DeleteOptions{}) if apierrors.IsNotFound(err) { return nil } return err }

You may want to get rid of the defer log because it would appear even in the case of an error

actually, you would want to remove the other log line as well as that would appear on every sync even if the route was already removed...

stlaz · 2020-03-27T08:00:56Z

pkg/console/controllers/route/controller.go

+// 4. check custom route health
+// 5. validate the route
+func (c *RouteSyncController) SyncCustomRoute(operatorConfig *operatorsv1.Console) (consoleRoute *routev1.Route, isNew bool, reason string, err error) {
+	if configErr := routesub.ValidateCustomRoute(operatorConfig, c.routeClient); configErr != nil {


As Ben suggested, move the IsCustomRouteSet() check here and return early if false

stlaz · 2020-03-27T08:35:01Z

pkg/console/controllers/route/controller.go

+	return cr, crIsNew, "", crErr
+}
+
+func (c *RouteSyncController) SyncCustomServigCertSecret(operatorConfig *operatorsv1.Console) error {


why are you syncing this? Also, I'd advise to use the name "RouteCert" instead of serving-cert because the latter is more commonly used for the certificate the pod uses for serving, rather than the certificate the route uses.

why are you syncing this?

we need to since the secret that contains the custom serving-cert from openshift-config, where the user/admin needs to create it manually, into the openshift-console, so we can mount it into the console pod

But you don't want to mount it in the pod, you want to have that inserted into the route

stlaz · 2020-03-27T08:39:16Z

pkg/console/subresource/deployment/deployment.go

 	tolerationSeconds := int64(120)
 	volumeConfig := defaultVolumeConfig()
+	if routesub.IsCustomRouteSecretSet(operatorConfig) {
+		volumeConfig = useCustomServingCertSecret(operatorConfig.Spec.Route.Secret.Name, volumeConfig)


why are you mounting this? The contents are supposed to be used in the route's spec.tls.termination.{cert,key,caCertificate} fields

Also, you should have a hard requirement on the secret you're syncing to contain all of tls.crt, tls.key and ca.crt fields

why are you mounting this? The contents are supposed to be used in the route's spec.tls.termination.{cert,key,caCertificate} fields

I though that the secret needs to be mounted into the console pod.

why are you mounting this? The contents are supposed to be used in the route's spec.tls.termination.{cert,key,caCertificate} fields

also ca.crt?
all of them needs to be used in the route's spec.tls.termination.{cert,key,caCertificate} fields?

I think you would want to have the CA certificate there as well to prevent issues that the routing team has - openshift/cluster-ingress-operator#329. Generally, it would make your life easier.

Also, you should validate the secret and don't progress further if it's invalid.

stlaz · 2020-03-27T08:42:04Z

pkg/console/subresource/route/route.go

+			Labels:      util.SharedLabels(),
+			Annotations: map[string]string{},
+		},
+		Spec: spec(),


where's all the fancy certificate stuff you're doing this for?

jhadvig · 2020-03-27T14:34:02Z

@stlaz so to point of having two routes is to have the default one reserved, in case that somebody could create one which would block admitting the default route if recreated. The default one will be active only in case that the custom route suffix will match the cluster domain

jhadvig · 2020-03-30T15:55:37Z

@stlaz @benjaminapetersen I've update the PR based on your comments.
Removed mounting the secret as a serving-cert into the console pod and instead setting the cert and key on the custom route.

PTAL

jhadvig · 2020-03-30T17:57:35Z

The unit tests are failing cause will update them after after we align on the logic, since that will be pretty big change itself

stlaz

needs tests

pkg/console/controllers/route/controller.go

pkg/console/operator/sync_v400.go

jhadvig · 2020-04-06T18:45:40Z

@benjaminapetersen @stlaz comments addressed. PTAL

pkg/console/controllers/route/controller.go

benjaminapetersen · 2020-04-09T19:21:28Z

pkg/console/controllers/route/controller.go

+		return nil, err
+	}
+	// Check if the custom hostname has cluster domain suffix. If it does,
+	// then the secret that contains TLS certificate and key needs to be


I'm a little confused by the comment, it implies something should be done, but it immediately returns nil, nil.

+ what if the user just wanted their custom certificate for the route in cluster domain?

The comment is wrong... if the suffix matches cluster domain then the secret is not necessary

but the user may still want their certificate to be used instead of the ones that the router adds by default, correct?

pkg/console/controllers/route/controller.go

benjaminapetersen · 2020-04-09T19:31:36Z

pkg/console/starter/starter.go

 		operatorConfigClient.OperatorV1().Consoles(),
 		routesClient.RouteV1(),
 		kubeClient.CoreV1(),
+		kubeClient.CoreV1(),


I think we only need to pass kubeClient.CoreV1() once.

Ah, I see what we did, we have our clients slightly misnamed. Hmm. And we do it in all our controllers. :)

jhadvig · 2020-04-15T08:21:27Z

manifests/03-rbac-role-ns-console.yaml

  - route.openshift.io
  resources:
  - routes
+  - routes/custom-host


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

stlaz

getting there

stlaz · 2020-04-15T08:44:25Z

pkg/console/controllers/route/controller.go

-		return nil, true, "InvalidRoute", customerrors.NewSyncError("route is invalid, correcting route state")
+		return nil, nil
+	}
+	customCertSecret, err := c.secretClient.Secrets(api.OpenShiftConfigNamespace).Get(c.ctx, operatorConfig.Spec.Route.Secret.Name, metav1.GetOptions{})


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

stlaz · 2020-04-15T08:45:25Z

pkg/console/controllers/route/controller.go

 		}
-		// abort sync, route changed, let it settle & retry
-		return nil, true, "InvalidRoute", customerrors.NewSyncError("route is invalid, correcting route state")
+		return nil, nil


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?

stlaz · 2020-04-15T08:51:04Z

pkg/console/controllers/route/controller.go

+	block = &pem.Block{
+		Type:  "CERTIFICATE",
+		Bytes: cert.Raw,
+	}


why are you doing this?

stlaz · 2020-04-15T08:52:31Z

pkg/console/controllers/route/controller.go

+func certificateVerifier(customCert []byte) (*pem.Block, error) {
+	block, _ := pem.Decode([]byte(customCert))
+	if block == nil {
+		return nil, fmt.Errorf("failed to parse custom certificate")


("failed to decode certificate PEM: %v", err) please

ah, nvm, pem.Decode does not return error, my bad

stlaz · 2020-04-15T08:57:02Z

pkg/console/controllers/route/controller.go

+	switch t := key.(type) {
+	case *rsa.PrivateKey:
+		block = &pem.Block{
+			Type:  "RSA PRIVATE KEY",
+			Bytes: x509.MarshalPKCS1PrivateKey(t),
+		}
+	case *ecdsa.PrivateKey:
+		block = &pem.Block{
+			Type: "ECDSA PRIVATE KEY",
+		}
+		if block.Bytes, err = x509.MarshalECPrivateKey(t); err != nil {
+			return nil, err
+		}
+	default:
+		return nil, fmt.Errorf("block private key %T is not valid", key)
+	}


why are you doing this?

stlaz · 2020-04-15T09:03:32Z

pkg/console/subresource/route/route.go

+	existingCopy := existing.DeepCopy()
+
+	if len(GetCanonicalHost(existingCopy)) == 0 {
+		return nil, false, customerrors.NewSyncError(fmt.Sprintf("route is not available at canonical host %s", existingCopy.Status.Ingress))


this is no place for this check though, you should be able to apply the route no matter what its status is.

Also, are you 100% sure that this will be valid even for routes outside the cluster domain?

stlaz · 2020-04-15T09:06:36Z

pkg/console/controllers/route/controller_test.go

+
+const (
+	validCertificate = `
+-----BEGIN CERTIFICATE-----


thanks, I completely forgot you should also be checking at least the expiry date. For example, this certificate appears to have expired 4 years ago.

Not Before: Apr 5 15:15:55 2013 GMT Not After : Apr 4 15:15:55 2015 GM

benjaminapetersen · 2020-04-16T17:05:30Z

manifests/03-rbac-role-ns-openshift-config.yaml

      - ""
    resources:
      - configmaps
+      - secrets


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

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.

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.

benjaminapetersen

/approve

So long as @stlaz is happy, we can tag this!

jhadvig · 2020-04-16T22:06:59Z

/retest

stlaz

just a few more cosmetic changes, otherwise LGTM

stlaz · 2020-04-17T07:19:42Z

pkg/console/controllers/route/controller.go

 	v1 "github.com/openshift/client-go/operator/informers/externalversions/operator/v1"
 	routeclientv1 "github.com/openshift/client-go/route/clientset/versioned/typed/route/v1"
 	routesinformersv1 "github.com/openshift/client-go/route/informers/externalversions/route/v1"
+	customerrors "github.com/openshift/console-operator/pkg/console/errors"


nit: should have stayed below

stlaz · 2020-04-17T07:30:43Z

pkg/console/controllers/route/controller.go

+	if len(customCertSecret.Data) == 0 {
+		return nil, fmt.Errorf("custom cert secret doesn't contain 'Data' field")
+	}


you probably don't need to check this since you're checking the presence of the interesting keys later on

stlaz · 2020-04-17T07:39:12Z

pkg/console/controllers/route/controller.go

+	_, err := x509.ParsePKCS8PrivateKey(block.Bytes)
+	if err != nil {
+		_, err = x509.ParsePKCS1PrivateKey(block.Bytes)
+		if err != nil {
+			_, err = x509.ParseECPrivateKey(block.Bytes)
+			if err != nil {
+				return fmt.Errorf("block %s is not valid", block.Type)
+			}


nit:

if _, err := x509.x509.ParsePKCS8PrivateKey(block.Bytes); err != nil { if _, err = x509.ParsePKCS1PrivateKey(block.Bytes); err != nil { ... } }

stlaz · 2020-04-17T07:39:50Z

pkg/console/controllers/route/controller.go

+		if err != nil {
+			_, err = x509.ParseECPrivateKey(block.Bytes)
+			if err != nil {
+				return fmt.Errorf("block %s is not valid", block.Type)


is not a valid private key PEM

jhadvig · 2020-04-17T08:14:20Z

@stlaz comments addressed.. lets merge this baby 😄

stlaz · 2020-04-17T08:19:03Z

/lgtm
🥳

openshift-ci-robot · 2020-04-17T08:19:30Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: benjaminapetersen, jhadvig, stlaz

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Details

Needs approval from an approver in each of these files:

~~OWNERS~~ [benjaminapetersen]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

stlaz · 2020-04-17T12:11:48Z

/retest

spadgett · 2020-04-17T13:05:49Z

/retest

jhadvig · 2020-04-17T22:10:00Z

/retest

openshift-bot · 2020-04-18T18:14:22Z

/retest

Please review the full test history for this PR and help us cut down flakes.

openshift-bot · 2020-04-18T19:19:23Z

/retest

Please review the full test history for this PR and help us cut down flakes.

openshift-bot · 2020-04-18T20:24:22Z

/retest

Please review the full test history for this PR and help us cut down flakes.

openshift-bot · 2020-04-18T21:04:15Z

/retest

Please review the full test history for this PR and help us cut down flakes.

openshift-bot · 2020-04-19T00:31:28Z

/retest

Please review the full test history for this PR and help us cut down flakes.

openshift-bot · 2020-04-19T01:11:28Z

/retest

Please review the full test history for this PR and help us cut down flakes.

openshift-bot · 2020-04-19T02:54:23Z

/retest

Please review the full test history for this PR and help us cut down flakes.

openshift-ci-robot assigned benjaminapetersen Mar 26, 2020

openshift-ci-robot added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Mar 26, 2020

openshift-ci-robot requested review from benjaminapetersen and spadgett March 26, 2020 18:21

benjaminapetersen reviewed Mar 26, 2020

View reviewed changes

pkg/console/controllers/route/controller.go Show resolved Hide resolved

benjaminapetersen reviewed Mar 26, 2020

View reviewed changes

pkg/console/controllers/route/controller.go Show resolved Hide resolved

benjaminapetersen reviewed Mar 26, 2020

View reviewed changes

stlaz suggested changes Mar 27, 2020

View reviewed changes

jhadvig force-pushed the customURL branch 2 times, most recently from 893d7c8 to f9dee4a Compare March 30, 2020 15:53

stlaz suggested changes Apr 1, 2020

View reviewed changes

jhadvig force-pushed the customURL branch from f9dee4a to d97cc7e Compare April 6, 2020 17:21

jhadvig changed the title ~~[WIP] Custom console route~~ Custom console route Apr 6, 2020

openshift-ci-robot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Apr 6, 2020

benjaminapetersen reviewed Apr 9, 2020

View reviewed changes

pkg/console/controllers/route/controller.go Show resolved Hide resolved

benjaminapetersen reviewed Apr 9, 2020

View reviewed changes

pkg/console/controllers/route/controller.go Show resolved Hide resolved

benjaminapetersen reviewed Apr 9, 2020

View reviewed changes

jhadvig commented Apr 15, 2020

View reviewed changes

stlaz suggested changes Apr 15, 2020

View reviewed changes

jhadvig force-pushed the customURL branch 5 times, most recently from d45be3d to 85c0790 Compare April 16, 2020 16:54

benjaminapetersen reviewed Apr 16, 2020

View reviewed changes

benjaminapetersen approved these changes Apr 16, 2020

View reviewed changes

openshift-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Apr 16, 2020

stlaz reviewed Apr 17, 2020

View reviewed changes

Custom console route

9ed882f

jhadvig force-pushed the customURL branch from 85c0790 to 9ed882f Compare April 17, 2020 08:12

openshift-ci-robot assigned stlaz Apr 17, 2020

openshift-ci-robot added the lgtm Indicates that a PR is ready to be merged. label Apr 17, 2020

openshift-merge-robot merged commit 299b5c4 into openshift:master Apr 19, 2020

Custom console route #402

Custom console route #402

Uh oh!

Conversation

jhadvig commented Mar 26, 2020

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

benjaminapetersen Mar 26, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

stlaz left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jhadvig commented Mar 27, 2020

Uh oh!

jhadvig commented Mar 30, 2020

Uh oh!

jhadvig commented Mar 30, 2020

Uh oh!

stlaz left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

benjaminapetersen Mar 26, 2020 •

edited

Loading