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

Use target service name instead of ID as connect proxy's base service name #4620

Merged
merged 1 commit into from
Sep 5, 2018

Conversation

uepoch
Copy link
Contributor

@uepoch uepoch commented Aug 30, 2018

This should address #4619

This fix issues appearing when you define a service Name which is not a prefix of the Service ID and have ACL configured for that name

Like

"""
ID: service_id
Name: name_of_service
"""
with an ACL permitting to write only on 'name_of_service*'

@pierresouchay
Copy link
Contributor

Thank you @uepoch for solving this.

This patch allow services having different IDs and Name to work properly when ACLs.

This patch has been tested in our production successfully, credit to @uepoch to the finding

@pierresouchay
Copy link
Contributor

@banks This is a major bug has it breaks Consul Connect for people using ACLs (and service.Name != service.ID)

Copy link
Member

@banks banks left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks @uepoch and @pierresouchay.

Yeah this make sense for now. This was a somewhat known issue and this fix will soon be partly broken by new ACLs that exact match by default (since an ACL for web will no longer allow web-proxy anyway.).

I think the real solution to that is more involved and means changing the ACL rules for registrations where Kind = "connect-proxy".

But this is a great fix for interim and has no real downsides - it makes more sense to operators so see the name rather than ID mirrored too.

@uepoch uepoch force-pushed the connect-service branch 3 times, most recently from 593e631 to 8df6714 Compare September 4, 2018 15:54
Copy link
Contributor

@pierresouchay pierresouchay left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Tested successfully on our Clusters

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

Successfully merging this pull request may close these issues.

3 participants