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

httproute: use lists insteads of maps for HTTPRequestHeaderFilter #681

Merged
merged 1 commit into from
Jun 10, 2021
Merged

httproute: use lists insteads of maps for HTTPRequestHeaderFilter #681

merged 1 commit into from
Jun 10, 2021

Conversation

hbagdi
Copy link
Contributor

@hbagdi hbagdi commented May 25, 2021

What type of PR is this?

/kind api-change
/kind cleanup
What this PR does / why we need it:
This matches the API conventions that prefer lists of named subobjects over maps.
Which issue(s) this PR fixes:

Fixes #658
See #657

Does this PR introduce a user-facing change?:

Headers in Request modifier filter are now represented with lists of named subobjects instead of maps.

@k8s-ci-robot k8s-ci-robot added kind/api-change Categorizes issue or PR as related to adding, removing, or otherwise changing an API kind/cleanup Categorizes issue or PR as related to cleaning up code, process, or technical debt. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. labels May 25, 2021
@k8s-ci-robot k8s-ci-robot requested review from danehans and robscott May 25, 2021 17:55
@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: hbagdi

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 /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@k8s-ci-robot k8s-ci-robot added approved Indicates a PR has been approved by an approver from all required OWNERS files. size/S Denotes a PR that changes 10-29 lines, ignoring generated files. labels May 25, 2021
@hbagdi
Copy link
Contributor Author

hbagdi commented May 25, 2021

/cc @robscott

@hbagdi hbagdi mentioned this pull request May 25, 2021
Closed
@hbagdi hbagdi force-pushed the feat/lists-header-modifier branch from 6019033 to 210b8b5 Compare May 25, 2021 18:07
@k8s-ci-robot k8s-ci-robot added size/M Denotes a PR that changes 30-99 lines, ignoring generated files. and removed size/S Denotes a PR that changes 10-29 lines, ignoring generated files. labels May 25, 2021
robscott
robscott reviewed May 26, 2021
View reviewed changes
apis/v1alpha2/httproute_types.go Outdated
// HTTPHeader represents an HTTP Header name and value as defined by RFC 7230.
type HTTPHeader struct {
// Name is the name of the HTTP query param to be matched. This must be an
// exact string match. (See
Copy link
Member

@robscott robscott May 26, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

Why is this an exact string match? Below we mention that header names are case insensitive. I think you may have meant to refer to a different section in the RFC: https://datatracker.ietf.org/doc/html/rfc7230#section-3.2.

Copy link
Contributor

Choose a reason for hiding this comment

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

I assume that an exact header match is case-insensitive?

Copy link
Member

Choose a reason for hiding this comment

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

Exact/Regex is how we match the value, the header name has always been exact (with the exception being case sensitivity).

jpeach
jpeach reviewed May 27, 2021
View reviewed changes
apis/v1alpha2/httproute_types.go Outdated
@@ -530,6 +530,23 @@ const (
HTTPRouteFilterExtensionRef HTTPRouteFilterType = "ExtensionRef"
)

// HTTPHeader represents an HTTP Header name and value as defined by RFC 7230.
type HTTPHeader struct {
// Name is the name of the HTTP query param to be matched. This must be an
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
// Name is the name of the HTTP query param to be matched. This must be an
// Name is the name of the HTTP header to be matched. This must be an

@hbagdi hbagdi force-pushed the feat/lists-header-modifier branch from 210b8b5 to 78de62e Compare May 27, 2021 16:11
@hbagdi
Copy link
Contributor Author

hbagdi commented May 27, 2021 •
edited
Loading

@jpeach @robscott I had made a copy pasta mistake. PTAL.

@hbagdi
Copy link
Contributor Author

hbagdi commented May 27, 2021 •
edited
Loading

One question:
I tried to do struct embedding here but kubebuilder CRD generator didn't like that. I know upstream doesn't recommend embedding unless it absolutely makes sense, which I think is the case here. Should we report this problem to kubebuilder community or is this expected?

@robscott
Copy link
Member

I tried to do struct embedding here but kubebuilder CRD generator didn't like that. I know upstream doesn't recommend embedding unless it absolutely makes sense, which I think is the case here. Should we report this problem to kubebuilder community or is this expected?

I don't have a strong opinion on struct embedding here, but don't think it needs to block this PR getting in. What happened to the CRD, was the yaml just inaccurate/unexpected? If so, I think it's worth filing a bug on controller-tools repo with a reference to the command we're using to generate CRDs.

@robscott
Copy link
Member

robscott commented Jun 8, 2021

@hbagdi This looks like it's close, let me know when it's ready for me to take another look.

@hbagdi hbagdi force-pushed the feat/lists-header-modifier branch from 55c1ce6 to 6b690f8 Compare June 10, 2021 03:42
@hbagdi
Copy link
Contributor Author

hbagdi commented Jun 10, 2021

@robscott updated. PTAL.

@robscott
Copy link
Member

Thanks @hbagdi! This LGTM but looks like it needs another make generate run.

@hbagdi hbagdi force-pushed the feat/lists-header-modifier branch from 6b690f8 to 5250dda Compare June 10, 2021 20:05
@hbagdi
Copy link
Contributor Author

hbagdi commented Jun 10, 2021

Fixed, I blazed by a bit too fast.

@robscott
Copy link
Member

Thanks!

/lgtm

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Jun 10, 2021
@k8s-ci-robot k8s-ci-robot merged commit c8fd333 into kubernetes-sigs:master Jun 10, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@robscott robscott robscott left review comments

@jpeach jpeach jpeach left review comments

@danehans danehans Awaiting requested review from danehans

Assignees

@robscott robscott

Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. breaking-change cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. kind/api-change Categorizes issue or PR as related to adding, removing, or otherwise changing an API kind/cleanup Categorizes issue or PR as related to cleaning up code, process, or technical debt. lgtm "Looks good to me", indicates that a PR is ready to be merged. size/M Denotes a PR that changes 30-99 lines, ignoring generated files.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Transition from maps to lists of named subobjects
4 participants
@hbagdi @k8s-ci-robot @robscott @jpeach