test: platform secrets as sealed secrets (PoC)#2956
test: platform secrets as sealed secrets (PoC)#2956ferruhcihan wants to merge 49 commits intomainfrom
Conversation
| envFrom: | ||
| - secretRef: | ||
| name: {{ include "otomi-api.fullname" . }} | ||
| {{- with .Values.existingSecret }} |
There was a problem hiding this comment.
please add default value for the existingSecret to the values.yaml of this chart.
CasLubbers
left a comment
CasLubbers
left a comment
There was a problem hiding this comment.
Really a lot of work impressive! The general idea is good. I have placed some comments on all the kubectl commands. We should not use those. Otherwise we can run in to issues during the apply.
And one thing I really disliked is the hardcoded mapping in the sealed-secrets.ts. If you want we can maybe think of something better together.
And you will have some merge conflicts. I moved all of the git stuff in to one place.
| // the bootstrap phase before install) may have set the URL with unresolved placeholder | ||
| // passwords because K8s secrets didn't exist yet. Now that secrets are decrypted, | ||
| // we need to update the URL with the real credentials. | ||
| cd(env.ENV_DIR) |
There was a problem hiding this comment.
We should try to avoid changing directories in code. With zx you can specify a directory the code should execute on:
await $`git remote set-url origin ${remote}`
.cwd(env.ENV_DIR)
.nothrow()
.quiet()
| d.info(`Waiting for ${secretsToWait.size} sealed secrets to be decrypted`) | ||
| const start = Date.now() | ||
|
|
||
| while (Date.now() - start < timeoutMs) { |
There was a problem hiding this comment.
To be more aligned with the rest of this repository you could use the async retry library. And then you throw an error if pending.length is not null
| labels: | ||
| name: ${namespace}` | ||
|
|
||
| await deps.$`echo ${nsYaml} | kubectl apply -f -`.nothrow().quiet() |
There was a problem hiding this comment.
We should not use kubectl apply anymore. Only use the kubernetes/client-node library.
| * Ensure a namespace exists. If it doesn't exist, create it with proper labels. | ||
| * This avoids overwriting labels on existing namespaces that were created by k8s-raw.gotmpl. | ||
| */ | ||
| export const ensureNamespaceExists = async (namespace: string, deps = { $, terminal }): Promise<void> => { |
There was a problem hiding this comment.
This is a function I would not expect to exist in the sealed-secrets.ts
I would move this one to the k8s.ts
| * Mapping from secret path prefix to target Kubernetes namespace. | ||
| * Dynamic entries like `teamConfig.{teamId}` are handled separately. | ||
| */ | ||
| export const APP_NAMESPACE_MAP: Record<string, string> = { |
There was a problem hiding this comment.
I would really try to avoid such hardcoded maps in code. It's really prone to human error.
Is this something we can maybe specify in the values-schema?
| const d = deps.terminal(`common:${cmdName}:restartSealedSecretsController`) | ||
| d.info('Restarting sealed-secrets controller to ensure correct key is used') | ||
|
|
||
| const result = await deps.$`kubectl rollout restart deployment/sealed-secrets -n sealed-secrets`.nothrow().quiet() |
There was a problem hiding this comment.
You can replace this with the kubernetes/client-node library by patching deployment annotation kubectl.kubernetes.io/restartedAt
| } | ||
|
|
||
| d.info('Waiting for sealed-secrets controller rollout') | ||
| const waitResult = await deps.$`kubectl rollout status deployment/sealed-secrets -n sealed-secrets --timeout=120s` |
There was a problem hiding this comment.
You can replace this with the kubernetes/client-node library by patching deployment annotation kubectl.kubernetes.io/restartedAt
| private async verifyGitRepoHasMainBranch(): Promise<boolean> { | ||
| try { | ||
| // Get credentials from K8s secret (created by Helm at deploy time) | ||
| const creds = await getK8sSecret('gitea-credentials', 'apl-operator') |
There was a problem hiding this comment.
To get the git config please use the functions in git-config.ts getStoredGitRepoConfig
There was a problem hiding this comment.
Otherwise this will not work for BYO Git.
| branch: string | ||
| } | ||
|
|
||
| export const getRepo = async (values: Record<string, any>, deps = { getK8sSecret, terminal }): Promise<Repo> => { |
There was a problem hiding this comment.
I think you need to update your branch. I removed this function
| const otomiGit = values?.otomi?.git | ||
| const agePrivateKey = values?.kms?.sops?.age?.privateKey | ||
| this.d.debug('Reading git credentials from K8s Secret') | ||
| const giteaSecrets = await getK8sSecret('gitea-secrets', 'sealed-secrets') |
There was a problem hiding this comment.
I moved all the code for retrieving git data to: git-config.ts
| pipeline: otomi-task-teams | ||
| values: | ||
| - ../values/team-ns/team-ns.gotmpl | ||
| - name: team-secrets-{{ $teamId }} |
There was a problem hiding this comment.
This could be a part of team-ns chart right?
| const result = await $`git ls-remote --exit-code --heads ${repoUrl} main`.nothrow().quiet() | ||
| return result.exitCode === 0 | ||
| } catch { | ||
| // If we can't check (e.g. gitea not ready yet), assume it's fine |
There was a problem hiding this comment.
I am not entirely convinced. What if user provides incorrect credentials?
| * Ensure a namespace exists. If it doesn't exist, create it with proper labels. | ||
| * This avoids overwriting labels on existing namespaces that were created by k8s-raw.gotmpl. | ||
| */ | ||
| export const ensureNamespaceExists = async (namespace: string, deps = { $, terminal }): Promise<void> => { |
There was a problem hiding this comment.
consider using the k8s library with patch that can eiter created or update.
4 participants
📌 Summary
🔍 Reviewer Notes
🧹 Checklist