DomainMapping Overriding the default HTTP behavior (#12786)

* DomainMapping Overriding the default HTTP behavior * fix lint issue * del usless domainmapping from ut * add comment * add log in http-protocol disabled and invalid case * reuse GetHTTPOption func * move GetHTTPOption to pkg/networking and add comment
knative · Apr 7, 2022 · 39c7b5b · 39c7b5b
1 parent 635bd49
commit 39c7b5b
Show file tree

Hide file tree

Showing 4 changed files with 144 additions and 40 deletions.
diff --git a/pkg/networking/util.go b/pkg/networking/util.go
@@ -0,0 +1,46 @@
+package networking
+
+import (
+	"context"
+	"fmt"
+	"strings"
+
+	networkingpkg "knative.dev/networking/pkg"
+	"knative.dev/networking/pkg/apis/networking"
+	netv1alpha1 "knative.dev/networking/pkg/apis/networking/v1alpha1"
+	"knative.dev/pkg/logging"
+)
+
+// GetHTTPOption get http-protocol from resource annotations if not, get it from configmap config-network
+func GetHTTPOption(ctx context.Context, networkConfig *networkingpkg.Config, annotations map[string]string) (netv1alpha1.HTTPOption, error) {
+	// Get HTTPOption via annotations.
+	if len(annotations) != 0 && networking.GetHTTPProtocol(annotations) != "" {
+		protocol := strings.ToLower(networking.GetHTTPProtocol(annotations))
+		switch networkingpkg.HTTPProtocol(protocol) {
+		case networkingpkg.HTTPEnabled:
+			return netv1alpha1.HTTPOptionEnabled, nil
+		case networkingpkg.HTTPRedirected:
+			return netv1alpha1.HTTPOptionRedirected, nil
+		default:
+			return "", fmt.Errorf("incorrect http-protocol annotation: " + protocol)
+		}
+	}
+
+	// Get logger from context
+	logger := logging.FromContext(ctx)
+
+	// Get HTTPOption via config-network.
+	switch httpProtocol := networkConfig.HTTPProtocol; httpProtocol {
+	case networkingpkg.HTTPEnabled:
+		return netv1alpha1.HTTPOptionEnabled, nil
+	case networkingpkg.HTTPRedirected:
+		return netv1alpha1.HTTPOptionRedirected, nil
+	// This will be deprecated soon
+	case networkingpkg.HTTPDisabled:
+		logger.Warnf("http-protocol %s in config-network ConfigMap will be deprecated soon", httpProtocol)
+		return "", nil
+	default:
+		logger.Warnf("http-protocol %s in config-network ConfigMap is not supported", httpProtocol)
+		return "", nil
+	}
+}
diff --git a/pkg/networking/util_test.go b/pkg/networking/util_test.go
@@ -0,0 +1,90 @@
+package networking
+
+import (
+	"errors"
+	"fmt"
+	"testing"
+
+	network "knative.dev/networking/pkg"
+	networkingpkg "knative.dev/networking/pkg"
+	"knative.dev/networking/pkg/apis/networking"
+	netv1alpha1 "knative.dev/networking/pkg/apis/networking/v1alpha1"
+	logtesting "knative.dev/pkg/logging/testing"
+	"knative.dev/serving/pkg/reconciler/domainmapping/config"
+)
+
+func TestGetHTTPOption(t *testing.T) {
+	for _, tc := range []struct {
+		name                   string
+		configHTTPProtocol     network.HTTPProtocol
+		annotationHTTPProtocol network.HTTPProtocol
+		wantHTTPOption         netv1alpha1.HTTPOption
+		wantError              error
+	}{{
+		name:                   "HTTPProtocol enabled by config, enabled by annotation",
+		configHTTPProtocol:     network.HTTPEnabled,
+		annotationHTTPProtocol: network.HTTPEnabled,
+		wantHTTPOption:         netv1alpha1.HTTPOptionEnabled,
+	}, {
+		name:                   "HTTPProtocol enabled by config, redirected by annotation",
+		configHTTPProtocol:     network.HTTPEnabled,
+		annotationHTTPProtocol: network.HTTPRedirected,
+		wantHTTPOption:         netv1alpha1.HTTPOptionRedirected,
+	}, {
+		name:                   "HTTPProtocol enabled by config, invalid by annotation",
+		configHTTPProtocol:     network.HTTPEnabled,
+		annotationHTTPProtocol: "foo",
+		wantError:              errors.New("incorrect http-protocol annotation: foo"),
+	}, {
+		name:                   "HTTPProtocol redirected by config, enabled by annotation",
+		configHTTPProtocol:     network.HTTPRedirected,
+		annotationHTTPProtocol: network.HTTPEnabled,
+		wantHTTPOption:         netv1alpha1.HTTPOptionEnabled,
+	}, {
+		name:                   "HTTPProtocol redirected by config, redirected by annotation",
+		configHTTPProtocol:     network.HTTPRedirected,
+		annotationHTTPProtocol: network.HTTPRedirected,
+		wantHTTPOption:         netv1alpha1.HTTPOptionRedirected,
+	}, {
+		name:                   "HTTPProtocol redirected by config, invalid by annotation",
+		configHTTPProtocol:     network.HTTPRedirected,
+		annotationHTTPProtocol: "foo",
+		wantError:              errors.New("incorrect http-protocol annotation: foo"),
+	}, {
+		name:               "HTTPProtocol enabled by config, nil annotations",
+		configHTTPProtocol: network.HTTPEnabled,
+		wantHTTPOption:     netv1alpha1.HTTPOptionEnabled,
+	}, {
+		name:               "HTTPProtocol redirected by config, nil annotations",
+		configHTTPProtocol: network.HTTPRedirected,
+		wantHTTPOption:     netv1alpha1.HTTPOptionRedirected,
+	}, {
+		name:               "HTTPProtocol disabled by config, nil annotations",
+		configHTTPProtocol: network.HTTPDisabled,
+		wantHTTPOption:     "",
+	}} {
+		t.Run(tc.name, func(t *testing.T) {
+			ctx := logtesting.TestContextWithLogger(t)
+			ctx = config.ToContext(ctx, &config.Config{
+				Network: &network.Config{
+					HTTPProtocol: tc.configHTTPProtocol,
+				},
+			})
+
+			var annotations map[string]string
+			if tc.annotationHTTPProtocol != "" {
+				annotations = map[string]string{
+					networking.HTTPProtocolAnnotationKey: string(tc.annotationHTTPProtocol),
+				}
+			}
+
+			got, err := GetHTTPOption(ctx, &networkingpkg.Config{HTTPProtocol: tc.configHTTPProtocol}, annotations)
+			if tc.wantError != nil && fmt.Sprintf("%s", err) != fmt.Sprintf("%s", tc.wantError) {
+				t.Errorf("err = %s, want %v", err, tc.wantError)
+			}
+			if tc.wantError == nil && got != tc.wantHTTPOption {
+				t.Errorf("GetHTTPOption = %s, want %s", got, tc.wantHTTPOption)
+			}
+		})
+	}
+}
diff --git a/pkg/reconciler/domainmapping/reconciler.go b/pkg/reconciler/domainmapping/reconciler.go
@@ -47,6 +47,7 @@ import (
 	v1 "knative.dev/serving/pkg/apis/serving/v1"
 	"knative.dev/serving/pkg/apis/serving/v1alpha1"
 	domainmappingreconciler "knative.dev/serving/pkg/client/injection/reconciler/serving/v1alpha1/domainmapping"
+	servingnetworking "knative.dev/serving/pkg/networking"
 	"knative.dev/serving/pkg/reconciler/domainmapping/config"
 	"knative.dev/serving/pkg/reconciler/domainmapping/resources"
 	routeresources "knative.dev/serving/pkg/reconciler/route/resources"
@@ -121,16 +122,10 @@ func (r *Reconciler) ReconcileKind(ctx context.Context, dm *v1alpha1.DomainMappi
 		return err
 	}
 
-	// Set HTTPOption via config-network.
-	var httpOption netv1alpha1.HTTPOption
-	switch config.FromContext(ctx).Network.HTTPProtocol {
-	case networkingpkg.HTTPEnabled:
-		httpOption = netv1alpha1.HTTPOptionEnabled
-	case networkingpkg.HTTPRedirected:
-		httpOption = netv1alpha1.HTTPOptionRedirected
-	// This will be deprecated soon
-	case networkingpkg.HTTPDisabled:
-		httpOption = ""
+	// HTTPOption can be set via annotations or in the config map.
+	httpOption, err := servingnetworking.GetHTTPOption(ctx, config.FromContext(ctx).Network, dm.GetAnnotations())
+	if err != nil {
+		return err
 	}
 
 	// Reconcile the Ingress resource corresponding to the requested Mapping.

diff --git a/pkg/reconciler/route/resources/ingress.go b/pkg/reconciler/route/resources/ingress.go
@@ -19,9 +19,7 @@ package resources
 import (
 	"context"
 	"encoding/json"
-	"fmt"
 	"sort"
-	"strings"
 
 	"github.com/davecgh/go-spew/spew"
 	"go.uber.org/zap"
@@ -39,6 +37,7 @@ import (
 	apicfg "knative.dev/serving/pkg/apis/config"
 	"knative.dev/serving/pkg/apis/serving"
 	servingv1 "knative.dev/serving/pkg/apis/serving/v1"
+	servingnetworking "knative.dev/serving/pkg/networking"
 	"knative.dev/serving/pkg/reconciler/route/config"
 	"knative.dev/serving/pkg/reconciler/route/domains"
 	"knative.dev/serving/pkg/reconciler/route/resources/labels"
@@ -192,44 +191,18 @@ func makeIngressSpec(
 		}
 	}
 
-	httpProtocol, err := getHTTPProtocol(ctx, r.Annotations)
+	httpOption, err := servingnetworking.GetHTTPOption(ctx, config.FromContext(ctx).Network, r.GetAnnotations())
 	if err != nil {
 		return netv1alpha1.IngressSpec{}, err
 	}
 
 	return netv1alpha1.IngressSpec{
 		Rules:      rules,
 		TLS:        tls,
-		HTTPOption: httpProtocol,
+		HTTPOption: httpOption,
 	}, nil
 }
 
-func getHTTPProtocol(ctx context.Context, annotations map[string]string) (netv1alpha1.HTTPOption, error) {
-	if len(annotations) != 0 && networking.GetHTTPProtocol(annotations) != "" {
-		protocol := strings.ToLower(networking.GetHTTPProtocol(annotations))
-		switch network.HTTPProtocol(protocol) {
-		case network.HTTPEnabled:
-			return netv1alpha1.HTTPOptionEnabled, nil
-		case network.HTTPRedirected:
-			return netv1alpha1.HTTPOptionRedirected, nil
-		default:
-			return "", fmt.Errorf("incorrect http-protocol annotation:" + protocol)
-		}
-	}
-
-	switch config.FromContext(ctx).Network.HTTPProtocol {
-	case network.HTTPEnabled:
-		return netv1alpha1.HTTPOptionEnabled, nil
-	case network.HTTPRedirected:
-		return netv1alpha1.HTTPOptionRedirected, nil
-	// This will be deprecated soon
-	case network.HTTPDisabled:
-		return "", nil
-	default:
-		return "", nil
-	}
-}
-
 func getChallengeHosts(challenges []netv1alpha1.HTTP01Challenge) map[string]netv1alpha1.HTTP01Challenge {
 	c := make(map[string]netv1alpha1.HTTP01Challenge, len(challenges))