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 enhancement for supporting user namespaces #1649

Merged
merged 1 commit into from
Aug 19, 2024

Conversation

haircommander
Copy link
Member

@haircommander
Copy link
Member Author

/hold

@deads2k is on vacation and should review this. I'm opening it for preliminary review though

@openshift-ci openshift-ci bot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Jun 25, 2024
@openshift-ci openshift-ci bot requested review from jmguzik and joepvd June 25, 2024 19:35
@haircommander haircommander force-pushed the scc-userns branch 2 times, most recently from 31705ee to 55032bf Compare June 25, 2024 20:11
@haircommander
Copy link
Member Author

SCC piece openshift/api#1939

@haircommander
Copy link
Member Author

TPNU piece openshift/api#1940

@haircommander
Copy link
Member Author

/hold cancel

I chatted with @deads2k on slack, he's okay with the piece I was waiting for his council on

@openshift-ci openshift-ci bot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Jul 8, 2024
@rphillips
Copy link
Contributor

/lgtm

@openshift-ci openshift-ci bot added the lgtm Indicates that a PR is ready to be merged. label Jul 15, 2024
Copy link
Contributor

openshift-ci bot commented Jul 15, 2024

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: rphillips

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 openshift-ci bot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Jul 15, 2024
@haircommander
Copy link
Member Author

/hold

@openshift-ci openshift-ci bot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Jul 15, 2024
@haircommander
Copy link
Member Author

for API and auth team reviews

Copy link
Contributor

@ibihim ibihim left a comment

Choose a reason for hiding this comment

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

I am currently checking the impact on SCCs overall, but I added my first thoughts on reading it.

enhancements/kubelet/user-namespaces-support.md Outdated Show resolved Hide resolved
enhancements/kubelet/user-namespaces-support.md Outdated Show resolved Hide resolved
enhancements/kubelet/user-namespaces-support.md Outdated Show resolved Hide resolved
@haircommander haircommander force-pushed the scc-userns branch 2 times, most recently from b8fc9cb to afb1d1f Compare July 31, 2024 14:50
@haircommander
Copy link
Member Author

/hold cancel

@haircommander haircommander force-pushed the scc-userns branch 4 times, most recently from 79c3553 to ed340f4 Compare August 8, 2024 15:05
Copy link
Contributor

@deads2k deads2k left a comment

Choose a reason for hiding this comment

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

some changes.

Comment on lines 228 to 229
// NamespaceLevelEmpty will set to the default, which is currently "AllowHostLevel".
NamespaceLevelEmpty NamespaceLevelType = ""
Copy link
Contributor

Choose a reason for hiding this comment

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

This field is closer to user workloads than to configuration. This one we'll default to AllowHostLevel instead of leaving empty because we know the intent will never change

Copy link
Member Author

Choose a reason for hiding this comment

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

sorry I don't think I fully understand--do you want me to drop this or keep it?

Copy link
Contributor

Choose a reason for hiding this comment

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

Empty should not be allowed. We should default the value.

Copy link
Contributor

Choose a reason for hiding this comment

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

If we ever wanted to start enforcing this, would we not want to change the default? Is the expectation there that we create a new version of the SCC and folks move over to it? In which case, having an API default doesn't really matter as we will create the object with the new value we desire

Copy link
Member Author

Choose a reason for hiding this comment

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

I dropped empty.

Is the expectation there that we create a new version of the SCC and folks move over to it?

this is my understanding of the approach

enhancements/kubelet/user-namespaces-support.md Outdated Show resolved Hide resolved
enhancements/kubelet/user-namespaces-support.md Outdated Show resolved Hide resolved
The `MinimalKubeletVersion` feature is to fix this problem. If all apiservers support this field upon GA, then a cluster admin can set the field and ensure
their kubelets are new enough to certainly support user namespaces. This frees the apiserver to relax validation and GA these SCC fields.

This feature is not required for TechPreview, but is required for GA if we do not want to wait until 1.33/4.20 to GA this feature (the point where the oldest
Copy link
Contributor

Choose a reason for hiding this comment

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

FeatureGate is still not enabled for kubelets in 1.31/4.18, so if MinimalKubeletVersion is not developed, it looks like 4.22 is the earliest possible. If MinimalKubeletVersion is developed, the earliest is 4.18.

Copy link
Member Author

Choose a reason for hiding this comment

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

why 4.22?

Copy link
Contributor

Choose a reason for hiding this comment

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

why 4.22?

Because as it currently stands 1.31/4.18 kubelets won't correctly honor the field. 4.19 becomes the first level where it might be properly honored and we have 4.19+3.

Copy link
Member

@liouk liouk left a comment

Choose a reason for hiding this comment

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

Adding some nits/typos I came across while reading this.

enhancements/kubelet/user-namespaces-support.md Outdated Show resolved Hide resolved
enhancements/kubelet/user-namespaces-support.md Outdated Show resolved Hide resolved
enhancements/kubelet/user-namespaces-support.md Outdated Show resolved Hide resolved
enhancements/kubelet/user-namespaces-support.md Outdated Show resolved Hide resolved
@haircommander
Copy link
Member Author

updated @deads2k @liouk ! thanks for your reviews, PTAL

Copy link
Member

@liouk liouk left a comment

Choose a reason for hiding this comment

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

Added a suggestion and a side-note for clarity.

enhancements/kubelet/user-namespaces-support.md Outdated Show resolved Hide resolved
@haircommander haircommander force-pushed the scc-userns branch 3 times, most recently from e819dc7 to 74c2cfe Compare August 19, 2024 15:37
@liouk
Copy link
Member

liouk commented Aug 19, 2024

/lgtm

Holding for @deads2k to finish his review and approve.

/hold

@openshift-ci openshift-ci bot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Aug 19, 2024
@openshift-ci openshift-ci bot added the lgtm Indicates that a PR is ready to be merged. label Aug 19, 2024
@openshift-ci openshift-ci bot removed the lgtm Indicates that a PR is ready to be merged. label Aug 19, 2024
Signed-off-by: Peter Hunt <pehunt@redhat.com>
@deads2k
Copy link
Contributor

deads2k commented Aug 19, 2024

/lgtm
/hold cancel

@openshift-ci openshift-ci bot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Aug 19, 2024
@openshift-ci openshift-ci bot added the lgtm Indicates that a PR is ready to be merged. label Aug 19, 2024
Copy link
Contributor

openshift-ci bot commented Aug 19, 2024

@haircommander: all tests passed!

Full PR test history. Your PR dashboard.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. I understand the commands that are listed here.

@openshift-merge-bot openshift-merge-bot bot merged commit ad5833d into openshift:master Aug 19, 2024
2 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
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.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants