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

Reference sensitive data from secrets and config maps #1024

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

mkaesz-wandb
Copy link
Contributor

Description

  • Added secrets refs for MySQL, Bucket and License
  • Added config map refs for Custom CAs
  • Removed some typos

Ticket

Does this PR fix an existing issue? If yes, provide a link here.

@mkaesz-wandb mkaesz-wandb requested a review from a team as a code owner January 24, 2025 14:21
```

**Important:**
If using a ConfigMap, each key in the ConfigMap must end with `.crt` (e.g., `my-cert.crt` or `ca-cert1.crt`). This naming convention is required for `update-ca-certificates` to parse and add each certificate to the system CA store.
Copy link
Contributor

Choose a reason for hiding this comment

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

🚫 [vale] reported by reviewdog 🐶
[Google.Latin] Use 'for example' instead of 'e.g.'.

Copy link
Contributor

@mdlinville mdlinville Jan 24, 2025

Choose a reason for hiding this comment

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

Please fix this and following instances of the same feedback. I took two years of Latin in high school, but most of our readers probably did not.

```

**Important:**
Each key in the ConfigMap must end with `.crt` (e.g., `my-cert.crt` or `ca-cert1.crt`). This naming convention is required for `update-ca-certificates` to parse and add each certificate to the system CA store.
Copy link
Contributor

Choose a reason for hiding this comment

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

🚫 [vale] reported by reviewdog 🐶
[Google.Latin] Use 'for example' instead of 'e.g.'.

Copy link

cloudflare-workers-and-pages bot commented Jan 24, 2025

Deploying docs with  Cloudflare Pages  Cloudflare Pages

Latest commit: 515e8c0
Status: ✅  Deploy successful!
Preview URL: https://cfbf0296.docodile.pages.dev
Branch Preview URL: https://operator-secret-refs.docodile.pages.dev

View logs

Copy link
Contributor

@flamarion flamarion left a comment

Choose a reason for hiding this comment

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

Not sure if the suggested names are the best ones, but I suggest we standardize the names and do not perfix with wandb

Copy link
Contributor

@flamarion flamarion left a comment

Choose a reason for hiding this comment

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

LGTM 🚢

@mdlinville mdlinville self-requested a review January 24, 2025 23:27
@@ -465,28 +465,67 @@ global:
secretKey: HDKYe4Q...JAp1YyjysnX
```

The `kmsKey` must be `null`.
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm confused -- if it must be null, then why have the key at all? In the example for AWS above, the kmsKey is the empty string. Must it be empty for S3 and null for S3-compatible storage that is not on AWS?

@@ -709,10 +748,13 @@ global:
oidc:
clientId: ""
secret: ""
# Only include if your IdP requires it.
Copy link
Contributor

Choose a reason for hiding this comment

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

Do we know of any major IdPs that do or don't require it? Okta? AAD? GCP?

authMethod: ""
issuer: ""
```

`authMethod` is optional.
Copy link
Contributor

Choose a reason for hiding this comment

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

The comment previously seems to imply that it is not optional, either your IdP requires it (it's not optional) or it doesn't (then does it even evaluate the field?)

metadata:
name: custom-ca-certs
data:
ca-cert1.crt: |
Copy link
Contributor

Choose a reason for hiding this comment

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

Do we have any best practices around compound certs? For example, they can become unwieldy and poor performing in a clustered environment with many nodes, if they share a compound certificate.

-----END CERTIFICATE-----
```

**Important:**
Copy link
Contributor

Choose a reason for hiding this comment

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

Also, this is no longer the righrt way to do an admonition . Instead, use the Hugo shortcode:

{{% alert %}}
...
{{% /alert %}}

@@ -784,7 +852,7 @@ The only valid value for `runAsGroup:` is `0`. Any other value is an error.
{{% /alert %}}


To configure the application pod, add a section `app` to your configuration:
Example: To configure the application pod, add a section `app` to your configuration:
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
Example: To configure the application pod, add a section `app` to your configuration:
For example, to configure the application pod, add a section `app` to your configuration:

@@ -807,7 +875,7 @@ app:
readOnlyRootFilesystem: false
allowPrivilegeEscalation: false
```
The same concept applies to `console`, `weave`, `otel`, `weave-trace`, `flat-run-fields-updater` and `parquet`.
The same concept applies to `console`, `weave`, `weave-trace` and `parquet`.
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
The same concept applies to `console`, `weave`, `weave-trace` and `parquet`.
The same concept applies to `console`, `weave`, `weave-trace` and `parquet`.

-----END CERTIFICATE-----
```

**Important:**
Copy link
Contributor

Choose a reason for hiding this comment

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

As above, please also update this admonition.

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.

3 participants