search

kubernetes-sigs / gateway-api

Repository for the next iteration of composite service (e.g. Ingress) and load balancing APIs.
https://gateway-api.sigs.k8s.io
Apache License 2.0
1.75k stars 454 forks source link

Labels specified in the Gatway Infrastructure (i.e., `.spec.infrastructure.labels`) might be invalid #3197

Closed snorwin closed 5 days ago

snorwin commented 1 month ago

This enhancement request is related to GEP: In Cluster Gateway Deployments (#1762)

What would you like to be added: Along with AnnotationValue (which was introduced in #886), a new type alias called LabelValue should be introduced. Label values follow a well-defined format that can be validated using a pattern:

Valid label value:

  • must be 63 characters or less (can be empty),
  • unless empty, must begin and end with an alphanumeric character ([a-z0-9A-Z]),
  • could contain dashes (-), underscores (_), dots (.), and alphanumerics between.

Reference: https://kubernetes.io/docs/concepts/overview/working-with-objects/labels/

Why this is needed: If users can specify invalid label values in the Gateway resource, it may cause issues in the controllers if the values are not validated. The validation in the API schema eliminates the need for each controller to implement its own validation.

gauravkghildiyal commented 1 month ago

Thanks for the details and including relevant links. I have two thoughts regarding this:

  1. There's a possibility that the choice of not having any strict validation on the AnnotationValue type (which is used both within .spec.infrastructure.labels and .spec.infrastructure.annotations) may have been deliberate to cater to the needs of implementations that have some other form of "infrastructure" i.e. non Kubernetes resources: image This would imply that validating what is and is-not valid as a label value rests on the implementation to communicate.

  2. Disregarding the previous point, enforcing such a constraint now may be considered a breaking change (since we are tightening the validation).

We can wait and see what other's might feel regarding this, but my take here is to keep things as they are.

snorwin commented 1 month ago

@gauravkghildiyal thanks for looking into the issue. I fully understand your perspective. I had assumed that the infrastructure section was more tightly integrated with Kubernetes, as it is part of the in-cluster deployment GEP.

robscott commented 1 month ago

Thanks @gauravkghildiyal and @snorwin! I think that's a reasonable explanation for why this might have happened, but especially as we're considering taking this API to GA, we likely want to make sure we have the validation as precise as we can.

@keithmattix or @howardjohn would it be problematic to tighten this validation to match k8s labels? We could theoretically loosen this in the future if needed.

howardjohn commented 1 month ago

From Istio POV we are fine with matching labels. I do see the valid point that it could be used for non-k8s, but I am not personally using it for that purpose at this time.

youngnick commented 1 month ago

I agree that this should match the validation for Kubernetes labels, since most if not all implementations will be using those.

snorwin commented 4 weeks ago

@youngnick is the discussion still ongoing or should I prepare a PR?

youngnick commented 3 weeks ago

I think a PR is a good next step, we'll make sure to mention it in the community meeting once it's opened, and leave it for folks to check out for a bit before merging.

This is a breaking change, so it's important we get this in as part of the 1.2 release.