Add support for TLS (conformance test passes) #316

evankanderson · 2022-06-28T09:41:12Z

Changes

🐛 Add support for TLS by filling out Gateway's Spec.Listener
🧹 Fix a few typo'ed import aliases

Still TODO:

Clean up Spec.Listener on delete (separate PR)
More testing in ingress_test.go (I got one done, but I want to test the "listener already exists" scenario, at least)

/kind enhancement

Fixes #199
Fixes #319

Release Note

Adds support for Knative-configured TLS ("auto-TLS")

Docs

This increases parity with other implementations, but should not require its own
documentation, as no configuration options were added.

codecov · 2022-06-28T16:58:01Z

Codecov Report

Merging #316 (ad027c7) into main (c82e90c) will increase coverage by 3.63%.
The diff coverage is 68.58%.

@@            Coverage Diff             @@
##             main     #316      +/-   ##
==========================================
+ Coverage   12.91%   16.55%   +3.63%     
==========================================
  Files          20       21       +1     
  Lines        2958     3172     +214     
==========================================
+ Hits          382      525     +143     
- Misses       2554     2616      +62     
- Partials       22       31       +9

Impacted Files	Coverage Δ
...kg/reconciler/ingress/resources/reference_grant.go	`0.00% <0.00%> (ø)`
pkg/reconciler/ingress/reconcile_resources.go	`67.67% <77.21%> (+35.96%)`	⬆️
pkg/reconciler/ingress/ingress.go	`74.19% <87.50%> (+2.19%)`	⬆️
pkg/reconciler/ingress/controller.go	`94.02% <100.00%> (+0.27%)`	⬆️
pkg/reconciler/ingress/resources/httproute.go	`94.77% <100.00%> (+0.16%)`	⬆️

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 c82e90c...ad027c7. Read the comment docs.

dprotaso · 2022-06-28T17:55:41Z

pkg/reconciler/ingress/reconcile_resources.go

+	secret := metav1.PartialObjectMetadata{
+		TypeMeta: metav1.TypeMeta{
+			Kind:       "Secret",
+			APIVersion: "v1", //metav1.GroupVersion.String(),


nit: corev1.Secret not metav1

Thanks, done

dprotaso

whoops accidentally hit the wrong button earlier

change looks good

tl;dr

I think one of the lint warnings is a legit bug
We should have a follow up where we create multiple gateways vs updating a single one

dprotaso · 2022-06-28T17:57:10Z

pkg/reconciler/ingress/reconcile_resources.go

+		_, err := c.gwapiclient.GatewayV1alpha2().Gateways(update.Namespace).Update(
+			ctx, update, metav1.UpdateOptions{})


Want to open up a new issue about creating multiple gateways vs. updating a single one?

dprotaso · 2022-06-28T18:01:56Z

pkg/reconciler/ingress/reconcile_resources.go

+		_, err := c.gwapiclient.GatewayV1alpha2().Gateways(update.Namespace).Update(
+			ctx, update, metav1.UpdateOptions{})
+		if err != nil {
+			recorder.Eventf(ing, corev1.EventTypeWarning, "GatewayUpdateFailed", "Failed to update Gateway %s: %v", gwName, err)


Given that these controllers have a default threadiness of 2 - I'd expect many update conflicts since we're updating a single shared resource - this can be addressed by using multiple gateways at a future date.

I'd probably skip recording the event since that might trigger client side throttling since we'll be hitting the API frequently

Also, we shouldn't run this controller in HA with the number of buckets being greater than 1.

Ah, I see what you're suggesting. I'll add o TODO and a comment here.

I'll also bring this up with the Gateway API maintainers -- it feels like generating hundreds of Gateways that are supposed to collapse onto the same proxy is probably not right (and doesn't seem to work for at least Contour today), but the one-Gateway model doesn't scale, either.

Filed #318

dprotaso · 2022-06-28T18:12:23Z

pkg/reconciler/ingress/ingress.go

+	if len(listeners) > 0 {
+		// For now, we only reconcile the external visibility, because there's
+		// no way to provide TLS for internal listeners.
+		err := c.reconcileGatewayListeners(


Curious if we should update the gateway listeners first before updating the httproute

I'm not sure -- how would you decide which to do?

I could imagine a poor implementation (of the gateway api) that doesn't trigger reconciliation of existing HTTP routes when ReferenceGrants for certain namespaces are create.

Thus I think I'd prefer to create the listener & grant and update the gateway prior to creating an HTTPRoute.

I'm ok if that's done in a followup PR

dprotaso · 2022-06-28T18:16:32Z

pkg/reconciler/ingress/reconcile_resources.go

+	for _, h := range tls.Hosts {
+		listener := gatewayv1alpha2.Listener{
+			Name:     gatewayv1alpha2.SectionName(h),
+			Hostname: (*gatewayv1alpha2.Hostname)(&h),


this lint warning seems legit -probably should make a copy of h otherwise this points to the address space that's updated by the loop iteration

Ah, I don't think I've ever had more than one entry in tls.Hosts, good catch!

dprotaso · 2022-06-28T19:01:20Z

pkg/reconciler/ingress/reconcile_resources.go

+	for _, l := range listeners {
+		lmap[string(l.Name)] = l
+	}
+	// TODO: how do we track and remove listeners if they are removed from the KIngress spec?


evankanderson · 2022-06-28T21:47:22Z

Okay, I've added a couple more test cases, going over Dave's comments now.

I think my next steps are going to be to figure out what the Contour maintainers expect from AppPrortocol to enable Webhook and TLS, and see whether or not I can reasonably hack that into Contour to get the websocket and grpc tests passing.

nak3 · 2022-06-29T12:06:09Z

This repo has two conformance tests:

the test with net-gateway-api controller. (test/kind-e2e-contour.sh)
the test without net-gateway-api controller (test/kind-conformance-contour.sh)

This PR has 1, but we still need to add a test for 2. I think we can do it in another PR but just wanted to mention about it.

Actually 2 still needs to add the test code into test/conformance/ingressv2 and enable it in run.go.
The procedure is described in DEVELOPMENT.md.

dprotaso · 2022-06-29T14:27:47Z

pkg/reconciler/ingress/ingress_test.go

@@ -28,6 +28,7 @@ import (
 	"k8s.io/apimachinery/pkg/runtime"
 	"k8s.io/apimachinery/pkg/types"
 	clientgotesting "k8s.io/client-go/testing"
+	"k8s.io/utils/pointer"


nit: knative/pkg/ptr

Why prefer knative.dev packages to the upstream k8s ones?

evankanderson · 2022-06-29T18:21:16Z

/hold

After discussion and testing, it turns out that we can create new Gateway resources in both contour and istio implementations which share the same IP address, so I'm going to re-work this to create new Gateway resources for each KIngress with TLS, rather than adding onto an existing Gateway.

evankanderson · 2022-06-30T15:31:44Z

/hold cancel

Gateway merging doesn't actually work for at least Contour, and if you have two Gateways, there's no clear way to determine which Gateway the ones without addresses merge to.

evankanderson · 2022-07-01T17:29:12Z

/hold

@carlisia asked that #315 go in first.

I'm going to see whether I can get listener cleanup in while she's working.

Added some (defaulted) fields to HTTPRoute to avoid controller looping.

evankanderson · 2022-07-06T00:12:04Z

I added some more code, we now have distinctly-named listeners of the form kni-$INGRESS_UID, and a finalizer to clean them up.

/assign @carlisia

@dprotaso @nak3 not sure if you want to take (another?) look.

nak3 · 2022-07-06T05:59:38Z

/test integration-tests_net-gateway-api_main

LGTM. Thank you!

evankanderson

Missed one comment earlier.

Also, this is now merged with #315, and unit tests should be passing. I'm checking e2e now, but this is ready for review.

@carlisia @dprotaso @nak3

I wouldn't mind this getting in before nightly testing / as a candidate for 1.6...

evankanderson · 2022-07-11T22:25:16Z

pkg/reconciler/ingress/ingress_test.go

@@ -28,6 +28,7 @@ import (
 	"k8s.io/apimachinery/pkg/runtime"
 	"k8s.io/apimachinery/pkg/types"
 	clientgotesting "k8s.io/client-go/testing"
+	"k8s.io/utils/pointer"


Why prefer knative.dev packages to the upstream k8s ones?

evankanderson · 2022-07-11T22:56:34Z

Ugh, I've somehow screwed up just the kind e2e tests...

evankanderson · 2022-07-11T22:57:26Z

/hold

(sorting out the timeout...)

evankanderson · 2022-07-11T23:17:25Z

I missed the rename ReferencePolicy -> ReferenceGrant in the ClusteRole for the controller, so it couldn't manage the resources it thought it needed to create.

/hold cancel

evankanderson · 2022-07-11T23:21:14Z

Tests run locally seem to pass now... (I had a cleanup issue I was debugging when I pushed before the /hold, so I figured I'd get the CI tests in parallel).

evankanderson · 2022-07-11T23:40:42Z

Hmm, it looks like the conformance tests in the e2e tests are unhappy; I can repro locally with Istio, but not with Contour. Debugging...

carlisia · 2022-07-11T23:44:01Z

The PR changes lgtm. I'll standby...

carlisia · 2022-07-11T23:46:44Z

pkg/reconciler/ingress/resources/httproute.go

@@ -75,6 +75,8 @@ func makeHTTPRouteSpec(
 	namespacedNameGateway := gatewayConfig.Gateways[rule.Visibility].Gateway

 	gatewayRef := gatewayv1alpha2.ParentReference{
+		Group:     (*gatewayv1alpha2.Group)(&gatewayv1alpha2.GroupVersion.Group),
+		Kind:      (*gatewayv1alpha2.Kind)(pointer.String("Gateway")),


Does the Port not need to be added as well?

carlisia · 2022-07-11T23:55:31Z

Whoa side note: I was wondering where and how ISTIO_VERSION was being used since I couldn't find it anywhere in the code base. It is used by the Istio script for its installation (https://raw.githubusercontent.com/istio/istio/master/release/downloadIstioCtl.sh).

Here are the changes to the latest version, v1.14.1: Istio / Announcing Istio 1.14.1. I wonder if this update would help with the tls stuff.

PS: I'll document the info about the Istio version.

Since the Istio version being currently used (1.13.2), this other update also adds a TLS related fix:
Istio / Announcing Istio 1.13.5

carlisia · 2022-07-12T00:07:37Z

Also: Istio has not had a release since upgrading their code to use v0.5.0-rc1 (gateway-api: upgrade to v0.5.0-rc1 by howardjohn · Pull Request #39506 · istio/istio).

carlisia · 2022-07-12T02:16:35Z

Trying to update Istio is failing: Bump Istio to v1.14.1 by carlisia · Pull Request #330 · knative-sandbox/net-gateway-api

…sn't support ReferenceGrant yet, and there's no converter.

evankanderson · 2022-07-12T13:40:17Z

I foolishly attempted to use the new ReferenceGrant API immediately; I've since mended my ways and switched back to using ReferencePolicy.

Pro tip when doing a merge like this:

Make sure the e2e test passes before upgrade
Upgrade the code and run the e2e test on the pre-merge controller with post-merge tests.
Redeploy the controller and verify the e2e tests continue to pass.

Once I did this, triage of the tests was a lot easier, because it was clear that it was something in the controller code, not the merge, that had broken the tests.

evankanderson · 2022-07-12T13:40:24Z

/hold cascel

evankanderson · 2022-07-12T13:40:49Z

Tests, including TLS tests, now pass locally.

carlisia · 2022-07-12T15:36:39Z

This is great, thank you! 🎉

I will add an issue to switch to ReferenceGrant once the providers are updated to support it. And I'll add your tips to the dev documentation.

Thanks for creating all the issues for followup work. Wanted to surface this one comment, I think it requires an issue as well? #316

/lgtm
/approve

🚀

carlisia · 2022-07-12T15:43:38Z

/approve

knative-prow · 2022-07-12T15:43:45Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: carlisia, evankanderson

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~~ [carlisia,evankanderson]

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

carlisia · 2022-07-12T15:45:38Z

/hold cancel

Add support for TLS (conformance test passes)

fcabfa5

knative-prow bot added the kind/enhancement label Jun 28, 2022

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

knative-prow bot added 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 Jun 28, 2022

Merge remote-tracking branch 'upstream/main' into enable-tls

f592dea

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

Fix codegen update and run

811acab

dprotaso reviewed Jun 28, 2022

View reviewed changes

Fix reconcile on no changes; add reconciliation tests

aba67bd

This was referenced Jun 28, 2022

Handle more than about 64 TLS Knative Routes #317

Open

Resolve HA / multi-thread concerns around Gateway updates #318

Open

Evan Anderson added 2 commits June 28, 2022 20:12

Update with Dave's feedback.

d986f35

Fix lint warnings.

2cdb1e8

dprotaso reviewed Jun 29, 2022

View reviewed changes

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

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

evankanderson mentioned this pull request Jul 1, 2022

⬆️ Bump Gateway API version to v0.5.0-rc1 #315

Merged

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

Add Finalizer to cleanup Gateway Listeners

1592176

Added some (defaulted) fields to HTTPRoute to avoid controller looping.

knative-prow bot assigned carlisia Jul 6, 2022

Fix test breakage after merge post-knative-extensions#315

4045dce

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

evankanderson commented Jul 11, 2022

View reviewed changes

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

Update referencepolicy->referencegrant in roles as well

7741195

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

carlisia reviewed Jul 11, 2022

View reviewed changes

Switch back to ReferencePolicy from ReferenceGrant, because Istio doe…

ad027c7

…sn't support ReferenceGrant yet, and there's no converter.

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

knative-prow bot added the lgtm Indicates that a PR is ready to be merged. label Jul 12, 2022

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

knative-prow bot merged commit 4e987da into knative-extensions:main Jul 12, 2022

This was referenced Aug 5, 2022

EPIC - Contour tests #36

Closed

EPIC - Implement each Knative Networking (Gateway API) conformance test #348

Closed

		_, err := c.gwapiclient.GatewayV1alpha2().Gateways(update.Namespace).Update(
		ctx, update, metav1.UpdateOptions{})

Add support for TLS (conformance test passes) #316

Add support for TLS (conformance test passes) #316

Conversation

evankanderson commented Jun 28, 2022 • edited Loading

Changes

codecov bot commented Jun 28, 2022 • edited Loading

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dprotaso left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

evankanderson commented Jun 28, 2022

nak3 commented Jun 29, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

evankanderson commented Jun 29, 2022

evankanderson commented Jun 30, 2022

evankanderson commented Jul 1, 2022 • edited Loading

evankanderson commented Jul 6, 2022

nak3 commented Jul 6, 2022

evankanderson left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

evankanderson commented Jul 11, 2022

evankanderson commented Jul 11, 2022

evankanderson commented Jul 11, 2022

evankanderson commented Jul 11, 2022

evankanderson commented Jul 11, 2022

carlisia commented Jul 11, 2022

Choose a reason for hiding this comment

carlisia commented Jul 11, 2022 • edited Loading

carlisia commented Jul 12, 2022

carlisia commented Jul 12, 2022

evankanderson commented Jul 12, 2022

evankanderson commented Jul 12, 2022

evankanderson commented Jul 12, 2022

carlisia commented Jul 12, 2022

carlisia commented Jul 12, 2022

knative-prow bot commented Jul 12, 2022

carlisia commented Jul 12, 2022

evankanderson commented Jun 28, 2022 •

edited

Loading

codecov bot commented Jun 28, 2022 •

edited

Loading

evankanderson commented Jul 1, 2022 •

edited

Loading

carlisia commented Jul 11, 2022 •

edited

Loading