Updated code for restricting namespaces that secrets-manager watches

[avenging]

Code fixing #59

[codecov-io]

Codecov Report

Merging #60 into master will increase coverage by 0.02%. The diff coverage is 100%.

@@            Coverage Diff             @@
##           master      #60      +/-   ##
==========================================
+ Coverage   84.98%   85.01%   +0.02%     
==========================================
  Files           8        8              
  Lines         433      427       -6     
==========================================
- Hits          368      363       -5     
+ Misses         48       47       -1     
  Partials       17       17

Impacted Files	Coverage Δ
controllers/secretdefinition_controller.go	76.74% <100%> (-0.3%)	:arrow_down:

---

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 1fda604...2cc6652. Read the comment docs.

[avenging]

@fcgravalos I have added tests after a bit of struggle. As part of it I also noticed running multiple controllers watching secretdefinitions would cause moans about 'duplicate metrics collector registration attempted'. I have therefore added the ability to name the controller and added some documentation if this does get merged. Thanks.

[fcgravalos]

Interesting! Did you enable leader election when running multiple controllers?

[avenging]

No I didn't try that. Leader election needs a ConfigMap setting up and I didn't create it. I have SecretsManager running with this code in multiple namespaces with leader election with no issues.

[avenging]

@fcgravalos Is the change too much or does it need more testing?

[fcgravalos]

Hey @avenging, sorry for the delay.

I no longer work at Tuenti, I started a new adventure on January and I no longer belong to Tuenti GitHub org.

I can't merge your PRs unfortunately, so I suggest you to contact Tuenti engineering team

[stevendborrelli]

@eduardogr is tuenti still maintaining this project? I think @avenging's patches are very interesting and I have a PR in for some label enhancements. I'd be willing to assist the project.

[eduardogr]

@stevendborrelli yes! Sorry for the delay, i'm checking...

[avenging]

@eduardogr Thanks for the code and comments I will take a look this week and do some testing.

[avenging]

@eduardogr

I have taken a long hard look at what was done and decided maybe some of it was over the top and it could also cause an issue where you would have to re-start the secrets-manager controller whenever a new namespace was added to a cluster. The change now is a bit smaller.

I have re-added the code that was in the controller loop that decides if a namespace should be excluded or not so that if a new namespace is added and only excludeNamepaces is set then that namespace will start to be monitored straight away instead of having to do a controller re-start.

The new code for watchNamespaces however is still outside the controller loop and still configures a multiNamespaceCache if it is set. With the excludeNamespaces code back in the controller there is no need to merge or remove items from slices and all the code is now irrelevant and has been removed. Now if you set watchNamespaces AND excludeNamespaces if a namespace is in both then it will be excluded due to code that I have put back in the controller which is what it was trying to achieve,

I have also re-named and switched the true/false of the watchNs map and the function in the controller so it is now all about excluding namespaces rather than a decision on watching namespaces as that is handled by the multi namespace cache. Because of this re-naming and changing the logic seemed the right thing to do.

I have re-added the test for excludeNs as well.

Answering the comments on your review maybe a bit irrelevant now. Can you take a look at the updated code

[eduardogr]

nice @avenging ! Going to merge :+1: