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

[WIP] Add filtering label to secret and enable secret informer filtering by default #1084

Closed
wants to merge 11 commits into from
Closed
Show file tree
Hide file tree
Changes from all commits
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
5 changes: 3 additions & 2 deletions cmd/kourier/main.go
Original file line number Diff line number Diff line change
Expand Up @@ -18,16 +18,17 @@ package main

import (
"knative.dev/net-kourier/pkg/config"
"knative.dev/net-kourier/pkg/reconciler/informerfiltering"
kourierIngressController "knative.dev/net-kourier/pkg/reconciler/ingress"
"knative.dev/networking/pkg/apis/networking"
"knative.dev/pkg/signals"

// This defines the shared main for injected controllers.
filteredFactory "knative.dev/pkg/client/injection/kube/informers/factory/filtered"
"knative.dev/pkg/injection/sharedmain"
)

func main() {
ctx := informerfiltering.GetContextWithFilteringLabelSelector(signals.NewContext())
ctx := filteredFactory.WithSelectors(signals.NewContext(), networking.CertificateUIDLabelKey)
ctx = sharedmain.WithHealthProbesDisabled(ctx)
sharedmain.MainWithContext(ctx, config.ControllerName, kourierIngressController.NewController)
}
5 changes: 4 additions & 1 deletion config/200-serviceaccount.yaml
Original file line number Diff line number Diff line change
Expand Up @@ -37,8 +37,11 @@ rules:
resources: ["events"]
verbs: ["create", "update", "patch"]
- apiGroups: [""]
resources: ["pods", "endpoints", "services", "secrets"]
resources: ["pods", "endpoints", "services"]
verbs: ["get", "list", "watch"]
- apiGroups: [""]
resources: ["secrets"]
verbs: ["get", "list", "watch", "update"]
- apiGroups: [""]
resources: ["configmaps"]
verbs: [ "get", "list", "watch" ]
Expand Down
2 changes: 0 additions & 2 deletions config/300-controller.yaml
Original file line number Diff line number Diff line change
Expand Up @@ -57,8 +57,6 @@ spec:
value: "knative.dev/samples"
- name: KOURIER_GATEWAY_NAMESPACE
value: "kourier-system"
- name: ENABLE_SECRET_INFORMER_FILTERING_BY_CERT_UID
value: "false"
# KUBE_API_BURST and KUBE_API_QPS allows to configure maximum burst for throttle and maximum QPS to the server from the client.
# Setting these values using env vars is possible since https://github.com/knative/pkg/pull/2755.
# 200 is an arbitrary value, but it speeds up kourier startup duration, and the whole ingress reconciliation process as a whole.
Expand Down
29 changes: 28 additions & 1 deletion pkg/generator/ingress_translator.go
Original file line number Diff line number Diff line change
Expand Up @@ -34,10 +34,13 @@ import (
"google.golang.org/protobuf/types/known/anypb"
corev1 "k8s.io/api/core/v1"
apierrors "k8s.io/apimachinery/pkg/api/errors"
metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
"k8s.io/apimachinery/pkg/types"
kubeclient "k8s.io/client-go/kubernetes"
pkgconfig "knative.dev/net-kourier/pkg/config"
envoy "knative.dev/net-kourier/pkg/envoy/api"
"knative.dev/net-kourier/pkg/reconciler/ingress/config"
"knative.dev/networking/pkg/apis/networking"
"knative.dev/networking/pkg/apis/networking/v1alpha1"
"knative.dev/networking/pkg/certificates"
netconfig "knative.dev/networking/pkg/config"
Expand All @@ -61,6 +64,7 @@ type IngressTranslator struct {
endpointsGetter func(ns, name string) (*corev1.Endpoints, error)
serviceGetter func(ns, name string) (*corev1.Service, error)
namespaceGetter func(name string) (*corev1.Namespace, error)
kubeClient kubeclient.Interface
tracker tracker.Interface
}

Expand All @@ -69,12 +73,14 @@ func NewIngressTranslator(
endpointsGetter func(ns, name string) (*corev1.Endpoints, error),
serviceGetter func(ns, name string) (*corev1.Service, error),
namespaceGetter func(name string) (*corev1.Namespace, error),
kubeClient kubeclient.Interface,
tracker tracker.Interface) IngressTranslator {
return IngressTranslator{
secretGetter: secretGetter,
endpointsGetter: endpointsGetter,
serviceGetter: serviceGetter,
namespaceGetter: namespaceGetter,
kubeClient: kubeClient,
tracker: tracker,
}
}
Expand All @@ -89,10 +95,31 @@ func (translator *IngressTranslator) translateIngress(ctx context.Context, ingre
}

secret, err := translator.secretGetter(ingressTLS.SecretNamespace, ingressTLS.SecretName)
if err != nil {
if apierrors.IsNotFound(err) {
Copy link
Contributor

@skonto skonto Aug 4, 2023

Choose a reason for hiding this comment

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

So we are trying to make the UX easier, non-breaking and we add the label ourselves.
Are there any other secrets that might need the same?

Copy link
Contributor Author

@nak3 nak3 Aug 4, 2023

Choose a reason for hiding this comment

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

Other secrets are deployed by us and so we have already added the label like https://github.com/knative/serving/blob/main/config/core/300-secret.yaml#L34 by default. So we don't need it.

// As secret does not have a CertificateUIDLabel for the first time, informer cannot get the secret.
// Try to use k8s client to get the secret. It may have some cost but it happens only once when a new secret is specified.
secret, err = translator.kubeClient.CoreV1().Secrets(ingressTLS.SecretNamespace).Get(ctx, ingressTLS.SecretName, metav1.GetOptions{})
if err != nil {
return nil, fmt.Errorf("failed to get secret: %w", err)
}
} else if err != nil {
return nil, fmt.Errorf("failed to fetch secret: %w", err)
}

if secret.Labels == nil || secret.Labels[networking.CertificateUIDLabelKey] == "" {
// Don't modify the informers copy
existing := secret.DeepCopy()
if existing.Labels == nil {
existing.Labels = make(map[string]string)
}
existing.Labels[networking.CertificateUIDLabelKey] = ingressTLS.SecretName
secret, err = translator.kubeClient.CoreV1().Secrets(ingressTLS.SecretNamespace).Update(ctx, existing, metav1.UpdateOptions{})
if err != nil {
return nil, fmt.Errorf("failed to update secret: %w", err)
}

}

// Validate certificate here as these are defined by users.
// We should not send Gateway without validation.
_, err = tls.X509KeyPair(
Expand Down
5 changes: 5 additions & 0 deletions pkg/generator/ingress_translator_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -673,6 +673,7 @@ func TestIngressTranslator(t *testing.T) {
func(name string) (*corev1.Namespace, error) {
return kubeclient.CoreV1().Namespaces().Get(ctx, name, metav1.GetOptions{})
},
kubeclient,
&pkgtest.FakeTracker{},
)

Expand Down Expand Up @@ -885,6 +886,7 @@ func TestIngressTranslatorWithHTTPOptionDisabled(t *testing.T) {
func(name string) (*corev1.Namespace, error) {
return kubeclient.CoreV1().Namespaces().Get(ctx, name, metav1.GetOptions{})
},
kubeclient,
&pkgtest.FakeTracker{},
)

Expand Down Expand Up @@ -1211,6 +1213,7 @@ func TestIngressTranslatorWithUpstreamTLS(t *testing.T) {
func(name string) (*corev1.Namespace, error) {
return kubeclient.CoreV1().Namespaces().Get(ctx, name, metav1.GetOptions{})
},
kubeclient,
&pkgtest.FakeTracker{},
)

Expand Down Expand Up @@ -1312,6 +1315,7 @@ func TestIngressTranslatorHTTP01Challenge(t *testing.T) {
func(name string) (*corev1.Namespace, error) {
return kubeclient.CoreV1().Namespaces().Get(ctx, name, metav1.GetOptions{})
},
kubeclient,
&pkgtest.FakeTracker{},
)

Expand Down Expand Up @@ -1424,6 +1428,7 @@ func TestIngressTranslatorDomainMappingDisableHTTP2(t *testing.T) {
func(name string) (*corev1.Namespace, error) {
return kubeclient.CoreV1().Namespaces().Get(ctx, name, metav1.GetOptions{})
},
kubeclient,
&pkgtest.FakeTracker{},
)

Expand Down
47 changes: 0 additions & 47 deletions pkg/reconciler/informerfiltering/util.go

This file was deleted.

2 changes: 2 additions & 0 deletions pkg/reconciler/ingress/controller.go
Original file line number Diff line number Diff line change
Expand Up @@ -190,6 +190,7 @@ func NewController(ctx context.Context, cmw configmap.Watcher) *controller.Impl
func(name string) (*corev1.Namespace, error) {
return namespaceInformer.Lister().Get(name)
},
kubernetesClient,
impl.Tracker)
r.ingressTranslator = &ingressTranslator

Expand Down Expand Up @@ -223,6 +224,7 @@ func NewController(ctx context.Context, cmw configmap.Watcher) *controller.Impl
func(name string) (*corev1.Namespace, error) {
return namespaceInformer.Lister().Get(name)
},
kubernetesClient,
impl.Tracker)

for _, ingress := range ingressesToSync {
Expand Down
Loading