Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[Bug] Persist irrelevant Kubernetes Secrets #1425

Open
kevin85421 opened this issue Dec 25, 2023 · 3 comments
Open

[Bug] Persist irrelevant Kubernetes Secrets #1425

kevin85421 opened this issue Dec 25, 2023 · 3 comments
Labels
area/performance Performance related area/security For security best practices bug Something isn't working
Milestone

Comments

@kevin85421
Copy link
Contributor

Describe the bug

When you create a Kind cluster, the Kubernetes Secret bootstrap-token-abcdef in the kube-system namespace will also be created. However, NGF watches all Kubernetes Secrets in the Kubernetes cluster. Is it necessary to persist these irrelevant Kubernetes Secrets?

{"level":"info","ts":"2023-12-25T08:15:03Z","msg":"Upserted the resource","controller":"secret","controllerGroup":"","controllerKind":"Secret","Secret":{"name":"bootstrap-token-abcdef","namespace":"kube-system"},"namespace":"kube-system","name":"bootstrap-token-abcdef","reconcileID":"acf04978-8167-4300-9249-d67caa5c19a5"}

To Reproduce

  • Create a Kind cluster
  • Deploy NGF
  • Check logs

Expected behavior

The irrelevant secret should not be reconciled and persisted.

@sjberman
Copy link
Contributor

The controller will see all Secrets because of our current RBAC rules. We can't exclude specific Secrets, it's all or nothing. However, we don't actually do any processing if the Secret is irrelevant. We see it, log a message, and ignore it.

There could be an opportunity to improve the logging, though. I see a couple of options for improvement:

  1. In the reconciler (where we log "Upserted the resource"), we could move that log message to the debug level instead of info to reduce the noise.
  2. If we ignore a resource, add a log message for that (probably also at the debug level).
  3. If we actually process and use a resource, ensure that we log that at the info level.

@pleshakov
Copy link
Contributor

There are few possible improvements:

(1) For the controller for Secrets, we can make it watch for metadata changes, instead of the whole object changes. This should prevent NGF from caching all cluster Secrets in its memory. This was implemented for the controller which watches for cluster CRDs -- d6bbdba In this case, only when NGF needs some secret(s), it will fetch it from the API.

(2) Restrict access to Secrets, so that fewer secrets will be watched -- #1321

We also have plans to split control plane from data plane into separate deployments, so that the data plane pods will not have direct access to Kubernetes APIs and thus any cluster Secrets, but only the ones used in generated NGINX configuration, fetched from the control plane pod(s).

@mpstefan mpstefan added this to the v1.3.0 milestone Jan 22, 2024
@mpstefan mpstefan added bug Something isn't working area/performance Performance related area/security For security best practices labels Jan 22, 2024
Copy link
Contributor

github-actions bot commented Feb 6, 2024

This issue is stale because it has been open 14 days with no activity. Remove stale label or comment or this will be closed in 14 days.

@github-actions github-actions bot added the stale Pull requests/issues with no activity label Feb 6, 2024
@sjberman sjberman removed the stale Pull requests/issues with no activity label Feb 6, 2024
@mpstefan mpstefan modified the milestones: v1.3.0, v1.4.0 May 1, 2024
@mpstefan mpstefan modified the milestones: v1.4.0, v2.1.0 Jul 24, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/performance Performance related area/security For security best practices bug Something isn't working
Projects
Status: 🆕 New
Development

No branches or pull requests

4 participants