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

Introduce helm-docs #984

Merged
merged 13 commits into from
Jun 6, 2024
Merged

Introduce helm-docs #984

merged 13 commits into from
Jun 6, 2024

Conversation

v0lkan
Copy link
Contributor

@v0lkan v0lkan commented Jun 6, 2024

Helm Docs

Description

This PR introduces helm-docs-generated Helm Charts documentation.

Changes

  • Created a helm-docs template for the main README.md
  • Updated existing Helm Charts values.yaml files to be helm-docs-compatible.
  • Added some additional atributes to the helm charts for TCx OpenShit deployment compatibility.
  • Other minor changes.

Test Policy Compliance

Since this was mainly a docs update, no major testing was needed.

I still ran the existing unit an integration tests to ensure nothing has regressed.

Code Quality

  • I have followed the coding standards for this project.
  • I have performed a self-review of my code.
  • My code is well-commented, particularly in areas that may be difficult
    to understand.

Documentation

  • I have made corresponding changes to the documentation (if applicable).
  • I have updated any relevant READMEs or wiki pages.

Additional Comments

This is an initial starting point.

helm-docs are pretty configurable; we can design our own doc templates based on our needs, and have a single source of truth (i.e. the inline comments in the docs, rather than multiple sources of truths: the READMEs and the docs)

The initial-generated docs look good-enough for now.

Checklist

Before you submit this PR, please make sure:

  • You have read the contributing guidelines and
    especially the test policy.
  • You have thoroughly tested your changes.
  • You have followed all the contributing guidelines for this project.
  • You understand and agree that your contributions will be publicly available
    under the project's license.

By submitting this pull request, you confirm that my contribution is made under
the terms of the project's license and that you have the authority to grant
these rights.


Thank you for your contribution to VMware Secrets Manager
🐢⚡️!

v0lkan added 12 commits June 4, 2024 19:01
Signed-off-by: Volkan Ozcelik <[email protected]>
Signed-off-by: Volkan Ozcelik <[email protected]>
Signed-off-by: Volkan Ozcelik <[email protected]>
Signed-off-by: Volkan Ozcelik <[email protected]>
Signed-off-by: Volkan Ozcelik <[email protected]>
Signed-off-by: Volkan Ozcelik <[email protected]>
Signed-off-by: Volkan Ozcelik <[email protected]>
Signed-of-by: Volkan Ozcelik <[email protected]>
Signed-off-by: Volkan Ozcelik <[email protected]>
Signed-off-by: Volkan Ozcelik <[email protected]>
Signed-of-by: Volkan Ozcelik <[email protected]>
@v0lkan v0lkan requested a review from abhishek44sharma June 6, 2024 15:53
@v0lkan v0lkan self-assigned this Jun 6, 2024
@v0lkan v0lkan requested a review from BulldromeQ as a code owner June 6, 2024 15:53
@vmwclabot
Copy link

@v0lkan, you must sign every commit in this pull request acknowledging our Developer Certificate of Origin before your changes are merged. This can be done by adding Signed-off-by: John Doe <[email protected]> to the last line of each Git commit message. The e-mail address used to sign must match the e-mail address of the Git author. Click here to view the Developer Certificate of Origin agreement.

@v0lkan
Copy link
Contributor Author

v0lkan commented Jun 6, 2024

cc: @BulldromeQ and @abhishek44sharma , since this is mainly helm-charts.

I’ll annotate the important changes in the code.

I ran the tests, it looks okay; I’ll merge it, but feel free to amend any comments you might have: we can create a follow-up PR for those.

@BulldromeQ I know you are on PTO, so this is more like a “check this out when you get back” reminder rather than “you must check it right now” ping :D — Enjoy your vacation, and have a great family time!

// it will be used as the buffer size.
// If the environment variable is set but cannot be parsed as an integer,
// the default buffer size is used.
func K8sSecretDeleteBufferSizeForSafe() int {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

We are not using this function anymore since our k8s secrets generation logic has changed.

Right now, k8s secrets are created, updated, but not deleted.

Later, we might think about how to handle the “deletion” use case.
There are issues about that too.

## Environment Variables

> **Using VSecM Helm Charts**?
>
> If you are using [**VMware Secrets Manager Helm Charts**][helm-charts],
> you can configure these environment variables using the `values.yaml` file.
>
> You can check out the details [from the official VSecM Helm Charts
> documentation][helm-charts].
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Added pointers to the official helm docs too.


### 11. Add a Snapshot of the Current Documentation
If you have updated inline documentation in helm charts, make sure to reflect
the changes by running `./hack/helm-docs.sh`.
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This function just calls helm-docs with a tiny wrapper to warn the user to install helm-docs if the binary cannot be found.

clusters.
* Introduced `helm-docs` to automatically augment the documentation with the
Helm chart's values.yaml.
* Added the ability to deploy VSecM without SPIRE Controller Manager. In this
Copy link
Contributor Author

Choose a reason for hiding this comment

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

The “VSecM w/o SCM” and “VSecM w/o clusterspiffeids” use cases still need integration testing.

In theory, it should work; and in theory it should make some of the users’ (read: Tomasso) life much better :) .

@@ -1,12 +1,15 @@
# VMware Secrets Manager (VSecM) Helm Chart
[![Artifact Hub](https://img.shields.io/endpoint?url=https://artifacthub.io/badge/repository/vsecm)](https://artifacthub.io/packages/helm/vsecm/vsecm)

VMware Secrets Manager keeps your secrets secret. With VSecM, you can rest assured
Copy link
Contributor Author

Choose a reason for hiding this comment

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

README is autogenerated from a template.

@@ -0,0 +1,104 @@
# VMware Secrets Manager (VSecM) Helm Chart
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is the template that generates the README.md.

I caried over as much docs from the original README as it makes sense.

@@ -0,0 +1,30 @@
# keystone
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Autogenerated.

@@ -62,6 +62,7 @@ spec:
#
{{- $safeInitEndpointUrlSet := false }}
{{- $safeInitSpiffeIdPrefixSet := false }}
{{- $workloadInitSpiffeIdPrefixSet := false }}
Copy link
Contributor Author

Choose a reason for hiding this comment

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

workload spiffeid prefixes need to be configurable too.

Long story, but mostly because OpenShift, or more properly because certain OpenShift configurations.

@@ -88,6 +92,10 @@ spec:
- name: VSECM_SAFE_SPIFFEID_PREFIX
value: {{ .Values.global.vsecm.safeSpiffeIdPrefix | quote }}
{{- end }}
{{- if not $workloadInitSpiffeIdPrefixSet }}
- name: VSECM_WORKLOAD_SPIFFEID_PREFIX
Copy link
Contributor Author

Choose a reason for hiding this comment

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

If not overridden in the local char, get it from the global values.

/ns/{{`{{ .PodMeta.Namespace }}`}}\
/sa/{{`{{ .PodSpec.ServiceAccountName }}`}}\
/n/{{`{{ .PodMeta.Name }}`}}"
spiffeIDTemplate: {{ .Values.global.vsecm.keystoneSpiffeIdTemplate }}
Copy link
Contributor Author

Choose a reason for hiding this comment

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

spiffeIDTemplates come from the global values too right now.

This is again useful for cases where all of these need to be configured to follow a certain pattern where it does not alight with what VSecM uses by default (i.e.
instead of domain/workload/etc/etc domain/n/$namespace/s/$serviceAccount (similar to how Istio assigns spiffe ids)

- name: VSECM_LOG_LEVEL
value: "7"
- name: VSECM_WORKLOAD_SPIFFEID_PREFIX
value: "spiffe://vsecm.com/workload/"
# -- The interval (in milliseconds) that the VSecM Init Container will poll
Copy link
Contributor Author

Choose a reason for hiding this comment

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

These are mostly massaging the documentation to align with helm-docs format with as minimal changes as possible.

We can always improve this later down the line.

podSecurityContext: {}
# fsGroup: 2000

securityContext: {}
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Removed; no-one was using it.

@@ -0,0 +1,51 @@
# safe
Copy link
Contributor Author

Choose a reason for hiding this comment

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

autogenerated.

@@ -8,20 +8,20 @@
# >/' SPDX-License-Identifier: BSD-2-Clause
# */

{{- if .Values.global.useClusterSpiffeIds }}
Copy link
Contributor Author

Choose a reason for hiding this comment

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

For cases that the operator wants to create ClusterSPIFFEIds off-cycle.

(I am, again, looking at you OpenShift).

@@ -85,6 +86,9 @@ spec:
{{- if eq .name "VSECM_SENTINEL_SPIFFEID_PREFIX" }}
{{- $sentinelSpiffeIdPrefixSet = true }}
{{- end }}
{{- if eq .name "VSECM_WORKLOAD_SPIFFEID_PREFIX" }}
{{- $sentinelSpiffeIdPrefixSet = true }}
Copy link
Contributor Author

Choose a reason for hiding this comment

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

this is a logic error; need to fix.

@@ -0,0 +1,36 @@
# sentinel
Copy link
Contributor Author

Choose a reason for hiding this comment

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

autogenerated.

@@ -0,0 +1,41 @@
# spire
Copy link
Contributor Author

Choose a reason for hiding this comment

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

autogenerated.

Signed-off-by: Volkan Ozcelik <[email protected]>
@vmwclabot
Copy link

@v0lkan, you must sign every commit in this pull request acknowledging our Developer Certificate of Origin before your changes are merged. This can be done by adding Signed-off-by: John Doe <[email protected]> to the last line of each Git commit message. The e-mail address used to sign must match the e-mail address of the Git author. Click here to view the Developer Certificate of Origin agreement.

@v0lkan v0lkan merged commit a8b8634 into main Jun 6, 2024
@v0lkan v0lkan deleted the ovolkan/helm branch June 6, 2024 16:17
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants