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

Tag Lookup #8

Open
banks opened this issue Mar 3, 2020 · 0 comments
Open

Tag Lookup #8

banks opened this issue Mar 3, 2020 · 0 comments

Comments

@banks
Copy link

banks commented Mar 3, 2020

Note if this is changed it might make #7 invalid.

if t == *serviceTag {
serviceID = s.ID
break OUTER
}
}
}
if serviceID == "" {
log.Fatalf("No sidecar proxy found for service with tag %s", *serviceTag)

Having skipped sidecars (assuming that works, see #7), this code attempts to find the first tag match on an actual service instance. But if it fails it returns an error about not finding the sidecar proxy which is confusing since it might have been there you just skipped it.

I think I understand the intent and it's rely on the existing sidecarFor lookup code copied from Consul (great - was going to recommend that part), but as it is there are a lot of confusing ways this can break: 

  • if there is a proxy registered but no service instance (which is technically valid although not that useful, but in practice can happen transiently due to timing of processes registering). 
  • if there are multiple services registered locally with that tag, it will arbitrarily pick the first one which isn't great UX. Better would be to detect the ambiguity and error in the same way LookupProxyIDForSidecar in the Consul command code does.

I suggest it might be easier if all the lookup stuff was moved out of the /consul package and into the main.go so that you can handle all this resolving stuff in one place and just start the main body of the work with a proxy's ID already resolved or all argument errors already handled with messages that are clear to the user where there is ambiguity.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

2 participants