Fix up use of helmette.SafeLookup calls

[andrewstucki]

redpanda-data/redpanda-operator#226 helped me discover that the clients initialized using a rest.Config passed in a dot context weren't being initialized properly when that config comes from the controller-runtime, namely since the operator service was using a service account, all of its CA/certificate/authentication settings were being pulled from disk. This fixes the RestToConfig method to properly initialize a clientcmdapi.Config object from a rest.Config containing parameters pulled from disk.

[chrisseto]

I don't think I understand the issue that we're attempting to solve here. What situation do we need to authenticate to Kubenernetes with mTLS and how is it being misconfigured? IIUC all the configuration for TLS/auth/etc should be present in the kubeconfig that's used to construct the client's we use.

[andrewstucki]

@chrisseto So, it could be that I'm initializing the dot struct improperly here, but here's what I get from the controller in acceptance tests trying to use the helmette methods with the kubeconfig from dot:

error fetching server root CA testing-v1tbga8fa3/sasl-default-root-certificate: failed to get API group resources: unable to retrieve the complete list of server APIs: v1: Get "https://10.96.0.1:443/api/v1": tls: failed to verify certificate: x509: certificate signed by unknown authority

And sorry, it's potentially not the client-side TLS certificate, it looks like it could just be the root CA is getting pulled from the rest.Config improperly?

[chrisseto]

Oh, I'd actually be suspicious of kube.RestToConfig(c.config) in that case. Chances are it's dropping something in the conversion. Strange that this hasn't surfaced before 🤔 Ah, It's probably because RestToConfig is only used in cases where we're connecting to an envtest apiserver.

[andrewstucki]

@chrisseto yeah, so there's some potential fields that could affect this and some other stuff, namely Cluster seems to have file-based fields that point to cert data. I wouldn't be surprised if something in the rest.Config returned by the controller runtime was using on-disk certs rather than certs already read into the struct. In addition there's a TLSServerName field that, given the IP I see in the log file makes me wonder if we'd have problems with server names matching anyway.

What are you thinking the way to proceed is? Trying to jump in an figure out exactly how we need to change RestToConfig to be compatible with the rest.Config handed back by our controller-runtime manager or just pass the already initialized client?

[chrisseto]

I lean towards fixing existing paths if at all possible as this will very likely come and bite us again. Hopefully it's just a matter of reading a few certs from the disk!

If it's not as simple as that, I'd vote to remove this method entirely so we don't have to fight these issues again and see if we could update helmette.Dot to accept either a rest.Config or a pre-constructed client.

[andrewstucki]

Got it, sounds good, I'll take a pass at modifying the RestToConfig method. It looks like the controller-runtime is handing us back a rest.Config that looks like this:

&rest.Config{Host:"https://10.96.0.1:443", APIPath:"", ContentConfig:rest.ContentConfig{AcceptContentTypes:"", ContentType:"", GroupVersion:(*schema.GroupVersion)(nil), NegotiatedSerializer:runtime.NegotiatedSerializer(nil)}, Username:"", Password:"", BearerToken:"--- REDACTED ---", BearerTokenFile:"/var/run/secrets/kubernetes.io/serviceaccount/token", Impersonate:rest.ImpersonationConfig{UserName:"", UID:"", Groups:[]string(nil), Extra:map[string][]string(nil)}, AuthProvider:, AuthConfigPersister:rest.AuthProviderConfigPersister(nil), ExecProvider:, TLSClientConfig:rest.sanitizedTLSClientConfig{Insecure:false, ServerName:"", CertFile:"", KeyFile:"", CAFile:"/var/run/secrets/kubernetes.io/serviceaccount/ca.crt", CertData:[]uint8(nil), KeyData:[]uint8(nil), CAData:[]uint8(nil), NextProtos:[]string(nil)}, UserAgent:"", DisableCompression:false, Transport:http.RoundTripper(nil), WrapTransport:(transport.WrapperFunc)(nil), QPS:20, Burst:30, RateLimiter:flowcontrol.RateLimiter(nil), WarningHandler:rest.WarningHandler(nil), Timeout:0, Dial:(func(context.Context, string, string) (net.Conn, error))(nil), Proxy:(func(*http.Request) (*url.URL, error))(nil)}}

So it does seem to be leveraging the on-disk files along with JWT tokens due to being from a service account, so we'll need to handle all of that.

[andrewstucki]

@chrisseto ok, so I just force updated with the fix for the RestToConfig implementation and tested it by hacking it in locally on my acceptance test branch in the operator repo. It works like a charm now.