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

Set NetNS mode instead of value #8792

Merged

Conversation

bziemons
Copy link
Contributor

Set NetNS mode instead of value when HostNetwork is true in the pod spec.
Also propagate whether host network namespace should be used for containers.

Closes #8790

@mheon
Copy link
Member

mheon commented Dec 20, 2020

LGTM, good catch
/approve

@openshift-ci-robot openshift-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Dec 20, 2020
@zhangguanzhang
Copy link
Collaborator

need add a test for it

@bziemons
Copy link
Contributor Author

need add a test for it

Will look into testing, but go and this project is all new to me so I wont promise anything.

@@ -226,7 +226,7 @@ func (ic *ContainerEngine) playKubePod(ctx context.Context, podName string, podY
return nil, err
}

specGen, err := kube.ToSpecGen(ctx, container, container.Image, newImage, volumes, pod.ID(), podName, podInfraID, configMaps, seccompPaths, ctrRestartPolicy)
specGen, err := kube.ToSpecGen(ctx, container, container.Image, newImage, volumes, pod.ID(), podName, podInfraID, configMaps, seccompPaths, ctrRestartPolicy, p.NetNS.IsHost())
Copy link
Member

Choose a reason for hiding this comment

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

I think it is time that this function take a struct, instead of constantly adding params. Not for this PR, But this function is too big.

@openshift-ci-robot
Copy link
Collaborator

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: bziemons, mheon, rhatdan

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@bziemons bziemons force-pushed the patch-host-network-spec-8790 branch from bbcd48b to 63de07e Compare December 21, 2020 15:46
@rhatdan
Copy link
Member

rhatdan commented Dec 21, 2020

/lgtm
/hold

@openshift-ci-robot openshift-ci-robot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Dec 21, 2020
@openshift-ci-robot openshift-ci-robot added the lgtm Indicates that a PR is ready to be merged. label Dec 21, 2020
@rhatdan
Copy link
Member

rhatdan commented Dec 21, 2020

This PR is failing tests.

@bziemons
Copy link
Contributor Author

True, the test needs to be skipped when running rootless. Will push a fix tomorrow.

@bziemons
Copy link
Contributor Author

On second thought that is not true. podman run --network host works rootless and I just tested this change on Fedora 33 successfully as a user. Can somebody help with understanding the test's problem? The error message is very generic and is what I got before I passed the network NSMode of host down to the pod's containers.

PS: For reference, this is the kube.yaml I used for testing:

# Generation of Kubernetes YAML is still under development!
#
# Save the output of this file and use kubectl create -f to import
# it into Kubernetes.
#
# Created with podman-2.2.1
apiVersion: v1
kind: Pod
metadata:
  creationTimestamp: "2020-12-22T00:08:39Z"
  labels:
    app: distractedjemison
  name: distracted_jemison
spec:
  restartPolicy: Never
  hostNetwork: true
  containers:
  - command:
    - ip
    - a
    env:
    - name: PATH
      value: /usr/local/sbin:/usr/local/bin:/usr/sbin:/usr/bin:/sbin:/bin
    - name: TERM
      value: xterm
    - name: container
      value: podman
    - name: HOSTNAME
      value: xeto
    image: quay.io/libpod/alpine
    name: distractedjemison
    resources: {}
    securityContext:
      allowPrivilegeEscalation: true
      capabilities:
        drop:
        - CAP_MKNOD
        - CAP_NET_RAW
        - CAP_AUDIT_WRITE
      privileged: false
      readOnlyRootFilesystem: false
      seLinuxOptions: {}
    workingDir: /
status: {}
---
metadata:
  creationTimestamp: null
spec: {}
status:
  loadBalancer: {}

@zhangguanzhang
Copy link
Collaborator

need rebase the commits to one commit

@bziemons bziemons force-pushed the patch-host-network-spec-8790 branch from 63de07e to ed59e83 Compare December 22, 2020 09:06
@openshift-ci-robot openshift-ci-robot removed the lgtm Indicates that a PR is ready to be merged. label Dec 22, 2020
@zhangguanzhang
Copy link
Collaborator

need rebase all the commits to one commit😁

@bziemons
Copy link
Contributor Author

you mean squash them together? Doesn't make sense to me, but fine.

@zhangguanzhang
Copy link
Collaborator

They are related, so they need to be in the same commit

@bziemons bziemons force-pushed the patch-host-network-spec-8790 branch 2 times, most recently from 8e05ab8 to 70c3552 Compare December 23, 2020 15:52
@bziemons
Copy link
Contributor Author

bziemons commented Dec 23, 2020

So I have found the problem.. It seems to be a limitation on runc, since I cannot run

podman --runtime runc run --pod "$(podman --runtime runc pod create --network host)" --rm -it quay.io/libpod/alpine ip a

It results in the same error (but also results in an error when run as root). This works fine on crun though, which was the default for me on Fedora 32. I will therefor exclude everything except crun for the HostNetwork test (as done for other tests).

when HostNetwork is true in the pod spec.
Also propagate whether host network namespace should be used for containers.

Add test for HostNetwork setting in kubeYaml.
The infra configuration should reflect the setting.

Signed-off-by: Benedikt Ziemons <[email protected]>
@bziemons bziemons force-pushed the patch-host-network-spec-8790 branch from 70c3552 to 14439b9 Compare December 23, 2020 18:28
@rhatdan
Copy link
Member

rhatdan commented Dec 23, 2020

/lgtm

@openshift-ci-robot openshift-ci-robot added the lgtm Indicates that a PR is ready to be merged. label Dec 23, 2020
@rhatdan rhatdan removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Dec 23, 2020
@rhatdan
Copy link
Member

rhatdan commented Dec 23, 2020

/hold cancel

@openshift-merge-robot openshift-merge-robot merged commit 61a2262 into containers:master Dec 23, 2020
@bziemons bziemons deleted the patch-host-network-spec-8790 branch December 23, 2020 20:40
bziemons added a commit to bziemons/podman that referenced this pull request Dec 23, 2020
Create kube.CtrSpecGenOptions and document parameters.
Follow-up on containers#8792 (comment)

Signed-off-by: Benedikt Ziemons <[email protected]>
@github-actions github-actions bot added the locked - please file new issue/PR Assist humans wanting to comment on an old issue or PR with locked comments. label Sep 23, 2023
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Sep 23, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. lgtm Indicates that a PR is ready to be merged. locked - please file new issue/PR Assist humans wanting to comment on an old issue or PR with locked comments.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Using hostNetwork with play kube results in error
6 participants