Skip to content
Open
Show file tree
Hide file tree
Changes from all 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 on lines +249 to +255
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.

The godoc needs a few additions:

  1. Document the MaxProperties=8 constraint.
  2. Explain what happens when the field is omitted (required for optional fields on config APIs).
  3. The comment claims "optional prefix up to 253 characters" but the CEL regex doesn't enforce the 253-char limit. Either enforce it or soften the wording.
  4. Mention the operational impact: changing labels can move routes between IngressController shards.
Suggested change
// 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.
// 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. Changing labels may cause the
// route to be reassigned to a different IngressController.
// When omitted, no additional labels are applied to the component route.
// Label keys and values must conform to Kubernetes label conventions:
// keys must be 1-63 characters (with optional DNS subdomain prefix followed
// by a slash), and values must be 0-63 characters, consisting of alphanumeric
// characters, '-', '_', or '.', and must start and end with an alphanumeric
// character.
// A maximum of 8 labels may be specified.

// +optional
// +mapType=granular
// +kubebuilder:validation:MaxProperties=8
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.

What is the rationale for limiting to 8 labels? Please document the reasoning.

// +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 on lines +249 to +260
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

⚠️ Potential issue | 🟡 Minor | ⚡ Quick win

Document omitted behavior and map-size constraint in the field comment.

The Labels comment (Lines 249-256) does not state behavior when omitted and does not document the MaxProperties=8 constraint declared on Line 258.

As per coding guidelines, **/types*.go: Documentation for +optional fields must explain the behavior when the field is omitted, and all kubebuilder validation markers must be documented in the field's comment.

🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@config/v1/types_ingress.go` around lines 249 - 260, The comment for the
Labels field is missing the behavior when omitted and doesn't document the
MaxProperties=8 kubebuilder constraint; update the comment for the Labels (the
map with +optional, +mapType=granular and
+kubebuilder:validation:MaxProperties=8) to state that the field is optional,
what happens when it is omitted (e.g., no additional labels will be applied to
the created route), and explicitly document the MaxProperties=8 limit (maximum
of 8 user-supplied labels) along with the existing label key/value validation
rules so all kubebuilder markers are reflected in the field documentation.

Comment on lines +259 to +260
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.

Labels map[string]string `json:"labels,omitempty"`
Comment on lines +249 to +261
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

⚠️ Potential issue | 🟠 Major | 🏗️ Heavy lift

Gate the new stable v1 API field.

Labels is introduced on Line 261 without a +openshift:enable:FeatureGate=... annotation. Stable API field additions should be feature-gated before merge.

As per coding guidelines, **/types*.go: New fields on stable APIs should be introduced behind a feature gate using +openshift:enable:FeatureGate=MyFeatureGate.

🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@config/v1/types_ingress.go` around lines 249 - 261, Add an OpenShift
feature-gate annotation above the new Labels field so the stable API field is
gated; specifically, add a kubebuilder/openShift annotation like
"+openshift:enable:FeatureGate=MyFeatureGate" immediately before the "Labels
map[string]string `json:\"labels,omitempty\"`" declaration in types_ingress.go
(replace MyFeatureGate with the chosen gate name, e.g., IngressLabels) so the
new Labels field is activated only when that feature gate is enabled.

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