feat: ensure server side apply to mitigate the last applied annotation overflow - #3452
Open
j-zimnowoda wants to merge 29 commits into
Open
feat: ensure server side apply to mitigate the last applied annotation overflow#3452j-zimnowoda wants to merge 29 commits into
j-zimnowoda wants to merge 29 commits into
Conversation
![github-code-quality[bot]](https://avatars.githubusercontent.com/u/9919?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Pull request overview
This PR aims to mitigate Kubernetes metadata.annotations 256KiB overflow (notably kubectl.kubernetes.io/last-applied-configuration) by ensuring server-side apply is used and by proactively stripping large last-applied annotations that can wedge ArgoCD sync/bootstraps on long-lived clusters.
Changes:
- Adds
--serverSide=trueto the default Helmfile sync args. - Introduces a pre-flight step in
apply-as-appsto striplast-applied-configurationfrom CRDs/ConfigMaps. - Expands unit tests to cover ArgoCD Application SSA sync options and the stripping behavior.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 5 comments.
| File | Description |
|---|---|
| src/common/hf.ts | Extends Helmfile sync args to attempt server-side apply behavior during sync. |
| src/cmd/apply-as-apps.ts | Adds last-applied annotation stripping and adjusts ArgoCD Application manifest generation behavior. |
| src/cmd/apply-as-apps.test.ts | Adds/updates tests for sync option handling and annotation stripping behavior. |
Comment on lines
+169
to
173
| export const getArgocdCoreAppManifest = ( | ||
| release: HelmRelease, | ||
| values: Record<string, any>, | ||
| otomiVersion: string, | ||
| ): ArgocdAppManifest => { |
Comment on lines
174
to
178
| const name = getAppName(release) | ||
| const patch = (appPatches[name] || genericPatch) as Record<string, any> | ||
|
|
||
| return getArgoCdAppManifest(name, ARGOCD_APP_DEFAULT_LABEL, { | ||
| syncPolicy: ARGOCD_APP_DEFAULT_SYNC_POLICY, |
Contributor
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 6 out of 6 changed files in this pull request and generated no new comments.
Suppressed comments (6)
src/common/runtime-upgrades/v6.2.0.test.ts:71
- Similarly, add a negative test to ensure ConfigMaps with an in-limit last-applied annotation are not patched. This guards the intended behavior (only strip oversized annotations) and prevents broad cluster-wide mutation in future refactors.
it('removes last-applied-configuration from a ConfigMap whose annotation exceeds the limit', async () => {
mockListConfigMaps.mockResolvedValue({
items: [
{
metadata: {
name: 'grafana-dashboards-k8s-admin',
namespace: 'grafana',
annotations: { [annotation]: oversizedValue },
},
},
],
})
await stripOversizedLastAppliedAnnotations(mockDeps as any)
expect(mockPatchConfigMap).toHaveBeenCalledWith(
expect.objectContaining({ name: 'grafana-dashboards-k8s-admin', namespace: 'grafana' }),
expect.anything(),
)
})
src/common/hf.ts:32
- Same camelCase flag appears in the initial-install sync args. Use kebab-case (
--server-side=true) to match standard flag naming and avoid an unrecognized option at runtime.
'sync',
'--reuse-values', // Preserve values from existing releases on retry - makes install idempotent
'--concurrency=1',
'--sync-args',
// These two need to be in same string as is passed as single argument to --sync-args
'--disable-openapi-validation --qps=20 --serverSide=true',
]
src/common/runtime-upgrades/v6.2.0.ts:32
- The filter claims to strip oversized last-applied annotations, but it currently matches any CRD that has the annotation (regardless of size) and then swallows patch failures via Promise.allSettled without reporting them. This can unintentionally remove valid annotations and makes failures hard to diagnose.
.filter((crd) => {
const value = crd.metadata?.annotations?.[LAST_APPLIED_ANNOTATION]
return value !== undefined
})
.map(async (crd) => {
const name = crd.metadata!.name!
log.info(`Stripping oversized last-applied-configuration from CRD ${name}`)
await crdApi.patchCustomResourceDefinition({ name, body: removePatch }, patchHeaders)
}),
src/common/runtime-upgrades/v6.2.0.ts:48
- Same issue for ConfigMaps: this strips the last-applied annotation from every ConfigMap that has it and ignores patch errors. It should only target annotations that exceed the 256KiB limit (and log failures) to avoid unexpected side-effects on unrelated ConfigMaps.
configMaps
.filter((cm) => {
const value = cm.metadata?.annotations?.[LAST_APPLIED_ANNOTATION]
return value !== undefined
})
.map(async (cm) => {
const name = cm.metadata!.name!
const namespace = cm.metadata!.namespace!
log.info(`Stripping oversized last-applied-configuration from ConfigMap ${namespace}/${name}`)
await coreApi.patchNamespacedConfigMap({ name, namespace, body: removePatch }, patchHeaders)
}),
src/common/runtime-upgrades/v6.2.0.test.ts:51
- The tests currently only check for presence/absence of the annotation, but the runtime upgrade is specifically meant to act only when the annotation exceeds the 256KiB limit. Add a negative test to ensure CRDs with an in-limit annotation are not patched (so we don't regress to stripping annotations unconditionally).
This issue also appears on line 52 of the same file.
it('removes last-applied-configuration from a CRD whose annotation exceeds the limit', async () => {
mockListCRDs.mockResolvedValue({
items: [{ metadata: { name: 'clusterpolicies.kyverno.io', annotations: { [annotation]: oversizedValue } } }],
})
await stripOversizedLastAppliedAnnotations(mockDeps as any)
expect(mockPatchCRD).toHaveBeenCalledWith(
expect.objectContaining({ name: 'clusterpolicies.kyverno.io' }),
expect.anything(),
)
})
src/common/hf.ts:23
- The added flag uses camelCase (
--serverSide=true), which is inconsistent with the rest of the CLI flags in this repo (and with the kubectl flag used elsewhere:--server-side). If the intention is to enable server-side apply, use kebab-case to avoid passing an unrecognized flag to helmfile/helm.
This issue also appears on line 26 of the same file.
'--concurrency=1',
'--sync-args',
// These two need to be in same string as is passed as single argument to --sync-args
'--disable-openapi-validation --qps=20 --serverSide=true',
]
Contributor
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 6 out of 6 changed files in this pull request and generated no new comments.
Suppressed comments (4)
src/common/runtime-upgrades/v6.2.0.ts:41
- Same as CRDs: the ConfigMap filter removes last-applied from all ConfigMaps that have it, which could include many unrelated ConfigMaps across all namespaces. Restrict patching to objects whose annotations are actually oversized to reduce upgrade load and avoid unnecessary mutation of user-managed resources.
.filter((cm) => {
const value = cm.metadata?.annotations?.[LAST_APPLIED_ANNOTATION]
return value !== undefined
})
src/common/hf.ts:32
- The added sync arg uses
--serverSide=true, but elsewhere in this repo the server-side apply flag is consistently spelled--server-side(kebab-case). If this string is passed through to kubectl/helm,--serverSidemay be ignored or error as an unknown flag, meaning SSA won’t actually be enabled.
export const HF_DEFAULT_SYNC_ARGS = [
'sync',
'--concurrency=1',
'--sync-args',
// These two need to be in same string as is passed as single argument to --sync-args
'--disable-openapi-validation --qps=20 --serverSide=true',
]
export const HF_DEFAULT_SYNC_ON_INITIAL_INSTALL_ARGS = [
'sync',
'--reuse-values', // Preserve values from existing releases on retry - makes install idempotent
'--concurrency=1',
'--sync-args',
// These two need to be in same string as is passed as single argument to --sync-args
'--disable-openapi-validation --qps=20 --serverSide=true',
]
src/common/runtime-upgrades/v6.2.0.ts:26
- The CRD filter currently removes the last-applied annotation from every CRD that has it, even when annotations are well below the 256KiB limit. This increases blast radius and work during upgrades, and it doesn’t match the function/log wording (“oversized”). Filter based on actual annotation size so only problematic objects are patched.
This issue also appears on line 38 of the same file.
.filter((crd) => {
const value = crd.metadata?.annotations?.[LAST_APPLIED_ANNOTATION]
return value !== undefined
})
src/common/runtime-upgrades/v6.2.0.test.ts:81
- The tests only cover the “oversized” case and “annotation missing” case. Since the intent is to strip annotations only when they overflow the limit, add a test that verifies objects with the annotation present but not oversized are not patched (prevents regressions to the current always-strip behavior).
it('does not patch a ConfigMap without the annotation', async () => {
mockListConfigMaps.mockResolvedValue({
items: [{ metadata: { name: 'some-config', namespace: 'default', annotations: {} } }],
})
await stripOversizedLastAppliedAnnotations(mockDeps as any)
expect(mockPatchConfigMap).not.toHaveBeenCalled()
})
Contributor
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 6 out of 6 changed files in this pull request and generated no new comments.
Suppressed comments (5)
src/common/hf.ts:22
--sync-argspasses flags through to Helm;--serverSide=trueis not a valid Helm flag and will causehelmfile syncto fail. Helm uses kebab-case--server-side(optionally=true).
'--disable-openapi-validation --qps=20 --serverSide=true',
src/common/hf.ts:31
--serverSide=trueis not a valid Helm flag (camelCase). This will likely break initial-install sync runs; use--server-sideinstead.
'--disable-openapi-validation --qps=20 --serverSide=true',
src/common/runtime-upgrades/v6.2.0.ts:32
- This currently patches every CRD that has the annotation (even small ones), and
Promise.allSettleddiscards failures so the upgrade can silently do nothing on RBAC/validation errors. Filter to only large annotations (to reduce API churn) and log failures from the settled results.
const { items: crds } = await crdApi.listCustomResourceDefinition()
await Promise.allSettled(
crds
.filter((crd) => {
const value = crd.metadata?.annotations?.[LAST_APPLIED_ANNOTATION]
return value !== undefined
})
.map(async (crd) => {
const name = crd.metadata!.name!
log.info(`Stripping oversized last-applied-configuration from CRD ${name}`)
await crdApi.patchCustomResourceDefinition({ name, body: removePatch }, patchHeaders)
}),
)
src/common/runtime-upgrades/v6.2.0.ts:48
- This lists all ConfigMaps across all namespaces and patches every one that has the annotation, which can be very expensive on larger clusters; additionally failures are currently swallowed by
Promise.allSettled. Filter to only large annotations and log any rejected patches so operators can diagnose RBAC/permission issues.
const { items: configMaps } = await coreApi.listConfigMapForAllNamespaces()
await Promise.allSettled(
configMaps
.filter((cm) => {
const value = cm.metadata?.annotations?.[LAST_APPLIED_ANNOTATION]
return value !== undefined
})
.map(async (cm) => {
const name = cm.metadata!.name!
const namespace = cm.metadata!.namespace!
log.info(`Stripping oversized last-applied-configuration from ConfigMap ${namespace}/${name}`)
await coreApi.patchNamespacedConfigMap({ name, namespace, body: removePatch }, patchHeaders)
}),
)
src/common/runtime-upgrades/v6.2.0.ts:7
- The function name/logging/tests say this only strips oversized
last-applied-configuration, but the implementation filters only on presence. Add an explicit size threshold constant so the behavior matches the intent and avoids patching every annotated object in the cluster.
This issue also appears in the following locations of the same file:
- line 20
- line 35
const LAST_APPLIED_ANNOTATION = 'kubectl.kubernetes.io/last-applied-configuration'
// JSON Patch requires '/' in key names to be escaped as '~1'
const LAST_APPLIED_PATCH_PATH = '/metadata/annotations/kubectl.kubernetes.io~1last-applied-configuration'
Contributor
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 6 out of 6 changed files in this pull request and generated 1 comment.
Suppressed comments (4)
src/common/hf.ts:31
- Same issue as above for the initial-install sync args:
--serverSide=trueis not a recognized Helm 3 flag and will likely causehelmfile syncto fail. Remove it here as well.
'--disable-openapi-validation --qps=20 --serverSide=true',
src/common/runtime-upgrades/v6.2.0.ts:41
- Same as the CRD block: this currently lists all ConfigMaps cluster-wide and patches every one that has the annotation (even if small), and any patch failures are swallowed due to ignored
allSettledresults. This should only patch ConfigMaps where the annotation exceeds the 256KiB limit and should log any failures.
configMaps
.filter((cm) => {
const value = cm.metadata?.annotations?.[LAST_APPLIED_ANNOTATION]
return value !== undefined
})
src/common/runtime-upgrades/v6.2.0.ts:33
- This block claims to strip oversized last-applied annotations, but it currently patches every CRD that merely has the annotation (no size check). Also,
Promise.allSettled(...)results are ignored, so per-object patch failures are silently swallowed and the caller’s.catch(...)will never run. Filter by actual byte size and log failures fromallSettled.
This issue also appears on line 37 of the same file.
await Promise.allSettled(
crds
.filter((crd) => {
const value = crd.metadata?.annotations?.[LAST_APPLIED_ANNOTATION]
return value !== undefined
src/common/runtime-upgrades/v6.2.0.test.ts:82
- With the runtime-upgrade logic intended to act only on oversized annotations, it’s worth adding a negative test that ensures ConfigMaps with a present but under-limit
last-applied-configurationare not patched. This prevents regressions where the upgrader churns through many small ConfigMaps.
it('does not patch a ConfigMap without the annotation', async () => {
mockListConfigMaps.mockResolvedValue({
items: [{ metadata: { name: 'some-config', namespace: 'default', annotations: {} } }],
})
| '--sync-args', | ||
| // These two need to be in same string as is passed as single argument to --sync-args | ||
| '--disable-openapi-validation --qps=20', | ||
| '--disable-openapi-validation --qps=20 --serverSide=true', |
Contributor
Author
There was a problem hiding this comment.
That is true. The server-side parameter needs was introduced in helm v4
Contributor
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 6 out of 6 changed files in this pull request and generated no new comments.
Suppressed comments (8)
src/common/runtime-upgrades/v6.2.0.test.ts:42
- This test description and input imply a value exceeding the API limit. If the implementation filters by a “large annotation” threshold, adjust the test name and use the large-but-valid test value.
it('removes last-applied-configuration from a CRD whose annotation exceeds the limit', async () => {
mockListCRDs.mockResolvedValue({
items: [{ metadata: { name: 'clusterpolicies.kyverno.io', annotations: { [annotation]: oversizedValue } } }],
})
src/common/runtime-upgrades/v6.2.0.test.ts:60
- Same issue as the CRD test: update this test to use the “large annotation” value and rename it so it reflects the actual behavior being validated (threshold-based stripping).
it('removes last-applied-configuration from a ConfigMap whose annotation exceeds the limit', async () => {
mockListConfigMaps.mockResolvedValue({
items: [
{
metadata: {
name: 'grafana-dashboards-k8s-admin',
namespace: 'grafana',
annotations: { [annotation]: oversizedValue },
},
src/common/hf.ts:31
- Same comment as above: this note refers to “two” args but the sync-args string contains three flags. Keeping comments accurate matters here because argument grouping is subtle.
'--sync-args',
// These two need to be in same string as is passed as single argument to --sync-args
'--disable-openapi-validation --qps=20 --serverSide=true',
src/common/runtime-upgrades/v6.2.0.ts:32
- The runtime upgrade currently strips the last-applied annotation from all CRDs that have it, but the intent (and log message) is to mitigate annotation-size overflows by targeting only very large annotations near the 256KiB limit. This broad patching can create unnecessary API traffic and noise on large clusters. Filter by annotation byte size (UTF-8) and log failures from Promise.allSettled so permission/patch errors aren’t silently swallowed.
.filter((crd) => {
const value = crd.metadata?.annotations?.[LAST_APPLIED_ANNOTATION]
return value !== undefined
})
.map(async (crd) => {
src/common/runtime-upgrades/v6.2.0.ts:48
- Same as the CRD loop: this patches every ConfigMap that merely has the last-applied annotation, even when it’s small. To mitigate the annotation overflow without churning the entire cluster, filter by annotation byte size and surface patch failures instead of discarding Promise.allSettled results.
.filter((cm) => {
const value = cm.metadata?.annotations?.[LAST_APPLIED_ANNOTATION]
return value !== undefined
})
.map(async (cm) => {
src/common/runtime-upgrades/v6.2.0.test.ts:17
- The test uses an annotation value larger than the Kubernetes limit (262145 bytes), which can never exist on a real object. If the runtime upgrade is meant to strip annotations that are near the limit, use a large-but-valid size and also define a small value for below-threshold cases.
This issue also appears in the following locations of the same file:
- line 39
- line 52
const annotation = 'kubectl.kubernetes.io/last-applied-configuration'
const oversizedValue = 'x'.repeat(262145)
src/common/runtime-upgrades/v6.2.0.test.ts:76
- With size-based filtering, it’s important to assert that resources with a small last-applied annotation are not patched (not just the “missing annotation” case). Update this test to include the annotation but keep it below the threshold.
it('does not patch a ConfigMap without the annotation', async () => {
mockListConfigMaps.mockResolvedValue({
items: [{ metadata: { name: 'some-config', namespace: 'default', annotations: {} } }],
})
src/common/hf.ts:22
- The inline comment says “These two” args must be in the same string, but the string now contains three flags. Update the comment to avoid becoming misleading as more sync args are added.
This issue also appears on line 29 of the same file.
'--sync-args',
// These two need to be in same string as is passed as single argument to --sync-args
'--disable-openapi-validation --qps=20 --serverSide=true',
Contributor
|
Upgrade to v6.2.0 worked and the following crd's and cm's are stripped: |
CasLubbers
approved these changes
Aug 5, 2026
Contributor
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 6 out of 6 changed files in this pull request and generated 2 comments.
Suppressed comments (2)
src/common/hf.ts:23
--sync-argsin Helmfile is passed through to the underlyinghelm upgradeinvocation. Helm’s server-side apply flag is--server-side(kebab-case), not--serverSide(camelCase). As written,--serverSide=trueis likely to be rejected as an unknown flag by standard Helm/Helmfile, breakinghelmfile sync.
export const HF_DEFAULT_SYNC_ARGS = [
'sync',
'--concurrency=1',
'--sync-args',
// These two need to be in same string as is passed as single argument to --sync-args
'--disable-openapi-validation --qps=20 --serverSide=true',
]
src/common/runtime-upgrades/v6.2.0.test.ts:51
- The tests describe behavior “exceeds the limit”, but currently there’s no assertion that non-oversized annotations are left intact. Add a regression test to ensure only oversized annotations trigger a patch once the implementation includes the size check.
it('removes last-applied-configuration from a CRD whose annotation exceeds the limit', async () => {
mockListCRDs.mockResolvedValue({
items: [{ metadata: { name: 'clusterpolicies.kyverno.io', annotations: { [annotation]: oversizedValue } } }],
})
Comment on lines
+19
to
+32
| const crdApi = deps.getCrdApi() | ||
| const { items: crds } = await crdApi.listCustomResourceDefinition() | ||
| await Promise.allSettled( | ||
| crds | ||
| .filter((crd) => { | ||
| const value = crd.metadata?.annotations?.[LAST_APPLIED_ANNOTATION] | ||
| return value !== undefined | ||
| }) | ||
| .map(async (crd) => { | ||
| const name = crd.metadata!.name! | ||
| log.info(`Stripping oversized last-applied-configuration from CRD ${name}`) | ||
| await crdApi.patchCustomResourceDefinition({ name, body: removePatch }, patchHeaders) | ||
| }), | ||
| ) |
Comment on lines
+34
to
+48
| const coreApi = deps.getCoreApi() | ||
| const { items: configMaps } = await coreApi.listConfigMapForAllNamespaces() | ||
| await Promise.allSettled( | ||
| configMaps | ||
| .filter((cm) => { | ||
| const value = cm.metadata?.annotations?.[LAST_APPLIED_ANNOTATION] | ||
| return value !== undefined | ||
| }) | ||
| .map(async (cm) => { | ||
| const name = cm.metadata!.name! | ||
| const namespace = cm.metadata!.namespace! | ||
| log.info(`Stripping oversized last-applied-configuration from ConfigMap ${namespace}/${name}`) | ||
| await coreApi.patchNamespacedConfigMap({ name, namespace, body: removePatch }, patchHeaders) | ||
| }), | ||
| ) |
Contributor
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 6 out of 6 changed files in this pull request and generated no new comments.
Suppressed comments (6)
src/common/hf.ts:23
--sync-argsare forwarded by helmfile to Helm. Adding--serverSide=truehere will be passed to Helm and is not a supported Helm v3 flag (per prior discussion, SSA support would require Helm v4), sohelmfile synccan fail with an unknown-flag error.
'--concurrency=1',
'--sync-args',
// These two need to be in same string as is passed as single argument to --sync-args
'--disable-openapi-validation --qps=20 --serverSide=true',
]
src/common/runtime-upgrades/v6.2.0.ts:41
- Same as CRDs: this currently patches every ConfigMap with the annotation, regardless of size. Given the cluster-wide scan rationale, it’s safer to only strip when the last-applied payload is large enough to risk the annotations limit.
.filter((cm) => {
const value = cm.metadata?.annotations?.[LAST_APPLIED_ANNOTATION]
return value !== undefined
})
src/common/hf.ts:32
- Same issue as above:
--serverSide=trueis forwarded to Helm viahelmfile --sync-argsand can break installs/retries with an unknown-flag error on Helm v3.
'--reuse-values', // Preserve values from existing releases on retry - makes install idempotent
'--concurrency=1',
'--sync-args',
// These two need to be in same string as is passed as single argument to --sync-args
'--disable-openapi-validation --qps=20 --serverSide=true',
]
src/common/runtime-upgrades/v6.2.0.ts:26
stripOversizedLastAppliedAnnotationscurrently strips the annotation from every CRD that has it, even if it’s not oversized. That contradicts the function name/logging and can cause unnecessary patch traffic. Consider filtering by byte size (useBuffer.byteLength) so only risky/large last-applied values are removed.
.filter((crd) => {
const value = crd.metadata?.annotations?.[LAST_APPLIED_ANNOTATION]
return value !== undefined
})
src/common/runtime-upgrades/v6.2.0.ts:37
listConfigMapForAllNamespaces()+Promise.allSettled(configMaps.map(...))can create a very large number of concurrent patch requests on clusters with many ConfigMaps, which may spike API server load and memory usage during the runtime upgrade. Consider adding a tighter filter (namespace/label) and/or limiting concurrency.
const { items: configMaps } = await coreApi.listConfigMapForAllNamespaces()
await Promise.allSettled(
configMaps
src/common/runtime-upgrades/v6.2.0.test.ts:81
- Tests currently only cover the missing-annotation case. If the implementation is meant to strip oversized last-applied payloads, add a negative test for a ConfigMap where the annotation exists but is below the size threshold to prevent accidental removal of last-applied from all ConfigMaps.
it('does not patch a ConfigMap without the annotation', async () => {
mockListConfigMaps.mockResolvedValue({
items: [{ metadata: { name: 'some-config', namespace: 'default', annotations: {} } }],
})
await stripOversizedLastAppliedAnnotations(mockDeps as any)
expect(mockPatchConfigMap).not.toHaveBeenCalled()
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
📌 Summary
#3449
#3421
🔍 Reviewer Notes
🧹 Checklist