refactor: tls policy status to state of the world tasks

[KevFan]

Description

Closes: #824

• Refactor the TLSPolicy status code to workflow tasks
• Added a new MissingDependency status of TLS Policy for if CertManager is not installed

Verification

• Passing integration tests should be sufficient to ensure current functionality around TLSPolicy Status still works as intended
• To verify new MissingDependency TLSPolicy status
  • Create cluster without CertManager and run operator

  make local-env-setup
  # Delete CertManager CRDs to minic cert manager is uninstalled
  kubectl delete crd certificaterequests.cert-manager.io 
  kubectl delete crd certificates.cert-manager.io
  kubectl delete crd challenges.acme.cert-manager.io
  kubectl delete crd clusterissuers.cert-manager.io
  kubectl delete crd issuers.cert-manager.io
  kubectl delete crd orders.acme.cert-manager.io
  # Run operator
  make run
  

  • Create kuadrant-system namespace and tls policiy

  # Create TLSPolicy
  kubectl apply -f - <<EOF
  apiVersion: kuadrant.io/v1alpha1
  kind: TLSPolicy
  metadata:
    name: gw-tls
  spec:
    targetRef:
      group: gateway.networking.k8s.io
      kind: Gateway
      name: istio-ingressgateway
    issuerRef:
      group: cert-manager.io
      kind: ClusterIssuer
      name: selfsigned-issuer
  EOF

  • Get TLSPolicy status

  kubectl get tlspolicy -A -o yaml | yq '.items[].status'

  • TLSPolicy status should be the following:

  conditions:
  - lastTransitionTime: "2024-10-02T13:34:59Z"
    message: Cert Manager is not installed, please restart pod once dependency has been installed
    reason: MissingDependency
    status: "False"
    type: Accepted
  observedGeneration: 1

[guicassolato]

Hmmm... We may expect more of these, and not only with this particular resource.

This is because, as a pattern, we check the presence of the resource in the topology, and not with the API server. But more importantly, because, even though it's "state of the world" reconciliation and therefore the controller already knows about all resources that exist (because it lists them all), the library watching for changes to the cache writes to the subscription channel resource by resource.

I think we should consider fixing this in the Policy Machinery instead, because now it may be defeating the purpose of "state of the world". We may be able to make the call to cache.Replace truly atomic – assuming it won't break with the granularity of multiple event types. I have a couple of ideas.

[guicassolato]

I believe Kuadrant/policy-machinery#35 may have fixed this issue.

[KevFan]

This still happens with the same error:

{"level":"error","ts":"2024-10-02T08:56:41+01:00","logger":"kuadrant-operator","msg":"reconciliation error","error":"configmaps \"topology\" already exists","stacktrace":"github.com/kuadrant/policy-machinery/controller.(*Controller).propagate\n\t/Users/chfan/dev/kuadrant-operator/vendor/github.com/kuadrant/policy-machinery/controller/controller.go:262\ngithub.com/kuadrant/policy-machinery/controller.(*Controller).subscribe.func2\n\t/Users/chfan/dev/kuadrant-operator/vendor/github.com/kuadrant/policy-machinery/controller/controller.go:312"}
{"level":"info","ts":"2024-10-02T08:56:41+01:00","logger":"kuadrant-operator.controller-runtime.metrics","msg":"Starting metrics server"}
{"level":"info","ts":"2024-10-02T08:56:41+01:00","logger":"kuadrant-operator","msg":"starting server","name":"health probe","addr":"[::]:8081"}

What looks to happens from debugging is that this code is ran on boot even when there has been no events and when the topology has been loaded with the cluster data yet 🤔

My guess is that since this is a task with no event matchers, the initial creation of the empty cache store is an event that causes these task to run 🤔

[guicassolato]

the initial creation of the empty cache store is an event that causes these task to run

Oh! This makes total sense and explains why it's triggering before the controller finishes starting. By simply creating the cache we may be generating events already. Nice catch, @KevFan!

[guicassolato]

I'm guessing that somewhere here, when the snapshot contains zero updates, then we just continue?

Another option is, if len(events) == 0 { continue } here.

[guicassolato]

Kuadrant/policy-machinery#41

[guicassolato]

Interesting approach. I wonder how it performs compared to labels.

[KevFan]

Yeah, I went with ownerReference instead since we already set the ownerRef to point to TLSPolicy since we want the associated Certificates deleted when the TLSPolicy is deleted.

Also we don't currently set a common label across all created Certificate's

kuadrant-operator/controllers/tlspolicy_certmanager_certificates.go

Lines 161 to 184 in 4793dc4

	func commonTLSCertificateLabels(gwKey client.ObjectKey, p *v1alpha1.TLSPolicy) map[string]string {
	common := map[string]string{}
	for k, v := range policyTLSCertificateLabels(p) {
	common[k] = v
	}
	for k, v := range gatewayTLSCertificateLabels(gwKey) {
	common[k] = v
	}
	return common
	}
	func policyTLSCertificateLabels(p *v1alpha1.TLSPolicy) map[string]string {
	return map[string]string{
	p.DirectReferenceAnnotationName(): p.Name,
	fmt.Sprintf("%s-namespace", p.DirectReferenceAnnotationName()): p.Namespace,
	}
	}
	func gatewayTLSCertificateLabels(gwKey client.ObjectKey) map[string]string {
	return map[string]string{
	"gateway-namespace": gwKey.Namespace,
	"gateway": gwKey.Name,
	}
	}

[guicassolato]

Interestingly, we may not need to find the object the policy points to.

Because policies are linked to their targets already, a policy p where len(p.GetTargetRefs()) > 0 && len(topology.Targetables().Children(p)) == 0 cannot be valid.

[guicassolato]

Obviously, what I'm suggesting won't give enough details to report in the message which target cannot be found.

[KevFan]

Oh cool, yeah that's a nicer way of knowing whether a policy can't find their targetRef, I'll update 👍

[KevFan]

len(p.GetTargetRefs()) != len(topology.Targetables().Children(p)) is probably what we expect instead? 🤔
I think this is fine as we currently don't support multiple target refs but this may not work if we want to report which is not found as you've said, and also not sure what the policy status should be in the case where at least one target ref in the list is not found 🤔

[guicassolato]

TLS is an interesting case. If the gateway is not Programmed, it doesn't mean we cannot provision the certificates for its listeners. Is that why you didn't include the status of the gateway here?

[KevFan]

Exactly, a gateway can transition to a Programmed state from the certs created by the current tls policy controller.

[guicassolato]

ok == false should never happen, given both reconcilers subscribe to the exact same event matchers, unless the validator panics half-way. Good call logging the error.

[KevFan]

Yeah, this should never happen. This can also happen if there is a difference in the subscriptions between the validator and status updater in the future

[guicassolato]

Here's how I'm solving for RateLimitPolicy the issue of updating the status after having previously validated the policy vs. when the policy validator is skipped but the status updater still has to run (e.g. due to different event matchers):

kuadrant-operator/controllers/ratelimitpolicy_status_updater.go

Line 56 in 3279ab7

policyAcceptedFunc := rateLimitPolicyAcceptedStatusFunc(state)

[guicassolato]

We need to be careful to never allow unexported fields in the TLSPolicyStatus type, otherwise this line will start panicking.

[guicassolato]

I haven't tested it, but the code LGTM.

I've left a comment about a pattern for picking up state from previous tasks vs falling back to the topology state, but I'd recommend considering another PR for it.