From 39c7b5bb2242509e2c67ad414cb183acfcd5dec4 Mon Sep 17 00:00:00 2001 From: wei Date: Fri, 8 Apr 2022 04:00:45 +0800 Subject: [PATCH] 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 --- pkg/networking/util.go | 46 +++++++++++ pkg/networking/util_test.go | 90 ++++++++++++++++++++++ pkg/reconciler/domainmapping/reconciler.go | 15 ++-- pkg/reconciler/route/resources/ingress.go | 33 +------- 4 files changed, 144 insertions(+), 40 deletions(-) create mode 100644 pkg/networking/util.go create mode 100644 pkg/networking/util_test.go diff --git a/pkg/networking/util.go b/pkg/networking/util.go new file mode 100644 index 000000000000..2b0e51ca9ef6 --- /dev/null +++ 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 new file mode 100644 index 000000000000..852b4894ed46 --- /dev/null +++ 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 index 84379e1d870a..0e8cd28fc1b6 100644 --- 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 index 9700ca41194f..13c272654d56 100644 --- 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,7 +191,7 @@ 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 } @@ -200,36 +199,10 @@ func makeIngressSpec( 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))