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

SCC: Promote sysctl annotations to fields #20151

Merged
merged 5 commits into from
Jul 21, 2018

Conversation

stlaz
Copy link
Member

@stlaz stlaz commented Jun 29, 2018

This PR promotes the experimental SCCs' sysctl-related annotations to fields.

  • promote sysctls from annotations to SCC fields
  • fix the sysctl-related tests disabled in the 1.11 rebase
  • extend the defaul privileged SCC to allow all sysctls

Trello: https://trello.com/c/WP50iqpB/222-add-forbiddensysctls-and-allowedunsafesysctls-options-to-sccs

@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 Jun 29, 2018
@openshift-ci-robot openshift-ci-robot added the size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. label Jun 29, 2018
}
if len(scc.ForbiddenSysctls) > 0 {
fmt.Fprintf(out, " Forbidden Sysctls:\t%s\n", sysctlsToString(scc.ForbiddenSysctls))
}
Copy link
Contributor

Choose a reason for hiding this comment

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

Looks like we always show the fields. If their content is empty, we just represent it as <all> or <none>. So, could you remove these if-s then?

Copy link
Member Author

Choose a reason for hiding this comment

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

Fixed :)

@stlaz stlaz force-pushed the sysctl_promotion branch 2 times, most recently from 17a47f7 to eff8d35 Compare June 29, 2018 13:24
}

const sysctlPatternSegmentFmt string = "([a-z0-9][-_a-z0-9]*)?[a-z0-9*]"
const SysctlPatternFmt string = "(" + kapivalidation.SysctlSegmentFmt + "\\.)*" + sysctlPatternSegmentFmt
Copy link
Contributor

Choose a reason for hiding this comment

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

Could we make it non-pubic or something else depends on it?

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 believe it should be alright to make it private, I'll change that.

@openshift-bot openshift-bot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jul 4, 2018
@openshift-bot
Copy link
Contributor

/test cross

@openshift-bot openshift-bot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jul 10, 2018
@stlaz
Copy link
Member Author

stlaz commented Jul 11, 2018

Apparently one of the dependencies is breaking the build:

# github.com/openshift/origin/vendor/github.com/spf13/viper
vendor/github.com/spf13/viper/viper.go:688:9: undefined: cast.ToInt32

@openshift-bot openshift-bot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jul 16, 2018
@openshift-bot openshift-bot added needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. and removed needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. labels Jul 16, 2018
@openshift-ci-robot openshift-ci-robot added size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. and removed size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. labels Jul 17, 2018
@openshift-bot openshift-bot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jul 18, 2018
@openshift-ci-robot openshift-ci-robot added size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. and removed size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. labels Jul 18, 2018
@openshift-bot openshift-bot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jul 20, 2018
@openshift-ci-robot openshift-ci-robot removed the size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. label Jul 20, 2018
@openshift-ci-robot openshift-ci-robot added the size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. label Jul 20, 2018
@openshift-ci-robot openshift-ci-robot added size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. and removed size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. labels Jul 20, 2018
@liggitt
Copy link
Contributor

liggitt commented Jul 20, 2018

/hold cancel
/approve
adding lgtm from #20151 (comment)

@openshift-ci-robot openshift-ci-robot added approved Indicates a PR has been approved by an approver from all required OWNERS files. and removed do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. labels Jul 20, 2018
@liggitt liggitt added lgtm Indicates that a PR is ready to be merged. api-approved approved Indicates a PR has been approved by an approver from all required OWNERS files. and removed approved Indicates a PR has been approved by an approver from all required OWNERS files. labels Jul 20, 2018
@openshift-bot
Copy link
Contributor

/retest

Please review the full test history for this PR and help us cut down flakes.

1 similar comment
@openshift-bot
Copy link
Contributor

/retest

Please review the full test history for this PR and help us cut down flakes.

@liggitt
Copy link
Contributor

liggitt commented Jul 20, 2018

travis failure is due to old commit checking, fixed in #20381

CI is green otherwise

@openshift-ci-robot openshift-ci-robot removed the lgtm Indicates that a PR is ready to be merged. label Jul 20, 2018
@liggitt
Copy link
Contributor

liggitt commented Jul 20, 2018

fixed bump commit title, re-tagging

@liggitt
Copy link
Contributor

liggitt commented Jul 20, 2018

/lgtm

@openshift-ci-robot openshift-ci-robot added the lgtm Indicates that a PR is ready to be merged. label Jul 20, 2018
@openshift-ci-robot
Copy link

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: liggitt, simo5, stlaz

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-bot
Copy link
Contributor

/retest

Please review the full test history for this PR and help us cut down flakes.

@openshift-ci-robot
Copy link

openshift-ci-robot commented Jul 21, 2018

@stlaz: The following tests failed, say /retest to rerun them all:

Test name Commit Details Rerun command
ci/openshift-jenkins/extended_image_registry eff8d35 link /test extended_image_registry
ci/openshift-jenkins/extended_builds 8b46d16 link /test extended_builds
ci/openshift-jenkins/extended_image_ecosystem 8b46d16 link /test extended_image_ecosystem
ci/openshift-jenkins/cross 8b46d16 link /test cross
ci/prow/cross 898677a link /test cross

Full PR test history. Your PR dashboard. Please help us cut down on flakes by linking to an open issue when you hit one in your PR.

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/test-infra repository. I understand the commands that are listed here.

@openshift-bot
Copy link
Contributor

/retest

Please review the full test history for this PR and help us cut down flakes.

@openshift-merge-robot openshift-merge-robot merged commit 70e941d into openshift:master Jul 21, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
api-approved 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. needs-api-review sig/security size/XL Denotes a PR that changes 500-999 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

8 participants