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.

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

Add antctl commands to set up and delete Multi-cluster ClusterSet #3992

Merged
merged 1 commit into from
Aug 10, 2022

Conversation

hjiajing
Copy link
Contributor

@hjiajing hjiajing commented Jul 12, 2022

Add 4 new commands for the users to set up and delete ClusterSets. And
delete some old commands.

  • antctl mc init: to set up a ClusterSet in a leader cluster.
  • antctl mc join: to join a ClusterSet in a member cluster.
  • antctl mc destroy: to delete a ClusterSet in a leader cluster.
  • antctl mc leave: to leave a ClusterSet in a member cluster.

@codecov
Copy link

codecov bot commented Jul 12, 2022

Codecov Report

Merging #3992 (0c7a042) into main (2bf6a9a) will increase coverage by 7.11%.
The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##             main    #3992      +/-   ##
==========================================
+ Coverage   61.19%   68.31%   +7.11%     
==========================================
  Files         294      310      +16     
  Lines       44262    45539    +1277     
==========================================
+ Hits        27088    31109    +4021     
+ Misses      14885    12015    -2870     
- Partials     2289     2415     +126     
Flag Coverage Δ
e2e-tests 41.74% <100.00%> (?)
integration-tests 35.37% <100.00%> (?)
kind-e2e-tests 50.57% <ø> (+6.41%) ⬆️
unit-tests 44.25% <0.00%> (+0.03%) ⬆️
Impacted Files Coverage Δ
pkg/antctl/antctl.go 50.00% <ø> (ø)
...lers/multicluster/commonarea/remote_common_area.go 73.06% <100.00%> (+45.30%) ⬆️
pkg/agent/flowexporter/exporter/certificate.go 27.77% <0.00%> (-22.23%) ⬇️
...agent/flowexporter/connections/deny_connections.go 65.59% <0.00%> (-13.98%) ⬇️
...kg/controller/networkpolicy/antreanetworkpolicy.go 74.75% <0.00%> (-12.43%) ⬇️
pkg/controller/networkpolicy/status_controller.go 65.10% <0.00%> (-3.45%) ⬇️
pkg/ovs/openflow/ofctrl_packetin.go 66.66% <0.00%> (-2.96%) ⬇️
...ntroller/networkpolicy/networkpolicy_controller.go 79.43% <0.00%> (-2.90%) ⬇️
pkg/controller/ipam/antrea_ipam_controller.go 76.25% <0.00%> (-2.79%) ⬇️
pkg/controller/externalippool/controller.go 83.03% <0.00%> (-1.79%) ⬇️
... and 128 more

@luolanzone luolanzone added the area/multi-cluster Issues or PRs related to multi cluster. label Jul 13, 2022
@luolanzone luolanzone added this to the Antrea v1.8 release milestone Jul 13, 2022
pkg/antctl/raw/multicluster/join/join.go Outdated Show resolved Hide resolved
--namespace=<NAMESPACE> \
--cluster-id=<MEMBER_CLUSTER_ID>

# Join the ClusterSet from a member cluster with config file. If you use both command line options and config file,
Copy link
Contributor

Choose a reason for hiding this comment

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

Please update the doc for antctl as well.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sure. I'll update the doc for antctl after finishing the commands JIanjun mentioned in another comment.

pkg/antctl/raw/multicluster/join/join.go Outdated Show resolved Hide resolved
pkg/antctl/raw/multicluster/join/join.go Outdated Show resolved Hide resolved
pkg/antctl/raw/multicluster/join/join.go Outdated Show resolved Hide resolved
func waitForLeaderClusterSetReady(client client.Client, name string, namespace string, memberClusterID string) error {
return wait.PollImmediate(
1*time.Second,
3*time.Minute,
Copy link
Contributor

Choose a reason for hiding this comment

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

Is this too long?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Maybe we can add a timeout argument? I test on my PC sometimes it just takes several seconds, sometimes more than 1 mins.

pkg/antctl/raw/multicluster/join/join.go Outdated Show resolved Hide resolved
pkg/antctl/raw/multicluster/join/join.go Outdated Show resolved Hide resolved
pkg/antctl/raw/multicluster/join/join.go Outdated Show resolved Hide resolved
pkg/antctl/raw/multicluster/join/join.go Outdated Show resolved Hide resolved
@luolanzone
Copy link
Contributor

@hjiajing Could you update the PR summary to link it with issue #3921. You can refer to the link here to understand how to link them.

@hjiajing
Copy link
Contributor Author

@luolanzone Sure. Thanks for reminding me.

@hjiajing hjiajing linked an issue Jul 14, 2022 that may be closed by this pull request
@lgtm-com
Copy link

lgtm-com bot commented Jul 15, 2022

This pull request introduces 2 alerts when merging 90b874a into 4b788e7 - view on LGTM.com

new alerts:

  • 2 for Unreachable statement

@jianjuns jianjuns changed the title Add antctl join command for member cluster to join the ClusterSet Add antctl join command for member cluster to join ClusterSet Jul 16, 2022
@@ -49,6 +50,8 @@ var DeployCmd = &cobra.Command{
Short: "Deploy Antrea Multi-cluster Controller to a leader or member cluster",
}

var JoinCmd = join.NewJoinCommand()
Copy link
Contributor

Choose a reason for hiding this comment

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

Besides join for member clusters, would you add a command for the leader or change "create clusterset" too? Pasted below what we discussed in #3921 :

So, we just need two commands:

  1. "deploy" to deploy MC controllers;
  2. "create clusterset" (or "init" in leader and "join" in member as kubeadmin) to create/join a clusterset. For the leader, the command can generate a shared SA/token for member to join the clusterset.

Copy link
Contributor

Choose a reason for hiding this comment

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

I meant we either:

  1. change the current "create clusterset" command to create a clusterset & a default SA/taken in the leader; and create a clusterset & join it in a member
  2. or, add an "init" command to create a clusterset & a default SA/taken in the leader; and a "join" command to create a clusterset & join it in a member

But if we go 2), how should a leader/member leave a ClsuterSet? Would you add anther "destroy" command for leader and a "leave" command for member?

In any case, we should remove the commands that are no longer valid, like "create claims", "add member", etc.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sure. I will add those commands. thanks for advice.

Copy link
Contributor

Choose a reason for hiding this comment

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

Could you talk about your design choices (what commands to add or revise) too? We do not have much time for 1.8.0, and need to be more productive on design/discussions.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Design added in #3921

pkg/antctl/raw/multicluster/join/join.go Outdated Show resolved Hide resolved
pkg/antctl/raw/multicluster/create/access_token.go Outdated Show resolved Hide resolved
pkg/antctl/raw/multicluster/create/access_token.go Outdated Show resolved Hide resolved
pkg/antctl/raw/multicluster/destroy.go Outdated Show resolved Hide resolved
pkg/antctl/raw/multicluster/destroy.go Outdated Show resolved Hide resolved
pkg/antctl/raw/multicluster/destroy.go Outdated Show resolved Hide resolved
pkg/antctl/raw/multicluster/join.go Outdated Show resolved Hide resolved
pkg/antctl/raw/multicluster/leave.go Outdated Show resolved Hide resolved
pkg/antctl/raw/multicluster/leave.go Outdated Show resolved Hide resolved
pkg/antctl/raw/multicluster/leave.go Outdated Show resolved Hide resolved
pkg/antctl/raw/multicluster/leave.go Outdated Show resolved Hide resolved
@jianjuns
Copy link
Contributor

@hjiajing : please wrap lines in commit message at column 72.

@hjiajing hjiajing force-pushed the antctl-join branch 11 times, most recently from cd8d492 to 46a644f Compare July 28, 2022 15:37
@hjiajing hjiajing force-pushed the antctl-join branch 2 times, most recently from 1c983ea to 99bee47 Compare August 9, 2022 02:07
+ `antctl mc create clusterclaims` command can create two ClusterClaims in a leader or member cluster. One for the leader or member cluster, and another for the ClusterSet.
+ `antctl mc create clusterset` command can create a ClusterSet in a leader or member cluster.
`antctl mc create` command can create tokens for member clusters to join a ClusterSet. The command will
also create a Secret to store the token, as well as a ServiceAccount and a RoleBinding. The --output-file
Copy link
Contributor

Choose a reason for hiding this comment

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

Add `` around --output-file

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Added.

+ `antctl mc delete clusterset` command can delete a ClusterSet in a leader or member cluster.
+ `antctl mc delete member-cluster` command can delete a member cluster in a specified Antrea Multi-cluster ClusterSet.
`antctl mc init` command initializes an Antrea Multi-cluster ClusterSet in a leader cluster. It will create a
ClusterSet and ClusterClaims for the leader cluster. If the `--output-file` option is set, the join config
Copy link
Contributor

Choose a reason for hiding this comment

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

Humm.. I feel we still need to rephrase the sentence. How about:

", the config arguments for member clusters to join the ClusterSet will be saved to the specified file."

Copy link
Contributor

Choose a reason for hiding this comment

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

Also change "set" to "specified"

pkg/antctl/raw/multicluster/join.go Outdated Show resolved Hide resolved
docs/multicluster/antctl.md Outdated Show resolved Hide resolved
pkg/antctl/raw/multicluster/join.go Show resolved Hide resolved
command.Flags().StringVarP(&joinOpts.LeaderNamespace, "leader-namespace", "", "", "Namespace of the leader cluster")
command.Flags().StringVarP(&joinOpts.LeaderClusterID, "leader-clusterid", "", "", "Cluster ID of the leader cluster")
command.Flags().StringVarP(&joinOpts.TokenSecretName, "token-secret-name", "", "", "Name of the Secret resource that contains the member token. "+
"Token Secret name takes precedence over token Secret file and Secret manifest in the join config file")
Copy link
Contributor

Choose a reason for hiding this comment

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

"and the Secret manifest"

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Added "and"

}
}

// The precedence order is that the TokenSecretName > TokenSecretFile > JoinConfigFile.
Copy link
Contributor

Choose a reason for hiding this comment

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

Nit: remove "the".

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Removed.

}
err = deleteClusterClaims(cmd, k8sClient, cleanOpts.Namespace)
if err != nil {
fmt.Fprintf(cmd.OutOrStdout(), "Failed to delete ClusterClaim: %v\n", err)
Copy link
Contributor

Choose a reason for hiding this comment

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

First, we should remove the logs. Otherwise, we got duplicated log messages.

I meant just to remove "err =", so we have:

deleteClusterClaims(cmd, k8sClient, cleanOpts.Namespace)
deleteClusterSet(cmd, k8sClient, cleanOpts.Namespace, cleanOpts.ClusterSet)
...

Will this really get an error of golang checking? What is the error?


+ `antctl mc deploy leadercluster` command can deploy Antrea Multi-cluster Controller to a leader cluster, and define all the CRDs the leader cluster needed.
+ `antctl mc deploy membercluster` command can deploy Antrea Multi-cluster Controller to a member cluster, and define all the CRDs the member cluster needed.
When the config file is provided, the command line options may be overridden by the file. A token is needed for a
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
When the config file is provided, the command line options may be overridden by the file. A token is needed for a
When the config file is provided, the command line options may be overwritten by the file. A token is needed for a

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done.


## antctl mc leave

`antctl mc leave` command lets a member cluster leave a ClusterSet. It will delete the ClusterSet and ClusterClaims
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
`antctl mc leave` command lets a member cluster leave a ClusterSet. It will delete the ClusterSet and ClusterClaims
`antctl mc leave` command lets a member cluster leave a ClusterSet. It will delete the ClusterSet and ClusterClaims

deleteClusterClaims(cmd, k8sClient, cleanOpts.Namespace)
deleteClusterSet(cmd, k8sClient, cleanOpts.Namespace, cleanOpts.ClusterSet)
Copy link
Contributor

Choose a reason for hiding this comment

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

Could you delete ClusterSet first, them ClusterClaim? Here is a on-going PR #4062 which will stop ClusterClaim deletion if there is any ClusterSet referring to it.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sure. Fixed.

return e
}

func deleteClusterSet(cmd *cobra.Command, k8sClient client.Client, namespace string, clusterSet string) 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 didn't see you catch the error by deleteClusterSet and other delete methods on cleanup, so the return error can be removed?

return fmt.Errorf("a member token Secret must be provided through the Secret Name, or Secret file, or Secret manifest in the config file")
}
if o.LeaderNamespace == "" {
return fmt.Errorf("the the leader cluster Namespace is required")
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
return fmt.Errorf("the the leader cluster Namespace is required")
return fmt.Errorf("the leader cluster Namespace is required")

return fmt.Errorf("the API server of the leader cluster is required")
}
if o.TokenSecretName == "" && o.Secret == nil {
return fmt.Errorf("a member token Secret must be provided through the Secret Name, or Secret file, or Secret manifest in the config file")
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
return fmt.Errorf("a member token Secret must be provided through the Secret Name, or Secret file, or Secret manifest in the config file")
return fmt.Errorf("a member token Secret must be provided through the Secret name, or Secret file, or Secret manifest in the config file")

Copy link
Contributor Author

Choose a reason for hiding this comment

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

fixed.

pkg/antctl/raw/multicluster/join.go Show resolved Hide resolved
var leaveOpts *common.CleanOptions

var leaveExamples = strings.Trim(`
# Leave the ClusterSet.
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
# Leave the ClusterSet.
# Leave the ClusterSet from a member cluster.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done.

func NewLeaveCommand() *cobra.Command {
command := &cobra.Command{
Use: "leave",
Short: "Leave ClusterSet from a member cluster",
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
Short: "Leave ClusterSet from a member cluster",
Short: "Leave the ClusterSet from a member cluster",

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done.

Copy link
Contributor

@jianjuns jianjuns left a comment

Choose a reason for hiding this comment

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

LGTM

if err != nil {
return err
}
if _, err := file.Write([]byte("\n# Create a token Secret with the embedded manifest.\n")); err != nil {
Copy link
Contributor

Choose a reason for hiding this comment

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

I feel this line is not necessary as it looks strange in the standalone token Secret YAML file (not the join-config YAML). But not a big deal.

Copy link
Contributor

Choose a reason for hiding this comment

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

If we want to keep it, maybe rephrase the comment to be more generic, like

"# Manifest to create a Secret for a member cluster token."

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Changed the comment to " Manifest to create a Secret for a member cluster token."


```bash
antctl mc add membercluster [CLUSTER_ID] [-n NAMESPACE] [--clusterset CLUSTERSET] [--service-account SERVICE_ACCOUNT]
antctl mc deploy leadercluster [--antrea-version ANTREA_VERSION] [-n NAMESPACE] [-f PATH_TO_MANIFEST]
Copy link
Contributor

Choose a reason for hiding this comment

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

Should we use:

antctl mc deploy leadercluster -n NAMESPACE [--antrea-version ANTREA_VERSION] [-f PATH_TO_MANIFEST]

assuming Namespace is mandatory?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done.

antctl mc delete clusterclaims [-n NAMESPACE]
antctl mc delete clusterset [NAME] [-n NAMESPACE]
antctl mc delete membercluster [MEMBER_CLUSTER_ID] [-n NAMESPACE] [--clusterset CLUSTERSET]
antctl mc init [-n NAMESPACE] [--clusterset CLUSTERSET_ID] [--clusterid CLUSTERID] [--create-token] [--output-file OUTPUT_FILE]
Copy link
Contributor

Choose a reason for hiding this comment

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

Should we use:

antctl mc init -n NAMESPACE --clusterset CLUSTERSET_ID --clusterid CLUSTERID [--create-token] [--output-file OUTPUT_FILE]

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done.


```bash
antctl mc deploy leadercluster [--antrea-version ANTREA_VERSION] [-n NAMESPACE] [-f PATH_TO_MANIFEST]
antctl mc deploy membercluster [--antrea-version ANTREA_VERSION] [-n NAMESPACE] [-f PATH_TO_MANIFEST]
antctl mc join --clusterset=[CLUSTERSET_ID] \
Copy link
Contributor

Choose a reason for hiding this comment

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

Should we remove "[]" around the values? "[]" usually means optional.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sure. Removed the "[]".

and other resources created by antctl for the member cluster.

```bash
antctl mc leave --clusterset=[CLUSTERSET_ID] --namespace [NAMESPACE]
Copy link
Contributor

Choose a reason for hiding this comment

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

antctl mc leave --clusterset=CLUSTERSET_ID [--namespace NAMESPACE]

?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done.

ClusterSet and ClusterClaims and other resources created by antctl for the leader cluster.

```bash
antctl mc destroy --clusterset=CLUSTERSET_ID --namespace [NAMESPACE]
Copy link
Contributor

Choose a reason for hiding this comment

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

Namespace is not optional for "destroy".

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Removed the "[]"

antctl mc deploy membercluster [--antrea-version ANTREA_VERSION] [-n NAMESPACE] [-f PATH_TO_MANIFEST]
antctl mc join --clusterset=CLUSTERSET_ID \
--clusterid=CLUSTER_ID \
--namespace=MEMBER_NAMESPACE \
Copy link
Contributor

Choose a reason for hiding this comment

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

We can change to [--namespace=MEMBER_NAMESPACE]

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Changed.

--leader-clusterid=LEADER_CLUSTER_ID \
--leader-namespace=LEADER_NAMESPACE \
--leader-apiserver=LEADER_APISERVER \
--token-secret-name=TOKEN_SECRET_NAME \
Copy link
Contributor

Choose a reason for hiding this comment

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

Ditto

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done.

--leader-namespace=LEADER_NAMESPACE \
--leader-apiserver=LEADER_APISERVER \
--token-secret-name=TOKEN_SECRET_NAME \
--token-secret-file=TOKEN_SECRET_FILE
Copy link
Contributor

Choose a reason for hiding this comment

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

Ditto

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done.

--token-secret-name=TOKEN_SECRET_NAME \
--token-secret-file=TOKEN_SECRET_FILE

antctl mc join --config-file PATH_TO_CONFIG_FILE
Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe we can add [--clusterid=CLUSTER_ID] [--token-secret-name=TOKEN_SECRET_NAME] [--token-secret-file=TOKEN_SECRET_FILE] to indicate these can be overwritten?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Added the optional arguments.

anctcl mc create accesstoken [NAME] [-n NAMESPACE] [--serviceaccount SERVICE_ACCOUNT] [--role-binding ROLE_BINDING]
antctl mc create clusterclaims [-n NAMESPACE] [--clusterset-id CLUSTERSET_ID] [--cluster-id CLUSTER_ID]
antctl mc create clusterset [NAME] [-n NAMESPACE] [--leader-server LEADER_SERVER] [--service-account SERVICE_ACCOUNT] [--secret SECRET] [--leader-cluster LEADER_CLUSTER_ID]
anctcl mc create membertoken NAME -n NAMESPACE [--output-file OUTPUT_FILE]
Copy link
Contributor

Choose a reason for hiding this comment

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

Consider using "-o".

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Change to -o.

docs/multicluster/antctl.md Outdated Show resolved Hide resolved
jianjuns
jianjuns previously approved these changes Aug 10, 2022
config := &ClusterSetJoinConfig{
APIVersion: common.ClusterSetJoinConfigAPIVersion,
Kind: common.ClusterSetJoinConfigKind,
Namespace: "",
Copy link
Contributor

Choose a reason for hiding this comment

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

@hjiajing : when we unmarshal the YAML, will these "" fields overwrite values in ClusterSetJoinConfig?

Copy link
Contributor

Choose a reason for hiding this comment

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

If so, I suggest not to set these fields in the generated file, as I hope the join command can use command line options to provide these arguments which seems the most convenient way for users.

I meant something like antctl mc join --clusterid test-cluster-leader -n kube-system --config-file join-config.yml

Or even better, if we can write clusterID and namespace and other optional fields to the config file as comments:

#clusterID: ""
#namespace: ""
# Use the pre-created token Secret.
#tokenSecretName: ""
# Create a token Secret with the manifest file.
#toeknSecretFile: ""

See also the discussion at #4096 (comment).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Added a raw string constant now the output join config looks like

---
kind: ClusterSetJoinConfig
apiVersion: multicluster.antrea.io/v1alpha1
clusterSetID: test-clusterset
leaderClusterID: test-cluster-north
leaderNamespace: antrea-multicluster
leaderAPIServer: https://172.18.0.3:6443
clusterID: "test-cluster-east"
#namespace: ""
# Use the pre-created token Secret.
#tokenSecretName: ""
# Create a token Secret with the manifest file.
#toeknSecretFile: ""

Copy link
Contributor

Choose a reason for hiding this comment

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

I feel we should comment out clusterID too, so it can be overwritten by command line argument.

#clusterID: ""

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sorry I copied the output file which I edited. The output is

"config.yaml" 25L, 3372B                                                                                                                  13,1          All
---
kind: ClusterSetJoinConfig
apiVersion: multicluster.antrea.io/v1alpha1
clusterSetID: test-clusterset
leaderClusterID: test-cluster-north
leaderNamespace: antrea-multicluster
leaderAPIServer: https://172.18.0.3:6443
#clusterID: ""
#namespace: ""
# Use the pre-created token Secret.
#tokenSecretName: ""
# Create a token Secret with the manifest file.
#toeknSecretFile: ""

The clusterID is commented out.

func unmarshallSecret(raw []byte) (*v1.Secret, error) {
decoder := yamlutil.NewYAMLOrJSONDecoder(bytes.NewReader(raw), 100)
secret := &v1.Secret{}
// We need to skip the first object in the config file which is ClusterSetJoinConfig
Copy link
Contributor

Choose a reason for hiding this comment

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

Question - why we need to do the same for Secret file? Maybe we need to revise the comment?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The output of Secret and join config both start with ---. So when we unmarshall the Secret, we need to skip the first object, too. Revised the comment now.

func unmarshallSecret(raw []byte) (*v1.Secret, error) {
decoder := yamlutil.NewYAMLOrJSONDecoder(bytes.NewReader(raw), 100)
secret := &v1.Secret{}
// We need to skip the first object, the Secret object is always the second object when unmarshalling the
Copy link
Contributor

Choose a reason for hiding this comment

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

, as the Secret object...

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed.

Signed-off-by: hujiajing <hjiajing@vmware.com>
@jianjuns
Copy link
Contributor

@hjiajing Thanks for the hard work and addressing all comments!

@jianjuns
Copy link
Contributor

/test-e2e
/test-multicluster-e2e

@jianjuns
Copy link
Contributor

/test-integration

@jianjuns
Copy link
Contributor

/skip-all

@jianjuns jianjuns merged commit 04b9dc6 into antrea-io:main Aug 10, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/multi-cluster Issues or PRs related to multi cluster.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Multi-cluster antctl command refactor
3 participants