Repository: kubernetes/kubernetes
File: staging/src/k8s.io/apiserver/pkg/server/options/serving_with_loopback.go (Go)

Code before:
	// Write to the front of SNICerts so that this overrides any other certs with the same name
	(*secureServingInfo).SNICerts = append([]dynamiccertificates.SNICertKeyContentProvider{certProvider}, (*secureServingInfo).SNICerts...)

	secureLoopbackClientConfig, err := (*secureServingInfo).NewLoopbackClientConfig(uuid.New().String(), certPem)
	switch {
	// if we failed and there's no fallback loopback client config, we need to fail
	case err != nil && *loopbackClientConfig == nil:
		(*secureServingInfo).SNICerts = (*secureServingInfo).SNICerts[1:]
		return err

	// if we failed, but we already have a fallback loopback client config (usually insecure), allow it
	case err != nil && *loopbackClientConfig != nil:

	default:
		*loopbackClientConfig = secureLoopbackClientConfig
	}

	return nil
}

func (s *SecureServingOptionsWithLoopback) ApplyWithServerConfigTo(cfg *server.Config) error {
	if err := s.ApplyTo(&cfg.SecureServing, &cfg.LoopbackClientConfig); err != nil {
		return err
	}

	s.addSecureServingHealthEndpoint(cfg)
	return nil
}

// AddSecureServingHealthEndpoint adds a health check called `secure-serving` to the
// server that fails when the loopback client certificate has expired, enabling
// liveness probes to be used to automatically restart the apiserver.
func (s *SecureServingOptionsWithLoopback) addSecureServingHealthEndpoint(c *server.Config) {
	expirationDate := time.Now().Add(maxAge)
	c.AddHealthChecks(healthz.NamedCheck("secure-serving", func(r *http.Request) error {
		if s.clock.Now().After(expirationDate) {
			return errors.New("loopback client certificate is expired")
		}

		return nil
	}))
}


Review comment (bug):
Open question related to this logic - do we need to account for the scenario where we fail to create the secure loopback client config and there _is_ a fallback client that doesn't contain a certificate?

Traversing the chain from the `(*secureServingInfo).NewLoopbackClientConfig(...)` call on line 84, I see that the only potential failure mode is https://github.com/kubernetes/kubernetes/blob/437184c05538a0dcb9e16d7b2b84f06c56320d7a/staging/src/k8s.io/apiserver/pkg/server/config_selfclient.go#L68-L72 and is marked as "should never happen".

With the current implementation, the health check that is unconditionally added (after a successful `ApplyTo()`) will fail after the ~3 year mark, and if liveness probes are configured the apiserver will be restarted.

In an earlier implementation, I checked whether or not the loopback client had a CA cert configured and did PEM decoding + certificate parsing to extract the "Not After" date to use in the health check. This seems more robust of a health check than just using a static `now() + maxAge` because it would cause health checks to fail on fallback clients having different certificates configured, but that seems like a very niche and unlikely scenario. This approach would also introduce additional failure modes during the configuration, so 🤷 .

Feedback/thoughts appreciated.

Code after:
	if err != nil {
		return fmt.Errorf("failed to generate self-signed certificate for loopback connection: %v", err)
	}
	certProvider, err := dynamiccertificates.NewStaticSNICertKeyContent("self-signed loopback", certPem, keyPem, server.LoopbackClientServerNameOverride)
	if err != nil {
		return fmt.Errorf("failed to generate self-signed certificate for loopback connection: %v", err)
	}

	// Write to the front of SNICerts so that this overrides any other certs with the same name
	(*secureServingInfo).SNICerts = append([]dynamiccertificates.SNICertKeyContentProvider{certProvider}, (*secureServingInfo).SNICerts...)

	secureLoopbackClientConfig, err := (*secureServingInfo).NewLoopbackClientConfig(uuid.New().String(), certPem)
	switch {
	// if we failed and there's no fallback loopback client config, we need to fail
	case err != nil && *loopbackClientConfig == nil:
		(*secureServingInfo).SNICerts = (*secureServingInfo).SNICerts[1:]
		return err

	// if we failed, but we already have a fallback loopback client config (usually insecure), allow it
	case err != nil && *loopbackClientConfig != nil:

	default:
		*loopbackClientConfig = secureLoopbackClientConfig
		if healthCheckAdder != nil {
			s.addLoopbackServingCertificateHealthCheck(healthCheckAdder)
		}
	}

	return nil
}

func (s *SecureServingOptionsWithLoopback) ApplyToConfig(cfg *server.Config) error {
	return s.applyTo(&cfg.SecureServing, &cfg.LoopbackClientConfig, cfg)
}

// addLoopbackServingCertificateHealthCheck adds a health check called `loopback-certificate-expiry` to the
// server that fails when the loopback client certificate has expired, enabling
// liveness probes to be used to automatically restart the apiserver.
func (s *SecureServingOptionsWithLoopback) addLoopbackServingCertificateHealthCheck(healthCheckAdder HealthzLivezHealthChecksAdder) {
	expirationDate := s.clock.Now().Add(maxAge)
	check := healthz.NamedCheck("loopback-serving-certificate", func(r *http.Request) error {
		if s.clock.Now().After(expirationDate) {
			return LoopbackCertificateExpiredError{}
		}

		return nil
	})

	healthCheckAdder.AddHealthzChecks(check)
	healthCheckAdder.AddLivezChecks(check)
}