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

Feat/plugins mesh field 1/2 #3930

Merged
merged 6 commits into from Nov 14, 2018

Conversation

Projects
None yet
5 participants
@kikito
Copy link
Member

commented Oct 29, 2018

This PR includes:

  • A fix on the schema, found by chance while studying how to best implement the new field
  • A new field called mesh_mode to tag plugins with inbound, outbound or two-way. Note that two-way plugin instances can be individually tagged as inbound or outbound on creation, but plugins tagged as inbound can only have inbound instances, and the same goes for outbound pluggins.

I have tagged 15 plugins as two-way on this PR. There are multiple commits with explanations for my reasons.

@kikito kikito requested review from hishamhm and james-callahan Oct 29, 2018

@kikito kikito force-pushed the feat/plugins-mesh-field-1 branch from 3a46a2b to d87b725 Oct 29, 2018

@hishamhm

This comment has been minimized.

Copy link
Member

commented Oct 29, 2018

@kikito You have added a new top-level field to the Plugins entity in the db, but in the schemas you added it as a subfield of the config entry. If mesh_mode is a top-level Plugins field, it should be alongside config in the schemas, and not inside it.

typedefs.mesh_two_way = Schema.define {
type = "string",
default = "two-way",
one_of = { "inbound", "outbound", "two-way" },

This comment has been minimized.

Copy link
@Tieske

Tieske Oct 29, 2018

Member

<opinion> use twice or both instead of two-way </opinion>

@Tieske

This comment has been minimized.

Copy link
Member

commented Oct 29, 2018

The auth plugins are all two-way, but I think their default value should be outbound. The functional effect will be the same. You want it to run on outbound to stop 'unwanted' traffic asap, and if it passes on one end, it will also pass on the other end (inbound), so there is no use in authenticating again.

Conceptually: currently the available values are tied to the default value. And I think they should be separate.

@coopr

This comment has been minimized.

Copy link
Contributor

commented Oct 29, 2018

The auth plugins are all two-way, but I think their default value should be outbound. The functional effect will be the same. You want it to run on outbound to stop 'unwanted' traffic asap, and if it passes on one end, it will also pass on the other end (inbound), so there is no use in authenticating again.

I was imagining all the auth plugins would be inbound - that is conceptually most similar to how Kong API Gateway works, and I think it is easier to reason about (eg. "OK, I'm configuring an auth plugin that'll decide who gets past the proxy and gets to talk to my application..." - that makes it all about inbound traffic to the local proxy+application, and makes no assumptions about the source of the traffic)

@bungle

This comment has been minimized.

Copy link
Member

commented Oct 29, 2018

In service mesh it is different. The forward proxy already knows that authentication is going to fail, so it stops the traffic on a sidecar (Kong proxy) before even entering the network. Eg. traffic is stopped on forward proxy's incoming traffic and not on the to be called service's reverse proxy incoming traffic. I'd like to call these forward and reverse proxies, e.g. plugin is executed on forward proxy or reverse proxy or both. Most of the auth plugins will be executed on service mesh only on forward proxy e.g. The traffic that is trying to enter the mesh. After The traffic has entered the mesh, it is trusted traffic by mutual tls. All The plugins still have same incoming and outgoing nginx phases. It is just a matter that will the plugin run on one proxy (and which) or both. I would use forward proxy, reverse proxy and both|always|..., e.g. run-on="forward-proxy". You could call forward and reverse proxies as exit and enter If you think about network structure. Traffic exits the service a and enters service b. But forward-proxy and reverse-proxy are My favourites. Mainly because I used enter in opposite meaning on this text above. Forward proxy and reverse proxy doesn't confuse me. And the plugins run in proxy, all the proxies handle incoming and outgoing. In mesh there are two proxies that both handle incoming and outgoing traffic. But the separation here that we want to make is what proxy gets to run the plugin code. All our authentication plugins run in access phase which is incoming traffic, in that sense incoming / inbound is irrelevant for this choice as it is always inbound but in which proxy, that is the question.

@bungle

This comment has been minimized.

Copy link
Member

commented Oct 30, 2018

After talking with James, I got new info, so new proposal for naming:

run-on="gateway|mesh|both"

Because we don't know before actual connection is made (and after the access phase is already executed) whether or not we are in service mesh, we call everything by default a gateway. All forward proxies in service mesh are gateways (gw to the mesh or non mesh too) and all reverse proxies in service mesh are mesh nodes. So only the incoming can be a mesh node in service mesh (because it knows it even before access phase). If there is no service mesh, then all Kong nodes are gateways, and usually they are reverse proxies (e.g. ingress). The role cannot change between different Nginx phases (by a design decision, it is not impossible, but makes it easier to understand).

So if you think about this, then all our current plugins should be marked run-on="gateway". But we may adjust some (the zipkin tracing to be "both"). We do not have mesh only plugins, e.g. run only on incoming (reverse proxy) mesh, because that would make plugin a incoming mesh only (e.g. you couldn't use the plugin in single Kong cluster or between nodes that are not mesh nodes) And as explained the outgoing (the forward proxy in mesh) doesn't know if it connects to mesh or not (until after balancer is executed, e.g. header filter phase and there on), the outgoing is gateway.

@bungle

This comment has been minimized.

Copy link
Member

commented Oct 30, 2018

That said ^

  1. We can run plugin code always = "both"
  2. We can run plugin only on incoming mesh = "mesh"
  3. We can run plugin only on non-incoming mesh = "gateway" (this is the default)

But we cannot run plugin only in outgoing mesh, because it knows it too late. In future we can add possibility to run plugin only on outgoing mesh header_filter, body_filter and log phases (and respective stream phases) BUT never only on outgoing mesh nodes access phase.

@bungle

This comment has been minimized.

Copy link
Member

commented Oct 30, 2018

^ the above is the default hard coded plugin level settings. The next question obviously is that do we allow change these defaults on plugin instance level, e.g. plugin.config.run_on=mesh overrides the hardcoded plugin.run_on=gateway.

@bungle

This comment has been minimized.

Copy link
Member

commented Oct 30, 2018

Okay, another take on this:

origin="untrusted|trusted|any"

The untrusted is the default. E.g. all the traffic that enters Kong will be untrusted. And all the plugins are by default configured untrusted. It makes sense to run e.g. auth plugins on untrusted connections, e.g. service 1 connecting its own side car proxy (that is untrusted). The traffic can only become trusted when the connection is made in service mesh and authenticated with mutual tls.

The trusted can thus only be the receiving end of the mesh in this simple exercise of SERVICE-A -> KONG-FORWARD-PROXY (a sidecar for SERVICE-A) -> KONG-REVERSE-PROXY (a sidecar for SERVICE-B) -> SERVICE-B. Yes the KONG-REVERSE-PROXY is the trusted and KONG-FORWARD-PROXY is the untrusted.

And then we have any (possibly for zipkin only for now) which just means that plugin runloop will execute the plugin regardless of the source of the request (it doesn't matter if the connection was trusted or untrusted).

Plugin configuration (if the user for some weird reason wants to run it differently than the sane default):
origin="any"

This is how to add dynamic config for origin:

http post :8001/services/service-b/plugins \
  name=key-auth \
  origin=any \
  config.key_names=Kong-Key -f

A new column in plugins table called origin will need to be added. Each plugins will have a sane default for this, e.g. most will be untrusted but some (zipkin) will have any, none will have trusted as of now.

origin could have values:
“unknown”, “mesh”, “any”
“untrusted”, “trusted”, “any”

or perhaps move back to:
run="gateway|mesh|both"

@kikito kikito force-pushed the feat/plugins-mesh-field-1 branch 5 times, most recently from 993808d to f12804b Oct 31, 2018

@kikito kikito force-pushed the feat/plugins-mesh-field-1 branch from 8c62662 to fec1317 Nov 5, 2018

@kikito kikito force-pushed the feat/plugins-mesh-field-1 branch 4 times, most recently from 7b0f95a to c9d2088 Nov 8, 2018

@bungle
Copy link
Member

left a comment

One correction needed.

Show resolved Hide resolved kong/db/migrations/core/001_14_to_15.lua Outdated

kikito added some commits Nov 13, 2018

@kikito kikito force-pushed the feat/plugins-mesh-field-1 branch from c9d2088 to 43bfaa2 Nov 14, 2018

@bungle

bungle approved these changes Nov 14, 2018

@kikito kikito merged commit 237a068 into next Nov 14, 2018

3 checks passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
continuous-integration/travis-ci/push The Travis CI build passed
Details
license/cla All CLA requirements met.

@kikito kikito deleted the feat/plugins-mesh-field-1 branch Nov 14, 2018

kikito added a commit to Kong/kong-plugin-azure-functions that referenced this pull request Nov 21, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.