Add handling for indistinct ParentRefs with conformance tests #2350
Conversation
k8s-ci-robot
added
release-note
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: youngnick The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
k8s-ci-robot
added
cncf-cla: yes
youngnick
force-pushed
the
add-indistinct-parentref
branch
2 times, most recently
from
a018257 to
6651d2c
Compare
Signed-off-by: Nick Young <[email protected]>
youngnick
force-pushed
the
add-indistinct-parentref
branch
from
6651d2c to
a5152ca
Compare
|
/hold for multiple review if necessary. |
k8s-ci-robot
added
the
do-not-merge/hold
|
/cc |
| // multiple ParentRefs that are not distinct. See the ParentRef | ||
| // definition for the definition of distinct. | ||
| RouteReasonParentsNotDistinct RouteConditionReason = "ParentsNotDistinct" | ||
|
|
||
| // This reason is used with the "Accepted" condition when a value for an Enum | ||
| // is not recognized. | ||
| RouteReasonUnsupportedValue RouteConditionReason = "UnsupportedValue" |
There was a problem hiding this comment.
I'm pretty happy with the language changes in here 👍
We should make sure we hold for @robscott before merging.
/hold
| @@ -0,0 +1,76 @@ | |||
| /* | |||
| Copyright 2022 The Kubernetes Authors. | |||
There was a problem hiding this comment.
| Copyright 2022 The Kubernetes Authors. | |
| Copyright 2023 The Kubernetes Authors. |
There was a problem hiding this comment.
Thanks @youngnick! Mostly LGTM, just one question on how this will work in practice.
| // This reason is used with the "Accepted" condition when there are | ||
| // multiple ParentRefs that are not distinct. See the ParentRef | ||
| // definition for the definition of distinct. | ||
| RouteReasonParentsNotDistinct RouteConditionReason = "ParentsNotDistinct" |
There was a problem hiding this comment.
What do we want to happen in the following case?
parentRefs:
- name: "foo"
- name: "foo"
sectionName: "bar"Do we always want status to look something like this?
status:
parents:
- parentRef:
- name: foo
conditions:
type: Accepted
status: True
- parentRef:
- name: foo
sectionName: Bar
conditions:
type: Accepted
status: False
reason: ParentsNotDistinctOr do we always want to consider both parents unaccepted in this state? I'd argue that it makes sense to accept one of the indistinct parents, preferably the oldest/first one.
There was a problem hiding this comment.
We talked about this one on the meeting today, and decided we should see if CEL can provide validation to protect us from these situations.
|
/cc |
k8s-ci-robot
added
the
needs-rebase
|
PR needs rebase. 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 kubernetes/test-infra repository. |
|
Closing in favor of #2433 (which copies some of the content from this one). /close |
|
@robscott: Closed this PR. In 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 kubernetes/test-infra repository. |
/kind cleanup
/kind test
/area conformance
-->
What this PR does / why we need it:
This PR adds clarity around how ParentRefs must be distinct, with conformance tests to verify.
Which issue(s) this PR fixes:
Fixes #2326
Fixes #1925
Does this PR introduce a user-facing change?: