docs: Clean up k8s with cri-containerd howto by jodh-intel · Pull Request #251 · kata-containers/documentation

jodh-intel · 2018-09-24T11:09:21Z

This PR is based on #124 but has been reworked and updated to take into
account review feedback and extra cleanups to bring this howto in line
with the latest documentation requirements.

Fixes #127.

Contributions-from: T. Nichole Williams tribecca@tribecc.us
Signed-off-by: James O. D. Hunt james.o.hunt@intel.com

jodh-intel · 2018-09-24T11:13:05Z

A couple of points on this PR:

I've added a SOB for the original author as we've documented in https://github.com/kata-containers/community/blob/master/CONTRIBUTING.md#assisted-pr-workflow, however, maybe that should be something like Contributions-From: email@address ?
I've added the full name of the original author as we implicitly require that I think.

@OGtrilliams - ptal! ;)

grahamwhaley

Overall looking good to me. thanks for picking this up @jodh-intel Just a few minor questions.

grahamwhaley · 2018-09-24T12:32:35Z


-For information about the supported version of these components see 
-Kata Containers [versions.yaml](https://github.com/kata-containers/runtime/blob/master/versions.yaml) file.
+**Note:** For information about the supported versions of these components,


Should this be an > indented note as per our guidelines

grahamwhaley · 2018-09-24T12:37:30Z

+If you are behind a proxy, use the following script to configure your proxy for docker, kubelet, and containerd:

 ```bash
-# Set proxys


I wonder if coding this as a #!/bin/bash script here will make it easier for a cut/paste for the user. I know we don't tend to do that for other inline scripts, so am happy to leave as is as well.

Good point - I've simplified that block to not require bash now.

grahamwhaley · 2018-09-24T12:41:34Z


-$ export KUBECONFIG=/etc/kubernetes/admin.conf
+  ```bash
+  $ sudo iptables -P FORWARD ACCEPT


Ideally I'd like a touch more info around what this is fixing, how, and why it's not a gaping security hole (or, a caveat note if it is ;-) ).

Well, this PR is for reworking the formatting but I'm happy to take input from @jcvenegas with more details... :)

@jodh-intel this is used for the case that docker is installed in the node, docker set iptables rules that not allow containers communication, the issue is kubernetes/kubernetes#40182

I did not test if this is not needed anymore, but keep as legacy from our initial k8s installation script.

Thanks @jcvenegas - branch updated.

@grahamwhaley, @jcvenegas - can you provide some suitable "security hole or not" words I can add?

/cc @mcastelino.

Phew. Well, that thread over on kubernetes/kubernetes#40182 shows some less broad-spectrum fixes, and also points to what look like two supported solutions:
containernetworking/plugins#75
kubernetes/kubernetes#52569

From reading the thread, it seems the above fix as is is only transient, and not 'sticky' over reboots as well. I think we should say something like:

For testing, temporarily apply the following rule. Please see thread https://github.com/kubernetes/kubernetes/issues/40182 for more details on how to configure the k8s networking to route Docker traffic on your system.

But, let's get some feedback on if that is the right thing...

We didn't resolve this. I'm not happy with it as is, as it applies a broad spectrum port forwared/allow, which is then not sticky. We need a decision. /cc @mcastelino @egernst .

grahamwhaley · 2018-09-24T12:44:26Z

+- Create a pod network using flannel

-### Allow run pods in master node
+  **Note:** There is not a programmatic way to know last what flannel commit use. See https://github.com/coreos/flannel/issues/995.


s/not a/no/

Yoda speak I read this: know last what flannel commit use (and how can there be no :yoda: emoji!) ;-)

grahamwhaley · 2018-09-24T12:46:24Z

-If a pod has the `io.kubernetes.cri.untrusted-workload annotation` set as
-`"true"`, the CRI plugin will run the pod with the Kata Containers runtime.
+      sleep 1s
+      ((timeout_dns-=1))


tiddly nit - I think you can use ((timeout_dns--)) here :-)

jodh-intel · 2018-09-24T13:14:36Z

Hi @grahamwhaley - good catches! Branch updated ;)

jodh-intel · 2018-09-24T14:00:48Z

Ping @egernst, @jcvenegas, @chavafg, @sboeuf - ptal y'all!

jcvenegas

Thanks looks much better!. Let me rerun this new docs locally to verify all works

jodh-intel · 2018-09-28T15:17:03Z

Hi @klynnrif and @kata-containers/documentation - ptal.

klynnrif

Scrubbed for grammar, structure, and flow. One small change and I'm all set :) Thanks!

klynnrif · 2018-10-01T01:11:59Z

+- Create a pod network using flannel

-By default, the cluster will not schedule pods in the master node to allow that run:
+  > **Note:** There is no known way to determine programmatically the best version (commit) of to use.


Remove "of": ...to determine programmatically the best version (commit) to use.

This PR is based on kata-containers#124 but has been reworked and updated to take into account review feedback and extra cleanups to bring this howto in line with the latest documentation requirements. Fixes kata-containers#127. Signed-off-by: T. Nichole Williams <tribecca@tribecc.us> Signed-off-by: James O. D. Hunt <james.o.hunt@intel.com>

jodh-intel · 2018-10-01T07:37:56Z

Thanks @klynnrif - branch updated.

/test

klynnrif

lgtm - thanks!

grahamwhaley · 2018-10-01T17:01:45Z

@jcvenegas - did you check this still works - if so, please re-review :-)

jcvenegas

It keeps working tested locally.

jodh-intel · 2018-10-02T08:08:39Z

Hi @grahamwhaley - one ack from you and we can land this at long last :)

grahamwhaley

Still a couple of niggles.

grahamwhaley · 2018-10-02T08:21:14Z

+
+Configure `containerd` to use the Kata runtime to run untrusted workloads by
+setting the `plugins.cri.containerd.untrusted_workload_runtime`
+[config option](https://github.com/containerd/cri/blob/v1.0.0-rc.0/docs/config.md):


The link here points to a specific, and old, version of containerd. Is that deliberate? Feels wrong. Probably should either point at the master branch - or the version that matches the one in your versions.yaml ? I vote for master.

grahamwhaley · 2018-10-02T08:25:54Z


-$ export KUBECONFIG=/etc/kubernetes/admin.conf
+  ```bash
+  $ sudo iptables -P FORWARD ACCEPT


We didn't resolve this. I'm not happy with it as is, as it applies a broad spectrum port forwared/allow, which is then not sticky. We need a decision. /cc @mcastelino @egernst .

jodh-intel · 2018-10-02T08:58:36Z

Hi @grahamwhaley - the problem is, I didn't write this document. I understand we're trying to improve this, but I'm also worried that we're now reviewing even the bits of the doc that the original PR didn't touch.

I think we're going to have to therefore wait until someone with more intimate knowledge of this can help me out.

/cc @egernst, @mcastelino, @egernst maybe?

jodh-intel · 2018-10-02T09:01:06Z

How about we use the usual compromise: I raise an issue for these outstanding niggles so we can atleast get the majority of these improvements landed? The overall issue has been outstanding for ~ 5 months now... :)

grahamwhaley · 2018-10-02T09:11:56Z

OK, let's see if we can get some feedback from those pinged in the next 24h - otherwise we'll go the Issue-and-move-on pragmatic route.

grahamwhaley · 2018-10-04T12:23:37Z

Feedback didn't materialise.... @jodh-intel - merging this, and let's open Issues for any remaining outstanding items.

jodh-intel · 2018-10-05T08:30:57Z

Thanks - #265 raised for the iptables query.

With the old code it was possible to see odd messages like: "INFO: Create root disk image. Attempt 6 out of 5." Move the attempt number print to after we check against the max Fixes kata-containers#251 Signed-off-by: Matt Fischer <matt@mattfischer.com>

jodh-intel force-pushed the improve-k8s+cri-containerd-howto branch from 184a293 to 34ad1e3 Compare September 24, 2018 11:10

jodh-intel mentioned this pull request Sep 24, 2018

Format update #124

Closed

jodh-intel force-pushed the improve-k8s+cri-containerd-howto branch from 34ad1e3 to 2325113 Compare September 24, 2018 11:14

grahamwhaley reviewed Sep 24, 2018

View reviewed changes

jodh-intel force-pushed the improve-k8s+cri-containerd-howto branch from 2325113 to ed60798 Compare September 24, 2018 13:14

jcvenegas reviewed Sep 24, 2018

View reviewed changes

jodh-intel force-pushed the improve-k8s+cri-containerd-howto branch from ed60798 to e651f4c Compare September 24, 2018 15:02

klynnrif suggested changes Oct 1, 2018

View reviewed changes

jodh-intel force-pushed the improve-k8s+cri-containerd-howto branch from e651f4c to 4cc9efe Compare October 1, 2018 07:35

klynnrif approved these changes Oct 1, 2018

View reviewed changes

jcvenegas approved these changes Oct 1, 2018

View reviewed changes

grahamwhaley reviewed Oct 2, 2018

View reviewed changes

grahamwhaley merged commit ec9f9d4 into kata-containers:master Oct 4, 2018

jodh-intel mentioned this pull request Oct 5, 2018

document why iptables rule required for cri-containerd #265

Closed

Conversation

jodh-intel commented Sep 24, 2018

Uh oh!

jodh-intel commented Sep 24, 2018

Uh oh!

grahamwhaley left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jodh-intel commented Sep 24, 2018

Uh oh!

jodh-intel commented Sep 24, 2018

Uh oh!

jcvenegas left a comment

Choose a reason for hiding this comment

Uh oh!

jodh-intel commented Sep 28, 2018

Uh oh!

klynnrif left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jodh-intel commented Oct 1, 2018

Uh oh!

klynnrif left a comment

Choose a reason for hiding this comment

Uh oh!

grahamwhaley commented Oct 1, 2018

Uh oh!

jcvenegas left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jodh-intel commented Oct 2, 2018

Uh oh!

grahamwhaley left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jodh-intel commented Oct 2, 2018

Uh oh!

jodh-intel commented Oct 2, 2018

Uh oh!

grahamwhaley commented Oct 2, 2018

Uh oh!

grahamwhaley commented Oct 4, 2018

Uh oh!

jodh-intel commented Oct 5, 2018

Uh oh!

jcvenegas left a comment •

edited

Loading