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

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

Open
brmzkw opened this issue Dec 19, 2023 · 6 comments
Open

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

brmzkw opened this issue Dec 19, 2023 · 6 comments
Labels
good first issue Good for newcomers

Comments

@brmzkw
Copy link
Contributor

brmzkw commented Dec 19, 2023

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)

@brmzkw brmzkw added the good first issue Good for newcomers label Dec 19, 2023
@munanadi
Copy link

Can I take this up?

func (idmap *IDMap) GetID(val string) (string, bool) {
id, ok := idmap.valCache[val]
return id, ok
}

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
Copy link
Contributor Author

brmzkw commented Feb 21, 2024

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
Copy link

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

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}$"
)

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
Copy link
Contributor Author

brmzkw commented Feb 21, 2024

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
Copy link

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
Copy link
Contributor Author

brmzkw commented Feb 21, 2024

thank you! Feel free to make a pull request

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
good first issue Good for newcomers
Projects
None yet
Development

No branches or pull requests

2 participants