Skip to content
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
2 changes: 2 additions & 0 deletions addons/metrics-server/README.md
Original file line number Diff line number Diff line change
@@ -1,5 +1,7 @@
# Kubernetes Metrics Server

**This addon is deprecated. Set `spec.metricsServer.enabled: true` instead**
Copy link
Contributor

@agilgur5 agilgur5 Oct 8, 2020

Choose a reason for hiding this comment

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

Wouldn't it be good to also update the manifest's line anyway?

- --kubelet-insecure-tls

Might need to add a new manifest though for a new version, not sure if this is getting backported

I would think that may make it easier for gradual transitions or just understanding that there's a difference between the two variants -- an important security one at that. Otherwise may make it seem like they're otherwise equivalent. I was using the metrics-server Helm Chart instead of the add-on as well, but looking at the add-on made me realize they both shared a security risk.
Alternatively, could add a note in the docs here that the deprecated manifest variant is less secure due to --kubelet-insecure-tls usage.

Copy link
Member Author

Choose a reason for hiding this comment

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

This is a new feature, so it won't be backported. Adding a new manifest would also only be for 1.19+, so that kind of defeats the purpose of the deprecation notice.

But what I can do is add a note to the release notes. Should have done so anyway.

Copy link
Contributor

@agilgur5 agilgur5 Apr 7, 2021

Choose a reason for hiding this comment

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

Gotcha, makes sense. Appreciate all the explanations! I was looking back into this due to some security settings (kube-apiserver not setting --kubelet-certificate-authority) and the release notes for #10836 , and it seems like I forgot to ever respond here.

The release note on metrics-server doesn't say that the new add-on is more secure -- which is a really key piece. The wording makes it sound like the add-on is just done via the new approach now, so other than deprecation it doesn't motivate one to move to the new method as the security piece isn't explicitly mentioned. I was previously using metrics-server directly via its Helm chart, so I also wouldn't know that I could now remove --kubelet-insecure-tls from my config if I didn't dive into and look through the issues / PRs another time.
I think it would be useful to mention that it is more secure than the now deprecated variant and that users of direct metrics-server should remove the --kubelet-insecure-tls flag when upgrading to kOps 1.19+

Copy link
Member Author

Choose a reason for hiding this comment

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

Good suggestions! Thanks! Would you be able to add a PR for this?

Copy link
Contributor

@agilgur5 agilgur5 Apr 16, 2023

Choose a reason for hiding this comment

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

It only took me 2 years to finally get the PR out, but finally made it: #15327 😅

Sorry for the delay -- at least I did get several other PRs I had in my backlog out as well!


## User guide

You can find the user guide in
Expand Down
6 changes: 6 additions & 0 deletions cmd/kops-controller/pkg/server/server.go
Original file line number Diff line number Diff line change
Expand Up @@ -175,6 +175,12 @@ func (s *Server) issueCert(name string, pubKey string, id *fi.VerifyResult, vali
CommonName: fmt.Sprintf("system:node:%s", id.NodeName),
Organization: []string{rbac.NodesGroup},
}
case "kubelet-server":
issueReq.Subject = pkix.Name{
CommonName: id.NodeName,
}
issueReq.AlternateNames = []string{id.NodeName}
issueReq.Type = "server"
case "kube-proxy":
issueReq.Subject = pkix.Name{
CommonName: rbac.KubeProxy,
Expand Down
4 changes: 4 additions & 0 deletions docs/releases/1.19-NOTES.md
Original file line number Diff line number Diff line change
Expand Up @@ -42,6 +42,8 @@ The expiration times vary randomly so that nodes are likely to have their certs

* New command for deleting a single instance: [kops delete instance](/docs/cli/kops_delete_instance/)

* Metrics Server is now available as a configurable addon. Add `spec.metricsServer.enabled: true` to the cluster spec to enable.

# Breaking changes

* Support for Kubernetes 1.9 and 1.10 has been removed.
Expand All @@ -62,6 +64,8 @@ The expiration times vary randomly so that nodes are likely to have their certs

* Support for feature flag `Terraform-0.12` has been deprecated and will be removed in kops 1.20. All generated Terraform HCL2/JSON files will support versions `0.12.26+` and `0.13.0+`.

* The [manifest based metrics server addon](https://github.com/kubernetes/kops/tree/master/addons/metrics-server) has been deprecated in favour of a configurable addon.

# Full change list since 1.18.0 release

## v1.18.0-alpha.3 to v1.19.0-alpha.1
Expand Down
10 changes: 10 additions & 0 deletions k8s/crds/kops.k8s.io_clusters.yaml
Original file line number Diff line number Diff line change
Expand Up @@ -2088,6 +2088,16 @@ spec:
masterPublicName:
description: MasterPublicName is the external DNS name for the master nodes
type: string
metricsServer:
description: MetricsServerConfig determines the metrics server configuration.
properties:
enabled:
description: 'Enabled enables the metrics server. Default: false'
type: boolean
image:
description: 'Image is the docker container used. Default: the latest supported image for the specified kubernetes version.'
type: string
type: object
networkCIDR:
description: NetworkCIDR is the CIDR used for the AWS VPC / GCE Network, or otherwise allocated to k8s This is a real CIDR, not the internal k8s network On AWS, it maps to the VPC CIDR. It is not required on GCE.
type: string
Expand Down
58 changes: 58 additions & 0 deletions nodeup/pkg/model/kubelet.go
Original file line number Diff line number Diff line change
Expand Up @@ -56,6 +56,12 @@ var _ fi.ModelBuilder = &KubeletBuilder{}

// Build is responsible for building the kubelet configuration
func (b *KubeletBuilder) Build(c *fi.ModelBuilderContext) error {

err := b.buildKubeletServingCertificate(c)
if err != nil {
return fmt.Errorf("error building kubelet server cert: %v", err)
}

kubeletConfig, err := b.buildKubeletConfig()
if err != nil {
return fmt.Errorf("error building kubelet config: %v", err)
Expand Down Expand Up @@ -226,6 +232,11 @@ func (b *KubeletBuilder) buildSystemdEnvironmentFile(kubeletConfig *kops.Kubelet
}
}

if b.UseKopsControllerForNodeBootstrap() {
flags += " --tls-cert-file=" + b.PathSrvKubernetes() + "/kubelet-server.crt"
flags += " --tls-private-key-file=" + b.PathSrvKubernetes() + "/kubelet-server.key"
}

sysconfig := "DAEMON_ARGS=\"" + flags + "\"\n"
// Makes kubelet read /root/.docker/config.json properly
sysconfig = sysconfig + "HOME=\"/root" + "\"\n"
Expand Down Expand Up @@ -538,3 +549,50 @@ func (b *KubeletBuilder) buildMasterKubeletKubeconfig(c *fi.ModelBuilderContext)

return b.BuildIssuedKubeconfig("kubelet", certName, c), nil
}

func (b *KubeletBuilder) buildKubeletServingCertificate(c *fi.ModelBuilderContext) error {

if b.UseKopsControllerForNodeBootstrap() {
name := "kubelet-server"
dir := b.PathSrvKubernetes()
signer := fi.CertificateIDCA

nodeName, err := b.NodeName()
if err != nil {
return err
}

if !b.IsMaster {
cert, key := b.GetBootstrapCert(name)

c.AddTask(&nodetasks.File{
Path: filepath.Join(dir, name+".crt"),
Contents: cert,
Type: nodetasks.FileType_File,
Mode: fi.String("0644"),
})

c.AddTask(&nodetasks.File{
Path: filepath.Join(dir, name+".key"),
Contents: key,
Type: nodetasks.FileType_File,
Mode: fi.String("0400"),
})

} else {
issueCert := &nodetasks.IssueCert{
Name: name,
Signer: signer,
Type: "server",
Subject: nodetasks.PKIXName{
CommonName: nodeName,
},
AlternateNames: []string{nodeName},
}
c.AddTask(issueCert)
return issueCert.AddFileTasks(c, dir, name, "", nil)
}
}
return nil

}
2 changes: 2 additions & 0 deletions pkg/apis/kops/cluster.go
Original file line number Diff line number Diff line change
Expand Up @@ -160,6 +160,8 @@ type ClusterSpec struct {

// NodeTerminationHandlerConfig determines the cluster autoscaler configuration.
NodeTerminationHandler *NodeTerminationHandlerConfig `json:"nodeTerminationHandler,omitempty"`
// MetricsServerConfig determines the metrics server configuration.
MetricsServer *MetricsServerConfig `json:"metricsServer,omitempty"`

// Networking configuration
Networking *NetworkingSpec `json:"networking,omitempty"`
Expand Down
10 changes: 10 additions & 0 deletions pkg/apis/kops/componentconfig.go
Original file line number Diff line number Diff line change
Expand Up @@ -803,6 +803,16 @@ type ClusterAutoscalerConfig struct {
Image *string `json:"image,omitempty"`
}

// MetricsServerConfig determines the metrics server configuration.
type MetricsServerConfig struct {
// Enabled enables the metrics server.
// Default: false
Enabled *bool `json:"enabled,omitempty"`
// Image is the docker container used.
// Default: the latest supported image for the specified kubernetes version.
Image *string `json:"image,omitempty"`
}

// HasAdmissionController checks if a specific admission controller is enabled
func (c *KubeAPIServerConfig) HasAdmissionController(name string) bool {
for _, x := range c.AdmissionControl {
Expand Down
2 changes: 2 additions & 0 deletions pkg/apis/kops/v1alpha2/cluster.go
Original file line number Diff line number Diff line change
Expand Up @@ -159,6 +159,8 @@ type ClusterSpec struct {

// NodeTerminationHandlerConfig determines the cluster autoscaler configuration.
NodeTerminationHandler *NodeTerminationHandlerConfig `json:"nodeTerminationHandler,omitempty"`
// MetricsServerConfig determines the metrics server configuration.
MetricsServer *MetricsServerConfig `json:"metricsServer,omitempty"`

// Networking configuration
Networking *NetworkingSpec `json:"networking,omitempty"`
Expand Down
10 changes: 10 additions & 0 deletions pkg/apis/kops/v1alpha2/componentconfig.go
Original file line number Diff line number Diff line change
Expand Up @@ -804,6 +804,16 @@ type ClusterAutoscalerConfig struct {
Image *string `json:"image,omitempty"`
}

// MetricsServerConfig determines the metrics server configuration.
type MetricsServerConfig struct {
// Enabled enables the metrics server.
// Default: false
Enabled *bool `json:"enabled,omitempty"`
// Image is the docker container used.
// Default: the latest supported image for the specified kubernetes version.
Image *string `json:"image,omitempty"`
}

// HasAdmissionController checks if a specific admission controller is enabled
func (c *KubeAPIServerConfig) HasAdmissionController(name string) bool {
for _, x := range c.AdmissionControl {
Expand Down
50 changes: 50 additions & 0 deletions pkg/apis/kops/v1alpha2/zz_generated.conversion.go

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

31 changes: 31 additions & 0 deletions pkg/apis/kops/v1alpha2/zz_generated.deepcopy.go

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

31 changes: 31 additions & 0 deletions pkg/apis/kops/zz_generated.deepcopy.go

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

Loading