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

Implement e2e test for internal encryption #13536

Closed

Conversation

KauzClay
Copy link
Contributor

@KauzClay KauzClay commented Dec 7, 2022

Fixes #

Proposed Changes

Release Note
N/A

@knative-prow
Copy link

knative-prow bot commented Dec 7, 2022

Skipping CI for Draft Pull Request.
If you want CI signal for your change, please convert it to an actual PR.
You can still manually trigger a test run with /test all

@knative-prow knative-prow bot added do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. area/test-and-release It flags unit/e2e/conformance/perf test issues for product features size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Dec 7, 2022
@codecov
Copy link

codecov bot commented Dec 7, 2022

Codecov Report

Base: 86.16% // Head: 86.24% // Increases project coverage by +0.07% 🎉

Coverage data is based on head (e5c1202) compared to base (f4792e4).
Patch has no changes to coverable lines.

❗ Current head e5c1202 differs from pull request most recent head 541d155. Consider uploading reports for the commit 541d155 to get more accurate results

Additional details and impacted files
@@            Coverage Diff             @@
##             main   #13536      +/-   ##
==========================================
+ Coverage   86.16%   86.24%   +0.07%     
==========================================
  Files         197      197              
  Lines       14790    14783       -7     
==========================================
+ Hits        12744    12749       +5     
+ Misses       1743     1733      -10     
+ Partials      303      301       -2     
Impacted Files Coverage Δ
pkg/autoscaler/statserver/server.go 75.91% <0.00%> (-1.46%) ⬇️
pkg/queue/sharedmain/main.go 0.61% <0.00%> (+<0.01%) ⬆️
pkg/reconciler/route/resources/ingress.go 95.00% <0.00%> (+0.19%) ⬆️
pkg/autoscaler/scaling/multiscaler.go 88.59% <0.00%> (+1.34%) ⬆️
pkg/reconciler/route/route.go 81.23% <0.00%> (+1.52%) ⬆️
pkg/reconciler/revision/resolve.go 85.00% <0.00%> (+7.50%) ⬆️

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

☔ View full report at Codecov.
📢 Do you have feedback about the report comment? Let us know in this issue.

@KauzClay KauzClay force-pushed the ck-internal-encryption-e2e-test branch from a22a75e to a7a1cdd Compare December 16, 2022 20:38
@KauzClay KauzClay changed the title Add e2e test for internal encryption Implement e2e test for internal encryption Dec 16, 2022
@knative-prow-robot knative-prow-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jan 12, 2023
@KauzClay KauzClay force-pushed the ck-internal-encryption-e2e-test branch from a7a1cdd to 1f6e0f2 Compare January 12, 2023 17:54
@knative-prow-robot knative-prow-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jan 12, 2023
@KauzClay
Copy link
Contributor Author

/test contour-internal-encryption

@mgencur
Copy link
Contributor

mgencur commented Jan 16, 2023

Hi! Looking at the "kourier" variant, the internal TLS is configured in e2e-common.sh here based on the ENABLE_TLS flag. This flag is enabled via github action here. This way all the E2E tests in Serving run with internal TLS enabled for kourier. The github action run can be seen above as e2e / test (v1.24.7, kourier-tls, e2e) (pull_request)
I'm wondering if you could use the same approach for net-kontour. Basically, you don't need to add any new test, just add one variant of github action with internal tls enabled. The test coverage will be higher as all tests will run with tls and it will be aligned with the already existing solution for kourier.

@KauzClay
Copy link
Contributor Author

hi @mgencur thanks for sharing that! I think that makes sense, I was worried my approach wasn't going to be able to test very much anyway.

I will look into it

@KauzClay KauzClay force-pushed the ck-internal-encryption-e2e-test branch from 052b346 to a1a7132 Compare January 24, 2023 18:36
@KauzClay
Copy link
Contributor Author

okay, I've tried out @mgencur 's suggestions. They're all failing currently, but I'm thinking that is because it is using Contour 1.23.x. The net-contour internal-encryption compatibility relies on some changes coming in Contour 1.24. That should be out soon.

In the mean-time, I will try bringing in the release candidate to my fork of net-contour and test this PR out with that.

@knative-prow-robot knative-prow-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jan 25, 2023
@knative-prow knative-prow bot added area/networking size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. and removed size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Jan 25, 2023
@KauzClay KauzClay force-pushed the ck-internal-encryption-e2e-test branch 2 times, most recently from b1afd86 to e5c1202 Compare January 25, 2023 15:45
@knative-prow-robot knative-prow-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jan 25, 2023
@KauzClay KauzClay force-pushed the ck-internal-encryption-e2e-test branch from e5c1202 to 7cf7589 Compare January 30, 2023 16:05
@knative-prow knative-prow bot removed the size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. label Jan 30, 2023
@knative-prow knative-prow bot added the size/L Denotes a PR that changes 100-499 lines, ignoring generated files. label Jan 30, 2023
@KauzClay KauzClay force-pushed the ck-internal-encryption-e2e-test branch from 7cf7589 to 8a1bed5 Compare January 30, 2023 16:11
@KauzClay
Copy link
Contributor Author

KauzClay commented Jan 30, 2023

As an update, looks like the only failing tests using the new Contour 1.24 are the DomainMapping api tests. I think I see what is going on there, so I started a new issue: #13659

EDIT: moved the issue to net-contour repo: knative-extensions/net-contour#862

@KauzClay
Copy link
Contributor Author

KauzClay commented Jan 31, 2023

Contour 1.24.0 is available, hopefully that can get pulled into net-contour soon.

UPDATE: I'm drafting a PR to test it out here

@KauzClay KauzClay force-pushed the ck-internal-encryption-e2e-test branch from 8a1bed5 to f172b1f Compare February 3, 2023 22:06
@knative-prow knative-prow bot added size/S Denotes a PR that changes 10-29 lines, ignoring generated files. and removed size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Feb 3, 2023
@KauzClay
Copy link
Contributor Author

KauzClay commented Feb 6, 2023

This PR (knative-extensions/net-contour#860) should make the failing contour-tls api tests pass here

* can achieve more test coverage by modifying workflow and reusing existing e2e tests
@KauzClay KauzClay force-pushed the ck-internal-encryption-e2e-test branch from f172b1f to 541d155 Compare February 22, 2023 21:58
@knative-prow
Copy link

knative-prow bot commented Feb 22, 2023

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: KauzClay
Once this PR has been reviewed and has the lgtm label, please assign zroubalik for approval. For more information see the Kubernetes Code Review Process.

The full list of commands accepted by this bot can be found 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

@KauzClay
Copy link
Contributor Author

KauzClay commented Feb 22, 2023

I just rebased with main, so serving should have my changes for knative-extensions/net-contour#860 in 51ed9ad. Hopefully the tests all pass now 🤞

@KauzClay
Copy link
Contributor Author

KauzClay commented Feb 23, 2023

looks like those changes didn't work, trying again with knative-extensions/net-contour#878

@github-actions
Copy link

This Pull Request is stale because it has been open for 90 days with
no activity. It will automatically close after 30 more days of
inactivity. Reopen with /reopen. Mark as fresh by adding the
comment /remove-lifecycle stale.

@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 25, 2023
@KauzClay
Copy link
Contributor Author

closing in favor of #14092

@KauzClay KauzClay closed this Jun 12, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/networking area/test-and-release It flags unit/e2e/conformance/perf test issues for product features do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. size/S Denotes a PR that changes 10-29 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants