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

Integrate net-certmanager in Serving #15066

Merged
merged 15 commits into from
Apr 25, 2024
Merged
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
9 changes: 4 additions & 5 deletions .github/workflows/kind-e2e.yaml
Original file line number Diff line number Diff line change
Expand Up @@ -87,7 +87,7 @@ jobs:

ingress:
- kourier
# - kourier-tls
- kourier-tls
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Reverted it so I can test. Tests seem to pass here consistently.

Copy link
Member

Choose a reason for hiding this comment

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

#15052 should also help.

- istio
- istio-tls
# Disabled due to flakiness: https://github.com/knative/serving/issues/14637
Expand Down Expand Up @@ -130,10 +130,9 @@ jobs:
# ingress-class: istio
# ambient: 1

# Disabled due to flakiness: https://github.com/knative/serving/issues/15052
# - ingress: kourier-tls
# ingress-class: kourier
# enable-tls: 1
- ingress: kourier-tls
ingress-class: kourier
enable-tls: 1

- test-suite: runtime
test-path: ./test/conformance/runtime/...
Expand Down
87 changes: 83 additions & 4 deletions cmd/controller/main.go
Original file line number Diff line number Diff line change
Expand Up @@ -17,12 +17,25 @@ limitations under the License.
package main

import (
"context"
"fmt"

// The set of controllers this controller process runs.
"flag"
"log"

v1 "k8s.io/api/core/v1"
metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
"k8s.io/client-go/kubernetes"
netcfg "knative.dev/networking/pkg/config"
"knative.dev/pkg/injection"
"knative.dev/pkg/injection/sharedmain"
"knative.dev/pkg/reconciler"
"knative.dev/pkg/signals"
"knative.dev/pkg/system"
"knative.dev/serving/pkg/reconciler/certificate"
"knative.dev/serving/pkg/reconciler/configuration"
"knative.dev/serving/pkg/reconciler/domainmapping"
"knative.dev/serving/pkg/reconciler/gc"
"knative.dev/serving/pkg/reconciler/labeler"
"knative.dev/serving/pkg/reconciler/nscert"
Expand All @@ -31,9 +44,12 @@ import (
"knative.dev/serving/pkg/reconciler/serverlessservice"
"knative.dev/serving/pkg/reconciler/service"

"knative.dev/pkg/injection"
"knative.dev/pkg/injection/sharedmain"
"knative.dev/serving/pkg/reconciler/domainmapping"
versioned "knative.dev/serving/pkg/client/certmanager/clientset/versioned"
"knative.dev/serving/pkg/client/certmanager/injection/informers/acme/v1/challenge"
v1certificate "knative.dev/serving/pkg/client/certmanager/injection/informers/certmanager/v1/certificate"
"knative.dev/serving/pkg/client/certmanager/injection/informers/certmanager/v1/certificaterequest"
"knative.dev/serving/pkg/client/certmanager/injection/informers/certmanager/v1/clusterissuer"
"knative.dev/serving/pkg/client/certmanager/injection/informers/certmanager/v1/issuer"
)

var ctors = []injection.ControllerConstructor{
Expand All @@ -53,5 +69,68 @@ func main() {
"reconciliation-timeout", reconciler.DefaultTimeout,
"The amount of time to give each reconciliation of a resource to complete before its context is canceled.")

sharedmain.MainWithContext(signals.NewContext(), "controller", ctors...)
Copy link
Member

Choose a reason for hiding this comment

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

Can we have a second method in sharedmain where we pass a function to do:

if shouldEnableNetCertManagerController(ctx, client) {
		injection.Default.RegisterInformer(challenge.WithInformer)
		injection.Default.RegisterInformer(v1certificate.WithInformer)
		injection.Default.RegisterInformer(certificaterequest.WithInformer)
		injection.Default.RegisterInformer(clusterissuer.WithInformer)
		injection.Default.RegisterInformer(issuer.WithInformer)
		ctors = append(ctors, netcertmanager.NewController)
	}

to avoid duplicating code here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Or we just iterate on an array of informers. Moving to sharedmain should be the next iteration imho.

ctx := signals.NewContext()

// HACK: This parses flags, so the above should be set once this runs.
cfg := injection.ParseAndGetRESTConfigOrDie()

// If nil it panics
client := kubernetes.NewForConfigOrDie(cfg)

if shouldEnableNetCertManagerController(ctx, client) {
v := versioned.NewForConfigOrDie(cfg)
if ok, err := certManagerCRDsExist(v); !ok {
log.Fatalf("Please install cert-manager: %v", err)
}
for _, inf := range []injection.InformerInjector{challenge.WithInformer, v1certificate.WithInformer, certificaterequest.WithInformer, clusterissuer.WithInformer, issuer.WithInformer} {
injection.Default.RegisterInformer(inf)
}
ctors = append(ctors, certificate.NewController)
}

sharedmain.MainWithConfig(ctx, "controller", cfg, ctors...)
}

func shouldEnableNetCertManagerController(ctx context.Context, client *kubernetes.Clientset) bool {
var cm *v1.ConfigMap
var err error
if cm, err = client.CoreV1().ConfigMaps(system.Namespace()).Get(ctx, "config-network", metav1.GetOptions{}); err != nil {
log.Fatalf("Failed to get cm config-network: %v", err)
}
netCfg, err := netcfg.NewConfigFromMap(cm.Data)
if err != nil {
log.Fatalf("Failed to construct network config: %v", err)
}

return netCfg.ExternalDomainTLS || netCfg.SystemInternalTLSEnabled() || (netCfg.ClusterLocalDomainTLS == netcfg.EncryptionEnabled) ||
Copy link
Member

Choose a reason for hiding this comment

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

We should validate cert manager CRDs as installed

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 can do that but also the user could know via docs that internal encryption requires cert manager. Personally I think it is a bit too much but I can add it.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done.

netCfg.NamespaceWildcardCertSelector != nil
Copy link
Member

Choose a reason for hiding this comment

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

can netCfg.NamespaceWildcardCertSelector != nil be used without netCfg.ExternalDomainTLS == true?

}

func certManagerCRDsExist(client *versioned.Clientset) (bool, error) {
if ok, err := findCRD(client, "cert-manager.io/v1", []string{"certificaterequests", "certificates", "clusterissuers", "issuers"}); !ok {
return false, err
}
if ok, err := findCRD(client, "acme.cert-manager.io/v1", []string{"challenges"}); !ok {
return false, err
}
return true, nil
}

func findCRD(client *versioned.Clientset, groupVersion string, crds []string) (bool, error) {
resourceList, err := client.Discovery().ServerResourcesForGroupVersion(groupVersion)
if err != nil {
return false, err
}
for _, crdName := range crds {
isCRDPresent := false
for _, resource := range resourceList.APIResources {
if resource.Name == crdName {
isCRDPresent = true
}
}
if !isCRDPresent {
return false, fmt.Errorf("cert manager crds are missing: %s", crdName)
}
}
return true, nil
}
24 changes: 13 additions & 11 deletions cmd/webhook/main.go
Original file line number Diff line number Diff line change
Expand Up @@ -34,6 +34,7 @@ import (
"knative.dev/pkg/webhook/resourcesemantics/defaulting"
"knative.dev/pkg/webhook/resourcesemantics/validation"
servingv1beta1 "knative.dev/serving/pkg/apis/serving/v1beta1"
certconfig "knative.dev/serving/pkg/reconciler/certificate/config"

// resource validation types
net "knative.dev/networking/pkg/apis/networking/v1alpha1"
Expand Down Expand Up @@ -143,17 +144,18 @@ func newConfigValidationController(ctx context.Context, cmw configmap.Watcher) *

// The configmaps to validate.
configmap.Constructors{
tracingconfig.ConfigName: tracingconfig.NewTracingConfigFromConfigMap,
autoscalerconfig.ConfigName: autoscalerconfig.NewConfigFromConfigMap,
gc.ConfigName: gc.NewConfigFromConfigMapFunc(ctx),
netcfg.ConfigMapName: network.NewConfigFromConfigMap,
deployment.ConfigName: deployment.NewConfigFromConfigMap,
apisconfig.FeaturesConfigName: apisconfig.NewFeaturesConfigFromConfigMap,
metrics.ConfigMapName(): metrics.NewObservabilityConfigFromConfigMap,
logging.ConfigMapName(): logging.NewConfigFromConfigMap,
leaderelection.ConfigMapName(): leaderelection.NewConfigFromConfigMap,
domainconfig.DomainConfigName: domainconfig.NewDomainFromConfigMap,
apisconfig.DefaultsConfigName: apisconfig.NewDefaultsConfigFromConfigMap,
tracingconfig.ConfigName: tracingconfig.NewTracingConfigFromConfigMap,
autoscalerconfig.ConfigName: autoscalerconfig.NewConfigFromConfigMap,
gc.ConfigName: gc.NewConfigFromConfigMapFunc(ctx),
netcfg.ConfigMapName: network.NewConfigFromConfigMap,
deployment.ConfigName: deployment.NewConfigFromConfigMap,
apisconfig.FeaturesConfigName: apisconfig.NewFeaturesConfigFromConfigMap,
metrics.ConfigMapName(): metrics.NewObservabilityConfigFromConfigMap,
logging.ConfigMapName(): logging.NewConfigFromConfigMap,
leaderelection.ConfigMapName(): leaderelection.NewConfigFromConfigMap,
domainconfig.DomainConfigName: domainconfig.NewDomainFromConfigMap,
apisconfig.DefaultsConfigName: apisconfig.NewDefaultsConfigFromConfigMap,
certconfig.CertManagerConfigName: certconfig.NewCertManagerConfigFromConfigMap,
},
)
}
Expand Down
6 changes: 6 additions & 0 deletions config/core/200-roles/clusterrole.yaml
Original file line number Diff line number Diff line change
Expand Up @@ -51,3 +51,9 @@ rules:
- apiGroups: ["caching.internal.knative.dev"]
resources: ["images"]
verbs: ["get", "list", "create", "update", "delete", "patch", "watch"]
- apiGroups: ["cert-manager.io"]
resources: ["certificates", "clusterissuers", "certificaterequests", "issuers"]
verbs: ["get", "list", "create", "update", "delete", "patch", "watch"]
- apiGroups: ["acme.cert-manager.io"]
resources: ["challenges"]
verbs: ["get", "list", "create", "update", "delete", "patch", "watch"]
68 changes: 68 additions & 0 deletions config/core/configmaps/certmanager.yaml
Original file line number Diff line number Diff line change
@@ -0,0 +1,68 @@
# Copyright 2020 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
#
# https://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.

apiVersion: v1
kind: ConfigMap
metadata:
name: config-certmanager
namespace: knative-serving
labels:
app.kubernetes.io/name: knative-serving
app.kubernetes.io/component: controller
app.kubernetes.io/version: devel
networking.knative.dev/certificate-provider: cert-manager
annotations:
knative.dev/example-checksum: "b7a9a602"
data:
_example: |
################################
# #
# EXAMPLE CONFIGURATION #
# #
################################
# This block is not actually functional configuration,
# but serves to illustrate the available configuration
# options and document them in a way that is accessible
# to users that `kubectl edit` this config map.
#
# These sample configuration options may be copied out of
# this block and unindented to actually change the configuration.
# issuerRef is a reference to the issuer for external-domain certificates used for ingress.
# IssuerRef should be either `ClusterIssuer` or `Issuer`.
# Please refer `IssuerRef` in https://cert-manager.io/docs/concepts/issuer/
# for more details about IssuerRef configuration.
# If the issuerRef is not specified, the self-signed `knative-selfsigned-issuer` ClusterIssuer is used.
issuerRef: |
kind: ClusterIssuer
name: letsencrypt-issuer
# clusterLocalIssuerRef is a reference to the issuer for cluster-local-domain certificates used for ingress.
# clusterLocalIssuerRef should be either `ClusterIssuer` or `Issuer`.
# Please refer `IssuerRef` in https://cert-manager.io/docs/concepts/issuer/
# for more details about ClusterInternalIssuerRef configuration.
# If the clusterLocalIssuerRef is not specified, the self-signed `knative-selfsigned-issuer` ClusterIssuer is used.
clusterLocalIssuerRef: |
kind: ClusterIssuer
name: your-company-issuer
# systemInternalIssuerRef is a reference to the issuer for certificates for system-internal-tls certificates used by Knative internal components.
# systemInternalIssuerRef should be either `ClusterIssuer` or `Issuer`.
# Please refer `IssuerRef` in https://cert-manager.io/docs/concepts/issuer/
# for more details about ClusterInternalIssuerRef configuration.
# If the systemInternalIssuerRef is not specified, the self-signed `knative-selfsigned-issuer` ClusterIssuer is used.
systemInternalIssuerRef: |
kind: ClusterIssuer
name: knative-selfsigned-issuer
2 changes: 1 addition & 1 deletion config/core/webhooks/configmap-validation.yaml
Original file line number Diff line number Diff line change
Expand Up @@ -36,5 +36,5 @@ webhooks:
values: ["knative-serving"]
- key: app.kubernetes.io/component
operator: In
values: ["autoscaler", "controller", "logging", "networking", "observability", "tracing"]
values: ["autoscaler", "controller", "logging", "networking", "observability", "tracing", "net-certmanager"]
timeoutSeconds: 10
23 changes: 13 additions & 10 deletions go.mod
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,9 @@ go 1.21

require (
github.com/ahmetb/gen-crd-api-reference-docs v0.3.1-0.20210609063737-0067dc6dcea2
github.com/davecgh/go-spew v1.1.1
github.com/cert-manager/cert-manager v1.13.3
github.com/davecgh/go-spew v1.1.2-0.20180830191138-d8f796af33cc
github.com/ghodss/yaml v1.0.0
github.com/gogo/protobuf v1.3.2
github.com/google/go-cmp v0.6.0
github.com/google/go-containerregistry v0.13.0
Expand Down Expand Up @@ -45,10 +47,10 @@ require (
contrib.go.opencensus.io/exporter/ocagent v0.7.1-0.20200907061046-05415f1de66d // indirect
contrib.go.opencensus.io/exporter/prometheus v0.4.2 // indirect
contrib.go.opencensus.io/exporter/zipkin v0.1.2 // indirect
github.com/Azure/azure-sdk-for-go v67.0.0+incompatible // indirect
github.com/Azure/azure-sdk-for-go v68.0.0+incompatible // indirect
github.com/Azure/go-autorest v14.2.0+incompatible // indirect
github.com/Azure/go-autorest/autorest v0.11.28 // indirect
github.com/Azure/go-autorest/autorest/adal v0.9.21 // indirect
github.com/Azure/go-autorest/autorest v0.11.29 // indirect
github.com/Azure/go-autorest/autorest/adal v0.9.23 // indirect
github.com/Azure/go-autorest/autorest/azure/auth v0.5.11 // indirect
github.com/Azure/go-autorest/autorest/azure/cli v0.4.6 // indirect
github.com/Azure/go-autorest/autorest/date v0.3.0 // indirect
Expand Down Expand Up @@ -84,16 +86,16 @@ require (
github.com/docker/docker v25.0.1+incompatible // indirect
github.com/docker/docker-credential-helpers v0.7.0 // indirect
github.com/emicklei/go-restful/v3 v3.11.0 // indirect
github.com/evanphx/json-patch v4.12.0+incompatible // indirect
github.com/evanphx/json-patch v5.6.0+incompatible // indirect
github.com/evanphx/json-patch/v5 v5.9.0 // indirect
github.com/felixge/httpsnoop v1.0.4 // indirect
github.com/go-kit/log v0.2.1 // indirect
github.com/go-logfmt/logfmt v0.5.1 // indirect
github.com/go-logr/logr v1.4.1 // indirect
github.com/go-logr/stdr v1.2.2 // indirect
github.com/go-openapi/jsonpointer v0.19.6 // indirect
github.com/go-openapi/jsonreference v0.20.2 // indirect
github.com/go-openapi/swag v0.22.3 // indirect
github.com/go-openapi/jsonpointer v0.20.2 // indirect
github.com/go-openapi/jsonreference v0.20.4 // indirect
github.com/go-openapi/swag v0.22.7 // indirect
github.com/gobuffalo/flect v1.0.2 // indirect
github.com/golang-jwt/jwt/v4 v4.5.0 // indirect
github.com/golang/groupcache v0.0.0-20210331224755-41bb18bfe9da // indirect
Expand All @@ -108,7 +110,7 @@ require (
github.com/imdario/mergo v0.3.12 // indirect
github.com/influxdata/line-protocol v0.0.0-20200327222509-2487e7298839 // indirect
github.com/influxdata/tdigest v0.0.1 // indirect
github.com/jmespath/go-jmespath v0.4.0 // indirect
github.com/jmespath/go-jmespath v0.4.1-0.20220621161143-b0104c826a24 // indirect
github.com/josharian/intern v1.0.0 // indirect
github.com/json-iterator/go v1.1.12 // indirect
github.com/klauspost/compress v1.16.6 // indirect
Expand All @@ -128,7 +130,7 @@ require (
github.com/prometheus/statsd_exporter v0.22.7 // indirect
github.com/rs/dnscache v0.0.0-20211102005908-e0241e321417 // indirect
github.com/russross/blackfriday/v2 v2.1.0 // indirect
github.com/sirupsen/logrus v1.9.0 // indirect
github.com/sirupsen/logrus v1.9.3 // indirect
github.com/spf13/pflag v1.0.5 // indirect
github.com/tsenart/go-tsz v0.0.0-20180814235614-0bd30b3df1c3 // indirect
github.com/vbatts/tar-split v0.11.2 // indirect
Expand All @@ -153,6 +155,7 @@ require (
k8s.io/gengo v0.0.0-20240129211411-f967bbeff4b4 // indirect
k8s.io/klog v1.0.0 // indirect
k8s.io/klog/v2 v2.120.1 // indirect
sigs.k8s.io/gateway-api v0.8.0 // indirect
sigs.k8s.io/json v0.0.0-20221116044647-bc3834ca7abd // indirect
sigs.k8s.io/structured-merge-diff/v4 v4.4.1 // indirect
)
Expand Down
Loading
Loading