Move join to its own pkg #1726

Jaanki · 2021-12-24T11:54:36Z

... and decouple cobra stuff

Signed-off-By: Janki Chhatbar [email protected]

Depends on #1724

submariner-bot · 2021-12-24T11:54:39Z

🤖 Created branch: z_pr1726/Jaanki/move_join
🚀 Full E2E won't run until the "ready-to-test" label is applied. I will add it automatically once the PR has 2 approvals, or you can add it manually.

pkg/join/join.go

Jaanki · 2021-12-24T14:04:40Z

This is still in WIP and not ready for review at all.A lot of work is still pending.

internal/exit/exit.go

tpantelis · 2022-01-04T14:14:42Z

#1724 is adding more code to join so we'll need to take that into account here.

github-actions · 2022-01-04T17:15:18Z

🎉 Great news! Looks like all the dependencies have been resolved:

~~Store the PSK in a secret, and mount it #1724~~

💡 To add or remove a dependency please update this issue/PR description.

Brought to you by Dependent Issues (:robot: ). Happy coding!

cmd/subctl/join.go

internal/cluster/cluster.go

internal/cluster/networks.go

pkg/deploy/submariner.go

pkg/discovery/globalnet/globalnet.go

pkg/join/join.go

internal/cluster/cluster.go

internal/cluster/networks.go

internal/nodes/nodes.go

Jaanki · 2022-01-13T10:26:27Z

cmd/subctl/join.go

@@ -69,6 +71,10 @@ var joinCmd = &cobra.Command{
 		clientProducer, err := client.NewProducerFromRestConfig(clientConfig)
 		exit.OnError(status.Error(err, "Error creating the client producer"))

+		networkDetails := getNetworkDetails(clientProducer, status)


I find 2 issues with this:

I strongly believe that cmd/subctl should just strictly be cobra stuff and nothing else.

we are expecting pkg/join to know *CIRD before hand. So join is not smart enough to figure out these details. Any third part code using join would also have to pre-determine these values and can't reply on join.

Where should survey-based code reside? The library itself can’t rely on that, since it can’t expect to have any way to interact with an end-user.

I find 2 issues with this:

I strongly believe that cmd/subctl should just strictly be cobra stuff and nothing else.

By cobra stuff I assume you mean processing arguments and prompting the user. That's what this code is doing. We call network.Discover here in order to determine if we should prompt the user for any CIDRs if one is not provided.

we are expecting pkg/join to know *CIRD before hand. So join is not smart enough to figure out these details. Any third part code using join would also have to pre-determine these values and can't reply on join.

They can by simply calling network.Discover. But they don't need to do this. If the CIDRs aren't provided then the operator controller will determine them via network.Discover.

survey- code should reside here. It should ask for CIDR here. join should network.Discover, get the CIDR and warn users if it is not same as user-specified adn update the ServiceCIDRAutoDetected accordingly. But I see you removed that field completely. So I guess, it won't make sense to just warn the users if the provided and detected values won't match.

survey- code should reside here. It should ask for CIDR here. join should network.Discover, get the CIDR and warn users if it is not same as user-specified

That's what we're doing here. It would be wasteful and unnecessary to perform network.Discover twice during subctl join. We don't currently have any other users of this API so I think it's reasonable at this point to assume the caller has done their due diligence wrt the CIDRs, if they provide them. We can always revisit based on feedback if we ever do have another user. Also the operator does this same check and logs a warning anyway.

adn update the ServiceCIDRAutoDetected accordingly. But I see you removed that field completely. So I guess, it won't make sense to just warn the users if the provided and detected values won't match.

ServiceCIDRAutoDetected was used to pass an empty CIDR to the operator if it was auto-detected and was really unnecessary.

cmd/subctl/join.go

internal/nodes/nodes.go

pkg/deploy/join.go

tpantelis · 2022-01-13T18:44:21Z

pkg/join/join.go

+	"k8s.io/client-go/kubernetes"
+)
+
+func SubmarinerCluster(brokerInfo *broker.Info, options *Options, clientProducer client.Producer,


Wondering if we should rename this function. The Submariner part seems redundant since we already know we're Submariner 😄 Some possibilities depending on perspective:

join.Cluster

join.ToBroker

join.ToClusterSet

I'm leaning towards 2. @Jaanki @skitt WDYT?

I am leaning towards 1. It joins a cluster (to a broker or to a clusterSet) or lets go overboard with a 4th option join.ClusterToBroker.

It depends on one's perspective and preference. join.ClusterToBroker would be the most complete. @skitt do you guys have any strong preference?

Remembering Java, I like 1, because it names the result we’re after; the caller shouldn’t care that joining a cluster means joining a broker. However since Go doesn’t have polymorphism, and we’re explicitly passing in a broker.Info, ClusterToBroker makes sense to me — if we end up having other variants which don’t use a broker, we’ll say ClusterToSomethingElse.

yeah makes sense. We'll go with ClusterToBroker. I think with just Cluster, it could be interpreted as "joining to a cluster" instead of "joining a cluster to something".

... and decouple cobra stuff Signed-off-By: Janki Chhatbar <[email protected]>

Signed-off-by: Tom Pantelis <[email protected]>

...instead of in join.SubmarinerCluster since it's obtained from user input. This also preserves the current behavior where we prompt the user for the cluster ID if the CLI argument is invalid. Subsequently we can then pass in a client.Producer rather than a restconfig.Producer to join.SubmarinerCluster improving reusability. Signed-off-by: Tom Pantelis <[email protected]>

...where it always checks and reports failed requirements but only fails the join if the ignoreRequirements flag is false. Also log the failures to the status reporter instead of fmt.Printf. Signed-off-by: Tom Pantelis <[email protected]>

The new implementation changed the behavior to first ask the user if they want to discover/autodetect the CIDR if it wasn't provided. If the user chooses no then it prompts for the CIDR. Otherwise if it's not autodetected, it fails. Changed it to preserve the existing behavior in the cmd front-end where if the CIDR is provided by the user, use it. Otherwise, if the CIDR wasn't autodetected, prompt the user for the CIDR. The library function, join.SubmarinerCluster, simply passes the CIDRs as is to the Submariner CR and assumes the caller has determined them appropriately or left one or more empty to let the operator controller discover it. Signed-off-by: Tom Pantelis <[email protected]>

...as advertised instead of whether or not there are any gateways. Signed-off-by: Tom Pantelis <[email protected]>

The join cmd front-end already handles most of the work by retrieving the worker nodes, prompting the user etc so it may as well do the last part (nodes.LabelAsGateway) instead of passing params to join.SubmarinerCluster to do it. A third-party client could also easily handle the labeling via the functions provided by the nodes pkg. Signed-off-by: Tom Pantelis <[email protected]>

...with deploy.Submariner as only a subset of WithJoinOptions fields apply. Signed-off-by: Tom Pantelis <[email protected]>

...with deploy.ServiceDiscovery as only a subset of WithJoinOptions fields apply. Signed-off-by: Tom Pantelis <[email protected]>

Signed-off-by: Tom Pantelis <[email protected]>

submariner-bot · 2022-01-18T12:27:03Z

🤖 Closed branches: [z_pr1726/Jaanki/move_join]

Jaanki requested review from Oats87, skitt, sridhargaddam and tpantelis as code owners December 24, 2021 11:54

Jaanki self-assigned this Dec 24, 2021

Jaanki marked this pull request as draft December 24, 2021 12:09

tpantelis requested changes Dec 24, 2021

View reviewed changes

pkg/join/join.go Outdated Show resolved Hide resolved

pkg/join/join.go Show resolved Hide resolved

pkg/join/join.go Outdated Show resolved Hide resolved

pkg/join/join.go Outdated Show resolved Hide resolved

pkg/join/join.go Show resolved Hide resolved

Jaanki changed the title ~~Move join to its own pkg~~ [WIP] Move join to its own pkg Dec 24, 2021

tpantelis mentioned this pull request Jan 3, 2022

Move join to new subctl implementation #1699

Closed

tpantelis reviewed Jan 4, 2022

View reviewed changes

internal/exit/exit.go Outdated Show resolved Hide resolved

Jaanki marked this pull request as ready for review January 4, 2022 04:36

Jaanki requested a review from mkolesnik as a code owner January 4, 2022 04:36

Jaanki changed the title ~~[WIP] Move join to its own pkg~~ Move join to its own pkg Jan 4, 2022

github-actions bot added the dependent label Jan 4, 2022

tpantelis mentioned this pull request Jan 4, 2022

Store the PSK in a secret, and mount it #1724

Merged

github-actions bot removed the dependent label Jan 4, 2022

tpantelis requested changes Jan 4, 2022

View reviewed changes

Jaanki marked this pull request as draft January 7, 2022 12:48

Jaanki marked this pull request as ready for review January 10, 2022 12:42

tpantelis requested changes Jan 11, 2022

View reviewed changes

tpantelis self-assigned this Jan 12, 2022

Jaanki commented Jan 13, 2022

View reviewed changes

cmd/subctl/join.go Show resolved Hide resolved

internal/nodes/nodes.go Outdated Show resolved Hide resolved

pkg/deploy/join.go Outdated Show resolved Hide resolved

pkg/deploy/join.go Outdated Show resolved Hide resolved

Jaanki requested a review from tpantelis January 13, 2022 11:00

tpantelis mentioned this pull request Jan 13, 2022

Deprecate colour codes, stop sending them to Submariner #1775

Merged

tpantelis reviewed Jan 13, 2022

View reviewed changes

tpantelis approved these changes Jan 17, 2022

View reviewed changes

Jaanki and others added 11 commits January 17, 2022 09:44

Move join to its own pkg

7b8ab2f

... and decouple cobra stuff Signed-off-By: Janki Chhatbar <[email protected]>

Move cluster.DetermineID to restconfig.Producer.GetClusterID

4fdb48e

Signed-off-by: Tom Pantelis <[email protected]>

Preserve check requirements behavior

b045039

...where it always checks and reports failed requirements but only fails the join if the ignoreRequirements flag is false. Also log the failures to the status reporter instead of fmt.Printf. Signed-off-by: Tom Pantelis <[email protected]>

Modify ListGateways to return the node names

0b9e81a

...as advertised instead of whether or not there are any gateways. Signed-off-by: Tom Pantelis <[email protected]>

Define SubmarinerOptions type in lieu of using WithJoinOptions

6ffc2a4

...with deploy.Submariner as only a subset of WithJoinOptions fields apply. Signed-off-by: Tom Pantelis <[email protected]>

Define ServiceDiscoveryOptions type in lieu of using WithJoinOptions

c49a9e2

...with deploy.ServiceDiscovery as only a subset of WithJoinOptions fields apply. Signed-off-by: Tom Pantelis <[email protected]>

Refactor deploy.WithJoinOptions to join.Options

6a2bd5f

Signed-off-by: Tom Pantelis <[email protected]>

Rename SubmarinerCluster to ClusterToBroker

e106821

Signed-off-by: Tom Pantelis <[email protected]>

dfarrell07 approved these changes Jan 17, 2022

View reviewed changes

submariner-bot added the ready-to-test When a PR is ready for full E2E testing label Jan 17, 2022

Merge branch 'devel' into move_join

bdb4e8a

skitt approved these changes Jan 18, 2022

View reviewed changes

Merge branch 'devel' into move_join

6f572c6

skitt enabled auto-merge (rebase) January 18, 2022 11:13

Merge branch 'devel' into move_join

6a431b8

skitt merged commit bce0cc7 into submariner-io:devel Jan 18, 2022

Jaanki mentioned this pull request Jan 26, 2022

subctl should be refactored to a library, and make subctl the CLI of that library #1329

Closed

48 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Move join to its own pkg #1726

Move join to its own pkg #1726

Jaanki commented Dec 24, 2021 •

edited by tpantelis

Loading

submariner-bot commented Dec 24, 2021

Jaanki commented Dec 24, 2021

tpantelis commented Jan 4, 2022

github-actions bot commented Jan 4, 2022

Jaanki Jan 13, 2022

skitt Jan 13, 2022

tpantelis Jan 13, 2022 •

edited

Loading

Jaanki Jan 17, 2022 •

edited

Loading

tpantelis Jan 17, 2022 •

edited

Loading

tpantelis Jan 13, 2022

Jaanki Jan 17, 2022

tpantelis Jan 17, 2022

skitt Jan 17, 2022

tpantelis Jan 17, 2022 •

edited

Loading

submariner-bot commented Jan 18, 2022

Move join to its own pkg #1726

Move join to its own pkg #1726

Conversation

Jaanki commented Dec 24, 2021 • edited by tpantelis Loading

submariner-bot commented Dec 24, 2021

Jaanki commented Dec 24, 2021

tpantelis commented Jan 4, 2022

github-actions bot commented Jan 4, 2022

Jaanki Jan 13, 2022

Choose a reason for hiding this comment

skitt Jan 13, 2022

Choose a reason for hiding this comment

tpantelis Jan 13, 2022 • edited Loading

Choose a reason for hiding this comment

Jaanki Jan 17, 2022 • edited Loading

Choose a reason for hiding this comment

tpantelis Jan 17, 2022 • edited Loading

Choose a reason for hiding this comment

tpantelis Jan 13, 2022

Choose a reason for hiding this comment

Jaanki Jan 17, 2022

Choose a reason for hiding this comment

tpantelis Jan 17, 2022

Choose a reason for hiding this comment

skitt Jan 17, 2022

Choose a reason for hiding this comment

tpantelis Jan 17, 2022 • edited Loading

Choose a reason for hiding this comment

submariner-bot commented Jan 18, 2022

Jaanki commented Dec 24, 2021 •

edited by tpantelis

Loading

tpantelis Jan 13, 2022 •

edited

Loading

Jaanki Jan 17, 2022 •

edited

Loading

tpantelis Jan 17, 2022 •

edited

Loading

tpantelis Jan 17, 2022 •

edited

Loading