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

Split up create config handling of namespaces and security #4265

Conversation

haircommander
Copy link
Collaborator

@haircommander haircommander commented Oct 14, 2019

As it stands, createconfig is a huge struct. This works fine when the only caller is when we create a container with a fully created config. However, if we wish to share code for security and namespace configuration, a single large struct becomes unweildy, as well as difficult to configure with the single createConfigToOCISpec function.

This PR breaks up namespace and security configuration into their own structs, with the eventual goal of allowing the namespace/security fields to be configured by the pod create cli, and allow the infra container to share this with the pod's containers. This will solve many requests (#3837, #2957, #2808) of further customizing pod creation.

This is NOT DONE AT ALL. but, I am putting the beginnings here for people to tell me their thoughts about the design, approach etc. The next steps include:

  • actually use the namespace functions in spec generation/creation (after this step I will likely push to have this PR merged so it doesn't bloat too much)
  • create a path for a pod create to take namespaces and security and configure the infra container's fields
  • add the appropriate subset of podman create cli flags for namespace/security creation
  • integrate into play kube
  • test the day lights out of it

@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 Oct 14, 2019
@openshift-ci-robot openshift-ci-robot added the size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. label Oct 14, 2019

cgroup := cc.CgroupConfig{
Cgroups: c.String("cgroups"),
Cgroupns: c.String("cgroupns"),
Copy link
Member

Choose a reason for hiding this comment

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

I'd prefer to put all namespace configuration together if we can

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Even if we do i'd still like proceessing of them to be separated so if a pod only shares a couple of namespaces, all fields don't have to be configured fully

@mheon
Copy link
Member

mheon commented Oct 14, 2019

I'm in the planning stages of related refactoring, so we might want to sync on this tomorrow

@haircommander
Copy link
Collaborator Author

Sgtm, i'm second half day PTO but on PST, maybe sync around watercooler?

@haircommander haircommander force-pushed the infra-namespaces-submit branch 7 times, most recently from 6a1cddc to 700f004 Compare October 18, 2019 16:26
@haircommander haircommander changed the title WIP: DO NOT MERGE: Split up create config handling of namespaces and security Split up create config handling of namespaces and security Oct 18, 2019
@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 Oct 18, 2019
@haircommander
Copy link
Collaborator Author

Alright, this PR as is is ready to be merged PTAL @mheon @baude @rhatdan @TomSweeneyRedHat @QiWang19 @jwhonce @vrothberg

@mheon
Copy link
Member

mheon commented Oct 18, 2019

One thing I'd really like to do is have a unified, sane type for namespaces. Podman should probably allow --uts=ns:$PATH like we do for --net, for example. It seems like it would be easier to handle them all together.

@haircommander
Copy link
Collaborator Author

@mheon the problem about that is where the options diverge. net has like a thousand options. We could create an interface for the shared ones, but we couldn't use that interface in a very generic way, so the added benefit is not clear to me

@rh-atomic-bot
Copy link
Collaborator

☔ The latest upstream changes (presumably #4310) made this pull request unmergeable. Please resolve the merge conflicts.

@rhatdan
Copy link
Member

rhatdan commented Nov 4, 2019

@haircommander Could you rebase?

@haircommander haircommander force-pushed the infra-namespaces-submit branch from 700f004 to 17d61fb Compare November 4, 2019 17:45
@haircommander
Copy link
Collaborator Author

@rhatdan done

@rhatdan
Copy link
Member

rhatdan commented Nov 4, 2019

LGTM

Copy link
Member

@vrothberg vrothberg left a comment

Choose a reason for hiding this comment

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

LGTM, nice work @haircommander

I will let @mheon and/or @baude have a final look.

@rhatdan
Copy link
Member

rhatdan commented Nov 5, 2019

/lgtm
/approve

@openshift-ci-robot openshift-ci-robot added the lgtm Indicates that a PR is ready to be merged. label Nov 5, 2019
@openshift-ci-robot
Copy link
Collaborator

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: haircommander, 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

@openshift-ci-robot openshift-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Nov 5, 2019
@rh-atomic-bot
Copy link
Collaborator

☔ The latest upstream changes (presumably #4374) made this pull request unmergeable. Please resolve the merge conflicts.

@haircommander haircommander force-pushed the infra-namespaces-submit branch from 17d61fb to f413c7f Compare November 5, 2019 19:30
@openshift-ci-robot openshift-ci-robot removed the lgtm Indicates that a PR is ready to be merged. label Nov 5, 2019
@haircommander
Copy link
Collaborator Author

rebased to resolve conflicts

@rhatdan
Copy link
Member

rhatdan commented Nov 5, 2019

/lgtm
/hold
hold cancel when tests pass.

@openshift-ci-robot openshift-ci-robot added do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. lgtm Indicates that a PR is ready to be merged. labels Nov 5, 2019
@haircommander
Copy link
Collaborator Author

/hold cancel

@openshift-ci-robot openshift-ci-robot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Nov 6, 2019
@haircommander
Copy link
Collaborator Author

/retest

@rhatdan
Copy link
Member

rhatdan commented Nov 7, 2019

/test images

@rh-atomic-bot
Copy link
Collaborator

☔ The latest upstream changes (presumably #4447) made this pull request unmergeable. Please resolve the merge conflicts.

@rhatdan
Copy link
Member

rhatdan commented Nov 7, 2019

@haircommander needs a rebase.

As it stands, createconfig is a huge struct. This works fine when the only caller is when we create a container with a fully created config. However, if we wish to share code for security and namespace configuration, a single large struct becomes unweildy, as well as difficult to configure with the single createConfigToOCISpec function.

This PR breaks up namespace and security configuration into their own structs, with the eventual goal of allowing the namespace/security fields to be configured by the pod create cli, and allow the infra container to share this with the pod's containers.

Signed-off-by: Peter Hunt <[email protected]>
@haircommander haircommander force-pushed the infra-namespaces-submit branch from f413c7f to dcf3c74 Compare November 8, 2019 02:23
@openshift-ci-robot openshift-ci-robot removed the lgtm Indicates that a PR is ready to be merged. label Nov 8, 2019
@rhatdan
Copy link
Member

rhatdan commented Nov 8, 2019

/lgtm

@openshift-ci-robot openshift-ci-robot added the lgtm Indicates that a PR is ready to be merged. label Nov 8, 2019
@openshift-merge-robot openshift-merge-robot merged commit 92af260 into containers:master Nov 8, 2019
@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 26, 2023
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Sep 26, 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. size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants