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.

Sign up for GitHub

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

[SRVKS-1179] enable secret filter informer by default #2445

Merged
merged 3 commits into from
Feb 1, 2024
Merged

[SRVKS-1179] enable secret filter informer by default #2445

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
31ba063
enable secret filter informer by default
skonto Jan 16, 2024
74284aa
lint
skonto Jan 16, 2024
6dae262
address comments
skonto Jan 23, 2024
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
27 changes: 9 additions & 18 deletions openshift-knative-operator/pkg/serving/secretinformerfiltering.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,18 +8,13 @@ import (
corev1 "k8s.io/api/core/v1"
"k8s.io/apimachinery/pkg/apis/meta/v1/unstructured"
"knative.dev/networking/pkg/apis/networking"
"knative.dev/networking/pkg/config"
"knative.dev/operator/pkg/apis/operator/base"
operatorv1beta1 "knative.dev/operator/pkg/apis/operator/v1beta1"
)

const (
// TODO: Remove when available in knative.dev/networking/config
ServingInternalCertName = "knative-serving-certs"
// TODO: Maybe decide to fetch from net-kourier deps instead
EnableSecretInformerFilteringByCertUIDEnv = "ENABLE_SECRET_INFORMER_FILTERING_BY_CERT_UID"
// TODO: Annotation is deprecated, remove in future releases
secretInformerFilteringAnnotation = "serverless.openshift.io/enable-secret-informer-filtering"
)
// TODO: Maybe decide to fetch from net-kourier deps instead
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I am thinking of moving that const to knative.dev/pkg upstream or something so net-istio could benefit from it. So not eager to bring in the net-kourier dep.

Copy link
Member

Choose a reason for hiding this comment

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

Yeah, probably better to move it to pkg upstream. Do you want to create a JIRA for that. We'll probably forget the TODO here.

const EnableSecretInformerFilteringByCertUIDEnv = "ENABLE_SECRET_INFORMER_FILTERING_BY_CERT_UID"

func enableSecretInformerFilteringTransformers(ks base.KComponent) []mf.Transformer {
shouldInject := false
Expand All @@ -41,7 +36,8 @@ func enableSecretInformerFilteringTransformers(ks base.KComponent) []mf.Transfor

func injectLabelIntoInternalEncryptionSecret() mf.Transformer {
return func(u *unstructured.Unstructured) error {
if u.GetKind() == "Secret" && u.GetName() == ServingInternalCertName {
//nolint:staticcheck // ignore the deprecation until internal encryption is implemented downstream
Copy link
Contributor Author

Choose a reason for hiding this comment

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

As good as it can gets: golangci/golangci-lint#741.

if u.GetKind() == "Secret" && u.GetName() == config.ServingInternalCertName {
labels := u.GetLabels()
if labels == nil {
labels = make(map[string]string, 1)
Expand Down Expand Up @@ -74,15 +70,10 @@ func configIfUnsetAndCheckIfShouldInject(comp *operatorv1beta1.KnativeServing, d
}
}

// The annotation is deprecated
// TODO: Remove this block in future releases
if deployment == "net-istio-controller" {
if v, ok := comp.GetAnnotations()[secretInformerFilteringAnnotation]; ok {
b, _ := strconv.ParseBool(v)
// Keep the same behavior as in 1.27
return b, common.InjectEnvironmentIntoDeployment("net-istio-controller", "controller",
corev1.EnvVar{Name: EnableSecretInformerFilteringByCertUIDEnv, Value: v})
}
// TODO: remove when set to true at the net-* repos upstream/midstream
ReToCode marked this conversation as resolved.
Show resolved Hide resolved
if deployment == "net-istio-controller" || deployment == "net-kourier-controller" {
return true, common.InjectEnvironmentIntoDeployment(deployment, "controller",
corev1.EnvVar{Name: EnableSecretInformerFilteringByCertUIDEnv, Value: "true"})
}
return false, nil
}
180 changes: 16 additions & 164 deletions openshift-knative-operator/pkg/serving/secretinformerfilterting_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,12 +11,12 @@ import (

func TestSecretInformerFilteringOverride(t *testing.T) {
cases := []struct {
name string
in *operatorv1beta1.KnativeServing
expected *operatorv1beta1.KnativeServing
shouldAddLabelToSecret bool
name string
in *operatorv1beta1.KnativeServing
expected *operatorv1beta1.KnativeServing
shouldEnableSecretInformerFiltering bool
}{{
name: "by default no overrides, disabled secret filtering, with kourier enabled",
name: "by default no overrides, enabled secret informer filtering, with kourier enabled",
in: ks(func(ks *operatorv1beta1.KnativeServing) {
ks.Spec.Ingress = &operatorv1beta1.IngressConfigs{Kourier: base.KourierIngressConfiguration{
Enabled: true,
Expand All @@ -27,9 +27,9 @@ func TestSecretInformerFilteringOverride(t *testing.T) {
Enabled: true,
}}
}),
shouldAddLabelToSecret: false,
shouldEnableSecretInformerFiltering: true,
}, {
name: "by default no overrides, disabled secret filtering, with istio enabled",
name: "by default no overrides, enabled secret informer filtering, with istio enabled",
in: ks(func(ks *operatorv1beta1.KnativeServing) {
ks.Spec.Ingress = &operatorv1beta1.IngressConfigs{Istio: base.IstioIngressConfiguration{
Enabled: true,
Expand All @@ -40,71 +40,9 @@ func TestSecretInformerFilteringOverride(t *testing.T) {
Enabled: true,
}}
}),
shouldAddLabelToSecret: false,
shouldEnableSecretInformerFiltering: true,
}, {
name: "by default no overrides, deprecated annotation set to true, enabled secret filtering, istio enabled",
in: ks(func(ks *operatorv1beta1.KnativeServing) {
ks.Spec.Ingress = &operatorv1beta1.IngressConfigs{Istio: base.IstioIngressConfiguration{
Enabled: true,
}}
ks.Annotations = map[string]string{}
ks.Annotations = map[string]string{secretInformerFilteringAnnotation: "true"}
}),
expected: ks(func(ks *operatorv1beta1.KnativeServing) {
ks.Spec.Ingress = &operatorv1beta1.IngressConfigs{Istio: base.IstioIngressConfiguration{
Enabled: true,
}}
ks.Annotations = map[string]string{secretInformerFilteringAnnotation: "true"}
}),
shouldAddLabelToSecret: true,
}, {
name: "by default no overrides, istio deprecated annotation set to true but has no effect, disabled secret filtering, kourier enabled",
in: ks(func(ks *operatorv1beta1.KnativeServing) {
ks.Spec.Ingress = &operatorv1beta1.IngressConfigs{Kourier: base.KourierIngressConfiguration{
Enabled: true,
}}
ks.Annotations = map[string]string{}
ks.Annotations = map[string]string{secretInformerFilteringAnnotation: "true"}
}),
expected: ks(func(ks *operatorv1beta1.KnativeServing) {
ks.Spec.Ingress = &operatorv1beta1.IngressConfigs{Kourier: base.KourierIngressConfiguration{
Enabled: true,
}}
ks.Annotations = map[string]string{secretInformerFilteringAnnotation: "true"}
}),
shouldAddLabelToSecret: false,
}, {
name: "by default no overrides, istio deprecated annotation set to false, disabled secret filtering, istio enabled",
in: ks(func(ks *operatorv1beta1.KnativeServing) {
ks.Spec.Ingress = &operatorv1beta1.IngressConfigs{Istio: base.IstioIngressConfiguration{
Enabled: true,
}}
ks.Annotations = map[string]string{secretInformerFilteringAnnotation: "false"}
}),
expected: ks(func(ks *operatorv1beta1.KnativeServing) {
ks.Spec.Ingress = &operatorv1beta1.IngressConfigs{Istio: base.IstioIngressConfiguration{
Enabled: true,
}}
ks.Annotations = map[string]string{secretInformerFilteringAnnotation: "false"}
}),
shouldAddLabelToSecret: false,
}, {
name: "by default no overrides, istio deprecated annotation value with bad string, disabled secret filtering, istio enabled",
in: ks(func(ks *operatorv1beta1.KnativeServing) {
ks.Spec.Ingress = &operatorv1beta1.IngressConfigs{Istio: base.IstioIngressConfiguration{
Enabled: true,
}}
ks.Annotations = map[string]string{secretInformerFilteringAnnotation: "wrong"}
}),
expected: ks(func(ks *operatorv1beta1.KnativeServing) {
ks.Spec.Ingress = &operatorv1beta1.IngressConfigs{Istio: base.IstioIngressConfiguration{
Enabled: true,
}}
ks.Annotations = map[string]string{secretInformerFilteringAnnotation: "wrong"}
}),
shouldAddLabelToSecret: false,
}, {
name: "disabled secret filtering with kourier enabled",
name: "disabled secret informer filtering with kourier enabled",
in: ks(func(ks *operatorv1beta1.KnativeServing) {
ks.Spec.Ingress = &operatorv1beta1.IngressConfigs{Kourier: base.KourierIngressConfiguration{
Enabled: true,
Expand Down Expand Up @@ -143,55 +81,13 @@ func TestSecretInformerFilteringOverride(t *testing.T) {
}},
})
}),
shouldAddLabelToSecret: false,
}, {
name: "disabled secret filtering with istio enabled",
in: ks(func(ks *operatorv1beta1.KnativeServing) {
ks.Spec.Ingress = &operatorv1beta1.IngressConfigs{Istio: base.IstioIngressConfiguration{
Enabled: true,
}}
ks.Spec.DeploymentOverride = append(ks.Spec.DeploymentOverride, base.WorkloadOverride{
Name: "net-istio-controller",
Env: []base.EnvRequirementsOverride{
{
Container: "controller",
EnvVars: []corev1.EnvVar{{
Name: EnableSecretInformerFilteringByCertUIDEnv,
Value: "false",
}, {
Name: "foo",
Value: "foo",
}},
}},
})
}),
expected: ks(func(ks *operatorv1beta1.KnativeServing) {
ks.Spec.Ingress = &operatorv1beta1.IngressConfigs{Istio: base.IstioIngressConfiguration{
Enabled: true,
}}
ks.Spec.DeploymentOverride = append(ks.Spec.DeploymentOverride, base.WorkloadOverride{
Name: "net-istio-controller",
Env: []base.EnvRequirementsOverride{
{
Container: "controller",
EnvVars: []corev1.EnvVar{{
Name: EnableSecretInformerFilteringByCertUIDEnv,
Value: "false",
}, {
Name: "foo",
Value: "foo",
}},
}},
})
}),
shouldAddLabelToSecret: false,
shouldEnableSecretInformerFiltering: false,
}, {
name: "deprecated annotation is set to true but env var takes precedence, disabled secret filtering, istio enabled",
name: "disabled secret informer filtering with istio enabled",
in: ks(func(ks *operatorv1beta1.KnativeServing) {
ks.Spec.Ingress = &operatorv1beta1.IngressConfigs{Istio: base.IstioIngressConfiguration{
Enabled: true,
}}
ks.Annotations = map[string]string{secretInformerFilteringAnnotation: "true"}
ks.Spec.DeploymentOverride = append(ks.Spec.DeploymentOverride, base.WorkloadOverride{
Name: "net-istio-controller",
Env: []base.EnvRequirementsOverride{
Expand All @@ -211,7 +107,6 @@ func TestSecretInformerFilteringOverride(t *testing.T) {
ks.Spec.Ingress = &operatorv1beta1.IngressConfigs{Istio: base.IstioIngressConfiguration{
Enabled: true,
}}
ks.Annotations = map[string]string{secretInformerFilteringAnnotation: "true"}
ks.Spec.DeploymentOverride = append(ks.Spec.DeploymentOverride, base.WorkloadOverride{
Name: "net-istio-controller",
Env: []base.EnvRequirementsOverride{
Expand All @@ -227,59 +122,16 @@ func TestSecretInformerFilteringOverride(t *testing.T) {
}},
})
}),
shouldAddLabelToSecret: false,
}, {
name: "deprecated annotation is set to false but env var takes precedence, enabled secret filtering, istio enabled",
in: ks(func(ks *operatorv1beta1.KnativeServing) {
ks.Spec.Ingress = &operatorv1beta1.IngressConfigs{Istio: base.IstioIngressConfiguration{
Enabled: true,
}}
ks.Annotations = map[string]string{secretInformerFilteringAnnotation: "false"}
ks.Spec.DeploymentOverride = append(ks.Spec.DeploymentOverride, base.WorkloadOverride{
Name: "net-istio-controller",
Env: []base.EnvRequirementsOverride{
{
Container: "controller",
EnvVars: []corev1.EnvVar{{
Name: EnableSecretInformerFilteringByCertUIDEnv,
Value: "true",
}, {
Name: "foo",
Value: "foo",
}},
}},
})
}),
expected: ks(func(ks *operatorv1beta1.KnativeServing) {
ks.Spec.Ingress = &operatorv1beta1.IngressConfigs{Istio: base.IstioIngressConfiguration{
Enabled: true,
}}
ks.Annotations = map[string]string{secretInformerFilteringAnnotation: "false"}
ks.Spec.DeploymentOverride = append(ks.Spec.DeploymentOverride, base.WorkloadOverride{
Name: "net-istio-controller",
Env: []base.EnvRequirementsOverride{
{
Container: "controller",
EnvVars: []corev1.EnvVar{{
Name: EnableSecretInformerFilteringByCertUIDEnv,
Value: "true",
}, {
Name: "foo",
Value: "foo",
}},
}},
})
}),
shouldAddLabelToSecret: false,
shouldEnableSecretInformerFiltering: false,
}}

for _, c := range cases {
t.Run(c.name, func(t *testing.T) {
tf := enableSecretInformerFilteringTransformers(c.in)
if c.shouldAddLabelToSecret {
if tf == nil {
t.Errorf("Secret transformer should not be nil")
}
if c.shouldEnableSecretInformerFiltering && tf == nil {
t.Errorf("Secret informer filtering transformer should not be nil")
} else if !c.shouldEnableSecretInformerFiltering && tf != nil {
t.Errorf("Secret informer filtering transformer should be nil")
}
if !cmp.Equal(c.in, c.expected) {
t.Errorf("Resource was not as expected:\n%s", cmp.Diff(c.in, c.expected))
Expand Down
Loading