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

[SPIKE] storage controller need to support ca cert for self sign signed certificate #61

Closed
Jooho opened this issue Jul 26, 2023 · 17 comments · Fixed by #62 or #63
Closed

[SPIKE] storage controller need to support ca cert for self sign signed certificate #61

Jooho opened this issue Jul 26, 2023 · 17 comments · Fixed by #62 or #63
Assignees
Labels
kind/enhancement New feature or request

Comments

@Jooho
Copy link
Contributor

Jooho commented Jul 26, 2023

Upstream ModelMesh provides a feature to set a self-signed certificate, but there is no input field for this in the ODH UI. As a workaround, attempting to update this manually was not successful because the odh-model-controller kept continuously updating the storage-config secret.

Therefore, we need two modifications through the odh-model-controller:

  1. Logic to handle the AWS_CA_BUNDLE parameter.
  2. Logic to exclude secrets that have opendatahub.io/managed=false annotation from reconcile.

These changes will allow us to address the issue effectively.

For the UI integration(@lucferbux), this will be the secret example:

apiVersion: v1
data:
  AWS_ACCESS_KEY_ID: VEhFQUNDRVNTS0VZ
  AWS_CA_BUNDLE: LS0tLS1CRUdJTiBDRVJUSUZJQ0FURS0tLS0tCk1JSUY2VENDQTlHZ0F3SUJBZ0lVYTRwekx2ZG5ZWFdabDNydnFSaHF0UUdxOFhVd0RRWUpLb1pJaHZjTkFRRUwKQlFBd2ZERUxNQWtHQTFVRUJoTUNRMEV4RFRBTEJnTlZCQWdNQkZSeWRXVXhEekFOQmdOVkJBY01CazFKVEZSUApUakVQTUEwR0ExVUVDZ3dHVWtWRVNFRlVNUXd3Q2dZRFZRUUxEQU5UUTBVeEVEQU9CZ05WQkFNTUIxSnZiM1FnClEwRXhIREFhQmdrcWhraUc5dzBCQ1FFV0RYUmxjM1JBZEdWemRDNWpiMjB3SGhjTk1qTXdOekkzTWpBeE16TTIKV2hjTk5ETXdOekl5TWpBeE16TTJXakI4TVFzd0NRWURWUVFHRXdKRFFURU5NQXNHQTFVRUNBd0VWSEoxWlRFUApNQTBHQTFVRUJ3d0dUVWxNVkU5T01ROHdEUVlEVlFRS0RBWlNSVVJJUVZReEREQUtCZ05WQkFzTUExTkRSVEVRCk1BNEdBMVVFQXd3SFVtOXZkQ0JEUVRFY01Cb0dDU3FHU0liM0RRRUpBUllOZEdWemRFQjBaWE4wTG1OdmJUQ0MKQWlJd0RRWUpLb1pJaHZjTkFRRUJCUUFEZ2dJUEFEQ0NBZ29DZ2dJQkFMWTFYWkEzbmxrbSszMjBIMDZUM1FEdgozVjVNL0VDZWQ0Yk5BNDdKalZDcWlrMTFNdUU3ZEd2UlpjTnFZeDhlVHlqL0FvdEw4NXZGTTAzdk1IZ2NJU2UxCnkwdktLeC9odDJKeFBWRitVNmxzS0QyTllUV1U0dFpmOFZMSjYyM0VLUGxSZmY0ZlF4bFd6VkJ1R3VpVXIxZGMKaHlPUjlpeWtVV0FmNllXYmk5L2J3RWlUYnloTG5VZm1oazVkN29TeE5mNC9KZEx4Nkh1M1doNk9HZWI4ODdZMApDcjlFODM3YitSeWpNQ3poYWw1K29pamtkS1dNcUVoUzh5K2dwcXVRTnVPWEkrMTJ1TGwyZDFDM0ZHVzd2MWIrCjMvL2QxeXliNnNpWFhvZC9ObGZBRGtuVnlxZXhXbWMzTWVFMXRRV2JTNGxPaVhjanFlRmp0QlVtaXhuaFNFQTcKRmN4ZjF6SXZsbzRWRE5Pa2QxdCtDSzJOWitkT1NLaHh4eHhWWk1sVEdWMjdMd0cxUzRkTHZ5Unk0V0hQVlJaZgo3WUgwL2FCRWdBSmtFSndxRzFWWEg3OEliVm03azVxL3FIVGYxekJsREZBeVVKbVI0a3J5clhobHhXWTlZZ1ByCmdGZnFFMm8rMC9wWU9CKzI4VWFsbXNuVXl3VDBFNUFBYmZCRk9BTmU2QUpzVWtrU044dW1BRnBFM09tTlI4NTEKQkFmMWFxRXdZMXMvMGpNSjNudVdzUU4zelNEUUViVWtqNlB6MFhKMUpEa1NjUE9oRjRnVDh1R2JVenZsTXF2TgpFTW9mZGhHN3ArU09WeEEyV1h0SVMweGlENDV4anJBNW9hbEEveHAxZmlocG1HbGVZYnhqOUhWU013L3ZhRUE1CmhVSi9nQVo2N2VpYVNNdjd6UnpqQWdNQkFBR2pZekJoTUIwR0ExVWREZ1FXQkJSbTU1aFVWWWVuWFNrc1RQM1kKSjBORTRJRVpRVEFmQmdOVkhTTUVHREFXZ0JSbTU1aFVWWWVuWFNrc1RQM1lKME5FNElFWlFUQVBCZ05WSFJNQgpBZjhFQlRBREFRSC9NQTRHQTFVZER3RUIvd1FFQXdJQmhqQU5CZ2txaGtpRzl3MEJBUXNGQUFPQ0FnRUFQU2p0Cm5yM1RqbkpGMEoyZjFmK2Y3S28yYXpNWnE1VWdZcm82WE5GUUw3SjdKZEZDRDFaTU5ZSlZJLzhxQjVqTkRNaEgKdXVZUnhCN1J5REJnUkZYSHhucC9UN0hiRy9XT1dNQ3U0UWd2ZXZPMVlsN1VQNFVvVFdab0VQYmxta1dJUlh6MQoxVWpKeVhLOFlGUERFc1pWT0NHL29oTWM0MXZJMzJKcy91MlJmOUxKNVVyeU5LSTRzOHYvQjI5Z3FBYUVtL1hwCjN6am9QaHBBcks2OEQ5bjVGbU9ZSU1SOStHUWpnM0dWSGN4c0Ricnp4ZEp2L0x4emwyN3Zxc25aQ21maitudTMKZURieUZhYlJISU51QzFjUXVjYlJCeGh5cXptY3A2OTFjeHkxZzBWTVpETE5xRllwVUJ4MDcxOEFPNExMSkI0eQplL1ZMRXF3M1ZpS2U5UkJqTGhXZnZabWsvNHNSMkZ4aW5wT1BadGtOUGJITU0rNHBzNHRMbTNMdmsxeFpDMFFtCkJSdlcwbEVmc1BRcVdpRUFpQlJTSDB3bGw4ZEUrcHV2bEo1YVhJdTUwdXlPcmxNUzk1b3RBM3FJaXBOejhoS2wKbVladTU4UzdoRGl4MmVQZFg4S1ZQbVR2MFZNUUcxaTFsVTZJOE56MHV5R0ZJcVRHUHNYd014UE8zak1Bano0YgpGeTk0RzFnMm0rOGcxc3lsRTJuUGs1ZlFhNDJnYVNRdGY2RllPdmppTlF6K0hyK2MyMUxkdGFxb2xqa3h2Y2N2CkJqaGY2QTZpdDVvcVpMUWd0RVdEU0dmbGxDK3RkN0NwQmdQYmpzUzV1M1BCQWltQy9zZ0RnY2dCYlhWL2IyYUIKU2kxK1ljcnRGZUoyY0d0NDJpSHVrSnNSOWRSMW55Q052RmR6QXp3PQotLS0tLUVORCBDRVJUSUZJQ0FURS0tLS0tCg==
  AWS_DEFAULT_REGION: dGVzZXQz
  AWS_S3_BUCKET: bW9kZWxtZXNoLWV4YW1wbGUtbW9kZWxz
  AWS_S3_ENDPOINT: aHR0cHM6Ly9taW5pby5taW5pby5zdmM6OTAwMA==
  AWS_SECRET_ACCESS_KEY: ZDdjOWM3M2NjYTE2YmRjOTJkODBlYTg4NmY4YTE1OTIzYzJhMDZmYTE0NWE5ZWYyY2EzNThhMTk1MjNlN2IzNQ==
  test: YWJjCg==
kind: Secret
metadata:
  annotations:
    opendatahub.io/connection-type: s3
    openshift.io/display-name: dc1
  labels:
    opendatahub.io/dashboard: "true"
    opendatahub.io/managed: "true"
  name: aws-connection-dc1
  namespace: modelmesh-serving
type: Opaque
@Jooho
Copy link
Contributor Author

Jooho commented Jul 28, 2023

#61

@lucferbux
Copy link

Hi @Jooho Does this mean that once this is implemented we are deprecating the storage-config secret that was handling secrets?

And does that secret need to be typed by the user? Just in case we need to adapt the dashboard for that. Thanks for pinging me out, we can start a conversation to start implementing this.

@Jooho
Copy link
Contributor Author

Jooho commented Aug 3, 2023

@lucferbux No, it just adds 1 more param to data connection UI.

@lucferbux
Copy link

@lucferbux No, it just adds 1 more param to data connection UI.

Ok, I guess that's fairly straightforward but we will need ux for this. Are we supposed to be able to upload the cert as a file or something?

@Jooho
Copy link
Contributor Author

Jooho commented Aug 3, 2023

It should be the same as openshift secret menu. It supports copy&paste and upload a file.

@lucferbux
Copy link

cc @vconzola @kywalker-rh I'm pinging you guys here, once model serving finish the spike we can start opening our trackers.

@lucferbux
Copy link

@vconzola @kywalker-rh I think we might wanna start to take a look.
cc @alexcreasy @andrewballantyne @christianvogt I'll ping you guys to be aware of it too.

@andrewballantyne
Copy link
Member

We have an on-going point about CA bundles. I would like for this to be more holistic. DSPA, Model Serving, Elyra Notebooks... they are all suffering from the same CA issue -- I wonder if we can handle this more at the ODH Deployment level rather than at each component.

cc @jwforres

@Jooho
Copy link
Contributor Author

Jooho commented Aug 28, 2023

I agree with @andrewballantyne.
What I expected is that If there is a centralized configmap to set this self signed certificate in OpenDataHub operator level, each component can use it by default but if each data connection specifies a unique ca, then it can overwrite the default one.
For example, from modelserving perspective, A model can be in A registry using A cert, B model can be in B registry using B cert. ( This may be a very exaggerated case, but it is possible.).
If both cert(A,B) were generated by 1 root cert and the root ca is set by opendatahub operater level, then it will be automatically set for data-connection by default. However, if they are using different root cert, then it could be controlled by each data-connection.

@kywalker-rh
Copy link

Thanks for pinging me @lucferbux. I just created a UX issue to start looking at it (opendatahub-io/odh-dashboard#1825). Since this is Projects/Admin related I have assigned @xianli123 to it for now, but that could change depending on workload.

@xianli123
Copy link

xianli123 commented Aug 29, 2023

@kywalker-rh I will keep an eye on this issue.

@Jooho
Copy link
Contributor Author

Jooho commented Nov 22, 2023

Implementation is done in serving team. The rest part is UI. This issue is just for tracking purpose so do not add any sprint label.

@andrewballantyne
Copy link
Member

For posterity, I mentioned this on the slack thread. I'm a little confused on what the UI should do here... I think a cluster solution is what we were after at the last AC. Adding the CA_BUNDLE to the Data Connection is a bandaid and not intended to be a solution afaik. We are happy to take on the effort if it is needed as a real solution and not a stop-gap, UX has some plans for S3-compatible flows (standard AWS S3 data connections wouldn't have this field).

@lucferbux
Copy link

For posterity, I mentioned this on the slack thread. I'm a little confused on what the UI should do here... I think a cluster solution is what we were after at the last AC. Adding the CA_BUNDLE to the Data Connection is a bandaid and not intended to be a solution afaik. We are happy to take on the effort if it is needed as a real solution and not a stop-gap, UX has some plans for S3-compatible flows (standard AWS S3 data connections wouldn't have this field).

I agree with andrew, this solution could be misleading, since Data Connections are used in other resources (pipelines, notebooks) and having that field would require us to have a special treatment for model serving. With no context, if we allow custom certs in data connections the user would assume it will work with everything. We can add this as a middle step, but we would need a broader solution soon.

@Jooho
Copy link
Contributor Author

Jooho commented Nov 27, 2023

From modelserving perspective, we are ok to use any other possible ways because odh-model-controller will convert the value into secret. But one thing that I concerned about is the behavior of the field certificate for kserve and modelmesh is different. For kserve, it is the path of certificate in the container but for modelmesh, it is certificate content.

After KServe is merged, I think we need to send a similar pr to modelmesh to make it the same way.

@israel-hdez
Copy link
Contributor

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment