feat!: Add tls support for ingress#6986
Conversation
Signed-off-by: Vishal Choudhary <sendtovishalchoudhary@gmail.com>
|
Skipping CI for Draft Pull Request. |
knative-prow
bot
added
do-not-merge/work-in-progress
Codecov ReportPatch coverage:
Additional details and impacted files@@ Coverage Diff @@
## main #6986 +/- ##
==========================================
- Coverage 78.59% 78.29% -0.30%
==========================================
Files 249 250 +1
Lines 13228 13291 +63
==========================================
+ Hits 10396 10406 +10
- Misses 2304 2353 +49
- Partials 528 532 +4
☔ View full report in Codecov by Sentry. |
Signed-off-by: Vishal Choudhary <sendtovishalchoudhary@gmail.com>
|
@pierDipi sorry for the delay on this, I have exams right now, I will be free by Friday evening |
Good luck! |
Signed-off-by: Vishal Choudhary <sendtovishalchoudhary@gmail.com>
|
/cc @pierDipi |
Signed-off-by: Vishal Choudhary <sendtovishalchoudhary@gmail.com>
vishal-chdhry
changed the title
knative-prow
bot
removed
the
do-not-merge/work-in-progress
|
@vishal-chdhry could you look into the failing tests? It looks like at least some of them are failing due to changes you made |
Signed-off-by: Vishal Choudhary <sendtovishalchoudhary@gmail.com>
creydr
left a comment
creydr
left a comment
There was a problem hiding this comment.
Hey @vishal-chdhry,
thanks for your PR. I left a few comments. In addition: could you fix the lint issues and add an e2e test to verify the TLS functionality?
Co-authored-by: Christoph Stäbler <cstabler@redhat.com>
Signed-off-by: Vishal Choudhary <sendtovishalchoudhary@gmail.com>
Signed-off-by: Vishal Choudhary <sendtovishalchoudhary@gmail.com>
Fix typo knative/community#1389 -pierDipi Produced by: knative-sandbox/knobots/actions/update-community Details: ``` ``` Signed-off-by: Knative Automation <automation@knative.team>
When running MT components [1] in mesh mode with Istio, we lose the ability to define fine grained policies since we don't know the resource namespace that originated such request, therefore, by having a `Kn-Namespace` header, in mesh mode, users case define fine-grained policies and isolate namespaces. [1] IMC, MTChannelBasedBroker, and PingSource Signed-off-by: Pierangelo Di Pilato <pierdipi@redhat.com>
For debugging [flaky tests](https://github.com/knative/eventing/issues?q=is%3Aissue+is%3Aopen+label%3Aauto%3Aflaky). As described here: https://github.com/knative-sandbox/reconciler-test#inspecting-zipkin-traces-for-failed-tests --------- Signed-off-by: Pierangelo Di Pilato <pierdipi@redhat.com>
As per title Signed-off-by: Pierangelo Di Pilato <pierdipi@redhat.com>
Manual update since there are a few braking chanages with knative-extensions/reconciler-test#526 first commit upgrade deps and vendor, the rest fixes the issues. --------- Signed-off-by: Pierangelo Di Pilato <pierdipi@redhat.com>
Fixes knative#7028 Looking at the logs, it seems like this test fails because it cannot bind to the required port: ``` === FAIL: pkg/adapter/mtping TestSendEventsTLS/Valid_CA_certs (unknown) panic: listen tcp :8334: bind: address already in use goroutine 226 [running]: knative.dev/eventing/pkg/eventingtls/eventingtlstesting.StartServer.func1() /home/prow/go/src/knative.dev/eventing/pkg/eventingtls/eventingtlstesting/eventingtlstesting.go:77 +0x88 created by knative.dev/eventing/pkg/eventingtls/eventingtlstesting.StartServer /home/prow/go/src/knative.dev/eventing/pkg/eventingtls/eventingtlstesting/eventingtlstesting.go:74 +0x7ea ``` <!-- Please include the 'why' behind your changes if no issue exists --> ## Proposed Changes <!-- Please categorize your changes: - 🎁 Add new feature - 🐛 Fix bug - 🧹 Update or clean up current behavior - 🗑️ Remove feature or internal logic --> - Update the test to bind to a free address, using port 0 (https://www.lifewire.com/port-0-in-tcp-and-udp-818145) ### Pre-review Checklist <!-- If these boxes are not checked, you will be asked to complete these requirements or explain why they do not apply to your PR. --> - [ ] **At least 80% unit test coverage** - [ ] **E2E tests** for any new behavior - [ ] **Docs PR** for any user-facing impact - [ ] **Spec PR** for any new API feature - [ ] **Conformance test** for any change to the spec --------- Signed-off-by: Calum Murray <cmurray@redhat.com>
Signed-off-by: Vishal Choudhary <sendtovishalchoudhary@gmail.com>
knative-prow
bot
added
area/test-and-release
knative-prow-robot
removed
the
needs-rebase
knative-prow
bot
added
size/L
|
@pierDipi Done! |
| } | ||
|
|
||
| return h.send(ctx, headers, event, channelAddress) | ||
| return h.send(ctx, headers, event, *channelAddress) |
There was a problem hiding this comment.
@vishal-chdhry could you take a look at the failing unit test?
=== FAIL: pkg/broker/ingress TestHandler_ServeHTTP/no_broker_annotations (0.00s)
=== FAIL: pkg/broker/ingress TestHandler_ServeHTTP (0.08s) panic: runtime error: invalid memory address or nil pointer dereference [recovered]
panic: runtime error: invalid memory address or nil pointer dereference
[signal SIGSEGV: segmentation violation code=0x1 addr=0x0 pc=0x273574b]
goroutine 77 [running]:
testing.tRunner.func1.2({0x2959a80, 0x3f81f90})
/root/.gvm/gos/go1.20.4/src/testing/testing.go:1526 +0x372
testing.tRunner.func1()
/root/.gvm/gos/go1.20.4/src/testing/testing.go:1529 +0x650
panic({0x2959a80, 0x3f81f90})
/root/.gvm/gos/go1.20.4/src/runtime/panic.go:890 +0x263
knative.dev/eventing/pkg/broker/ingress.(*Handler).receive(0xc00058e8c0, {0x2fdf750, 0xc0006adb90}, 0xc000228e82?, 0xc00058e900, {0xc000154876, 0x2}, {0xc000154879, 0x4})
/home/prow/go/src/knative.dev/eventing/pkg/broker/ingress/ingress_handler.go:279 +0x8eb
It seems like the problem is on this line, and is probably from deferencing a nil channelAddress when there are no broker annotations. WDYT?
There was a problem hiding this comment.
I think this is because of the fact that we are not guessing the channel address anymore. I have updated the function to return an status code 400 with noDuration in this case: 0faea53 (#6986)
After guess channel address is removed, there is a case where no broker is found at all, updated test cases to account for that Signed-off-by: Vishal Choudhary <sendtovishalchoudhary@gmail.com>
| func getServerTLSConfig(ctx context.Context) (*tls.Config, error) { | ||
| secret := types.NamespacedName{ | ||
| Namespace: "knative-eventing", | ||
| Name: "mt-broker-ingress-server-tls", |
There was a problem hiding this comment.
Nit: can we introduce a constant somewhere for this secret name?
There was a problem hiding this comment.
@Cali0707 There are other secrets as well, such as the one for the filter.
Should we add constants for them in eventing-tls?
Maybe we should make a separate PR as I would like this PR to only contain ingress changes
There was a problem hiding this comment.
Let's record an issue for this improvement
vishal-chdhry
changed the title
| func getServerTLSConfig(ctx context.Context) (*tls.Config, error) { | ||
| secret := types.NamespacedName{ | ||
| Namespace: "knative-eventing", | ||
| Name: "mt-broker-ingress-server-tls", |
There was a problem hiding this comment.
Let's record an issue for this improvement
|
Thanks! |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: pierDipi, Vishal-Chdhry The full list of commands accepted by this bot can be found here. The pull request process is described here DetailsNeeds approval from an approver in each of these files:
Approvers can indicate their approval by writing |
Follow up on #6986 (comment): Moved the tls secret names for mt-broker-ingress & mt-broker-filter into constants
11 participants
Fixes #6876
Proposed Changes
Pre-review Checklist
Release Note
Docs