Refactor more certs to be issued by nodeup

nodeup/pkg/model/BUILD.bazel

@@ -94,6 +94,7 @@ go_test(
         "kubectl_test.go",
         "kubelet_test.go",
         "protokube_test.go",
+        "secrets_test.go",
     ],
     data = glob(["tests/**"]),  #keep
     embed = [":go_default_library"],

nodeup/pkg/model/kube_apiserver.go

@@ -288,7 +288,7 @@ func (b *KubeAPIServerBuilder) writeAuthenticationConfig(c *fi.ModelBuilderConte
 func (b *KubeAPIServerBuilder) buildPod() (*v1.Pod, error) {
 	kubeAPIServer := b.Cluster.Spec.KubeAPIServer
 	kubeAPIServer.ClientCAFile = filepath.Join(b.PathSrvKubernetes(), "ca.crt")
-	kubeAPIServer.TLSCertFile = filepath.Join(b.PathSrvKubernetes(), "server.cert")
+	kubeAPIServer.TLSCertFile = filepath.Join(b.PathSrvKubernetes(), "server.crt")
 	kubeAPIServer.TLSPrivateKeyFile = filepath.Join(b.PathSrvKubernetes(), "server.key")
 
 	// Support for basic auth was deprecated 1.16 and removed in 1.19
@@ -327,7 +327,7 @@ func (b *KubeAPIServerBuilder) buildPod() (*v1.Pod, error) {
 	}
 
 	{
-		certPath := filepath.Join(b.PathSrvKubernetes(), "apiserver-aggregator.cert")
+		certPath := filepath.Join(b.PathSrvKubernetes(), "apiserver-aggregator.crt")
 		kubeAPIServer.ProxyClientCertFile = &certPath
 		keyPath := filepath.Join(b.PathSrvKubernetes(), "apiserver-aggregator.key")
 		kubeAPIServer.ProxyClientKeyFile = &keyPath
@@ -341,7 +341,7 @@ func (b *KubeAPIServerBuilder) buildPod() (*v1.Pod, error) {
 		}
 
 		if cert != nil {
-			certPath := filepath.Join(b.PathSrvKubernetes(), "apiserver-aggregator-ca.cert")
+			certPath := filepath.Join(b.PathSrvKubernetes(), "apiserver-aggregator-ca.crt")
 			kubeAPIServer.RequestheaderClientCAFile = certPath
 		}
 	}

nodeup/pkg/model/secrets.go

@@ -21,6 +21,7 @@ import (
 	"path/filepath"
 	"strings"
 
+	"k8s.io/kops/pkg/model/components"
 	"k8s.io/kops/pkg/tokens"
 	"k8s.io/kops/upup/pkg/fi"
 	"k8s.io/kops/upup/pkg/fi/nodeup/nodetasks"
@@ -70,26 +71,59 @@ func (b *SecretBuilder) Build(c *fi.ModelBuilderContext) error {
 	}
 
 	{
-		name := "master"
+		// A few names used from inside the cluster, which all resolve the same based on our default suffixes
-		if err := b.BuildCertificateTask(c, name, "server.cert"); err != nil {
+		alternateNames := []string{
+			"kubernetes",
+			"kubernetes.default",
+			"kubernetes.default.svc",
+			"kubernetes.default.svc." + b.Cluster.Spec.ClusterDNSDomain,
+		}
+
+		// Names specified in the cluster spec
+		alternateNames = append(alternateNames, b.Cluster.Spec.MasterPublicName)
+		alternateNames = append(alternateNames, b.Cluster.Spec.MasterInternalName)
+		alternateNames = append(alternateNames, b.Cluster.Spec.AdditionalSANs...)
+
+		// Load balancer IPs passed in through NodeupConfig
+		alternateNames = append(alternateNames, b.NodeupConfig.ApiserverAdditionalIPs...)
+
+		// Referencing it by internal IP should work also
+		{
+			ip, err := components.WellKnownServiceIP(&b.Cluster.Spec, 1)
+			if err != nil {
 				return err
 			}
-		if err := b.BuildPrivateKeyTask(c, name, "server.key"); err != nil {
+			alternateNames = append(alternateNames, ip.String())
+		}
+
+		// We also want to be able to reference it locally via https://127.0.0.1
+		alternateNames = append(alternateNames, "127.0.0.1")
+
+		issueCert := &nodetasks.IssueCert{
+			Name:           "master",
+			Signer:         fi.CertificateIDCA,
+			Type:           "server",
+			Subject:        nodetasks.PKIXName{CommonName: "kubernetes-master"},
+			AlternateNames: alternateNames,
+		}
+		c.AddTask(issueCert)
+		err := issueCert.AddFileTasks(c, b.PathSrvKubernetes(), "server", "", nil)
+		if err != nil {
 			return err
 		}
 	}
 
 	{
-		if err := b.BuildCertificateTask(c, "apiserver-aggregator", "apiserver-aggregator.cert"); err != nil {
+		issueCert := &nodetasks.IssueCert{
-			return err
+			Name:   "apiserver-aggregator",
+			Signer: "apiserver-aggregator-ca",
+			Type:   "client",
+			// Must match RequestheaderAllowedNames
+			Subject: nodetasks.PKIXName{CommonName: "aggregator"},
 		}
-		if err := b.BuildPrivateKeyTask(c, "apiserver-aggregator", "apiserver-aggregator.key"); err != nil {
+		c.AddTask(issueCert)
-			return err
+		err := issueCert.AddFileTasks(c, b.PathSrvKubernetes(), "apiserver-aggregator", "apiserver-aggregator-ca", nil)
-		}
+		if err != nil {
-	}
-
-	{
-		if err := b.BuildCertificateTask(c, "apiserver-aggregator-ca", "apiserver-aggregator-ca.cert"); err != nil {
 			return err
 		}
 	}

nodeup/pkg/model/secrets_test.go

@@ -0,0 +1,30 @@
+/*
+Copyright 2020 The Kubernetes 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 model
+
+import (
+	"testing"
+
+	"k8s.io/kops/upup/pkg/fi"
+)
+
+func TestSecretBuilder(t *testing.T) {
+	RunGoldenTest(t, "tests/golden/minimal", "secret", func(nodeupModelContext *NodeupModelContext, target *fi.ModelBuilderContext) error {
+		builder := SecretBuilder{NodeupModelContext: nodeupModelContext}
+		return builder.Build(target)
+	})
+}

nodeup/pkg/model/tests/golden/minimal/tasks-kube-apiserver.yaml

@@ -32,17 +32,17 @@ contents: |
       - --kubelet-client-certificate=/srv/kubernetes/kubelet-api.crt
       - --kubelet-client-key=/srv/kubernetes/kubelet-api.key
       - --kubelet-preferred-address-types=InternalIP,Hostname,ExternalIP
-      - --proxy-client-cert-file=/srv/kubernetes/apiserver-aggregator.cert
+      - --proxy-client-cert-file=/srv/kubernetes/apiserver-aggregator.crt
       - --proxy-client-key-file=/srv/kubernetes/apiserver-aggregator.key
       - --requestheader-allowed-names=aggregator
-      - --requestheader-client-ca-file=/srv/kubernetes/apiserver-aggregator-ca.cert
+      - --requestheader-client-ca-file=/srv/kubernetes/apiserver-aggregator-ca.crt
       - --requestheader-extra-headers-prefix=X-Remote-Extra-
       - --requestheader-group-headers=X-Remote-Group
       - --requestheader-username-headers=X-Remote-User
       - --secure-port=443
       - --service-cluster-ip-range=100.64.0.0/13
       - --storage-backend=etcd3
-      - --tls-cert-file=/srv/kubernetes/server.cert
+      - --tls-cert-file=/srv/kubernetes/server.crt
       - --tls-private-key-file=/srv/kubernetes/server.key
       - --v=2
       - --logtostderr=false

nodeup/pkg/model/tests/golden/minimal/tasks-secret.yaml

@@ -0,0 +1,120 @@
+mode: "0755"
+path: /srv/kubernetes
+type: directory
+---
+contents:
+  task:
+    Name: apiserver-aggregator
+    signer: apiserver-aggregator-ca
+    subject:
+      CommonName: aggregator
+    type: client
+mode: "0644"
+path: /srv/kubernetes/apiserver-aggregator-ca.crt
+type: file
+---
+contents:
+  task:
+    Name: apiserver-aggregator
+    signer: apiserver-aggregator-ca
+    subject:
+      CommonName: aggregator
+    type: client
+mode: "0644"
+path: /srv/kubernetes/apiserver-aggregator.crt
+type: file
+---
+contents:
+  task:
+    Name: apiserver-aggregator
+    signer: apiserver-aggregator-ca
+    subject:
+      CommonName: aggregator
+    type: client
+mode: "0600"
+path: /srv/kubernetes/apiserver-aggregator.key
+type: file
+---
+contents: |
+  -----BEGIN CERTIFICATE-----
+  MIIC2DCCAcCgAwIBAgIRALJXAkVj964tq67wMSI8oJQwDQYJKoZIhvcNAQELBQAw
+  FTETMBEGA1UEAxMKa3ViZXJuZXRlczAeFw0xNzEyMjcyMzUyNDBaFw0yNzEyMjcy
+  MzUyNDBaMBUxEzARBgNVBAMTCmt1YmVybmV0ZXMwggEiMA0GCSqGSIb3DQEBAQUA
+  A4IBDwAwggEKAoIBAQDgnCkSmtnmfxEgS3qNPaUCH5QOBGDH/inHbWCODLBCK9gd
+  XEcBl7FVv8T2kFr1DYb0HVDtMI7tixRVFDLgkwNlW34xwWdZXB7GeoFgU1xWOQSY
+  OACC8JgYTQ/139HBEvgq4sej67p+/s/SNcw34Kk7HIuFhlk1rRk5kMexKIlJBKP1
+  YYUYetsJ/QpUOkqJ5HW4GoetE76YtHnORfYvnybviSMrh2wGGaN6r/s4ChOaIbZC
+  An8/YiPKGIDaZGpj6GXnmXARRX/TIdgSQkLwt0aTDBnPZ4XvtpI8aaL8DYJIqAzA
+  NPH2b4/uNylat5jDo0b0G54agMi97+2AUrC9UUXpAgMBAAGjIzAhMA4GA1UdDwEB
+  /wQEAwIBBjAPBgNVHRMBAf8EBTADAQH/MA0GCSqGSIb3DQEBCwUAA4IBAQBVGR2r
+  hzXzRMU5wriPQAJScszNORvoBpXfZoZ09FIupudFxBVU3d4hV9StKnQgPSGA5XQO
+  HE97+BxJDuA/rB5oBUsMBjc7y1cde/T6hmi3rLoEYBSnSudCOXJE4G9/0f8byAJe
+  rN8+No1r2VgZvZh6p74TEkXv/l3HBPWM7IdUV0HO9JDhSgOVF1fyQKJxRuLJR8jt
+  O6mPH2UX0vMwVa4jvwtkddqk2OAdYQvH9rbDjjbzaiW0KnmdueRo92KHAN7BsDZy
+  VpXHpqo1Kzg7D3fpaXCf5si7lqqrdJVXH4JC72zxsPehqgi8eIuqOBkiDWmRxAxh
+  8yGeRx9AbknHh4Ia
+  -----END CERTIFICATE-----
+mode: "0600"
+path: /srv/kubernetes/ca.crt
+type: file
+---
+contents:
+  task:
+    Name: master
+    alternateNames:
+    - kubernetes
+    - kubernetes.default
+    - kubernetes.default.svc
+    - kubernetes.default.svc.cluster.local
+    - api.minimal.example.com
+    - api.internal.minimal.example.com
+    - 100.64.0.1
+    - 127.0.0.1
+    signer: ca
+    subject:
+      CommonName: kubernetes-master
+    type: server
+mode: "0644"
+path: /srv/kubernetes/server.crt
+type: file
+---
+contents:
+  task:
+    Name: master
+    alternateNames:
+    - kubernetes
+    - kubernetes.default
+    - kubernetes.default.svc
+    - kubernetes.default.svc.cluster.local
+    - api.minimal.example.com
+    - api.internal.minimal.example.com
+    - 100.64.0.1
+    - 127.0.0.1
+    signer: ca
+    subject:
+      CommonName: kubernetes-master
+    type: server
+mode: "0600"
+path: /srv/kubernetes/server.key
+type: file
+---
+Name: apiserver-aggregator
+signer: apiserver-aggregator-ca
+subject:
+  CommonName: aggregator
+type: client
+---
+Name: master
+alternateNames:
+- kubernetes
+- kubernetes.default
+- kubernetes.default.svc
+- kubernetes.default.svc.cluster.local
+- api.minimal.example.com
+- api.internal.minimal.example.com
+- 100.64.0.1
+- 127.0.0.1
+signer: ca
+subject:
+  CommonName: kubernetes-master
+type: server

pkg/apis/nodeup/config.go

@@ -44,6 +44,8 @@ type Config struct {
 	ProtokubeImage *Image `json:"protokubeImage,omitempty"`
 	// Channels is a list of channels that we should apply
 	Channels []string `json:"channels,omitempty"`
+	// ApiserverAdditionalIPs are additional IP address to put in the apiserver server cert.
+	ApiserverAdditionalIPs []string `json:",omitempty"`
 
 	// Manifests for running etcd
 	EtcdManifests []string `json:"etcdManifests,omitempty"`

pkg/model/bootstrapscript.go

@@ -40,7 +40,7 @@ import (
 )
 
 type NodeUpConfigBuilder interface {
-	BuildConfig(ig *kops.InstanceGroup) (*nodeup.Config, error)
+	BuildConfig(ig *kops.InstanceGroup, apiserverAdditionalIPs []string) (*nodeup.Config, error)
 }
 
 // BootstrapScriptBuilder creates the bootstrap script
@@ -55,14 +55,33 @@ type BootstrapScript struct {
 	ig       *kops.InstanceGroup
 	builder  *BootstrapScriptBuilder
 	resource fi.TaskDependentResource
+	// alternateNameTasks are tasks that contribute api-server IP addresses.
+	alternateNameTasks []fi.HasAddress
 }
 
 var _ fi.Task = &BootstrapScript{}
 var _ fi.HasName = &BootstrapScript{}
+var _ fi.HasDependencies = &BootstrapScript{}
 
 // kubeEnv returns the nodeup config for the instance group
-func (b *BootstrapScript) kubeEnv(ig *kops.InstanceGroup) (string, error) {
+func (b *BootstrapScript) kubeEnv(ig *kops.InstanceGroup, c *fi.Context) (string, error) {
-	config, err := b.builder.NodeUpConfigBuilder.BuildConfig(ig)
+	var alternateNames []string
+
+	for _, hasAddress := range b.alternateNameTasks {
+		address, err := hasAddress.FindIPAddress(c)
+		if err != nil {
+			return "", fmt.Errorf("error finding address for %v: %v", hasAddress, err)
+		}
+		if address == nil {
+			klog.Warningf("Task did not have an address: %v", hasAddress)
+			continue
+		}
+		klog.V(8).Infof("Resolved alternateName %q for %q", *address, hasAddress)
+		alternateNames = append(alternateNames, *address)
+	}
+
+	sort.Strings(alternateNames)
+	config, err := b.builder.NodeUpConfigBuilder.BuildConfig(ig, alternateNames)
 	if err != nil {
 		return "", err
 	}
@@ -169,6 +188,19 @@ func (b *BootstrapScript) GetName() *string {
 	return &b.Name
 }
 
+func (b *BootstrapScript) GetDependencies(tasks map[string]fi.Task) []fi.Task {
+	var deps []fi.Task
+
+	for _, task := range tasks {
+		if hasAddress, ok := task.(fi.HasAddress); ok && hasAddress.IsForAPIServer() {
+			deps = append(deps, task)
+			b.alternateNameTasks = append(b.alternateNameTasks, hasAddress)
+		}
+	}
+
+	return deps
+}
+
 func (b *BootstrapScript) Run(c *fi.Context) error {
 	functions := template.FuncMap{
 		"NodeUpSourceAmd64": func() string {
@@ -184,7 +216,7 @@ func (b *BootstrapScript) Run(c *fi.Context) error {
 			return b.builder.NodeUpSourceHash[architectures.ArchitectureArm64]
 		},
 		"KubeEnv": func() (string, error) {
-			return b.kubeEnv(b.ig)
+			return b.kubeEnv(b.ig, c)
 		},
 
 		"EnvironmentVariables": func() (string, error) {

pkg/model/bootstrapscript_test.go

@@ -60,7 +60,7 @@ type nodeupConfigBuilder struct {
 	cluster *kops.Cluster
 }
 
-func (n *nodeupConfigBuilder) BuildConfig(ig *kops.InstanceGroup) (*nodeup.Config, error) {
+func (n *nodeupConfigBuilder) BuildConfig(ig *kops.InstanceGroup, apiserverAdditionalIPs []string) (*nodeup.Config, error) {
 	return nodeup.NewConfig(n.cluster, ig), nil
 }
 

pkg/model/openstackmodel/servergroup_test.go

@@ -3203,7 +3203,7 @@ func compareErrors(t *testing.T, actual, expected error) {
 type nodeupConfigBuilder struct {
 }
 
-func (n *nodeupConfigBuilder) BuildConfig(ig *kops.InstanceGroup) (*nodeup.Config, error) {
+func (n *nodeupConfigBuilder) BuildConfig(ig *kops.InstanceGroup, apiserverAdditionalIPs []string) (*nodeup.Config, error) {
 	return &nodeup.Config{}, nil
 }
 

pkg/model/pki.go

@@ -169,53 +169,6 @@ func (b *PKIModelBuilder) Build(c *fi.ModelBuilderContext) error {
 			Type:      "ca",
 		}
 		c.AddTask(aggregatorCA)
-
-		aggregator := &fitasks.Keypair{
-			Name:      fi.String("apiserver-aggregator"),
-			Lifecycle: b.Lifecycle,
-			// Must match RequestheaderAllowedNames
-			Subject: "cn=aggregator",
-			Type:    "client",
-			Signer:  aggregatorCA,
-		}
-		c.AddTask(aggregator)
-	}
-
-	{
-		// A few names used from inside the cluster, which all resolve the same based on our default suffixes
-		alternateNames := []string{
-			"kubernetes",
-			"kubernetes.default",
-			"kubernetes.default.svc",
-			"kubernetes.default.svc." + b.Cluster.Spec.ClusterDNSDomain,
-		}
-
-		// Names specified in the cluster spec
-		alternateNames = append(alternateNames, b.Cluster.Spec.MasterPublicName)
-		alternateNames = append(alternateNames, b.Cluster.Spec.MasterInternalName)
-		alternateNames = append(alternateNames, b.Cluster.Spec.AdditionalSANs...)
-
-		// Referencing it by internal IP should work also
-		{
-			ip, err := b.WellKnownServiceIP(1)
-			if err != nil {
-				return err
-			}
-			alternateNames = append(alternateNames, ip.String())
-		}
-
-		// We also want to be able to reference it locally via https://127.0.0.1
-		alternateNames = append(alternateNames, "127.0.0.1")
-
-		t := &fitasks.Keypair{
-			Name:           fi.String("master"),
-			Lifecycle:      b.Lifecycle,
-			Subject:        "cn=kubernetes-master",
-			Type:           "server",
-			AlternateNames: alternateNames,
-			Signer:         defaultCA,
-		}
-		c.AddTask(t)
 	}
 
 	// @TODO this is VERY presumptuous, i'm going on the basis we can make it configurable in the future.

upup/pkg/fi/cloudup/apply_cluster.go

@@ -1279,7 +1279,7 @@ func (c *ApplyClusterCmd) newNodeUpConfigBuilder(assetBuilder *assets.AssetBuild
 }
 
 // BuildNodeUpConfig returns the NodeUp config, in YAML format
-func (n *nodeUpConfigBuilder) BuildConfig(ig *kops.InstanceGroup) (*nodeup.Config, error) {
+func (n *nodeUpConfigBuilder) BuildConfig(ig *kops.InstanceGroup, apiserverAdditionalIPs []string) (*nodeup.Config, error) {
 	cluster := n.cluster
 
 	if ig == nil {
@@ -1305,6 +1305,10 @@ func (n *nodeUpConfigBuilder) BuildConfig(ig *kops.InstanceGroup) (*nodeup.Confi
 	config.ConfigBase = fi.String(n.configBase.Path())
 	config.InstanceGroupName = ig.ObjectMeta.Name
 
+	if role == kops.InstanceGroupRoleMaster {
+		config.ApiserverAdditionalIPs = apiserverAdditionalIPs
+	}
+
 	for _, manifest := range n.assetBuilder.StaticManifests {
 		match := false
 		for _, r := range manifest.Roles {

upup/pkg/fi/fitasks/keypair.go

@@ -34,8 +34,6 @@ type Keypair struct {
 	Name *string
 	// AlternateNames a list of alternative names for this certificate
 	AlternateNames []string `json:"alternateNames"`
-	// AlternateNameTasks is a collection of subtask
-	AlternateNameTasks []fi.HasAddress `json:"alternateNameTasks"`
 	// Lifecycle is context for a task
 	Lifecycle *fi.Lifecycle
 	// Signer is the keypair to use to sign, for when we want to use an alternative CA
@@ -50,7 +48,6 @@ type Keypair struct {
 
 var _ fi.HasCheckExisting = &Keypair{}
 var _ fi.HasName = &Keypair{}
-var _ fi.HasDependencies = &Keypair{}
 
 // It's important always to check for the existing key, so we don't regenerate keys e.g. on terraform
 func (e *Keypair) CheckExisting(c *fi.Context) bool {
@@ -63,25 +60,6 @@ func (e *Keypair) CompareWithID() *string {
 	return &e.Subject
 }
 
-func (e *Keypair) GetDependencies(tasks map[string]fi.Task) []fi.Task {
-	var deps []fi.Task
-
-	if e.Signer != nil {
-		deps = append(deps, e.Signer)
-	}
-
-	if *e.Name == "master" {
-		for _, task := range tasks {
-			if hasAddress, ok := task.(fi.HasAddress); ok && hasAddress.IsForAPIServer() {
-				deps = append(deps, task)
-				e.AlternateNameTasks = append(e.AlternateNameTasks, hasAddress)
-			}
-		}
-	}
-
-	return deps
-}
-
 func (e *Keypair) Find(c *fi.Context) (*Keypair, error) {
 	name := fi.StringValue(e.Name)
 	if name == "" {
@@ -124,14 +102,14 @@ func (e *Keypair) Find(c *fi.Context) (*Keypair, error) {
 }
 
 func (e *Keypair) Run(c *fi.Context) error {
-	err := e.normalize(c)
+	err := e.normalize()
 	if err != nil {
 		return err
 	}
 	return fi.DefaultDeltaRunMethod(e, c)
 }
 
-func (e *Keypair) normalize(c *fi.Context) error {
+func (e *Keypair) normalize() error {
 	var alternateNames []string
 
 	for _, s := range e.AlternateNames {
@@ -142,22 +120,8 @@ func (e *Keypair) normalize(c *fi.Context) error {
 		alternateNames = append(alternateNames, s)
 	}
 
-	for _, hasAddress := range e.AlternateNameTasks {
-		address, err := hasAddress.FindIPAddress(c)
-		if err != nil {
-			return fmt.Errorf("error finding address for %v: %v", hasAddress, err)
-		}
-		if address == nil {
-			klog.Warningf("Task did not have an address: %v", hasAddress)
-			continue
-		}
-		klog.V(8).Infof("Resolved alternateName %q for %q", *address, hasAddress)
-		alternateNames = append(alternateNames, *address)
-	}
-
 	sort.Strings(alternateNames)
 	e.AlternateNames = alternateNames
-	e.AlternateNameTasks = nil
 
 	return nil
 }