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

fix: unregister-member deletes secret & has needed permissions #99

Merged
merged 3 commits into from
Jan 10, 2025
Merged

fix: unregister-member deletes secret & has needed permissions #99

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
90fc236
fix: unregister-member deletes secret & has needed permissions
MatousJobanek Jan 10, 2025
8abd609
add secret to other test cases
MatousJobanek Jan 10, 2025
325af8b
meesage when secret not present
MatousJobanek Jan 10, 2025
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
22 changes: 21 additions & 1 deletion pkg/cmd/adm/unregister_member.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,8 @@
"github.com/kubesaw/ksctl/pkg/configuration"
clicontext "github.com/kubesaw/ksctl/pkg/context"
"github.com/kubesaw/ksctl/pkg/ioutils"
v1 "k8s.io/api/core/v1"
"k8s.io/apimachinery/pkg/api/errors"

"github.com/spf13/cobra"
"k8s.io/apimachinery/pkg/types"
Expand Down Expand Up @@ -59,10 +61,28 @@
return nil
}

tcSecret := &v1.Secret{}
if err := hostClusterClient.Get(context.TODO(), types.NamespacedName{Namespace: hostClusterConfig.OperatorNamespace, Name: toolchainCluster.Spec.SecretRef.Name}, tcSecret); err != nil {
if !errors.IsNotFound(err) {
return err
}

Check warning on line 68 in pkg/cmd/adm/unregister_member.go

View check run for this annotation

Codecov / codecov/patch

pkg/cmd/adm/unregister_member.go#L67-L68 

Added lines #L67 - L68 were not covered by tests
ctx.Printlnf("\nThe referenced ToolchainCluster secret %s, is not present.", toolchainCluster.Spec.SecretRef.Name)
} else {
ctx.Printlnf("\nDeleting the ToolchainCluster secret %s...", toolchainCluster.Spec.SecretRef.Name)
if err := hostClusterClient.Delete(context.TODO(), tcSecret); err != nil {
return err
}

Check warning on line 74 in pkg/cmd/adm/unregister_member.go

View check run for this annotation

Codecov / codecov/patch

pkg/cmd/adm/unregister_member.go#L73-L74 

Added lines #L73 - L74 were not covered by tests
ctx.Printlnf("The ToolchainCluster secret %s has been deleted", toolchainCluster.Spec.SecretRef.Name)
}

ctx.Printlnf("\nDeleting the ToolchainCluster CR %s...", toolchainCluster.Name)
if err := hostClusterClient.Delete(context.TODO(), toolchainCluster); err != nil {
return err
}
ctx.Printlnf("\nThe deletion of the Toolchain member cluster from the Host cluster has been triggered")
ctx.Printlnf("The ToolchainCluster CR %s has been deleted", toolchainCluster.Name)

ctx.Printlnf("\nThe deletion of the Member cluster from the Host cluster has been finished.")

ctx.Printlnf("\nRestarting the Host operator components to reload the current setup...")
return restart(ctx, "host", getConfigFlagsAndClient)
}
74 changes: 61 additions & 13 deletions pkg/cmd/adm/unregister_member_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,17 +4,25 @@ import (
"fmt"
"testing"

toolchainv1alpha1 "github.com/codeready-toolchain/api/api/v1alpha1"
"github.com/codeready-toolchain/toolchain-common/pkg/test"
clicontext "github.com/kubesaw/ksctl/pkg/context"
. "github.com/kubesaw/ksctl/pkg/test"
"github.com/stretchr/testify/assert"
"github.com/stretchr/testify/require"
v1 "k8s.io/api/core/v1"
metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
"sigs.k8s.io/controller-runtime/pkg/client"
)

func TestUnregisterMemberWhenAnswerIsY(t *testing.T) {
// given
toolchainCluster := NewToolchainCluster(ToolchainClusterName("member-cool-server.com"))
noiseToolchainCluster := NewToolchainCluster(ToolchainClusterName("noise"))
secret := newSecret(toolchainCluster)
noiseSecret := newSecret(noiseToolchainCluster)

newClient, fakeClient := NewFakeClients(t, toolchainCluster)
newClient, fakeClient := NewFakeClients(t, noiseToolchainCluster, toolchainCluster, secret, noiseSecret)

SetFileConfig(t, Host(), Member())
term := NewFakeTerminalWithResponse("y")
Expand All @@ -28,18 +36,50 @@ func TestUnregisterMemberWhenAnswerIsY(t *testing.T) {
// then
require.NoError(t, err)
AssertToolchainClusterDoesNotExist(t, fakeClient, toolchainCluster)
AssertObjectExists(t, fakeClient, client.ObjectKeyFromObject(noiseToolchainCluster), &toolchainv1alpha1.ToolchainCluster{})
AssertObjectExists(t, fakeClient, client.ObjectKeyFromObject(noiseSecret), &v1.Secret{})
assert.Contains(t, term.Output(), "!!! DANGER ZONE !!!")
assert.NotContains(t, term.Output(), "THIS COMMAND WILL CAUSE UNREGISTER MEMBER CLUSTER FORM HOST CLUSTER. MAKE SURE THERE IS NO USERS LEFT IN THE MEMBER CLUSTER BEFORE UNREGISTERING IT")
assert.Contains(t, term.Output(), "Delete Member cluster stated above from the Host cluster?")
assert.Contains(t, term.Output(), "The deletion of the Toolchain member cluster from the Host cluster has been triggered")
assert.Contains(t, term.Output(), "The deletion of the Member cluster from the Host cluster has been finished.")
assert.NotContains(t, term.Output(), "cool-token")
}

func TestUnregisterMemberWhenRestartError(t *testing.T) {
func TestUnregisterMemberWhenSecretIsMissing(t *testing.T) {
// given
toolchainCluster := NewToolchainCluster(ToolchainClusterName("member-cool-server.com"))
newClient, fakeClient := NewFakeClients(t, toolchainCluster)

newClient, _ := NewFakeClients(t, toolchainCluster)
SetFileConfig(t, Host(), Member())
term := NewFakeTerminalWithResponse("y")
ctx := clicontext.NewCommandContext(term, newClient)

// when
err := UnregisterMemberCluster(ctx, "member1", func(_ *clicontext.CommandContext, _ string, _ ConfigFlagsAndClientGetterFunc) error {
return nil
})

// then
require.NoError(t, err)
AssertToolchainClusterDoesNotExist(t, fakeClient, toolchainCluster)
assert.Contains(t, term.Output(), "The referenced ToolchainCluster secret member-cool-server.com-secret, is not present.")
assert.NotContains(t, term.Output(), "cool-token")
}

func newSecret(tc *toolchainv1alpha1.ToolchainCluster) *v1.Secret {
return &v1.Secret{
ObjectMeta: metav1.ObjectMeta{
Name: tc.Spec.SecretRef.Name,
Namespace: test.HostOperatorNs,
},
}
}

func TestUnregisterMemberWhenRestartError(t *testing.T) {
// given
toolchainCluster := NewToolchainCluster(ToolchainClusterName("member-cool-server.com"))
secret := newSecret(toolchainCluster)
newClient, _ := NewFakeClients(t, toolchainCluster, secret)

SetFileConfig(t, Host(), Member())
term := NewFakeTerminalWithResponse("y")
Expand All @@ -57,8 +97,8 @@ func TestUnregisterMemberWhenRestartError(t *testing.T) {
func TestUnregisterMemberCallsRestart(t *testing.T) {
// given
toolchainCluster := NewToolchainCluster(ToolchainClusterName("member-cool-server.com"))

newClient, _ := NewFakeClients(t, toolchainCluster)
secret := newSecret(toolchainCluster)
newClient, _ := NewFakeClients(t, toolchainCluster, secret)

SetFileConfig(t, Host(), Member())
term := NewFakeTerminalWithResponse("y")
Expand All @@ -78,7 +118,8 @@ func TestUnregisterMemberCallsRestart(t *testing.T) {
func TestUnregisterMemberWhenAnswerIsN(t *testing.T) {
// given
toolchainCluster := NewToolchainCluster(ToolchainClusterName("member-cool-server.com"))
newClient, fakeClient := NewFakeClients(t, toolchainCluster)
secret := newSecret(toolchainCluster)
newClient, fakeClient := NewFakeClients(t, toolchainCluster, secret)
SetFileConfig(t, Host(), Member())
term := NewFakeTerminalWithResponse("n")
ctx := clicontext.NewCommandContext(term, newClient)
Expand All @@ -91,17 +132,19 @@ func TestUnregisterMemberWhenAnswerIsN(t *testing.T) {
// then
require.NoError(t, err)
AssertToolchainClusterSpec(t, fakeClient, toolchainCluster)
AssertObjectExists(t, fakeClient, client.ObjectKeyFromObject(secret), &v1.Secret{})
assert.Contains(t, term.Output(), "!!! DANGER ZONE !!!")
assert.NotContains(t, term.Output(), "THIS COMMAND WILL CAUSE UNREGISTER MEMBER CLUSTER FORM HOST CLUSTER. MAKE SURE THERE IS NO USERS LEFT IN THE MEMBER CLUSTER BEFORE UNREGISTERING IT")
assert.Contains(t, term.Output(), "Delete Member cluster stated above from the Host cluster?")
assert.NotContains(t, term.Output(), "The deletion of the Toolchain member cluster from the Host cluster has been triggered")
assert.NotContains(t, term.Output(), "The deletion of the Member cluster from the Host cluster has been finished.")
assert.NotContains(t, term.Output(), "cool-token")
}

func TestUnregisterMemberWhenNotFound(t *testing.T) {
// given
toolchainCluster := NewToolchainCluster(ToolchainClusterName("another-cool-server.com"))
newClient, fakeClient := NewFakeClients(t, toolchainCluster)
secret := newSecret(toolchainCluster)
newClient, fakeClient := NewFakeClients(t, toolchainCluster, secret)
SetFileConfig(t, Host(), Member())
term := NewFakeTerminalWithResponse("n")
ctx := clicontext.NewCommandContext(term, newClient)
Expand All @@ -114,17 +157,19 @@ func TestUnregisterMemberWhenNotFound(t *testing.T) {
// then
require.EqualError(t, err, "toolchainclusters.toolchain.dev.openshift.com \"member-cool-server.com\" not found")
AssertToolchainClusterSpec(t, fakeClient, toolchainCluster)
AssertObjectExists(t, fakeClient, client.ObjectKeyFromObject(secret), &v1.Secret{})
assert.NotContains(t, term.Output(), "!!! DANGER ZONE !!!")
assert.NotContains(t, term.Output(), "THIS COMMAND WILL CAUSE UNREGISTER MEMBER CLUSTER FORM HOST CLUSTER. MAKE SURE THERE IS NO USERS LEFT IN THE MEMBER CLUSTER BEFORE UNREGISTERING IT")
assert.NotContains(t, term.Output(), "Delete Member cluster stated above from the Host cluster?")
assert.NotContains(t, term.Output(), "The deletion of the Toolchain member cluster from the Host cluster has been triggered")
assert.NotContains(t, term.Output(), "The deletion of the Member cluster from the Host cluster has been finished.")
assert.NotContains(t, term.Output(), "cool-token")
}

func TestUnregisterMemberWhenUnknownClusterName(t *testing.T) {
// given
toolchainCluster := NewToolchainCluster(ToolchainClusterName("member-cool-server.com"))
newClient, fakeClient := NewFakeClients(t, toolchainCluster)
secret := newSecret(toolchainCluster)
newClient, fakeClient := NewFakeClients(t, toolchainCluster, secret)
SetFileConfig(t, Host(), Member())
term := NewFakeTerminalWithResponse("n")
ctx := clicontext.NewCommandContext(term, newClient)
Expand All @@ -138,10 +183,11 @@ func TestUnregisterMemberWhenUnknownClusterName(t *testing.T) {
require.Error(t, err)
assert.Contains(t, err.Error(), "the provided cluster-name 'some' is not present in your ksctl.yaml file.")
AssertToolchainClusterSpec(t, fakeClient, toolchainCluster)
AssertObjectExists(t, fakeClient, client.ObjectKeyFromObject(secret), &v1.Secret{})
assert.NotContains(t, term.Output(), "!!! DANGER ZONE !!!")
assert.NotContains(t, term.Output(), "THIS COMMAND WILL CAUSE UNREGISTER MEMBER CLUSTER FORM HOST CLUSTER. MAKE SURE THERE IS NO USERS LEFT IN THE MEMBER CLUSTER BEFORE UNREGISTERING IT")
assert.NotContains(t, term.Output(), "Delete Member cluster stated above from the Host cluster?")
assert.NotContains(t, term.Output(), "The deletion of the Toolchain member cluster from the Host cluster has been triggered")
assert.NotContains(t, term.Output(), "The deletion of the Member cluster from the Host cluster has been finished.")
assert.NotContains(t, term.Output(), "cool-token")
}

Expand All @@ -150,7 +196,8 @@ func TestUnregisterMemberLacksPermissions(t *testing.T) {
SetFileConfig(t, Host(NoToken()), Member(NoToken()))

toolchainCluster := NewToolchainCluster(ToolchainClusterName("member-cool-server.com"))
newClient, fakeClient := NewFakeClients(t, toolchainCluster)
secret := newSecret(toolchainCluster)
newClient, fakeClient := NewFakeClients(t, toolchainCluster, secret)
term := NewFakeTerminal()
ctx := clicontext.NewCommandContext(term, newClient)

Expand All @@ -162,6 +209,7 @@ func TestUnregisterMemberLacksPermissions(t *testing.T) {
// then
require.EqualError(t, err, "ksctl command failed: the token in your ksctl.yaml file is missing")
AssertToolchainClusterSpec(t, fakeClient, toolchainCluster)
AssertObjectExists(t, fakeClient, client.ObjectKeyFromObject(secret), &v1.Secret{})
}

func mockRestart(ctx *clicontext.CommandContext, clusterName string, cfc ConfigFlagsAndClientGetterFunc) error {
Expand Down
10 changes: 10 additions & 0 deletions pkg/test/toolchaincluster.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,7 @@ import (

toolchainv1alpha1 "github.com/codeready-toolchain/api/api/v1alpha1"
"github.com/codeready-toolchain/toolchain-common/pkg/test"
v1 "k8s.io/api/core/v1"

"github.com/stretchr/testify/assert"
"github.com/stretchr/testify/require"
Expand All @@ -19,6 +20,11 @@ func NewToolchainCluster(modifiers ...ToolchainClusterModifier) *toolchainv1alph
Name: "member1",
Namespace: test.HostOperatorNs,
},
Spec: toolchainv1alpha1.ToolchainClusterSpec{
SecretRef: toolchainv1alpha1.LocalSecretReference{
Name: "member1-secret",
},
},
Status: toolchainv1alpha1.ToolchainClusterStatus{
APIEndpoint: "https://api.member.com:6443",
},
Expand All @@ -33,6 +39,9 @@ func AssertToolchainClusterDoesNotExist(t *testing.T, fakeClient *test.FakeClien
deletedCluster := &toolchainv1alpha1.ToolchainCluster{}
err := fakeClient.Get(context.TODO(), test.NamespacedName(toolchainCluster.Namespace, toolchainCluster.Name), deletedCluster)
require.True(t, apierrors.IsNotFound(err), "the ToolchainCluster should be deleted")

err = fakeClient.Get(context.TODO(), test.NamespacedName(toolchainCluster.Namespace, toolchainCluster.Spec.SecretRef.Name), &v1.Secret{})
require.True(t, apierrors.IsNotFound(err), "the ToolchainCluster secret should be deleted")
}

func AssertToolchainClusterSpec(t *testing.T, fakeClient *test.FakeClient, expectedToolchainCluster *toolchainv1alpha1.ToolchainCluster) {
Expand All @@ -47,5 +56,6 @@ type ToolchainClusterModifier func(toolchainCluster *toolchainv1alpha1.Toolchain
func ToolchainClusterName(name string) ToolchainClusterModifier {
return func(toolchainCluster *toolchainv1alpha1.ToolchainCluster) {
toolchainCluster.Name = name
toolchainCluster.Spec.SecretRef.Name = name + "-secret"
}
}
24 changes: 24 additions & 0 deletions resources/roles/host.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -263,3 +263,27 @@ objects:
- "list"
- "patch"
- "update"

- kind: Role
apiVersion: rbac.authorization.k8s.io/v1
metadata:
name: unregister-member
labels:
provider: ksctl
rules:
- apiGroups:
- toolchain.dev.openshift.com
resources:
- "toolchainclusters"
verbs:
- "get"
- "list"
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 know the list is not needed, but it's technically the same as get so it's fine to keep it there

- "delete"
- apiGroups:
- ""
resources:
- "secrets"
verbs:
- "get"
- "list"
- "delete"
Loading