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

Add links to reference arch in storage class helm values #14572

Closed
wants to merge 3 commits into from

Conversation

im2nguyen
Copy link
Contributor

Description

Add links to reference arch in storage class helm values.

Most customers are using default values for storageclass for Consul helm chart which is not suitable for running enterprise workloads and since Consul is write limited by disk I/O and as per reference architecture it needs disk with minimum of 3000+ IOPS ( even for Dev clusters) this can result in performance issues.

Testing & Reproduction steps

  • In the case of bugs, describe how to replicate
  • If any manual tests were done, document the steps and the conditions to replicate
  • Call out any important/ relevant unit tests, e2e tests or integration tests you have added or are adding

Links

Include any links here that might be helpful for people reviewing your PR (Tickets, GH issues, API docs, external benchmarks, tools docs, etc). If there are none, feel free to delete this section.

Please be mindful not to leak any customer or confidential information. HashiCorp employees may want to use our internal URL shortener to obfuscate links.

PR Checklist

  • updated test coverage
  • external facing docs updated
  • not a security concern

@im2nguyen im2nguyen added type/docs Documentation needs to be created/updated/clarified type/docs-cherrypick labels Sep 12, 2022
@im2nguyen im2nguyen marked this pull request as ready for review September 12, 2022 17:05
@im2nguyen im2nguyen requested a review from a team as a code owner September 12, 2022 17:05
@im2nguyen im2nguyen requested a review from david-yu September 12, 2022 17:06
Copy link
Contributor

@boruszak boruszak left a comment

Choose a reason for hiding this comment

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

Outside of some style guide fixes and what appears to be text that was supposed to be part of a link, these edits LGTM. Approving on behalf of consul-docs.

@@ -295,14 +295,14 @@ Use these links to navigate to a particular top-level stanza.

- `controller` ((#v-global-secretsbackend-vault-controller))

- `tlsCert` ((#v-global-secretsbackend-vault-controller-tlscert)) - Configuration to the Vault Secret that Kubernetes will use on
- `tlsCert` ((#v-global-secretsbackend-vault-controller-tlscert)) - Configuration to the Vault Secret that Kubernetes will use on
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
- `tlsCert` ((#v-global-secretsbackend-vault-controller-tlscert)) - Configuration to the Vault Secret that Kubernetes will use on
- `tlsCert` ((#v-global-secretsbackend-vault-controller-tlscert)) - Configuration to the Vault Secret that Kubernetes uses on

Present tense instead of future tense, per style guide

Kubernetes CRD creation, deletion, and update, to get TLS certificates
used issued from vault to send webhooks to the controller.

- `secretName` ((#v-global-secretsbackend-vault-controller-tlscert-secretname)) (`string: null`) - The Vault secret path that issues TLS certificates for controller
webhooks.

- `caCert` ((#v-global-secretsbackend-vault-controller-cacert)) - Configuration to the Vault Secret that Kubernetes will use on
- `caCert` ((#v-global-secretsbackend-vault-controller-cacert)) - Configuration to the Vault Secret that Kubernetes will use on
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
- `caCert` ((#v-global-secretsbackend-vault-controller-cacert)) - Configuration to the Vault Secret that Kubernetes will use on
- `caCert` ((#v-global-secretsbackend-vault-controller-cacert)) - Configuration to the Vault Secret that Kubernetes uses on

@@ -311,14 +311,14 @@ Use these links to navigate to a particular top-level stanza.

- `connectInject` ((#v-global-secretsbackend-vault-connectinject))

- `caCert` ((#v-global-secretsbackend-vault-connectinject-cacert)) - Configuration to the Vault Secret that Kubernetes will use on
- `caCert` ((#v-global-secretsbackend-vault-connectinject-cacert)) - Configuration to the Vault Secret that Kubernetes will use on
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
- `caCert` ((#v-global-secretsbackend-vault-connectinject-cacert)) - Configuration to the Vault Secret that Kubernetes will use on
- `caCert` ((#v-global-secretsbackend-vault-connectinject-cacert)) - Configuration to the Vault Secret that Kubernetes uses on

Kubernetes pod creation, deletion, and update, to get CA certificates
used issued from vault to send webhooks to the ConnectInject.

- `secretName` ((#v-global-secretsbackend-vault-connectinject-cacert-secretname)) (`string: null`) - The Vault secret path that contains the CA certificate for
Connect Inject webhooks.

- `tlsCert` ((#v-global-secretsbackend-vault-connectinject-tlscert)) - Configuration to the Vault Secret that Kubernetes will use on
- `tlsCert` ((#v-global-secretsbackend-vault-connectinject-tlscert)) - Configuration to the Vault Secret that Kubernetes will use on
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
- `tlsCert` ((#v-global-secretsbackend-vault-connectinject-tlscert)) - Configuration to the Vault Secret that Kubernetes will use on
- `tlsCert` ((#v-global-secretsbackend-vault-connectinject-tlscert)) - Configuration to the Vault Secret that Kubernetes uses on

@@ -564,7 +564,7 @@ Use these links to navigate to a particular top-level stanza.
- `enabled` ((#v-global-openshift-enabled)) (`boolean: false`) - If true, the Helm chart will create necessary configuration for running
its components on OpenShift.

- `consulAPITimeout` ((#v-global-consulapitimeout)) (`string: 5s`) - The time in seconds that the consul API client will wait for a response from
- `consulAPITimeout` ((#v-global-consulapitimeout)) (`string: 5s`) - The time in seconds that the consul API client will wait for a response from
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
- `consulAPITimeout` ((#v-global-consulapitimeout)) (`string: 5s`) - The time in seconds that the consul API client will wait for a response from
- `consulAPITimeout` ((#v-global-consulapitimeout)) (`string: 5s`) - The time in seconds that the consul API client waits for a response from

to be automatically created. For example, to use local
(https://kubernetes.io/docs/concepts/storage/storage-classes/#local)
to be automatically created. For example, to use
local(https://kubernetes.io/docs/concepts/storage/storage-classes/#local)
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
local(https://kubernetes.io/docs/concepts/storage/storage-classes/#local)
[local storage classes](https://kubernetes.io/docs/concepts/storage/storage-classes/#local),

I assume that the intent was to add a link here. Missing brackets + when possible, include multiple words in hyperlinks to make the link text more descriptive.

to be automatically created. For example, to use local
(https://kubernetes.io/docs/concepts/storage/storage-classes/#local)
to be automatically created. For example, to use
local(https://kubernetes.io/docs/concepts/storage/storage-classes/#local)
storage classes, the PersistentVolumeClaims would need to be manually created.
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
storage classes, the PersistentVolumeClaims would need to be manually created.
the PersistentVolumeClaims would need to be manually created.

To match link fix above.

storage classes, the PersistentVolumeClaims would need to be manually created.
A `null` value will use the Kubernetes cluster's default StorageClass. If a default
StorageClass does not exist, you will need to create one.
See https://www.consul.io/docs/install/performance#read-write-tuning for considerations around choosing a
performant storage class.
Refer to the [Read/Write Tuning](https://www.consul.io/docs/install/performance#read-write-tuning)
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
Refer to the [Read/Write Tuning](https://www.consul.io/docs/install/performance#read-write-tuning)
Refer to the [Read/Write Tuning](/docs/install/performance#read-write-tuning)

@@ -1566,7 +1571,7 @@ Use these links to navigate to a particular top-level stanza.
- `disruptionBudget` ((#v-connectinject-disruptionbudget)) - This configures the PodDisruptionBudget (https://kubernetes.io/docs/tasks/run-application/configure-pdb/)
for the service mesh sidecar injector.

- `enabled` ((#v-connectinject-disruptionbudget-enabled)) (`boolean: true`) - This will enable/disable registering a PodDisruptionBudget for the
- `enabled` ((#v-connectinject-disruptionbudget-enabled)) (`boolean: true`) - This will enable/disable registering a PodDisruptionBudget for the
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
- `enabled` ((#v-connectinject-disruptionbudget-enabled)) (`boolean: true`) - This will enable/disable registering a PodDisruptionBudget for the
- `enabled` ((#v-connectinject-disruptionbudget-enabled)) (`boolean: true`) - This value enables or disables a registering a PodDisruptionBudget for the

@@ -1578,7 +1583,7 @@ Use these links to navigate to a particular top-level stanza.

- `cni` ((#v-connectinject-cni)) - Configures consul-cni plugin for Consul Service mesh services

- `enabled` ((#v-connectinject-cni-enabled)) (`boolean: false`) - If true, then all traffic redirection setup will use the consul-cni plugin.
- `enabled` ((#v-connectinject-cni-enabled)) (`boolean: false`) - If true, then all traffic redirection setup will use the consul-cni plugin.
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
- `enabled` ((#v-connectinject-cni-enabled)) (`boolean: false`) - If true, then all traffic redirection setup will use the consul-cni plugin.
- `enabled` ((#v-connectinject-cni-enabled)) (`boolean: false`) - If true, then all traffic redirection setup uses the consul-cni plugin.

@im2nguyen
Copy link
Contributor Author

Hey @boruszak, I'm going to temporarily merge these changes.

Going to go back and update the corresponding ones in consul-k8s since these changes were auto-generated from there. Here's the associated PR from the upstream repo: hashicorp/consul-k8s#1493

There's also a bunch of links that aren't rendering correctly due to helm shenanigans

@kschoche
Copy link
Contributor

driving by with a comment that maybe we should tag consul-docs team for reviews of the source file in consul-k8s repo sometime since it's auto-generated?

@david-yu
Copy link
Contributor

I believe we can close these as these changes are now already reflected on consul.io due to the PR @im2nguyen made to consul-k8s

@david-yu david-yu closed this Sep 28, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
type/docs Documentation needs to be created/updated/clarified
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants