Enforce name length and content validation for rukpak generated resources

[exdx]

Rukpak generates kubernetes resources such as pods and configmaps based on the name of the underlying Bundle provided by the user. These resources' names are by default restricted by kubernetes (based on DNS naming conventions) so there should be a way to add these checks before creating resources on-cluster. The checks can be as simple as ensuring that any metadata.name is less than 63 characters and does not contain any invalid characters. See #114 for additional discussion.

[joelanford]

+1

An interesting concern of this problem is that different provisioners will likely use different prefixes for their generated resources. However the validation is on the API, not the provisioner implementation. I'd suggest that we basically reserve a portion of the 63-character metadata.name limit for use by provisioner implementations.

For example: we would add validation on a Bundle's metadata.name to restrict the length to 43 characters, thus reserving 20 characters for provisioner-specific prefixes.

We would ideally have documentation for provisioner developers that talks about this and a helper function that provisioner developers can use to ensure they don't exceed the reservation.

[timflannagan]

For example: we would add validation on a Bundle's metadata.name to restrict the length to 43 characters, thus reserving 20 characters for provisioner-specific prefixes.

That's traditionally how I'd stepped around this issue by validating the metadata.Name is below a certain threshold to allow a buffer for the generated name, e.g.

const (
    namespacePrefixCharLimit = 10
)
if len(namespacePrefix) > namespacePrefixCharLimit {
    logger.Fatalf("Error: the --namespace-prefix exceeds the limit of %d characters", namespacePrefixCharLimit)
}

I think this is reserving a buffer and validating it fits within that buffer is a reasonable workaround, but it's unclear whether there's any obvious usability issues around that implementation.

[joelanford]

When would that code execute? I was thinking more like:

github.com/operator-framework/rukpak/pkg/provisioner/util

func MustBuildName(prefix string, bundle *rukpakv1alpha1.Bundle) string {
	if len(prefix) > 20 {
		panic("prefix exceeds reserved length of 20")
	}
	return fmt.Sprintf("%s%s", prefix, bundle.Name)
}

We document that in provisioner docs as the library to use when building names of sub-objects based on the bundle's name.

That would still require schema validation or an admission webhook on bundle names to make sure they don't exceed their reservation of 43 characters.

It's a pretty terrible UX for a cluster admin to be allowed to create the bundle and then have the controller asynchronously say "that's not gonna work, sorry!" if its something we could have told them at admission time.

[tylerslaton]

Just to add another idea to the mix - would it be worth finding if we can just define this in the CRD validation? I was looking at this part of the Kubebuilder book and looks like what we want.

The main issue that I'm penciling out is if we can apply these validations to metadata fields.

[exdx]

Just to add another idea to the mix - would it be worth finding if we can just define this in the CRD validation? I was looking at this part of the Kubebuilder book and looks like what we want.

The main issue that I'm penciling out is if we can apply these validations to metadata fields.

That's a great thing to point out @tylerslaton -- I think we can rely on these kubebuilder markers to get us some validation at admission time without building out an entire webhook.

[timflannagan]

Note: this can likely be a 0.3 candidate when grooming the 0.2 milestone.

[akihikokuroda]

I looked at the kubebuilder book. There is no way to specify the max length of the CRD resource name. The generated yaml file doesn't have the properties in the OpenAPIV3Scheme, either. It is just object

          properties:
            apiVersion:
              type: string
            kind:
              type: string
            metadata:
              type: object

The custom admission webhook seems necessary.

[exdx]

That's unfortunate that we cannot validate at admission type by using the OpenAPI schema validation for the Bundle type. I think we could add our validation as a library and decide whether to package it as a webhook later. There is also some thoughts around implementing #93 as a webhook as well. Shipping webhooks for admission-time validation is nice to have, but increases the complexity significantly and requires additional work around cert management for the webhook. The instant feedback is nice though, versus the resource being created and something erroring on the cluster at a later point in time.

[joelanford]

This is where kustomize could be useful. We could do the normal CRD generation with controller-tools, and then add a kustomize patch that updates that section of the schema to force our name validation into it.

[joelanford]

cert management for the webhook

Cert manager makes this pretty painless, but it means adding cert-manager as a dependency.