ingress: nginx controller watches referenced tls secrets #1063
Conversation
| for _, tls := range ing.Spec.TLS { | ||
| secretName := tls.SecretName | ||
| secrMetadata[fmt.Sprintf("%s/%s", ing.Namespace, secretName)] = true | ||
| secret, err := lbc.client.Secrets(ing.Namespace).Get(secretName) |
There was a problem hiding this comment.
@simonswine can you change this to use secrLister instead of api.Client?
There was a problem hiding this comment.
@aledbf thanks for that hint, now using the secrLister
simonswine
force-pushed
the
feature-nginx-ingress-watch-secrets
branch
from
aba8c84
to
a7101cd
Compare
simonswine
force-pushed
the
feature-nginx-ingress-watch-secrets
branch
from
a7101cd
to
fc63f57
Compare
|
LGTM |
| @@ -797,9 +860,24 @@ func (lbc *loadBalancerController) getPemsFromIngress(data []interface{}) map[st | |||
| } | |||
| } | |||
|
|
|||
| lbc.secrMetadataLock.Lock() | |||
There was a problem hiding this comment.
do we really need to maintain this map/locks or can we just relist all ingresses on secret update and break on first name match ?
There was a problem hiding this comment.
@bprashanth I had the feeling that this would be too expensive, but I can modify the code accordingly
There was a problem hiding this comment.
I'm fine if you think it's required. I usually try to avoid locking unless I absolutely need to. If you just need a locked map, suggest using cache.Store (or cache.ThreadSafeStore as the need may be:https://github.com/kubernetes/kubernetes/blob/master/pkg/client/cache/thread_safe_store.go).
* reload certificates on change
simonswine
force-pushed
the
feature-nginx-ingress-watch-secrets
branch
from
fc63f57
to
9b9c7da
Compare
|
I see you already modified it per my last comment, @simonswine thanks! LGTM, will merge on green |
|
@bprashanth, seems like it has happened in the same minute :) |
|
Merged, but no new image. If you need this in an image please send a pr with the version bump and I can push it out. |
This enables the controller to reload certificates on changes in the secrets.
We are maintaining a list of referenced certificates in secrMetadata to only reload/sync the controller when something in referenced certificates changes.