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

All Hops Encrypted: alpha Kourier support for encrypted backends #761

Merged
merged 7 commits into from
Mar 26, 2022
Merged

All Hops Encrypted: alpha Kourier support for encrypted backends #761

merged 7 commits into from
Mar 26, 2022

Conversation

nak3
Copy link
Contributor

@nak3 nak3 commented Jan 26, 2022 •
edited
Loading

This patch adds the TLS support between Kourier and Activator.

TODO:

Fix #750

@knative-prow-robot knative-prow-robot added do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. approved Indicates a PR has been approved by an approver from all required OWNERS files. size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. labels Jan 26, 2022
@codecov
Copy link

codecov bot commented Jan 26, 2022 •
edited
Loading

Codecov Report

Merging #761 (5ef250e) into main (647c9ea) will increase coverage by 0.57%.
The diff coverage is 88.88%.

@@            Coverage Diff             @@
##             main     #761      +/-   ##
==========================================
+ Coverage   82.49%   83.07%   +0.57%     
==========================================
  Files          18       18              
  Lines         777      827      +50     
==========================================
+ Hits          641      687      +46     
  Misses         99       99              
- Partials       37       41       +4
Impacted Files Coverage Δ
pkg/generator/ingress_translator.go 86.80% <86.36%> (-0.22%) ⬇️
pkg/envoy/api/cluster.go 100.00% <100.00%> (ø)
pkg/reconciler/ingress/config/store.go 91.66% <100.00%> (+13.88%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 647c9ea...5ef250e. Read the comment docs.

This was referenced Jan 26, 2022
Merged
Merged
knative-prow-robot pushed a commit to knative/networking that referenced this pull request Feb 8, 2022
@nak3
Add activator-ca and activator-name keys in config-network (#608)
c9e7082 
* Add `activator-ca` and `activator-name` keys in `config-network`

This pach adds `activator-ca` and `activator-name` keys in `config-network`.

Part of knative-extensions/net-kourier#750
knative-extensions/net-kourier#761 demonstrates how it works.

* Rename activator-name to activator-san
@knative-prow-robot knative-prow-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Feb 9, 2022
knative-prow-robot pushed a commit to knative/networking that referenced this pull request Mar 2, 2022
@nak3
Support TLS server for each test image (#609)
e2b9815 
This pach supports TLS server on each test image.
It needs to verify if Ingress surely connects to the backend with TLS.

The usage is as follows:

1. Create server certificate with the name `server-certs` in `serving-tests` namespace.

  ```shell
  $ kubectl create -n serving-tests secret tls server-certs \
      --key=tls.key --cert=tls.crt
  ```

1. Set env variable `UPSTREAM_TLS_CERT=server-certs` and run the tests.

  ```shell
  $ export UPSTREAM_TLS_CERT=server-certs
  $ go test -race -count=1 -tags=e2e ./test/conformance/ -run "TestIngressConformance/basic"
  ```

1. The backend test server starts running with TLS.

  ```shell
  $ kubectl -n serving-tests logs ingress-conformance-basics-tfpnykaw
  2022/01/27 11:54:14 Server starting on port with TLS 8047
    ...
  ```

Part of knative-extensions/net-kourier#750
knative-extensions/net-kourier#761 demonstrates how it works.
@knative-prow-robot knative-prow-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Mar 2, 2022
@nak3 nak3 added this to the v1.4.0 milestone Mar 3, 2022
@knative-prow-robot knative-prow-robot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. and removed size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Mar 15, 2022
@nak3 nak3 changed the title [WIP] All Hops Encrypted: alpha Kourier support for encrypted backends All Hops Encrypted: alpha Kourier support for encrypted backends Mar 15, 2022
@knative-prow-robot knative-prow-robot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Mar 15, 2022
@nak3 nak3 mentioned this pull request Mar 15, 2022
Closed
@nak3
Copy link
Contributor Author

nak3 commented Mar 15, 2022

This PR may be too big. I will separate it.

@nak3 nak3 added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Mar 15, 2022
@nak3
Copy link
Contributor Author

nak3 commented Mar 16, 2022

I started to send a separated PR as #795
(Of course, it would be great if we get a review for this PR though 😉 )

skonto
skonto reviewed Mar 17, 2022
View reviewed changes
pkg/envoy/api/cluster.go Outdated Show resolved Hide resolved
skonto
skonto reviewed Mar 17, 2022
View reviewed changes
pkg/envoy/api/cluster.go Outdated Show resolved Hide resolved
@nak3
Copy link
Contributor Author

nak3 commented Mar 18, 2022

/hold cancel

@knative-prow-robot knative-prow-robot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Mar 18, 2022
@skonto
Copy link
Contributor

skonto commented Mar 18, 2022 •
edited
Loading

@nak3 Shouldn't the autogenerated file be ignored because of https://github.com/knative-sandbox/net-kourier/blob/8bbcbab80ee42a027c28073dcda67142f968af4c/.codecov.yaml#L15 ?

I think we need to add another rule:

- "**/zz_generated.deepcopy.go" # Ignore generated files.

I suspect . is a special char but dont see much details in https://docs.codecov.com/docs/ignoring-paths, however noticed this fix here.
Can we try it here since this PR breaks the coverage? I suspect also that coverage job looks at the changed files (other generated files with the same pattern are not considered eg. pkg/config/zz_generated.deepcopy.go in the report although they have no coverage)?

@nak3
Copy link
Contributor Author

nak3 commented Mar 18, 2022

pull-knative-sandbox-net-kourier-go-coverage is an optional test so I think we can try to fix it by the follow up PR.

@skonto
Copy link
Contributor

skonto commented Mar 18, 2022

Ok wfm.

@skonto
Copy link
Contributor

skonto commented Mar 21, 2022 •
edited
Loading

LGTM, @rhuss may want to take a look too. I will create the PR to fix the coverage issue.

@skonto skonto mentioned this pull request Mar 22, 2022
Closed
@knative-metrics-robot
Copy link

The following is the coverage report on the affected files.
Say /test pull-knative-sandbox-net-kourier-go-coverage to re-run this coverage report

File Old Coverage New Coverage Delta
pkg/generator/ingress_translator.go 90.9% 89.8% -1.1
pkg/reconciler/ingress/config/store.go 66.7% 90.0% 23.3
pkg/reconciler/ingress/config/zz_generated.deepcopy.go Do not exist 0.0%

@knative-prow-robot
Copy link

@nak3: The following test failed, say /retest to rerun all failed tests or /retest-required to rerun all mandatory failed tests:

Test name Commit Details Required Rerun command
pull-knative-sandbox-net-kourier-go-coverage 5ef250e link false /test pull-knative-sandbox-net-kourier-go-coverage

Full PR test history. Your PR dashboard.

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. I understand the commands that are listed here.

@nak3 nak3 mentioned this pull request Mar 23, 2022
Merged
7 tasks
@skonto skonto mentioned this pull request Mar 24, 2022
Merged
rhuss
rhuss reviewed Mar 24, 2022
View reviewed changes
Copy link
Contributor

@rhuss rhuss left a comment

Choose a reason for hiding this comment

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

/approve
/lgtm

@knative-prow-robot knative-prow-robot added the lgtm Indicates that a PR is ready to be merged. label Mar 24, 2022
@knative-prow-robot
Copy link

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: nak3, rhuss

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

@nak3
Copy link
Contributor Author

nak3 commented Mar 26, 2022

/retest

@knative-prow-robot knative-prow-robot merged commit 0ee8198 into knative-extensions:main Mar 26, 2022
@nak3 nak3 mentioned this pull request Mar 31, 2022
Merged
@nak3 nak3 deleted the add-tls-2 branch April 15, 2022 10:51
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@skonto skonto skonto left review comments

@rhuss rhuss rhuss left review comments

Assignees

@rhuss rhuss

Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. lgtm Indicates that a PR is ready to be merged. size/XL Denotes a PR that changes 500-999 lines, ignoring generated files.
Projects
None yet
Milestone
v1.4.0
Development

Successfully merging this pull request may close these issues.

All Hops Encrypted: alpha Kourier support for encrypted backends
5 participants
@nak3 @skonto @knative-metrics-robot @knative-prow-robot @rhuss