-
Notifications
You must be signed in to change notification settings - Fork 181
ECE and ECH to ECK remote clusters setup updated for API keys #3849
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
base: main
Are you sure you want to change the base?
Conversation
🔍 Preview links for changed docs |
shainaraskas
left a comment
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.
structurally lgtm. left some minor notes :)
|
|
||
| #### Establish trust in the {{ece}} cluster [ece_establish_trust_in_the_elastic_cloud_enterprise_cluster] | ||
|
|
||
| 1. Save the ECK CA certificate to a file. For a cluster named `quickstart`, run: |
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.
this is old - not sure if you were planning on changing it - but this pattern of linking deeply into the other tutorial is not ideal. I realize this is the deprecated approach so it's not as important to tidy up, but just stating this for the record.
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.
tbh I didn't want to lose time with the deprecated method.
deploy-manage/remote-clusters/_snippets/eck_rcs_connect_intro.md
Outdated
Show resolved
Hide resolved
deploy-manage/remote-clusters/_snippets/eck_expose_transport.md
Outdated
Show resolved
Hide resolved
Co-authored-by: shainaraskas <[email protected]>
✅ Vale Linting ResultsNo issues found on modified lines! |
shainaraskas
left a comment
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.
lgtm. couple comments to take or leave
Co-authored-by: shainaraskas <[email protected]>
|
Added a sentence for |
barkbay
left a comment
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.
Nice work! Thanks Edu 🙇
I think the spec section in deploy-manage/remote-clusters/_snippets/eck_rcs_expose.md is missing, LGTM otherwise.
(not fully tested, let me know if you'd like me to run additional tests)
| 1. On cloud providers that support external load balancers, setting the type to `LoadBalancer` provisions a load balancer for your service. Alternatively, expose the service `<cluster-name>-es-remote-cluster` through one of the Kubernetes Ingress controllers that support TCP services. | ||
| :::: | ||
|
|
||
| ::::{applies-item} eck: ga 3.0 |
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.
Should it be 3.2?
| ::::{applies-item} eck: ga 3.0 | |
| ::::{applies-item} eck: ga 3.2 |
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 text of that tab stars with In ECK 3.2 and earlier.... I was proposing 3.0 as that should imply 3.0+, but I think 3.2 would work too, so I'll double check with Shaina and I'll change it without issues.
Anyway besides if we label this as 3.2 or 3.0 I've a quick question for you, @barkbay :)
As you can see we are suggesting 2 tabs here:
- A 3.2 (or 3.0) tab explaining that the users cannot customize the service but they can create their own.
- A 3.3+ tab (rendered in the docs as
ECK: plannedbecause 3.3 is not released yet) explaining that the user will be able to customize the service directly in the manifest.
Do you think it's ok to publish both ways now even if 3.3 is not released yet? IMO and considering it will be rendered as planned, it shouldn't be a big deal, but if you prefer we can just publish the "current" approach and change the docs at a later stage after releasing 3.3.
Whatever you prefer :)
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.
Do you think it's ok to publish both ways now even if 3.3 is not released yet? IMO and considering it will be rendered as planned, it shouldn't be a big deal
SGTM 👍
Co-authored-by: Michael Morello <[email protected]>
This PR introduces the Remote Clusters -> API Key security model configuration for the following use cases:
TLS certificate-based authentication method (deprecated) has been left untouched, except of a minor step to expose the transport service of the destination ECK cluster, which was missing.
The procedure has been fully tested.
All based on snippets as the content of both pages is almost identital.
Relates to #1645
In future PRs we will handle the opposite use cases: ECK --> external clusters (self-managed, ECH/ECE, etc) setup.
Reviewing requests --> I'd like first a review from style and writing point of view and afterward we'll request a review from ECK devs in case they want to tweak or change anything.