upbound / up

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

`up ctp connect` does not work with spaces #407

Closed sttts closed 5 months ago

sttts commented 9 months ago

What happened?

Two issues:

  1. even though on a Space profile, the up ctp connect command requires an account (which is not set for a pure-Spaces profile):

    up: error: controlplane.connectCmd.Run(): error: account is missing from profile
  2. the connection secret holding the kubeconfig of a Space controlplane has arbitrary cluster, auth and context names. The up ctp connect code requires a format used by the cloud:

    up: error: controlplane.connectCmd.Run(): config is broken, missing cluster: "somefakeaccount-ctp1"

How can we reproduce it?

Create a profile with up profile set space --profile space and switch to it.

Then up ctp connect to some controlplane on that Spaces cluster.

What environment did it happen in?

All latest main branches.

tnthornton commented 9 months ago

I wonder if up profile set space --profile space is not setting the account in the profile for the space 🤔 . These flows for spaces were mostly tested against:

  1. up space init
  2. up create ctpX
  3. up ctp connect ctpX
sttts commented 9 months ago

Account is not set, that's correct. But even after that it does not work because the kubeconfig does not have the expected format. See (2) in the description.

tnthornton commented 9 months ago

I saw that comment in the description 😄

But even after that it does not work because the kubeconfig does not have the expected format.

What's interesting is I'm not able to reproduce using the steps I called out above. In addition, we're in control of the cluster name for the control planes, we have a block that looks like this name: {{ .Organization }}-{{ .ControlPlaneName }} in the current template.

I'll give your repro steps a try and see what's up 👍

tnthornton commented 9 months ago

OK given the details we have above and what I'm seeing locally, I have a couple of clarifying questions:

  1. "which is not set for a pure-Spaces profile". Does this mean that you performed a up space init without setting --set account=some-account-name? Assuming you did, then the default account name associated with the space is notdemo. I'm not a fan of that name making it into 1.0, etc. but it's intent was to signal to us that the account config was incorrect for a given install.
  2. If I'm right about (1), did you manually set somefakeaccount as the profile account name? If so that would make sense why the connect step is failing. If I'm right about (1), then the cluster name is notdemo-ctp1, hence we couldn't find it using somefakeaccount.

For me, locally, if I do the following:

  1. rm /Users/tnthornton/.up/config.json - this removes my current up configs.
  2. up profile list
    No profiles found
  3. up profile set space
    CURRENT   NAME      TYPE    ACCOUNT   KUBECONFIG   KUBECONTEXT
    *         default   space                          kind-kind
  4. up ctp create one
    one created
  5. Once the control plane is available, up ctp connect one
    up: error: controlplane.connectCmd.Run(): error: account is missing from profile

Now if I add the account to that profile (my spaces install was initialized using up space init --token-file=key.json "v1.1.0" --set account=taylort)

  1. up profile list
    CURRENT   NAME      TYPE    ACCOUNT   KUBECONFIG   KUBECONTEXT
    *         default   space   taylort                kind-kind
  2. up ctp connect one
  3. kubectl get ns
    NAME              STATUS   AGE
    default           Active   21m
    kube-node-lease   Active   21m
    kube-public       Active   21m
    kube-system       Active   21m
    upbound-system    Active   21m

I thought there might be a bug in the profile set code where we weren't passing in the account, but that appears to not be the case https://github.com/upbound/up/blob/476d617a6c6f27016bc61bb3fc141c64ef28b80d/cmd/up/profile/set.go#L74. So if we have it, either through the UP_ACCOUNT environment variable or the --account parameter we should set it on the profile.

So far this is looking like a clunky user experience with some gaps that were highlighted and a callout for some better doc strings, however that's based on the assumptions I'm making above. Let me know if any of assumptions I made in response to the clarifying questions are incorrect and I can dig in some more to see what else may be going on 👍

sttts commented 9 months ago

I used make cluster.up tild.up. That might explain why I have no account. Do we enforce an account in a helm installation?

This is the kubeconfig for the controlplane I see:

apiVersion: v1
clusters:
- cluster:
    certificate-authority-data: LS0tLS1CRUdJTiBDRVJUSUZJQ0FURS0tLS0tCk1JSUJ5VENDQVc2Z0F3SUJBZ0lSQUxQL3BwRkw1Tms4b1M4SXhKc25hWXd3Q2dZSUtvWkl6ajBFQXdJd05ERUwKTUFrR0ExVUVCaE1DVlZNeERUQUxCZ05WQkFvVEJHRmpiV1V4RmpBVUJnTlZCQU1URFZWd1ltOTFibVFzSUVsdQpZeTR3SGhjTk1qTXhNRE14TVRJeE1ERXpXaGNOTWpReE1ETXdNVEl4TURFeldqQTBNUXN3Q1FZRFZRUUdFd0pWClV6RU5NQXNHQTFVRUNoTUVZV050WlRFV01CUUdBMVVFQXhNTlZYQmliM1Z1WkN3Z1NXNWpMakJaTUJNR0J5cUcKU000OUFnRUdDQ3FHU000OUF3RUhBMElBQkhmaGtYTGVHeWozcmFMd09tc041eEpwM3Y5M05ObWI5ZGNBUVE1SQp1SG5nV1pDL3kwMFg1NXpuNEFJTC94MnBGck5DUWUzTDFoSnRzVUNlK3BOZGxxR2pZVEJmTUE0R0ExVWREd0VCCi93UUVBd0lCcGpBZEJnTlZIU1VFRmpBVUJnZ3JCZ0VGQlFjREFRWUlLd1lCQlFVSEF3SXdEd1lEVlIwVEFRSC8KQkFVd0F3RUIvekFkQmdOVkhRNEVGZ1FVSlZQYU1QalVVVGlPdlFlWnplVDFLaHRZMlNjd0NnWUlLb1pJemowRQpBd0lEU1FBd1JnSWhBSVF3OVEwTndEcHg3cUVHc2JZRi81YkhHNHhaQy9DUmVFcnpWUjBDUnN1NkFpRUF2ajVDCjhkeTlTYlNndUFDaWY1eE41bk9UZlFDdlFtazB4Y3ZNSG5oTng4ST0KLS0tLS1FTkQgQ0VSVElGSUNBVEUtLS0tLQo=
    server: "https://ingress.local/v1/controlPlanes/localdev/ctp1/k8s"
  name: localdev-ctp1
contexts:
- context:
    cluster: localdev-ctp1
    user: localdev-ctp1
  name: localdev-ctp1
current-context: localdev-ctp1
kind: Config
preferences: {}
users:
- name: localdev-ctp1
  user:
    token: eyJhbGciOiJFUzI1NiIsInR5cCI6IkpXVCJ9.eyJwYXlsb2FkIjp7Imdyb3VwcyI6W10sInVwYm91bmRJRCI6InVzZXIvMCJ9LCJhdWQiOiJjdHAxIiwiZXhwIjoxNzMwNDUzNzE3LCJpYXQiOjE2OTg5MTc3MTd9.0zjIxC4TjoeCAY61xfyWEGiUWTVGKsOJrq1d8UtmPngSyUZ7ub1lMGZ7krOXJopEB_QQyVGr8PuuV5ZgBaIDfQ

So, likely an accountname "localdev" would work here.

sttts commented 9 months ago

I would just drop this account prefix logic in the kubeconfig code. Just take the default context and follow the cluster and user names. Don't expect anything there. Then it works with or without account names.

tnthornton commented 9 months ago

I used make cluster.up tild.up. That might explain why I have no account. Do we enforce an account in a helm installation?

We have the default set in the values.yaml, but we aren't currently requiring it to be overridden.

So, likely an accountname "localdev" would work here. Ya we have '--set=account=localdev', in the Tiltfile.

tnthornton commented 9 months ago

Just take the default context and follow the cluster and user names. Don't expect anything there. Then it works with or without account names.

I think that's fair. It'll also reduce some of the complexity in there.