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

Add support for TLS (conformance test passes) #316

Merged
merged 12 commits into from
Jul 12, 2022
Merged

Add support for TLS (conformance test passes) #316

Show file tree
Hide file tree
Changes from 8 commits
Commits
Show all changes
12 commits
Select commit Hold shift + click to select a range
fcabfa5
Add support for TLS (conformance test passes)
Jun 28, 2022
f592dea
Merge remote-tracking branch 'upstream/main' into enable-tls
Jun 28, 2022
811acab
Fix codegen update and run
Jun 28, 2022
aba67bd
Fix reconcile on no changes; add reconciliation tests
Jun 28, 2022
d986f35
Update with Dave's feedback.
Jun 29, 2022
2cdb1e8
Fix lint warnings.
Jun 29, 2022
1592176
Add Finalizer to cleanup Gateway Listeners
Jul 6, 2022
f244165
Fix lint complaint.
Jul 7, 2022
17dba7f
Merge remote-tracking branch 'upstream/main' into enable-tls
Jul 11, 2022
4045dce
Fix test breakage after merge post-#315
Jul 11, 2022
7741195
Update referencepolicy->referencegrant in roles as well
Jul 11, 2022
ad027c7
Switch back to ReferencePolicy from ReferenceGrant, because Istio doe…
Jul 12, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 4 additions & 1 deletion config/200-clusterrole.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -39,5 +39,8 @@ metadata:
app.kubernetes.io/version: devel
rules:
- apiGroups: ["gateway.networking.k8s.io"]
resources: ["httproutes", "gateways"]
resources: ["httproutes", "referencepolicies"]
verbs: ["get", "list", "create", "update", "delete", "patch", "watch"]
- apiGroups: ["gateway.networking.k8s.io"]
resources: ["gateways"]
verbs: ["get", "list", "update", "patch", "watch"]
4 changes: 2 additions & 2 deletions docs/test-version.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,5 +15,5 @@ The following Gateway API version and Ingress were tested as part of the release

| Ingress | Tested version | Unavailable features |
| ------- | ----------------------- | ------------------------------ |
| Istio | v1.13.2 | tls,retry,httpoption,host-rewrite |
| Contour | v1.21.0 | tls,retry,httpoption,basics/http2,websocket,websocket/split,grpc,grpc/split,visibility/path,visibility,update,host-rewrite |
| Istio | v1.13.2 | retry,httpoption,host-rewrite |
| Contour | v1.21.0 | retry,httpoption,basics/http2,websocket,websocket/split,grpc,grpc/split,visibility/path,visibility,update,host-rewrite |
4 changes: 2 additions & 2 deletions hack/test-env.sh
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,6 @@

export GATEWAY_API_VERSION="v0.4.3"
export ISTIO_VERSION="1.13.2"
export ISTIO_UNSUPPORTED_E2E_TESTS="tls,retry,httpoption,host-rewrite"
export ISTIO_UNSUPPORTED_E2E_TESTS="retry,httpoption,host-rewrite"
export CONTOUR_VERSION="v1.21.0"
export CONTOUR_UNSUPPORTED_E2E_TESTS="tls,retry,httpoption,basics/http2,websocket,websocket/split,grpc,grpc/split,visibility/path,visibility,update,host-rewrite"
export CONTOUR_UNSUPPORTED_E2E_TESTS="retry,httpoption,basics/http2,websocket,websocket/split,grpc,grpc/split,visibility/path,visibility,update,host-rewrite"
8 changes: 6 additions & 2 deletions pkg/reconciler/ingress/controller.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -37,6 +37,7 @@ import (
gwapiclient "knative.dev/net-gateway-api/pkg/client/gatewayapi/injection/client"
gatewayinformer "knative.dev/net-gateway-api/pkg/client/gatewayapi/injection/informers/apis/v1alpha2/gateway"
httprouteinformer "knative.dev/net-gateway-api/pkg/client/gatewayapi/injection/informers/apis/v1alpha2/httproute"
referencepolicyinformer "knative.dev/net-gateway-api/pkg/client/gatewayapi/injection/informers/apis/v1alpha2/referencepolicy"
"knative.dev/net-gateway-api/pkg/reconciler/ingress/config"
)

Expand All @@ -55,12 +56,15 @@ func NewController(

ingressInformer := ingressinformer.Get(ctx)
httprouteInformer := httprouteinformer.Get(ctx)
referencePolicyInformer := referencepolicyinformer.Get(ctx)
gatewayInformer := gatewayinformer.Get(ctx)
endpointsInformer := endpointsinformer.Get(ctx)

c := &Reconciler{
gwapiclient: gwapiclient.Get(ctx),
httprouteLister: httprouteInformer.Lister(),
gwapiclient: gwapiclient.Get(ctx),
httprouteLister: httprouteInformer.Lister(),
referencePolicyLister: referencePolicyInformer.Lister(),
gatewayLister: gatewayInformer.Lister(),
}

filterFunc := reconciler.AnnotationFilterFunc(networking.IngressClassAnnotationKey, gatewayAPIIngressClassName, false)
Expand Down
1 change: 1 addition & 0 deletions pkg/reconciler/ingress/controller_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -32,6 +32,7 @@ import (

_ "knative.dev/net-gateway-api/pkg/client/gatewayapi/injection/informers/apis/v1alpha2/gateway/fake"
_ "knative.dev/net-gateway-api/pkg/client/gatewayapi/injection/informers/apis/v1alpha2/httproute/fake"
_ "knative.dev/net-gateway-api/pkg/client/gatewayapi/injection/informers/apis/v1alpha2/referencepolicy/fake"
"knative.dev/net-gateway-api/pkg/reconciler/ingress/config"
)

Expand Down
44 changes: 37 additions & 7 deletions pkg/reconciler/ingress/ingress.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -27,7 +27,6 @@ import (
ingressreconciler "knative.dev/networking/pkg/client/injection/reconciler/networking/v1alpha1/ingress"
"knative.dev/networking/pkg/ingress"
"knative.dev/networking/pkg/status"
"knative.dev/pkg/logging"
"knative.dev/pkg/network"
pkgreconciler "knative.dev/pkg/reconciler"

Expand All @@ -49,6 +48,10 @@ type Reconciler struct {

// Listers index properties about resources
httprouteLister gatewayListers.HTTPRouteLister

referencePolicyLister gatewayListers.ReferencePolicyLister

gatewayLister gatewayListers.GatewayLister
}

var (
Expand All @@ -58,6 +61,7 @@ var (
// ReconcileKind implements Interface.ReconcileKind.
func (c *Reconciler) ReconcileKind(ctx context.Context, ingress *v1alpha1.Ingress) pkgreconciler.Event {
reconcileErr := c.reconcileIngress(ctx, ingress)

if reconcileErr != nil {
ingress.Status.MarkIngressNotReady(notReconciledReason, notReconciledMessage)
return reconcileErr
Expand All @@ -66,8 +70,16 @@ func (c *Reconciler) ReconcileKind(ctx context.Context, ingress *v1alpha1.Ingres
return nil
}

// FinalizeKind implements Interface.FinalizeKind
func (c *Reconciler) FinalizeKind(ctx context.Context, ingress *v1alpha1.Ingress) pkgreconciler.Event {
gatewayConfig := config.FromContext(ctx).Gateway

// We currently only support TLS on the external IP
return c.clearGatewayListeners(ctx, ingress, gatewayConfig.Gateways[v1alpha1.IngressVisibilityExternalIP].Gateway)
}

func (c *Reconciler) reconcileIngress(ctx context.Context, ing *v1alpha1.Ingress) error {
logger := logging.FromContext(ctx)
gatewayConfig := config.FromContext(ctx).Gateway

// We may be reading a version of the object that was stored at an older version
// and may not have had all of the assumed defaults specified. This won't result
Expand All @@ -82,8 +94,6 @@ func (c *Reconciler) reconcileIngress(ctx context.Context, ing *v1alpha1.Ingress
return fmt.Errorf("failed to add knative probe header: %w", err)
}

logger.Infof("Reconciling ingress: %#v", ing)

for _, rule := range ing.Spec.Rules {
rule := rule

Expand All @@ -97,17 +107,37 @@ func (c *Reconciler) reconcileIngress(ctx context.Context, ing *v1alpha1.Ingress
} else {
ing.Status.MarkIngressNotReady("HTTPRouteNotReady", "Waiting for HTTPRoute becomes Ready.")
}
logger.Infof("HTTPRoute successfully synced %v", httproutes)
}

listeners := make([]*gatewayv1alpha2.Listener, 0, len(ing.Spec.TLS))
for _, tls := range ing.Spec.TLS {
tls := tls

l, err := c.reconcileTLS(ctx, &tls, ing)
if err != nil {
return err
}
listeners = append(listeners, l...)
}

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(
Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

Copy link
Contributor

Choose a reason for hiding this comment

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

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

ctx, listeners, ing, *gatewayConfig.Gateways[v1alpha1.IngressVisibilityExternalIP].Gateway)
if err != nil {
return err
}
}

// TODO: check Gateway readiness before reporting Ingress ready

ready, err := c.statusManager.IsReady(ctx, before)
if err != nil {
return fmt.Errorf("failed to probe Ingress: %w", err)
}

if ready {
gatewayConfig := config.FromContext(ctx).Gateway

namespacedNameService := gatewayConfig.Gateways[v1alpha1.IngressVisibilityExternalIP].Service
publicLbs := []v1alpha1.LoadBalancerIngressStatus{
{DomainInternal: network.GetServiceHostname(namespacedNameService.Name, namespacedNameService.Namespace)},
Expand Down
Loading