Discover Good First Issues for kubernetes/kubernetes

There are a few cases when the Unknown result can be returned by the probe. For example see: https://github.com/kubernetes/kubernetes/issues/106682.

It will be useful to expose those in logs and as an event similar to what we do for Warning: https://github.com/kubernetes/kubernetes/blob/35f3fc59c1f29eebc8ff8705ecc3d4db7c4cbbc6/pkg/kubelet/prober/prober.go#L115

What needs to be done:

1. Implement logging and exposing event in case of Unknown probe result somewhere around https://github.com/kubernetes/kubernetes/blob/35f3fc59c1f29eebc8ff8705ecc3d4db7c4cbbc6/pkg/kubelet/prober/prober.go#L115

2. For each instance of probes potentially returning the Unknown result have a test that validates this test case. It may be either e2e test or unit test. You may find an example of e2e test that results in Unknown result from exec probe in one of the links from https://github.com/kubernetes/kubernetes/issues/106682

/sig node

/help wanted

/good-first-issue

/priority backlog

This issue is marked as “good first issue”. Before assigning issue to yourself, please make sure you understand the assignment and list cases you will need to cover with tests.

cc @aojea

Comments: 23

Today we construct the request object for every http request for the http probes. This involves concatenating some strings, setting headers, etc.

Probes are simple http get requests that are executed synchronously (for a single probe) and has no request body.

The optimization may be to reuse the request object for all probe executions. This likely save some cpu and memory for the kubelet process.

Some code references:

• DoProbe is executed today on each probe execution.

• New request are being created for each execution.

• New object will need to be stored in worker that for http probes will hold the http request object. This object will need to encapsulate the logic of runProbe method of the prober.

I’m marking this issue as a good first issue, as the issue doesn’t require deep knowledge of Kubernetes. However it is not an easy issue, a few parts of code will be affected and change needs to be done very carefully.

Please review code and make sure you understand the task before self-assigning the issue.

Implementation notes

1. If you can send the small benchmark results on probes with and without this optimization, it will be a good starting point. Note, for the benchmark, each probe can run once a second, not more often. But the number of probes is virtually unlimited since we are not defining the limit on number of containers per Pod. Realistically we are talking about 110 pods (max) with maybe 2 containers each, with 3 probes each (660 probes).

2. Code change may be big. So make sure to help reviewers to review the code. One idea may be to split implementation into multiple commits to simplify the review. First, creating the object encapsulating the logic of runProbe, second storing and initializing this object in worker, and third, actual reusing of the request object.

/sig node

/good-first-issue

/help-wanted

/area kubelet

Comments: 24

Probes of all kinds currently support FailureThreshold (and SuccessThreshold), these properties allow a user to specify that Kubernetes should not take action in response to a failed probe unless it fails a successive number of times.

This is useful for end-users as it allows them to mitigate the effects of any probes that “flake” by requiring successive failure.

When a probe fails in Kubernetes, we emit a container event indicating this here: https://github.com/kubernetes/kubernetes/blob/master/pkg/kubelet/prober/prober.go#L110 and end-users can consume these events via the API for their own purposes. This event is emitted regardless of whether the FailureThreshold has been reached or not.

Currently when a user consumes a probe failure event they have no way of knowing whether the event resulted in action on the control plane (because the event can be ignored due to FailureThreshold, and information on this is not included in the event). This can lead to users assuming there is a problem and a container/pod was restarted when nothing occurred.

I think we should expose the keepGoing value from https://github.com/kubernetes/kubernetes/blob/master/pkg/kubelet/prober/worker.go#L203 in the emitted event somehow, my preferred solution is to emit the probe failure event in the worker rather than where it currently sits in https://github.com/kubernetes/kubernetes/blob/master/pkg/kubelet/prober/prober.go#L110 - there is also the option of passing some information down the stack into the prober from the worker (such as making the FailureThreshold/SuccessThreshold decision in the prober) but I’m worried about separation of concerns, happy to hear what other folks think :)

Also of note is that FailureThreshold/SuccessThreshold is the only filter I can see where a probe can be ignored after being run (and therefore emitting a container event)

I’m happy to write this PR once we’re confident in our approach :)

Comments: 14

Write a new e2e test that will be marked as [Serial] and will be covering this bug fix: https://github.com/kubernetes/kubernetes/issues/89898

The test will serve as a replacement for the skipped unit test: https://github.com/kubernetes/kubernetes/pull/115329

The logic of the test will be as following:

• Create MANY containers (see the unit test logic)

• Each container has a liveness probe with 1s interval

• After some time validate that neither of containers has restartCount > 0

/cc @aojea

/sig node

/kind cleanup

/priority important-soon

/area tests

/help-wanted

/good-first-issue

This is part of KEP: https://github.com/kubernetes/enhancements/issues/2727 GA requirements, but also need the same tests for http and tcp.

Comments: 20

The NP docs point out that NP’s semantics for north/south traffic are not very clearly defined:

Cluster ingress and egress mechanisms often require rewriting the source or destination IP of packets. In cases where this happens, it is not defined whether this happens before or after NetworkPolicy processing, and the behavior may be different for different combinations of network plugin, cloud provider, Service implementation, etc.

In the case of ingress, this means that in some cases you may be able to filter incoming packets based on the actual original source IP, while in other cases, the “source IP” that the NetworkPolicy acts on may be the IP of a LoadBalancer or of the Pod’s node, etc.

For egress, this means that connections from pods to Service IPs that get rewritten to cluster-external IPs may or may not be subject to ipBlock-based policies.

However, what’s not ambiguous is that a pod which is fully isolated for ingress should not accept E/W or N/S traffic. Regardless of how the ingress/LB/cloud handles the traffic, it should end up getting blocked. (Well, unless it gets masqueraded to the pod’s node IP, because then it hits that exception to the rules.)

Unfortunately we can’t say the same for egress; I think we assume that a pod-to-service-IP connection will be allowed to reach kube-proxy even if the pod is fully isolated-for-egress, but we explicitly don’t require that ipBlock policies get applied after service proxying.

Anyway, we should be able to add a test to test/e2e/network/netpol/network_policy.go that confirms that cluster-ingress traffic to a fully isolated-for-ingress pod is not allowed. In particular, if we create a LoadBalancer Service with externalTrafficPolicy: Local, and a NetworkPolicy blocking all ingress to that Service’s Pods, then we should not be able to connect to the service via either the LoadBalancer IP/name or via its NodePort (on one of the correct nodes).

/sig network

/area network-policy

/priority backlog

/help

/good-first-issue

Comments: 23

What happened?

When kubelet loses connect, the node goes into the unknown state. The node lifecycle controller marks the pod as not ready by the markPodsNotReady function because the health check status of the pod can not be obtained through kubelet. This feature is available only when node’s Ready state transitions from true to unknown.

However, if the node is already in the fail state (such as a containerd failure), markPodsNotReady will not take effect if the node loses its connection at this time.

https://github.com/kubernetes/kubernetes/blob/cac53883f4714452f3084a22e4be20d042a9df33/pkg/controller/nodelifecycle/node_lifecycle_controller.go#L883-L888

In this case, the pod may accidentally remain ready, which may cause some network traffic to be accidentally forwarded to this node.

What did you expect to happen?

As long as the node loses its connection beyond grace time, MarkPodsNotReady should always work

How can we reproduce it (as minimally and precisely as possible)?

1. Stop containerd and wait for the node Ready state to false

2. Stop kubelet or shutdown the node and wait the node Ready state to unknown

3. The pods which not be evicted on this node would be always ready

Anything else we need to know?

In the node lifecycle controller logic,MarkPodsNotReady is just triggered when a node goes from true state to an unknown state. The correct way is to trigger when the node becomes unknown state regardless of whether the node state was previously true

Kubernetes version

$ kubectl version

Server Version: version.Info{Major:"1", Minor:"22", GitVersion:"v1.22.15", GitCommit:"1d79bc3bcccfba7466c44cc2055d6e7442e140ea", GitTreeState:"clean", BuildDate:"2022-09-22T06:03:36Z", GoVersion:"go1.16.15", Compiler:"gc", Platform:"linux/amd64"}

Cloud provider

OS version

# On Linux:

$ cat /etc/os-release



$ uname -a

5.4.119-1-tlinux4-0008 #1 SMP Fri Nov 26 11:17:45 CST 2021 x86_64 x86_64 x86_64 GNU/Linux



# On Windows:

C:\> wmic os get Caption, Version, BuildNumber, OSArchitecture

# paste output here

Install tools

Container runtime (CRI) and version (if applicable)

Related plugins (CNI, CSI, …) and versions (if applicable)

Comments: 21

kubectl apply with below yaml and follow below prune command.

**

namespace sandbox

**

---

apiVersion: v1

kind: Service

metadata:

annotations:

environment: sandbox

provider: kustomize

labels:

app: nginx

name: nginx

namespace: sandbox

spec:

ports:

- port: 80

selector:

app: nginx

type: NodePort

seems I have the same problem. save this to a file named a.yaml

apiVersion: v1

kind: Service

metadata:

annotations:

environment: sandbox

provider: kustomize

labels:

app: nginx

name: nginx

namespace: sandbox

spec:

ports:

- port: 80

selector:

app: nginx

type: NodePort

And run kubectl apply --prune --dry-run=client --all -n sandbox -f a.yaml, the results

service/nginx created (dry run)

namespace/cattle-system pruned (dry run)

I have no idea why it want to prune the namespace/cattle-system.

Originally posted by @wd in https://github.com/kubernetes/kubernetes/issues/66430#issuecomment-708922169

Comments: 28

What would you like to be added?

Pod resize E2E tests need a test case to revert the applied patch and verify that the original values are restored. PR discussion: https://github.com/kubernetes/kubernetes/pull/102884#discussion_r865662604

This should be done for both cases:

1. Fully actuated and resized resources are reflected in pod status.
2. Fully actuated but not yet reflected in pod status.

Why is this needed?

This verifies that actual.resources == desired.resources in instances where a resize is backed out after it has been partially actuated.

Comments: 25

There is a strong and growing consensus in sig-node about the need of increasing the test coverage in the kubelet, improving the current testsuite about coverage and reliability. See for example the April 26 sig-node meeting notes and the April 27 sig-node CI subgroup meeting notes.

This work has already begun and there are already some PR posted (see: https://github.com/kubernetes/kubernetes/pull/108024#pullrequestreview-954083698). This is a great start. There are more areas in the kubelet, especially in the container manager and resource manager area, that can use better unit test coverage.

Besides the obvious benefits of documenting and preserving the current behaviour, adding tests is meant to lower the barrier for future work and contributions.

Examples of some areas which can benefit of more tests:

• k/k/pkg/kubelet/cm/internal_container_lifecycle.go

• k/k/pkg/kubelet/cm/internal_container_lifecycle_linux.go

• k/k/pkg/kubelet/cm/pod_container_manager_linux.go

• k/k/pkg/kubelet/cm/qos_container_manager_linux.go

• everything coverage-driven (check test code coverage and build from there)

• error paths in general

• e2e tests in general

Comments: 37

What happened?

We found go vet issues of lostcancel in #109184. (It seems that go vet checks are not running under /staging dir.)

staging/src/k8s.io/legacy-cloud-providers/vsphere/nodemanager.go:190:5: lostcancel: the cancel function is not used on all paths (possible context leak) (govet)

ctx, cancel := context.WithCancel(context.Background())

^

staging/src/k8s.io/legacy-cloud-providers/vsphere/nodemanager.go:236:3: lostcancel: this return statement may be reached without using the cancel var defined on line 190 (govet)

}()

We should fix them to ensure we don’t introduce context leak as the message said.

What did you expect to happen?

These issues from go vet are fixed.

How can we reproduce it (as minimally and precisely as possible)?

Running go vet under staging/src/k8s.io/legacy-cloud-providers/

Anything else we need to know?

No response

Kubernetes version

latest master

Cloud provider

OS version

# On Linux:

$ cat /etc/os-release

# paste output here

$ uname -a

# paste output here



# On Windows:

C:\> wmic os get Caption, Version, BuildNumber, OSArchitecture

# paste output here

Install tools

Container runtime (CRI) and version (if applicable)

Related plugins (CNI, CSI, …) and versions (if applicable)

Comments: 15

This issue was reported in the Kubernetes Security Audit Report

Description

Kubernetes uses files and directories to store information ranging from key-value data to certificate data to logs. However, a number of locations have world-writable directories:

cluster/images/etcd/migrate/rollback_v2.go:110:	if err :=

os.MkdirAll(path.Join(migrateDatadir, "member", "snap"), 0777); err != nil {

cluster/images/etcd/migrate/data_dir.go:49:		err := os.MkdirAll(path, 0777)

cluster/images/etcd/migrate/data_dir.go:87:	err = os.MkdirAll(backupDir, 0777)

third_party/forked/godep/save.go:472:	err := os.MkdirAll(filepath.Dir(dst), 0777)

third_party/forked/godep/save.go:585:	err := os.MkdirAll(filepath.Dir(name), 0777)

pkg/volume/azure_file/azure_util.go:34:	defaultFileMode = "0777"

pkg/volume/azure_file/azure_util.go:35:	defaultDirMode  = "0777"

pkg/volume/emptydir/empty_dir.go:41:const perm os.FileMode = 0777

Figure 7.1: World-writable (0777) directories and defaults

Other areas of the system use world-writable files as well:

cluster/images/etcd/migrate/data_dir.go:147:	return ioutil.WriteFile(v.path, data, 0666)

cluster/images/etcd/migrate/migrator.go:120:	err := os.Mkdir(backupDir, 0666)

third_party/forked/godep/save.go:589:	return ioutil.WriteFile(name, []byte(body), 0666)

pkg/kubelet/kuberuntime/kuberuntime_container.go:306:			if err := m.osInterface.Chmod(containerLogPath, 0666); err != nil {

pkg/volume/cinder/cinder_util.go:271:			ioutil.WriteFile(name, data, 0666)

pkg/volume/fc/fc_util.go:118:	io.WriteFile(fileName, data, 0666)

pkg/volume/fc/fc_util.go:128:			io.WriteFile(name, data, 0666)

pkg/volume/azure_dd/azure_common_linux.go:77:			if err = io.WriteFile(name, data, 0666); err != nil {

pkg/volume/photon_pd/photon_util.go:55:	ioutil.WriteFile(fileName, data, 0666)

pkg/volume/photon_pd/photon_util.go:65:			ioutil.WriteFile(name, data, 0666)

Figure 7.2: World-writable (0666) files

A number of locations in the code base also rely on world-readable directories and files. For example, Certificate Signing Requests (CSRs) are written to a directory with mode 0755 (world readable and browseable) with the actual CSR having mode 0644 (world-readable):

// WriteCSR writes the pem-encoded CSR data to csrPath.

// The CSR file will be created with file mode 0644.

// If the CSR file already exists, it will be overwritten.

// The parent directory of the csrPath will be created as needed with file mode 0755.

func WriteCSR(csrDir, name string, csr *x509.CertificateRequest) error {

...

if err := os.MkdirAll(filepath.Dir(csrPath), os.FileMode(0755)); err != nil {

...

}



if err := ioutil.WriteFile(csrPath, EncodeCSRPEM(csr), os.FileMode(0644)); err != nil {

...

}

...

}

Figure 7.3: Documentation and code from cmd/kubeadm/app/util/pkiutil/pki_helpers.go

Exploit Scenario

Alice wishes to migrate some etcd values during normal cluster maintenance. Eve has local access to the cluster’s filesystem, and modifies the values stored during the migration process, granting Eve further access to the cluster as a whole.

Recommendation

Short term, audit all locations that use world-accessible permissions. Revoke those that are unnecessary. Very few files truly need to be readable by any user on a system. Almost none should need to allow arbitrary system users write access.

Long term, use system groups and extended Access Control Lists (ACLs) to ensure that all files and directories created by Kuberenetes are accessible by only those users and groups that should be able to access them. This will ensure that only the appropriate users with the correct Unix-level groups may access data. Kubernetes may describe what these groups should be, or create a role-based system to which administrators may assign users and groups.

Anything else we need to know?:

See #81146 for current status of all issues created from these findings.

The vendor gave this issue an ID of TOB-K8S-004 and it was finding 8 of the report.

The vendor considers this issue Medium Severity.

To view the original finding, begin on page 32 of the Kubernetes Security Review Report

Environment:

• Kubernetes version: 1.13.4

Comments: 36

What happened: I’ve been seeing a number of evictions recently that appear to be due to disk pressure:

$$$ kubectl get pod kumo-go-api-d46f56779-jl6s2 --namespace=kumo-main -o yaml

apiVersion: v1

kind: Pod

metadata:

creationTimestamp: 2018-12-06T10:05:25Z

generateName: kumo-go-api-d46f56779-

labels:

io.kompose.service: kumo-go-api

pod-template-hash: "802912335"

name: kumo-go-api-d46f56779-jl6s2

namespace: kumo-main

ownerReferences:

- apiVersion: extensions/v1beta1

blockOwnerDeletion: true

controller: true

kind: ReplicaSet

name: kumo-go-api-d46f56779

uid: c0a9355e-f780-11e8-b336-42010aa80057

resourceVersion: "11617978"

selfLink: /api/v1/namespaces/kumo-main/pods/kumo-go-api-d46f56779-jl6s2

uid: 7337e854-f93e-11e8-b336-42010aa80057

spec:

containers:

- env:

- redacted...

image: gcr.io/<redacted>/kumo-go-api@sha256:c6a94fc1ffeb09ea6d967f9ab14b9a26304fa4d71c5798acbfba5e98125b81da

imagePullPolicy: Always

name: kumo-go-api

ports:

- containerPort: 5000

protocol: TCP

resources: {}

terminationMessagePath: /dev/termination-log

terminationMessagePolicy: File

volumeMounts:

- mountPath: /var/run/secrets/kubernetes.io/serviceaccount

name: default-token-t6jkx

readOnly: true

dnsPolicy: ClusterFirst

nodeName: gke-kumo-customers-n1-standard-1-pree-0cd7990c-jg9s

restartPolicy: Always

schedulerName: default-scheduler

securityContext: {}

serviceAccount: default

serviceAccountName: default

terminationGracePeriodSeconds: 30

tolerations:

- effect: NoExecute

key: node.kubernetes.io/not-ready

operator: Exists

tolerationSeconds: 300

- effect: NoExecute

key: node.kubernetes.io/unreachable

operator: Exists

tolerationSeconds: 300

volumes:

- name: default-token-t6jkx

secret:

defaultMode: 420

secretName: default-token-t6jkx

status:

message: 'The node was low on resource: nodefs.'

phase: Failed

reason: Evicted

startTime: 2018-12-06T10:05:25Z

Taking a look at kubectl get events, I see these warnings:

$$$ kubectl get events

LAST SEEN   FIRST SEEN   COUNT     NAME                                                                   KIND      SUBOBJECT   TYPE      REASON          SOURCE                                                         MESSAGE

2m          13h          152       gke-kumo-customers-n1-standard-1-pree-0cd7990c-jg9s.156e07f40b90ed91   Node                  Warning   ImageGCFailed   kubelet, gke-kumo-customers-n1-standard-1-pree-0cd7990c-jg9s   (combined from similar events): failed to garbage collect required amount of images. Wanted to free 473948979 bytes, but freed 0 bytes

37m         37m          1         gke-kumo-customers-n1-standard-1-pree-0cd7990c-jg9s.156e3127ebc715c3   Node                  Warning   ImageGCFailed   kubelet, gke-kumo-customers-n1-standard-1-pree-0cd7990c-jg9s   failed to garbage collect required amount of images. Wanted to free 473674547 bytes, but freed 0 bytes

Digging a bit deeper:

$$$ kubectl get event gke-kumo-customers-n1-standard-1-pree-0cd7990c-jg9s.156e07f40b90ed91 -o yaml

apiVersion: v1

count: 153

eventTime: null

firstTimestamp: 2018-12-07T11:01:06Z

involvedObject:

kind: Node

name: gke-kumo-customers-n1-standard-1-pree-0cd7990c-jg9s

uid: gke-kumo-customers-n1-standard-1-pree-0cd7990c-jg9s

kind: Event

lastTimestamp: 2018-12-08T00:16:09Z

message: '(combined from similar events): failed to garbage collect required amount

of images. Wanted to free 474006323 bytes, but freed 0 bytes'

metadata:

creationTimestamp: 2018-12-07T11:01:07Z

name: gke-kumo-customers-n1-standard-1-pree-0cd7990c-jg9s.156e07f40b90ed91

namespace: default

resourceVersion: "381976"

selfLink: /api/v1/namespaces/default/events/gke-kumo-customers-n1-standard-1-pree-0cd7990c-jg9s.156e07f40b90ed91

uid: 65916e4b-fa0f-11e8-ae9a-42010aa80058

reason: ImageGCFailed

reportingComponent: ""

reportingInstance: ""

source:

component: kubelet

host: gke-kumo-customers-n1-standard-1-pree-0cd7990c-jg9s

type: Warning

There’s actually remarkably little here. This message doesn’t say anything regarding why ImageGC was initiated or why it was unable recover more space.

What you expected to happen: Image GC to work correctly, or at least fail to schedule pods onto nodes that do not have sufficient disk space.

How to reproduce it (as minimally and precisely as possible): Run and stop as many pods as possible on a node in order to encourage disk pressure. Then observe these errors.

Anything else we need to know?: n/a

Environment:

• Kubernetes version (use kubectl version):

Client Version: version.Info{Major:"1", Minor:"10", GitVersion:"v1.10.7", GitCommit:"0c38c362511b20a098d7cd855f1314dad92c2780", GitTreeState:"clean", BuildDate:"2018-08-20T10:09:03Z", GoVersion:"go1.9.3", Compiler:"gc", Platform:"darwin/amd64"}

Server Version: version.Info{Major:"1", Minor:"10+", GitVersion:"v1.10.7-gke.11", GitCommit:"fa90543563c9cfafca69128ce8cd9ecd5941940f", GitTreeState:"clean", BuildDate:"2018-11-08T20:22:21Z", GoVersion:"go1.9.3b4", Compiler:"gc", Platform:"linux/amd64"}

• Cloud provider or hardware configuration: GKE

• OS (e.g. from /etc/os-release): I’m running macOS 10.14, nodes are running Container-Optimized OS (cos).

• Kernel (e.g. uname -a): Darwin D-10-19-169-80.dhcp4.washington.edu 18.0.0 Darwin Kernel Version 18.0.0: Wed Aug 22 20:13:40 PDT 2018; root:xnu-4903.201.2~1/RELEASE_X86_64 x86_64

• Install tools: n/a

• Others: n/a

/kind bug

Comments: 68

https://github.com/kubernetes/kubernetes/pull/60275/files/2d942dab68b64e684dd2b75232cf9daabc6e0a95#diff-10055ae93a8699af13ceba0482fc43c3

I am expecting we have more windows specific config like code coming to Kubelet and other node components. We should figure out a way to handle this, so that the code can be easily managed? Instead of letting windows-special handling code like this scattered everywhere in our codebase.

cc/ @feiskyer @michmike @kubernetes/sig-windows-bugs

Comments: 21

The check for selector immutability is located at PrepareForUpdate functions of deploymentStrategy, rsStrategy and daemonSetStrategy. We are not able to have the check at validation before v1beta1 API version retires due to breaking change to some tests (discussed in this closed PR). Once v1beta1 retires, we should move the check to validation.

Comments: 12