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

Add support for specifying CNI networks in podman play kube #5620

Merged

Conversation

cfelder
Copy link
Contributor

@cfelder cfelder commented Mar 26, 2020

As far as I understood it is not possible to define container network on the yaml level (https://kubernetes.io/docs/reference/generated/kubernetes-api/v1.18/#pod-v1-core).

I am adding networking options to podman play cube though.

@openshift-ci-robot openshift-ci-robot added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Mar 26, 2020
@cfelder cfelder force-pushed the play-kube-networking branch from ad18a5d to 399f7a8 Compare March 26, 2020 13:24
@mheon
Copy link
Member

mheon commented Mar 26, 2020

I don't know if we want to do this - should we be sourcing from the Kube YAML instead? @haircommander thoughts?

@cfelder
Copy link
Contributor Author

cfelder commented Mar 26, 2020

@mheon I would prefer specifying this on yaml level but I have not found an option to declare a network for a pod in the specs.

@haircommander
Copy link
Collaborator

haircommander commented Mar 26, 2020

yeah I also wouldn't go the flag route. There are some first class pod API features that relate to pod networking:

the dnsConfig object would take care of --dns --dns-opt and --dns-search
hostAliases seem to take care of --add-hosts and --ip

finally, you can replicate --publish on the container level with the ports object

there doesn't seem to be a way to specify --mac-address, and --network also seems inaccessible. It is also hard to figure out how similar behavior to --no-hosts could be implemented.

@haircommander
Copy link
Collaborator

I suppose --network could be added as a hacky way to specify a network (Because k8s api doesn't provide it). But the others should be done the k8s way IMO

@cfelder
Copy link
Contributor Author

cfelder commented Mar 26, 2020

I am more than happy to implement this in yaml if you can help me with the specification as I don't know how to specify the network options in yaml syntax.

it would be a lot easier to implement this in PlayKubeYAML in pkg/adapter/pods.go using libPod.WithPodNetworks.

@haircommander
Copy link
Collaborator

I am more than happy to implement this in yaml if you can help me with the specification as I don't know how to specify the network options in yaml syntax.

were the above links not enough to get started? do you have specific questions about them?

it would be a lot easier to implement this in PlayKubeYAML in pkg/adapter/pods.go using libPod.WithPodNetworks.

As an example, when iterating through the container objects, you can access container.Ports and take the information defined here and translate it into the equivalent values for libpod/options:WithNetNS()

does that help?

@haircommander
Copy link
Collaborator

I am more than happy to implement this in yaml if you can help me with the specification as I don't know how to specify the network options in yaml syntax.

were the above links not enough to get started? do you have specific questions about them?

it would be a lot easier to implement this in PlayKubeYAML in pkg/adapter/pods.go using libPod.WithPodNetworks.

As an example, when iterating through the container objects, you can access container.Ports and take the information defined here and translate it into the equivalent values for libpod/options:WithNetNS()

does that help?

nvm on that, we already do something similar in getPodPorts in pkg/adapter/pods.go . but hopefully that serves as an example for the other options

@cfelder cfelder force-pushed the play-kube-networking branch 2 times, most recently from 7672762 to dfd2884 Compare March 26, 2020 14:22
@cfelder cfelder changed the title [WIP]: Add networking options to podman play kube Add support for specifying CNI networks in podman play kube Mar 26, 2020
@openshift-ci-robot openshift-ci-robot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Mar 26, 2020
@cfelder
Copy link
Contributor Author

cfelder commented Mar 26, 2020

Thanks for your remarks I changed my PR to just add --network for podman play kube in order to support specifying CNI networks which cannot be done in yaml syntax right now.

@cfelder cfelder force-pushed the play-kube-networking branch 2 times, most recently from a58c719 to 0872d78 Compare March 26, 2020 14:32
@mheon
Copy link
Member

mheon commented Mar 26, 2020

Concur that just --network is appropriate...

We probably also want to restrict it to only listing a set of CNI networks - we should not support --net=host and --net=bridge as those are handled in the YAML.

@TomSweeneyRedHat
Copy link
Member

Any tests possible? Code LGTM, just a few nitty doc things. Thanks @cfelder !

@cfelder cfelder force-pushed the play-kube-networking branch from 0872d78 to 5ecadbf Compare March 27, 2020 08:54
@cfelder cfelder force-pushed the play-kube-networking branch from 5ecadbf to 91dbdff Compare March 27, 2020 09:00
@vrothberg
Copy link
Member

Ping. Let's get this in.

@mheon
Copy link
Member

mheon commented Apr 6, 2020

/approve
LGTM

@openshift-ci-robot
Copy link
Collaborator

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: cfelder, mheon

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

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

/lgtm

@openshift-ci-robot openshift-ci-robot added the lgtm Indicates that a PR is ready to be merged. label Apr 6, 2020
@openshift-merge-robot openshift-merge-robot merged commit e318b09 into containers:master Apr 6, 2020
@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 25, 2023
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Sep 25, 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.

7 participants