From 21c05dc9d9a4a89298e12425e7c32cbb1ef1adf9 Mon Sep 17 00:00:00 2001 From: Kenjiro Nakayama Date: Sat, 2 Apr 2022 21:48:40 +0900 Subject: [PATCH] Force activator always in path when activator CA is specified (#12790) * Force activator always in path when activator CA is specified As described in the feature doc of https://github.com/knative/serving/issues/11906, activator needs to serve TLS traffic for the traffic from ingress. So, this patch force SKS proxy mode when `activator-ca` in `config-network` is specified. * Fix typo * Use switch --- pkg/reconciler/autoscaling/config/store.go | 4 ++ .../autoscaling/config/store_test.go | 15 ++++- .../config/testdata/config-network.yaml | 1 + pkg/reconciler/autoscaling/hpa/hpa_test.go | 8 +++ pkg/reconciler/autoscaling/kpa/kpa.go | 8 ++- pkg/reconciler/autoscaling/kpa/kpa_test.go | 64 +++++++++++++++++++ 6 files changed, 98 insertions(+), 2 deletions(-) create mode 120000 pkg/reconciler/autoscaling/config/testdata/config-network.yaml diff --git a/pkg/reconciler/autoscaling/config/store.go b/pkg/reconciler/autoscaling/config/store.go index 7a9c91ccf926..2f30e8ec6377 100644 --- a/pkg/reconciler/autoscaling/config/store.go +++ b/pkg/reconciler/autoscaling/config/store.go @@ -19,6 +19,7 @@ package config import ( "context" + network "knative.dev/networking/pkg" "knative.dev/pkg/configmap" asconfig "knative.dev/serving/pkg/autoscaler/config" "knative.dev/serving/pkg/autoscaler/config/autoscalerconfig" @@ -31,6 +32,7 @@ type cfgKey struct{} type Config struct { Autoscaler *autoscalerconfig.Config Deployment *deployment.Config + Network *network.Config } // FromContext fetch config from context. @@ -65,6 +67,7 @@ func NewStore(logger configmap.Logger, onAfterStore ...func(name string, value i configmap.Constructors{ asconfig.ConfigName: asconfig.NewConfigFromConfigMap, deployment.ConfigName: deployment.NewConfigFromConfigMap, + network.ConfigName: network.NewConfigFromConfigMap, }, onAfterStore..., ), @@ -82,5 +85,6 @@ func (s *Store) Load() *Config { return &Config{ Autoscaler: s.UntypedLoad(asconfig.ConfigName).(*autoscalerconfig.Config).DeepCopy(), Deployment: s.UntypedLoad(deployment.ConfigName).(*deployment.Config).DeepCopy(), + Network: s.UntypedLoad(network.ConfigName).(*network.Config).DeepCopy(), } } diff --git a/pkg/reconciler/autoscaling/config/store_test.go b/pkg/reconciler/autoscaling/config/store_test.go index 4afa9f2e86ae..f9761be177f6 100644 --- a/pkg/reconciler/autoscaling/config/store_test.go +++ b/pkg/reconciler/autoscaling/config/store_test.go @@ -24,6 +24,7 @@ import ( "github.com/google/go-cmp/cmp" logtesting "knative.dev/pkg/logging/testing" + network "knative.dev/networking/pkg" . "knative.dev/pkg/configmap/testing" autoscalerconfig "knative.dev/serving/pkg/autoscaler/config" "knative.dev/serving/pkg/deployment" @@ -34,8 +35,10 @@ func TestStoreLoadWithContext(t *testing.T) { autoscalerConfig := ConfigMapFromTestFile(t, autoscalerconfig.ConfigName) depConfig := ConfigMapFromTestFile(t, deployment.ConfigName, deployment.DeprecatedQueueSidecarImageKey) + netConfig := ConfigMapFromTestFile(t, network.ConfigName) store.OnConfigChanged(autoscalerConfig) store.OnConfigChanged(depConfig) + store.OnConfigChanged(netConfig) config := FromContext(store.ToContext(context.Background())) wantAS, _ := autoscalerconfig.NewConfigFromConfigMap(autoscalerConfig) @@ -46,6 +49,10 @@ func TestStoreLoadWithContext(t *testing.T) { if !cmp.Equal(wantD, config.Deployment) { t.Error("Deployment ConfigMap mismatch (-want, +got):", cmp.Diff(wantD, config.Deployment)) } + wantNet, _ := network.NewConfigFromConfigMap(netConfig) + if !cmp.Equal(wantNet, config.Network) { + t.Error("Network ConfigMap mismatch (-want, +got):", cmp.Diff(wantNet, config.Network)) + } } func TestStoreImmutableConfig(t *testing.T) { @@ -54,16 +61,22 @@ func TestStoreImmutableConfig(t *testing.T) { store.OnConfigChanged(ConfigMapFromTestFile(t, autoscalerconfig.ConfigName)) store.OnConfigChanged(ConfigMapFromTestFile(t, deployment.ConfigName, deployment.DeprecatedQueueSidecarImageKey)) + store.OnConfigChanged(ConfigMapFromTestFile(t, network.ConfigName)) config := store.Load() config.Autoscaler.MaxScaleUpRate = 100.0 config.Deployment.ProgressDeadline = 3 * time.Minute + config.Network.ActivatorCA = "activator-ca" + config.Network.ActivatorSAN = "activator-san" newConfig := store.Load() if newConfig.Autoscaler.MaxScaleUpRate == 100.0 { t.Error("Autoscaler config is not immuable") } if newConfig.Deployment.ProgressDeadline == 3*time.Minute { - t.Error("Autoscaler config is not immuable") + t.Error("Deployment config is not immuable") + } + if newConfig.Network.ActivatorCA == "activator-ca" || newConfig.Network.ActivatorSAN == "activator-san" { + t.Error("Network config is not immuable") } } diff --git a/pkg/reconciler/autoscaling/config/testdata/config-network.yaml b/pkg/reconciler/autoscaling/config/testdata/config-network.yaml new file mode 120000 index 000000000000..56cb332a0426 --- /dev/null +++ b/pkg/reconciler/autoscaling/config/testdata/config-network.yaml @@ -0,0 +1 @@ +../../../../../config/core/configmaps/network.yaml \ No newline at end of file diff --git a/pkg/reconciler/autoscaling/hpa/hpa_test.go b/pkg/reconciler/autoscaling/hpa/hpa_test.go index ed5e9331cc60..b1f35198f164 100644 --- a/pkg/reconciler/autoscaling/hpa/hpa_test.go +++ b/pkg/reconciler/autoscaling/hpa/hpa_test.go @@ -45,6 +45,7 @@ import ( "k8s.io/apimachinery/pkg/types" ktesting "k8s.io/client-go/testing" + netpkg "knative.dev/networking/pkg" "knative.dev/networking/pkg/apis/networking" nv1a1 "knative.dev/networking/pkg/apis/networking/v1alpha1" "knative.dev/pkg/configmap" @@ -93,6 +94,13 @@ func TestControllerCanReconcile(t *testing.T) { Data: map[string]string{ deployment.QueueSidecarImageKey: "motorbike-sidecar", }, + }, + &corev1.ConfigMap{ + ObjectMeta: metav1.ObjectMeta{ + Namespace: system.Namespace(), + Name: netpkg.ConfigName, + }, + Data: map[string]string{}, })) waitInformers, err := RunAndSyncInformers(ctx, infs...) diff --git a/pkg/reconciler/autoscaling/kpa/kpa.go b/pkg/reconciler/autoscaling/kpa/kpa.go index 432abbc1b061..d0d18712f373 100644 --- a/pkg/reconciler/autoscaling/kpa/kpa.go +++ b/pkg/reconciler/autoscaling/kpa/kpa.go @@ -124,6 +124,12 @@ func (c *Reconciler) ReconcileKind(ctx context.Context, pa *autoscalingv1alpha1. } mode := nv1alpha1.SKSOperationModeServe + switch { + // When activator CA is enabled, force activator always in path. + // TODO: This is a temporary state and to be fixed. + // See also issues/11906 and issues/12797. + case len(config.FromContext(ctx).Network.ActivatorCA) > 0: + mode = nv1alpha1.SKSOperationModeProxy // We put activator in the serving path in the following cases: // 1. The revision is scaled to 0: // a. want == 0 @@ -131,7 +137,7 @@ func (c *Reconciler) ReconcileKind(ctx context.Context, pa *autoscalingv1alpha1. // this revision, e.g. after a restart) but PA status is inactive (it was // already scaled to 0). // 2. The excess burst capacity is negative. - if want == 0 || decider.Status.ExcessBurstCapacity < 0 || want == scaleUnknown && pa.Status.IsInactive() { + case want == 0 || decider.Status.ExcessBurstCapacity < 0 || want == scaleUnknown && pa.Status.IsInactive(): mode = nv1alpha1.SKSOperationModeProxy } diff --git a/pkg/reconciler/autoscaling/kpa/kpa_test.go b/pkg/reconciler/autoscaling/kpa/kpa_test.go index acee4af4b923..8ec8afa2405d 100644 --- a/pkg/reconciler/autoscaling/kpa/kpa_test.go +++ b/pkg/reconciler/autoscaling/kpa/kpa_test.go @@ -61,6 +61,7 @@ import ( "go.uber.org/atomic" "golang.org/x/sync/errgroup" + netpkg "knative.dev/networking/pkg" nv1a1 "knative.dev/networking/pkg/apis/networking/v1alpha1" duckv1 "knative.dev/pkg/apis/duck/v1" "knative.dev/pkg/configmap" @@ -125,15 +126,29 @@ func initialScaleZeroASConfig() *autoscalerconfig.Config { return ac } +func activatorCertsNetConfig() *netpkg.Config { + nc, _ := netpkg.NewConfigFromMap(map[string]string{ + netpkg.ActivatorCAKey: "knative-ca", + netpkg.ActivatorSANKey: "knative-san", + }) + return nc +} + func defaultConfig() *config.Config { ac, _ := asconfig.NewConfigFromMap(defaultConfigMapData()) deploymentConfig, _ := deployment.NewConfigFromMap(map[string]string{ deployment.QueueSidecarImageKey: "bob", deployment.ProgressDeadlineKey: progressDeadline.String(), }) + networkConfig, _ := netpkg.NewConfigFromMap(map[string]string{ + netpkg.ActivatorCAKey: "", + netpkg.ActivatorSANKey: "", + }) + return &config.Config{ Autoscaler: ac, Deployment: deploymentConfig, + Network: networkConfig, } } @@ -152,6 +167,12 @@ func newConfigWatcher() configmap.Watcher { Data: map[string]string{ deployment.QueueSidecarImageKey: "covid is here", }, + }, &corev1.ConfigMap{ + ObjectMeta: metav1.ObjectMeta{ + Namespace: system.Namespace(), + Name: netpkg.ConfigName, + }, + Data: map[string]string{}, }) } @@ -293,6 +314,7 @@ func TestReconcile(t *testing.T) { type deciderKey struct{} type asConfigKey struct{} + type netConfigKey struct{} retryAttempted := false @@ -1120,6 +1142,38 @@ func TestReconcile(t *testing.T) { WithPAMetricsService(privateSvc), WithObservedGeneration(1), ), }}, + }, { + Name: "we have enough burst capacity, but keep proxy mode as activator CA is enabled", + Key: key, + Ctx: context.WithValue(context.WithValue(context.Background(), netConfigKey{}, activatorCertsNetConfig()), deciderKey{}, + decider(testNamespace, testRevision, defaultScale, /* desiredScale */ + 1 /* ebc */)), + Objects: []runtime.Object{ + kpa(testNamespace, testRevision, WithPASKSReady, WithTraffic, markScaleTargetInitialized, + WithPAMetricsService(privateSvc), withScales(1, defaultScale), + WithPAStatusService(testRevision), WithObservedGeneration(1)), + defaultProxySKS, + metric(testNamespace, testRevision), + defaultDeployment, + defaultReady}, + // No update from ProxySKS. + }, { + Name: "we have enough burst capacity, but switch to keep proxy mode as activator CA is turned on", + Key: key, + Ctx: context.WithValue(context.WithValue(context.Background(), netConfigKey{}, activatorCertsNetConfig()), deciderKey{}, + decider(testNamespace, testRevision, defaultScale, /* desiredScale */ + 1 /* ebc */)), + Objects: []runtime.Object{ + kpa(testNamespace, testRevision, WithPASKSReady, WithTraffic, markScaleTargetInitialized, + WithPAMetricsService(privateSvc), withScales(1, defaultScale), + WithPAStatusService(testRevision), WithObservedGeneration(1)), + defaultSKS, + metric(testNamespace, testRevision), + defaultDeployment, + defaultReady}, + WantUpdates: []clientgotesting.UpdateActionImpl{{ + Object: defaultProxySKS, + }}, }} table.Test(t, MakeFactory(func(ctx context.Context, listers *Listers, cmw configmap.Watcher) controller.Reconciler { @@ -1145,6 +1199,9 @@ func TestReconcile(t *testing.T) { if asConfig := ctx.Value(asConfigKey{}); asConfig != nil { testConfigs.Autoscaler = asConfig.(*autoscalerconfig.Config) } + if netConfig := ctx.Value(netConfigKey{}); netConfig != nil { + testConfigs.Network = netConfig.(*netpkg.Config) + } psf := podscalable.Get(ctx) scaler := newScaler(ctx, psf, func(interface{}, time.Duration) {}) scaler.activatorProbe = func(*autoscalingv1alpha1.PodAutoscaler, http.RoundTripper) (bool, error) { return true, nil } @@ -1219,6 +1276,13 @@ func TestGlobalResyncOnUpdateAutoscalerConfigMap(t *testing.T) { deployment.QueueSidecarImageKey: "i'm on a bike", }, }) + watcher.OnChange(&corev1.ConfigMap{ + ObjectMeta: metav1.ObjectMeta{ + Name: netpkg.ConfigName, + Namespace: system.Namespace(), + }, + Data: map[string]string{}, + }) grp := errgroup.Group{} waitInformers, err := RunAndSyncInformers(ctx, informers...)