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

Client Authorization over localhost with Envoy sidecar #3373

Closed
erwbgy opened this issue Feb 17, 2021 · 7 comments · Fixed by #3419
Closed

Client Authorization over localhost with Envoy sidecar #3373

erwbgy opened this issue Feb 17, 2021 · 7 comments · Fixed by #3419
Labels
kind/feature Categorizes issue or PR as related to a new feature. lifecycle/needs-triage Indicates that an issue needs to be triaged by a project contributor.

Comments

@erwbgy
Copy link
Contributor

erwbgy commented Feb 17, 2021

Please describe the problem you have
Currently configuring client authorization requires using an ExtensionService object with a reference to a Service that resolves to Pod endpoints for an authorization server that implements the Envoy External Authorisation API.

But, instead of running the authorization server as a separate set of Pods I would like to run it as a side-car with each Envoy Pod (opa-envoy) accessed over localhost as this is much easier to scale and secure.

Perhaps ExtensionService could support a host and port to call as an alternative to a Service?

@erwbgy erwbgy added kind/feature Categorizes issue or PR as related to a new feature. lifecycle/needs-triage Indicates that an issue needs to be triaged by a project contributor. labels Feb 17, 2021
@youngnick
Copy link
Member

youngnick commented Feb 18, 2021

Originally, I think we were hoping that people would use an ExternalName service to point to things that weren't just a Kubernetes Service.

~~However, I think there's an argument for adding functionality to ExtensionService to say "this extension service runs as a sidecar on the Envoy pod". This would then be syntactic sugar for a "localhost" cluster, which would allow you to tweak load balancing and timeouts easily, since they'll be different for localhost.

What I'm thinking is about adding a "sideCarPort" directive or similar, that is a sibling to the Services field, with only one of them being able to be set at once.~~
After chatting with @jpeach, I've been won over to the idea of having an ExtensionSidecar resource to describe this instead.
That would allow us to set a good default for load balancing and timeouts, while allowing you to tweak the timeouts if required.

This would need a proper design, probably with a design doc, but I think it would help in this case, and possibly in other cases like logging (#1691) and tracing (#399), since both of those could conceivably use a sidecar collector.

@stevesloka
Copy link
Member

Couldn't an externalName service work as described vs adding another CRD?

@youngnick
Copy link
Member

youngnick commented Feb 19, 2021

It could work yes. But it doesn't feel right to me to have an "ExternalName" service that refers to the pod's localhost. Also, when things are going to a local sidecar, the timeout and load balancing requirements are totally different.

I guess what I'm saying is that I think that using ExternalName for something that's not really a name and definitely not External feels a bit dirty.

@erwbgy
Copy link
Contributor Author

erwbgy commented Feb 28, 2021

You could argue that localhost is a name and the side-car is external to Envoy :-) I'd be favour of using an ExternalName Service rather than another CRD as ExtensionService provides everything that is required. We just need to document this as the pattern to use for sidecars.

@youngnick
Copy link
Member

It certainly gets us working for now. I would still argue that there's some stuff where the ExtensionService defaults just don't make sense (like timeouts), and having load balancing for localhost might be counter-productive.

But an ExternalName will definitely get everyone working, and we can always change our mind on this one if we need to. Thanks for the PR, @erwbgy!

@skriss
Copy link
Member

skriss commented Jun 16, 2021

Was just looking at this as part of the design for tracing, and unless I'm missing something, this approach doesn't actually work right now -- https://github.com/projectcontour/contour/blob/main/internal/dag/extension_processor.go#L179-L184 explicitly disallows ExternalName services, for reasons described in #2875.

@youngnick
Copy link
Member

hmm, thanks for pointing that out @skriss. I think that we should definitely have a way to allow for localhost, as sidecar clusters will be increasingly common. Including a DNS name field per #2875 seems like a good way to do that.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
kind/feature Categorizes issue or PR as related to a new feature. lifecycle/needs-triage Indicates that an issue needs to be triaged by a project contributor.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants