liqoctl: add incoming flag to peer and unpeer commands

[hamzalsheikh]

Description

The pull request adds an incoming flag in the peer and unpeer commands.
User will be able to explicitly set the incoming flag while executing the commands.

Passing --incoming in peer commands sets the ForeignCluster resource .spec.incomingPeeringEnabled to Yes.
Passing --incoming in unpeer commands sets the ForeignCluster resource .spec.incomingPeeringEnabled to No.

Fixes #957

How Has This Been Tested?

• locally

[adamjensenbot]

Hi @hamzalsheikh. 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:

• kind/breaking: 💥 Breaking Change
• kind/feature: 🚀 New Feature
• kind/bug: 🐛 Bug Fix
• kind/cleanup: 🧹 Code Refactoring
• kind/docs: 📝 Documentation

[hamzalsheikh]

Both unpeering commands delete the foreign cluster resource.

The unpeer out-of-band have a boolean that prevents that, but is always set to true at the moment.

[aleoli]

Thank @hamzalsheikh! When the --incoming flag is set, no change has to be made to the outgoing peering (neither for peer nor unpeer).

Can you change the behavior of the flag you are pointing out?

[hamzalsheikh]

Sorry, my comment was ambiguous.

No change is being made to the outgoing peering. The flag only changes incoming peering variable in foreign cluster spec.

I'm pointing out that the current implementation of the unpeer commands automatically deletes the foreign cluster resource. I haven't changed that, but would suggest actively using the UnpeerOOBMode and adding a similar option to Inband in future enhancements.

[aleoli]

Ok, I tested your implementation; if I peer c1 with c2:

# on c2
liqoctl generate peer-command

# on c1
liqoctl peer ...

k get foreignclusters.discovery.liqo.io
NAME         TYPE        OUTGOING PEERING   INCOMING PEERING   NETWORKING    AUTHENTICATION   AGE
snowy-wood   OutOfBand   Established        None               Established   Established      97s

Then disable the incoming on the other cluster

# on c2
liqoctl unpeer --incoming twilight-shape

k get foreignclusters.discovery.liqo.io
NAME             TYPE        OUTGOING PEERING   INCOMING PEERING   NETWORKING   AUTHENTICATION   AGE
twilight-shape   OutOfBand   None               Pending            None         Established      3m5s

and re-enable the incoming on c2

# on c2
liqoctl peer --incoming twilight-shape

k get foreignclusters.discovery.liqo.io
NAME             TYPE        OUTGOING PEERING   INCOMING PEERING   NETWORKING    AUTHENTICATION   AGE
twilight-shape   OutOfBand   Established        Pending            Established   Established      4m16s

It enables the outgoing peering but not the incoming; I expect to do exactly the opposite, leaving the outgoing in the previous state and enabling the incoming.

[hamzalsheikh]

The flag should work as expected now. Running peer and unpeer commands with the --incoming flag will only change the foreign cluster resource .spec.incomingPeeringEnabled to yes and no, respectively.

Can you please test and confirm. :)

However, the PR is not ready to be merged yet.

1. My initial changes was keeping the "outgoing peering unchanged," as in keep it running as is, with the --incoming flag adding to that. I think this is what you wanted to achieve in this comment with the --all flag. Should I keep those changes and create a new flag --all for them, or do you prefer merging the simple version at the moment?

2. The flag change is reflected in:

:~/liqo$ sudo kubectl describe foreignclusters
   .
   .
   .

Spec:
   .
   .
   .

  Incoming Peering Enabled:  Yes/No

Correct me if I'm wrong, I don't think the flag should change the Incoming peering status too (in k get foreignclusters.discovery.liqo.io, that should be done when establishing an incoming peering. If this is not the case, what other changes are needed?

3. (related to the above) For error handling and correctness I am currently waiting on the foreign cluster resource change, not the peering condition. Is that the right thing to wait for / is there other conditions I should consider (e.g. ForAuth, ForNode, etc ...)? I'm also not using the GetStatus method since it expects a peering condition which would be misleading, but I can add a new condition and use it.

[aleoli]

I think the --all flag could be confusing, and I'm not sure it is a good idea to implement it. For now, we can merge a simple solution when: if you use the --incoming, you only change the incoming field, and if nothing is specified, you control the outgoing field.

The liqoctl command should not touch the fc status; it has to be handled by the controller, and it is doing that. I see the status change when I change the incoming sets with the cluster peered. It works as expected.

What are you saying about the conditions? Can you give an example? It should be ok not to wait because if the cluster is not peered, no change happens in the status. You may consider waiting for incoming != established if the cluster was initially peered.

[hamzalsheikh]

The peer and unpeer commands should be working as intended.

I'm not waiting on the peering status change, but the change in the foreign cluster resource:
1- https://github.com/hamzalsheikh/liqo/blob/860d783a992ff06c1a967d9532773875682082a1/pkg/liqoctl/wait/wait.go#L81
2- https://github.com/hamzalsheikh/liqo/blob/860d783a992ff06c1a967d9532773875682082a1/pkg/liqoctl/wait/wait.go#L136

[aleoli]

Thanks @hamzalsheikh! It looks ok; I will test it deeper in the next few days.

[aleoli]

Hi @hamzalsheikh, there is only a small change; then it's ok and we can merge!

Can you please fix it and squash your commits?

[fra98]

/rebase

[fra98]

@hamzalsheikh you need to resolve linting and formatting errors. I think if you run make generate it should fix most of them. Also, remember to squash all the commits into one at the end

[aleoli]

@hamzalsheikh, you still have a minor linting issue; thanks for fixing the others!

[fra98]

@hamzalsheikh you still have a minor linting issue (it is a whitespace). Running go fmt ./... should fix it.

[fra98]

/rebase test=true

[fra98]

/merge