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

kudoinit refactoring #1491

Merged
merged 5 commits into from
May 5, 2020
Merged

kudoinit refactoring #1491

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
69dad38
Extracted kudoinit refactoring
ANeumann82 May 4, 2020
6114f6d
Merge branch 'master' into an/refactor-kudoinit
ANeumann82 May 4, 2020
83905e8
Small review requests
ANeumann82 May 4, 2020
8c55057
Fixed unit test
ANeumann82 May 4, 2020
6a8cf4e
Review requests
ANeumann82 May 4, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
30 changes: 15 additions & 15 deletions pkg/controller/instance/instance_controller_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -175,9 +175,9 @@ func Test_makePipes(t *testing.T) {
plan: &v1beta1.Plan{Strategy: "serial", Phases: []v1beta1.Phase{
{
Name: "phase", Strategy: "serial", Steps: []v1beta1.Step{
{
Name: "step", Tasks: []string{}},
}},
{
Name: "step", Tasks: []string{}},
}},
}},
tasks: []v1beta1.Task{},
emeta: meta,
Expand All @@ -189,9 +189,9 @@ func Test_makePipes(t *testing.T) {
plan: &v1beta1.Plan{Strategy: "serial", Phases: []v1beta1.Phase{
{
Name: "phase", Strategy: "serial", Steps: []v1beta1.Step{
{
Name: "step", Tasks: []string{"task"}},
}},
{
Name: "step", Tasks: []string{"task"}},
}},
}},
tasks: []v1beta1.Task{
{
Expand All @@ -211,9 +211,9 @@ func Test_makePipes(t *testing.T) {
plan: &v1beta1.Plan{Strategy: "serial", Phases: []v1beta1.Phase{
{
Name: "phase", Strategy: "serial", Steps: []v1beta1.Step{
{
Name: "step", Tasks: []string{"task"}},
}},
{
Name: "step", Tasks: []string{"task"}},
}},
}},
tasks: []v1beta1.Task{
{
Expand Down Expand Up @@ -242,9 +242,9 @@ func Test_makePipes(t *testing.T) {
plan: &v1beta1.Plan{Strategy: "serial", Phases: []v1beta1.Phase{
{
Name: "phase", Strategy: "serial", Steps: []v1beta1.Step{
{Name: "stepOne", Tasks: []string{"task-one"}},
{Name: "stepTwo", Tasks: []string{"task-two"}},
}},
{Name: "stepOne", Tasks: []string{"task-one"}},
{Name: "stepTwo", Tasks: []string{"task-two"}},
}},
}},
tasks: []v1beta1.Task{
{
Expand Down Expand Up @@ -292,9 +292,9 @@ func Test_makePipes(t *testing.T) {
plan: &v1beta1.Plan{Strategy: "serial", Phases: []v1beta1.Phase{
{
Name: "phase", Strategy: "serial", Steps: []v1beta1.Step{
{
Name: "step", Tasks: []string{"task"}},
}},
{
Name: "step", Tasks: []string{"task"}},
}},
}},
tasks: []v1beta1.Task{
{
Expand Down
23 changes: 23 additions & 0 deletions pkg/kudoctl/cmd/init.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,8 @@ import (
"io"
"strings"

"github.com/kudobuilder/kudo/pkg/kudoctl/verifier"

"github.com/spf13/afero"
"github.com/spf13/cobra"
flag "github.com/spf13/pflag"
Expand Down Expand Up @@ -179,6 +181,13 @@ func (initCmd *initCmd) run() error {
}
initCmd.client = client
}
ok, err := initCmd.preInstallVerify(opts)
if err != nil {
return err
}
if !ok {
return fmt.Errorf("failed to verify installation requirements")
}

if err := setup.Install(initCmd.client, opts, initCmd.crdOnly); err != nil {
return clog.Errorf("error installing: %s", err)
Expand All @@ -196,6 +205,20 @@ func (initCmd *initCmd) run() error {
return nil
}

// preInstallVerify runs the pre-installation verification and returns true if the installation can continue
func (initCmd *initCmd) preInstallVerify(opts kudoinit.Options) (bool, error) {
Copy link
Contributor

Choose a reason for hiding this comment

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

I like this 👍

result := verifier.NewResult()
if err := setup.PreInstallVerify(initCmd.client, opts, initCmd.crdOnly, &result); err != nil {
return false, err
}
result.PrintWarnings(initCmd.out)
if !result.IsValid() {
result.PrintErrors(initCmd.out)
return false, nil
}
return true, nil
}

func webhooksArray(webhooksAsStr string) []string {
if webhooksAsStr == "" {
return []string{}
Expand Down
213 changes: 108 additions & 105 deletions pkg/kudoctl/cmd/init_integration_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -71,6 +71,10 @@ func assertManifestFileMatch(t *testing.T, fileName string, expectedObject runti
assert.Equal(t, string(expectedContent), string(of), fmt.Sprintf("embedded file %s does not match the source, run 'make generate'", fileName))
}

func assertStringContains(t *testing.T, expected string, actual string) {
assert.True(t, strings.Contains(actual, expected), "Expected to find '%s' in '%s'", expected, actual)
}

func runtimeObjectAsBytes(o runtime.Object) ([]byte, error) {
bytes, err := yaml.Marshal(o)
if err != nil {
Expand Down Expand Up @@ -145,7 +149,8 @@ func TestIntegInitWithNameSpace(t *testing.T) {
// On first attempt, the namespace does not exist, so the error is expected.
err = cmd.run()
require.Error(t, err)
assert.Equal(t, "error installing: Namespace integration-test does not exist - KUDO expects that any namespace except the default kudo-system is created beforehand\n", err.Error())
assert.Equal(t, "failed to verify installation requirements", err.Error())
assertStringContains(t, "Namespace integration-test does not exist - KUDO expects that any namespace except the default kudo-system is created beforehand", buf.String())

// Then we manually create the namespace.
ns := testutils.NewResource("v1", "Namespace", namespace, "")
Expand Down Expand Up @@ -187,111 +192,105 @@ func TestIntegInitWithNameSpace(t *testing.T) {
4. Run Init command with a serviceAccount that does have cluster-admin role, but not in the expected namespace.
5. Run Init command with a serviceAccount that is present in the cluster and also has cluster-admin role.
*/

func TestIntegInitWithServiceAccount(t *testing.T) {
namespace := "sa-integration-test"
serviceAccount := "sa-integration"
// Kubernetes client caches the types, se we need to re-initialize it.
testClient, err := testutils.NewRetryClient(testenv.Config, client.Options{
Scheme: testutils.Scheme(),
})
assert.Nil(t, err)
kclient := getKubeClient(t)

instance := testutils.NewResource("kudo.dev/v1beta1", "Instance", "zk", "ns")
// Verify that we cannot create the instance, because the test environment is empty.
assert.IsType(t, &meta.NoKindMatchError{}, testClient.Create(context.TODO(), instance))

// Install all of the CRDs.
crds := crd.NewInitializer().Resources()
defer deleteInitObjects(testClient)

var buf bytes.Buffer
cmd := &initCmd{
out: &buf,
fs: afero.NewMemMapFs(),
client: kclient,
ns: namespace,
serviceAccount: "test-account",
func TestInitWithServiceAccount(t *testing.T) {
tests := []struct {
name string
serviceAccount string
roleBindingRole string
roleBindingNs string
errMessageContains string
}{
{name: "service account not present", serviceAccount: "", errMessageContains: "Service Account test-account does not exists - KUDO expects the serviceAccount to be present in the namespace sa-integration-test-0"},
{name: "service account has no rb", serviceAccount: "test-account", errMessageContains: "Service Account test-account does not have cluster-admin role - KUDO expects the serviceAccount passed to be in the namespace sa-integration-test-1 and to have cluster-admin role"},
{name: "rb has no cluster-admin role", serviceAccount: "test-account", roleBindingRole: "not-admin", errMessageContains: "Service Account test-account does not have cluster-admin role - KUDO expects the serviceAccount passed to be in the namespace sa-integration-test-2 and to have cluster-admin role"},
{name: "rb has different ns", serviceAccount: "test-account", roleBindingRole: "not-admin", roleBindingNs: "otherns", errMessageContains: "Service Account test-account does not have cluster-admin role - KUDO expects the serviceAccount passed to be in the namespace sa-integration-test-3 and to have cluster-admin role"},
{name: "rb has admin in different ns", serviceAccount: "test-account", roleBindingRole: "cluster-admin", roleBindingNs: "otherns", errMessageContains: "Service Account test-account does not have cluster-admin role - KUDO expects the serviceAccount passed to be in the namespace sa-integration-test-4 and to have cluster-admin role"},
{name: "rb has cluster-admin role", serviceAccount: "test-account", roleBindingRole: "cluster-admin", errMessageContains: ""},
}

// Manually create the namespace and the serviceAccount to be used later
ns := testutils.NewResource("v1", "Namespace", namespace, "")
assert.NoError(t, testClient.Create(context.TODO(), ns))
defer testClient.Delete(context.TODO(), ns)
sa := testutils.NewResource("v1", "ServiceAccount", serviceAccount, namespace)
assert.NoError(t, testClient.Create(context.TODO(), sa))
defer testClient.Delete(context.TODO(), sa)

// Test Case 1, the serviceAccount does not exist, expect serviceAccount not exists error
err = cmd.run()
require.Error(t, err)
assert.Equal(t, "error installing: Service Account test-account does not exists - KUDO expects the serviceAccount to be present in the namespace sa-integration-test\n", err.Error())

// Create the serviceAccount, in the default namespace.
ns2 := testutils.NewResource("v1", "Namespace", "test-ns", "")
assert.NoError(t, testClient.Create(context.TODO(), ns2))
defer testClient.Delete(context.TODO(), ns2)
sa2 := testutils.NewResource("v1", "ServiceAccount", "sa-nonadmin", "test-ns")
assert.NoError(t, testClient.Create(context.TODO(), sa2))
defer testClient.Delete(context.TODO(), sa2)

// Test Case 2, the serviceAccount exists, but does not part of clusterrolebindings
cmd.serviceAccount = "sa-nonadmin"
cmd.ns = "test-ns"
err = cmd.run()
require.Error(t, err)
assert.Equal(t, "error installing: Service Account sa-nonadmin does not have cluster-admin role - KUDO expects the serviceAccount passed to be in the namespace test-ns and to have cluster-admin role\n", err.Error())

// Test case 3: Run Init command with a serviceAccount that does not have cluster-admin role.
cmd.serviceAccount = serviceAccount
cmd.ns = namespace
crb := testutils.NewClusterRoleBinding("rbac.authorization.k8s.io/v1", "ClusterRoleBinding", "kudo-test1", "test-ns", serviceAccount, "cluster-temp")
assert.NoError(t, testClient.Create(context.TODO(), crb))
defer testClient.Delete(context.TODO(), crb)

err = cmd.run()
require.Error(t, err)
assert.Equal(t, "error installing: Service Account sa-integration does not have cluster-admin role - KUDO expects the serviceAccount passed to be in the namespace sa-integration-test and to have cluster-admin role\n", err.Error())

// Test case 4: Run Init command with a serviceAccount that does not have cluster-admin role.
crb2 := testutils.NewClusterRoleBinding("rbac.authorization.k8s.io/v1", "ClusterRoleBinding", "kudo-test2", namespace, serviceAccount, "cluster-temp")
assert.NoError(t, testClient.Create(context.TODO(), crb2))
defer testClient.Delete(context.TODO(), crb2)

err = cmd.run()
require.Error(t, err)
assert.Equal(t, "error installing: Service Account sa-integration does not have cluster-admin role - KUDO expects the serviceAccount passed to be in the namespace sa-integration-test and to have cluster-admin role\n", err.Error())

// Test case 5: Run Init command with a serviceAccount that is present in the cluster and also has cluster-admin role.
crb3 := testutils.NewClusterRoleBinding("rbac.authorization.k8s.io/v1", "ClusterRoleBinding", "kudo-clusterrole-binding", namespace, serviceAccount, "cluster-admin")
assert.NoError(t, testClient.Create(context.TODO(), crb3))
defer testClient.Delete(context.TODO(), crb3)

err = cmd.run()
assert.NoError(t, err)

// WaitForCRDs to be created... the init cmd did NOT wait
assert.Nil(t, testutils.WaitForCRDs(testenv.DiscoveryClient, crds))

// Kubernetes client caches the types, so we need to re-initialize it.
testClient, err = testutils.NewRetryClient(testenv.Config, client.Options{
Scheme: testutils.Scheme(),
})
assert.Nil(t, err)
kclient = getKubeClient(t)

// make sure that the controller lives in the correct namespace
statefulsets, err := kclient.KubeClient.AppsV1().StatefulSets(namespace).List(metav1.ListOptions{})
assert.Nil(t, err)

kudoControllerFound := false
for _, ss := range statefulsets.Items {
if ss.Name == kudoinit.DefaultManagerName {
kudoControllerFound = true
}
namespaceBase := "sa-integration-test"

for idx, tt := range tests {
t.Run(tt.name, func(t *testing.T) {
namespace := fmt.Sprintf("%s-%d", namespaceBase, idx)

// Kubernetes client caches the types, se we need to re-initialize it.
testClient, err := testutils.NewRetryClient(testenv.Config, client.Options{
Scheme: testutils.Scheme(),
})
assert.Nil(t, err)
kclient := getKubeClient(t)

instance := testutils.NewResource("kudo.dev/v1beta1", "Instance", "zk", "ns")
// Verify that we cannot create the instance, because the test environment is empty.
assert.IsType(t, &meta.NoKindMatchError{}, testClient.Create(context.TODO(), instance))

// Install all of the CRDs.
crds := crd.NewInitializer().Resources()
defer deleteInitObjects(testClient)

var buf bytes.Buffer
cmd := &initCmd{
out: &buf,
fs: afero.NewMemMapFs(),
client: kclient,
ns: namespace,
serviceAccount: "test-account",
}

ns := testutils.NewResource("v1", "Namespace", namespace, "")
assert.NoError(t, testClient.Create(context.TODO(), ns))
defer testClient.Delete(context.TODO(), ns)

if tt.serviceAccount != "" {
sa2 := testutils.NewResource("v1", "ServiceAccount", tt.serviceAccount, namespace)
assert.NoError(t, testClient.Create(context.TODO(), sa2))
defer testClient.Delete(context.TODO(), sa2)
}

if tt.roleBindingRole != "" {
rbNamespace := tt.roleBindingNs
if rbNamespace == "" {
rbNamespace = namespace
}
crb := testutils.NewClusterRoleBinding("rbac.authorization.k8s.io/v1", "ClusterRoleBinding", "kudo-clusterrole-binding", rbNamespace, tt.serviceAccount, tt.roleBindingRole)
assert.NoError(t, testClient.Create(context.TODO(), crb))
defer testClient.Delete(context.TODO(), crb)
}

err = cmd.run()

if tt.errMessageContains != "" {
require.Error(t, err)
assert.Equal(t, "failed to verify installation requirements", err.Error())
assertStringContains(t, tt.errMessageContains, buf.String())
} else {
assert.NoError(t, err)

// WaitForCRDs to be created... the init cmd did NOT wait
assert.Nil(t, testutils.WaitForCRDs(testenv.DiscoveryClient, crds))

// Kubernetes client caches the types, so we need to re-initialize it.
testClient, err = testutils.NewRetryClient(testenv.Config, client.Options{
Scheme: testutils.Scheme(),
})
assert.Nil(t, err)
kclient = getKubeClient(t)

// make sure that the controller lives in the correct namespace
statefulsets, err := kclient.KubeClient.AppsV1().StatefulSets(namespace).List(metav1.ListOptions{})
assert.Nil(t, err)

kudoControllerFound := false
for _, ss := range statefulsets.Items {
if ss.Name == kudoinit.DefaultManagerName {
kudoControllerFound = true
}
}
assert.True(t, kudoControllerFound, fmt.Sprintf("No kudo-controller-manager statefulset found in namespace %s", namespace))
}
})
}
assert.True(t, kudoControllerFound, fmt.Sprintf("No kudo-controller-manager statefulset found in namespace %s", namespace))
}

func TestNoErrorOnReInit(t *testing.T) {
Expand Down Expand Up @@ -340,10 +339,14 @@ func TestNoErrorOnReInit(t *testing.T) {
}

func deleteInitObjects(client *testutils.RetryClient) {
opts := kudoinit.NewOptions("", "", "", []string{}, false)

crds := crd.NewInitializer()
prereqs := prereq.NewInitializer(kudoinit.NewOptions("", "", "", []string{}, false))

deleteCRDs(crds.Resources(), client)
deletePrereq(prereqs.Resources(), client)
deletePrereq(prereq.NewNamespaceInitializer(opts).Resources(), client)
deletePrereq(prereq.NewServiceAccountInitializer(opts).Resources(), client)
deletePrereq(prereq.NewWebHookInitializer(opts).Resources(), client)
}

func deleteCRDs(crds []runtime.Object, client *testutils.RetryClient) {
Expand Down
Loading