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

Force activator always in path when activator CA is specified #12790

Merged
merged 3 commits into from
Apr 2, 2022
Merged
Show file tree
Hide file tree
Changes from 1 commit
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
4 changes: 4 additions & 0 deletions pkg/reconciler/autoscaling/config/store.go
Original file line number Diff line number Diff line change
Expand Up @@ -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"
Expand All @@ -31,6 +32,7 @@ type cfgKey struct{}
type Config struct {
Autoscaler *autoscalerconfig.Config
Deployment *deployment.Config
Network *network.Config
}

// FromContext fetch config from context.
Expand Down Expand Up @@ -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...,
),
Expand All @@ -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(),
}
}
15 changes: 14 additions & 1 deletion pkg/reconciler/autoscaling/config/store_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -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"
Expand All @@ -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)
Expand All @@ -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) {
Expand All @@ -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")
}
}
8 changes: 8 additions & 0 deletions pkg/reconciler/autoscaling/hpa/hpa_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -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"
Expand Down Expand Up @@ -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...)
Expand Down
6 changes: 5 additions & 1 deletion pkg/reconciler/autoscaling/kpa/kpa.go
Original file line number Diff line number Diff line change
Expand Up @@ -131,7 +131,11 @@ 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() {
if len(config.FromContext(ctx).Network.ActivatorCA) > 0 {
// When activator CA is enabled, foce activator always in path.
nak3 marked this conversation as resolved.
Show resolved Hide resolved
// See: https://github.com/knative/serving/issues/11906
mode = nv1alpha1.SKSOperationModeProxy
} else if want == 0 || decider.Status.ExcessBurstCapacity < 0 || want == scaleUnknown && pa.Status.IsInactive() {
mode = nv1alpha1.SKSOperationModeProxy
}

Expand Down
64 changes: 64 additions & 0 deletions pkg/reconciler/autoscaling/kpa/kpa_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -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"
Expand Down Expand Up @@ -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,
}
}

Expand All @@ -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{},
})
}

Expand Down Expand Up @@ -293,6 +314,7 @@ func TestReconcile(t *testing.T) {

type deciderKey struct{}
type asConfigKey struct{}
type netConfigKey struct{}

retryAttempted := false

Expand Down Expand Up @@ -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 {
Expand All @@ -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 }
Expand Down Expand Up @@ -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...)
Expand Down