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

[Federation][kubefed] Create a dedicated service account for federation controller manager in the host cluster and give it appropriate permissions. #40392

Merged
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
2 changes: 2 additions & 0 deletions federation/pkg/kubefed/init/BUILD
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,7 @@ go_library(
"//pkg/api:go_default_library",
"//pkg/api/resource:go_default_library",
"//pkg/apis/extensions:go_default_library",
"//pkg/apis/rbac:go_default_library",
"//pkg/client/clientset_generated/internalclientset:go_default_library",
"//pkg/kubectl/cmd/templates:go_default_library",
"//pkg/kubectl/cmd/util:go_default_library",
Expand Down Expand Up @@ -46,6 +47,7 @@ go_test(
"//pkg/api/testapi:go_default_library",
"//pkg/api/v1:go_default_library",
"//pkg/apis/extensions/v1beta1:go_default_library",
"//pkg/apis/rbac/v1beta1:go_default_library",
"//pkg/kubectl/cmd/testing:go_default_library",
"//pkg/kubectl/cmd/util:go_default_library",
"//pkg/util/intstr:go_default_library",
Expand Down
82 changes: 79 additions & 3 deletions federation/pkg/kubefed/init/init.go
Original file line number Diff line number Diff line change
Expand Up @@ -47,6 +47,7 @@ import (
"k8s.io/kubernetes/pkg/api"
"k8s.io/kubernetes/pkg/api/resource"
"k8s.io/kubernetes/pkg/apis/extensions"
"k8s.io/kubernetes/pkg/apis/rbac"
client "k8s.io/kubernetes/pkg/client/clientset_generated/internalclientset"
"k8s.io/kubernetes/pkg/kubectl/cmd/templates"
cmdutil "k8s.io/kubernetes/pkg/kubectl/cmd/util"
Expand All @@ -62,6 +63,17 @@ const (
AdminCN = "admin"
HostClusterLocalDNSZoneName = "cluster.local."

// User name used by federation controller manager to make
Copy link
Contributor

Choose a reason for hiding this comment

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

Dont we need similar constant for federation controller manager to talk to host cluster?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That's the service account below.

// calls to federation API server.
ControllerManagerUser = "federation-controller-manager"

// Name of the ServiceAccount used by the federation controller manager
// to access the secrets in the host cluster.
ControllerManagerSA = "federation-controller-manager"

// Group name of the legacy/core API group
legacyAPIGroup = ""

lbAddrRetryInterval = 5 * time.Second
podWaitInterval = 2 * time.Second
)
Expand Down Expand Up @@ -220,7 +232,22 @@ func initFederation(cmdOut io.Writer, config util.AdminConfig, cmd *cobra.Comman
}

// 7. Create federation controller manager
_, err = createControllerManager(hostClientset, initFlags.FederationSystemNamespace, initFlags.Name, svc.Name, cmName, image, cmKubeconfigName, dnsZoneName, dnsProvider, dryRun)
// 7a. Create a service account in the host cluster for federation
// controller manager.
sa, err := createControllerManagerSA(hostClientset, initFlags.FederationSystemNamespace, dryRun)
if err != nil {
return err
}

// 7b. Create RBAC role and role binding for federation controller
// manager service account.
_, _, err = createRoleBindings(hostClientset, initFlags.FederationSystemNamespace, sa.Name, dryRun)
if err != nil {
return err
}

// 7c. Create federation controller manager deployment.
_, err = createControllerManager(hostClientset, initFlags.FederationSystemNamespace, initFlags.Name, svc.Name, cmName, image, cmKubeconfigName, dnsZoneName, dnsProvider, sa.Name, dryRun)
if err != nil {
return err
}
Expand Down Expand Up @@ -375,7 +402,7 @@ func createControllerManagerKubeconfigSecret(clientset *client.Clientset, namesp
config := kubeadmkubeconfigphase.MakeClientConfigWithCerts(
fmt.Sprintf("https://%s", svcName),
name,
"federation-controller-manager",
ControllerManagerUser,
certutil.EncodeCertPEM(entKeyPairs.ca.Cert),
certutil.EncodePrivateKeyPEM(entKeyPairs.controllerManager.Key),
certutil.EncodeCertPEM(entKeyPairs.controllerManager.Cert),
Expand Down Expand Up @@ -520,7 +547,55 @@ func createAPIServer(clientset *client.Clientset, namespace, name, image, creden
return clientset.Extensions().Deployments(namespace).Create(dep)
}

func createControllerManager(clientset *client.Clientset, namespace, name, svcName, cmName, image, kubeconfigName, dnsZoneName, dnsProvider string, dryRun bool) (*extensions.Deployment, error) {
func createControllerManagerSA(clientset *client.Clientset, namespace string, dryRun bool) (*api.ServiceAccount, error) {
sa := &api.ServiceAccount{
ObjectMeta: metav1.ObjectMeta{
Name: ControllerManagerSA,
Namespace: namespace,
Labels: componentLabel,
},
}
if dryRun {
return sa, nil
}
return clientset.Core().ServiceAccounts(namespace).Create(sa)
}

func createRoleBindings(clientset *client.Clientset, namespace, saName string, dryRun bool) (*rbac.Role, *rbac.RoleBinding, error) {
roleName := "federation-system:federation-controller-manager"
Copy link
Contributor

Choose a reason for hiding this comment

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

Use the constants here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is that constant. I intentionally made it a function-level constant as opposed to a top-level/package-level constant. This isn't supposed to be reused outside.

Copy link
Member

Choose a reason for hiding this comment

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

nit: if it's constant, declare it as a constant.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Will do that in a follow up. Thanks for the approval.

role := &rbac.Role{
Copy link
Member

@liggitt liggitt Jan 26, 2017

Choose a reason for hiding this comment

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

is the intent to have these permissions within a single namespace or across all namespaces? to grant this across all namespaces, this needs to be a clusterrole. if just inside namespace, this is fine

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This RoleBinding is very specifically for the federation-system namespace. No other namespace access is required.

// a role to use for bootstrapping the federation-controller-manager so it can access
Copy link
Member

Choose a reason for hiding this comment

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

just to clarify, this is for watching secrets stored in the federation server, where it gets credentials for other servers?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is for watching secrets stored in the host cluster where federation control plane components are hosted. And yes, to get get the credentials for all the clusters.

// secrets in the host cluster to access other clusters.
ObjectMeta: metav1.ObjectMeta{
Name: roleName,
Namespace: namespace,
Labels: componentLabel,
},
Rules: []rbac.PolicyRule{
rbac.NewRule("get", "list", "watch").Groups(legacyAPIGroup).Resources("secrets").RuleOrDie(),
},
}

rolebinding, err := rbac.NewRoleBinding(roleName, namespace).SAs(namespace, saName).Binding()
if err != nil {
return nil, nil, err
}
rolebinding.Labels = componentLabel

if dryRun {
Copy link
Contributor

Choose a reason for hiding this comment

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

Did you mean if !dryRun?

return role, &rolebinding, nil
}

newRole, err := clientset.Rbac().Roles(namespace).Create(role)
if err != nil {
return nil, nil, err
}

newRolebinding, err := clientset.Rbac().RoleBindings(namespace).Create(&rolebinding)
return newRole, newRolebinding, err
}

func createControllerManager(clientset *client.Clientset, namespace, name, svcName, cmName, image, kubeconfigName, dnsZoneName, dnsProvider, saName string, dryRun bool) (*extensions.Deployment, error) {
dep := &extensions.Deployment{
ObjectMeta: metav1.ObjectMeta{
Name: cmName,
Expand Down Expand Up @@ -578,6 +653,7 @@ func createControllerManager(clientset *client.Clientset, namespace, name, svcNa
},
},
},
ServiceAccountName: saName,
},
},
},
Expand Down
102 changes: 102 additions & 0 deletions federation/pkg/kubefed/init/init_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -43,6 +43,7 @@ import (
"k8s.io/kubernetes/pkg/api/testapi"
"k8s.io/kubernetes/pkg/api/v1"
"k8s.io/kubernetes/pkg/apis/extensions/v1beta1"
rbacv1beta1 "k8s.io/kubernetes/pkg/apis/rbac/v1beta1"
cmdtesting "k8s.io/kubernetes/pkg/kubectl/cmd/testing"
cmdutil "k8s.io/kubernetes/pkg/kubectl/cmd/util"
"k8s.io/kubernetes/pkg/util/intstr"
Expand Down Expand Up @@ -554,6 +555,62 @@ func fakeInitHostFactory(federationName, namespaceName, ip, dnsZoneName, image,
},
}

sa := v1.ServiceAccount{
TypeMeta: metav1.TypeMeta{
Kind: "ServiceAccount",
APIVersion: testapi.Default.GroupVersion().String(),
},
ObjectMeta: metav1.ObjectMeta{
Name: "federation-controller-manager",
Namespace: namespaceName,
Labels: componentLabel,
},
}

role := rbacv1beta1.Role{
TypeMeta: metav1.TypeMeta{
Kind: "Role",
APIVersion: testapi.Rbac.GroupVersion().String(),
},
ObjectMeta: metav1.ObjectMeta{
Name: "federation-system:federation-controller-manager",
Namespace: namespaceName,
Labels: componentLabel,
},
Rules: []rbacv1beta1.PolicyRule{
{
Verbs: []string{"get", "list", "watch"},
APIGroups: []string{""},
Resources: []string{"secrets"},
},
},
}

rolebinding := rbacv1beta1.RoleBinding{
TypeMeta: metav1.TypeMeta{
Kind: "RoleBinding",
APIVersion: testapi.Rbac.GroupVersion().String(),
},
ObjectMeta: metav1.ObjectMeta{
Name: "federation-system:federation-controller-manager",
Namespace: namespaceName,
Labels: componentLabel,
},
Subjects: []rbacv1beta1.Subject{
{
Kind: "ServiceAccount",
APIVersion: "",
Name: "federation-controller-manager",
Namespace: "federation-system",
},
},
RoleRef: rbacv1beta1.RoleRef{
APIGroup: "rbac.authorization.k8s.io",
Kind: "Role",
Name: "federation-system:federation-controller-manager",
},
}

apiserver := v1beta1.Deployment{
TypeMeta: metav1.TypeMeta{
Kind: "Deployment",
Expand Down Expand Up @@ -709,6 +766,8 @@ func fakeInitHostFactory(federationName, namespaceName, ip, dnsZoneName, image,
},
},
},
ServiceAccountName: "federation-controller-manager",
DeprecatedServiceAccount: "federation-controller-manager",
},
},
},
Expand Down Expand Up @@ -748,6 +807,7 @@ func fakeInitHostFactory(federationName, namespaceName, ip, dnsZoneName, image,

f, tf, codec, _ := cmdtesting.NewAPIFactory()
extCodec := testapi.Extensions.Codec()
rbacCodec := testapi.Rbac.Codec()
ns := dynamic.ContentConfig().NegotiatedSerializer
tf.ClientConfig = kubefedtesting.DefaultClientConfig()
tf.Client = &fake.RESTClient{
Expand Down Expand Up @@ -852,6 +912,48 @@ func fakeInitHostFactory(federationName, namespaceName, ip, dnsZoneName, image,
return &http.Response{StatusCode: http.StatusCreated, Header: kubefedtesting.DefaultHeader(), Body: kubefedtesting.ObjBody(extCodec, &want)}, nil
case p == "/api/v1/namespaces/federation-system/pods" && m == http.MethodGet:
return &http.Response{StatusCode: http.StatusOK, Header: kubefedtesting.DefaultHeader(), Body: kubefedtesting.ObjBody(codec, &podList)}, nil
case p == "/api/v1/namespaces/federation-system/serviceaccounts" && m == http.MethodPost:
body, err := ioutil.ReadAll(req.Body)
if err != nil {
return nil, err
}
var got v1.ServiceAccount
_, _, err = codec.Decode(body, nil, &got)
if err != nil {
return nil, err
}
if !api.Semantic.DeepEqual(got, sa) {
return nil, fmt.Errorf("Unexpected service account object\n\tDiff: %s", diff.ObjectGoPrintDiff(got, sa))
}
return &http.Response{StatusCode: http.StatusCreated, Header: kubefedtesting.DefaultHeader(), Body: kubefedtesting.ObjBody(codec, &sa)}, nil
case p == "/apis/rbac.authorization.k8s.io/v1beta1/namespaces/federation-system/roles" && m == http.MethodPost:
body, err := ioutil.ReadAll(req.Body)
if err != nil {
return nil, err
}
var got rbacv1beta1.Role
_, _, err = codec.Decode(body, nil, &got)
if err != nil {
return nil, err
}
if !api.Semantic.DeepEqual(got, role) {
return nil, fmt.Errorf("Unexpected role object\n\tDiff: %s", diff.ObjectGoPrintDiff(got, role))
}
return &http.Response{StatusCode: http.StatusCreated, Header: kubefedtesting.DefaultHeader(), Body: kubefedtesting.ObjBody(rbacCodec, &role)}, nil
case p == "/apis/rbac.authorization.k8s.io/v1beta1/namespaces/federation-system/rolebindings" && m == http.MethodPost:
body, err := ioutil.ReadAll(req.Body)
if err != nil {
return nil, err
}
var got rbacv1beta1.RoleBinding
_, _, err = codec.Decode(body, nil, &got)
if err != nil {
return nil, err
}
if !api.Semantic.DeepEqual(got, rolebinding) {
return nil, fmt.Errorf("Unexpected rolebinding object\n\tDiff: %s", diff.ObjectGoPrintDiff(got, rolebinding))
}
return &http.Response{StatusCode: http.StatusCreated, Header: kubefedtesting.DefaultHeader(), Body: kubefedtesting.ObjBody(rbacCodec, &rolebinding)}, nil
default:
return nil, fmt.Errorf("unexpected request: %#v\n%#v", req.URL, req)
}
Expand Down
71 changes: 71 additions & 0 deletions pkg/apis/rbac/helpers.go
Original file line number Diff line number Diff line change
Expand Up @@ -254,3 +254,74 @@ func (r *ClusterRoleBindingBuilder) Binding() (ClusterRoleBinding, error) {

return r.ClusterRoleBinding, nil
}

// +k8s:deepcopy-gen=false
Copy link
Member

Choose a reason for hiding this comment

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

I'm not a huge fan of having helpers in the internal types api packages. @deads2k

I guess you're not making it worse, so I'm not asking for you to solve this problem.

Copy link
Contributor

Choose a reason for hiding this comment

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

I'm not a huge fan of having helpers in the internal types api packages. @deads2k

RBAC really demonstrates the issue with using external versions. The RBAC authorizer doesn't care which version of the API you use, but you run into crazy problems with caches, informers, conversions, and helpers if you don't use the internal types.

I suspect that if you saw the code duplication involved in having v1beta1 and v1alpha1 support, you'd be displeased.

// RoleBindingBuilder let's us attach methods. It is similar to
// ClusterRoleBindingBuilder above.
type RoleBindingBuilder struct {
RoleBinding RoleBinding
}

// NewRoleBinding creates a RoleBinding builder that can be used
// to define the subjects of a role binding. At least one of
// the `Groups`, `Users` or `SAs` method must be called before
// calling the `Binding*` methods.
func NewRoleBinding(roleName, namespace string) *RoleBindingBuilder {
Copy link
Member

Choose a reason for hiding this comment

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

Document that at least one of the functions below must be called.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done.

return &RoleBindingBuilder{
RoleBinding: RoleBinding{
ObjectMeta: metav1.ObjectMeta{
Name: roleName,
Namespace: namespace,
},
RoleRef: RoleRef{
APIGroup: GroupName,
Kind: "Role",
Name: roleName,
},
},
}
}

// Groups adds the specified groups as the subjects of the RoleBinding.
func (r *RoleBindingBuilder) Groups(groups ...string) *RoleBindingBuilder {
Copy link
Member

Choose a reason for hiding this comment

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

Public methods should have a godoc string.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done.

for _, group := range groups {
r.RoleBinding.Subjects = append(r.RoleBinding.Subjects, Subject{Kind: GroupKind, Name: group})
}
return r
}

// Users adds the specified users as the subjects of the RoleBinding.
func (r *RoleBindingBuilder) Users(users ...string) *RoleBindingBuilder {
for _, user := range users {
r.RoleBinding.Subjects = append(r.RoleBinding.Subjects, Subject{Kind: UserKind, Name: user})
}
return r
}

// SAs adds the specified service accounts as the subjects of the
// RoleBinding.
func (r *RoleBindingBuilder) SAs(namespace string, serviceAccountNames ...string) *RoleBindingBuilder {
for _, saName := range serviceAccountNames {
r.RoleBinding.Subjects = append(r.RoleBinding.Subjects, Subject{Kind: ServiceAccountKind, Namespace: namespace, Name: saName})
}
return r
}

// BindingOrDie calls the binding method and panics if there is an error.
func (r *RoleBindingBuilder) BindingOrDie() RoleBinding {
ret, err := r.Binding()
if err != nil {
panic(err)
}
return ret
}

// Binding builds and returns the RoleBinding API object from the builder
// object.
func (r *RoleBindingBuilder) Binding() (RoleBinding, error) {
if len(r.RoleBinding.Subjects) == 0 {
return RoleBinding{}, fmt.Errorf("subjects are required: %#v", r.RoleBinding)
}

return r.RoleBinding, nil
}