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

Secure kube-scheduler #41285

Merged
merged 3 commits into from
Feb 15, 2017
Merged

Conversation

liggitt
Copy link
Member

@liggitt liggitt commented Feb 11, 2017

This PR:

  • Adds a bootstrap system:kube-scheduler clusterrole
  • Adds a bootstrap clusterrolebinding to the system:kube-scheduler user
  • Sets up a kubeconfig for kube-scheduler on GCE (following the controller-manager pattern)
  • Switches kube-scheduler to running with kubeconfig against secured port (salt changes, beware)
  • Removes superuser permissions from kube-scheduler in local-up-cluster.sh
  • Adds detailed RBAC deny logging
On kube-up.sh clusters on GCE, kube-scheduler now contacts the API on the secured port.

@k8s-ci-robot k8s-ci-robot added the cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. label Feb 11, 2017
@k8s-reviewable
Copy link

This change is Reviewable

@k8s-github-robot k8s-github-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Feb 11, 2017
@k8s-github-robot k8s-github-robot added the size/L Denotes a PR that changes 100-499 lines, ignoring generated files. label Feb 11, 2017
@k8s-github-robot k8s-github-robot added release-note-label-needed and removed approved Indicates a PR has been approved by an approver from all required OWNERS files. labels Feb 11, 2017
@liggitt liggitt added release-note Denotes a PR that will be considered when it comes time to generate release notes. release-note-none Denotes a PR that doesn't merit a release note. and removed release-note-label-needed release-note Denotes a PR that will be considered when it comes time to generate release notes. release-note-none Denotes a PR that doesn't merit a release note. labels Feb 11, 2017
@liggitt liggitt changed the title Create bootstrap system:kube-scheduler role Secure kube-scheduler Feb 11, 2017
@smarterclayton
Copy link
Contributor

Audit and role changes lgtm, not qualified for salt

rbac.NewRule("update").Groups(legacyGroup).Resources("pods/status").RuleOrDie(),
// things that select pods
rbac.NewRule(Read...).Groups(legacyGroup).Resources("services", "replicationcontrollers").RuleOrDie(),
rbac.NewRule(Read...).Groups(extensionsGroup).Resources("replicasets").RuleOrDie(),
Copy link
Contributor

Choose a reason for hiding this comment

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

This isn't a pattern that's going to scale out well people build other resources that result in pods. Something @kubernetes/sig-scheduling-misc is looking at? We can't bake them all in.

Copy link
Member

Choose a reason for hiding this comment

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

Can we just say that "system:kube-scheduler" is for the built-in scheduler, and custom schedulers may need to come packaged w/ ClusterRoles & ClusterRoleBindings?

Copy link
Member Author

Choose a reason for hiding this comment

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

yeah, we're saying this won't scale well for the built-in scheduler... it's currently trying to spread selected pods by watching all the resources that use label selectors over pods

Copy link
Member

Choose a reason for hiding this comment

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

😟 yeah that is concerning.

Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe a subresource for selectors?

@deads2k
Copy link
Contributor

deads2k commented Feb 13, 2017

permissions look fine.

@cjcullen @mikedanese The script changes seem to be working enough for e2e. Is there something else you'd like to check?

@@ -360,6 +363,30 @@ current-context: service-account-context
EOF
}

function create-kubescheduler-kubeconfig {
Copy link
Member

@timothysc timothysc Feb 13, 2017

Choose a reason for hiding this comment

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

Has this percolated to all other systems?

kops/kargo/kubeadm?

/cc @luxas

Copy link
Member Author

@liggitt liggitt Feb 13, 2017

Choose a reason for hiding this comment

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

no. presumably most of those are still running the scheduler against the unsecured port as well, but that doesn't need to block this.

Copy link
Member

Choose a reason for hiding this comment

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

/cc @justinsb @kubernetes/kubeadm-maintainers

Copy link
Member Author

Choose a reason for hiding this comment

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

working on a guide for deployers setting up control plane components to work out of the box with RBAC at https://docs.google.com/document/d/1PqI--ql3LQsA69fEvRq1nQWgiIoE5Dyftja5Um9ML7Q/edit

that will be pulled into the doc repo as soon as we have a way to open PRs for 1.6 doc content

requestAttributes.GetResource(), requestAttributes.GetAPIGroup(), requestAttributes.GetSubresource(),
)
b := &bytes.Buffer{}
fmt.Fprint(b, `"`)
Copy link
Member

Choose a reason for hiding this comment

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

Replacing these Fprints with b.WriteString() would eliminate some unnecessary string scanning.

Copy link
Member Author

Choose a reason for hiding this comment

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

done

@cjcullen
Copy link
Member

Changes LGTM. I don't think there is anything on the GKE-side that will need to change for this (the scripts should be self-contained for this token).

@@ -769,7 +769,7 @@ start_kube_scheduler() {
if [ -n "${SCHEDULER_TEST_LOG_LEVEL:-}" ]; then
log_level="${SCHEDULER_TEST_LOG_LEVEL}"
fi
params="${log_level} ${SCHEDULER_TEST_ARGS:-}"
params="--master=127.0.0.1:8080 ${log_level} ${SCHEDULER_TEST_ARGS:-}"
Copy link
Member

Choose a reason for hiding this comment

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

Why does this one not use the secure port?

Copy link
Member

Choose a reason for hiding this comment

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

Do we even test or use this kube-up?

Copy link
Member Author

Choose a reason for hiding this comment

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

Not that I know of, so I was just maintaining status quo since the --master arg moved into the params

@liggitt
Copy link
Member Author

liggitt commented Feb 14, 2017

@wojtek-t for kubemark review and approval

@wojtek-t
Copy link
Member

changess to kubemark lgtm

@smarterclayton
Copy link
Contributor

@mikedanese can you approve the salt changes?

@liggitt liggitt added this to the v1.6 milestone Feb 14, 2017
@mikedanese
Copy link
Member

/approve
/lgtm

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Feb 14, 2017
@smarterclayton
Copy link
Contributor

smarterclayton commented Feb 14, 2017 via email

@k8s-github-robot k8s-github-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Feb 14, 2017
@k8s-github-robot
Copy link

[APPROVALNOTIFIER] This PR is APPROVED

The following people have approved this PR: liggitt, mikedanese, smarterclayton

Needs approval from an approver in each of these OWNERS Files:

You can indicate your approval by writing /approve in a comment
You can cancel your approval by writing /approve cancel in a comment

@k8s-github-robot k8s-github-robot removed the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Feb 15, 2017
@liggitt
Copy link
Member Author

liggitt commented Feb 15, 2017

rebased, changed apiVersion to apiGroup in bootstrap role testdata

@liggitt liggitt added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Feb 15, 2017
@k8s-github-robot
Copy link

Automatic merge from submit-queue (batch tested with PRs 40297, 41285, 41211, 41243, 39735)

@k8s-github-robot k8s-github-robot merged commit e4a4fe4 into kubernetes:master Feb 15, 2017
@liggitt liggitt deleted the kube-scheduler-role branch February 15, 2017 14:08
@wojtek-t
Copy link
Member

This unfortunately broke kubemark - I'm going to revert it (to unblock submit queue).

@deads2k
Copy link
Contributor

deads2k commented Feb 15, 2017

This unfortunately broke kubemark - I'm going to revert it (to unblock submit queue).

@liggitt

@liggitt
Copy link
Member Author

liggitt commented Feb 15, 2017

fixed in #41480

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. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. lgtm "Looks good to me", indicates that a PR is ready to be merged. release-note Denotes a PR that will be considered when it comes time to generate release notes. size/L Denotes a PR that changes 100-499 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.