Add support for `system-internal-tls` in net-istio #1085

ReToCode · 2023-03-28T11:52:04Z

Changes

Adds support for system-internal-tls in net-istio
Adds a build matrix for conformance and e2e tests with upstream TLS enabled
Verified on Serving: Test istio with system-internal-tls enabled knative/serving#14494

Things to discuss

config/700-istio-secret.yaml is necessary because the CA secrets needs to be in istio-system namespace for istio to pick it up. It's currently the only way to get the CA there. This is why https://docs.google.com/document/d/1YdcdBVg_zpT4WSNRsWlihut5JuLddOTIggFvLni3A28/edit?disco=AAAAtaDADjo proposes to add the CA to KIngress
Handling of DomainMappings is a bit hacky. It is adapted based on net-kourier. If we do Provide HTTPS for cluster.local domains knative/serving#13472 this handling can be dropped (here and in net-kourier and net-contour)
Handling/detection of http2 is currently based on service naming, which is not ideal. Thus https://docs.google.com/document/d/1YdcdBVg_zpT4WSNRsWlihut5JuLddOTIggFvLni3A28/edit?disco=AAAAtaDADjo also proposes to add the upstream protocol to the KIngress resource to avoid this in the future.

Release Note
No release notes yet, as this is still an internal/alpha feature.

Docs
No docs yet, as this still an internal/alpha feature.

/kind enhancement
Fixes #1063

ReToCode · 2023-03-28T12:20:53Z

/assign @nak3, @skonto, @evankanderson for review

/cc @dprotaso, @KauzClay for inputs/opinions on the discussion points (and - of course - also review if you'd like)

ReToCode · 2023-03-28T12:21:04Z

/hold I'm going to look into the test failures.

evankanderson · 2023-03-28T13:49:21Z

I like the suggestion to do https://github.com/knative/serving/issue/13472, particularly since we've seen it show up three times and within-cluster TLS is not an unreasonable desire.

(Non-blocking, will review the rest soon.)

evankanderson · 2023-03-28T13:51:46Z

One other thought: with websockets being defined for http2, I wonder whether we should simply run http2 to the queue-proxy and do the down-conversion there.

Downside: we may get scale-from-zero wakeups for requests the app won't actually be able to handle.

Upside: people can stop needing to worry about naming their port.

evankanderson

Initial review; this was less painful than I anticipated.

.github/workflows/kind-e2e.yaml

pkg/reconciler/ingress/ingress.go

pkg/reconciler/ingress/resources/destinationrule.go

ReToCode · 2023-03-29T06:48:22Z

I like the suggestion to do https://github.com/knative/serving/issue/13472, particularly since we've seen it show up three times and within-cluster TLS is not an unreasonable desire.

I synced with @KauzClay. Well come up with some GH-issues to track tasks out of the Findings document shortly to start working on that. But we'd need to agree on the contract and KIngress changes first.

One other thought: with websockets being defined for http2, I wonder whether we should simply run http2 to the queue-proxy and do the down-conversion there.

I think it would be worth to look into this. Should we track/discuss this in a new issue?

Initial review; this was less painful than I anticipated.

That's a hopefully a good thing? ;)

ReToCode · 2023-03-29T07:43:50Z

/test integration-tests

/test latest-mesh

nak3

I guess we should add the same #114 for DestinationRules in cmd/controller/main.go?

pkg/reconciler/ingress/resources/destinationrule.go

ReToCode · 2023-03-31T06:24:27Z

I guess we should add the same #114 for DestinationRules in cmd/controller/main.go?

@nak3 any idea why we are configuring v1beta1 here but are using v1alpha3 everywhere else?

nak3 · 2023-03-31T06:41:36Z

I don't know the reason. As far as I researched the commit history (you might already know but...)

Move from Istio API v1alpha3 to v1beta1. #147 bumped to v1beta1
Rollback #147 #203 reverted to v1alpha3 but missed cmd/controller/main.go (not sure if it is accident or intention though)

The revert is waiting for knative/serving#8896 but knative/serving#8896 should be fixed most probably so we should bump the API to v1beta1. I opened #1087

ReToCode · 2023-03-31T07:17:53Z

/test latest-mesh

ReToCode · 2023-04-03T06:28:13Z

/unhold

nak3 · 2023-04-04T01:26:16Z

Overall looks good to me.

With said, Istio has a large variety of network configuration so I am worried that adding a non-edge network feature conflicts with their features 😥 For example, does this internal-encryption in net-istio assume that there is no sidecar at all? Or it can work with the partial mesh (moreover using PERMISSIVE mode but it means HTTPS over mTLS)?...etc
This is still alpha feature so we can move forward, though.

nak3 · 2023-04-04T01:26:28Z

@evankanderson Would you like to have another review?

ReToCode · 2023-04-11T06:00:35Z

For example, does this internal-encryption in net-istio assume that there is no sidecar at all?

My personal take would be, if you use service-mesh, you do not need the Knative internal-encryption feature. I also would not invest in making this compatible with istios internal mesh certificates.

codecov · 2023-04-18T11:38:47Z

Codecov Report

Merging #1085 (28dd4c5) into main (a271e0b) will increase coverage by 0.24%.
Report is 5 commits behind head on main.
The diff coverage is 87.35%.

@@            Coverage Diff             @@
##             main    #1085      +/-   ##
==========================================
+ Coverage   81.42%   81.66%   +0.24%     
==========================================
  Files          18       19       +1     
  Lines        1599     1680      +81     
==========================================
+ Hits         1302     1372      +70     
- Misses        213      220       +7     
- Partials       84       88       +4

Files	Coverage Δ
pkg/reconciler/ingress/controller.go	`92.30% <100.00%> (+0.48%)`	⬆️
...kg/reconciler/ingress/resources/destinationrule.go	`100.00% <100.00%> (ø)`
pkg/reconciler/ingress/ingress.go	`69.49% <69.44%> (-0.01%)`	⬇️

ReToCode · 2023-04-18T12:20:21Z

/test latest-mesh

ReToCode · 2023-04-18T12:53:41Z

Rebased and istio version bumped.

@evankanderson any thoughts about #1085 (comment) and #1085 (comment)? For the latter, what is the plan to move/migrate stuff from control-protocol to networking? Can we maybe start with migrating the constants instead of adding another dependency on control-protocol?

github-actions · 2023-07-29T01:31:09Z

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.

ReToCode · 2023-07-31T05:11:51Z

/remove-lifecycle stale

Co-authored-by: Kenjiro Nakayama <nakayamakenjiro@gmail.com>

ReToCode · 2023-10-11T08:25:12Z

/test latest

ReToCode · 2023-10-11T08:27:57Z

/retest

nak3 · 2023-10-12T01:05:57Z

/lgtm
/approve
/hold please feel free to unhold if there is no other reviewers.

knative-prow · 2023-10-12T01:06:04Z

[APPROVALNOTIFIER] This PR is APPROVED

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

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:

~~OWNERS~~ [ReToCode,nak3]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

ReToCode · 2023-10-12T06:24:54Z

@dprotaso do you want to take a look as well?

ReToCode · 2023-10-17T06:39:41Z

/unhold

knative-prow bot added kind/enhancement size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. labels Mar 28, 2023

knative-prow bot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Mar 28, 2023

evankanderson reviewed Mar 28, 2023

View reviewed changes

nak3 reviewed Mar 29, 2023

View reviewed changes

pkg/reconciler/ingress/resources/destinationrule.go Show resolved Hide resolved

knative-prow bot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Apr 3, 2023

ReToCode force-pushed the istio-internal-encryption branch from 442d207 to 35ebcb8 Compare April 18, 2023 11:33

knative-prow-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Apr 29, 2023

github-actions bot added the lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. label Jul 29, 2023

knative-prow bot removed the lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. label Jul 31, 2023

ReToCode force-pushed the istio-internal-encryption branch from 13c76e4 to a5c4941 Compare October 9, 2023 09:39

knative-prow-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Oct 9, 2023

knative-prow bot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Oct 11, 2023

ReToCode and others added 15 commits October 11, 2023 09:04

Add support for internal-encryption in net-istio

4937efb

fixes

f8d5c87

Enable internal-encryption in config-network

4a6571e

DEBUG tests

3c62ecf

Change CI step order and add documentation

cf65d81

review improvements

e0c82d7

propagate annotations and labels from ingress

89b4284

Use istio api and client version v1beta1

59a2b85

Fix lint and shellcheck warnings

7a86834

Fix more lint warnings

0540bac

Update to aligned networking flags

1285b40

Minor fixes

461959e

Run encryption tests only in no-mesh mode

373effd

Update .github/workflows/kind-e2e.yaml

896a10e

Co-authored-by: Kenjiro Nakayama <nakayamakenjiro@gmail.com>

Update pkg/reconciler/ingress/ingress.go

13fcbd5

Co-authored-by: Kenjiro Nakayama <nakayamakenjiro@gmail.com>

ReToCode force-pushed the istio-internal-encryption branch from 51e94ae to 9222267 Compare October 11, 2023 07:06

Use config flag instead of constant

28dd4c5

ReToCode force-pushed the istio-internal-encryption branch from 9222267 to 28dd4c5 Compare October 11, 2023 07:25

knative-prow bot added do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. lgtm Indicates that a PR is ready to be merged. labels Oct 12, 2023

knative-prow bot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Oct 17, 2023

knative-prow bot merged commit d06426a into knative-extensions:main Oct 17, 2023
48 checks passed

ReToCode mentioned this pull request Nov 30, 2023

Update net-istio to use KnativeCertificate instead of control-protocol secret #1221

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add support for `system-internal-tls` in net-istio #1085

Add support for `system-internal-tls` in net-istio #1085

ReToCode commented Mar 28, 2023 •

edited

ReToCode commented Mar 28, 2023

ReToCode commented Mar 28, 2023

evankanderson commented Mar 28, 2023

evankanderson commented Mar 28, 2023

evankanderson left a comment

ReToCode commented Mar 29, 2023

ReToCode commented Mar 29, 2023

nak3 left a comment

ReToCode commented Mar 31, 2023

nak3 commented Mar 31, 2023

ReToCode commented Mar 31, 2023

ReToCode commented Apr 3, 2023

nak3 commented Apr 4, 2023

nak3 commented Apr 4, 2023

ReToCode commented Apr 11, 2023

codecov bot commented Apr 18, 2023 •

edited

ReToCode commented Apr 18, 2023

ReToCode commented Apr 18, 2023

github-actions bot commented Jul 29, 2023

ReToCode commented Jul 31, 2023

ReToCode commented Oct 11, 2023

ReToCode commented Oct 11, 2023

nak3 commented Oct 12, 2023

knative-prow bot commented Oct 12, 2023

ReToCode commented Oct 12, 2023

ReToCode commented Oct 17, 2023

Add support for system-internal-tls in net-istio #1085

Add support for system-internal-tls in net-istio #1085

Conversation

ReToCode commented Mar 28, 2023 • edited

Changes

Things to discuss

ReToCode commented Mar 28, 2023

ReToCode commented Mar 28, 2023

evankanderson commented Mar 28, 2023

evankanderson commented Mar 28, 2023

evankanderson left a comment

Choose a reason for hiding this comment

ReToCode commented Mar 29, 2023

ReToCode commented Mar 29, 2023

nak3 left a comment

Choose a reason for hiding this comment

ReToCode commented Mar 31, 2023

nak3 commented Mar 31, 2023

ReToCode commented Mar 31, 2023

ReToCode commented Apr 3, 2023

nak3 commented Apr 4, 2023

nak3 commented Apr 4, 2023

ReToCode commented Apr 11, 2023

codecov bot commented Apr 18, 2023 • edited

Codecov Report

ReToCode commented Apr 18, 2023

ReToCode commented Apr 18, 2023

github-actions bot commented Jul 29, 2023

ReToCode commented Jul 31, 2023

ReToCode commented Oct 11, 2023

ReToCode commented Oct 11, 2023

nak3 commented Oct 12, 2023

knative-prow bot commented Oct 12, 2023

ReToCode commented Oct 12, 2023

ReToCode commented Oct 17, 2023

Add support for `system-internal-tls` in net-istio #1085

Add support for `system-internal-tls` in net-istio #1085

ReToCode commented Mar 28, 2023 •

edited

codecov bot commented Apr 18, 2023 •

edited