Operator API - A Home Grown alternative

[k-wall]

I'm not sure that 'Gateway API flavoured' approach for modelling ingress is the right one for Kroxylicious Operator.
I think that our ingress API should work with Gateway APIs (and OpenShift Ingress), rather than be one itself. So, this
has prompted me to think about an alternative API design.

In this design we have a Proxy and ProxyIngress (responsibility of the infra admin), and a VirtualCluster and the FIlter (responsibility of the application developer).

[tombentley]

It would watch the strimzi resource in order to know n

If we're going to go this route then ading this functionality to the proxy is something we can progress in parallel to bikeshedding on this API.

[k-wall]

You mean have the proxy probe the target cluster and discover the broker topology and somehow have the operator discover that from the proxy? REST API? I don't think that's completely straightforward - I see a cold start issue.

1. Cluster gets started (3 brokers + bootstrap route)
2. Proxy gets started. Proxy doesn't know how many brokers cluster has.
3. Operator establishes proxy bootstrap route.
4. Client connects through proxy to bootstrap, proxy forwards connection.
5. Proxy learns cluster has 3 brokers.
6. Operator learns (polling?) the Proxy that the Cluster has three brokers.
7. Operator establishes broker routes.

Until we've reached step 7, we'll see connection failures.

[k-wall]

Not too happy to be surfacing infrastructure type stuff to the developer. My use case to enable the serving cert feature. Maybe that could be done in another way - explicit - way?

[robobario]

Might need more of an idea how we would use serving certs. My read of it is we could potentially use them for in-cluster TLS and maybe in combination with openshift routes using TLS re-encryption?

So if we have N brokers, we would have N Services all annotated for serving certs. This would generate a PEM/cert for each of the N services. We configure the proxy to load all the generated PEMs into that virtual cluster (some work required to take multiple PEM files and construct a single KeyStore).

So I think it's pretty painful, you need to be able to target a secret name to each of the services maybe using a pattern. Might be better to have the Operator handle this for you with another Ingress strategy and have the operator look after secret names for the user. The end result could be pretty cool though, secure OpenShift Routes for off-cluster without having to set up cert-manager, and secure in-cluster access.

The off-cluster routes would be using the OpenShift ingress wildcard certs and would be publicly trusted

[k-wall]

Might need more of an idea how we would use serving certs. My read of it is we could potentially use them for in-cluster TLS and maybe in combination with openshift routes using TLS re-encryption?

I don't think they have a role to play with OpenShift Routes in our use-case. Kroxylicious needs passthrough behaviour in order to get the SNI. I don't believe reencypt has a way to propagate the original SNI (and if it did, it would fail the hostname check).

Reencrypt works with HTTP because of the Host: header. I don't think it works with Kafka.

[k-wall]

Might need more of an idea how we would use serving certs.

The use-case is for on-cluster services secured with TLS.

The OpenShift cluster operator takes the responsibility for automatically creating a certificate and periodically rolling it. You gesture to the OpenShift cluster operator that you want a certificate with an annotation added to the service. The cluster operator creates a secret containing the key-material. Your operand needs to serve that certificate. The client use https://docs.openshift.com/container-platform/4.16/security/certificates/service-serving-certificate.html#add-service-certificate-configmap_service-serving-certificate so that it trusts the cluster operator's CA.

So if we have N brokers, we would have N Services all annotated for serving certs.

There's a choice there.

1. We could create n+1 services. Each has the annotation. The cluster operator create n+1 secrets. We'd somehow mount those into Kroxylicious and give the SNI handler the smarts to serve the right certificate. The client will be connecting through different services bootstrap-myvcluster, broker0-myvcluster, etc.. so we can then use SNI to route the traffic within the Proxy.

2. The alternative it is create 1 service (with n+1 port mappings) and use port based routing. That means there's only 1 service to annotate and one 1 certificate to mount. The client now connects to myvcluster:2000 and gets back brokers myvcluster:2001..myvcluster:200n.

I think either way works. In the proposal, I was suggesting the second. I said we need a new PortPerBroker scheme in order to make sure the port/broker mapping says stable across cluster topology changes.

Reflecting about the API design, I think it would be better to have an explicit switch for this feature, rather than having the Developer have to set the annotation via the infrastructure field.

clusterIp:
  openshiftFeatures:
     generateServiceCertificate: true

then the needs for the infrastructure field on the VirtualCluster goes away. The operator would take responsibility to add annotation to the clusterIp's service(s) and wire up the generated secrets.

[robobario]

. I don't believe reencypt has a way to propagate the original SNI (and if it did, it would fail the hostname check).

I was more wondering if we would get the internal SNI. So if the service is broker-1.svc.cluster.local, and the route is broker-1.ingress.domain would HAProxy send broker-1.svc.cluster.local as the SNI on connection to Kroxy?

[robobario]

another kink, the tls.key generated by the serving cert feature is in PKCS1 format. Netty would need bouncycastle to handle it. edit: or we could DIY a conversion to PKCS8

[robobario]

Also I think you're right, it's hard to find it documented but I think re-encrypt routes expect HTTPS and can't handle TCP. So I think passthrough is the only termination type that's relevant to us.

[k-wall]

Or does JDK now support PKCS1?? https://bugs.openjdk.org/browse/JDK-8023980

edit: that seems to be true, but Netty still doesn't support it without BC.

Anyway, I think service cert integration would be low in the priority list.

[k-wall]

I'm tempted to call this a ProxyListener.

[SamBarker]

I certainly think Listener is better than ingress, though does listener clash with Kafka listeners (for our user base)? Maybe proxyAccess

[k-wall]

Naming is so hard!

I certainly think Listener is better than ingress, though does listener clash with Kafka listeners

Isn't it the proxy's analogue for kafka's listener? So maybe the clash is exactly what we want.

Maybe proxyAccess

I think I prefer either Ingress or Listener over Access. Access makes with think of ACLs.

[k-wall]

Why don't the existing PortPerBroker schemes work? The issue is during a reconciliation where the number of nodes in target cluster has changed, some proxy replicas will be still using previous port assignments, whereas some will be using the new. That risks that the client might connect to the wrong broker (or even wrong cluster).

[robobario]

Do you mean that the port assignment would be stateful? That the operator would consider the previous deployed state to ensure new nodes receive a previously un-allocated port? (or at least track the last-allocated-port and keep rolling forward)

[SamBarker]

Does life get easier if we just mandate SNI (for day 1) and then worry about stateful port assignment later?

[k-wall]

Do you mean that the port assignment would be stateful? That the operator would consider the previous deployed state to ensure new nodes receive a previously un-allocated port? (or at least track the last-allocated-port and keep rolling forward)

Yes, exactly. I'd initially thought that the operator would just use the PortPerBrokerClusterNetworkAddress scheme. I then got thinking about the case where the broker topology of the target cluster changed (or even a new virtual cluster added). If proxy is replicas > 1, I think all kinds of bad things happen depending on the relative ordering of the port assignments in the services being updated and the replicas being restarted. I worried that :1234 which previously pointed at broker 3 now points to broker 4, or even a entirely different cluster. (i.e. crossed-lines in old telephone terminology). How's a client with a connection to :1234 going to respond? And which client are we talking about (java, sarama)? That lead me to the thought that port/broker assignments need to be sticky.

So that got me thinking about a new PortPerBroker schema that allows the Operator to precisely control the port/broker assignments (its config would literally be a broker-id / port number mapping). It could then use the previous port/broker assignment state as input to the next state.

Does life get easier if we just mandate SNI (for day 1) and then worry about stateful port assignment later?

That'd crossed my mind too.

We could just change the implementation roadmap - implement ClusterIP/TLS first (using SNI), then come back to ClusterIP/TCP later.

The disadvantages:

1. it makes https://docs.openshift.com/container-platform/4.16/security/certificates/service-serving-certificate.html#add-service-certificate-configmap_service-serving-certificate slightly harder - more secrets to juggle (see the conversation above).

2. Being able to proxy a cluster without TLS sure makes a demo easy to follow.

[k-wall]

This could use SNI based routing, but I figure since we've had to use port based routing for TCP, we may as well use port based routing for TLS too.

[tombentley]

Can you elaborate for each of these mechanisms what resources are expected to already exist/are the responsibility of the user, vs what resources will be managed by the operator?

[k-wall]

I've updated the "Personas / Responsibilies" section to make it clear what the Developer needs to provide. I've also called it out in the comments within the API. It is also reiterated in the Worked Examples where it talks about what the Developer brings and what the Operator needs to do.

[tombentley]

I wonder whether calling both the targetCluster and the ingress property tls is the right choice. (I know that's what we did for the proxy config itself, but I have by doubts there too). It means you have to look to see what the container object is to know what you're configuring. I can imagine people finding that confusing.

[SamBarker]

For me the main motivation is clarifying that your configuring TLS in each of these places and you have exactly the same set of TLS options in each. There is nothing more annoying that failing to configure property X in context B when it works exactly as you want in Context A.

[k-wall]

In the proxy config, I think you make a valid point.

  demo:
    tls:  <---   tls setting applying to the virtual cluster (TLS server role)
       x: ...
    targetCluster:
      bootstrapServers: localhost:9092
      tls: <---   tls setting applying to the upstream (TLS client role)
         x: ..

I'm not convinced we have same issue in this proposal as the two tls nodes appear in separate branches of the tree. I think there is much less potential for confusion.

ingresses:
   name: foo
   tls:  <---   tls setting applying to the virtual cluster (TLS server role)
   
targetCluster:
   tls: <---   tls setting applying to the upstream (TLS client role)

we could rename them ingressTls: and targetTls: but is that really the kube way?

WDYAT?

[tombentley]

Is there a reason we need to name this?

[k-wall]

I think we borrowed the idea from the Strimzi model when RangeAwarePortPerNodeCluster scheme was implemented.

Naming the range isn't required for the function. We could remove it.

[robobario]

We need the openshift routes to match the virtual cluster broker address pattern. Would it be the Operator's responsibility to set up the Route hosts, or should the users configure something like broker-${nodeId}.abc.def.${ingressDomain}?

[k-wall]

We need the openshift routes to match the virtual cluster broker address pattern.

I don't think this is something we'd surface to users.

I was imagining it would be the Operator's responsibility. It would create routes with names in a well-known form:

broker-${nodeId}-${virtualcluster}
bootstrap-${virtualcluster}

and the Ingress Operator would assign the hostnames.

broker-${nodeId}-${virtualcluster}-${namespace}.${ingressDomain}
bootstrap-${virtualcluster}-${namespace}.${ingressDomain}

We'd need to document how the name will be formed so that the user can generate their certificate.

There's a potential to support vanity names (by setting the Routes spec.host field), but I think we'd leave that for an RFE.

[robobario]

TLS config and target cluster location feels more like an Infrastructure admin concern

[k-wall]

I agree that TLS config (I mean cipher suite and TLS protocols) feels like an Infrastructure admin concern. I had wondered, for the downstream, to push that into the ProxyIngress resource.

I'm not sure about the target cluster location - that to me feels more like an Application Developer concern. As an app developer, i know which topic I'm trying to encrypt. I don't think that's something an Infrastructure admin would know.

So there's a bit of a tension.. I ended up plonking all the TLS stuff in the virtualcluster.

[robobario]

Yep, I can see that trying to separate out frontend and backend tls config would end up with an unwieldy amount of CRs required

[robobario]

If ProxyIngress is associated with one Proxy and they're both infrastructure-admin oriented why not combine them in the one CR?

[k-wall]

We definitely could.. But there are counterarguments:

• For OpenShift and Gateway, the user might need to use more than one controller (you could have two operators implementing the Gateway API on the cluster. so many instances of ProxyIngress would be required.
• CRs seem to work best when they focus tightly on a single concern. Big APIs get unwieldy.
• Decoupling might give us more wiggle/innovation room.

[robobario]

So virtualClusterName is an Operator only concept that lets one Ingress be used for multiple VirtualClusters?

[k-wall]

$(vitrtualClusterName) would be understood by Kroxylicious, rather than the Operator. Both $(vitrtualClusterName) and $(nodeId) would be passed down as literals into the proxy config file. $(vitrtualClusterName) would be substituted for the virtual cluster's name. $(nodeId) is as-is.

btw. I did wonder about pushing bootstrapAddressPattern and brokerAddressPattern into the VirtualCluster. That would do away with the need introduce the new pattern.

[SamBarker]

Thanks @k-wall without digging into the details this feels like the use case persona driven API I'd been hoping to see.

[SamBarker]

I'm wondering if TargetCluster should be a CR as well less from a technical point of view but more of a who manages it.

As in the developer persona here actually know about Strimzi resources? Or is that something that should be abstracted away by the infra owner? Arguably that is do able via resource path but I think there is a conversation to be had here. Especially due to the leak of nodeIdRanges (probably unavoidable at the moment)

[k-wall]

I'm wondering if TargetCluster should be a CR as well less from a technical point of view but more of a who manages it.

Yeah, I wondered about making TargetClusterRef its own CR too. I went through an iteration where I had a TargetClusterRef CR.

targetCluster:
  clusterRef:   <-- points at the cluster being proxied.
      kind: Kafka|TargetClusterRef
      group: kafka.strimzi.io|proxy.kroxylicious.io
      name: mycluster
   tls:
      clientCert:...

kind:  TargetClusterRef
spec:
   bootstrap: xxx:1234
   protocol: TLS
   nodeIdRange: ....
   tls:
      trustAnchorsRef:...

The nice thing is that ClusterRef resources could be shared by many VirtualClusters, which seems quite nice for Multi Tenancy, for instance.

I'm not sure it is really clean cut who would be responsible for the ClusterRef object. There are times when it seems Developer, others when it Infra Admin.

[SamBarker]

I'm not sure it is really clean cut who would be responsible for the ClusterRef object. There are times when it seems Developer, others when it Infra Admin.

When would it be a developer responsibility? Isn't it always infra (even if the infra person is the same developer waring a different hat?)

[k-wall]

I was thinking about the case where the Kafka Cluster being proxied was entirely the responsibility of the Developer team. So a use-case where a app dev team is standing up a Kafka service for other in their organisation we say, schema enforcement.

[k-wall]

I experimented with a spec only TargetClusterRef in #53

[SamBarker]

I was thinking about the case where the Kafka Cluster being proxied was entirely the responsibility of the Developer team. So a use-case where a app dev team is standing up a Kafka service for other in their organisation we say, schema enforcement.

I think we are on the same page then, but to me in that case the app dev team are waring the infra hat as well as the dev hat, and the proxied cluster is still an infra concern.

[SamBarker]

For me the main motivation is clarifying that your configuring TLS in each of these places and you have exactly the same set of TLS options in each. There is nothing more annoying that failing to configure property X in context B when it works exactly as you want in Context A.

[SamBarker]

What is your thinking here around the schema for Kind: filter? Are all filters going to have standard generic schema? e.g. type + arbitrary config?

Or should we be offering a higher level CR for Filters shipped by the project?

I started wondering about this from the angle of what happens if we want to have multiple filter instances pointed at the same KMS possibly even a corporate standard KMS instance that the infrastructure persona manages. Should the filter also support a kmsRef or just a configRef? that points to things configured once?

[k-wall]

I haven't thought about this aspect at all this proposal! The Filter definition you see is what we have on main right now.

In https://github.com/kroxylicious/design/blob/playing-with-operator-ingress-ideas/proposals/kroxylicious-operator-api-alternative.md#phased-high-level-implementation-plan I've said 'allow filters to accept secrets'. Personally I feel that @robobario "secret notation" is good enough. I don't think we want to start thinking about making KMSs toplevel right now.

[SamBarker]

I certainly think Listener is better than ingress, though does listener clash with Kafka listeners (for our user base)? Maybe proxyAccess

[SamBarker]

Does life get easier if we just mandate SNI (for day 1) and then worry about stateful port assignment later?

[k-wall]

Need metrics: etc

[k-wall]

I think the management port is an internal concern, so doesn't need to be surfaced in the API.
I think all we need is a switch to turn metrics on or off. It should defect to on.
I don't see any reason to make the health probes configurable at this stage.