diff --git a/install/kubernetes/helm/istio/charts/security/templates/deployment.yaml b/install/kubernetes/helm/istio/charts/security/templates/deployment.yaml index 373e98a03b31..7b43e9a61e8f 100644 --- a/install/kubernetes/helm/istio/charts/security/templates/deployment.yaml +++ b/install/kubernetes/helm/istio/charts/security/templates/deployment.yaml @@ -71,6 +71,9 @@ spec: - --liveness-probe-interval=60s # interval for health check file update - --probe-check-interval=15s # interval for health status check {{- end }} + env: + - name: CITADEL_ENABLE_NAMESPACES_BY_DEFAULT + value: "{{ .Values.enableNamespacesByDefault }}" {{- if .Values.citadelHealthCheck }} livenessProbe: exec: diff --git a/install/kubernetes/helm/istio/charts/security/values.yaml b/install/kubernetes/helm/istio/charts/security/values.yaml index 8cdc738d2180..6bbb43401724 100644 --- a/install/kubernetes/helm/istio/charts/security/values.yaml +++ b/install/kubernetes/helm/istio/charts/security/values.yaml @@ -16,6 +16,16 @@ citadelHealthCheck: false # 90*24hour = 2160h workloadCertTtl: 2160h +# Determines Citadel default behavior if the ca.istio.io/env or ca.istio.io/override +# labels are not found on a given namespace. +# +# For example: consider a namespace called "target", which has neither the "ca.istio.io/env" +# nor the "ca.istio.io/override" namespace labels. To decide whether or not to generate secrets +# for service accounts created in this "target" namespace, Citadel will defer to this option. If the value +# of this option is "true" in this case, secrets will be generated for the "target" namespace. +# If the value of this option is "false" Citadel will not generate secrets upon service account creation. +enableNamespacesByDefault: true + # Specify the pod anti-affinity that allows you to constrain which nodes # your pod is eligible to be scheduled based on labels on pods that are # already running on the node rather than based on labels on nodes. diff --git a/security/cmd/istio_ca/main.go b/security/cmd/istio_ca/main.go index a9cbcdfec595..aa27a208e0a3 100644 --- a/security/cmd/istio_ca/main.go +++ b/security/cmd/istio_ca/main.go @@ -48,10 +48,11 @@ import ( type cliOptions struct { // nolint: maligned // Comma separated string containing all listened namespaces - listenedNamespaces string - istioCaStorageNamespace string - kubeConfigFile string - readSigningCertOnly bool + listenedNamespaces string + enableNamespacesByDefault bool + istioCaStorageNamespace string + kubeConfigFile string + readSigningCertOnly bool certChainFile string signingCertFile string @@ -61,9 +62,6 @@ type cliOptions struct { // nolint: maligned selfSignedCA bool selfSignedCACertTTL time.Duration - // if set, namespaces require explicit labeling to have Citadel generate secrets. - explicitOptInRequired bool - workloadCertTTL time.Duration maxWorkloadCertTTL time.Duration // The length of certificate rotation grace period, configured as the ratio of the certificate TTL. @@ -152,6 +150,11 @@ func fatalf(template string, args ...interface{}) { } func init() { + initCLI() + initEnvVars() +} + +func initCLI() { flags := rootCmd.Flags() // General configuration. flags.StringVar(&opts.listenedNamespaces, "listened-namespace", metav1.NamespaceAll, "deprecated") @@ -165,9 +168,6 @@ func init() { cmd.ListenedNamespaceKey+"} environment variable. If neither is set, Citadel listens to all namespaces.") flags.StringVar(&opts.istioCaStorageNamespace, "citadel-storage-namespace", "istio-system", "Namespace where "+ "the Citadel pod is running. Will not be used if explicit file or other storage mechanism is specified.") - flags.BoolVar(&opts.explicitOptInRequired, "explicit-opt-in", false, "Specifies whether Citadel requires "+ - "explicit opt-in for creating secrets. If set, only namespaces labeled with 'istio-managed=enabled' will "+ - "have secrets created. This feature is only available in key and certificates delivered through secret volume mount.") flags.StringVar(&opts.kubeConfigFile, "kube-config", "", "Specifies path to kubeconfig file. This must be specified when not running inside a Kubernetes pod.") @@ -262,6 +262,12 @@ func init() { cmd.InitializeFlags(rootCmd) } +func initEnvVars() { + enableNamespacesByDefault := env.RegisterBoolVar("CITADEL_ENABLE_NAMESPACES_BY_DEFAULT", true, + "Determines whether unlabeled namespaces should be targeted by this Citadel instance").Get() + opts.enableNamespacesByDefault = enableNamespacesByDefault +} + func main() { if err := rootCmd.Execute(); err != nil { log.Errora(err) @@ -312,10 +318,11 @@ func runCA() { if !opts.serverOnly { log.Infof("Creating Kubernetes controller to write issued keys and certs into secret ...") // For workloads in K8s, we apply the configured workload cert TTL. - sc, err := controller.NewSecretController(ca, opts.explicitOptInRequired, + sc, err := controller.NewSecretController(ca, + opts.enableNamespacesByDefault, opts.workloadCertTTL, opts.workloadCertGracePeriodRatio, opts.workloadCertMinGracePeriod, opts.dualUse, - cs.CoreV1(), opts.signCACerts, opts.pkcs8Keys, listenedNamespaces, webhooks) + cs.CoreV1(), opts.signCACerts, opts.pkcs8Keys, listenedNamespaces, webhooks, opts.istioCaStorageNamespace) if err != nil { fatalf("Failed to create secret controller: %v", err) } diff --git a/security/pkg/k8s/controller/workloadsecret.go b/security/pkg/k8s/controller/workloadsecret.go index 4a08feb42ad5..31bb3bf1c52c 100644 --- a/security/pkg/k8s/controller/workloadsecret.go +++ b/security/pkg/k8s/controller/workloadsecret.go @@ -41,6 +41,15 @@ const ( // The Istio secret annotation type IstioSecretType = "istio.io/key-and-cert" + // NamespaceManagedLabel (string, with namespace as value) and NamespaceOverrideLabel (boolean) contribute to determining + // whether or not a given Citadel instance should operate on a namespace. The behavior is as follows: + // 1) If NamespaceOverrideLabel exists and is valid, follow what this label tells us + // 2) If not, check NamespaceManagedLabel. If the value matches the Citadel instance's NS, then it should be active + // 3) If NamespaceManagedLabel nonexistent or invalid, follow enableNamespacesByDefault, set from "CITADEL_ENABLE_NAMESPACES_BY_DEFAULT" envvar + // 4) If enableNamespacesByDefault is "true", the Citadel instance should operate on unlabeled namespaces, otherwise should not + NamespaceManagedLabel = "ca.istio.io/env" + NamespaceOverrideLabel = "ca.istio.io/override" + // The ID/name for the certificate chain file. CertChainID = "cert-chain.pem" // The ID/name for the private key file. @@ -50,8 +59,9 @@ const ( // The key to specify corresponding service account in the annotation of K8s secrets. ServiceAccountNameAnnotationKey = "istio.io/service-account.name" - secretNamePrefix = "istio." - secretResyncPeriod = time.Minute + secretNamePrefix = "istio." + secretResyncPeriod = time.Minute + namespaceResyncPeriod = time.Second * 5 recommendedMinGracePeriodRatio = 0.2 recommendedMaxGracePeriodRatio = 0.8 @@ -87,6 +97,9 @@ type SecretController struct { // Length of the grace period for the certificate rotation. gracePeriodRatio float32 + // Whether controller loop should target namespaces without the NamespaceManagedLabel + enableNamespacesByDefault bool + // Whether the certificates are for dual-use clients (SAN+CN). dualUse bool @@ -96,12 +109,6 @@ type SecretController struct { // If true, generate a PKCS#8 private key. pkcs8Key bool - // whether ServiceAccount objects must explicitly opt-in for secrets. - // Object explicit opt-in is based on "istio-inject" NS label value. - // The default value should be read from a configmap and applied consistently - // to all control plane operations - explicitOptIn bool - // The set of namespaces explicitly set for monitoring via commandline (an entry could be metav1.NamespaceAll) namespaces map[string]struct{} @@ -116,14 +123,20 @@ type SecretController struct { scrtController cache.Controller scrtStore cache.Store - monitoring monitoringMetrics + // Controller and store for namespace objects + namespaceController cache.Controller + namespaceStore cache.Store + + // Used to coordinate with label and check if this instance of Citadel should create secret + istioCaStorageNamespace string + monitoring monitoringMetrics } // NewSecretController returns a pointer to a newly constructed SecretController instance. -func NewSecretController(ca ca.CertificateAuthority, requireOptIn bool, certTTL time.Duration, +func NewSecretController(ca ca.CertificateAuthority, enableNamespacesByDefault bool, certTTL time.Duration, gracePeriodRatio float32, minGracePeriod time.Duration, dualUse bool, core corev1.CoreV1Interface, forCA bool, pkcs8Key bool, namespaces []string, - dnsNames map[string]*DNSNameEntry) (*SecretController, error) { + dnsNames map[string]*DNSNameEntry, istioCaStorageNamespace string) (*SecretController, error) { if gracePeriodRatio < 0 || gracePeriodRatio > 1 { return nil, fmt.Errorf("grace period ratio %f should be within [0, 1]", gracePeriodRatio) @@ -134,24 +147,26 @@ func NewSecretController(ca ca.CertificateAuthority, requireOptIn bool, certTTL } c := &SecretController{ - ca: ca, - certTTL: certTTL, - gracePeriodRatio: gracePeriodRatio, - minGracePeriod: minGracePeriod, - dualUse: dualUse, - core: core, - forCA: forCA, - pkcs8Key: pkcs8Key, - explicitOptIn: requireOptIn, - namespaces: make(map[string]struct{}), - dnsNames: dnsNames, - monitoring: newMonitoringMetrics(), + ca: ca, + certTTL: certTTL, + istioCaStorageNamespace: istioCaStorageNamespace, + gracePeriodRatio: gracePeriodRatio, + enableNamespacesByDefault: enableNamespacesByDefault, + minGracePeriod: minGracePeriod, + dualUse: dualUse, + core: core, + forCA: forCA, + pkcs8Key: pkcs8Key, + namespaces: make(map[string]struct{}), + dnsNames: dnsNames, + monitoring: newMonitoringMetrics(), } for _, ns := range namespaces { c.namespaces[ns] = struct{}{} } + // listen for service account creation across all listened namespaces, filter out which should be ignored upon receipt saLW := listwatch.MultiNamespaceListerWatcher(namespaces, func(namespace string) cache.ListerWatcher { return &cache.ListWatch{ ListFunc: func(options metav1.ListOptions) (runtime.Object, error) { @@ -162,12 +177,11 @@ func NewSecretController(ca ca.CertificateAuthority, requireOptIn bool, certTTL }, } }) - - rehf := cache.ResourceEventHandlerFuncs{ - AddFunc: c.saAdded, - DeleteFunc: c.saDeleted, - } - c.saStore, c.saController = cache.NewInformer(saLW, &v1.ServiceAccount{}, time.Minute, rehf) + c.saStore, c.saController = + cache.NewInformer(saLW, &v1.ServiceAccount{}, time.Minute, cache.ResourceEventHandlerFuncs{ + AddFunc: c.saAdded, + DeleteFunc: c.saDeleted, + }) istioSecretSelector := fields.SelectorFromSet(map[string]string{"type": IstioSecretType}).String() scrtLW := listwatch.MultiNamespaceListerWatcher(namespaces, func(namespace string) cache.ListerWatcher { @@ -188,6 +202,20 @@ func NewSecretController(ca ca.CertificateAuthority, requireOptIn bool, certTTL UpdateFunc: c.scrtUpdated, }) + namespaceLW := listwatch.MultiNamespaceListerWatcher(namespaces, func(namespace string) cache.ListerWatcher { + return &cache.ListWatch{ + ListFunc: func(options metav1.ListOptions) (runtime.Object, error) { + return core.Namespaces().List(options) + }, + WatchFunc: func(options metav1.ListOptions) (watch.Interface, error) { + return core.Namespaces().Watch(options) + }} + }) + c.namespaceStore, c.namespaceController = + cache.NewInformer(namespaceLW, &v1.Namespace{}, namespaceResyncPeriod, cache.ResourceEventHandlerFuncs{ + UpdateFunc: c.namespaceUpdated, + }) + return c, nil } @@ -200,6 +228,7 @@ func (sc *SecretController) Run(stopCh chan struct{}) { cache.WaitForCacheSync(stopCh, sc.scrtController.HasSynced) go sc.saController.Run(stopCh) + go sc.namespaceController.Run(stopCh) } // GetSecretName returns the secret name for a given service account name. @@ -207,42 +236,29 @@ func GetSecretName(saName string) string { return secretNamePrefix + saName } -// Determine if the object is "enabled" for Istio. +// Determine if the object is "enabled" for Citadel. // Currently this looks at the list of watched namespaces and the object's namespace annotation -func (sc *SecretController) istioEnabledObject(obj metav1.Object) bool { - if _, watched := sc.namespaces[obj.GetNamespace()]; watched || !sc.explicitOptIn { +func (sc *SecretController) citadelManagedObject(obj metav1.Object) bool { + + // todo(incfly) + // should be removed once listened namespaces flag phased out + if _, watched := sc.namespaces[obj.GetNamespace()]; watched { return true } - const label = "istio-managed" - enabled := !sc.explicitOptIn // for backward compatibility, Citadel always creates secrets - // @todo this should be changed to false once we communicate behavior change and ensure customers - // correctly mark their namespaces. Currently controlled via command line - ns, err := sc.core.Namespaces().Get(obj.GetNamespace(), metav1.GetOptions{}) - if err != nil || ns == nil { // @todo handle errors? Unit tests mocks don't create NS, only secrets - return enabled + if err != nil || ns == nil { // if we can't retrieve namespace details, fall back on default value + log.Errorf("could not retrieve namespace resource for object %s", obj.GetName()) + return sc.enableNamespacesByDefault } - if ns.Labels != nil { - if v, ok := ns.Labels[label]; ok { - switch strings.ToLower(v) { - case "enabled", "enable", "true", "yes", "y": - enabled = true - case "disabled", "disable", "false", "no", "n": - enabled = false - default: // leave default unchanged - break - } - } - } - return enabled + return sc.namespaceIsManaged(ns) } // Handles the event where a service account is added. func (sc *SecretController) saAdded(obj interface{}) { acct := obj.(*v1.ServiceAccount) - if sc.istioEnabledObject(acct.GetObjectMeta()) { + if sc.citadelManagedObject(acct.GetObjectMeta()) { sc.upsertSecret(acct.GetName(), acct.GetNamespace()) } sc.monitoring.ServiceAccountCreation.Increment() @@ -321,13 +337,69 @@ func (sc *SecretController) scrtDeleted(obj interface{}) { saName := scrt.Annotations[ServiceAccountNameAnnotationKey] if sa, err := sc.core.ServiceAccounts(scrt.GetNamespace()).Get(saName, metav1.GetOptions{}); err == nil { log.Infof("Re-creating deleted secret %s/%s.", scrt.GetNamespace(), GetSecretName(saName)) - if sc.istioEnabledObject(sa.GetObjectMeta()) { + if sc.citadelManagedObject(sa.GetObjectMeta()) { sc.upsertSecret(saName, scrt.GetNamespace()) } sc.monitoring.SecretDeletion.Increment() } } +func (sc *SecretController) namespaceUpdated(oldObj, newObj interface{}) { + oldNs := oldObj.(*v1.Namespace) + newNs := newObj.(*v1.Namespace) + + oldManaged := sc.namespaceIsManaged(oldNs) + newManaged := sc.namespaceIsManaged(newNs) + + if !oldManaged && newManaged { + sc.enableNamespaceRetroactive(newNs.GetName()) + return + } +} + +// namespaceIsManaged returns whether a given namespace object should be managed by Citadel +func (sc *SecretController) namespaceIsManaged(ns *v1.Namespace) bool { + nsLabels := ns.GetLabels() + + override, exists := nsLabels[NamespaceOverrideLabel] + if exists { + switch override { + case "true": + return true + case "false": + return false + } // if exists but not valid, should fall through to check for NamespaceManagedLabel + } + + targetNamespace, exists := nsLabels[NamespaceManagedLabel] + if !exists { + return sc.enableNamespacesByDefault + } + + if targetNamespace != sc.istioCaStorageNamespace { + return false // this instance is not the intended CA + } + + return true +} + +// enableNamespaceRetroactive generates secrets for all service accounts in a newly enabled namespace +// for instance: if a namespace had its {NamespaceOverrideLabel: false} label removed, and its namespace is targeted +// in NamespaceManagedLabel, then we should generate secrets for its ServiceAccounts +func (sc *SecretController) enableNamespaceRetroactive(namespace string) { + + serviceAccounts, err := sc.core.ServiceAccounts(namespace).List(metav1.ListOptions{}) + if err != nil { + log.Errorf("could not retrieve service account resources from namespace %s", namespace) + return + } + + for _, sa := range serviceAccounts.Items { + sc.upsertSecret(sa.GetName(), sa.GetNamespace()) // idempotent, since does not gen duplicate secrets + } + +} + func (sc *SecretController) generateKeyAndCert(saName string, saNamespace string) ([]byte, []byte, error) { id := spiffe.MustGenSpiffeURI(saNamespace, saName) if sc.dnsNames != nil { @@ -410,6 +482,17 @@ func (sc *SecretController) scrtUpdated(oldObj, newObj interface{}) { // one held by the ca (this may happen when the CA is restarted and // a new self-signed CA cert is generated). if certLifeTimeLeft < gracePeriod || !bytes.Equal(rootCertificate, scrt.Data[RootCertID]) { + // if the namespace is not managed, don't refresh the expired secret, delete it + secretNamespace, err := sc.core.Namespaces().Get(namespace, metav1.GetOptions{}) + if err == nil { + if !sc.namespaceIsManaged(secretNamespace) { // delete the expiring secret if namespace not Citadel managed + sc.deleteSecret(name, namespace) + return + } + } else { // in the case we couldn't retrieve namespace, we should proceed with cert refresh + log.Errorf("Failed to retrieve details for namespace %s, err %v", namespace, err) + } + if certLifeTimeLeft < gracePeriod { log.Infof("Refreshing about to expire secret %s/%s", namespace, GetSecretName(name)) } else { diff --git a/security/pkg/k8s/controller/workloadsecret_test.go b/security/pkg/k8s/controller/workloadsecret_test.go index 0c37022d82d1..297ca42f87c5 100644 --- a/security/pkg/k8s/controller/workloadsecret_test.go +++ b/security/pkg/k8s/controller/workloadsecret_test.go @@ -43,7 +43,7 @@ const ( ) var ( - requireExplicitOptIn = false + enableNamespacesByDefault = true caCert = []byte("fake CA cert") caKey = []byte("fake private key") @@ -58,6 +58,10 @@ func TestSecretController(t *testing.T) { Resource: "secrets", Version: "v1", } + nsSchema := schema.GroupVersionResource{ + Resource: "namespaces", + Version: "v1", + } testCases := map[string]struct { existingSecret *v1.Secret saToAdd *v1.ServiceAccount @@ -78,6 +82,7 @@ func TestSecretController(t *testing.T) { "adding service account creates new secret": { saToAdd: createServiceAccount("test", "test-ns"), expectedActions: []ktesting.Action{ + ktesting.NewGetAction(nsSchema, "test-ns", "test-ns"), ktesting.NewCreateAction(gvr, "test-ns", istioTestSecret), }, gracePeriodRatio: defaultGracePeriodRatio, @@ -95,12 +100,15 @@ func TestSecretController(t *testing.T) { existingSecret: istioTestSecret, saToAdd: createServiceAccount("test", "test-ns"), gracePeriodRatio: defaultGracePeriodRatio, - expectedActions: []ktesting.Action{}, - shouldFail: false, + expectedActions: []ktesting.Action{ + ktesting.NewGetAction(nsSchema, "test-ns", "test-ns"), + }, + shouldFail: false, }, "adding service account retries when failed": { saToAdd: createServiceAccount("test", "test-ns"), expectedActions: []ktesting.Action{ + ktesting.NewGetAction(nsSchema, "test-ns", "test-ns"), ktesting.NewCreateAction(gvr, "test-ns", istioTestSecret), ktesting.NewCreateAction(gvr, "test-ns", istioTestSecret), ktesting.NewCreateAction(gvr, "test-ns", istioTestSecret), @@ -112,6 +120,7 @@ func TestSecretController(t *testing.T) { "adding webhook service account": { saToAdd: createServiceAccount(sidecarInjectorSvcAccount, "test-ns"), expectedActions: []ktesting.Action{ + ktesting.NewGetAction(nsSchema, "test-ns", "test-ns"), ktesting.NewCreateAction(gvr, "test-ns", ca.BuildSecret("test", sidecarInjectorSvcAccount, "test-ns", certChain, caKey, rootCert, nil, nil, IstioSecretType)), }, @@ -143,9 +152,9 @@ func TestSecretController(t *testing.T) { Namespace: "test-ns", }, } - controller, err := NewSecretController(createFakeCA(), requireExplicitOptIn, defaultTTL, + controller, err := NewSecretController(createFakeCA(), enableNamespacesByDefault, defaultTTL, tc.gracePeriodRatio, defaultMinGracePeriod, false, client.CoreV1(), false, false, - []string{metav1.NamespaceAll}, webhooks) + []string{metav1.NamespaceAll}, webhooks, "test-ns") if tc.shouldFail { if err == nil { t.Errorf("should have failed to create secret controller") @@ -181,9 +190,9 @@ func TestSecretContent(t *testing.T) { saName := "test-serviceaccount" saNamespace := "test-namespace" client := fake.NewSimpleClientset() - controller, err := NewSecretController(createFakeCA(), requireExplicitOptIn, defaultTTL, + controller, err := NewSecretController(createFakeCA(), enableNamespacesByDefault, defaultTTL, defaultGracePeriodRatio, defaultMinGracePeriod, false, client.CoreV1(), false, false, - []string{metav1.NamespaceAll}, map[string]*DNSNameEntry{}) + []string{metav1.NamespaceAll}, map[string]*DNSNameEntry{}, "test-namespace") if err != nil { t.Errorf("Failed to create secret controller: %v", err) } @@ -199,14 +208,14 @@ func TestSecretContent(t *testing.T) { t.Errorf("Root cert verification error: expected %v but got %v", rootCert, secret.Data[RootCertID]) } if !bytes.Equal(append(signedCert, certChain...), secret.Data[CertChainID]) { - t.Errorf("Cert chain verification error: expected %v but got %v", certChain, secret.Data[CertChainID]) + t.Errorf("Cert chain verification error: expected %v but got %v\n\n\n", certChain, secret.Data[CertChainID]) } } func TestDeletedIstioSecret(t *testing.T) { client := fake.NewSimpleClientset() - controller, err := NewSecretController(createFakeCA(), requireExplicitOptIn, defaultTTL, + controller, err := NewSecretController(createFakeCA(), enableNamespacesByDefault, defaultTTL, defaultGracePeriodRatio, defaultMinGracePeriod, false, client.CoreV1(), false, false, - []string{metav1.NamespaceAll}, nil) + []string{metav1.NamespaceAll}, nil, "test-ns") if err != nil { t.Errorf("failed to create secret controller: %v", err) } @@ -223,6 +232,10 @@ func TestDeletedIstioSecret(t *testing.T) { Resource: "secrets", Version: "v1", } + nsGvr := schema.GroupVersionResource{ + Resource: "namespaces", + Version: "v1", + } testCases := map[string]struct { secret *v1.Secret @@ -232,6 +245,7 @@ func TestDeletedIstioSecret(t *testing.T) { secret: ca.BuildSecret("test-sa", "istio.test-sa", "test-ns", nil, nil, nil, nil, nil, IstioSecretType), expectedActions: []ktesting.Action{ ktesting.NewGetAction(saGvr, "test-ns", "test-sa"), + ktesting.NewGetAction(nsGvr, "test-ns", "test-ns"), ktesting.NewCreateAction(scrtGvr, "test-ns", ca.BuildSecret("test-sa", "istio.test-sa", "test-ns", nil, nil, nil, nil, nil, IstioSecretType)), }, }, @@ -259,10 +273,15 @@ func TestDeletedIstioSecret(t *testing.T) { } func TestUpdateSecret(t *testing.T) { - gvr := schema.GroupVersionResource{ + secretSchema := schema.GroupVersionResource{ Resource: "secrets", Version: "v1", } + nsSchema := schema.GroupVersionResource{ + Resource: "namespaces", + Version: "v1", + } + testCases := map[string]struct { expectedActions []ktesting.Action ttl time.Duration @@ -279,7 +298,8 @@ func TestUpdateSecret(t *testing.T) { }, "Update secret in grace period": { expectedActions: []ktesting.Action{ - ktesting.NewUpdateAction(gvr, "test-ns", istioTestSecret), + ktesting.NewGetAction(nsSchema, "test-ns", "test-ns"), + ktesting.NewUpdateAction(secretSchema, "test-ns", istioTestSecret), }, ttl: time.Hour, gracePeriodRatio: 1, // Always in grace period @@ -287,7 +307,8 @@ func TestUpdateSecret(t *testing.T) { }, "Update secret in min grace period": { expectedActions: []ktesting.Action{ - ktesting.NewUpdateAction(gvr, "test-ns", istioTestSecret), + ktesting.NewGetAction(nsSchema, "test-ns", "test-ns"), + ktesting.NewUpdateAction(secretSchema, "test-ns", istioTestSecret), }, ttl: 10 * time.Minute, gracePeriodRatio: 0.5, @@ -295,7 +316,8 @@ func TestUpdateSecret(t *testing.T) { }, "Update expired secret": { expectedActions: []ktesting.Action{ - ktesting.NewUpdateAction(gvr, "test-ns", istioTestSecret), + ktesting.NewGetAction(nsSchema, "test-ns", "test-ns"), + ktesting.NewUpdateAction(secretSchema, "test-ns", istioTestSecret), }, ttl: -time.Second, gracePeriodRatio: 0.5, @@ -303,7 +325,8 @@ func TestUpdateSecret(t *testing.T) { }, "Update secret with different root cert": { expectedActions: []ktesting.Action{ - ktesting.NewUpdateAction(gvr, "test-ns", istioTestSecret), + ktesting.NewGetAction(nsSchema, "test-ns", "test-ns"), + ktesting.NewUpdateAction(secretSchema, "test-ns", istioTestSecret), }, ttl: time.Hour, gracePeriodRatio: 0.5, @@ -312,7 +335,7 @@ func TestUpdateSecret(t *testing.T) { }, "Update secret with invalid certificate": { expectedActions: []ktesting.Action{ - ktesting.NewUpdateAction(gvr, "test-ns", istioTestSecret), + ktesting.NewUpdateAction(secretSchema, "test-ns", istioTestSecret), }, ttl: time.Hour, gracePeriodRatio: 0.5, @@ -324,9 +347,9 @@ func TestUpdateSecret(t *testing.T) { for k, tc := range testCases { client := fake.NewSimpleClientset() - controller, err := NewSecretController(createFakeCA(), requireExplicitOptIn, time.Hour, + controller, err := NewSecretController(createFakeCA(), enableNamespacesByDefault, time.Hour, tc.gracePeriodRatio, tc.minGracePeriod, false, client.CoreV1(), false, false, - []string{metav1.NamespaceAll}, nil) + []string{metav1.NamespaceAll}, nil, "") if err != nil { t.Errorf("failed to create secret controller: %v", err) } @@ -357,80 +380,143 @@ func TestUpdateSecret(t *testing.T) { } } -func TestSecretOptIn(t *testing.T) { +func TestManagedNamespaceRules(t *testing.T) { + testCases := map[string]struct { + ns *v1.Namespace + istioCaStorageNamespace string + enableNamespacesByDefault bool + result bool + }{ + "not managed by default, no override, and namespace label does not match actual ns => no secret": { + ns: createNS("unlabeled", map[string]string{}), + istioCaStorageNamespace: "random", + enableNamespacesByDefault: false, + result: false, + }, + "not managed by default, no override, and namespace matches => secret": { + ns: createNS("unlabeled", map[string]string{NamespaceManagedLabel: "test-ns"}), + istioCaStorageNamespace: "test-ns", + enableNamespacesByDefault: false, + result: true, + }, + "not managed by default, override is false, and namespace matches => no secret": { + ns: createNS("unlabeled", map[string]string{NamespaceManagedLabel: "test-ns", NamespaceOverrideLabel: "false"}), + istioCaStorageNamespace: "test-ns", + enableNamespacesByDefault: false, + result: false, + }, + "is managed by default, override is not present, and no namespace tag => secret": { + ns: createNS("unlabeled", map[string]string{}), + istioCaStorageNamespace: "test-ns", + enableNamespacesByDefault: true, + result: true, + }, + "is managed by default, override is false, and no namespace tag => no secret": { + ns: createNS("unlabeled", map[string]string{NamespaceOverrideLabel: "false"}), + istioCaStorageNamespace: "test-ns", + enableNamespacesByDefault: true, + result: false, + }, + } + + for k, tc := range testCases { + t.Run(k, func(t *testing.T) { + client := fake.NewSimpleClientset() + controller, err := NewSecretController(createFakeCA(), tc.enableNamespacesByDefault, defaultTTL, + defaultGracePeriodRatio, defaultMinGracePeriod, false, client.CoreV1(), false, false, + []string{metav1.NamespaceAll}, nil, tc.istioCaStorageNamespace) + if err != nil { + t.Errorf("failed to create secret controller: %v", err) + } + client.ClearActions() + + if err != nil { + t.Errorf("failed to create ns in %s: %v", k, err) + } + isManaged := controller.namespaceIsManaged(tc.ns) + + if isManaged != tc.result { + t.Errorf("Failure in test case %s: expected %t but got %t", k, tc.result, isManaged) + } + }) + } +} + +func TestRetroactiveNamespaceActivation(t *testing.T) { nsSchema := schema.GroupVersionResource{ Resource: "namespaces", Version: "v1", } + saSchema := schema.GroupVersionResource{ + Resource: "serviceaccounts", + Version: "v1", + } secretSchema := schema.GroupVersionResource{ Resource: "secrets", Version: "v1", } testCases := map[string]struct { - requireOptIn bool - ns *v1.Namespace - secret *v1.Secret - expectedActions []ktesting.Action + enableNamespacesByDefault bool + istioCaStorageNamespace string + oldNamespace *v1.Namespace + newNamespace *v1.Namespace + secret *v1.Secret + sa *v1.ServiceAccount + expectedActions []ktesting.Action }{ - "always create when opt-in not required": { - requireOptIn: false, - ns: createNS("unlabeled", map[string]string{}), - secret: ca.BuildSecret("test-sa", "istio.test-sa", "unlabeled", nil, nil, nil, nil, nil, IstioSecretType), + "toggling label ca.istio.io/env from false->true generates service accounts": { + enableNamespacesByDefault: false, + istioCaStorageNamespace: "citadel", + oldNamespace: createNS("test", map[string]string{NamespaceManagedLabel: ""}), + newNamespace: createNS("test", map[string]string{NamespaceManagedLabel: "citadel"}), + secret: ca.BuildSecret("test-sa", "istio.test-sa", "test", nil, nil, nil, nil, nil, IstioSecretType), + sa: createServiceAccount("test-sa", "test"), expectedActions: []ktesting.Action{ - ktesting.NewCreateAction(nsSchema, "", createNS("unlabeled", map[string]string{})), - ktesting.NewCreateAction(secretSchema, "unlabeled", ca.BuildSecret("test-sa", "istio.test-sa", "unlabeled", nil, nil, nil, nil, nil, IstioSecretType)), + ktesting.NewCreateAction(nsSchema, "", createNS("test", map[string]string{})), + ktesting.NewCreateAction(saSchema, "test", createServiceAccount("test-sa", "test")), + ktesting.NewListAction(saSchema, schema.GroupVersionKind{}, "test", metav1.ListOptions{}), + ktesting.NewCreateAction(secretSchema, "test", ca.BuildSecret("test-sa", "istio.test-sa", "test", nil, nil, nil, nil, nil, IstioSecretType)), }, }, - "opt-in required, no label => disabled": { - requireOptIn: true, - ns: createNS("unlabeled", map[string]string{}), - secret: ca.BuildSecret("test-sa", "istio.test-sa", "unlabeled", nil, nil, nil, nil, nil, IstioSecretType), + "toggling label ca.istio.io/env from unlabeled to false should not generate secret": { + enableNamespacesByDefault: false, + istioCaStorageNamespace: "citadel", + oldNamespace: createNS("test", map[string]string{}), + newNamespace: createNS("test", map[string]string{NamespaceManagedLabel: "false"}), + secret: ca.BuildSecret("test-sa", "istio.test-sa", "test", nil, nil, nil, nil, nil, IstioSecretType), + sa: createServiceAccount("test-sa", "test"), expectedActions: []ktesting.Action{ - ktesting.NewCreateAction(nsSchema, "", createNS("unlabeled", map[string]string{})), - ktesting.NewGetAction(nsSchema, "", "unlabeled"), - }, - }, - "opt-in required, disabled label => disabled": { - requireOptIn: true, - ns: createNS("disabled-ns", map[string]string{"istio-managed": "disabled"}), - secret: ca.BuildSecret("test-sa", "istio.test-sa", "disabled-ns", nil, nil, nil, nil, nil, IstioSecretType), - expectedActions: []ktesting.Action{ - ktesting.NewCreateAction(nsSchema, "", createNS("disabled-ns", map[string]string{"istio-managed": "disabled"})), - ktesting.NewGetAction(nsSchema, "", "disabled-ns"), - }, - }, - "opt-in required, enabled label => enabled": { - requireOptIn: true, - ns: createNS("enabled-ns", map[string]string{"istio-managed": "enabled"}), - secret: ca.BuildSecret("test-sa", "istio.test-sa", "enabled-ns", nil, nil, nil, nil, nil, IstioSecretType), - expectedActions: []ktesting.Action{ - ktesting.NewCreateAction(nsSchema, "", createNS("enabled-ns", map[string]string{"istio-managed": "enabled"})), - ktesting.NewGetAction(nsSchema, "", "enabled-ns"), - ktesting.NewCreateAction(secretSchema, "enabled-ns", ca.BuildSecret("test-sa", "istio.test-sa", "enabled-ns", nil, nil, nil, nil, nil, IstioSecretType)), + ktesting.NewCreateAction(nsSchema, "", createNS("test", map[string]string{})), + ktesting.NewCreateAction(saSchema, "test", createServiceAccount("test-sa", "test")), }, }, } for k, tc := range testCases { - client := fake.NewSimpleClientset() - controller, err := NewSecretController(createFakeCA(), tc.requireOptIn, defaultTTL, - defaultGracePeriodRatio, defaultMinGracePeriod, false, client.CoreV1(), false, false, - []string{metav1.NamespaceAll}, nil) - if err != nil { - t.Errorf("failed to create secret controller: %v", err) - } - client.ClearActions() + t.Run(k, func(t *testing.T) { + client := fake.NewSimpleClientset() + controller, err := NewSecretController(createFakeCA(), tc.enableNamespacesByDefault, defaultTTL, + defaultGracePeriodRatio, defaultMinGracePeriod, false, client.CoreV1(), false, false, + []string{metav1.NamespaceAll}, nil, tc.istioCaStorageNamespace) + if err != nil { + t.Errorf("failed to create secret controller: %v", err) + } + client.ClearActions() - _, err = client.Core().Namespaces().Create(tc.ns) - if err != nil { - t.Errorf("failed to create ns in %s: %v", k, err) - } - controller.saAdded(createServiceAccount("test-sa", tc.ns.Name)) + if _, err := client.Core().Namespaces().Create(tc.oldNamespace); err != nil { + t.Error(err) + } + if _, err := client.Core().ServiceAccounts(tc.oldNamespace.GetName()).Create(tc.sa); err != nil { + t.Error(err) + } - if err := checkActions(client.Actions(), tc.expectedActions); err != nil { - t.Errorf("Failure in test case %s: %v", k, err) - } + controller.namespaceUpdated(tc.oldNamespace, tc.newNamespace) + + if err := checkActions(client.Actions(), tc.expectedActions); err != nil { + t.Errorf("Failure in test case %s: %v", k, err) + } + }) } }