[liqoctl] generate peering-user #2911

claudiolor · 2025-01-29T17:15:48Z

Description

This PR introduces a set of commands allowing to generate a user with the minimum permissions to create a peering from the cluster with the specified cluster ID.

To create an user with the minimum permissions to peer with the current cluster from a cluster with ID $CLUSTER_ID:

liqoctl generate peering-user --consumer-cluster-id $CLUSTER_ID

to remove all the permissions to the user:

liqoctl delete peering-user --consumer-cluster-id $CLUSTER_ID

adamjensenbot · 2025-01-29T17:16:03Z

Hi @claudiolor. Thanks for your PR!

I am @adamjensenbot.
You can interact with me issuing a slash command in the first line of a comment.
Currently, I understand the following commands:

/rebase: Rebase this PR onto the master branch (You can add the option test=true to launch the tests
when the rebase operation is completed)
/merge: Merge this PR into the master branch
/build Build Liqo components
/test Launch the E2E and Unit tests
/hold, /unhold Add/remove the hold label to prevent merging with /merge

Make sure this PR appears in the liqo changelog, adding one of the following labels:

feat: 🚀 New Feature
fix: 🐛 Bug Fix
refactor: 🧹 Code Refactoring
docs: 📝 Documentation
style: 💄 Code Style
perf: 🐎 Performance Improvement
test: ✅ Tests
chore: 🚚 Dependencies Management
build: 📦 Builds Management
ci: 👷 CI/CD
revert: ⏪ Reverts Previous Changes

claudiolor · 2025-02-04T17:32:36Z

/test

docs/usage/peer.md

This patch introduces a set of commands allowing to generate a user with the minimum permissions to create a peering from the cluster with the specified cluster ID

pkg/consts/authentication.go

fra98 · 2025-02-18T15:57:30Z

pkg/liqoctl/peer/handler.go

@@ -86,7 +86,7 @@ func (o *Options) RunPeer(ctx context.Context) error {
 	// Ensure networking
 	if !o.NetworkingDisabled {
 		if err := ensureNetworking(ctx, o); err != nil {
-			o.LocalFactory.PrinterGlobal.Error.Println("unable to ensure networking")
+			o.LocalFactory.PrinterGlobal.Error.Printfln("Unable to ensure networking: %v", err)


Can you do the same also for ensureAuthentication and ensureOffloading?

fra98 · 2025-02-18T16:03:36Z

pkg/liqoctl/unpeer/handler.go

-	if bidirectional && !o.KeepNamespaces {
-		err = fmt.Errorf("cannot unpeer bidirectional peering without keeping namespaces, please set the --keep-namespaces flag")
+	if bidirectional && o.DeleteNamespace {
+		err = fmt.Errorf("cannot delete the tenant namespace when a bidirectional is enabled, please remote the --delete-namespaces flag")


Suggested change

err = fmt.Errorf("cannot delete the tenant namespace when a bidirectional is enabled, please remote the --delete-namespaces flag")

err = fmt.Errorf("cannot delete the tenant namespace when a bidirectional is enabled, please remove the --delete-namespaces flag")

fra98 · 2025-02-18T16:08:27Z

pkg/utils/getters/k8sGetters.go

 	if err := cl.List(ctx, &secrets, &client.ListOptions{
+		Namespace: tenantNamespace,
 		LabelSelector: labels.SelectorFromSet(map[string]string{
 			consts.RemoteClusterID:     string(remoteClusterID),
 			consts.NonceSecretLabelKey: "true",


minor issue here but can you use the same method to pass the ListOptions in GetNonceSecretByClusterID and GetSignedNonceSecretByClusterID?

pkg/liqoctl/rest/peering-user/userfactory/cert.go

fra98 · 2025-02-18T16:49:16Z

pkg/liqoctl/rest/peering-user/userfactory/cert.go

+	return fmt.Sprintf("liqo-peer-user-%s", clusterID)
+}
+
+func generateKubeconfig(clusterID string, ca, cert, private []byte, apiAddr string) (string, error) {


We basically do the same here.
Maybe not in this PR, but for the future we can export that function in a util package and reuse the same function.

I also noticed that the other function accept a optional namespace argument to set default the scope of the requests. It may be useful to use it by defaulting to the tenant namespace to further indiciate that scope of the kubeconfig is limited to the tenant namespace.

fra98 · 2025-02-18T16:53:53Z

pkg/liqoctl/rest/peering-user/userfactory/roles.go

+		APIGroups: []string{"ipam.liqo.io"},
+		Resources: []string{"networks"},
+		Verbs:     []string{"get", "list"},
+	},
+	{
+		APIGroups: []string{"networking.liqo.io"},
+		Resources: []string{"configurations", "gatewayclients", "gatewayservers", "publickeies"},
+		Verbs:     []string{"get", "list"},
+	},
+	{
+		APIGroups: []string{"core.liqo.io"},
+		Resources: []string{"foreignclusters"},
+		Verbs:     []string{"get", "list"},
+	},
+	{
+		APIGroups: []string{"authentication.liqo.io"},
+		Resources: []string{"tenants"},
+		Verbs:     []string{"create", "list"},
+	},
+}


Can you use the exported costants in our API?
E.g.:

{ APIGroups: []string{liqov1beta1.GroupVersion.Group}, Resources: []string{liqov1beta1.ForeignClusterResource}, Verbs: []string{"get", "list"}, },

fra98 · 2025-02-18T16:58:18Z

pkg/liqoctl/rest/peering-user/userfactory/roles.go

+				APIGroups:     []string{"authentication.liqo.io"},
+				Resources:     []string{"tenants"},


fra98 · 2025-02-18T17:08:25Z

pkg/liqoctl/rest/peering-user/userfactory/roles.go

+}
+
+// RemovePermissions removes the permissions related to the user.
+func RemovePermissions(ctx context.Context, c client.Client, clusterID liqov1beta1.ClusterID) error {


Should we use client.IgnoreNotFound for the deletions? Could it be problematic when re-running this command if an error occured in the middle when run for the first time?

fra98 · 2025-02-18T17:09:13Z

pkg/liqoctl/rest/peering-user/userfactory/roles.go

+	roleBindingList := &rbacv1.RoleBindingList{}
+
+	if err := c.List(ctx, roleBindingList, &client.ListOptions{
+		LabelSelector: getUserLabelSelector(userName),


Suggested change

LabelSelector: getUserLabelSelector(userName),

LabelSelector: userLabelSelector,

pull-request-size bot added the size/XXL label Jan 29, 2025

github-actions bot added feat Adds a new feature to the codebase fix Fixes a bug in the codebase. style Changes related to code formatting, whitespace, or linting, without affecting functionality labels Jan 29, 2025

claudiolor force-pushed the clo/liqoctl-new-user branch from f19a67a to 3ea3943 Compare January 30, 2025 09:47

github-actions bot added docs Updates or adds documentation test Adds or updates tests for the codebase labels Jan 30, 2025

claudiolor force-pushed the clo/liqoctl-new-user branch 8 times, most recently from 0f0e212 to f203cff Compare January 31, 2025 10:09

adamjensenbot force-pushed the clo/liqoctl-new-user branch from 2be20b4 to e162a78 Compare February 4, 2025 08:26

claudiolor force-pushed the clo/liqoctl-new-user branch 2 times, most recently from 6bb3540 to 063dd70 Compare February 4, 2025 11:18

claudiolor force-pushed the clo/liqoctl-new-user branch from b3928aa to 064a423 Compare February 5, 2025 16:38

claudiolor marked this pull request as ready for review February 5, 2025 16:39

claudiolor force-pushed the clo/liqoctl-new-user branch 2 times, most recently from 91f6e27 to 90029fe Compare February 10, 2025 14:50

frisso reviewed Feb 10, 2025

View reviewed changes

docs/usage/peer.md Outdated Show resolved Hide resolved

claudiolor force-pushed the clo/liqoctl-new-user branch 4 times, most recently from eba2b29 to b33f8f5 Compare February 17, 2025 11:39

feat: liqoctl generate peering user

336e124

This patch introduces a set of commands allowing to generate a user with the minimum permissions to create a peering from the cluster with the specified cluster ID

claudiolor added 5 commits February 17, 2025 17:50

style: liqoctl logs on stderr

8c32fba

fix: liqoctl retrieve nonce only from tenant namespace

48e19ef

refactor(liqoctl)!:preserve tenant namespace after unpeer

8cb8eb1

docs: liqoctl generate/delete peering-user

ef8e184

test: liqoctl generate peering-user

e703e11

claudiolor force-pushed the clo/liqoctl-new-user branch from b33f8f5 to e703e11 Compare February 17, 2025 16:50

fra98 reviewed Feb 18, 2025

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[liqoctl] generate peering-user #2911

[liqoctl] generate peering-user #2911

claudiolor commented Jan 29, 2025

adamjensenbot commented Jan 29, 2025

claudiolor commented Feb 4, 2025

fra98 Feb 18, 2025

fra98 Feb 18, 2025

fra98 Feb 18, 2025

fra98 Feb 18, 2025

fra98 Feb 18, 2025

fra98 Feb 18, 2025

fra98 Feb 18, 2025

fra98 Feb 18, 2025

	err = fmt.Errorf("cannot delete the tenant namespace when a bidirectional is enabled, please remote the --delete-namespaces flag")
	err = fmt.Errorf("cannot delete the tenant namespace when a bidirectional is enabled, please remove the --delete-namespaces flag")

		APIGroups: []string{"authentication.liqo.io"},
		Resources: []string{"tenants"},

	LabelSelector: getUserLabelSelector(userName),
	LabelSelector: userLabelSelector,

[liqoctl] generate peering-user #2911

Are you sure you want to change the base?

[liqoctl] generate peering-user #2911

Conversation

claudiolor commented Jan 29, 2025

Description

adamjensenbot commented Jan 29, 2025

claudiolor commented Feb 4, 2025

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment