koyeb / koyeb-cli

Koyeb cli
Apache License 2.0
57 stars 33 forks source link

Fix `koyeb instance exec xxx` when instance id is a full uuid #167

Open brmzkw opened 10 months ago

brmzkw commented 10 months ago

If we provide a full uuid, the following error message is returned:

❌ Unable to find the instance `56baae83-1223-5165-b8bb-cd6e7b35c5b3`: no object could be found from the provided identifier

🔎 Additional details
The supported formats to resolve a instance are:
* instance full UUID
* instance short ID (8 characters)

🏥 How to solve the issue?
Provide a valid instance identifier

It works well if the short uuid is used (56baae83)

munanadi commented 8 months ago

Can I take this up? https://github.com/koyeb/koyeb-cli/blob/7f71b41900e041ec77c185d65b323ec339fd5aca/pkg/koyeb/idmapper/idmap.go#L19-L22

The issue is because we are looking for the full uuid in the valCache which stores the shortId -> longId mapping

Can I do something like

func (idmap *IDMap) GetID(val string) (string, bool) {
    id, ok := idmap.valCache[val]
        // If there is a match for the long id, I simply return the long id
    if _, ok := idmap.idCache[val]; ok { 
        return val, ok
    }
    return id, ok
}

I thought this doesn't change any other functions and has the least impact? does this work? Happy to take another approach if you advice so.

Cheers.

brmzkw commented 8 months ago

Hey,

That's not what we should do. We already check specifically for UUIDv4 at this spot in our code: instance.go#L28.

A few months ago, we changed our UUIDs from v4 to a fully random format. The difference? UUIDv4 has some bits set aside, but our new ones don't.

Instead of checking for UUIDv4, we should check for any UUID.

munanadi commented 8 months ago

Got it. So, I need to check for a generic UUID instead of the more stricter rule that checks for v4 alone.

https://github.com/koyeb/koyeb-cli/blob/7f71b41900e041ec77c185d65b323ec339fd5aca/pkg/koyeb/idmapper/regex.go#L5-L8

Replacing this with the more flexible version of UUID regex

const (
    // UUIDv4 is the regular expressions for an UUID v4.
-   UUIDv4 string = "^[0-9a-f]{8}-[0-9a-f]{4}-4[0-9a-f]{3}-[89ab][0-9a-f]{3}-[0-9a-f]{12}$"
+       UUIDv4 string = "^[0-9a-f]{8}-[0-9a-f]{4}-[0-9a-f]{4}-[0-9a-f]{4}-[0-9a-f]{12}$"
)

Works?

brmzkw commented 8 months ago

I don't know why we are doing a regexp matching here. I believe we should use the uuid module instead:

uuid, err := uuid.Parse(value)

We can change the existing function IsUUIDv4 and return true if err is nil, and uuid.Version() == 4. We can also create a new function IsUUID (which we should call in the instances mapper), and only check if err is nil.

munanadi commented 8 months ago

I was trying to not change a lot of code around and work with what's already there. Got it, Will look into this. 👍

brmzkw commented 8 months ago

thank you! Feel free to make a pull request