Skip to content
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.

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

fix: add additional name validation for custom resources #219

Merged
merged 4 commits into from
Apr 3, 2024
Merged

Conversation

cbzzz
Copy link
Contributor

@cbzzz cbzzz commented Mar 27, 2024

What type of PR is this?
/kind bug

What this PR does / why we need it:
This propagates the label constraints of Linode resources to their associated CAPL CustomResourceDefinitions via the Kubernetes Validation Rules feature. When a custom resource is created, the Kubernetes object name is validated against the label constraints of its backing Linode resources. This allows CAPL-managed resources to maintain a human-readable naming scheme between its Kubernetes representation and the backing Linode resource implementations.

See:

Testing

  1. Create Linode* resources with invalid names:
## LinodeCluster
$ export NAME="xxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxx" && kubectl apply -f - <<EOF
apiVersion: infrastructure.cluster.x-k8s.io/v1alpha1
kind: LinodeCluster
metadata:
  name: ${NAME}
spec:
  region: us-sea
EOF

The LinodeCluster "xxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxx" is invalid: metadata.name: Invalid value: "string": custom validation: linode nodebalancer: labels must be between 3..32 characters

## LinodeMachine
$ export NAME="bad--name" && kubectl apply -f - <<EOF
apiVersion: infrastructure.cluster.x-k8s.io/v1alpha1
kind: LinodeMachine
metadata:
    name: ${NAME}
spec:
    region: us-sea
    type: g5-nanode-1
EOF
The LinodeMachine "bad--name" is invalid: metadata.name: Invalid value: "string": custom validation: linode instance: labels cannot have two hyphens (--), underscores (__) or periods (..) in a row

## LinodeVPC
export NAME="bad.name"
kubectl apply -f - <<EOF
apiVersion: infrastructure.cluster.x-k8s.io/v1alpha1
kind: LinodeVPC
metadata:
  name: ${NAME}
spec:
  region: us-sea
EOF
The LinodeVPC "bad.name" is invalid: metadata.name: Invalid value: "string": custom validation: linode vpc: labels: can only contain ASCII letters, numbers, and hyphens (-), cannot have two consecutive hyphens (--), regex used for validation is: '^[-[:alnum:]]*$', see: https://www.linode.com/docs/api/vpcs/#vpc-create

Which issue(s) this PR fixes (optional, in fixes #<issue number>(, fixes #<issue_number>, ...) format, will close the issue(s) when PR gets merged):
Fixes #

Special notes for your reviewer:
Linode resources have constraints on the format of their labels, e.g.:

UIDs vs. Names
Prior to #143, CAPL was using the Kubernetes resource UID as a generalized injection(?) between the "space" of valid Kubernetes resource names (RFC 1123) to the "space" of valid Linode labels across various resource types. This provided three important properties:

  1. Uniqueness
  2. Identity: A Kubernetes resource can be mapped to a Linode resource label via its UID in the ObjectMeta.
  3. Formatting: Kubernetes UIDs are standardized and well-formatted such that they do not break any existing Linode label constraints.

Why not validate in the controller?
It didn't seem as user-friendly to require users to debug through controller logs, resource events, etc. to fix what is essentially a naming error. Kubernetes already enforces metadata validation on resources and we can ratchet off that mechanism to inject in our own additive validation constraints to the built-in ones.

See: kubernetes/kubernetes#74620

Why not automatically format Linode labels?
For many Linode resources, labels also have an additional constraint that they must be unique across a resource type in an account. Auto-formatting labels could lead to a conflict where two independent Kubernetes resources attempt to use the same label for two independent Linode resources.

Unfortunately, this issue is still occurs if a user creates an object of the same name, but in a different namespace. 😢

TODOs:

  • squashed commits
  • includes documentation
  • adds unit tests
  • adds or updates e2e tests

Copy link

codecov bot commented Mar 27, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 42.10%. Comparing base (5343789) to head (42643cd).

Additional details and impacted files
@@           Coverage Diff           @@
##             main     #219   +/-   ##
=======================================
  Coverage   42.10%   42.10%           
=======================================
  Files          25       25           
  Lines        1375     1375           
=======================================
  Hits          579      579           
  Misses        783      783           
  Partials       13       13           

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@cbzzz cbzzz added bug Something isn't working go Pull requests that update Go code labels Mar 27, 2024
x-kubernetes-validations:
- rule: 3 <= size(self) && size(self) <= 64
message: linode instance labels must be between 3..64 characters
# TODO: Combine these into one regex to validate ALL constraints on Linode instance labels at once?
Copy link
Collaborator

Choose a reason for hiding this comment

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

would these commented-out rules be covered by the k8s default metadata.name validation already?

Copy link
Contributor Author

@cbzzz cbzzz Mar 27, 2024

Choose a reason for hiding this comment

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

Not necessarily. For example, RFC 1123 allows double-hypens (which is disallowed by some Linode resource labels).

Some checks are covered (e.g. "must begin and end with an alphanumeric character"), but for the sake of completeness, I added it to the validation rules. I can look into smashing this all together into one regex if you'd like, but it (IMO) makes this very hard to read.

Copy link
Collaborator

Choose a reason for hiding this comment

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

yeah, I'm talking specifically lines 11-15 here

Copy link
Contributor Author

@cbzzz cbzzz Mar 27, 2024

Choose a reason for hiding this comment

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

Yes, they should be. However, I opted to add in a 1:1 mapping with all the Linode instance label validation rules just to make this complete in itself and easier to read.

I can remove this though if you feel it's unnecessary 🙂

Copy link
Collaborator

Choose a reason for hiding this comment

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

that is fair, if we are going to leave it commented out could we just write something describe why we are keeping them in there but not enabling them?

Copy link
Contributor Author

@cbzzz cbzzz Mar 28, 2024

Choose a reason for hiding this comment

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

I've cleaned up the commented-out rules and updated the error messages. I've also updated the "Testing" section of the Description to better show what the actual error messages from the API server would look like.

Technically, this rule in the LinodeMachine and LinodeMachineTemplate definitions is covered by the default validation, but I've left it in for context:

      - rule: self.matches('^[[:alnum:]]([-_.[:alnum:]]+[[:alnum:]])*$')
        message: >-
          custom validation:
          linode instance: labels:
          must begin and end with an alphanumeric character,
          may only consist of alphanumeric characters, hyphens (-), underscores (_) or periods (.),
          cannot have two hyphens (--), underscores (__) or periods (..) in a row,
          regex used for validation is: '^[[:alnum:]]([-_.[:alnum:]]+[[:alnum:]])*$',
          see: https://www.linode.com/docs/api/linode-instances/#linode-create

(and in case we would like to optimize this in the future by combining all the rules into one regex idk regex sue me 🤷‍♀️)

@cbzzz cbzzz force-pushed the fix.labels branch 7 times, most recently from 914561f to f53293a Compare March 28, 2024 16:38
eljohnson92
eljohnson92 previously approved these changes Mar 29, 2024
komer3
komer3 previously approved these changes Apr 1, 2024
Copy link
Contributor

@komer3 komer3 left a comment

Choose a reason for hiding this comment

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

LGTM!

eljohnson92
eljohnson92 previously approved these changes Apr 2, 2024
@cbzzz cbzzz dismissed stale reviews from komer3 and eljohnson92 via 9d905de April 2, 2024 14:16
Removes the label suffix from cluster loadbalancers. This allows for
longer cluster names since Linode Nodebalancer labels are constrained
to strings of 3..32 characters in length.

See: https://www.linode.com/docs/api/nodebalancers/#nodebalancer-create
This propagates the label constraints of Linode resources to their associated
CustomResourceDefinitions via the Kubernetes Validation Rules feature. When a
custom resource is created, the Kubernetes object name is validated against the
label constraints of its backing Linode resources. This allows CAPL-managed
resources to maintain a human-readable naming scheme between its Kubernetes
representation and the backing Linode implementation.

Validation rules are implemented via Kustomize JSON patches due to limitations
with Kubebuilder and Strategic Merge Patching with CRDs in Kubernetes.

See:
- https://kubernetes.io/docs/tasks/extend-kubernetes/custom-resources/custom-resource-definitions/#validation-rules
- https://www.github.com/kubernetes/kubernetes/issues/74620
- https://www.github.com/kubernetes-sigs/kubebuilder/issues/1074
- https://www.github.com/kubernetes/kubernetes/issues/113223
This further propagates the label constraints of Linode resources to their
associated CustomResourceDefinition templates via the Kubernetes Validation
Rules feature.
@cbzzz cbzzz merged commit de59fb1 into main Apr 3, 2024
8 checks passed
@cbzzz cbzzz deleted the fix.labels branch April 3, 2024 14:12
AshleyDumaine pushed a commit that referenced this pull request Apr 19, 2024
* fix: remove cluster nodebalancer label suffix

Removes the label suffix from cluster loadbalancers. This allows for
longer cluster names since Linode Nodebalancer labels are constrained
to strings of 3..32 characters in length.

See: https://www.linode.com/docs/api/nodebalancers/#nodebalancer-create

* fix: add name validation for custom resources

This propagates the label constraints of Linode resources to their associated
CustomResourceDefinitions via the Kubernetes Validation Rules feature. When a
custom resource is created, the Kubernetes object name is validated against the
label constraints of its backing Linode resources. This allows CAPL-managed
resources to maintain a human-readable naming scheme between its Kubernetes
representation and the backing Linode implementation.

Validation rules are implemented via Kustomize JSON patches due to limitations
with Kubebuilder and Strategic Merge Patching with CRDs in Kubernetes.

See:
- https://kubernetes.io/docs/tasks/extend-kubernetes/custom-resources/custom-resource-definitions/#validation-rules
- https://www.github.com/kubernetes/kubernetes/issues/74620
- https://www.github.com/kubernetes-sigs/kubebuilder/issues/1074
- https://www.github.com/kubernetes/kubernetes/issues/113223

* fix: add name validation for custom resource templates

This further propagates the label constraints of Linode resources to their
associated CustomResourceDefinition templates via the Kubernetes Validation
Rules feature.

* chore: e2e: clean up cluster name format
amold1 pushed a commit that referenced this pull request May 17, 2024
* fix: remove cluster nodebalancer label suffix

Removes the label suffix from cluster loadbalancers. This allows for
longer cluster names since Linode Nodebalancer labels are constrained
to strings of 3..32 characters in length.

See: https://www.linode.com/docs/api/nodebalancers/#nodebalancer-create

* fix: add name validation for custom resources

This propagates the label constraints of Linode resources to their associated
CustomResourceDefinitions via the Kubernetes Validation Rules feature. When a
custom resource is created, the Kubernetes object name is validated against the
label constraints of its backing Linode resources. This allows CAPL-managed
resources to maintain a human-readable naming scheme between its Kubernetes
representation and the backing Linode implementation.

Validation rules are implemented via Kustomize JSON patches due to limitations
with Kubebuilder and Strategic Merge Patching with CRDs in Kubernetes.

See:
- https://kubernetes.io/docs/tasks/extend-kubernetes/custom-resources/custom-resource-definitions/#validation-rules
- https://www.github.com/kubernetes/kubernetes/issues/74620
- https://www.github.com/kubernetes-sigs/kubebuilder/issues/1074
- https://www.github.com/kubernetes/kubernetes/issues/113223

* fix: add name validation for custom resource templates

This further propagates the label constraints of Linode resources to their
associated CustomResourceDefinition templates via the Kubernetes Validation
Rules feature.

* chore: e2e: clean up cluster name format
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working go Pull requests that update Go code
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants