Skip to content
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.

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

Fix issues with checks for kubelet configuration files #228

Merged
merged 4 commits into from
Mar 3, 2019
Merged

Conversation

ttousai
Copy link
Contributor

@ttousai ttousai commented Feb 27, 2019

This is a more detailed solution to the issue reported in the PR #208. It fixes issues with variable substitutions for node benchmarks.

There are 3 major config files for kubelets k8s nodes:

  • /etc/kubernetes/kubelet.conf ($kubeletkubeconfig)
  • /var/lib/kubelet.config.yaml ($kubeletconf)
  • /etc/systemd/system/kubelet.service.d/10-kubeadm.conf ($kubeletsvc)

In the previous versions of kube-bench there was no distinction between these files and results of running node checks were misleading. This PR adds the distinction and also adds support for a new type of variable kubeconfig.

This fix applies to only checks for kubernetes versions 1.8 and 1.11.
See #208.
There are checks for the kubeconfig for both kubelet and proxy which
the current kube-bench implementation does not check for properly.
kube-bench checks the wrong files.

This PR adds support for variable substitution for all the config file
types are that should be checked in the CIS benchmarks.

This PR also fixes a buggy in CIS 1.3.0 check 2.2.9, which checks for
ownership of the kubelet config file /var/lib/kubelet/config.yaml but
recommends changing ownership of kubelet kubeconfig file
/etc/kubernetes/kubelet.conf as remediation.
@ttousai ttousai requested a review from lizrice February 27, 2019 22:18
Copy link
Contributor

@lizrice lizrice left a comment

Choose a reason for hiding this comment

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

Basically looks good! I think we need to hang on to the .manifest extension files though.

Do we need to take another look at the control plane components too? I wonder if we have confused the config yaml and the kubeconfig (credentials) files for things like kube-apiserver.yaml as well. We can do that in a separate issue if you prefer - could you take a look and raise an issue if you think there is a problem there?

@@ -9,36 +9,13 @@

master:
apiserver:
confs:
- /etc/kubernetes/manifests/kube-apiserver.yaml
- /etc/kubernetes/manifests/kube-apiserver.manifest
Copy link
Contributor

Choose a reason for hiding this comment

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

I think we still need the possibility of .manifest extension (as an alternative to .yaml) for kops and kubespray installations (see c44e0db)

Copy link
Contributor

Choose a reason for hiding this comment

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

(same for scheduler, controller manager & etcd below in this file)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I don't see anything wrong with keeping them. My idea was to clean up our configs a bit and there is no actual problem keeping them.

defaultconf: /etc/kubernetes/manifests/etcd.yaml

node:
Copy link
Contributor

Choose a reason for hiding this comment

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

Just checking - do we not need these node settings in the 1.8 version of config.yaml because we pick it up from the defaults in cfg/config.yaml?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes we pick the default config from cfg/config.yaml

Copy link
Contributor

@lizrice lizrice left a comment

Choose a reason for hiding this comment

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

Lgtm

@lizrice lizrice merged commit 2d4019a into master Mar 3, 2019
yoavrotems added a commit that referenced this pull request Mar 6, 2019
Fix the start from 1.11 to 1.13 and adding changes from pull #227, and pull #228.
@lizrice lizrice deleted the fix-208 branch March 6, 2019 13:33
@lizrice lizrice mentioned this pull request Apr 2, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants