Skip router ConfigMap entries when TLS credential secrets are missing#2449
Skip router ConfigMap entries when TLS credential secrets are missing#2449AryanP123 wants to merge 3 commits into
Conversation
AryanP123
force-pushed
the
secret-router-access
branch
7 times, most recently
from
990bf32 to
30510e0
Compare
AryanP123
force-pushed
the
secret-router-access
branch
from
30510e0 to
f0a59d1
Compare
| func (m RouterAccessMap) DesiredConfigAllowingTlsSecrets(targetGroups []string, profilePath string, tlsSecretAllowed func(string) bool) *RouterAccessConfig { | ||
| source := m | ||
| if tlsSecretAllowed != nil { | ||
| source = make(RouterAccessMap, len(m)) | ||
| for k, ra := range m { | ||
| if ra.Spec.TlsCredentials != "" && !tlsSecretAllowed(ra.Spec.TlsCredentials) { | ||
| continue | ||
| } | ||
| source[k] = ra | ||
| } | ||
| } |
There was a problem hiding this comment.
if this function is skipping RouterAccess entries with tlsCredentials configured but not included in the profile watcher cache; I would suggest some naming changes:
tlsSecretAllowed-->isCredentialInCacheorcredentialExistsDesiredConfigAllowingTlsSecrets-->DesiredConfigWithAvailableCredentials
| listeners map[string]*skupperv2alpha1.Listener | ||
| multiKeyListeners map[string]*skupperv2alpha1.MultiKeyListener | ||
| handler BindingEventHandler | ||
| tlsSecretAllowed func(secretName string) bool |
There was a problem hiding this comment.
maybe it could be renamed to isTlsSecretAvailable or isTlsSecretPresent ?
| b.SiteId = siteId | ||
| } | ||
|
|
||
| func (b *Bindings) SetTlsSecretAllowed(f func(string) bool) { |
There was a problem hiding this comment.
maybe it could be renamed to SetIsTlsSecretPresent or to SetSecretChecker for better readability, what do you think?
AryanP123
force-pushed
the
secret-router-access
branch
from
5d69ec0 to
dddb71a
Compare
fgiorgetti
left a comment
fgiorgetti
left a comment
There was a problem hiding this comment.
It worked well for me.
I have been able to create links and accesses resources without providing a secret and that did not prevent other resources from being reconciled and configured.
Restarting the skupper-router deployment worked fine without blocking the config-init container.
One comment, though, was the Listeners and Connectors that refer to tlsCredentials that do not exist, are processed without warning or errors on the CR status. It would be nice to get a similar behavior to what we have with Links and RouterAccesses.
| func (b *ExtendedBindings) AddSslProfiles(config *qdr.RouterConfig, definitions map[string]*skupperv2alpha1.AttachedConnector) bool { | ||
| profiles := map[string]qdr.SslProfile{} | ||
| for _, c := range definitions { | ||
| if c.Spec.TlsCredentials != "" && !b.bindings.TlsCredentialIncluded(c.Spec.TlsCredentials) { |
There was a problem hiding this comment.
- Minor (optional), but we could move the 2nd condition here under the if statement below.
- Maybe log a message saying attached connector is being skipped?
| } | ||
| for _, secretName := range profileSecrets(credentialName) { | ||
| secret, err := w.Cache.Get(w.keyfunc(secretName)) | ||
| if err == nil && secret != nil && secret.Type == corev1.SecretTypeTLS { |
There was a problem hiding this comment.
Have we committed anywhere else that the Secret must have type kubernetes.io/tls? IIRC I've used opaque before just out of kubectl convenience and it works out fine.
There was a problem hiding this comment.
Was matching existing code. ProfilesWatcher and kube-adaptor secrets.Sync already only handle kubernetes.io/tls secrets, and doc/tls/README.md documents that layout. It states Skupper expects TLS credentials in standard kubernetes.io/tls format with tls.crt, tls.key, and optionally ca.crt. I kept the same check here so we don’t treat a secret as “ready” if the adaptor won’t sync it. Let me know if you prefer to change it.
4 participants
Fixes #2433