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

Add support for HTTPOption #437

Open
wants to merge 7 commits into
base: main
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
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
2 changes: 2 additions & 0 deletions config/config-gateway.yaml
Original file line number Diff line number Diff line change
Expand Up @@ -50,6 +50,7 @@ data:
- class: istio
gateway: istio-system/knative-gateway
service: istio-system/istio-ingressgateway
http-listener-name: http
supported-features:
- HTTPRouteRequestTimeout

Expand All @@ -58,5 +59,6 @@ data:
- class: istio
gateway: istio-system/knative-local-gateway
service: istio-system/knative-local-gateway
http-listener-name: http
supported-features:
- HTTPRouteRequestTimeout
4 changes: 2 additions & 2 deletions docs/test-version.md
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.22.1 | retry,httpoption |
| Contour | v1.29.1 | httpoption |
| Istio | v1.22.1 | retry |
| Contour | v1.29.1 | |
6 changes: 3 additions & 3 deletions hack/test-env.sh
Original file line number Diff line number Diff line change
Expand Up @@ -16,9 +16,9 @@

export GATEWAY_API_VERSION="v1.1.0"
export ISTIO_VERSION="1.22.1"
export ISTIO_UNSUPPORTED_E2E_TESTS="retry,httpoption"
export ISTIO_UNSUPPORTED_E2E_TESTS="retry"
export CONTOUR_VERSION="v1.29.1"
export CONTOUR_UNSUPPORTED_E2E_TESTS="httpoption"
export CONTOUR_UNSUPPORTED_E2E_TESTS=""

export ENVOY_GATEWAY_VERSION="latest"
export ENVOY_GATEWAY_UNSUPPORTED_E2E_TESTS="httpoption,host-rewrite"
export ENVOY_GATEWAY_UNSUPPORTED_E2E_TESTS="host-rewrite"
8 changes: 8 additions & 0 deletions pkg/reconciler/ingress/config/gateway.go
Original file line number Diff line number Diff line change
Expand Up @@ -47,6 +47,7 @@ func defaultExternalGateways() []Gateway {
Name: "istio-ingressgateway",
Namespace: "istio-system",
},
HTTPListenerName: "http",
SupportedFeatures: sets.New(
features.SupportHTTPRouteRequestTimeout,
),
Expand All @@ -64,6 +65,7 @@ func defaultLocalGateways() []Gateway {
Name: "knative-local-gateway",
Namespace: "istio-system",
},
HTTPListenerName: "http",
SupportedFeatures: sets.New(
features.SupportHTTPRouteRequestTimeout,
),
Expand All @@ -90,6 +92,7 @@ type Gateway struct {
types.NamespacedName

Class string
HTTPListenerName string
Service *types.NamespacedName
SupportedFeatures sets.Set[features.SupportedFeature]
}
Expand Down Expand Up @@ -138,6 +141,7 @@ type gatewayEntry struct {
Gateway string `json:"gateway"`
Service *string `json:"service"`
Class string `json:"class"`
HTTPListenerName string `json:"http-listener-name"`
SupportedFeatures []features.SupportedFeature `json:"supported-features"`
}

Expand All @@ -152,6 +156,7 @@ func parseGatewayConfig(data string) ([]Gateway, error) {
for i, entry := range entries {
gw := Gateway{
Class: entry.Class,
HTTPListenerName: entry.HTTPListenerName,
SupportedFeatures: sets.New(entry.SupportedFeatures...),
}

Expand All @@ -174,6 +179,9 @@ func parseGatewayConfig(data string) ([]Gateway, error) {
if len(strings.TrimSpace(gw.Class)) == 0 {
return nil, fmt.Errorf(`entry [%d] field "class" is required`, i)
}
if len(strings.TrimSpace(gw.HTTPListenerName)) == 0 {
return nil, fmt.Errorf(`entry [%d] field "http-listener-name" is required`, i)
}

gws = append(gws, gw)
}
Expand Down
16 changes: 14 additions & 2 deletions pkg/reconciler/ingress/config/gateway_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -21,11 +21,11 @@ import (
"testing"

corev1 "k8s.io/api/core/v1"
. "knative.dev/pkg/configmap/testing"
ktesting "knative.dev/pkg/configmap/testing"
)

func TestFromConfigMap(t *testing.T) {
cm, example := ConfigMapsFromTestFile(t, GatewayConfigName)
cm, example := ktesting.ConfigMapsFromTestFile(t, GatewayConfigName)

if _, err := FromConfigMap(cm); err != nil {
t.Error("FromConfigMap(actual) =", err)
Expand Down Expand Up @@ -58,9 +58,11 @@ func TestFromConfigMapErrors(t *testing.T) {
data: map[string]string{
"external-gateways": `[{
"class":"boo",
"http-listener-name":"http",
"gateway": "ns/n"
},{
"class":"boo",
"http-listener-name":"http",
"gateway": "ns/n"
}]`,
},
Expand All @@ -70,9 +72,11 @@ func TestFromConfigMapErrors(t *testing.T) {
data: map[string]string{
"local-gateways": `[{
"class":"boo",
"http-listener-name":"http",
"gateway": "ns/n"
},{
"class":"boo",
"http-listener-name":"http",
"gateway": "ns/n"
}]`,
},
Expand Down Expand Up @@ -119,6 +123,12 @@ func TestFromConfigMapErrors(t *testing.T) {
"local-gateways": `[{"class": "class", "gateway": "ns/n", "service":"name"}]`,
},
want: `unable to parse "local-gateways"`,
}, {
name: "missing gateway http-listener-name",
data: map[string]string{
"local-gateways": `[{"class":"class", "gateway": "namespace/name"}]`,
},
want: `unable to parse "local-gateways": entry [0] field "http-listener-name" is required`,
}}

for _, tc := range cases {
Expand All @@ -141,9 +151,11 @@ func TestGatewayNoService(t *testing.T) {
Data: map[string]string{
"external-gateways": `
- class: istio
http-listener-name: http
gateway: istio-system/knative-gateway`,
"local-gateways": `
- class: istio
http-listener-name: http
gateway: istio-system/knative-local-gateway`,
},
})
Expand Down
4 changes: 2 additions & 2 deletions pkg/reconciler/ingress/controller_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -33,11 +33,11 @@ import (
_ "knative.dev/pkg/client/injection/kube/informers/core/v1/endpoints/fake"
_ "knative.dev/pkg/client/injection/kube/informers/core/v1/pod/fake"

. "knative.dev/pkg/reconciler/testing"
ktesting "knative.dev/pkg/reconciler/testing"
)

func TestNew(t *testing.T) {
ctx, _ := SetupFakeContext(t)
ctx, _ := ktesting.SetupFakeContext(t)

c := NewController(ctx, configmap.NewStaticWatcher(&corev1.ConfigMap{
ObjectMeta: metav1.ObjectMeta{
Expand Down
14 changes: 12 additions & 2 deletions pkg/reconciler/ingress/ingress.go
Original file line number Diff line number Diff line change
Expand Up @@ -107,12 +107,22 @@ func (c *Reconciler) reconcileIngress(ctx context.Context, ing *v1alpha1.Ingress
for _, rule := range ing.Spec.Rules {
rule := rule

httproute, probeTargets, err := c.reconcileHTTPRoute(ctx, ingressHash, ing, &rule)
workloadRoute, probeTargets, err := c.reconcileWorkloadRoute(ctx, ingressHash, ing, &rule)
if err != nil {
return err
}

if isHTTPRouteReady(httproute) {
// For now, we only generate the redirected HTTPRoute for external visibility,
// because currently we do not (yet) support HTTPs for cluster-local domains in net-gateway-api.
Copy link

@skonto skonto Sep 27, 2024

Choose a reason for hiding this comment

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

Do we support that for internal encryption and Kourier? 🤔
What is the status for the rest of the ingress implementations?

Copy link
Member Author

Choose a reason for hiding this comment

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

yes Kourier supports it, net-istio does not need it and net-contour work was started but never finished.
But as this is quite niche and not in the conformance spec, I think we are fine without it here.

var redirectRoute *gatewayapi.HTTPRoute
if ing.Spec.HTTPOption == v1alpha1.HTTPOptionRedirected && rule.Visibility == v1alpha1.IngressVisibilityExternalIP {
redirectRoute, err = c.reconcileRedirectHTTPRoute(ctx, ing, &rule)
if err != nil {
return err
}
}

if isHTTPRouteReady(workloadRoute) && (redirectRoute == nil || isHTTPRouteReady(redirectRoute)) {
ing.Status.MarkNetworkConfigured()

state, err := c.statusManager.DoProbes(ctx, probeTargets)
Expand Down
Loading
Loading