upbound / up

The @upbound CLI
Apache License 2.0
51 stars 41 forks source link

`up ctp connect` works exactly once #401

Closed sttts closed 6 months ago

sttts commented 9 months ago

What happened?

up ctp connect (added in https://github.com/upbound/up/pull/393/files) works with the first control plane. When trying to run it again to switch to another control plane, this code triggers and the command does nothing:

    // Check if the fs kubeconfig is already pointing to a control plane and
    // return early if so.
    origContext := kcloader.CurrentContext
    if strings.HasPrefix(origContext, upboundPrefix) {
        return nil
    }

How can we reproduce it?

Run up ctp connect against two control planes. Second command is a noop.

What environment did it happen in?

Cloud prod.

sttts commented 9 months ago

I guess the intention was that up ctp disconnect restored the old context, and before doing that up ctp connect is a noop. That's surprising for the user and not documented.

Am not a fan of that, but at least it should print not doing anything because a controlplane is already connected and return with an error. It would be more logical to just connect to the new controlplane instead. It would be what the command says: connect.

AlainRoy commented 9 months ago

I've assigned it to @tnthornton to fix next week. He's the one that implemented it and he can comment on the intended behavior. I agree -- I also find the behavior surprising, but we can discuss it with him next week instead of preemptively changing it. He'll be back on Tuesday.

sttts commented 9 months ago

A related topic here is about tokens. The --token flag kind of destroys the experience. If we reused the token from a previous connect, that would make this much nicer. E.g. by defining an auth setting in the kubeconfig as the default one for Upbound cloud.

sttts commented 9 months ago

Also in this context, maybe I just missed it and it already exists: --context <the-kube-context-name-for-this-ctp. People use kubectx or other tooling to switch between Kube clusters all the time. This would make Upbound compatible with those workflows.

AlainRoy commented 9 months ago

Taylor and I chatted about tokens before -- is there a way we can eliminate the need for the tokens? Most other commands don't require a token. Why should this one/

tnthornton commented 9 months ago

I guess the intention was that up ctp disconnect restored the old context, and before doing that up ctp connect is a noop. That's surprising for the user and not documented.

That is accurate. Originally the idea was that you were only ever attempting to connect from a "real" cluster; hence the need to disconnect before attempt to connect to a different control plane.

The main hurdle with allowing to connect back to back and then performing a disconnect is that the disconnect will return you to the previous control plane rather than the "real" cluster. That's due to the previous context being tracked in the context name (you can see this by connecting to a control plane and then running kubectl config current-context).

tnthornton commented 9 months ago

Also in this context, maybe I just missed it and it already exists: --context <the-kube-context-name-for-this-ctp. People use kubectx or other tooling to switch between Kube clusters all the time. This would make Upbound compatible with those workflows.

Nope you didn't miss it. It was out of scope for the original implementation.