Skip to content

fix(affinity): apply component-specific pod anti-affinity to aggregator and server deployments#223

Closed
pinkneyj wants to merge 3 commits into
Cloudzero:developfrom
pinkneyj:develop
Closed

fix(affinity): apply component-specific pod anti-affinity to aggregator and server deployments#223
pinkneyj wants to merge 3 commits into
Cloudzero:developfrom
pinkneyj:develop

Conversation

@pinkneyj

Copy link
Copy Markdown

By submitting a PR to this repository, you agree to the terms within the CloudZero Code of Conduct. Please see the contributing guidelines for how to create and submit a high-quality PR for this repo.

Please note that changes to the cloudzero-agent Helm chart should be made instead in the helm directory in the cloudzero-agent repository, and will automatically be mirrored to this repository as soon as they are merged.

Description

Describe the purpose of this PR along with any background information and the impacts of the proposed change. For the benefit of the community, please do not assume prior context.

Provide details that support your chosen implementation, including: breaking changes, alternatives considered, changes to the API, etc.

If the UI is being changed, please provide screenshots.

The aggregator-deploy.yaml template passed only defaults.affinity to the generateAffinity helper, with no component-specific anti-affinity logic. This caused any label set in defaults.affinity to be applied to all components indiscriminately — for example, setting an aggregator-targeted matchLabels in defaults would incorrectly propagate that label selector to the server deployment, making the server avoid nodes running aggregator pods rather than other server pods.

This change brings aggregator-deploy.yaml and agent-deploy.yaml (server) in line with the existing pattern used in webhook-deploy.yaml: each template now constructs a $podAntiAffinity variable with its own component-specific app.kubernetes.io/name label, merges it with any user-supplied affinity from values, and passes the result to generateAffinity. This ensures each component's pod anti-affinity rule targets the correct peer pods, and that defaults.affinity is no longer required to carry component-specific label selectors.

No breaking changes. The aggregator.affinity and server.affinity values fields continue to work as user overrides.

References

Include any links supporting this change such as a:

  • GitHub Issue/PR number addressed or fixed
  • StackOverflow post
  • Support forum thread
  • Related pull requests/issues from other repos

Pattern sourced from existing webhook-deploy.yaml implementation (lines 106–108)

Testing

Describe how this can be tested by reviewers. Be specific about anything not tested and reasons why. If this library has unit and/or integration testing, tests should be added for new functionality and existing tests should complete without errors.

Please include any manual steps for testing end-to-end or functionality not covered by unit/integration tests.

Also include details of the environment this PR was developed in (language/platform/browser version).

Tested by deploying the Helm chart to a Kubernetes cluster and verifying:

  • Aggregator pods have matchLabels: app.kubernetes.io/name: aggregator in their anti-affinity spec

  • Server pods have matchLabels: app.kubernetes.io/name: server in their anti-affinity spec

  • Neither component inherits the other's label selector

  • defaults.affinity can be set to {} without affecting component-specific anti-affinity behaviour

  • labels can be manually set for one component without others being changed eg app.kubernetes.io/part-of: cloudzero-agent set for aggregator while others remain unchanged

  • This change adds test coverage for new/changed/fixed functionality

Checklist

  • I have added documentation for new/changed functionality in this PR
  • All active GitHub checks for tests, formatting, and security are passing
  • The correct base branch is being used, if not main

@pinkneyj pinkneyj requested a review from a team as a code owner June 30, 2026 15:47
@greptile-apps

greptile-apps Bot commented Jun 30, 2026

Copy link
Copy Markdown

Greptile Summary

This PR fixes component-specific pod anti-affinity rules for the aggregator and server deployments, bringing them in line with the existing pattern used in webhook-deploy.yaml. Each template now builds its own $podAntiAffinity dict with the correct app.kubernetes.io/name label and merges it with any user-supplied overrides before passing to generateAffinity.

  • agent-deploy.yaml and aggregator-deploy.yaml each construct a preferredDuringSchedulingIgnoredDuringExecution anti-affinity rule targeting app.kubernetes.io/name: server and app.kubernetes.io/name: aggregator respectively — these values match the pod template labels rendered by generateLabels.
  • A new values-staging-nonprod.yaml file is added that contains what appears to be a real API key and real cloud account ID; the file itself includes a warning against committing real credentials and should be reviewed before merge.

Confidence Score: 2/5

The affinity template changes are correct and safe, but the new values file appears to contain a real CloudZero API key committed to the repository — do not merge until the key is confirmed fake or revoked and replaced.

The two template files are a clean, correct fix. The blocking concern is values-staging-nonprod.yaml: despite an inline warning to avoid committing real credentials, the apiKey field contains a non-placeholder value alongside a real AWS account ID. If the key is genuine, it is now in git history and must be revoked before the branch can safely be merged.

charts/cloudzero-agent/values-staging-nonprod.yaml — verify whether the API key is real and whether this file belongs in version control at all.

Security Review

  • Credential exposure in values-staging-nonprod.yaml (line 7): the apiKey field holds a non-placeholder value (gn451xCSZHUCLzS2jS5gaz6d6T-_G4vj2s8cLA) despite an in-file comment warning "do not commit this file with a real API key." If genuine, the key should be revoked and the file scrubbed or gitignored before this PR merges.

Important Files Changed

Filename Overview
charts/cloudzero-agent/templates/agent-deploy.yaml Adds component-specific podAntiAffinity for the server deployment; matchLabels correctly target app.kubernetes.io/name: server, matching actual pod labels.
charts/cloudzero-agent/templates/aggregator-deploy.yaml Adds component-specific podAntiAffinity for the aggregator; matchLabels correctly target app.kubernetes.io/name: aggregator, matching actual pod labels.
charts/cloudzero-agent/values-staging-nonprod.yaml New staging values file committed with a non-placeholder apiKey value despite an in-file warning not to do so; also exposes a cloud account ID and cluster name in version control.

Reviews (2): Last reviewed commit: "fix trailing whitespace" | Re-trigger Greptile

Comment thread charts/cloudzero-agent/templates/aggregator-deploy.yaml Outdated
@pinkneyj pinkneyj marked this pull request as draft June 30, 2026 15:55
@pinkneyj pinkneyj marked this pull request as ready for review June 30, 2026 15:55

# WARNING: do not commit this file with a real API key
apiKey: "gn451xCSZHUCLzS2jS5gaz6d6T-_G4vj2s8cLA"

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

P0 security Possible real API key committed to git

The apiKey field contains a non-placeholder value (gn451xCSZHUCLzS2jS5gaz6d6T-_G4vj2s8cLA), and the inline comment on line 6 explicitly warns "do not commit this file with a real API key." If this is a genuine CloudZero API key, it should be revoked immediately and replaced with a placeholder (e.g. "REPLACE_ME") before merging. The file also embeds a cloud account ID and cluster details that should not live in version control. Consider adding values-staging-nonprod.yaml to .gitignore.

@pinkneyj pinkneyj marked this pull request as draft June 30, 2026 15:59
@pinkneyj pinkneyj closed this Jun 30, 2026
@pinkneyj

Copy link
Copy Markdown
Author

Rotated api key after mess up here...
Will create new PR for work.

@evan-cz

evan-cz commented Jun 30, 2026

Copy link
Copy Markdown
Contributor

Hi @pinkneyj, thanks for the contribution! It's deeply appreciated. This is definitely an issue we want to get resolved before the next release.

This content is just mirrored from the helm/ directory in the Cloudzero/cloudzero-agent repository, so we can't accept patches to this content here; they would just get overwritten the next time anything got merged into the cloudzero-agent repo.

I've opened Cloudzero/cloudzero-agent#881 for the aggregator (using aggregator.affinity). For the server, you can use server.affinity, so no changes should be necessary. Hopefully that addresses your need?

This also uncovered Cloudzero/cloudzero-agent#882

@pinkneyj

pinkneyj commented Jul 6, 2026

Copy link
Copy Markdown
Author

Hi @evan-cz I've still opened a PR to the other repository: Cloudzero/cloudzero-agent#898

Would it still not be better to have the affinity set automatically, as is done in webhook-server?

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.

2 participants