-
Notifications
You must be signed in to change notification settings - Fork 681
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
[api-gateway]: Support http(s) as AppProtocol in Kubernetes svc #6616
[api-gateway]: Support http(s) as AppProtocol in Kubernetes svc #6616
Conversation
Hi @Krast76! 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 |
…ernetes' service Fix projectcontour#6560 Signed-off-by: Ludovic Logiou <[email protected]>
23dd9c2
to
ccb97c6
Compare
Signed-off-by: Ludovic Logiou <[email protected]>
Thanks @Krast76! Could you check also the linter nags here https://github.com/projectcontour/contour/pull/6616/files and add a changelog entry in the PR, the filename should be |
Signed-off-by: Ludovic Logiou <[email protected]>
Signed-off-by: Ludovic Logiou <[email protected]>
f81fb6d
to
190b167
Compare
Done :) |
Signed-off-by: Ludovic Logiou <[email protected]>
@tsaarni There is something else I should do to make the PR merged ? The linter still failed, but I don't really understand why. 🤔 |
Almost impossible to see in the error, but it was the alignment that it complains :-) Run |
It could be good to add a unit test, something like this diff --git a/internal/dag/accessors_test.go b/internal/dag/accessors_test.go
index bc3e2b86..8c3bc553 100644
--- a/internal/dag/accessors_test.go
+++ b/internal/dag/accessors_test.go
@@ -127,6 +127,18 @@ func TestBuilderLookupService(t *testing.T) {
AppProtocol: ptr.To("kubernetes.io/wss"),
Port: 8444,
},
+ {
+ Name: "iana-https",
+ Protocol: "TCP",
+ AppProtocol: ptr.To("https"),
+ Port: 8445,
+ },
+ {
+ Name: "iana-http",
+ Protocol: "TCP",
+ AppProtocol: ptr.To("http"),
+ Port: 8446,
+ },
},
},
}
@@ -212,12 +224,21 @@ func TestBuilderLookupService(t *testing.T) {
port: 8443,
want: appProtcolService(appProtoService, "h2c"),
},
-
"lookup service by port number with unsupported k8s app protocol: wss": {
NamespacedName: types.NamespacedName{Name: appProtoService.Name, Namespace: appProtoService.Namespace},
port: 8444,
want: appProtcolService(appProtoService, "", 1),
},
+ "lookup service by port number with supported IANA app protocol: https": {
+ NamespacedName: types.NamespacedName{Name: appProtoService.Name, Namespace: appProtoService.Namespace},
+ port: 8445,
+ want: appProtcolService(appProtoService, "tls", 2),
+ },
+ "lookup service by port number with supported IANA app protocol: http": {
+ NamespacedName: types.NamespacedName{Name: appProtoService.Name, Namespace: appProtoService.Namespace},
+ port: 8446,
+ want: appProtcolService(appProtoService, "", 3),
+ },
}
for name, tc := range tests { |
Signed-off-by: Ludovic Logiou <[email protected]>
I should have guessed that 😅 The linter must be ok (for real) this time, and I have added the unit tests you have suggested, thanks ! :) |
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #6616 +/- ##
=======================================
Coverage 81.76% 81.76%
=======================================
Files 133 133
Lines 15942 15944 +2
=======================================
+ Hits 13035 13037 +2
Misses 2614 2614
Partials 293 293
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank You @Krast76!
* add changelog Signed-off-by: gang.liu <[email protected]> * build(deps): bump actions/upload-artifact in the artifact-actions group (projectcontour#6608) Bumps the artifact-actions group with 1 update: [actions/upload-artifact](https://github.com/actions/upload-artifact). Updates `actions/upload-artifact` from 4.3.5 to 4.3.6 - [Release notes](https://github.com/actions/upload-artifact/releases) - [Commits](actions/upload-artifact@89ef406...834a144) --- updated-dependencies: - dependency-name: actions/upload-artifact dependency-type: direct:production update-type: version-update:semver-patch dependency-group: artifact-actions ... Signed-off-by: dependabot[bot] <[email protected]> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com> * build(deps): bump github/codeql-action from 3.25.15 to 3.26.0 (projectcontour#6609) Bumps [github/codeql-action](https://github.com/github/codeql-action) from 3.25.15 to 3.26.0. - [Release notes](https://github.com/github/codeql-action/releases) - [Changelog](https://github.com/github/codeql-action/blob/main/CHANGELOG.md) - [Commits](github/codeql-action@afb54ba...eb055d7) --- updated-dependencies: - dependency-name: github/codeql-action dependency-type: direct:production update-type: version-update:semver-minor ... Signed-off-by: dependabot[bot] <[email protected]> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com> * build(deps): bump github.com/onsi/ginkgo/v2 from 2.19.1 to 2.20.0 (projectcontour#6607) Bumps [github.com/onsi/ginkgo/v2](https://github.com/onsi/ginkgo) from 2.19.1 to 2.20.0. - [Release notes](https://github.com/onsi/ginkgo/releases) - [Changelog](https://github.com/onsi/ginkgo/blob/master/CHANGELOG.md) - [Commits](onsi/ginkgo@v2.19.1...v2.20.0) --- updated-dependencies: - dependency-name: github.com/onsi/ginkgo/v2 dependency-type: direct:production update-type: version-update:semver-minor ... Signed-off-by: dependabot[bot] <[email protected]> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com> * docs: Clarify how XFCC headers are handled (projectcontour#6586) Since XFCC headers contain authentication information, it's important to know precisely how Contour (ie Envoy) handle existing XFCC headers from clients - ie, are they blocked, or appended to, and in what circumstances are they blocked? Getting this wrong could allow serious vulnerabilities such as spoofing client certs. This documents Contours behaviour, so that users can know exactly how they are required to handle that header without needing to dive into the Contour source code. My understanding from reading the source code: https://github.com/gautierdelorme/contour/blob/main/internal/envoy/v3/listener.go#L483 as well as the Envoy documentation: https://www.envoyproxy.io/docs/envoy/latest/api-v3/extensions/filters/network/http_connection_manager/v3/http_connection_manager.proto#envoy-v3-api-enum-extensions-filters-network-http-connection-manager-v3-httpconnectionmanager-forwardclientcertdetails is that when forwarding client certificate details is not configured in Contour, Contour leaves `ForwardClientCertDetails` in Envoy unset, which means it defaults to `SANITIZE`, which means incoming headers from clients are blocked. Meanwhile, when forwarding client certificate details is configured in Contour, Contour sets `ForwardClientCertDetails` to `SANITIZE_SET` in Envoy, which means incoming XFCC headers are blocked, and if an incoming cert is present, a new XFCC header is added. Signed-off-by: James Roper <[email protected]> * build(deps): bump dario.cat/mergo from 1.0.0 to 1.0.1 (projectcontour#6627) Bumps [dario.cat/mergo](https://github.com/imdario/mergo) from 1.0.0 to 1.0.1. - [Release notes](https://github.com/imdario/mergo/releases) - [Commits](darccio/mergo@v1.0.0...v1.0.1) --- updated-dependencies: - dependency-name: dario.cat/mergo dependency-type: direct:production update-type: version-update:semver-patch ... Signed-off-by: dependabot[bot] <[email protected]> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com> * build(deps): bump github/codeql-action from 3.26.0 to 3.26.2 (projectcontour#6622) Bumps [github/codeql-action](https://github.com/github/codeql-action) from 3.26.0 to 3.26.2. - [Release notes](https://github.com/github/codeql-action/releases) - [Changelog](https://github.com/github/codeql-action/blob/main/CHANGELOG.md) - [Commits](github/codeql-action@eb055d7...429e197) --- updated-dependencies: - dependency-name: github/codeql-action dependency-type: direct:production update-type: version-update:semver-patch ... Signed-off-by: dependabot[bot] <[email protected]> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com> * build(deps): bump github.com/prometheus/client_golang (projectcontour#6626) Bumps [github.com/prometheus/client_golang](https://github.com/prometheus/client_golang) from 1.19.1 to 1.20.0. - [Release notes](https://github.com/prometheus/client_golang/releases) - [Changelog](https://github.com/prometheus/client_golang/blob/main/CHANGELOG.md) - [Commits](prometheus/client_golang@v1.19.1...v1.20.0) --- updated-dependencies: - dependency-name: github.com/prometheus/client_golang dependency-type: direct:production update-type: version-update:semver-minor ... Signed-off-by: dependabot[bot] <[email protected]> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com> * build(deps): bump github.com/envoyproxy/go-control-plane (projectcontour#6625) Bumps [github.com/envoyproxy/go-control-plane](https://github.com/envoyproxy/go-control-plane) from 0.12.1-0.20240111020705-5401a878d8bb to 0.13.0. - [Release notes](https://github.com/envoyproxy/go-control-plane/releases) - [Changelog](https://github.com/envoyproxy/go-control-plane/blob/main/CHANGELOG.md) - [Commits](https://github.com/envoyproxy/go-control-plane/commits/v0.13.0) --- updated-dependencies: - dependency-name: github.com/envoyproxy/go-control-plane dependency-type: direct:production update-type: version-update:semver-minor ... Signed-off-by: dependabot[bot] <[email protected]> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com> * docs: Update README.md to be more helpful (projectcontour#6585) The docs/README.md made no sense. Anyone reading it in GitHub clearly wants to contribute to the documentation, that's why they're in the source code of Contour, why else would they have found their way to the source repository? So, it should point to where the documentation lives in the git repository, not to the website where it's served. Signed-off-by: James Roper <[email protected]> Co-authored-by: Steve Kriss <[email protected]> * [api-gateway]: Support http(s) as AppProtocol in Kubernetes svc (projectcontour#6616) * [api-gateway]: Support http, https and www-http as AppProtocol in kubernetes' service Fix projectcontour#6560 Signed-off-by: Ludovic Logiou <[email protected]> * Remove legacy www-http Signed-off-by: Ludovic Logiou <[email protected]> * Fix undefined vars Signed-off-by: Ludovic Logiou <[email protected]> * Add changelog Signed-off-by: Ludovic Logiou <[email protected]> * Fix issues found by the linter Signed-off-by: Ludovic Logiou <[email protected]> * Fix format and add unit tests Signed-off-by: Ludovic Logiou <[email protected]> --------- Signed-off-by: Ludovic Logiou <[email protected]> * build(deps): bump codespell-project/actions-codespell from 2.0 to 2.1 (projectcontour#6635) Bumps [codespell-project/actions-codespell](https://github.com/codespell-project/actions-codespell) from 2.0 to 2.1. - [Release notes](https://github.com/codespell-project/actions-codespell/releases) - [Commits](codespell-project/actions-codespell@94259cd...406322e) --- updated-dependencies: - dependency-name: codespell-project/actions-codespell dependency-type: direct:production update-type: version-update:semver-minor ... Signed-off-by: dependabot[bot] <[email protected]> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com> * build(deps): bump github.com/prometheus/client_golang (projectcontour#6640) Bumps [github.com/prometheus/client_golang](https://github.com/prometheus/client_golang) from 1.20.0 to 1.20.2. - [Release notes](https://github.com/prometheus/client_golang/releases) - [Changelog](https://github.com/prometheus/client_golang/blob/main/CHANGELOG.md) - [Commits](prometheus/client_golang@v1.20.0...v1.20.2) --- updated-dependencies: - dependency-name: github.com/prometheus/client_golang dependency-type: direct:production update-type: version-update:semver-patch ... Signed-off-by: dependabot[bot] <[email protected]> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com> * build(deps): bump github.com/onsi/ginkgo/v2 from 2.20.0 to 2.20.1 (projectcontour#6639) Bumps [github.com/onsi/ginkgo/v2](https://github.com/onsi/ginkgo) from 2.20.0 to 2.20.1. - [Release notes](https://github.com/onsi/ginkgo/releases) - [Changelog](https://github.com/onsi/ginkgo/blob/master/CHANGELOG.md) - [Commits](onsi/ginkgo@v2.20.0...v2.20.1) --- updated-dependencies: - dependency-name: github.com/onsi/ginkgo/v2 dependency-type: direct:production update-type: version-update:semver-patch ... Signed-off-by: dependabot[bot] <[email protected]> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com> * build(deps): bump github.com/vektra/mockery/v2 from 2.44.1 to 2.45.0 (projectcontour#6638) Bumps [github.com/vektra/mockery/v2](https://github.com/vektra/mockery) from 2.44.1 to 2.45.0. - [Release notes](https://github.com/vektra/mockery/releases) - [Changelog](https://github.com/vektra/mockery/blob/master/docs/changelog.md) - [Commits](vektra/mockery@v2.44.1...v2.45.0) --- updated-dependencies: - dependency-name: github.com/vektra/mockery/v2 dependency-type: direct:production update-type: version-update:semver-minor ... Signed-off-by: dependabot[bot] <[email protected]> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com> * Bump kind and kubectl tools (projectcontour#6642) kind: 0.24.0 kubectl: 1.31.0 Signed-off-by: Sunjay Bhatia <[email protected]> * fix lb address Signed-off-by: gang.liu <[email protected]> * fix ut Signed-off-by: gang.liu <[email protected]> * revert wrong file Signed-off-by: gang.liu <[email protected]> --------- Signed-off-by: gang.liu <[email protected]> Signed-off-by: dependabot[bot] <[email protected]> Signed-off-by: James Roper <[email protected]> Signed-off-by: Ludovic Logiou <[email protected]> Signed-off-by: Sunjay Bhatia <[email protected]> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com> Co-authored-by: James Roper <[email protected]> Co-authored-by: Steve Kriss <[email protected]> Co-authored-by: Ludovic Logiou <[email protected]> Co-authored-by: Sunjay Bhatia <[email protected]>
…ectcontour#6616) * [api-gateway]: Support http, https and www-http as AppProtocol in kubernetes' service Fix projectcontour#6560 Signed-off-by: Ludovic Logiou <[email protected]> * Remove legacy www-http Signed-off-by: Ludovic Logiou <[email protected]> * Fix undefined vars Signed-off-by: Ludovic Logiou <[email protected]> * Add changelog Signed-off-by: Ludovic Logiou <[email protected]> * Fix issues found by the linter Signed-off-by: Ludovic Logiou <[email protected]> * Fix format and add unit tests Signed-off-by: Ludovic Logiou <[email protected]> --------- Signed-off-by: Ludovic Logiou <[email protected]> Signed-off-by: Saman Mahdanian <[email protected]>
When contour is used as api-gateway in kubernetes clusters, appProtocol set to http or https doesn't work.
This PR, if applied, allow the following protocol (as defined by IANA) :
That doesn't allow all the IANA services, but that'll support a common case
Fix #6560