-
Notifications
You must be signed in to change notification settings - Fork 87
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
Fixes to enable Admiralty on OpenShift #134
base: master
Are you sure you want to change the base?
Conversation
hfwen0502
commented
Jan 28, 2022
- Helm Chart: Fixed the RBAC changes to work with OpenShift. The changes have also been tested on Kubernetes clusters.
- Documentation: Added a file explaining how to create a kubeconfig secret for OpenShift.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you for submitting this pull request.
docs/tutorials/ocp-ibm.md
Outdated
|
||
The [quick start guide](https://admiralty.io/docs/quick_start) provides clear instructions how to use Admiralty on Kubernetes clusters. The only | ||
thing you need to pay special attention to is how to create a kubeconfig secret that would work in your OpenShift cluster on IBM Cloud. This tutorial will | ||
guide you how to create the kubeconfig secret when you use the Red Hat OpenShift on IBM Cloud service. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Does this only work between ROKS clusters or would it also work to connect any source cluster (e.g., IKS, kind, etc.) to a target ROKS cluster?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The ROKS cluster will work with any IKS cluster. I have tested this using one ROKS cluster and one IKS cluster.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Okay, please update the doc to make that clear.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
doc is updated.
user: | ||
token: <service account token> | ||
``` | ||
The fields, client-certificate and client-key, are being removed and certificate-authority-data and token fields are added. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could you use jq to edit the downloaded kubeconfig, like in the quick start guide, to make this more foolproof/automatisable?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sure. I can provide the jq command. Shall I create another PR ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No, please update this PR to use jq.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You now get the config twice, and the CA cert twice... this could be simplified.
Actually, I'm thinking this should become part of the quick start page itself. Using Tabs/TabItem, the user could select between kind and Red Hat OpenShift on IBM Cloud. (We'd add GKE/EKS/AKS tabs too later.) What do you think?
update based on the feedback from Adrien Trouillaud
user: | ||
token: <service account token> | ||
``` | ||
The fields, client-certificate and client-key, are being removed and certificate-authority-data and token fields are added. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You now get the config twice, and the CA cert twice... this could be simplified.
Actually, I'm thinking this should become part of the quick start page itself. Using Tabs/TabItem, the user could select between kind and Red Hat OpenShift on IBM Cloud. (We'd add GKE/EKS/AKS tabs too later.) What do you think?
For the suggestions to incorporate OpenShift in the quick start guide, I am not sure how we would easily incorporate that in. The quick start guide just focused on clusters created by kind. I do not understand your comments about getting CA and config twice. Users can just include those commands to create a new kubeconfig secret based on the original one. They do not need to manually prepare a modified kubeconfig. The part to compare the original kubeconfig and the modified kubeconfig is just for the explanation purpose. |
You'd also provide RHOKS equivalents for
As is, explanations read like instructions, which is confusing. For example:
|
Ok. Now I understand the comments about getting CA and config twice. Certainly can improve it. There is no simple CLI command to create an OpenShift cluster on IBM Cloud. The best way I would recommend is to do it through the web console. That's why I included a doc link there regarding how to create OCP on IBM Cloud. |
I don't know if it makes sense to use a long command like this:
|
Yes, something like that. We're about to release Admiralty 0.15 and I'd like to make it compatible with OpenShift. Since the doc change might take a while, could you please create a separate PR for the chart RBAC change, so I can merge that already? |
Sure. This is the new pull request: #144. |