Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Updates ParentRef Status Conditions #495

Merged
merged 2 commits into from
Oct 12, 2022
Merged

Updates ParentRef Status Conditions #495

merged 2 commits into from
Oct 12, 2022

Conversation

danehans
Copy link
Contributor

@danehans danehans commented Oct 4, 2022

Updates the Accepted condition type to use RouteReasonUnsupportedValue instead of the ResolvedRefs condition type.

Fixes #494

Signed-off-by: danehans daneyonhansen@gmail.com

@danehans danehans requested a review from a team as a code owner October 4, 2022 19:39
@danehans
Copy link
Contributor Author

danehans commented Oct 4, 2022

@skriss with this PR an Accepted: False route is still considered an attached route from the parent Gateway. I would expect Accepted: False routes to not be attached. Is that correct?

@danehans danehans added the area/translator Issues related to Gateway's translation service, e.g. translating Gateway APIs into the IR. label Oct 4, 2022
@danehans danehans added this to the 0.2.0 milestone Oct 4, 2022
@danehans danehans mentioned this pull request Oct 5, 2022
Closed
@danehans
Copy link
Contributor Author

danehans commented Oct 8, 2022

@arkodg from my understanding of #516, this PR is still required.

@skriss
Copy link
Contributor

skriss commented Oct 11, 2022

@skriss with this PR an Accepted: False route is still considered an attached route from the parent Gateway. I would expect Accepted: False routes to not be attached. Is that correct?

Agree, I'd expect an Accepted: False route to not be attached to its parent(s).

skriss
skriss reviewed Oct 11, 2022
View reviewed changes
...nal/gatewayapi/testdata/httproute-with-header-filter-duplicate-add-multiple-filters.out.yaml Show resolved Hide resolved
internal/gatewayapi/testdata/httproute-with-header-filter-duplicate-adds.out.yaml Show resolved Hide resolved
.../gatewayapi/testdata/httproute-with-header-filter-duplicate-remove-multiple-filters.out.yaml Show resolved Hide resolved
internal/gatewayapi/testdata/httproute-with-header-filter-duplicate-removes.out.yaml Show resolved Hide resolved
internal/gatewayapi/translator.go Outdated
Comment on lines 841 to 846
parentRef.SetCondition(
v1beta1.RouteConditionResolvedRefs,
metav1.ConditionFalse,
v1beta1.RouteConditionAccepted,
metav1.ConditionTrue,
v1beta1.RouteReasonUnsupportedValue,
fmt.Sprintf("RequestHeaderModifier Filter already configures request header: %s to be added, ignoring second entry", headerKey),
)
Copy link
Contributor

Choose a reason for hiding this comment

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

From my read of the spec, we should either be ignoring the duplicate header, or appending its value:

But it doesn't seem like we need to be setting a condition here.

Regardless, the combo of Accepted: true with reason UnsupportedValue does not seem valid -- if we're using the UnsupportedValue reason, then the status should be False.

Copy link
Contributor

Choose a reason for hiding this comment

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

/cc @AliceProxy

Copy link
Contributor Author

@danehans danehans Oct 11, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

From my read of the spec, we should either be ignoring the duplicate header, or appending its value:

The code currently ignores duplicate headers from the add/remove operations. Commit e3210aa no longer sets a status condition for these cases and therefore an Accepted=true condition is surfaced.

internal/gatewayapi/translator.go Outdated Show resolved Hide resolved
internal/gatewayapi/translator.go Outdated Show resolved Hide resolved
@danehans danehans mentioned this pull request Oct 11, 2022
Closed
@danehans
Updates ParentRef Status Conditions
4b114ed 
Signed-off-by: danehans <daneyonhansen@gmail.com>
@codecov-commenter
Copy link

codecov-commenter commented Oct 11, 2022 •
edited
Loading

Codecov Report

Merging #495 (e3210aa) into main (5d5c28e) will increase coverage by 0.03%.
The diff coverage is 66.66%.

@@            Coverage Diff             @@
##             main     #495      +/-   ##
==========================================
+ Coverage   60.21%   60.24%   +0.03%     
==========================================
  Files          45       45              
  Lines        5582     5564      -18     
==========================================
- Hits         3361     3352       -9     
+ Misses       2011     2003       -8     
+ Partials      210      209       -1
Impacted Files Coverage Δ
internal/gatewayapi/translator.go 85.52% <66.66%> (-0.23%) ⬇️
internal/provider/kubernetes/gatewayclass.go 68.84% <0.00%> (-1.45%) ⬇️
internal/provider/kubernetes/gateway.go 49.23% <0.00%> (-1.10%) ⬇️
internal/provider/kubernetes/tlsroute.go 59.72% <0.00%> (+6.01%) ⬆️
internal/provider/kubernetes/helpers.go 80.43% <0.00%> (+6.52%) ⬆️

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

@danehans
Resolved @skriss 10-11-22 Feedback
e3210aa 
Signed-off-by: danehans <daneyonhansen@gmail.com>
skriss
skriss approved these changes Oct 11, 2022
View reviewed changes
Copy link
Contributor

@skriss skriss left a comment

Choose a reason for hiding this comment

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

Looks reasonable to me, and we'll likely revisit some of this with upcoming Gateway API spec changes around conditions.

@danehans danehans merged commit 2fb3ca8 into envoyproxy:main Oct 12, 2022
@danehans danehans deleted the issue_494 branch October 12, 2022 00:57
danehans added a commit to danehans/gateway that referenced this pull request Nov 3, 2022
@danehans
Updates ParentRef Status Conditions (envoyproxy#495)
b901e5e 
* Updates ParentRef Status Conditions

Signed-off-by: danehans <daneyonhansen@gmail.com>

* Resolved @skriss 10-11-22 Feedback

Signed-off-by: danehans <daneyonhansen@gmail.com>

Signed-off-by: danehans <daneyonhansen@gmail.com>
This pull request was closed.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@LukeShu LukeShu LukeShu left review comments

@skriss skriss skriss approved these changes

Assignees
No one assigned
Labels
area/translator Issues related to Gateway's translation service, e.g. translating Gateway APIs into the IR.
Projects
None yet
Milestone
0.2.0
Development

Successfully merging this pull request may close these issues.

Improper Use of UnsupportedValue HTTPRoute Condition Reason
4 participants
@danehans @skriss @codecov-commenter @LukeShu