CONSOLE-5163: Add labels field to Ingress componentRoutes#2845
Conversation
Add a labels field (map[string]string) to ComponentRouteSpec in the Ingress config API. This allows cluster admins to specify labels on component routes that IngressControllers use for route selection and sharding.
Add a labels field (map[string]string) to ComponentRouteSpec in the Ingress config API. This allows cluster admins to specify labels on component routes that IngressControllers use for route selection and sharding. The field includes: - +mapType=granular for proper strategic merge patch behavior - CEL validation for Kubernetes label key/value format compliance - MaxProperties=8 to bound the number of labels - Unit tests for valid labels, empty labels, and invalid key/value rejection
|
Pipeline controller notification For optional jobs, comment This repository is configured in: LGTM mode |
openshift-ci-robot
added
the
jira/valid-reference
|
@Leo6Leo: This pull request references CONSOLE-5163 which is a valid jira issue. Warning: The referenced jira issue has an invalid target version for the target branch this PR targets: expected the story to target the "5.0.0" version, but no target version was set. DetailsIn response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the openshift-eng/jira-lifecycle-plugin repository. |
|
Hello @Leo6Leo! Some important instructions when contributing to openshift/api: |
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughThis PR introduces the Suggested reviewers
🚥 Pre-merge checks | ✅ 14 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (14 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
openshift-ci
Bot
added
the
size/L
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: The full list of commands accepted by this bot can be found here. DetailsNeeds approval from an approver in each of these files:Approvers can indicate their approval by writing |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with 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.
Inline comments:
In `@config/v1/types_ingress.go`:
- Around line 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.
- Around line 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.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository YAML (base), Central YAML (inherited)
Review profile: CHILL
Plan: Enterprise
Run ID: b6586099-0cdf-4a7c-bbfb-fe467ed6e955
⛔ Files ignored due to path filters (5)
config/v1/zz_generated.crd-manifests/0000_10_config-operator_01_ingresses.crd.yamlis excluded by!**/zz_generated.crd-manifests/*config/v1/zz_generated.deepcopy.gois excluded by!**/zz_generated*config/v1/zz_generated.featuregated-crd-manifests/ingresses.config.openshift.io/AAA_ungated.yamlis excluded by!**/zz_generated.featuregated-crd-manifests/**config/v1/zz_generated.swagger_doc_generated.gois excluded by!**/zz_generated*openapi/generated_openapi/zz_generated.openapi.gois excluded by!openapi/**,!**/zz_generated*
📒 Files selected for processing (3)
config/v1/tests/ingresses.config.openshift.io/AAA_ungated.yamlconfig/v1/types_ingress.gopayload-manifests/crds/0000_10_config-operator_01_ingresses.crd.yaml
| // '-', '_', or '.', and must start and end with an alphanumeric character. | ||
| // +optional | ||
| // +mapType=granular | ||
| // +kubebuilder:validation:MaxProperties=8 |
There was a problem hiding this comment.
What is the rationale for limiting to 8 labels? Please document the reasoning.
There was a problem hiding this comment.
The limit of 8 was chosen as a practical upper bound — in typical route sharding scenarios, IngressController selectors only need 1-2 labels, so 8 seemed generous while preventing unbounded growth. That said, there is no strong technical justification for this specific number.
Would you prefer we remove the limit entirely, or do you have a recommended ceiling for map fields like this? Happy to adjust. @saschagrunert
There was a problem hiding this comment.
8 is fine. Maps need a bound for CEL cost estimation and to prevent unbounded growth. For route sharding where 1-2 labels is typical, 8 is generous enough. No need to change it.
| // +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)" |
There was a problem hiding this comment.
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.
| // +kubebuilder:validation:MaxProperties=8 | ||
| // +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)" | ||
| Labels map[string]string `json:"labels,omitempty"` |
There was a problem hiding this comment.
Two issues here:
-
FeatureGate required. New fields on stable (compatibility-level-1) APIs must be behind a FeatureGate. Add a gate in
features/features.goand mark this field with+openshift:enable:FeatureGate=<GateName>. -
make lintfails. Add+kubebuilder:validation:MinProperties=1to prevent semantically meaninglesslabels: {}.
| kind: Ingress | ||
| spec: | ||
| domain: apps.example.com | ||
| - name: Should be able to create an Ingress with componentRoutes containing labels |
There was a problem hiding this comment.
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
Co-authored-by: Sascha Grunert <sgrunert@redhat.com>
openshift-ci
Bot
added
size/XXL
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
config/v1/tests/ingresses.config.openshift.io/IngressComponentRouteLabels.yaml (1)
7-183: ⚡ Quick winAdd an explicit
labels: {}rejection test.This suite validates most constraints, but it does not lock in the
MinProperties=1behavior for an explicitly emptylabelsmap.🧪 Suggested test addition
onCreate: + - name: Should reject 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: {} + expectedError: "Too few properties"As per coding guidelines,
**/tests/**/*.yaml: “Add validation tests for new stable APIs in<group>/<version>/tests/<crd-name>/directories.”🤖 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/tests/ingresses.config.openshift.io/IngressComponentRouteLabels.yaml` around lines 7 - 183, Add a new onCreate test case that ensures an explicitly empty labels map is rejected: add a test named like "Should reject componentRoutes with empty labels map" under onCreate (alongside the other componentRoutes label tests) whose initial manifest includes labels: {} for the componentRoute and which sets expectedError to the validator message for MinProperties=1 (e.g. "Too few properties" or the project’s standard "Too few properties" string). Place it near the other negative label tests (e.g. after the "Should reject componentRoutes with invalid label value") so the suite asserts that labels: {} is invalid.
🤖 Prompt for all review comments with 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.
Inline comments:
In `@config/v1/types_ingress.go`:
- Around line 253-262: Update the field comment for the labels map to document
the kubebuilder validation markers: state that when the labels field is present
it must be a non-empty map (labels: {} is invalid) due to
+kubebuilder:validation:MinProperties=1, and note the maximum of 8 entries
enforced by +kubebuilder:validation:MaxProperties=8; keep the existing guidance
about Kubernetes-reserved prefixes and the FeatureGate
(+openshift:enable:FeatureGate=IngressComponentRouteLabels) and mark the field
as optional.
---
Nitpick comments:
In
`@config/v1/tests/ingresses.config.openshift.io/IngressComponentRouteLabels.yaml`:
- Around line 7-183: Add a new onCreate test case that ensures an explicitly
empty labels map is rejected: add a test named like "Should reject
componentRoutes with empty labels map" under onCreate (alongside the other
componentRoutes label tests) whose initial manifest includes labels: {} for the
componentRoute and which sets expectedError to the validator message for
MinProperties=1 (e.g. "Too few properties" or the project’s standard "Too few
properties" string). Place it near the other negative label tests (e.g. after
the "Should reject componentRoutes with invalid label value") so the suite
asserts that labels: {} is invalid.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository YAML (base), Central YAML (inherited)
Review profile: CHILL
Plan: Enterprise
Run ID: cafd3e64-cebb-4ddf-81fb-31f009795a25
⛔ Files ignored due to path filters (9)
config/v1/zz_generated.crd-manifests/0000_10_config-operator_01_ingresses-CustomNoUpgrade.crd.yamlis excluded by!**/zz_generated.crd-manifests/*config/v1/zz_generated.crd-manifests/0000_10_config-operator_01_ingresses-Default.crd.yamlis excluded by!**/zz_generated.crd-manifests/*config/v1/zz_generated.crd-manifests/0000_10_config-operator_01_ingresses-DevPreviewNoUpgrade.crd.yamlis excluded by!**/zz_generated.crd-manifests/*config/v1/zz_generated.crd-manifests/0000_10_config-operator_01_ingresses-OKD.crd.yamlis excluded by!**/zz_generated.crd-manifests/*config/v1/zz_generated.crd-manifests/0000_10_config-operator_01_ingresses-TechPreviewNoUpgrade.crd.yamlis excluded by!**/zz_generated.crd-manifests/*config/v1/zz_generated.featuregated-crd-manifests.yamlis excluded by!**/zz_generated*config/v1/zz_generated.featuregated-crd-manifests/ingresses.config.openshift.io/IngressComponentRouteLabels.yamlis excluded by!**/zz_generated.featuregated-crd-manifests/**config/v1/zz_generated.swagger_doc_generated.gois excluded by!**/zz_generated*openapi/generated_openapi/zz_generated.openapi.gois excluded by!openapi/**,!**/zz_generated*
📒 Files selected for processing (5)
config/v1/tests/ingresses.config.openshift.io/IngressComponentRouteLabels.yamlconfig/v1/types_ingress.gofeatures.mdfeatures/features.gopayload-manifests/crds/0000_10_config-operator_01_ingresses.crd.yaml
saschagrunert
left a comment
saschagrunert
left a comment
There was a problem hiding this comment.
Thanks for the updates, good progress addressing the earlier feedback. A few remaining items.
| reportProblemsToJiraComponent("Management Console"). | ||
| contactPerson("leoli"). | ||
| productScope(ocpSpecific). | ||
| enhancementPR("https://github.com/openshift/enhancements/pull/XXXX"). |
There was a problem hiding this comment.
Placeholder URL. This needs a real enhancement PR before merge.
There was a problem hiding this comment.
Just got a go ahead from our team's architect, so I will draft a enhancement soon and update the PR link whenever it is available.
| @@ -7,6 +7,7 @@ metadata: | |||
| include.release.openshift.io/ibm-cloud-managed: "true" | |||
| include.release.openshift.io/self-managed-high-availability: "true" | |||
| release.openshift.io/bootstrap-required: "true" | |||
| release.openshift.io/feature-set: Default | |||
There was a problem hiding this comment.
This looks manually edited. With the FeatureGate splitting the CRD into feature-set variants under zz_generated.crd-manifests/, running make update (which calls hack/update-payload-crds.sh) should produce separate payload-manifest files per feature set (like authentications does). Run the full make update and let the script handle this.
| label8: value8 | ||
| label9: value9 | ||
| expectedError: "Too many properties" | ||
| onUpdate: |
There was a problem hiding this comment.
The tests cover basic CRUD and some negatives but the CEL validations need more coverage. Please add:
- Empty string value (
ingress: "") accepted (valid per K8s label spec, non-obvious edge case) - Key name part over 63 characters rejected (tests
format.qualifiedName()enforcement) - Value with valid characters in invalid position (e.g.,
-starts-with-dash) rejected - Non-reserved prefix accepted (e.g.,
openshift.io/my-label), confirms the reserved check is not overly broad
The MinProperties/MaxProperties boundary tests are basic OpenAPI and do not need separate test cases.
Co-authored-by: Sascha Grunert <sgrunert@redhat.com>
|
@Leo6Leo: The following tests failed, say
Full PR test history. Your PR dashboard. DetailsInstructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. I understand the commands that are listed here. |
4 participants
Summary
Add a
labelsfield (map[string]string) toComponentRouteSpecin the Ingress config API (config/v1). This allows cluster admins to specify labels on component routes that IngressControllers use for route selection and sharding.Motivation
When running multiple IngressControllers (e.g., for internal vs. external traffic), each controller uses route labels/selectors to determine which routes it manages. Currently,
componentRoutesentries have no way to specify labels, so component routes (like the console) always land on the default IngressController. This makes it impossible to route console traffic through a specific shard without manual post-creation patching.Changes
labelsfield toComponentRouteSpecwith: -+optional,+mapType=granularfor proper strategic merge patchmaxProperties=8upper boundExample