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.

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

internal/contour: Add Support for custom headers on HTTPProxy level #5586

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

deveshk0
Copy link

Introduces support for request and repsonse headers on http proxy level

Fixes: #5576

@deveshk0 deveshk0 requested a review from a team as a code owner July 25, 2023 23:37
@deveshk0 deveshk0 requested review from tsaarni and stevesloka and removed request for a team July 25, 2023 23:37
@github-actions
Copy link

Hi @deveshk0! Welcome to our community and thank you for opening your first Pull Request. Someone will review it soon. Thank you for committing to making Contour better. You can also join us on our mailing list and in our channel in the Kubernetes Slack Workspace

@github-actions
Copy link

The Contour project currently lacks enough contributors to adequately respond to all PRs.

This bot triages PRs according to the following rules:

  • After 14d of inactivity, lifecycle/stale is applied
  • After 30d of inactivity since lifecycle/stale was applied, the PR is closed

You can:

  • Mark this PR as fresh by commenting or pushing a commit
  • Close this PR
  • Offer to help out with triage

Please send feedback to the #contour channel in the Kubernetes Slack

@github-actions github-actions bot added the lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. label Aug 10, 2023
@deveshk0
Copy link
Author

can some please have a look into this?

@github-actions github-actions bot removed the lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. label Aug 14, 2023
@github-actions
Copy link

The Contour project currently lacks enough contributors to adequately respond to all PRs.

This bot triages PRs according to the following rules:

  • After 14d of inactivity, lifecycle/stale is applied
  • After 30d of inactivity since lifecycle/stale was applied, the PR is closed

You can:

  • Mark this PR as fresh by commenting or pushing a commit
  • Close this PR
  • Offer to help out with triage

Please send feedback to the #contour channel in the Kubernetes Slack

@github-actions github-actions bot added the lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. label Aug 28, 2023
@deveshk0
Copy link
Author

deveshk0 commented Sep 5, 2023

is this planned for next release?

@github-actions github-actions bot removed the lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. label Sep 6, 2023
@github-actions
Copy link

The Contour project currently lacks enough contributors to adequately respond to all PRs.

This bot triages PRs according to the following rules:

  • After 14d of inactivity, lifecycle/stale is applied
  • After 30d of inactivity since lifecycle/stale was applied, the PR is closed

You can:

  • Mark this PR as fresh by commenting or pushing a commit
  • Close this PR
  • Offer to help out with triage

Please send feedback to the #contour channel in the Kubernetes Slack

@github-actions github-actions bot added the lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. label Sep 20, 2023
@skriss skriss self-requested a review October 4, 2023 18:41
@skriss skriss added release-note/minor A minor change that needs about a paragraph of explanation in the release notes. and removed lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. labels Oct 4, 2023
@skriss skriss closed this Oct 4, 2023
@skriss skriss reopened this Oct 4, 2023
Copy link
Member

@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.

@deveshk0 thanks for the PR, this is looking good. Left a few comments to address.

apis/projectcontour/v1/httpproxy.go Show resolved Hide resolved
)
requestHeadersPolicyDefined := false
responseHeadersPolicyDefined := false
if proxy.Spec.VirtualHost.RequestHeadersPolicy != nil {
Copy link
Member

Choose a reason for hiding this comment

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

I think we will want to reuse the existing logic we have for processing these HeaderPolicies - in particular e.g. headersPolicyRoute. That function has some better validation and also handles parsing dynamic header values (see https://projectcontour.io/docs/1.26/config/request-rewriting/#dynamic-header-values), which I think we should support for vhost header policies as well. We'd want to pass false as the second argument since we won't support rewriting the Host header for vhosts.

Copy link
Author

Choose a reason for hiding this comment

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

Done

internal/dag/httpproxy_processor.go Outdated Show resolved Hide resolved
internal/envoy/v3/route.go Show resolved Hide resolved
Copy link
Member

@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.

Please also follow the instructions in https://github.com/projectcontour/contour/actions/runs/6410166216/job/17403616430?pr=5586 to add a changelog describing the change for users.

@deveshk0
Copy link
Author

deveshk0 commented Oct 5, 2023

@skriss Thanks for the review I will update to suggested changes.

@github-actions
Copy link

The Contour project currently lacks enough contributors to adequately respond to all PRs.

This bot triages PRs according to the following rules:

  • After 14d of inactivity, lifecycle/stale is applied
  • After 30d of inactivity since lifecycle/stale was applied, the PR is closed

You can:

  • Mark this PR as fresh by commenting or pushing a commit
  • Close this PR
  • Offer to help out with triage

Please send feedback to the #contour channel in the Kubernetes Slack

@github-actions github-actions bot added the lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. label Oct 20, 2023
Copy link

The Contour project currently lacks enough contributors to adequately respond to all PRs.

This bot triages PRs according to the following rules:

  • After 14d of inactivity, lifecycle/stale is applied
  • After 30d of inactivity since lifecycle/stale was applied, the PR is closed

You can:

  • Ensure your PR is passing all CI checks. PRs that are fully green are more likely to be reviewed. If you are having trouble with CI checks, reach out to the #contour channel in the Kubernetes Slack workspace.
  • Mark this PR as fresh by commenting or pushing a commit
  • Close this PR
  • Offer to help out with triage

Please send feedback to the #contour channel in the Kubernetes Slack

@github-actions github-actions bot closed this Nov 19, 2023
@deveshk0
Copy link
Author

@skriss Can you please open this? I will finish it this week

@skriss skriss reopened this Dec 6, 2023
@deveshk0
Copy link
Author

@sunjayBhatia @skriss @m-yosefpor all requested changes have been added, I have rebased it also, Please have a look

@deveshk0
Copy link
Author

Please also follow the instructions in https://github.com/projectcontour/contour/actions/runs/6410166216/job/17403616430?pr=5586 to add a changelog describing the change for users.

done

Copy link

codecov bot commented Jan 18, 2024

Codecov Report

Attention: Patch coverage is 63.63636% with 8 lines in your changes are missing coverage. Please review.

Project coverage is 81.31%. Comparing base (c3d6cb4) to head (9993502).
Report is 50 commits behind head on main.

❗ Current head 9993502 differs from pull request most recent head 86b578c. Consider uploading reports for the commit 86b578c to get more accurate results

Additional details and impacted files

Impacted file tree graph

@@            Coverage Diff             @@
##             main    #5586      +/-   ##
==========================================
- Coverage   81.56%   81.31%   -0.26%     
==========================================
  Files         133      133              
  Lines       15801    15797       -4     
==========================================
- Hits        12888    12845      -43     
- Misses       2617     2656      +39     
  Partials      296      296              
Files Coverage Δ
internal/dag/dag.go 98.40% <ø> (ø)
internal/envoy/v3/route.go 80.06% <100.00%> (+0.18%) ⬆️
internal/dag/httpproxy_processor.go 90.84% <50.00%> (-0.57%) ⬇️

... and 10 files with indirect coverage changes

@m-yosefpor
Copy link
Contributor

LGTM. thanks.

Copy link

github-actions bot commented Feb 8, 2024

The Contour project currently lacks enough contributors to adequately respond to all PRs.

This bot triages PRs according to the following rules:

  • After 14d of inactivity, lifecycle/stale is applied
  • After 30d of inactivity since lifecycle/stale was applied, the PR is closed

You can:

  • Ensure your PR is passing all CI checks. PRs that are fully green are more likely to be reviewed. If you are having trouble with CI checks, reach out to the #contour channel in the Kubernetes Slack workspace.
  • Mark this PR as fresh by commenting or pushing a commit
  • Close this PR
  • Offer to help out with triage

Please send feedback to the #contour channel in the Kubernetes Slack

@github-actions github-actions bot added the lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. label Feb 8, 2024
@m-yosefpor
Copy link
Contributor

not stale

Copy link
Member

@sunjayBhatia sunjayBhatia left a comment

Choose a reason for hiding this comment

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

For full test coverage over the precedence rules of setting header policies in different places on an HTTPRoute I think we should probably add an integration/featuretests test which should also serve as a kind of documentation for how the precedence works. A test for this would:

  • live in internal/featuretests/headerpolicy_test.go
  • configure an HTTPProxy with header policies at all levels (global, virtualhost, route, service) for the same Header name with different values
  • expect the Route/Cluster/etc are configured as expected

}

requestHeadersPolicy, err := headersPolicyRoute(proxy.Spec.VirtualHost.RequestHeadersPolicy, false, dynamicHeaders)
if err != nil {
Copy link
Member

Choose a reason for hiding this comment

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

lets add some negative test cases for if validation fails on virtualhost specified header policies

Copy link
Member

Choose a reason for hiding this comment

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

this can go alongside the other test added in builder_test.go

@@ -327,6 +327,24 @@ type VirtualHost struct {
// +optional
JWTProviders []JWTProvider `json:"jwtProviders,omitempty"`

// The policy for managing request headers during proxying.
// Headers are appended to requests in the following order,
// weighted cluster level headers,
Copy link
Member

Choose a reason for hiding this comment

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

nit: let's maybe not use "weighted cluster" just since that is an explicitly Envoy rather than Contour-specific term, we can use Service instead I think

Copy link
Author

Choose a reason for hiding this comment

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

Updated

vh: &dag.VirtualHost{
Name: "example.com",
RequestHeadersPolicy: &dag.HeadersPolicy{
Set: map[string]string{
Copy link
Member

Choose a reason for hiding this comment

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

We're only testing Set and Remove here though the implementation also includes supporting the Add operation for headers. Technically the HTTPProxy API does not have an Add mechanism so we could either omit it in the implementation or just leave it but make sure it is tested here

Copy link
Author

Choose a reason for hiding this comment

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

Added test cases for Add case also

@github-actions github-actions bot removed the lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. label Feb 9, 2024
@deveshk0
Copy link
Author

deveshk0 commented Feb 9, 2024

Thanks, @sunjayBhatia for the review I will add the requested changes.

Copy link

The Contour project currently lacks enough contributors to adequately respond to all PRs.

This bot triages PRs according to the following rules:

  • After 14d of inactivity, lifecycle/stale is applied
  • After 30d of inactivity since lifecycle/stale was applied, the PR is closed

You can:

  • Ensure your PR is passing all CI checks. PRs that are fully green are more likely to be reviewed. If you are having trouble with CI checks, reach out to the #contour channel in the Kubernetes Slack workspace.
  • Mark this PR as fresh by commenting or pushing a commit
  • Close this PR
  • Offer to help out with triage

Please send feedback to the #contour channel in the Kubernetes Slack

@github-actions github-actions bot added the lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. label Feb 24, 2024
@m-yosefpor
Copy link
Contributor

/ remove-lifecycle-stale

@github-actions github-actions bot removed the lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. label Feb 26, 2024
@deveshk0 deveshk0 force-pushed the main branch 2 times, most recently from 6b4ed1f to 27739ff Compare March 6, 2024 23:24
@sunjayBhatia sunjayBhatia self-requested a review March 6, 2024 23:24
Copy link

The Contour project currently lacks enough contributors to adequately respond to all PRs.

This bot triages PRs according to the following rules:

  • After 14d of inactivity, lifecycle/stale is applied
  • After 30d of inactivity since lifecycle/stale was applied, the PR is closed

You can:

  • Ensure your PR is passing all CI checks. PRs that are fully green are more likely to be reviewed. If you are having trouble with CI checks, reach out to the #contour channel in the Kubernetes Slack workspace.
  • Mark this PR as fresh by commenting or pushing a commit
  • Close this PR
  • Offer to help out with triage

Please send feedback to the #contour channel in the Kubernetes Slack

@github-actions github-actions bot added lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. and removed lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. labels Mar 21, 2024
Introduces support for request and repsonse headers on http proxy level

Fixes: projectcontour#5576
Signed-off-by: Devesh Kumar <vrshu112@gmail.com>
Copy link

The Contour project currently lacks enough contributors to adequately respond to all PRs.

This bot triages PRs according to the following rules:

  • After 14d of inactivity, lifecycle/stale is applied
  • After 30d of inactivity since lifecycle/stale was applied, the PR is closed

You can:

  • Ensure your PR is passing all CI checks. PRs that are fully green are more likely to be reviewed. If you are having trouble with CI checks, reach out to the #contour channel in the Kubernetes Slack workspace.
  • Mark this PR as fresh by commenting or pushing a commit
  • Close this PR
  • Offer to help out with triage

Please send feedback to the #contour channel in the Kubernetes Slack

@github-actions github-actions bot added lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. and removed lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. labels Apr 12, 2024
Copy link

github-actions bot commented May 1, 2024

The Contour project currently lacks enough contributors to adequately respond to all PRs.

This bot triages PRs according to the following rules:

  • After 14d of inactivity, lifecycle/stale is applied
  • After 30d of inactivity since lifecycle/stale was applied, the PR is closed

You can:

  • Ensure your PR is passing all CI checks. PRs that are fully green are more likely to be reviewed. If you are having trouble with CI checks, reach out to the #contour channel in the Kubernetes Slack workspace.
  • Mark this PR as fresh by commenting or pushing a commit
  • Close this PR
  • Offer to help out with triage

Please send feedback to the #contour channel in the Kubernetes Slack

@github-actions github-actions bot added the lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. label May 1, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. release-note/minor A minor change that needs about a paragraph of explanation in the release notes.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

How to set custom header for a HttpProxy for all routes
4 participants