-
Notifications
You must be signed in to change notification settings - Fork 1.2k
Add reconciler logic for DomainMapping AutoTLS #10467
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
Changes from all commits
5d73642
c674cdf
c48e0dd
76b3036
dd9e65d
258644c
4ea8346
6220801
8e8b427
5593262
1aa845b
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -19,8 +19,13 @@ package domainmapping | |
| import ( | ||
| "context" | ||
| "fmt" | ||
| "sort" | ||
| "strconv" | ||
| "strings" | ||
|
|
||
| kaccessor "knative.dev/serving/pkg/reconciler/accessor" | ||
| networkaccessor "knative.dev/serving/pkg/reconciler/accessor/networking" | ||
|
|
||
| corev1 "k8s.io/api/core/v1" | ||
| "k8s.io/apimachinery/pkg/api/equality" | ||
| apierrs "k8s.io/apimachinery/pkg/api/errors" | ||
|
|
@@ -37,30 +42,43 @@ import ( | |
| "knative.dev/pkg/network" | ||
| "knative.dev/pkg/reconciler" | ||
| "knative.dev/pkg/resolver" | ||
| v1 "knative.dev/serving/pkg/apis/serving/v1" | ||
| "knative.dev/serving/pkg/apis/serving/v1alpha1" | ||
| domainmappingreconciler "knative.dev/serving/pkg/client/injection/reconciler/serving/v1alpha1/domainmapping" | ||
| "knative.dev/serving/pkg/reconciler/domainmapping/config" | ||
| "knative.dev/serving/pkg/reconciler/domainmapping/resources" | ||
| routeresources "knative.dev/serving/pkg/reconciler/route/resources" | ||
| ) | ||
|
|
||
| // Reconciler implements controller.Reconciler for DomainMapping resources. | ||
| type Reconciler struct { | ||
| ingressLister networkinglisters.IngressLister | ||
| netclient netclientset.Interface | ||
| resolver *resolver.URIResolver | ||
| certificateLister networkinglisters.CertificateLister | ||
| ingressLister networkinglisters.IngressLister | ||
| netclient netclientset.Interface | ||
| resolver *resolver.URIResolver | ||
| } | ||
|
|
||
| // Check that our Reconciler implements Interface | ||
| var _ domainmappingreconciler.Interface = (*Reconciler)(nil) | ||
|
|
||
| // Check that our Reconciler implements CertificateAccessor | ||
| var _ networkaccessor.CertificateAccessor = (*Reconciler)(nil) | ||
|
|
||
| // GetNetworkingClient implements networking.CertificateAccessor | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. this is unused?
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I don't see a call to GetNetworkingClient there.
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Hm, it's passed down... not sure we should do pass reconciler down. Rather we should pass down the networking client.
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I can follow up with a change to fix route and domainmapping reconciler both. |
||
| func (r *Reconciler) GetNetworkingClient() netclientset.Interface { | ||
| return r.netclient | ||
| } | ||
|
|
||
| // GetCertificateLister implements networking.CertificateAccessor | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. ditto?
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. |
||
| func (r *Reconciler) GetCertificateLister() networkinglisters.CertificateLister { | ||
| return r.certificateLister | ||
| } | ||
|
|
||
| // ReconcileKind implements Interface.ReconcileKind. | ||
| func (r *Reconciler) ReconcileKind(ctx context.Context, dm *v1alpha1.DomainMapping) reconciler.Event { | ||
| logger := logging.FromContext(ctx) | ||
| logger.Debugf("Reconciling DomainMapping %s/%s", dm.Namespace, dm.Name) | ||
|
|
||
| // TODO(https://github.com/knative/serving/issues/10247) | ||
| dm.Status.MarkTLSNotEnabled("AutoTLS for DomainMapping is not implemented") | ||
|
|
||
| // Defensively assume the ingress is not configured until we manage to | ||
| // successfully reconcile it below. This avoids error cases where we fail | ||
| // before we've reconciled the ingress and get a new ObservedGeneration but | ||
|
|
@@ -74,6 +92,11 @@ func (r *Reconciler) ReconcileKind(ctx context.Context, dm *v1alpha1.DomainMappi | |
| dm.Status.URL = url | ||
| dm.Status.Address = &duckv1.Addressable{URL: url} | ||
|
|
||
| tls, acmeChallenges, err := r.tls(ctx, dm) | ||
| if err != nil { | ||
| return err | ||
| } | ||
|
|
||
| // IngressClass can be set via annotations or in the config map. | ||
| ingressClass := dm.Annotations[networking.IngressClassAnnotationKey] | ||
| if ingressClass == "" { | ||
|
|
@@ -94,7 +117,7 @@ func (r *Reconciler) ReconcileKind(ctx context.Context, dm *v1alpha1.DomainMappi | |
|
|
||
| // Reconcile the Ingress resource corresponding to the requested Mapping. | ||
| logger.Debugf("Mapping %s to ref %s/%s (host: %q, svc: %q)", url, dm.Spec.Ref.Namespace, dm.Spec.Ref.Name, targetHost, targetBackendSvc) | ||
| desired := resources.MakeIngress(dm, targetBackendSvc, targetHost, ingressClass, nil /* tls */) | ||
| desired := resources.MakeIngress(dm, targetBackendSvc, targetHost, ingressClass, tls, acmeChallenges...) | ||
| ingress, err := r.reconcileIngress(ctx, dm, desired) | ||
| if err != nil { | ||
| return err | ||
|
|
@@ -110,6 +133,64 @@ func (r *Reconciler) ReconcileKind(ctx context.Context, dm *v1alpha1.DomainMappi | |
| return err | ||
| } | ||
|
|
||
| func autoTLSEnabled(ctx context.Context, dm *v1alpha1.DomainMapping) bool { | ||
| if !config.FromContext(ctx).Network.AutoTLS { | ||
| return false | ||
| } | ||
| annotationValue := dm.Annotations[networking.DisableAutoTLSAnnotationKey] | ||
| disabledByAnnotation, err := strconv.ParseBool(annotationValue) | ||
| if annotationValue != "" && err != nil { | ||
| logger := logging.FromContext(ctx) | ||
| // Validation should've caught an invalid value here. | ||
| // If we have one anyway, assume not disabled and log a warning. | ||
| logger.Warnf("DM.Annotations[%s] = %q is invalid", | ||
| networking.DisableAutoTLSAnnotationKey, annotationValue) | ||
| } | ||
|
|
||
| return !disabledByAnnotation | ||
| } | ||
|
|
||
| func certClass(ctx context.Context) string { | ||
| return config.FromContext(ctx).Network.DefaultCertificateClass | ||
| } | ||
|
|
||
| func (r *Reconciler) tls(ctx context.Context, dm *v1alpha1.DomainMapping) ([]netv1alpha1.IngressTLS, []netv1alpha1.HTTP01Challenge, error) { | ||
| if !autoTLSEnabled(ctx, dm) { | ||
| dm.Status.MarkTLSNotEnabled(v1.AutoTLSNotEnabledMessage) | ||
| return nil, nil, nil | ||
| } | ||
|
|
||
| acmeChallenges := []netv1alpha1.HTTP01Challenge{} | ||
| desiredCert := resources.MakeCertificate(dm, certClass(ctx)) | ||
| cert, err := networkaccessor.ReconcileCertificate(ctx, dm, desiredCert, r) | ||
| if err != nil { | ||
| if kaccessor.IsNotOwned(err) { | ||
| dm.Status.MarkCertificateNotOwned(desiredCert.Name) | ||
| } else { | ||
| dm.Status.MarkCertificateProvisionFailed(desiredCert.Name) | ||
| } | ||
| return nil, nil, err | ||
| } | ||
|
|
||
| for _, dnsName := range desiredCert.Spec.DNSNames { | ||
| if dnsName == dm.Name { | ||
| dm.Status.URL.Scheme = "https" | ||
| break | ||
| } | ||
| } | ||
| if cert.IsReady() { | ||
| dm.Status.MarkCertificateReady(cert.Name) | ||
| return []netv1alpha1.IngressTLS{routeresources.MakeIngressTLS(cert, desiredCert.Spec.DNSNames)}, nil, nil | ||
| } | ||
| acmeChallenges = append(acmeChallenges, cert.Status.HTTP01Challenges...) | ||
| dm.Status.MarkCertificateNotReady(cert.Name) | ||
|
|
||
| sort.Slice(acmeChallenges, func(i, j int) bool { | ||
tcnghia marked this conversation as resolved.
Show resolved
Hide resolved
|
||
| return acmeChallenges[i].URL.String() < acmeChallenges[j].URL.String() | ||
| }) | ||
| return nil, acmeChallenges, nil | ||
| } | ||
|
|
||
| func (r *Reconciler) reconcileIngress(ctx context.Context, dm *v1alpha1.DomainMapping, desired *netv1alpha1.Ingress) (*netv1alpha1.Ingress, error) { | ||
| recorder := controller.GetEventRecorder(ctx) | ||
| ingress, err := r.ingressLister.Ingresses(desired.Namespace).Get(desired.Name) | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,89 @@ | ||
| /* | ||
| Copyright 2021 The Knative Authors | ||
|
|
||
| Licensed under the Apache License, Version 2.0 (the "License"); | ||
| you may not use this file except in compliance with the License. | ||
| You may obtain a copy of the License at | ||
|
|
||
| http://www.apache.org/licenses/LICENSE-2.0 | ||
|
|
||
| Unless required by applicable law or agreed to in writing, software | ||
| distributed under the License is distributed on an "AS IS" BASIS, | ||
| WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. | ||
| See the License for the specific language governing permissions and | ||
| limitations under the License. | ||
| */ | ||
|
|
||
| package domainmapping | ||
|
|
||
| import ( | ||
| "context" | ||
| "testing" | ||
|
|
||
| network "knative.dev/networking/pkg" | ||
| "knative.dev/networking/pkg/apis/networking" | ||
| "knative.dev/serving/pkg/reconciler/domainmapping/config" | ||
| ) | ||
|
|
||
| func TestAutoTLSEnabled(t *testing.T) { | ||
| dm := domainMapping("test-ns", "test-route") | ||
|
|
||
| for _, tc := range []struct { | ||
| name string | ||
| configAutoTLSEnabled bool | ||
| tlsDisabledAnnotation string | ||
| wantAutoTLSEnabled bool | ||
| }{{ | ||
| name: "AutoTLS enabled by config, not disabled by annotation", | ||
| configAutoTLSEnabled: true, | ||
| wantAutoTLSEnabled: true, | ||
| }, { | ||
| name: "AutoTLS enabled by config, disabled by annotation", | ||
| configAutoTLSEnabled: true, | ||
| tlsDisabledAnnotation: "true", | ||
| wantAutoTLSEnabled: false, | ||
| }, { | ||
| name: "AutoTLS disabled by config, not disabled by annotation", | ||
| configAutoTLSEnabled: false, | ||
| wantAutoTLSEnabled: false, | ||
| }, { | ||
| name: "AutoTLS disabled by config, disabled by annotation", | ||
| configAutoTLSEnabled: false, | ||
| tlsDisabledAnnotation: "true", | ||
| wantAutoTLSEnabled: false, | ||
| }, { | ||
| name: "AutoTLS enabled by config, invalid annotation", | ||
| configAutoTLSEnabled: true, | ||
| tlsDisabledAnnotation: "foo", | ||
| wantAutoTLSEnabled: true, | ||
| }, { | ||
| name: "AutoTLS disabled by config, invalid annotation", | ||
| configAutoTLSEnabled: false, | ||
| tlsDisabledAnnotation: "foo", | ||
| wantAutoTLSEnabled: false, | ||
| }, { | ||
| name: "AutoTLS disabled by config nil annotations", | ||
| configAutoTLSEnabled: false, | ||
| wantAutoTLSEnabled: false, | ||
| }, { | ||
| name: "AutoTLS enabled by config, nil annotations", | ||
| configAutoTLSEnabled: true, | ||
| wantAutoTLSEnabled: true, | ||
| }} { | ||
| t.Run(tc.name, func(t *testing.T) { | ||
| ctx := config.ToContext(context.Background(), &config.Config{ | ||
| Network: &network.Config{ | ||
| AutoTLS: tc.configAutoTLSEnabled, | ||
| }, | ||
| }) | ||
| if tc.tlsDisabledAnnotation != "" { | ||
| dm.Annotations = map[string]string{ | ||
| networking.DisableAutoTLSAnnotationKey: tc.tlsDisabledAnnotation, | ||
| } | ||
| } | ||
| if got := autoTLSEnabled(ctx, dm); got != tc.wantAutoTLSEnabled { | ||
| t.Errorf("autoTLSEnabled = %t, want %t", got, tc.wantAutoTLSEnabled) | ||
| } | ||
| }) | ||
| } | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we move this somewhere common?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'll follow up with that as well.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Got an issue with a DomainMapping:
Looking at the logs of the kcert:
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I believe you need to update net-http01, let's discuss in slack