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

Refresh the teleport-cluster Helm guide #25287

Merged
merged 4 commits into from
May 11, 2023

Conversation

ptgott
Copy link
Contributor

@ptgott ptgott commented Apr 27, 2023

Closes #13853

Updated the guide while testing it for v13.

The goals of this change are to:

  • Update the guide for v13 (these are very minor changes consisting of editing example outputs)
  • Simplify the guide
  • Make the guide easier to use

Updating the guide:

  • Remove the video, which is two years old.

Simplifying the guide:

  • Some Tabs components differ only in mentioning an Enterprise-specific namespace instead of an OSS-specific namespace. By using the same namespace for both editions, we can remove these tabs.
  • Add tsh as a prerequisite so we don't need to install it midway through the guide.
  • Remove the SSO instructions: The SSO instructions rely on following other guides to complete, so these aren't really appropriate for this step-by-step guide. The only difference the SSO instructions in this guide offer from other guides is is running tctl on the Auth Service pod via kubectl exec. Since this isn't actually necessary, we can remove these instructions.
  • Refactor out common instructions in the OSS/Enterprise installation tabs
  • Remove the command to get the Proxy Service's load balancer IP, since the get services command already shows this

Make the guide easier to use:

  • Add more specific Enterprise installation steps, including showing how to get the license file.
  • Make the DNS instructions more specific.
  • Add guidance in the Prerequisites for ensuring that your cluster supports Persistent Volumes.
  • Add a warning re: system:masters
  • Add Var components to code snippets so they are easier to copy/paste.

Stray edits:

  • Adjust line widths for easier reviews.
  • Recommend the local Kubenetes guide in the first Admonition, rather than the Docker Compose guide, since readers will already be interested in Kubernetes.

Comment on lines 49 to 80
<Details title="Launching a fresh EKS cluster with eksctl?" open="false">

If you are using [eksctl](https://eksctl.io/) to launch a fresh Amazon Elastic
Kubernetes Service cluster in order to follow this guide, the following
example configuration sets up the EBS CSI driver add-on. Update the cluster
name, version, node group size, and region as required:

```yaml
apiVersion: eksctl.io/v1alpha5
kind: ClusterConfig
metadata:
name: my-cluster
region: us-east-1
version: "1.23"

iam:
withOIDC: true

addons:
- name: aws-ebs-csi-driver
version: v1.11.4-eksbuild.1
attachPolicyARNs:
- arn:aws:iam::aws:policy/service-role/AmazonEBSCSIDriverPolicy

managedNodeGroups:
- name: managed-ng-2
instanceType: t3.medium
minSize: 2
maxSize: 3
```

</Details>
Copy link
Contributor

@hugoShaka hugoShaka Apr 27, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

(non-blocking)

I'm not sure putting eksctl in the hands of unsuspecting new users is good. The tool creates a lot of resources, has complex behaviour when things are going wrong. Cleaning up after eksctl is quite complex.

Do we really want to be in the business of teaching users how to deploy the EKS cluster Rube Goldberg machine?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I added this because I found it helpful when spinning up a quick demo cluster via eksctl, and didn't want users in a similar boat as me to spend as much time as I did going through the AWS docs. I get what you're saying, though. Maybe we can add a red Notice box at the top of this Details box indicating that:

  • This is for demo clusters only
  • You should already be experienced with eksctl

@ptgott
Copy link
Contributor Author

ptgott commented Apr 28, 2023

@alexfornuto This should be ready for copy review

Copy link
Contributor

@alexfornuto alexfornuto left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Generally 👍 but with some suggestions.

Comment on lines 93 to 99
<Admonition title="No cloud provider?" type="tip">

Teleport also supports Kubernetes in on-premise and air-gapped environments. If
you would like to try out Teleport on your local machine, we recommend following
our [local Kubernetes guide](../../try-out-teleport/local-kubernetes.mdx).

<Admonition title="Local-only setups" type="tip">
Teleport also supports Kubernetes in on-premise and air-gapped environments. If you would like to try out Teleport on your local machine, we recommend following our [Docker Compose guide](../../try-out-teleport/docker-compose.mdx).
</Admonition>
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This admonition breaks the indent flow of the prerequisites section. I suggest moving it under the "A Kubernetest cluster..." bullet point and indenting it.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'll remove this Admonition since we have removed the local Kubernetes guide. I'm guessing that orgs interested in Kubernetes in air-gapped environments will be sophisticated enough that removing this message will put them off Teleport.

Comment on lines 241 to 244
|Record Type|Domain Name|Value|
|---|---|---|
|A|teleport.example.com|The IP address of your load balancer|
|A|*.teleport.example.com|The IP address of your load balancer|

</TabItem>
<TabItem label="Domain Name">

|Record Type|Domain Name|Value|
|---|---|---|
|CNAME|teleport.example.com|The domain name of your load balancer|
|CNAME|*.teleport.example.com|The domain name of your load balancer|
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The tables seem like a bit of overkill, since in both tabs they are either A or CNAME records and all have the same value.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I could use prose instead to make this more compact, but I'm not sure this needs to be more compact. For me, having a table makes things easier to keep track of when (for example) I'm in the Route 53 console.

Updated the guide while testing it for v13.

The goals of this change are to:
- Update the guide for v13 (these are very minor changes consisting of
  editing example outputs)
- Simplify the guide
- Make the guide easier to use

Updating the guide:
- Remove the video, which is two years old.

Simplifying the guide:
- Some `Tabs` components differ only in mentioning an
  Enterprise-specific namespace instead of an OSS-specific namespace. By
  using the same namespace for both editions, we can remove these tabs.
- Add tsh as a prerequisite so we don't need to install it midway
  through the guide.
- Remove the SSO instructions: The SSO instructions rely on following
  other guides to complete, so these aren't really appropriate for this
  step-by-step guide. The only difference the SSO instructions in this
  guide offer from other guides is is running `tctl` on the Auth Service
  pod via `kubectl exec`. Since this isn't actually necessary, we can
  remove these instructions.
- Refactor out common instructions in the OSS/Enterprise installation
  tabs
- Remove the command to get the Proxy Service's load balancer IP, since
  the `get services` command already shows this

Make the guide easier to use:
- Add more specific Enterprise installation steps, including showing how
  to get the license file.
- Make the DNS instructions more specific.
- Add guidance in the Prerequisites for ensuring that your cluster
  supports Persistent Volumes.
- Add a warning re: `system:masters`
- Add `Var` components to code snippets so they are easier to
  copy/paste.

Stray edits:
- Adjust line widths for easier reviews.
- Recommend the local Kubenetes guide in the first Admonition, rather
  than the Docker Compose guide, since readers will already be
  interested in Kubernetes.
@ptgott ptgott force-pushed the paul.gottschling/2023-04-teleport-cluster branch from 79430fc to cfbb399 Compare May 11, 2023 14:51
@ptgott ptgott force-pushed the paul.gottschling/2023-04-teleport-cluster branch from cfbb399 to db4585d Compare May 11, 2023 14:58
@ptgott ptgott enabled auto-merge May 11, 2023 14:58
@ptgott ptgott added this pull request to the merge queue May 11, 2023
Merged via the queue into master with commit a954bad May 11, 2023
@ptgott ptgott deleted the paul.gottschling/2023-04-teleport-cluster branch May 11, 2023 15:27
@public-teleport-github-review-bot

@ptgott See the table below for backport results.

Branch Result
branch/v13 Failed

ptgott added a commit that referenced this pull request May 12, 2023
Backports #25287

* Refresh the teleport-cluster Helm guide

Updated the guide while testing it for v13.

The goals of this change are to:
- Update the guide for v13 (these are very minor changes consisting of
  editing example outputs)
- Simplify the guide
- Make the guide easier to use

Updating the guide:
- Remove the video, which is two years old.

Simplifying the guide:
- Some `Tabs` components differ only in mentioning an
  Enterprise-specific namespace instead of an OSS-specific namespace. By
  using the same namespace for both editions, we can remove these tabs.
- Add tsh as a prerequisite so we don't need to install it midway
  through the guide.
- Remove the SSO instructions: The SSO instructions rely on following
  other guides to complete, so these aren't really appropriate for this
  step-by-step guide. The only difference the SSO instructions in this
  guide offer from other guides is is running `tctl` on the Auth Service
  pod via `kubectl exec`. Since this isn't actually necessary, we can
  remove these instructions.
- Refactor out common instructions in the OSS/Enterprise installation
  tabs
- Remove the command to get the Proxy Service's load balancer IP, since
  the `get services` command already shows this

Make the guide easier to use:
- Add more specific Enterprise installation steps, including showing how
  to get the license file.
- Make the DNS instructions more specific.
- Add guidance in the Prerequisites for ensuring that your cluster
  supports Persistent Volumes.
- Add a warning re: `system:masters`
- Add `Var` components to code snippets so they are easier to
  copy/paste.

Stray edits:
- Adjust line widths for easier reviews.
- Recommend the local Kubenetes guide in the first Admonition, rather
  than the Docker Compose guide, since readers will already be
  interested in Kubernetes.

* Respond to hugoShaka feedback

* Linter fixes

* Respond to alexfornuto feedback
ptgott added a commit that referenced this pull request May 16, 2023
Backports #25287

* Refresh the teleport-cluster Helm guide

Updated the guide while testing it for v13.

The goals of this change are to:
- Update the guide for v13 (these are very minor changes consisting of
  editing example outputs)
- Simplify the guide
- Make the guide easier to use

Updating the guide:
- Remove the video, which is two years old.

Simplifying the guide:
- Some `Tabs` components differ only in mentioning an
  Enterprise-specific namespace instead of an OSS-specific namespace. By
  using the same namespace for both editions, we can remove these tabs.
- Add tsh as a prerequisite so we don't need to install it midway
  through the guide.
- Remove the SSO instructions: The SSO instructions rely on following
  other guides to complete, so these aren't really appropriate for this
  step-by-step guide. The only difference the SSO instructions in this
  guide offer from other guides is is running `tctl` on the Auth Service
  pod via `kubectl exec`. Since this isn't actually necessary, we can
  remove these instructions.
- Refactor out common instructions in the OSS/Enterprise installation
  tabs
- Remove the command to get the Proxy Service's load balancer IP, since
  the `get services` command already shows this

Make the guide easier to use:
- Add more specific Enterprise installation steps, including showing how
  to get the license file.
- Make the DNS instructions more specific.
- Add guidance in the Prerequisites for ensuring that your cluster
  supports Persistent Volumes.
- Add a warning re: `system:masters`
- Add `Var` components to code snippets so they are easier to
  copy/paste.

Stray edits:
- Adjust line widths for easier reviews.
- Recommend the local Kubenetes guide in the first Admonition, rather
  than the Docker Compose guide, since readers will already be
  interested in Kubernetes.

* Respond to hugoShaka feedback

* Linter fixes

* Respond to alexfornuto feedback
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add the eksctl Helm guide in the discussion forum to the docs
3 participants