Skip to content

Giji in prod#1534

Open
YustinaKvr wants to merge 26 commits intomainfrom
giji_first_build
Open

Giji in prod#1534
YustinaKvr wants to merge 26 commits intomainfrom
giji_first_build

Conversation

@YustinaKvr
Copy link
Contributor

  1. Vault agent file
  2. Cronjobs

Copy link
Contributor

@LukasCuperDT LukasCuperDT left a comment

Choose a reason for hiding this comment

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

The PR demonstrates good security practices and infrastructure-as-code principles, but has several critical issues that would prevent successful deployment:

  • Health probes incompatible with CronJob workload type
  • Missing volume mounts for Vault secrets injection
  • Unclear/potentially incorrect cron schedules

Remove all HTTP health probes from CronJob definitions

  • Clarify and fix cron schedules
  • Add volume/volumeMount configuration for Vault secrets
  • Fix container naming inconsistencies
  • Add resource requests/limits
  • Fix vault-agent.hcl typos (DB_CSV → DB_NAME)
  • Add namespace resource or document prerequisites

Schedule Clarification:
Are the September 11 schedules intentional? What's the expected frequency?
Vault Integration: How is the Vault Agent sidecar being injected? Via mutation webhook or manual configuration?
Testing: Have these CronJobs been tested ?

Copy link

@mistressxalexis mistressxalexis left a comment

Choose a reason for hiding this comment

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

``

Copy link

@mistressxalexis mistressxalexis left a comment

Choose a reason for hiding this comment

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

Y

Copy link
Contributor

@LukasCuperDT LukasCuperDT left a comment

Choose a reason for hiding this comment

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

Please, test the entire solution after application of the new changes suggestions

Copy link
Contributor

@LukasCuperDT LukasCuperDT left a comment

Choose a reason for hiding this comment

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

PR Review: Giji in prod (#1534)

Critical Issues

1. Kustomization resource conflict / missing files
The overlay kustomization.yaml references individual files (sa.yaml, secret.yaml, cronjob-*.yaml) and ../../base. If these files exist in both the overlay and the base, you will get duplicate resource errors. If they only exist in base, the local references will fail. Additionally, secret.yaml is referenced but not included in this PR - this will cause a deployment failure.

2. Base directory missing kustomization.yaml
The overlay references ../../base as a resource, but base has no kustomization.yaml. Kustomize requires one when referencing a directory. This will error on kustomize build.


Security Concerns

3. readOnlyRootFilesystem: false
All 3 containers have this set to false. Best practice for CronJobs is true unless the app writes to the filesystem. You already mount an emptyDir for /tmp, so consider setting this to true.

4. imagePullPolicy: IfNotPresent with :latest tag in base
Base templates use image quay.io/opentelekomcloud/giji:latest with imagePullPolicy: IfNotPresent. While the overlay overrides the tag to 1.0.0, the base is misleading and could cause stale images if used standalone. Consider setting the base to Always or removing the tag.


Code Quality

5. Massive duplication across CronJobs
All 3 cronjobs share ~70 identical lines of Vault annotations (lines 13-81). This is a maintenance risk. Consider using a Kustomize patchesStrategicMerge or components for the shared Vault config.


Verdict: Please fix the kustomization structure issues (missing base kustomization.yaml, missing secret.yaml, potential duplicates), test with kustomize build, and address the duplication before merging.

Copy link
Contributor

@LukasCuperDT LukasCuperDT left a comment

Choose a reason for hiding this comment

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

  1. Kustomization resource filenames likely mismatched

The base kustomization.yaml references:

resources:
  - cronjob-bulk-import.yaml
  - cronjob-bug-postgres.yaml
  - cronjob-demand-postgres.yaml

But the actual files in the PR are named cronjob-1-bulk-import.yaml, cronjob-2-bug-postgres.yaml, cronjob-3-demand-postgres.yaml. If the "Fixes" commit renamed them, fine — but this needs verification with kustomize build.

  1. Vault annotations patch is NOT applied to any CronJob

The patchesStrategicMerge section lists vault-annotations-patch.yaml, but that patch has name: placeholder — it won't match any actual CronJob resource. The patches section with explicit targets only applies bug-demand-config-patch.yaml and demand-config-patch.yaml, not vault-annotations-patch.yaml. This means none of the CronJobs will get:

vault.hashicorp.com/agent-inject: "true"
vault.hashicorp.com/role: "giji"
Jira cert/key injection
The giji-env environment template
The master_component exports
This is a deployment-breaking issue — the CronJobs will run without any Vault secrets.

Fix: Add vault-annotations-patch.yaml to the patches section targeting all 3 CronJobs:

patches:
  - target:
      kind: CronJob
    path: vault-annotations-patch.yaml
  - target:
      kind: CronJob
      name: giji-bulk-import
    path: bug-demand-config-patch.yaml
  # ...existing entries...
  1. Mixed/deprecated patching — remove patchesStrategicMerge

patchesStrategicMerge is deprecated in newer Kustomize versions and the name: placeholder entries won't match anything anyway. The entire patchesStrategicMerge block should be removed — all patching should use the patches section with explicit targets.

  1. ClusterRoleBinding with system:auth-delegator — this is needed for Vault Kubernetes auth but grants significant cluster-level permissions. A brief comment explaining why would help future reviewers.

  2. Change imagePullPolicy to IfNotPresent.

ptichoid and others added 7 commits February 19, 2026 11:12
- Fix critical typo: gigi-bulk-import -> giji-bulk-import in kustomization patch target
- Set imagePullPolicy to Always in base CronJobs to prevent stale cached images
- Add successfulJobsHistoryLimit/failedJobsHistoryLimit (3) to all CronJobs
- Add backoffLimit (3) to prevent excessive retries
- Add startingDeadlineSeconds (300s/600s) for missed schedule handling
- Remove redundant namespace declaration from prod overlay
* Convert Giji from Vault Agent Injector to ArgoCD Vault Plugin (AVP)

- Replace Vault Agent sidecar annotations with AVP Secret manifests
- Create secret-giji-env.yaml with AVP placeholders for shared config
- Create secret-bug-config.yaml and secret-demand-config.yaml for job-specific config
- Create secret-jira-certs.yaml for Jira PEM cert/key files
- Update CronJobs to use envFrom secretRefs and cert volume mounts
- Remove ClusterRoleBinding for system:auth-delegator (not needed with AVP)
- Remove vault-annotations-patch.yaml, bug-demand-config-patch.yaml, demand-config-patch.yaml
- Simplify kustomization.yaml: remove all patch directives, add secret resources

* Add imagePullSecrets to giji ServiceAccount for quay.io access

* Add quay.io pull secret via AVP for giji image pulls
LukasCuperDT
LukasCuperDT previously approved these changes Feb 19, 2026
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.

4 participants