kserve / modelmesh-serving

Controller for ModelMesh
Apache License 2.0
205 stars 115 forks source link

tls key and cert for etcd should be copied to user namespace along with etcd config #273

Open lizzzcai opened 2 years ago

lizzzcai commented 2 years ago

Is your feature request related to a problem? If so, please describe.

When tls is enabled for etcd, user need to provide the following model-serving-etcd.

apiVersion: v1
kind: Secret
metadata:
  name: model-serving-etcd
  namespace: modelmesh-serving
type: Opaque
data:
  etcd_connection: xxx
  ca.crt: xxxxxxxx
  tls.crt: xxxxxxxxx
  tls.key: xxxxxxxxx

where etcd_connection contains:

# json string
{
  "endpoints": "https://etcd.modelmesh-serving.svc.cluster.local:2379",
  "root_prefix": "modelmesh-serving",
  "userid": "root",
  "password": "xxxxx",
  "certificate_file": "ca.crt",
  "client_key_file": "tls.key",
  "client_certificate_file": "tls.crt"
}

However, the model-serving-etcd in user namespace only contains etcd_connection, ref. (to make it works, tls cert and key and ectd_connection have to in a single model-serving-etcd)

Another limitation in model-serving-etcd is that etcd_connection is a json string, it is hard to refer the value (like user id and password) via valueFrom, and I have to maintain two sets of secret, one for modelmesh, another one for etcd.

Describe your proposed solution

Options:

  1. Copy the the whole model-serving-etcd secret (including tls cert, key and ca and others) to user namespace. (However, it seems like the current modelmesh controller will not update the secret to the user namespace when the secret is updated in the root namespace)
  2. Separate the tls secret from the model-serving-etcd, only keep the tls secret name in the model-serving-etcd for reference, user has to sync the tls secret to user namespace manually.

Personally I prefer option 1 if the model mesh controller is able to sync the updated secret to the user namespace automatically, and unpack the etcd_connection as key-value pairs under data.

Describe alternatives you have considered

Additional context

njhill commented 2 years ago

Thanks @lizzzcai, I think I recall this coming up before, I agree this should be fixed. The low-hanging fix would be to make sure the entire secret is copied, we can do that first.

And we can look at better syncing (with watch) if that's not currently done.

I'm not sure about decomposing the parameters into individual key-values, keeping those in sync might be a bit fragile.

I do have some work in progress to support separate secrets as discussed in https://github.com/kserve/modelmesh-serving/issues/204. Not sure how soon I'll get a chance to finish that and get it in though.

cc @chinhuang007

lizzzcai commented 2 years ago

Thanks @njhill .

The low-hanging fix would be to make sure the entire secret is copied, we can do that first.

This can help but usually we are using cert-manager to manage the secret, so the idea in #204 probably will be a better option. (separate the tls secret from etcd secret and modelmesh help to copy the tls secret to user namespace [optional] )