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

Remove OwnerReferences from CA configmaps #1467

Merged
merged 2 commits into from
Jun 10, 2021

Conversation

rubenvp8510
Copy link
Collaborator

@rubenvp8510 rubenvp8510 commented Jun 8, 2021

Signed-off-by: Ruben Vargas [email protected]

This change is to avoid ConfigMaps which are not in the same namespace as the operator from being garbage collected.

As the documentation says:

Cross-namespace owner references are disallowed by design.
Namespaced dependents can specify cluster-scoped or namespaced owners. A namespaced owner must exist in the same namespace as the dependent. If it does not, the owner reference is treated as absent, and the dependent is subject to deletion once all owners are verified absent.

https://kubernetes.io/docs/concepts/workloads/controllers/garbage-collection/

Which problem is this PR solving?

  • This solves an issue that deletes CA/Service CA ConfigMaps from sidecars that are not in the same namespace as the operator.

Short description of the changes

  • Removed the OwnerReferences from ConfigMaps
  • Implemented a deletion logic for those ConfigMaps when a Jaeger instance is deleted.

NOTE: The clean up process consider that there is no more than one Jaeger instance with the same name on the cluster. I think we did this assumption for other parts of the code, so I don't think this will be a problem for now.

@rubenvp8510 rubenvp8510 marked this pull request as ready for review June 9, 2021 01:51
@codecov
Copy link

codecov bot commented Jun 9, 2021

Codecov Report

Merging #1467 (b5f7f33) into master (32e8f54) will decrease coverage by 0.17%.
The diff coverage is 50.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #1467      +/-   ##
==========================================
- Coverage   87.33%   87.15%   -0.18%     
==========================================
  Files          90       90              
  Lines        4982     4983       +1     
==========================================
- Hits         4351     4343       -8     
- Misses        478      484       +6     
- Partials      153      156       +3     
Impacted Files Coverage Δ
pkg/config/ca/ca.go 100.00% <ø> (ø)
pkg/controller/jaeger/jaeger_controller.go 37.01% <0.00%> (-0.77%) ⬇️
pkg/controller/jaeger/configmap.go 68.08% <56.25%> (-6.11%) ⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 32e8f54...b5f7f33. Read the comment docs.

@rubenvp8510 rubenvp8510 force-pushed the fix-configmaps branch 3 times, most recently from 8d706ae to e905aa2 Compare June 9, 2021 05:29
@rubenvp8510
Copy link
Collaborator Author

Struggling to reach the desired coverage due the tests can't reach error blocks.

Copy link
Contributor

@jpkrohling jpkrohling left a comment

Choose a reason for hiding this comment

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

Looks good, just need to properly format the imports and document why errors are being swallowed.

pkg/controller/jaeger/configmap.go Outdated Show resolved Hide resolved
pkg/controller/jaeger/configmap.go Show resolved Hide resolved
@jpkrohling jpkrohling enabled auto-merge (squash) June 10, 2021 09:10
@jpkrohling jpkrohling merged commit 86a9284 into jaegertracing:master Jun 10, 2021
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