Skip to content

OTA-423: Validate route host before creating app #135

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
openshift-merge-robot merged 1 commit into from
Nov 5, 2021
Merged

OTA-423: Validate route host before creating app #135

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
2 changes: 2 additions & 0 deletions api/v1/updateservice_types.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -50,6 +50,8 @@ const (

// ConditionRegistryCACertFound reports whether the updateservice registry CA cert had been found
ConditionRegistryCACertFound conditionsv1.ConditionType = "RegistryCACertFound"

ConditionReconcileError conditionsv1.ConditionType = "ReconcileError"
)

// +kubebuilder:object:root=true
Expand Down
2 changes: 1 addition & 1 deletion controllers/names.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -47,7 +47,7 @@ func nameGraphBuilderService(instance *cv1.UpdateService) string {
}

func namePolicyEngineRoute(instance *cv1.UpdateService) string {
return namePolicyEngineService(instance) + "-route"
return instance.Name + "-route"
}

func nameAdditionalTrustedCA(instance *cv1.UpdateService) string {
Expand Down
44 changes: 44 additions & 0 deletions controllers/updateservice_controller.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@ import (
"context"
"fmt"
"reflect"
"regexp"

"github.com/go-logr/logr"
conditionsv1 "github.com/openshift/custom-resource-status/conditions/v1"
Expand All @@ -28,6 +29,13 @@ import (
"github.com/openshift/library-go/pkg/route/routeapihelpers"
)

const dns1123LabelFmt string = "^[a-z]([-a-z0-9]*[a-z0-9])?$"

// DNS1123LabelMaxLength is a label's max length in DNS (RFC 1123)
const DNS1123LabelMaxLength int = 63

var dns1123LabelRegexp = regexp.MustCompile(dns1123LabelFmt)

var log = logf.Log.WithName("controller_updateservice")

// blank assignment to verify that ReconcileUpdateService implements reconcile.Reconciler
Expand Down Expand Up @@ -78,6 +86,20 @@ func (r *UpdateServiceReconciler) Reconcile(ctx context.Context, req ctrl.Reques
instanceCopy := instance.DeepCopy()
instanceCopy.Status = cv1.UpdateServiceStatus{}

if err := validateRouteName(instanceCopy, req.Name, req.Namespace); err != nil {
conditionsv1.SetStatusCondition(&instanceCopy.Status.Conditions, conditionsv1.Condition{
Type: cv1.ConditionReconcileError,
Status: corev1.ConditionTrue,
Reason: "Unable to create UpdateService route",
Comment thread
jottofar marked this conversation as resolved.
Message: err.Error(),
})
if err := r.Client.Status().Update(ctx, instanceCopy); err != nil {
reqLogger.Error(err, "Failed to update Status")
}
reqLogger.Error(err, "Unable to create UpdateService route")
return ctrl.Result{}, nil
}

// 1. Gather conditions
// Look at the existing cluster resources and communicate to kubeResources
// how it should create resources.
Expand Down Expand Up @@ -464,6 +486,28 @@ func (r *UpdateServiceReconciler) ensurePolicyEngineService(ctx context.Context,
return nil
}

func validateRouteName(instance *cv1.UpdateService, name string, namespace string) error {
var errReasons []string
routeName := namePolicyEngineRoute(instance) + "-" + namespace
Copy link
Copy Markdown
Member

@wking wking Oct 26, 2021

Choose a reason for hiding this comment

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

Some kind of // hack: copying the logic from https://github.com/openshift/openshift-apiserver/blame/d7b5d2432130ae600c4ab6e047d8a7ce29698a56/pkg/route/apiserver/simplerouteallocation/plugin.go#L50-L54 or some such, so if/when this goes stale, folks know where to look to see how it should be updated.

It's going to be a pain to match our operator-side validation with a potentially evolving default route plugin. If we are going to grow opinions like this, perhaps we should set spec.subdomain to decouple ourselves somewhat from the underlying route controller's logic?

Copy link
Copy Markdown
Contributor Author

@jottofar jottofar Oct 27, 2021

Choose a reason for hiding this comment

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

I don't feel strongly that we need to have this precheck so if you and/or Lala are okay with dropping it so am I. With the shortened route name change in here I wouldn't think we'll encounter the >63 issue very often if at all.

So it's the cost of maintenance vs. catching the error before the operand is created and thereby eliminating the need for the user to have to delete the operand after route creation failure.

/cc @LalatenduMohanty

Copy link
Copy Markdown
Member

@wking wking Oct 28, 2021

Choose a reason for hiding this comment

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

So it's the cost of maintenance vs. catching the error before the operand is created and thereby eliminating the need for the user to have to delete the operand after route creation failure.

Hmm... Do they even need to delete the operand? I'd expect them to have to delete the UpdateService and create a replacement with a shorter name (unless you're allowed to patch resource names?). And when the old UpdateService goes away, I'd expect our operator to notice and remove the operand and other associated resources we'd been using to fulfill the outgoing UpdateService's request.

Copy link
Copy Markdown
Contributor Author

@jottofar jottofar Nov 2, 2021

Choose a reason for hiding this comment

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

If this check fails nothing is created so if they could try again with a shorter operand name.

Copy link
Copy Markdown
Member

@LalatenduMohanty LalatenduMohanty Nov 4, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

@jottofar AFAIU this check will consider the name given to the route and then validate if the name falls within the 63 character length. To do that it needs to take the namespace length in to account because that's how the route name length is calculated. Is that right?

Copy link
Copy Markdown
Contributor Author

@jottofar jottofar Nov 4, 2021

Choose a reason for hiding this comment

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

Correct. Namespace is part of the route name and is out of our control and something that could change. For example, if we created the operand named test in namespace openshift-update-service the route name, before this change, would be test-policy-engine-route-openshift-update-service and after change would be test-route-openshift-update-service.

Copy link
Copy Markdown
Member

@wking wking Nov 4, 2021

Choose a reason for hiding this comment

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

Confirming via a 4.9.0 OpenShift Update Service operator in a 4.10.0-0.ci-2021-11-04-133306 cluster, nobody can change the name on the UpdateService (or other resources?):

image

So the repair for "the name you picked is too long" will be "notice, delete the UpdateService, and create a replacement with a shorter name". I don't think it's worth jumping through hoops to guess at Route validity, because the upside would be shaving a minute or so off that notice cycle. And the downside is that we need this inlined copy making assumptions about how the cluster will be generating domain names for Routes.

Copy link
Copy Markdown
Contributor Author

@jottofar jottofar Nov 4, 2021

Choose a reason for hiding this comment

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

I'm fine with dropping the validation piece. It's not how I originally envisioned it anyway. I'll just change this to only include the name shortening change which should reduce/eliminate issue occurrence.


if len(routeName) > DNS1123LabelMaxLength {
errReasons = append(errReasons,
fmt.Sprintf("cannot exceed RFC 1123 maximum length of %d. Shorten the application name and/or namespace.",
DNS1123LabelMaxLength))
}
if !dns1123LabelRegexp.MatchString(routeName) {
errReasons = append(errReasons,
fmt.Sprintf("has invalid format; must comply with %q.", dns1123LabelFmt))
}
numErrors := len(errReasons)
if numErrors == 0 {
return nil
} else if numErrors == 1 {
return fmt.Errorf(fmt.Sprintf("UpdateService route name %q %s", routeName, errReasons[0]))
}
return fmt.Errorf(fmt.Sprintf("UpdateService route name %q %s Route name %s", routeName, errReasons[0], errReasons[1]))
}

func (r *UpdateServiceReconciler) ensurePolicyEngineRoute(ctx context.Context, reqLogger logr.Logger, instance *cv1.UpdateService, resources *kubeResources) error {
route := resources.policyEngineRoute
// Set UpdateService instance as the owner and controller
Expand Down
56 changes: 53 additions & 3 deletions controllers/updateservice_controller_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@ package controllers

import (
"context"
"errors"
"fmt"
"os"
"testing"
Expand All @@ -16,7 +17,7 @@ import (
appsv1 "k8s.io/api/apps/v1"
corev1 "k8s.io/api/core/v1"
policyv1beta1 "k8s.io/api/policy/v1beta1"
"k8s.io/apimachinery/pkg/api/errors"
apierrors "k8s.io/apimachinery/pkg/api/errors"
metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
"k8s.io/apimachinery/pkg/runtime"
"k8s.io/apimachinery/pkg/types"
Expand Down Expand Up @@ -190,7 +191,7 @@ func TestEnsurePullSecret(t *testing.T) {

resources, err := newKubeResources(updateservice, testOperandImage, ps, cm)

if !errors.IsNotFound(err) {
if !apierrors.IsNotFound(err) {
err = r.ensurePullSecret(context.TODO(), log, updateservice, resources)
}
found := &corev1.Secret{}
Expand Down Expand Up @@ -480,7 +481,7 @@ func TestEnsurePodDisruptionBudget(t *testing.T) {
updateservice := &cv1.UpdateService{}
err := r.Client.Get(context.TODO(), types.NamespacedName{Name: testName, Namespace: testNamespace}, updateservice)
if err != nil {
if errors.IsNotFound(err) {
if apierrors.IsNotFound(err) {
assert.Error(t, err)
}
assert.Error(t, err)
Expand Down Expand Up @@ -517,6 +518,55 @@ func TestEnsurePodDisruptionBudget(t *testing.T) {
}
}

func TestValidateRouteName(t *testing.T) {
tests := []struct {
name string
appName string
namespace string
err error
}{
{
name: "RouteNameValid",
appName: "foo",
namespace: "openshift-update-service",
err: nil,
},
{
name: "RouteNameMaxLen",
appName: "foo",
namespace: "openshift-update-service-0123456789012345678901234567",
err: nil,
},
{
name: "RouteNameTooLong",
appName: "foo",
namespace: "openshift-update-service-01234567890123456789012345678",
err: errors.New("UpdateService route name \"foo-route-openshift-update-service-01234567890123456789012345678\" cannot exceed RFC 1123 maximum length of 63. Shorten the application name and/or namespace."),
},
{
name: "RouteNameInvalidFormat",
appName: "foo",
namespace: "openshift-update-service-012345678901234567890123456.",
err: errors.New(fmt.Sprintf("UpdateService route name \"foo-route-openshift-update-service-012345678901234567890123456.\" has invalid format; must comply with %q.", dns1123LabelFmt)),
},
{
name: "RouteNameMultipleErrors",
appName: "foo",
namespace: "openshift-update-service-0123456789012345678901234567.",
err: errors.New("UpdateService route name \"foo-route-openshift-update-service-0123456789012345678901234567.\" cannot exceed RFC 1123 maximum length of 63. Shorten the application name and/or namespace. " +
fmt.Sprintf("Route name has invalid format; must comply with %q.", dns1123LabelFmt)),
},
}

for _, test := range tests {
t.Run(test.name, func(t *testing.T) {
updateservice := newDefaultUpdateService()
err := validateRouteName(updateservice, test.appName, test.namespace)
assert.Equal(t, test.err, err)
})
}
}

func TestEnsurePolicyEngineRoute(t *testing.T) {
tests := []struct {
name string
Expand Down