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.

Sign up for GitHub

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

Jump to bottom

kubeadm: Make the creation of the RBAC rules phase idempotent #47081

Merged
merged 1 commit into from
Jun 7, 2017
Merged

kubeadm: Make the creation of the RBAC rules phase idempotent #47081

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
1 change: 1 addition & 0 deletions cmd/kubeadm/app/phases/addons/BUILD
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -21,6 +21,7 @@ go_library(
"//cmd/kubeadm/app/images:go_default_library",
"//cmd/kubeadm/app/util:go_default_library",
"//pkg/api:go_default_library",
"//vendor/k8s.io/apimachinery/pkg/api/errors:go_default_library",
"//vendor/k8s.io/apimachinery/pkg/apis/meta/v1:go_default_library",
"//vendor/k8s.io/apimachinery/pkg/runtime:go_default_library",
"//vendor/k8s.io/client-go/kubernetes:go_default_library",
Expand Down
42 changes: 34 additions & 8 deletions cmd/kubeadm/app/phases/addons/addons.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,9 +19,9 @@ package addons
import (
"fmt"
"net"

"runtime"

apierrors "k8s.io/apimachinery/pkg/api/errors"
metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
kuberuntime "k8s.io/apimachinery/pkg/runtime"
clientset "k8s.io/client-go/kubernetes"
Expand Down Expand Up @@ -80,13 +80,13 @@ func CreateEssentialAddons(cfg *kubeadmapi.MasterConfiguration, client *clientse
if err != nil {
return err
}
fmt.Println("[addons] Created essential addon: kube-proxy")
fmt.Println("[addons] Applied essential addon: kube-proxy")

err = CreateKubeDNSAddon(dnsDeploymentBytes, dnsServiceBytes, client)
if err != nil {
return err
}
fmt.Println("[addons] Created essential addon: kube-dns")
fmt.Println("[addons] Applied essential addon: kube-dns")
return nil
}

Expand All @@ -97,7 +97,13 @@ func CreateKubeProxyAddon(configMapBytes, daemonSetbytes []byte, client *clients
}

if _, err := client.CoreV1().ConfigMaps(metav1.NamespaceSystem).Create(kubeproxyConfigMap); err != nil {
return fmt.Errorf("unable to create a new kube-proxy configmap: %v", err)
if !apierrors.IsAlreadyExists(err) {
return fmt.Errorf("unable to create a new kube-proxy configmap: %v", err)
}

if _, err := client.CoreV1().ConfigMaps(metav1.NamespaceSystem).Update(kubeproxyConfigMap); err != nil {
Copy link
Member

Choose a reason for hiding this comment

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

Isn't this really an overwrite?

Could we stash somewhere, the previous one as a backup in case someone has modified the originals.

Same applies for *.

Copy link
Member Author

Choose a reason for hiding this comment

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

What's the reason to upgrade system-internal manifests if we don't overwrite?
Yes, it's overwriting, but on the other hand it will be super hard to detect a change without building in earlier manifests into kubeadm.

Also, how do we know what the user wants? Do they want to keep old, modified manifests or do they really want to upgrade.

If you have a preferred location for stashing things, I'm all ears though...

Copy link
Member

Choose a reason for hiding this comment

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

If I treat package management as an analogous example, it would never explicitly overwrite without user specification so they know they are going to blast them.

As of today, there is no ObjectMeta record on who authored/signed a resource update, if we knew that we could have a check.

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, that's the problem. If that was in ObjectMeta (maybe shouldn't be, but anyway), we could easily check that.

I think this should just be a caveat in the instructions. If you have modified things kubeadm installed in the kube-system name, you must redo those mods after upgrading.

Agree so we can proceed with this?

Copy link
Member

Choose a reason for hiding this comment

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

So long as we spell it out in bold in the docs I'm ok.

Copy link
Member

Choose a reason for hiding this comment

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

Also update a release note on this one to denote, and lgtm from me.

return fmt.Errorf("unable to update the kube-proxy configmap: %v", err)
}
}

kubeproxyDaemonSet := &extensions.DaemonSet{}
Expand All @@ -107,7 +113,13 @@ func CreateKubeProxyAddon(configMapBytes, daemonSetbytes []byte, client *clients
kubeproxyDaemonSet.Spec.Template.Spec.Tolerations = []v1.Toleration{kubeadmconstants.MasterToleration}

if _, err := client.ExtensionsV1beta1().DaemonSets(metav1.NamespaceSystem).Create(kubeproxyDaemonSet); err != nil {
return fmt.Errorf("unable to create a new kube-proxy daemonset: %v", err)
if !apierrors.IsAlreadyExists(err) {
return fmt.Errorf("unable to create a new kube-proxy daemonset: %v", err)
}

if _, err := client.ExtensionsV1beta1().DaemonSets(metav1.NamespaceSystem).Update(kubeproxyDaemonSet); err != nil {
return fmt.Errorf("unable to update the kube-proxy daemonset: %v", err)
}
}
return nil
}
Expand All @@ -125,9 +137,14 @@ func CreateKubeDNSAddon(deploymentBytes, serviceBytes []byte, client *clientset.
},
}

// TODO: All these .Create(foo) calls should instead be more like "kubectl apply -f" commands; they should not fail if there are existing objects with the same name
if _, err := client.ExtensionsV1beta1().Deployments(metav1.NamespaceSystem).Create(kubednsDeployment); err != nil {
return fmt.Errorf("unable to create a new kube-dns deployment: %v", err)
if !apierrors.IsAlreadyExists(err) {
return fmt.Errorf("unable to create a new kube-dns deployment: %v", err)
}

if _, err := client.ExtensionsV1beta1().Deployments(metav1.NamespaceSystem).Update(kubednsDeployment); err != nil {
return fmt.Errorf("unable to update the kube-dns deployment: %v", err)
}
}

kubednsService := &v1.Service{}
Expand All @@ -136,7 +153,16 @@ func CreateKubeDNSAddon(deploymentBytes, serviceBytes []byte, client *clientset.
}

if _, err := client.CoreV1().Services(metav1.NamespaceSystem).Create(kubednsService); err != nil {
return fmt.Errorf("unable to create a new kube-dns service: %v", err)
// Ignore if the Service is invalid with this error message:
// Service "kube-dns" is invalid: spec.clusterIP: Invalid value: "10.96.0.10": provided IP is already allocated

if !apierrors.IsAlreadyExists(err) && !apierrors.IsInvalid(err) {
return fmt.Errorf("unable to create a new kube-dns service: %v", err)
}

if _, err := client.CoreV1().Services(metav1.NamespaceSystem).Update(kubednsService); err != nil {
return fmt.Errorf("unable to create/update the kube-dns service: %v", err)
}
}
return nil
}
Expand Down
3 changes: 3 additions & 0 deletions cmd/kubeadm/app/phases/addons/manifests.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -271,6 +271,9 @@ metadata:
kubernetes.io/name: "KubeDNS"
name: kube-dns
namespace: kube-system
# Without this resourceVersion value, an update of the Service between versions will yield:
# Service "kube-dns" is invalid: metadata.resourceVersion: Invalid value: "": must be specified for an update
resourceVersion: "0"
spec:
clusterIP: {{ .DNSIP }}
ports:
Expand Down
36 changes: 30 additions & 6 deletions cmd/kubeadm/app/phases/apiconfig/clusterroles.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -105,8 +105,14 @@ func createRoles(clientset *clientset.Clientset) error {
},
}
for _, role := range roles {
if _, err := clientset.RbacV1beta1().Roles(metav1.NamespacePublic).Create(&role); err != nil {
return err
if _, err := clientset.RbacV1beta1().Roles(role.ObjectMeta.Namespace).Create(&role); err != nil {
if !apierrors.IsAlreadyExists(err) {
return fmt.Errorf("unable to create RBAC role: %v", err)
}

if _, err := clientset.RbacV1beta1().Roles(role.ObjectMeta.Namespace).Update(&role); err != nil {
return fmt.Errorf("unable to update RBAC role: %v", err)
}
}
}
return nil
Expand Down Expand Up @@ -134,8 +140,14 @@ func createRoleBindings(clientset *clientset.Clientset) error {
}

for _, roleBinding := range roleBindings {
if _, err := clientset.RbacV1beta1().RoleBindings(metav1.NamespacePublic).Create(&roleBinding); err != nil {
return err
if _, err := clientset.RbacV1beta1().RoleBindings(roleBinding.ObjectMeta.Namespace).Create(&roleBinding); err != nil {
if !apierrors.IsAlreadyExists(err) {
return fmt.Errorf("unable to create RBAC rolebinding: %v", err)
}

if _, err := clientset.RbacV1beta1().RoleBindings(roleBinding.ObjectMeta.Namespace).Update(&roleBinding); err != nil {
return fmt.Errorf("unable to update RBAC rolebinding: %v", err)
}
}
}
return nil
Expand All @@ -155,7 +167,13 @@ func createClusterRoles(clientset *clientset.Clientset) error {

for _, roleBinding := range clusterRoles {
if _, err := clientset.RbacV1beta1().ClusterRoles().Create(&roleBinding); err != nil {
return err
if !apierrors.IsAlreadyExists(err) {
return fmt.Errorf("unable to create RBAC clusterrole: %v", err)
}

if _, err := clientset.RbacV1beta1().ClusterRoles().Update(&roleBinding); err != nil {
return fmt.Errorf("unable to update RBAC clusterrole: %v", err)
}
}
}
return nil
Expand Down Expand Up @@ -216,7 +234,13 @@ func createClusterRoleBindings(clientset *clientset.Clientset) error {

for _, clusterRoleBinding := range clusterRoleBindings {
if _, err := clientset.RbacV1beta1().ClusterRoleBindings().Create(&clusterRoleBinding); err != nil {
return err
if !apierrors.IsAlreadyExists(err) {
return fmt.Errorf("unable to create RBAC clusterrolebinding: %v", err)
}

if _, err := clientset.RbacV1beta1().ClusterRoleBindings().Update(&clusterRoleBinding); err != nil {
return fmt.Errorf("unable to update RBAC clusterrolebinding: %v", err)
}
}
}
return nil
Expand Down