Skip to content
Open
Show file tree
Hide file tree
Changes from 2 commits
Commits
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
155 changes: 155 additions & 0 deletions config/v1/tests/ingresses.config.openshift.io/AAA_ungated.yaml
Original file line number Diff line number Diff line change
Expand Up @@ -23,6 +23,129 @@ tests:
kind: Ingress
spec:
domain: apps.example.com
- name: Should be able to create an Ingress with componentRoutes containing labels

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Once the FeatureGate is added, these tests should move to a gate-specific file (e.g., ComponentRouteLabels.yaml) instead of AAA_ungated.yaml.

Also, please add the following test cases:

  • Label key with DNS prefix (e.g., example.com/my-label: value), since the prefix path is the most complex part of the key regex
  • Keys and values at the maximum allowed length (63 characters) to verify boundary behavior
  • Update to remove labels (labels present -> no labels)
  • Update to change an existing label's value

initial: |
apiVersion: config.openshift.io/v1
kind: Ingress
spec:
domain: apps.example.com
componentRoutes:
- name: console
namespace: openshift-console
hostname: console.example.com
labels:
ingress: shard-console
expected: |
apiVersion: config.openshift.io/v1
kind: Ingress
spec:
domain: apps.example.com
componentRoutes:
- name: console
namespace: openshift-console
hostname: console.example.com
labels:
ingress: shard-console
- name: Should be able to create an Ingress with multiple componentRoutes with labels
initial: |
apiVersion: config.openshift.io/v1
kind: Ingress
spec:
domain: apps.example.com
componentRoutes:
- name: console-2
namespace: openshift-console
hostname: console.internal.corp.example.com
labels:
ingress: shard-console2
- name: console-3
namespace: openshift-console
hostname: console.private.corp.example.com
labels:
ingress: shard-console3
expected: |
apiVersion: config.openshift.io/v1
kind: Ingress
spec:
domain: apps.example.com
componentRoutes:
- name: console-2
namespace: openshift-console
hostname: console.internal.corp.example.com
labels:
ingress: shard-console2
- name: console-3
namespace: openshift-console
hostname: console.private.corp.example.com
labels:
ingress: shard-console3
- name: Should be able to create componentRoutes with empty labels map
initial: |
apiVersion: config.openshift.io/v1
kind: Ingress
spec:
domain: apps.example.com
componentRoutes:
- name: console
namespace: openshift-console
hostname: console.example.com
labels: {}
expected: |
apiVersion: config.openshift.io/v1
kind: Ingress
spec:
domain: apps.example.com
componentRoutes:
- name: console
namespace: openshift-console
hostname: console.example.com
- name: Should reject componentRoutes with invalid label key
initial: |
apiVersion: config.openshift.io/v1
kind: Ingress
spec:
domain: apps.example.com
componentRoutes:
- name: console
namespace: openshift-console
hostname: console.example.com
labels:
"!!!bad": value
expectedError: "label keys must be valid Kubernetes label keys"
- name: Should reject componentRoutes with invalid label value
initial: |
apiVersion: config.openshift.io/v1
kind: Ingress
spec:
domain: apps.example.com
componentRoutes:
- name: console
namespace: openshift-console
hostname: console.example.com
labels:
ingress: "invalid value with spaces!"
expectedError: "label values must be valid Kubernetes label values"
- name: Should reject componentRoutes with more than 8 labels
initial: |
apiVersion: config.openshift.io/v1
kind: Ingress
spec:
domain: apps.example.com
componentRoutes:
- name: console
namespace: openshift-console
hostname: console.example.com
labels:
label1: value1
label2: value2
label3: value3
label4: value4
label5: value5
label6: value6
label7: value7
label8: value8
label9: value9
expectedError: "Too many properties"
onUpdate:
- name: Should not be able to change domain once set
initial: |
Expand Down Expand Up @@ -54,3 +177,35 @@ tests:
spec:
domain: apps.example.com
appsDomain: custom.example.com
- name: Should be able to add labels to componentRoutes on update
initial: |
apiVersion: config.openshift.io/v1
kind: Ingress
spec:
domain: apps.example.com
componentRoutes:
- name: console
namespace: openshift-console
hostname: console.example.com
updated: |
apiVersion: config.openshift.io/v1
kind: Ingress
spec:
domain: apps.example.com
componentRoutes:
- name: console
namespace: openshift-console
hostname: console.example.com
labels:
ingress: shard-console
expected: |
apiVersion: config.openshift.io/v1
kind: Ingress
spec:
domain: apps.example.com
componentRoutes:
- name: console
namespace: openshift-console
hostname: console.example.com
labels:
ingress: shard-console
14 changes: 14 additions & 0 deletions config/v1/types_ingress.go
Original file line number Diff line number Diff line change
Expand Up @@ -245,6 +245,20 @@ type ComponentRouteSpec struct {
// the Secret specification for a serving certificate will not be needed.
// +optional
ServingCertKeyPairSecret SecretNameReference `json:"servingCertKeyPairSecret"`

// labels defines additional labels to be applied to the route created
// for the component. These labels are used by the IngressController to
// determine which routes it should manage.
// Label keys and values must conform to Kubernetes label conventions:
// keys must be 1-63 characters (with optional prefix up to 253 characters),
// and values must be 0-63 characters, consisting of alphanumeric characters,
// '-', '_', or '.', and must start and end with an alphanumeric character.
Comment thread
Leo6Leo marked this conversation as resolved.
Outdated
// +optional
// +mapType=granular
// +kubebuilder:validation:MaxProperties=8
Comment thread
Leo6Leo marked this conversation as resolved.
Comment thread
coderabbitai[bot] marked this conversation as resolved.
// +kubebuilder:validation:XValidation:rule="self.all(key, key.matches('^([a-z0-9]([-a-z0-9]*[a-z0-9])?(\\\\.[a-z0-9]([-a-z0-9]*[a-z0-9])?)*[/])?([A-Za-z0-9]([-A-Za-z0-9_.]{0,61}[A-Za-z0-9])?)$'))",message="label keys must be valid Kubernetes label keys"
// +kubebuilder:validation:XValidation:rule="self.all(key, self[key].matches('^(([A-Za-z0-9][-A-Za-z0-9_.]{0,61})?[A-Za-z0-9])?$'))",message="label values must be valid Kubernetes label values (at most 63 characters, alphanumeric, '-', '_', or '.', must start and end with alphanumeric)"
Comment thread
coderabbitai[bot] marked this conversation as resolved.
Outdated

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Consider using CEL Kubernetes library functions (format.qualifiedName() for keys, format.labelValue() for values) instead of hand-rolled regexes. They are more maintainable, cover the full Kubernetes label spec (including the 253-char prefix limit the current regex misses), and provide better error messages.

Also consider whether keys with reserved prefixes (kubernetes.io/, k8s.io/) should be rejected, since those are reserved for system use.

Comment thread
Leo6Leo marked this conversation as resolved.
Outdated
Labels map[string]string `json:"labels,omitempty"`
Comment thread
coderabbitai[bot] marked this conversation as resolved.
Outdated

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Two issues here:

  1. FeatureGate required. New fields on stable (compatibility-level-1) APIs must be behind a FeatureGate. Add a gate in features/features.go and mark this field with +openshift:enable:FeatureGate=<GateName>.

  2. make lint fails. Add +kubebuilder:validation:MinProperties=1 to prevent semantically meaningless labels: {}.

}

// ComponentRouteStatus contains information allowing configuration of a route's hostname and serving certificate.
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -75,6 +75,27 @@ spec:
the route.
pattern: ^([a-zA-Z0-9\p{S}\p{L}]((-?[a-zA-Z0-9\p{S}\p{L}]{0,62})?)|([a-zA-Z0-9\p{S}\p{L}](([a-zA-Z0-9-\p{S}\p{L}]{0,61}[a-zA-Z0-9\p{S}\p{L}])?)(\.)){1,}([a-zA-Z\p{L}]){2,63})$|^(([a-z0-9][-a-z0-9]{0,61}[a-z0-9]|[a-z0-9]{1,63})[\.]){0,}([a-z0-9][-a-z0-9]{0,61}[a-z0-9]|[a-z0-9]{1,63})$
type: string
labels:
additionalProperties:
type: string
description: |-
labels defines additional labels to be applied to the route created
for the component. These labels are used by the IngressController to
determine which routes it should manage.
Label keys and values must conform to Kubernetes label conventions:
keys must be 1-63 characters (with optional prefix up to 253 characters),
and values must be 0-63 characters, consisting of alphanumeric characters,
'-', '_', or '.', and must start and end with an alphanumeric character.
maxProperties: 8
type: object
x-kubernetes-map-type: granular
x-kubernetes-validations:
- message: label keys must be valid Kubernetes label keys
rule: self.all(key, key.matches('^([a-z0-9]([-a-z0-9]*[a-z0-9])?(\\.[a-z0-9]([-a-z0-9]*[a-z0-9])?)*[/])?([A-Za-z0-9]([-A-Za-z0-9_.]{0,61}[A-Za-z0-9])?)$'))
- message: label values must be valid Kubernetes label values
(at most 63 characters, alphanumeric, '-', '_', or '.',
must start and end with alphanumeric)
rule: self.all(key, self[key].matches('^(([A-Za-z0-9][-A-Za-z0-9_.]{0,61})?[A-Za-z0-9])?$'))
name:
description: |-
name is the logical name of the route to customize.
Expand Down
11 changes: 10 additions & 1 deletion config/v1/zz_generated.deepcopy.go

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

Original file line number Diff line number Diff line change
Expand Up @@ -76,6 +76,27 @@ spec:
the route.
pattern: ^([a-zA-Z0-9\p{S}\p{L}]((-?[a-zA-Z0-9\p{S}\p{L}]{0,62})?)|([a-zA-Z0-9\p{S}\p{L}](([a-zA-Z0-9-\p{S}\p{L}]{0,61}[a-zA-Z0-9\p{S}\p{L}])?)(\.)){1,}([a-zA-Z\p{L}]){2,63})$|^(([a-z0-9][-a-z0-9]{0,61}[a-z0-9]|[a-z0-9]{1,63})[\.]){0,}([a-z0-9][-a-z0-9]{0,61}[a-z0-9]|[a-z0-9]{1,63})$
type: string
labels:
additionalProperties:
type: string
description: |-
labels defines additional labels to be applied to the route created
for the component. These labels are used by the IngressController to
determine which routes it should manage.
Label keys and values must conform to Kubernetes label conventions:
keys must be 1-63 characters (with optional prefix up to 253 characters),
and values must be 0-63 characters, consisting of alphanumeric characters,
'-', '_', or '.', and must start and end with an alphanumeric character.
maxProperties: 8
type: object
x-kubernetes-map-type: granular
x-kubernetes-validations:
- message: label keys must be valid Kubernetes label keys
rule: self.all(key, key.matches('^([a-z0-9]([-a-z0-9]*[a-z0-9])?(\\.[a-z0-9]([-a-z0-9]*[a-z0-9])?)*[/])?([A-Za-z0-9]([-A-Za-z0-9_.]{0,61}[A-Za-z0-9])?)$'))
- message: label values must be valid Kubernetes label values
(at most 63 characters, alphanumeric, '-', '_', or '.',
must start and end with alphanumeric)
rule: self.all(key, self[key].matches('^(([A-Za-z0-9][-A-Za-z0-9_.]{0,61})?[A-Za-z0-9])?$'))
name:
description: |-
name is the logical name of the route to customize.
Expand Down
1 change: 1 addition & 0 deletions config/v1/zz_generated.swagger_doc_generated.go

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

21 changes: 21 additions & 0 deletions openapi/generated_openapi/zz_generated.openapi.go

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

Original file line number Diff line number Diff line change
Expand Up @@ -75,6 +75,27 @@ spec:
the route.
pattern: ^([a-zA-Z0-9\p{S}\p{L}]((-?[a-zA-Z0-9\p{S}\p{L}]{0,62})?)|([a-zA-Z0-9\p{S}\p{L}](([a-zA-Z0-9-\p{S}\p{L}]{0,61}[a-zA-Z0-9\p{S}\p{L}])?)(\.)){1,}([a-zA-Z\p{L}]){2,63})$|^(([a-z0-9][-a-z0-9]{0,61}[a-z0-9]|[a-z0-9]{1,63})[\.]){0,}([a-z0-9][-a-z0-9]{0,61}[a-z0-9]|[a-z0-9]{1,63})$
type: string
labels:
additionalProperties:
type: string
description: |-
labels defines additional labels to be applied to the route created
for the component. These labels are used by the IngressController to
determine which routes it should manage.
Label keys and values must conform to Kubernetes label conventions:
keys must be 1-63 characters (with optional prefix up to 253 characters),
and values must be 0-63 characters, consisting of alphanumeric characters,
'-', '_', or '.', and must start and end with an alphanumeric character.
maxProperties: 8
type: object
x-kubernetes-map-type: granular
x-kubernetes-validations:
- message: label keys must be valid Kubernetes label keys
rule: self.all(key, key.matches('^([a-z0-9]([-a-z0-9]*[a-z0-9])?(\\.[a-z0-9]([-a-z0-9]*[a-z0-9])?)*[/])?([A-Za-z0-9]([-A-Za-z0-9_.]{0,61}[A-Za-z0-9])?)$'))
- message: label values must be valid Kubernetes label values
(at most 63 characters, alphanumeric, '-', '_', or '.',
must start and end with alphanumeric)
rule: self.all(key, self[key].matches('^(([A-Za-z0-9][-A-Za-z0-9_.]{0,61})?[A-Za-z0-9])?$'))
name:
description: |-
name is the logical name of the route to customize.
Expand Down