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

implement cluster-local-domain-tls in serving #14610

Merged
merged 10 commits into from
Jan 25, 2024
7 changes: 5 additions & 2 deletions cmd/activator/main.go
Original file line number Diff line number Diff line change
Expand Up @@ -167,8 +167,11 @@ func main() {
// At this moment activator with TLS does not disable HTTP.
// See also https://github.com/knative/serving/issues/12808.
if tlsEnabled {
logger.Info("Knative Internal TLS is enabled")
certCache = certificate.NewCertCache(ctx)
logger.Info("Knative system-internal-tls is enabled")
certCache, err = certificate.NewCertCache(ctx)
if err != nil {
logger.Fatalw("Failed to create certificate cache", zap.Error(err))
}
transport = pkgnet.NewProxyAutoTLSTransport(env.MaxIdleProxyConns, env.MaxIdleProxyConnsPerHost, certCache.TLSContext())
}

Expand Down
13 changes: 1 addition & 12 deletions cmd/controller/main.go
Original file line number Diff line number Diff line change
Expand Up @@ -20,7 +20,6 @@ import (
// The set of controllers this controller process runs.
"flag"

certificate "knative.dev/networking/pkg/certificates/reconciler"
"knative.dev/pkg/reconciler"
"knative.dev/pkg/signals"
"knative.dev/serving/pkg/reconciler/configuration"
Expand All @@ -32,18 +31,11 @@ import (
"knative.dev/serving/pkg/reconciler/serverlessservice"
"knative.dev/serving/pkg/reconciler/service"

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

const (
secretLabelNamePostfix = "-ctrl"
)

var ctors = []injection.ControllerConstructor{
configuration.NewController,
labeler.NewController,
Expand All @@ -53,7 +45,6 @@ var ctors = []injection.ControllerConstructor{
service.NewController,
gc.NewController,
nscert.NewController,
certificate.NewControllerFactory(networking.ServingCertName),
domainmapping.NewController,
}

Expand All @@ -62,7 +53,5 @@ func main() {
"reconciliation-timeout", reconciler.DefaultTimeout,
"The amount of time to give each reconciliation of a resource to complete before its context is canceled.")

labelName := networking.ServingCertName + secretLabelNamePostfix
ctx := filteredFactory.WithSelectors(signals.NewContext(), labelName)
sharedmain.MainWithContext(ctx, "controller", ctors...)
sharedmain.MainWithContext(signals.NewContext(), "controller", ctors...)
}
14 changes: 14 additions & 0 deletions config/core/300-knativecertificate.yaml
Original file line number Diff line number Diff line change
@@ -0,0 +1,14 @@
apiVersion: networking.internal.knative.dev/v1alpha1
kind: Certificate
metadata:
annotations:
networking.knative.dev/certificate.class: cert-manager.certificate.networking.knative.dev
labels:
networking.knative.dev/certificate-type: system-internal
name: routing-serving-certs
namespace: knative-serving
spec:
dnsNames:
- kn-routing
- data-plane.knative.dev # for reverse-compatibility with net-* implementations that do not work with multi-SANs
secretName: routing-serving-certs
47 changes: 0 additions & 47 deletions config/core/300-secret.yaml

This file was deleted.

11 changes: 6 additions & 5 deletions pkg/activator/certificate/cache.go
Original file line number Diff line number Diff line change
Expand Up @@ -21,6 +21,7 @@ import (
"crypto/tls"
"crypto/x509"
"encoding/pem"
"fmt"
"sync"

"go.uber.org/zap"
Expand All @@ -47,8 +48,8 @@ type CertCache struct {
certificatesMux sync.RWMutex
}

// NewCertCache starts secretInformer.
func NewCertCache(ctx context.Context) *CertCache {
// NewCertCache creates and starts the certificate cache that watches Activators certificate.
func NewCertCache(ctx context.Context) (*CertCache, error) {
secretInformer := secretinformer.Get(ctx)

cr := &CertCache{
Expand All @@ -58,8 +59,8 @@ func NewCertCache(ctx context.Context) *CertCache {

secret, err := cr.secretInformer.Lister().Secrets(system.Namespace()).Get(netcfg.ServingRoutingCertName)
if err != nil {
cr.logger.Warnw("failed to get secret", zap.Error(err))
return nil
return nil, fmt.Errorf("failed to get activator certificate, secret %s/%s was not found: %w. Enabling system-internal-tls requires the secret to be present and populated with a valid certificate and CA",
system.Namespace(), netcfg.ServingRoutingCertName, err)
}

cr.updateCache(secret)
Expand All @@ -72,7 +73,7 @@ func NewCertCache(ctx context.Context) *CertCache {
},
})

return cr
return cr, nil
}

func (cr *CertCache) handleCertificateAdd(added interface{}) {
Expand Down
5 changes: 0 additions & 5 deletions pkg/apis/serving/v1/route_lifecycle.go
Original file line number Diff line number Diff line change
Expand Up @@ -195,11 +195,6 @@ const (
// RouteConditionCertificateProvisioned condition when it is set to True
// because external-domain-tls was not enabled.
ExternalDomainTLSNotEnabledMessage = "external-domain-tls is not enabled"

// TLSNotEnabledForClusterLocalMessage is the message which is set on the
// RouteConditionCertificateProvisioned condition when it is set to True
// because the domain is cluster-local.
TLSNotEnabledForClusterLocalMessage = "TLS is not enabled for cluster-local"
)

// MarkTLSNotEnabled sets RouteConditionCertificateProvisioned to true when
Expand Down
41 changes: 34 additions & 7 deletions pkg/queue/certificate/watcher_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -17,9 +17,15 @@ limitations under the License.
package certificate

import (
"bytes"
"context"
"crypto/rand"
"crypto/rsa"
"crypto/tls"
"crypto/x509"
"crypto/x509/pkix"
"encoding/pem"
"math/big"
"os"
"testing"
"time"
Expand Down Expand Up @@ -94,26 +100,47 @@ func TestCertificateRotation(t *testing.T) {
}

func createAndSaveCertificate(san, dir string) error {
ca, err := certificates.CreateCACerts(1 * time.Hour)
if err != nil {
return err
cert := &x509.Certificate{
SerialNumber: big.NewInt(2019),
Subject: pkix.Name{
dprotaso marked this conversation as resolved.
Show resolved Hide resolved
Organization: []string{"Knative"},
},
NotBefore: time.Now(),
NotAfter: time.Now().AddDate(10, 0, 0),
IsCA: false,
ExtKeyUsage: []x509.ExtKeyUsage{x509.ExtKeyUsageClientAuth, x509.ExtKeyUsageServerAuth},
KeyUsage: x509.KeyUsageDigitalSignature | x509.KeyUsageCertSign,
BasicConstraintsValid: true,
}
cert.DNSNames = []string{san}

caCert, caKey, err := ca.Parse()
pk, err := rsa.GenerateKey(rand.Reader, 4096)
if err != nil {
return err
}

cert, err := certificates.CreateCert(caKey, caCert, 1*time.Hour, san)
certBytes, err := x509.CreateCertificate(rand.Reader, cert, cert, &pk.PublicKey, pk)
if err != nil {
return err
}

if err := os.WriteFile(dir+"/"+certificates.CertName, cert.CertBytes(), 0644); err != nil {
caPEM := new(bytes.Buffer)
pem.Encode(caPEM, &pem.Block{
Type: "CERTIFICATE",
Bytes: certBytes,
})

caPrivKeyPEM := new(bytes.Buffer)
pem.Encode(caPrivKeyPEM, &pem.Block{
Type: "RSA PRIVATE KEY",
Bytes: x509.MarshalPKCS1PrivateKey(pk),
})

if err := os.WriteFile(dir+"/"+certificates.CertName, caPEM.Bytes(), 0644); err != nil {
return err
}

return os.WriteFile(dir+"/"+certificates.PrivateKeyName, cert.PrivateKeyBytes(), 0644)
return os.WriteFile(dir+"/"+certificates.PrivateKeyName, caPrivKeyPEM.Bytes(), 0644)
}

func getSAN(c *tls.Certificate) (string, error) {
Expand Down
7 changes: 6 additions & 1 deletion pkg/reconciler/accessor/networking/certificate.go
Original file line number Diff line number Diff line change
Expand Up @@ -63,10 +63,15 @@ func ReconcileCertificate(ctx context.Context, owner kmeta.Accessor, desired *v1
return nil, kaccessor.NewAccessorError(
fmt.Errorf("owner: %s with Type %T does not own Certificate: %q", owner.GetName(), owner, cert.Name),
kaccessor.NotOwnResource)
} else if !equality.Semantic.DeepEqual(cert.Spec, desired.Spec) {
} else if !equality.Semantic.DeepEqual(cert.Spec, desired.Spec) ||
!equality.Semantic.DeepEqual(cert.Annotations, desired.Annotations) ||
!equality.Semantic.DeepEqual(cert.Labels, desired.Labels) {

// Don't modify the informers copy
existing := cert.DeepCopy()
existing.Spec = desired.Spec
existing.Annotations = desired.Annotations
existing.Labels = desired.Labels
cert, err = certAccessor.GetNetworkingClient().NetworkingV1alpha1().Certificates(existing.Namespace).Update(ctx, existing, metav1.UpdateOptions{})
if err != nil {
recorder.Eventf(owner, corev1.EventTypeWarning, "UpdateFailed",
Expand Down
8 changes: 5 additions & 3 deletions pkg/reconciler/domainmapping/resources/certificate_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -22,14 +22,14 @@ import (
"github.com/google/go-cmp/cmp"
corev1 "k8s.io/api/core/v1"
metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
"knative.dev/networking/pkg/config"

"knative.dev/networking/pkg/apis/networking"
networkingv1alpha1 "knative.dev/networking/pkg/apis/networking/v1alpha1"
duckv1 "knative.dev/pkg/apis/duck/v1"
"knative.dev/pkg/kmeta"
"knative.dev/serving/pkg/apis/serving"
"knative.dev/serving/pkg/apis/serving/v1beta1"
// . "knative.dev/serving/pkg/testing/v1"
)

func TestMakeCertificate(t *testing.T) {
Expand Down Expand Up @@ -58,7 +58,8 @@ func TestMakeCertificate(t *testing.T) {
Namespace: "the-namespace",
Annotations: map[string]string{networking.CertificateClassAnnotationKey: certClass},
Labels: map[string]string{
serving.DomainMappingUIDLabelKey: "mapping.com",
serving.DomainMappingUIDLabelKey: "mapping.com",
networking.CertificateTypeLabelKey: string(config.CertificateExternalDomain),
},
},
Spec: networkingv1alpha1.CertificateSpec{
Expand Down Expand Up @@ -96,7 +97,8 @@ func TestMakeCertificate(t *testing.T) {
"others": "kept",
},
Labels: map[string]string{
serving.DomainMappingUIDLabelKey: "mapping.com",
serving.DomainMappingUIDLabelKey: "mapping.com",
networking.CertificateTypeLabelKey: string(config.CertificateExternalDomain),
},
},
Spec: networkingv1alpha1.CertificateSpec{
Expand Down
2 changes: 2 additions & 0 deletions pkg/reconciler/domainmapping/table_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -967,6 +967,7 @@ func TestReconcileTLSEnabled(t *testing.T) {
},
Labels: map[string]string{
serving.DomainMappingUIDLabelKey: "becomes.ready.run",
netapi.CertificateTypeLabelKey: string(netcfg.CertificateExternalDomain),
Copy link
Member

Choose a reason for hiding this comment

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

Should we assume not having the label means it's external?

Copy link
Member Author

Choose a reason for hiding this comment

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

We can, I thought this is more explicit. I don't have a strong opinion on which is better.

},
},
Spec: netv1alpha1.CertificateSpec{
Expand Down Expand Up @@ -1114,6 +1115,7 @@ func TestReconcileTLSEnabled(t *testing.T) {
},
Labels: map[string]string{
serving.DomainMappingUIDLabelKey: "challenged.com",
netapi.CertificateTypeLabelKey: string(netcfg.CertificateExternalDomain),
},
},
Spec: netv1alpha1.CertificateSpec{
Expand Down
23 changes: 20 additions & 3 deletions pkg/reconciler/revision/controller.go
Original file line number Diff line number Diff line change
Expand Up @@ -26,6 +26,9 @@ import (
"golang.org/x/time/rate"
cachingclient "knative.dev/caching/pkg/client/injection/client"
imageinformer "knative.dev/caching/pkg/client/injection/informers/caching/v1alpha1/image"
"knative.dev/networking/pkg/apis/networking/v1alpha1"
networkingclient "knative.dev/networking/pkg/client/injection/client"
certificateinformer "knative.dev/networking/pkg/client/injection/informers/networking/v1alpha1/certificate"
"knative.dev/pkg/changeset"
kubeclient "knative.dev/pkg/client/injection/kube/client"
deploymentinformer "knative.dev/pkg/client/injection/kube/informers/apps/v1/deployment"
Expand Down Expand Up @@ -73,15 +76,18 @@ func newControllerWithOptions(
deploymentInformer := deploymentinformer.Get(ctx)
imageInformer := imageinformer.Get(ctx)
paInformer := painformer.Get(ctx)
certificateInformer := certificateinformer.Get(ctx)

c := &Reconciler{
kubeclient: kubeclient.Get(ctx),
client: servingclient.Get(ctx),
cachingclient: cachingclient.Get(ctx),
kubeclient: kubeclient.Get(ctx),
client: servingclient.Get(ctx),
networkingclient: networkingclient.Get(ctx),
cachingclient: cachingclient.Get(ctx),

podAutoscalerLister: paInformer.Lister(),
imageLister: imageInformer.Lister(),
deploymentLister: deploymentInformer.Lister(),
certificateLister: certificateInformer.Lister(),
ReToCode marked this conversation as resolved.
Show resolved Hide resolved
}

impl := revisionreconciler.NewImpl(ctx, c, func(impl *controller.Impl) controller.Options {
Expand All @@ -103,6 +109,8 @@ func newControllerWithOptions(
return controller.Options{ConfigStore: configStore}
})

c.tracker = impl.Tracker

transport := http.DefaultTransport
if rt, err := newResolverTransport(k8sCertPath, digestResolutionWorkers, digestResolutionWorkers); err != nil {
logging.FromContext(ctx).Errorw("Failed to create resolver transport", zap.Error(err))
Expand Down Expand Up @@ -131,6 +139,15 @@ func newControllerWithOptions(
}
deploymentInformer.Informer().AddEventHandler(handleMatchingControllers)
paInformer.Informer().AddEventHandler(handleMatchingControllers)
certificateInformer.Informer().AddEventHandler(controller.HandleAll(
// Call the tracker's OnChanged method, but we've seen the objects
// coming through this path missing TypeMeta, so ensure it is properly
// populated.
controller.EnsureTypeMeta(
c.tracker.OnChanged,
v1alpha1.SchemeGroupVersion.WithKind("Certificate"),
),
))

// We don't watch for changes to Image because we don't incorporate any of its
// properties into our own status and should work completely in the absence of
Expand Down