Conversation
![github-code-quality[bot]](https://avatars.githubusercontent.com/u/9919?s=60&v=4){kind=small}
This reverts commit 4b03f28.
CasLubbers
left a comment
CasLubbers
left a comment
There was a problem hiding this comment.
Also really good improvmenets in here. I didn't saw any changes for the settings. Is that correct? I don't know from the top of my mind if we can change any secret values in the settings.
src/utils/sealedSecretUtils.ts
Outdated
| } | ||
|
|
||
| export async function encryptSecretValue(pem: string, namespace: string, value: string): Promise<string> { | ||
| const { encryptSecretItem } = await import('@linode/kubeseal-encrypt') |
There was a problem hiding this comment.
Does this import need to be in the function?
src/otomi-stack.ts
Outdated
| this.sessionId = sessionId ?? 'main' | ||
| } | ||
|
|
||
| private static sealedSecretToUserData(manifest: SealedSecretManifestResponse): UserSecretData { |
There was a problem hiding this comment.
Static functions should mostly be avoided. In this case its better to move this function outside of this class even this file because otomi-stack is already quite big
src/otomi-stack.ts
Outdated
| } as UserSecretData | ||
| } | ||
|
|
||
| private async listUserSecretData(): Promise<UserSecretData[]> { |
There was a problem hiding this comment.
In the future we could also opt for reading the users directly from the k8s cluster. Then we can remove this code outside of the otomi-stack
| const { metadata } = data | ||
|
|
||
| // Server-side encryption fallback: if any encryptedData values are plain text, encrypt them | ||
| if (data.spec.encryptedData && Object.keys(data.spec.encryptedData).length > 0) { |
There was a problem hiding this comment.
How does this check if the values are in plain text? Now it checks the length is that correct?
src/otomi-stack.ts
Outdated
| const username = (otomi?.git?.username ?? '') as string | ||
| const password = (otomi?.git?.password ?? '') as string | ||
| const { cluster } = this.getSettings(['cluster']) | ||
| const username = env.GIT_USER |
There was a problem hiding this comment.
I don't think I saw that we pass down the git user through the enviroment variables. We should adjust apl-core
CasLubbers
left a comment
CasLubbers
left a comment
There was a problem hiding this comment.
Only some nice to have changes to make the code more readable. Rest looks good to me
| for (const [, content] of files) { | ||
| settings[name] = content?.spec || content | ||
| // Merge sealed secret encrypted data back into settings at their original dot-paths | ||
| const valuesSchema = await getValuesSchema() |
There was a problem hiding this comment.
Would be good to put this fetch of the sealed secrets in a function with a clear name of what it does
| } | ||
|
|
||
| settings[settingId] = removeBlankAttributes(updatedSettingsData[settingId] as Record<string, any>) | ||
| // Extract secrets from settings data and store as SealedSecret |
There was a problem hiding this comment.
This is al really complex code. It would be good to put it in to small functions with clear domain naming. So we can understand better what separate pieces are doing.
| // Server-side encryption fallback: ensureEncryptedData checks each value using isEncryptedValue(), | ||
| // which detects plain text by verifying that kubeseal ciphertext is always a long (200+ chars) base64 string. | ||
| // Any value that is shorter or not valid base64 is treated as plain text and encrypted server-side. | ||
| if (data.spec.encryptedData && Object.keys(data.spec.encryptedData).length > 0) { |
There was a problem hiding this comment.
I think creating a const for this magic number would make it more understandable for me
| const username = keycloak?.values?.adminUsername as string | ||
| const password = otomi?.adminPassword as string | ||
| existingUsersEmail = await getKeycloakUsers(keycloakBaseUrl, realm, username, password) | ||
| const platformSecrets = await getSecretValues('otomi-platform-secrets', 'apl-secrets') |
There was a problem hiding this comment.
Magic strings. Good to make constants
3 participants
📌 Summary
PRs: apl-core | apl-tasks