karmada-io
/
karmada
mirror of https://github.com/karmada-io/karmada.git
1
0
Fork 0
Code Issues Packages Projects Releases Wiki Activity

Merge pull request #2825 from jwcesign/code-review-fix 

Add more logs and delete some redundant code
This commit is contained in:
karmada-bot 2022-11-24 14:28:07 +08:00 committed by GitHub
parent dfd0ace0b1 2c3141980e
commit 63a67d7cbd
No known key found for this signature in database
GPG Key ID: 4AEE18F83AFDEB23
9 changed files with 154 additions and 351 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

4
pkg/karmadactl/register/register.go
View File

@ -580,7 +580,7 @@ func (o *CommandRegisterOption) createSecretAndRBACInMemberCluster(karmadaAgentC
}
// create a karmada-agent ClusterRole in member cluster.
if err := karmadautil.CreateOrUpdateClusterRole(o.memberClusterClient, clusterRole); err != nil {
if err := cmdutil.CreateOrUpdateClusterRole(o.memberClusterClient, clusterRole); err != nil {
return err
}
@ -616,7 +616,7 @@ func (o *CommandRegisterOption) createSecretAndRBACInMemberCluster(karmadaAgentC
}
// grant karmada-agent clusterrole to karmada-agent service account
if err := karmadautil.CreateOrUpdateClusterRoleBinding(o.memberClusterClient, clusterRoleBinding); err != nil {
if err := cmdutil.CreateOrUpdateClusterRoleBinding(o.memberClusterClient, clusterRoleBinding); err != nil {
return err
}

6
pkg/karmadactl/util/idempotency.go
View File

@ -48,6 +48,8 @@ func CreateOrUpdateSecret(client kubeclient.Interface, secret *corev1.Secret) er
return fmt.Errorf("unable to update Secret: %v", err)
}
}
klog.V(2).Infof("Secret %s/%s has been created or updated.", secret.Namespace, secret.Name)
return nil
}
@ -70,6 +72,8 @@ func CreateOrUpdateDeployment(client kubeclient.Interface, deploy *appsv1.Deploy
return fmt.Errorf("unable to update Deployment: %v", err)
}
}
klog.V(2).Infof("Deployment %s/%s has been created or updated.", deploy.Namespace, deploy.Name)
return nil
}
@ -86,7 +90,7 @@ func CreateOrUpdateAPIService(apiRegistrationClient *aggregator.Clientset, apise
return err
}
apiservice.ObjectMeta.ResourceVersion = existAPIService.ObjectMeta.ResourceVersion
apiservice.ResourceVersion = existAPIService.ResourceVersion
if _, err := apiRegistrationClient.ApiregistrationV1().APIServices().Update(context.TODO(), apiservice, metav1.UpdateOptions{}); err != nil {
return fmt.Errorf("unable to update APIService: %v", err)

142
pkg/karmadactl/util/idempotency_test.go
View File

@ -2,9 +2,11 @@ package util
import (
"errors"
"fmt"
"testing"
corev1 "k8s.io/api/core/v1"
rbacv1 "k8s.io/api/rbac/v1"
metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
"k8s.io/apimachinery/pkg/runtime"
"k8s.io/client-go/kubernetes"
@ -12,6 +14,10 @@ import (
coretesting "k8s.io/client-go/testing"
)
var errorAction = func(coretesting.Action) (handled bool, ret runtime.Object, err error) {
return true, nil, fmt.Errorf("always error")
}
func TestCreateOrUpdateSecret(t *testing.T) {
type args struct {
client kubernetes.Interface
@ -84,3 +90,139 @@ func makeSecret(name string) *corev1.Secret {
},
}
}
func TestCreateOrUpdateClusterRole(t *testing.T) {
type args struct {
client kubernetes.Interface
clusterRole *rbacv1.ClusterRole
}
tests := []struct {
name string
args args
wantErr bool
}{
{
name: "not exist and create",
args: args{
client: fake.NewSimpleClientset(),
clusterRole: makeClusterRole("test"),
},
wantErr: false,
},
{
name: "already exist and update",
args: args{
client: fake.NewSimpleClientset(makeClusterRole("test")),
clusterRole: makeClusterRole("test"),
},
wantErr: false,
},
{
name: "create error",
args: args{
client: func() kubernetes.Interface {
c := fake.NewSimpleClientset()
c.PrependReactor("create", "*", errorAction)
return c
}(),
clusterRole: makeClusterRole("test"),
},
wantErr: true,
},
{
name: "update error",
args: args{
client: func() kubernetes.Interface {
c := fake.NewSimpleClientset(makeClusterRole("test"))
c.PrependReactor("update", "*", errorAction)
return c
}(),
clusterRole: makeClusterRole("test"),
},
wantErr: true,
},
}
for _, tt := range tests {
t.Run(tt.name, func(t *testing.T) {
if err := CreateOrUpdateClusterRole(tt.args.client, tt.args.clusterRole); (err != nil) != tt.wantErr {
t.Errorf("CreateOrUpdateClusterRole() error = %v, wantErr %v", err, tt.wantErr)
}
})
}
}
func TestCreateOrUpdateClusterRoleBinding(t *testing.T) {
type args struct {
client kubernetes.Interface
clusterRoleBinding *rbacv1.ClusterRoleBinding
}
tests := []struct {
name string
args args
wantErr bool
}{
{
name: "not exist and create",
args: args{
client: fake.NewSimpleClientset(),
clusterRoleBinding: makeClusterRoleBinding("test"),
},
wantErr: false,
},
{
name: "already exist and update",
args: args{
client: fake.NewSimpleClientset(makeClusterRole("test")),
clusterRoleBinding: makeClusterRoleBinding("test"),
},
wantErr: false,
},
{
name: "create error",
args: args{
client: func() kubernetes.Interface {
c := fake.NewSimpleClientset()
c.PrependReactor("create", "*", errorAction)
return c
}(),
clusterRoleBinding: makeClusterRoleBinding("test"),
},
wantErr: true,
},
{
name: "update error",
args: args{
client: func() kubernetes.Interface {
c := fake.NewSimpleClientset(makeClusterRoleBinding("test"))
c.PrependReactor("update", "*", errorAction)
return c
}(),
clusterRoleBinding: makeClusterRoleBinding("test"),
},
wantErr: true,
},
}
for _, tt := range tests {
t.Run(tt.name, func(t *testing.T) {
if err := CreateOrUpdateClusterRoleBinding(tt.args.client, tt.args.clusterRoleBinding); (err != nil) != tt.wantErr {
t.Errorf("CreateOrUpdateClusterRoleBinding() error = %v, wantErr %v", err, tt.wantErr)
}
})
}
}
func makeClusterRole(name string) *rbacv1.ClusterRole {
return &rbacv1.ClusterRole{
ObjectMeta: metav1.ObjectMeta{
Name: name,
},
}
}
func makeClusterRoleBinding(name string) *rbacv1.ClusterRoleBinding {
return &rbacv1.ClusterRoleBinding{
ObjectMeta: metav1.ObjectMeta{
Name: name,
},
}
}

22
pkg/util/namespace.go
View File

@ -10,20 +10,6 @@ import (
kubeclient "k8s.io/client-go/kubernetes"
)
// IsNamespaceExist tells if specific already exists.
func IsNamespaceExist(client kubeclient.Interface, namespace string) (bool, error) {
_, err := client.CoreV1().Namespaces().Get(context.Background(), namespace, metav1.GetOptions{})
if err != nil {
if apierrors.IsNotFound(err) {
return false, nil
}
return false, err
}
return true, nil
}
// CreateNamespace just try to create the namespace.
func CreateNamespace(client kubeclient.Interface, namespaceObj *corev1.Namespace) (*corev1.Namespace, error) {
_, err := client.CoreV1().Namespaces().Create(context.TODO(), namespaceObj, metav1.CreateOptions{})
@ -57,14 +43,6 @@ func EnsureNamespaceExist(client kubeclient.Interface, namespace string, dryRun
return namespaceObj, nil
}
exist, err := IsNamespaceExist(client, namespace)
if err != nil {
return nil, fmt.Errorf("failed to check if namespace exist. namespace: %s, error: %v", namespace, err)
}
if exist {
return namespaceObj, nil
}
createdObj, err := CreateNamespace(client, namespaceObj)
if err != nil {
return nil, fmt.Errorf("ensure namespace failed due to create failed. namespace: %s, error: %v", namespace, err)

84
pkg/util/namespace_test.go
View File

@ -6,76 +6,14 @@ import (
"testing"
corev1 "k8s.io/api/core/v1"
apierrors "k8s.io/apimachinery/pkg/api/errors"
metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
"k8s.io/apimachinery/pkg/runtime"
"k8s.io/apimachinery/pkg/runtime/schema"
"k8s.io/client-go/kubernetes/fake"
coretesting "k8s.io/client-go/testing"
)
func TestIsNamespaceExist(t *testing.T) {
type args struct {
client *fake.Clientset
namespace string
reactor coretesting.ReactionFunc
}
tests := []struct {
name string
args args
want bool
wantErr bool
}{
{
name: "query namespace error",
args: args{
client: fake.NewSimpleClientset(&corev1.Namespace{ObjectMeta: metav1.ObjectMeta{Name: "default"}}),
namespace: "default",
reactor: func(action coretesting.Action) (handled bool, ret runtime.Object, err error) {
return true, &corev1.Namespace{}, errors.New("failed to get namespace")
},
},
want: false,
wantErr: true,
},
{
name: "namespace not exists",
args: args{
client: fake.NewSimpleClientset(&corev1.Namespace{ObjectMeta: metav1.ObjectMeta{Name: "default-1"}}),
namespace: "default",
reactor: nil,
},
want: false,
wantErr: false,
},
{
name: "namespace already exists",
args: args{
client: fake.NewSimpleClientset(&corev1.Namespace{ObjectMeta: metav1.ObjectMeta{Name: "default"}}),
namespace: "default",
reactor: nil,
},
want: true,
wantErr: false,
},
}
for _, tt := range tests {
t.Run(tt.name, func(t *testing.T) {
if tt.args.reactor != nil {
tt.args.client.PrependReactor("get", "namespaces", tt.args.reactor)
}
got, err := IsNamespaceExist(tt.args.client, tt.args.namespace)
if (err == nil && tt.wantErr == true) || (err != nil && tt.wantErr == false) {
t.Errorf("IsNamespaceExist() error = %v, wantErr %v", err, tt.wantErr)
return
}
if got != tt.want {
t.Errorf("IsNamespaceExist() got = %v, want %v", got, tt.want)
}
})
}
}
func TestCreateNamespace(t *testing.T) {
type args struct {
client *fake.Clientset
@ -197,7 +135,6 @@ func TestEnsureNamespaceExist(t *testing.T) {
client *fake.Clientset
namespace string
dryRun bool
reactorGet coretesting.ReactionFunc
reactorCreate coretesting.ReactionFunc
}
tests := []struct {
@ -219,19 +156,11 @@ func TestEnsureNamespaceExist(t *testing.T) {
args: args{
client: fake.NewSimpleClientset(&corev1.Namespace{ObjectMeta: metav1.ObjectMeta{Name: "default"}}),
namespace: "default",
},
wantErr: false,
},
{
name: "check namespace exists error",
args: args{
client: fake.NewSimpleClientset(&corev1.Namespace{ObjectMeta: metav1.ObjectMeta{Name: "default"}}),
namespace: "default",
reactorGet: func(action coretesting.Action) (handled bool, ret runtime.Object, err error) {
return true, nil, errors.New("failed to get namespace")
reactorCreate: func(action coretesting.Action) (handled bool, ret runtime.Object, err error) {
return true, nil, apierrors.NewAlreadyExists(schema.ParseGroupResource("namespaces"), "default")
},
},
wantErr: true,
wantErr: false,
},
{
name: "create namespace error",
@ -247,9 +176,6 @@ func TestEnsureNamespaceExist(t *testing.T) {
}
for _, tt := range tests {
t.Run(tt.name, func(t *testing.T) {
if tt.args.reactorGet != nil {
tt.args.client.PrependReactor("get", "namespaces", tt.args.reactorGet)
}
if tt.args.reactorCreate != nil {
tt.args.client.PrependReactor("create", "namespaces", tt.args.reactorCreate)
}

34
pkg/util/rbac.go
View File

@ -2,13 +2,11 @@ package util
import (
"context"
"fmt"
rbacv1 "k8s.io/api/rbac/v1"
apierrors "k8s.io/apimachinery/pkg/api/errors"
metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
kubeclient "k8s.io/client-go/kubernetes"
"k8s.io/klog/v2"
)
// IsClusterRoleExist tells if specific ClusterRole already exists.
@ -161,35 +159,3 @@ func GenerateImpersonationRules(allSubjects []rbacv1.Subject) []rbacv1.PolicyRul
return rules
}
// CreateOrUpdateClusterRole creates a ClusterRole if the target resource doesn't exist. If the resource exists already, this function will update the resource instead.
func CreateOrUpdateClusterRole(client kubeclient.Interface, clusterRole *rbacv1.ClusterRole) error {
if _, err := client.RbacV1().ClusterRoles().Create(context.TODO(), clusterRole, metav1.CreateOptions{}); err != nil {
if !apierrors.IsAlreadyExists(err) {
return fmt.Errorf("unable to create RBAC clusterrole: %v", err)
}
if _, err := client.RbacV1().ClusterRoles().Update(context.TODO(), clusterRole, metav1.UpdateOptions{}); err != nil {
return fmt.Errorf("unable to update RBAC clusterrole: %v", err)
}
}
klog.V(1).Infof("ClusterRole %s has been created or updated.", clusterRole.Name)
return nil
}
// CreateOrUpdateClusterRoleBinding creates a ClusterRoleBinding if the target resource doesn't exist. If the resource exists already, this function will update the resource instead.
func CreateOrUpdateClusterRoleBinding(client kubeclient.Interface, clusterRoleBinding *rbacv1.ClusterRoleBinding) error {
if _, err := client.RbacV1().ClusterRoleBindings().Create(context.TODO(), clusterRoleBinding, metav1.CreateOptions{}); err != nil {
if !apierrors.IsAlreadyExists(err) {
return fmt.Errorf("unable to create RBAC clusterrolebinding: %v", err)
}
if _, err := client.RbacV1().ClusterRoleBindings().Update(context.TODO(), clusterRoleBinding, metav1.UpdateOptions{}); err != nil {
return fmt.Errorf("unable to update RBAC clusterrolebinding: %v", err)
}
}
klog.V(1).Infof("ClusterRoleBinding %s has been created or updated.", clusterRoleBinding.Name)
return nil
}

120
pkg/util/rbac_test.go
View File

@ -115,126 +115,6 @@ func TestCreateClusterRoleBinding(t *testing.T) {
}
}
func TestCreateOrUpdateClusterRole(t *testing.T) {
type args struct {
client kubernetes.Interface
clusterRole *rbacv1.ClusterRole
}
tests := []struct {
name string
args args
wantErr bool
}{
{
name: "not exist and create",
args: args{
client: fake.NewSimpleClientset(),
clusterRole: makeClusterRole("test"),
},
wantErr: false,
},
{
name: "already exist and update",
args: args{
client: fake.NewSimpleClientset(makeClusterRole("test")),
clusterRole: makeClusterRole("test"),
},
wantErr: false,
},
{
name: "create error",
args: args{
client: func() kubernetes.Interface {
c := fake.NewSimpleClientset()
c.PrependReactor("create", "*", errorAction)
return c
}(),
clusterRole: makeClusterRole("test"),
},
wantErr: true,
},
{
name: "update error",
args: args{
client: func() kubernetes.Interface {
c := fake.NewSimpleClientset(makeClusterRole("test"))
c.PrependReactor("update", "*", errorAction)
return c
}(),
clusterRole: makeClusterRole("test"),
},
wantErr: true,
},
}
for _, tt := range tests {
t.Run(tt.name, func(t *testing.T) {
if err := CreateOrUpdateClusterRole(tt.args.client, tt.args.clusterRole); (err != nil) != tt.wantErr {
t.Errorf("CreateOrUpdateClusterRole() error = %v, wantErr %v", err, tt.wantErr)
}
})
}
}
func TestCreateOrUpdateClusterRoleBinding(t *testing.T) {
type args struct {
client kubernetes.Interface
clusterRoleBinding *rbacv1.ClusterRoleBinding
}
tests := []struct {
name string
args args
wantErr bool
}{
{
name: "not exist and create",
args: args{
client: fake.NewSimpleClientset(),
clusterRoleBinding: makeClusterRoleBinding("test"),
},
wantErr: false,
},
{
name: "already exist and update",
args: args{
client: fake.NewSimpleClientset(makeClusterRole("test")),
clusterRoleBinding: makeClusterRoleBinding("test"),
},
wantErr: false,
},
{
name: "create error",
args: args{
client: func() kubernetes.Interface {
c := fake.NewSimpleClientset()
c.PrependReactor("create", "*", errorAction)
return c
}(),
clusterRoleBinding: makeClusterRoleBinding("test"),
},
wantErr: true,
},
{
name: "update error",
args: args{
client: func() kubernetes.Interface {
c := fake.NewSimpleClientset(makeClusterRoleBinding("test"))
c.PrependReactor("update", "*", errorAction)
return c
}(),
clusterRoleBinding: makeClusterRoleBinding("test"),
},
wantErr: true,
},
}
for _, tt := range tests {
t.Run(tt.name, func(t *testing.T) {
if err := CreateOrUpdateClusterRoleBinding(tt.args.client, tt.args.clusterRoleBinding); (err != nil) != tt.wantErr {
t.Errorf("CreateOrUpdateClusterRoleBinding() error = %v, wantErr %v", err, tt.wantErr)
}
})
}
}
func TestDeleteClusterRole(t *testing.T) {
type args struct {
client kubernetes.Interface

22
pkg/util/serviceaccount.go
View File

@ -26,20 +26,6 @@ func CreateServiceAccount(client kubeclient.Interface, saObj *corev1.ServiceAcco
return saObj, nil
}
// IsServiceAccountExist tells if specific service account already exists.
func IsServiceAccountExist(client kubeclient.Interface, namespace string, name string) (bool, error) {
_, err := client.CoreV1().ServiceAccounts(namespace).Get(context.Background(), name, metav1.GetOptions{})
if err != nil {
if apierrors.IsNotFound(err) {
return false, nil
}
return false, err
}
return true, nil
}
// DeleteServiceAccount just try to delete the ServiceAccount.
func DeleteServiceAccount(client kubeclient.Interface, namespace, name string) error {
err := client.CoreV1().ServiceAccounts(namespace).Delete(context.Background(), name, metav1.DeleteOptions{})
@ -56,14 +42,6 @@ func EnsureServiceAccountExist(client kubeclient.Interface, serviceAccountObj *c
return serviceAccountObj, nil
}
exist, err := IsServiceAccountExist(client, serviceAccountObj.Namespace, serviceAccountObj.Name)
if err != nil {
return nil, fmt.Errorf("failed to check if service account exist. service account: %s/%s, error: %v", serviceAccountObj.Namespace, serviceAccountObj.Name, err)
}
if exist {
return serviceAccountObj, nil
}
createdObj, err := CreateServiceAccount(client, serviceAccountObj)
if err != nil {
return nil, fmt.Errorf("ensure service account failed due to create failed. service account: %s/%s, error: %v", serviceAccountObj.Namespace, serviceAccountObj.Name, err)

71
pkg/util/serviceaccount_test.go
View File

@ -158,20 +158,6 @@ func TestEnsureServiceAccountExist(t *testing.T) {
want: makeServiceAccount("test"),
wantErr: false,
},
{
name: "get error",
args: args{
client: func() kubernetes.Interface {
c := fake.NewSimpleClientset()
c.PrependReactor("get", "*", errorAction)
return c
}(),
serviceAccountObj: makeServiceAccount("test"),
dryRun: false,
},
want: nil,
wantErr: true,
},
{
name: "create error",
args: args{
@ -201,63 +187,6 @@ func TestEnsureServiceAccountExist(t *testing.T) {
}
}
func TestIsServiceAccountExist(t *testing.T) {
type args struct {
client kubernetes.Interface
namespace string
name string
}
tests := []struct {
name string
args args
want bool
wantErr bool
}{
{
name: "not found",
args: args{
client: fake.NewSimpleClientset(),
namespace: metav1.NamespaceDefault,
name: "test",
},
want: false,
wantErr: false,
},
{
name: "error",
args: args{
client: alwaysErrorKubeClient,
namespace: metav1.NamespaceDefault,
name: "test",
},
want: false,
wantErr: true,
},
{
name: "error",
args: args{
client: fake.NewSimpleClientset(makeServiceAccount("test")),
namespace: metav1.NamespaceDefault,
name: "test",
},
want: true,
wantErr: false,
},
}
for _, tt := range tests {
t.Run(tt.name, func(t *testing.T) {
got, err := IsServiceAccountExist(tt.args.client, tt.args.namespace, tt.args.name)
if (err != nil) != tt.wantErr {
t.Errorf("IsServiceAccountExist() error = %v, wantErr %v", err, tt.wantErr)
return
}
if got != tt.want {
t.Errorf("IsServiceAccountExist() got = %v, want %v", got, tt.want)
}
})
}
}
func TestWaitForServiceAccountSecretCreation(t *testing.T) {
sa := makeServiceAccount("test")
saVisitCount, secretVisitCount := 0, 0