Skip to content

Support config to deploy internal certificates automatically #13005

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

Merged
knative-prow merged 14 commits into from
Jun 30, 2022
Merged

Support config to deploy internal certificates automatically #13005

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
14 commits
Select commit Hold shift + click to select a range
70d58d7
Add certificate reconciler for internal certs
nak3 Jun 7, 2022
3af4f43
Fix cert path
nak3 Jun 8, 2022
fb30cfc
Temporary use local networking repo
nak3 Jun 8, 2022
4d19c6a
Support internal-encryption configuration
nak3 Jun 8, 2022
8140587
Use const for cert name
nak3 Jun 8, 2022
d313bd5
Fix lint
nak3 Jun 8, 2022
56d0063
rm blank line
nak3 Jun 15, 2022
6e5ba2a
Drop unused variable
nak3 Jun 16, 2022
0389bee
Use one line style
nak3 Jun 16, 2022
dba0c95
Use one line code
nak3 Jun 16, 2022
200a1f3
Update net-kourier nightly
knative-automation Jun 16, 2022
b829926
Verify SecretPKKey as well
nak3 Jun 27, 2022
bd56955
Do not drop activator always in the path
nak3 Jun 27, 2022
7a929b0
Comment about ctrl-ca suffix
nak3 Jun 29, 2022
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
19 changes: 10 additions & 9 deletions cmd/activator/main.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -40,6 +40,7 @@ import (
metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
"k8s.io/apimachinery/pkg/util/wait"

"knative.dev/control-protocol/pkg/certificates"
network "knative.dev/networking/pkg"
netcfg "knative.dev/networking/pkg/config"
netprobe "knative.dev/networking/pkg/http/probe"
Expand Down Expand Up @@ -156,14 +157,14 @@ func main() {
logger.Fatalw("Failed to construct network config", zap.Error(err))
}

// Enable TLS against queue-proxy when the CA and SA are specified.
tlsEnabled := networkConfig.QueueProxyCA != "" && networkConfig.QueueProxySAN != ""
// Enable TLS against queue-proxy when internal-encryption is enabled.
tlsEnabled := networkConfig.InternalEncryption

// Enable TLS client when queue-proxy-ca is specified.
// At this moment activator with TLS does not disable HTTP.
// See also https://github.com/knative/serving/issues/12808.
if tlsEnabled {
caSecret, err := kubeClient.CoreV1().Secrets(system.Namespace()).Get(ctx, networkConfig.QueueProxyCA, metav1.GetOptions{})
caSecret, err := kubeClient.CoreV1().Secrets(system.Namespace()).Get(ctx, netcfg.ServingInternalCertName, metav1.GetOptions{})
if err != nil {
logger.Fatalw("Failed to get secret", zap.Error(err))
}
Comment on lines 166 to 170
Copy link
Copy Markdown
Member

@evankanderson evankanderson Jun 24, 2022

Choose a reason for hiding this comment

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

I wonder whether we should provide an option on these errors to fall back to HTTP (for example, if the configmap has been switched, but the cert hasn't propagated yet).

Copy link
Copy Markdown
Contributor Author

@nak3 nak3 Jun 27, 2022

Choose a reason for hiding this comment

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

It makes sense, but I believe that current implementation is troublesome to verify the internal traffic is surely encrypted or not.
So some users do not want to expect the fallback feature and I think we should add it later.

(I am thinking that we should add an option based on Istio's mTLS mode like STRICT(TLS only) and PERMISSIVE(allow to fallback).

Copy link
Copy Markdown
Member

@evankanderson evankanderson Jun 29, 2022

Choose a reason for hiding this comment

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

Agreed -- I think it makes sense to be able to operate in three phases: encryption off, encryption opportunistically enabled, encryption required.

I think it's okay to defer this work to "beta"

Expand All @@ -173,14 +174,14 @@ func main() {
pool = x509.NewCertPool()
}

if ok := pool.AppendCertsFromPEM(caSecret.Data["ca.crt"]); !ok {
if ok := pool.AppendCertsFromPEM(caSecret.Data[certificates.SecretCaCertKey]); !ok {
logger.Fatalw("Failed to append ca cert to the RootCAs")
}

tlsConf := &tls.Config{
RootCAs: pool,
InsecureSkipVerify: false,
ServerName: networkConfig.QueueProxySAN,
ServerName: certificates.FakeDnsName,
Copy link
Copy Markdown
Member

@evankanderson evankanderson Jun 24, 2022

Choose a reason for hiding this comment

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

I'd like to incorporate the destination namespace in this ServerName parameter, possibly when NewProxyAutoTLSTransport is used in activatorhandler.

Copy link
Copy Markdown
Contributor Author

@nak3 nak3 Jun 27, 2022

Choose a reason for hiding this comment

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

As I commented below (or above), it would be possible with "activator always in the path" workaround but it will be super difficult to drop the "activator always in the path" if we support it.
I think we need to consider it carefully.

Copy link
Copy Markdown
Member

@evankanderson evankanderson Jun 29, 2022

Choose a reason for hiding this comment

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

I think there are a couple options here:

  1. Force "activator always in the path" for now.
  2. Activator can use SNI to serve different certs on a per-connection basis (basically, activator would spoof the cert from the queue-proxy namespace).

For now, I'd suggest we do 1, and then add 2 in a future iteration.

Copy link
Copy Markdown
Contributor Author

@nak3 nak3 Jun 29, 2022

Choose a reason for hiding this comment

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

Alright, I keep "the activator always in the path" for 1 in this PR.

MinVersion: tls.VersionTLS12,
}
transport = pkgnet.NewProxyAutoTLSTransport(env.MaxIdleProxyConns, env.MaxIdleProxyConnsPerHost, tlsConf)
Expand Down Expand Up @@ -275,15 +276,15 @@ func main() {
}(name, server)
}

// Enable TLS server when activator-server-cert is specified.
// Enable TLS server when internal-encryption is specified.
// At this moment activator with TLS does not disable HTTP.
// See also https://github.com/knative/serving/issues/12808.
if networkConfig.ActivatorCertSecret != "" {
secret, err := kubeClient.CoreV1().Secrets(system.Namespace()).Get(ctx, networkConfig.ActivatorCertSecret, metav1.GetOptions{})
if networkConfig.InternalEncryption {
secret, err := kubeClient.CoreV1().Secrets(system.Namespace()).Get(ctx, netcfg.ServingInternalCertName, metav1.GetOptions{})
if err != nil {
logger.Fatalw("failed to get secret", zap.Error(err))
}
cert, err := tls.X509KeyPair(secret.Data["tls.crt"], secret.Data["tls.key"])
cert, err := tls.X509KeyPair(secret.Data[certificates.SecretCertKey], secret.Data[certificates.SecretPKKey])
if err != nil {
logger.Fatalw("failed to load certs", zap.Error(err))
}
Expand Down
3 changes: 3 additions & 0 deletions cmd/controller/main.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,7 @@ package main

import (
// The set of controllers this controller process runs.
certificate "knative.dev/control-protocol/pkg/certificates/reconciler"
Comment thread
nak3 marked this conversation as resolved.
"knative.dev/serving/pkg/reconciler/configuration"
"knative.dev/serving/pkg/reconciler/gc"
"knative.dev/serving/pkg/reconciler/labeler"
Expand All @@ -30,6 +31,7 @@ import (
// This defines the shared main for injected controllers.
"knative.dev/pkg/injection"
"knative.dev/pkg/injection/sharedmain"
"knative.dev/serving/pkg/networking"
)

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

func main() {
Expand Down
5 changes: 3 additions & 2 deletions cmd/queue/main.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -33,6 +33,7 @@ import (

"k8s.io/apimachinery/pkg/types"

"knative.dev/control-protocol/pkg/certificates"
netheader "knative.dev/networking/pkg/http/header"
netproxy "knative.dev/networking/pkg/http/proxy"
netstats "knative.dev/networking/pkg/http/stats"
Expand Down Expand Up @@ -66,10 +67,10 @@ const (
drainSleepDuration = 30 * time.Second

// certPath is the path for the server certificate mounted by queue-proxy.
certPath = queue.CertDirectory + "/tls.crt"
certPath = queue.CertDirectory + "/" + certificates.SecretCertKey
Comment thread
nak3 marked this conversation as resolved.

// keyPath is the path for the server certificate key mounted by queue-proxy.
keyPath = queue.CertDirectory + "/tls.key"
keyPath = queue.CertDirectory + "/" + certificates.SecretPKKey
Comment thread
nak3 marked this conversation as resolved.
)

type config struct {
Expand Down
26 changes: 14 additions & 12 deletions test/config/tls/config-network.yaml → config/core/300-secret.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,17 +13,19 @@
# limitations under the License.

apiVersion: v1
kind: ConfigMap
kind: Secret
metadata:
name: config-network
# Do not drop -ctrl-ca suffix as control-protocol requires it.
# https://github.com/knative-sandbox/control-protocol/blob/main/pkg/certificates/reconciler/controller.go
name: serving-certs-ctrl-ca
Comment thread
evankanderson marked this conversation as resolved.
namespace: knative-serving
# The data is populated when internal-encryption is enabled.
---
apiVersion: v1
kind: Secret
metadata:
name: knative-serving-certs
Comment thread
evankanderson marked this conversation as resolved.
namespace: knative-serving
labels:
app.kubernetes.io/name: knative-serving
app.kubernetes.io/version: devel
serving.knative.dev/release: devel
data:
activator-ca: "serving-ca"
activator-san: "knative"
activator-cert-secret: "server-certs"
queue-proxy-ca: "serving-ca"
queue-proxy-san: "knative"
queue-proxy-cert-secret: "server-certs"
serving-certs-ctrl: "data-plane"
# The data is populated when internal-encryption is enabled.
3 changes: 2 additions & 1 deletion go.mod
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -34,8 +34,9 @@ require (
k8s.io/code-generator v0.23.5
k8s.io/kube-openapi v0.0.0-20220124234850-424119656bbf
knative.dev/caching v0.0.0-20220610113725-9c092893371a
knative.dev/control-protocol v0.0.0-20220610133426-4a1c8e84039f
knative.dev/hack v0.0.0-20220610014127-dc6c287516dc
knative.dev/networking v0.0.0-20220610013825-3103f3a72792
knative.dev/networking v0.0.0-20220614203516-07c9d7614c61
knative.dev/pkg v0.0.0-20220610014025-7d607d643ee2
sigs.k8s.io/yaml v1.3.0
)
Expand Down
10 changes: 8 additions & 2 deletions go.sum
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -296,6 +296,8 @@ github.com/cilium/ebpf v0.7.0/go.mod h1:/oI2+1shJiTGAMgl6/RgJr36Eo1jzrRcAWbcXO2u
github.com/circonus-labs/circonus-gometrics v2.3.1+incompatible/go.mod h1:nmEj6Dob7S7YxXgwXpfOuvO54S+tGdZdw9fuRZt25Ag=
github.com/circonus-labs/circonusllhist v0.1.3/go.mod h1:kMXHVDlOchFAehlya5ePtbp5jckzBHf4XRpQvBOLI+I=
github.com/client9/misspell v0.3.4/go.mod h1:qj6jICC3Q7zFZvVWo7KLAzC3yx5G7kyvSDkc90ppPyw=
github.com/cloudevents/conformance v0.2.0/go.mod h1:rHKDwylBH89Rns6U3wL9ww8bg9/4GbwRCDNuyoC6bcc=
github.com/cloudevents/sdk-go/v2 v2.4.1/go.mod h1:MZiMwmAh5tGj+fPFvtHv9hKurKqXtdB9haJYMJ/7GJY=
github.com/cncf/udpa/go v0.0.0-20191209042840-269d4d468f6f/go.mod h1:M8M6+tZqaGXZJjfX53e64911xZQV5JYwmTeXPW+k8Sc=
github.com/cncf/udpa/go v0.0.0-20200629203442-efcf912fb354/go.mod h1:WmhPx2Nbnhtbo57+VJT5O0JRkEi1Wbu0z5j0R8u5Hbk=
github.com/cncf/udpa/go v0.0.0-20201120205902-5459f2c99403/go.mod h1:WmhPx2Nbnhtbo57+VJT5O0JRkEi1Wbu0z5j0R8u5Hbk=
Expand Down Expand Up @@ -856,6 +858,7 @@ github.com/ianlancetaylor/demangle v0.0.0-20200824232613-28f6c0f3b639/go.mod h1:
github.com/imdario/mergo v0.3.4/go.mod h1:2EnlNZ0deacrJVfApfmtdGgDfMuh/nq6Ok1EcJh5FfA=
github.com/imdario/mergo v0.3.5/go.mod h1:2EnlNZ0deacrJVfApfmtdGgDfMuh/nq6Ok1EcJh5FfA=
github.com/imdario/mergo v0.3.8/go.mod h1:2EnlNZ0deacrJVfApfmtdGgDfMuh/nq6Ok1EcJh5FfA=
github.com/imdario/mergo v0.3.9/go.mod h1:2EnlNZ0deacrJVfApfmtdGgDfMuh/nq6Ok1EcJh5FfA=
github.com/imdario/mergo v0.3.10/go.mod h1:jmQim1M+e3UYxmgPu/WyfjB3N3VflVyUjjjwH0dnCYA=
github.com/imdario/mergo v0.3.11/go.mod h1:jmQim1M+e3UYxmgPu/WyfjB3N3VflVyUjjjwH0dnCYA=
github.com/imdario/mergo v0.3.12 h1:b6R2BslTbIEToALKP7LxUvijTsNI9TAe80pLWN2g/HU=
Expand Down Expand Up @@ -2244,16 +2247,19 @@ k8s.io/utils v0.0.0-20220210201930-3a6ce19ff2f9 h1:HNSDgDCrr/6Ly3WEGKZftiE7IY19V
k8s.io/utils v0.0.0-20220210201930-3a6ce19ff2f9/go.mod h1:jPW/WVKK9YHAvNhRxK0md/EJ228hCsBRufyofKtW8HA=
knative.dev/caching v0.0.0-20220610113725-9c092893371a h1:HMiI0L60m16KhkBLDyb8XV3GSaBM6ZPqRSluAwJ4XKs=
knative.dev/caching v0.0.0-20220610113725-9c092893371a/go.mod h1:IcfEPqEP6ma4EcRUcPkwVNx5FWHHWu8w4/eqRQguDwc=
knative.dev/control-protocol v0.0.0-20220610133426-4a1c8e84039f h1:vdhs0WWGojtUldsM/ijbOfYY2LTO3GlulilCgeZX4Js=
knative.dev/control-protocol v0.0.0-20220610133426-4a1c8e84039f/go.mod h1:MjnhSes1u2GIoqwQia5bSe3Ny8r+d5//UB+Y/en2ZL8=
knative.dev/hack v0.0.0-20220524153203-12d3e2a7addc/go.mod h1:PHt8x8yX5Z9pPquBEfIj0X66f8iWkWfR0S/sarACJrI=
knative.dev/hack v0.0.0-20220609132040-fd240e2cef5c/go.mod h1:PHt8x8yX5Z9pPquBEfIj0X66f8iWkWfR0S/sarACJrI=
knative.dev/hack v0.0.0-20220610014127-dc6c287516dc h1:LyqyT+rtgZYfOb3ChGE5jTFApCOcUmAcSV+TzgLxnys=
knative.dev/hack v0.0.0-20220610014127-dc6c287516dc/go.mod h1:PHt8x8yX5Z9pPquBEfIj0X66f8iWkWfR0S/sarACJrI=
knative.dev/networking v0.0.0-20220610013825-3103f3a72792 h1:GDLLrx8w9oEXoE/am8AdWSTtSsLoZevmEMaeAdetM1Q=
knative.dev/networking v0.0.0-20220610013825-3103f3a72792/go.mod h1:oIETD09Q4GSOXjdBdiPc0eEQxMwmjH7/gdhfg+sgdW8=
knative.dev/networking v0.0.0-20220614203516-07c9d7614c61 h1:IsEfLWjHFt10XEUnAE/W4XcEU2uA+PMd1aZFkz5vrzA=
knative.dev/networking v0.0.0-20220614203516-07c9d7614c61/go.mod h1:oIETD09Q4GSOXjdBdiPc0eEQxMwmjH7/gdhfg+sgdW8=
knative.dev/pkg v0.0.0-20220524202603-19adf798efb8/go.mod h1:pApypeWDkGrsMkUDkV6StWXS4CXhwGWuJEID9GGZY0Y=
knative.dev/pkg v0.0.0-20220609131940-865e331abfa5/go.mod h1:pApypeWDkGrsMkUDkV6StWXS4CXhwGWuJEID9GGZY0Y=
knative.dev/pkg v0.0.0-20220610014025-7d607d643ee2 h1:MMClRZRz6rzhrpySJ21XCJqVDd4K3rurUEJ1Yrh8DmA=
knative.dev/pkg v0.0.0-20220610014025-7d607d643ee2/go.mod h1:pApypeWDkGrsMkUDkV6StWXS4CXhwGWuJEID9GGZY0Y=
knative.dev/reconciler-test v0.0.0-20220610014025-b62b10257cbf/go.mod h1:/ps2aEdmtjId+pUGJuuADQN4IucIp4rI7KnrYEahOgE=
mvdan.cc/gofumpt v0.1.1/go.mod h1:yXG1r1WqZVKWbVRtBWKWX9+CxGYfA51nSomhM0woR48=
mvdan.cc/interfacer v0.0.0-20180901003855-c20040233aed/go.mod h1:Xkxe497xwlCKkIaQYRfC7CSLworTXY9RMqwhhCm+8Nc=
mvdan.cc/lint v0.0.0-20170908181259-adc824a0674b/go.mod h1:2odslEg/xrtNQqCYg2/jCoyKnw3vv5biOc3JnIcYfL4=
Expand Down
4 changes: 4 additions & 0 deletions pkg/networking/constants.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -51,6 +51,10 @@ const (
// ServiceTypeKey is the label key attached to a service specifying the type of service.
// e.g. Public, Private.
ServiceTypeKey = networking.GroupName + "/serviceType"

// ServingCertName is used by the secret name for internal TLS as "namespace-${ServingCertName}".
// Also the secret name has the label with "${ServingCertName}: data-plane"
ServingCertName = "serving-certs"
)

// ServiceType is the enumeration type for the Kubernetes services
Expand Down
6 changes: 3 additions & 3 deletions pkg/reconciler/autoscaling/config/store_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -67,8 +67,7 @@ func TestStoreImmutableConfig(t *testing.T) {
config := store.Load()
config.Autoscaler.MaxScaleUpRate = 100.0
config.Deployment.ProgressDeadline = 3 * time.Minute
config.Network.ActivatorCA = "activator-ca"
config.Network.ActivatorSAN = "activator-san"
config.Network.InternalEncryption = true
newConfig := store.Load()

if newConfig.Autoscaler.MaxScaleUpRate == 100.0 {
Expand All @@ -77,7 +76,8 @@ func TestStoreImmutableConfig(t *testing.T) {
if newConfig.Deployment.ProgressDeadline == 3*time.Minute {
t.Error("Deployment config is not immuable")
}
if newConfig.Network.ActivatorCA == "activator-ca" || newConfig.Network.ActivatorSAN == "activator-san" {

if newConfig.Network.InternalEncryption {
t.Error("Network config is not immuable")
}
}
2 changes: 1 addition & 1 deletion pkg/reconciler/autoscaling/kpa/kpa.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -129,7 +129,7 @@ func (c *Reconciler) ReconcileKind(ctx context.Context, pa *autoscalingv1alpha1.
// When activator CA is enabled, force activator always in path.
// TODO: This is a temporary state and to be fixed.
// See also issues/11906 and issues/12797.
case len(config.FromContext(ctx).Network.ActivatorCA) > 0:
case config.FromContext(ctx).Network.InternalEncryption:
mode = nv1alpha1.SKSOperationModeProxy

// If the want == -1 and PA is inactive that implies the autoscaler
Expand Down
6 changes: 2 additions & 4 deletions pkg/reconciler/autoscaling/kpa/kpa_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -128,8 +128,7 @@ func initialScaleZeroASConfig() *autoscalerconfig.Config {

func activatorCertsNetConfig() *netcfg.Config {
nc, _ := netcfg.NewConfigFromMap(map[string]string{
netcfg.ActivatorCAKey: "knative-ca",
netcfg.ActivatorSANKey: "knative-san",
netcfg.InternalEncryptionKey: "true",
})
return nc
}
Expand All @@ -141,8 +140,7 @@ func defaultConfig() *config.Config {
deployment.ProgressDeadlineKey: progressDeadline.String(),
})
networkConfig, _ := netcfg.NewConfigFromMap(map[string]string{
netcfg.ActivatorCAKey: "",
netcfg.ActivatorSANKey: "",
netcfg.InternalEncryptionKey: "false",
Comment thread
nak3 marked this conversation as resolved.
})

return &config.Config{
Expand Down
16 changes: 16 additions & 0 deletions pkg/reconciler/revision/cruds.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -21,6 +21,7 @@ import (
"fmt"

appsv1 "k8s.io/api/apps/v1"
corev1 "k8s.io/api/core/v1"
"k8s.io/apimachinery/pkg/api/equality"
metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"

Expand All @@ -30,6 +31,7 @@ import (
"knative.dev/pkg/logging"
autoscalingv1alpha1 "knative.dev/serving/pkg/apis/autoscaling/v1alpha1"
v1 "knative.dev/serving/pkg/apis/serving/v1"
"knative.dev/serving/pkg/networking"
"knative.dev/serving/pkg/reconciler/revision/config"
"knative.dev/serving/pkg/reconciler/revision/resources"
)
Expand All @@ -46,6 +48,20 @@ func (c *Reconciler) createDeployment(ctx context.Context, rev *v1.Revision) (*a
return c.kubeclient.AppsV1().Deployments(deployment.Namespace).Create(ctx, deployment, metav1.CreateOptions{})
}

func (c *Reconciler) createSecret(ctx context.Context, ns *corev1.Namespace) (*corev1.Secret, error) {
secret := &corev1.Secret{
Copy link
Copy Markdown
Contributor

@skonto skonto Jun 16, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

I guess data are filled in by the control-plane reconciler. Was this part of the original design to use the control-plane? It would be nice to have a high a level diagram of secrets created and encrypted paths (maybe there is one).
There might be one just I may lack context.

Copy link
Copy Markdown
Member

@evankanderson evankanderson Jun 24, 2022

Choose a reason for hiding this comment

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

There is a doc, but I'm not sure it specifies this level of detail.

I'm slightly confused about where the secret comes from here, and what the lifecycle is if internal encryption is disabled. (It may be that it's fine to drop this resource, but I don't quite understand the lifecycle.)

Copy link
Copy Markdown
Contributor Author

@nak3 nak3 Jun 27, 2022

Choose a reason for hiding this comment

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

The lifecycle of the secret in user's ns (queue-proxy's server cert) is:

  1. The secret is created when users deploy the first Kservice in the namespace.
  2. The secret is shared by all Kservice in the namespace.
  3. The secret is dropped when the namespace is deleted. (the secret is neither dropped when all Kservice is deleted in the namespace nor the internal encryption is disabled.).

There are many ways to manage the secret in user's ns and so I chose the simplest way for now.

ObjectMeta: metav1.ObjectMeta{
Name: ns.Name + "-" + networking.ServingCertName,
Namespace: ns.Name,
OwnerReferences: []metav1.OwnerReference{*metav1.NewControllerRef(ns, corev1.SchemeGroupVersion.WithKind("Namespace"))},
Labels: map[string]string{
networking.ServingCertName + "-ctrl": "data-plane",
},
},
}
return c.kubeclient.CoreV1().Secrets(secret.Namespace).Create(ctx, secret, metav1.CreateOptions{})
}

func (c *Reconciler) checkAndUpdateDeployment(ctx context.Context, rev *v1.Revision, have *appsv1.Deployment) (*appsv1.Deployment, error) {
logger := logging.FromContext(ctx)
cfgs := config.FromContext(ctx)
Expand Down
33 changes: 33 additions & 0 deletions pkg/reconciler/revision/reconcile_resources.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -28,11 +28,13 @@ import (
apierrs "k8s.io/apimachinery/pkg/api/errors"
metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"

"knative.dev/control-protocol/pkg/certificates"
"knative.dev/pkg/kmeta"
"knative.dev/pkg/kmp"
"knative.dev/pkg/logging"
"knative.dev/pkg/logging/logkey"
v1 "knative.dev/serving/pkg/apis/serving/v1"
"knative.dev/serving/pkg/networking"
"knative.dev/serving/pkg/reconciler/revision/resources"
resourcenames "knative.dev/serving/pkg/reconciler/revision/resources/names"
)
Expand Down Expand Up @@ -197,3 +199,34 @@ func hasDeploymentTimedOut(deployment *appsv1.Deployment) bool {
}
return false
}

func (c *Reconciler) reconcileSecret(ctx context.Context, rev *v1.Revision) error {
ns := rev.Namespace
secretName := ns + "-" + networking.ServingCertName
logger := logging.FromContext(ctx)
logger.Info("Reconciling Secret: ", secretName)

secret, err := c.kubeclient.CoreV1().Secrets(ns).Get(ctx, secretName, metav1.GetOptions{})
if apierrs.IsNotFound(err) {
namespace, err := c.kubeclient.CoreV1().Namespaces().Get(ctx, ns, metav1.GetOptions{})
if err != nil {
return fmt.Errorf("failed to get Namespace %q: %w", secretName, err)
}
if secret, err = c.createSecret(ctx, namespace); err != nil {
return fmt.Errorf("failed to create Secret %q: %w", secretName, err)
}
logger.Info("Created Secret: ", secretName)
} else if err != nil {
return fmt.Errorf("failed to get secret %q: %w", secretName, err)
}

// Verify if secret has been added the data.
if _, ok := secret.Data[certificates.SecretCertKey]; !ok {
Copy link
Copy Markdown
Member

@evankanderson evankanderson Jun 24, 2022

Choose a reason for hiding this comment

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

Do we need both the SecretCertKey and the SecretPKKey?

Copy link
Copy Markdown
Member

@evankanderson evankanderson Jun 30, 2022

Choose a reason for hiding this comment

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

I think you want a check for the SecretPKKey as well.

return fmt.Errorf("public cert in the secret is not ready yet")
}
if _, ok := secret.Data[certificates.SecretPKKey]; !ok {
return fmt.Errorf("private key in the secret is not ready yet")
}

return nil
}
4 changes: 2 additions & 2 deletions pkg/reconciler/revision/resources/deploy.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -139,9 +139,9 @@ func makePodSpec(rev *v1.Revision, cfg *config.Config) (*corev1.PodSpec, error)
extraVolumes = append(extraVolumes, varTokenVolume)
}

if cfg.Network.QueueProxyCertSecret != "" {
if cfg.Network.InternalEncryption {
queueContainer.VolumeMounts = append(queueContainer.VolumeMounts, certVolumeMount)
extraVolumes = append(extraVolumes, certVolume(cfg.Network.QueueProxyCertSecret))
extraVolumes = append(extraVolumes, certVolume(rev.Namespace+"-"+networking.ServingCertName))
}

podSpec := BuildPodSpec(rev, append(BuildUserContainers(rev), *queueContainer), cfg)
Expand Down
7 changes: 7 additions & 0 deletions pkg/reconciler/revision/revision.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -138,6 +138,13 @@ func (c *Reconciler) ReconcileKind(ctx context.Context, rev *v1.Revision) pkgrec
logger.Debug("Revision meta: " + spew.Sdump(rev.ObjectMeta))
}

// Deploy certificate when internal-encryption is enabled.
if config.FromContext(ctx).Network.InternalEncryption {
if err := c.reconcileSecret(ctx, rev); err != nil {
return err
}
}

for _, phase := range []func(context.Context, *v1.Revision) error{
c.reconcileDeployment,
c.reconcileImageCache,
Expand Down
Loading