Skip to content
This repository has been archived by the owner on May 6, 2022. It is now read-only.

Make the service-catalog chart setup more secure with RBAC #981

Closed
wants to merge 1 commit into from

Conversation

luxas
Copy link
Contributor

@luxas luxas commented Jun 25, 2017

Take advantage of API Aggregation
Creates the necessary RBAC rules for the apiserver and controller-manager

cc @pmorie @liggitt

…age of API Aggregation. Create RBAC rules for the apiserver and controller-manager
@k8s-ci-robot k8s-ci-robot added the cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. label Jun 25, 2017
@luxas
Copy link
Contributor Author

luxas commented Jun 26, 2017

So this is code that has been lying in my fork of service-catalog for use in https://github.com/luxas/kubeadm-workshop

I now noticed @MHBauer's PR that is kind of similar: #936, but not everything necessarily have to be the same.

We'll just see what happens I guess...

caBundle: {{ .Values.apiserver.tls.ca }}
priority: 100
{{- end }}
# Extension API Servers should have a priority value higher than the core API groups that have 100
Copy link
Contributor

Choose a reason for hiding this comment

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

Why?

Copy link

@liggitt liggitt Jun 26, 2017

Choose a reason for hiding this comment

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

The priority field actually split to groupPriorityMinimum and versionPriority in v1beta1. See kubernetes/kubernetes#46800

The ordering was also made more comprehensible (bigger numbers have higher priority). I'd recommend something like:

groupPriorityMinimum: 1000
versionPriority: 5

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks, I hadn't seen that it was updated between alpha and beta

Copy link
Contributor

@MHBauer MHBauer left a comment

Choose a reason for hiding this comment

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

If you're an expert on RBAC, I would have preferred your help in creating and reviewing #936 rather than making a new PR.

| `apiserver.tls.cert` | Base64-encoded x509 certificate | A self-signed certificate |
| `apiserver.tls.key` | Base64-encoded private key | The private key for the certificate above |
| `apiserver.tls.ca` | Base64-encoded CA certificate used to sign the above certificate | |
| `apiserver.tls.requestHeaderCA` | Base64-encoded CA used to validate request-header authentication, when receiving delegated authentication from an aggregator | *none (will disable requestheader authentication)* |
| `apiserver.service.type` | Type of service; valid values are `LoadBalancer` and `NodePort` | `NodePort` |
| `apiserver.service.type` | Type of service; valid values are `ClusterIP` (the most secure), `LoadBalancer` and `NodePort` | `ClusterIP` |
Copy link
Contributor

Choose a reason for hiding this comment

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

I think this change makes sense if we are going to enable this by default, as we won't have any other use for an external access of the apiserver.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, NodePort shouldn't be the default in an AA world

@@ -1,3 +1,63 @@
kind: ServiceAccount
Copy link
Contributor

Choose a reason for hiding this comment

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

It probably does make more sense to shove the rbac stuff into the deployments that need it set up.

Is there any guidance on the difference or preference for multi-yaml split by --- or using a List object?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I prefer --- personally at least

@luxas
Copy link
Contributor Author

luxas commented Jun 26, 2017

If you're an expert on RBAC, I would have preferred your help in creating and reviewing #936 rather than making a new PR.

I can definitely help out on that PR. As said, I wrote this code quite long ago, but first now realized I should upload a PR of it and didn't see yours.

@arschles arschles added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jul 6, 2017
@arschles
Copy link
Contributor

arschles commented Jul 6, 2017

This will need a rebase

@pmorie
Copy link
Contributor

pmorie commented Jul 18, 2017

@luxas I think this can be closed now that #936 is in, correct?

@kibbles-n-bytes
Copy link
Contributor

@luxas bump

@luxas
Copy link
Contributor Author

luxas commented Aug 4, 2017

@kibbles-n-bytes ah, forgot about this one. I'll check what's left to do here, update and let you know.
Thanks for the ping

@pmorie
Copy link
Contributor

pmorie commented Jan 4, 2018

Closing - I believe this is obsolete.

@pmorie pmorie closed this Jan 4, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

7 participants