enhancement: report dependency not installed to kuadrant & policy status #994

KevFan · 2024-11-08T15:39:10Z

Description

Closes: #730

Report in the Kuadrant and Policy status when required dependencies are not installed

Kuadrant CR
- Gateway API not installed
- Gateway Provider (istio / envoy gateway) not intalled
- Limitador Operator not installed
- Authorino Operator not installed
AuthPolicy CR
- Gateway API not installed
- Gateway Provider (istio / envoy gateway) not intalled
- Authorino Operator not installed
RateLimitPolicy CR
- Gateway API not installed
- Gateway Provider (istio / envoy gateway) not intalled
- Limitador Operator not installed
TLSPolicy CR (does not require a Gateway Provider)
- Gateway API not installed
- CertManager not installed
DNSPolicy CR (does not require a Gateway Provider)
- Gateway API not installed
- DNS Operator not installed

Verification

Gateway API and Gateway API provider status are tested by integration tests, so passing integration tests should be enough

To verify other status:

Create cluster

make local-env-setup

Delete CRDs to simulate not installed

# DNS Operator
kubectl delete crd dnsrecords.kuadrant.io   
# Authorino Operator                                                                    
kubectl delete crd authconfigs.authorino.kuadrant.io
kubectl delete crd authorinos.operator.authorino.kuadrant.io
# Limitador Operator
kubectl delete crd limitadors.limitador.kuadrant.io
# Cert Manager
kubectl delete crd certificates.cert-manager.io

Run operator

make run

Apply CRs

kubectl apply -f config/samples/kuadrant_v1_authpolicy.yaml                                      
kubectl apply -f config/samples/kuadrant_v1_dnspolicy.yaml
kubectl apply -f config/samples/kuadrant_v1_ratelimitpolicy.yaml
kubectl apply -f config/samples/kuadrant_v1_tlspolicy.yaml
kubectl apply -f config/samples/kuadrant_v1beta1_kuadrant.yaml

Check status

kubectl get kuadrant kuadrant-sample -o yaml | yq '.status'
kubectl get authpolicy authpolicy-sample -o yaml | yq '.status'
kubectl get ratelimitpolicy ratelimitpolicy-sample -o yaml | yq '.status'
kubectl get dnspolicy dnspolicy-sample -o yaml | yq '.status'
kubectl get tlspolicy tlspolicy-sample -o yaml | yq '.status'

Boomatang · 2025-01-08T14:56:11Z

I think there is some improvement needed here. Firstly I rebased this to current main, and everything worked as expected.

The simpler issue is around the message in the status block Limitador Operator is not installed, please restart pod once dependency has been installed. When I first seen this message I ask what pod do I restart. I think we need to be clear that it is the kuadrant operator that the user should restart. This might not be as important for the kuadrant CR directly, but on the policy CR's I think it is need.

The second issue is are clarity, but raises its own issue. Doing the example above the kuadrant CR states that Limitador Operator is missing, but all the resources are missing. The user would fix one issue, restart the operator only to find the next issue. Where they would have to repeat the process over and over. This is not great. I think we should be telling the user all the dependencies that are missing at the same time.

The third point and I don't think it is an issue but more of an opportunity. When I tried the example give, at first I didn't make every CR type, I only made the one for auth. As a user the only on thing I am currently using kuadrant for is auth. Why do I care if limitador and DNS operators are not working. Wouldn't it be nice if kuadrant only told you had a dependency issue if you apply resources to the cluster that depends on that dependency. I also think this could help us on the part of solving this issue: #71

While I am at it I think we might need more log level than info & error for production. I think there needs to be a warning level.

{"level":"info","ts":"2025-01-08T14:21:52Z","logger":"kuadrant-operator","msg":"envoygateway is not installed, skipping related watches and reconcilers","err":null}

The above message is from the logs in pr, and I think it should be a warning over info level log. This possible is out side the scope of this PR, but it is a good example of what would be a warning.

One last point, which is splitting hairs, technically the Limitador Operator is not installed is wrong. It is the limitador CRD that is not installed. Do we want to get that fine grained, I don't think so.

KevFan · 2025-01-09T11:44:15Z

The simpler issue is around the message in the status block Limitador Operator is not installed, please restart pod once dependency has been installed. When I first seen this message I ask what pod do I restart. I think we need to be clear that it is the kuadrant operator that the user should restart. This might not be as important for the kuadrant CR directly, but on the policy CR's I think it is need.

I think this is a good shout. Are we thinking of just calling it "Kuadrant Operator pod"? 🤔

The second issue is are clarity, but raises its own issue. Doing the example above the kuadrant CR states that Limitador Operator is missing, but all the resources are missing. The user would fix one issue, restart the operator only to find the next issue. Where they would have to repeat the process over and over. This is not great. I think we should be telling the user all the dependencies that are missing at the same time.

Yeah, this makes sense 👍

The third point and I don't think it is an issue but more of an opportunity. When I tried the example give, at first I didn't make every CR type, I only made the one for auth. As a user the only on thing I am currently using kuadrant for is auth. Why do I care if limitador and DNS operators are not working. Wouldn't it be nice if kuadrant only told you had a dependency issue if you apply resources to the cluster that depends on that dependency. I also think this could help us on the part of solving this issue: #71

This one, I'm not sure I understand the scenario you are saying. If you are doing Auth only, this missing dependency will only be reported in Kuadrant and AuthPolicy CR status. If you didn't apply Kuadrant CR, then it will be reported in AuthPolicy CR status only. Kuadrant CR cares about limitador and authorino since it creates an instance of those CRs. The current changes should already provide what you are saying - CRs that kuadrant provide should only report missing dependencies that each CR depend on - unless I'm missing something and you are saying this is not the case 🤔

While I am at it I think we might need more log level than info & error for production. I think there needs to be a warning level.
{"level":"info","ts":"2025-01-08T14:21:52Z","logger":"kuadrant-operator","msg":"envoygateway is not installed, skipping related watches and reconcilers","err":null}
The above message is from the logs in pr, and I think it should be a warning over info level log. This possible is out side the scope of this PR, but it is a good example of what would be a warning.

This would be outside of scope for sure. I don't believe the logger that we use even has a warning log type currently.

One last point, which is splitting hairs, technically the Limitador Operator is not installed is wrong. It is the limitador CRD that is not installed. Do we want to get that fine grained, I don't think so.

This is true, I don't think we want to be that fine grained. Might be able to improve this seperately, but outside of scope for this PR I would say. Kuadrant depends more on the CRD but would also have issues if the Operator is not running. Since we generally report status based on the status of the dependant CRs, I think we indicate something is unexepcted from the environment as our CR status will never go into a Ready/Enforced state 🤔 I can rename if that is our general preference everywhere. Will be a seperate issue/PR though.

Boomatang · 2025-01-13T09:49:47Z

The third point and I don't think it is an issue but more of an opportunity. When I tried the example give, at first I didn't make every CR type, I only made the one for auth. As a user the only on thing I am currently using kuadrant for is auth. Why do I care if limitador and DNS operators are not working. Wouldn't it be nice if kuadrant only told you had a dependency issue if you apply resources to the cluster that depends on that dependency. I also think this could help us on the part of solving this issue: #71

This one, I'm not sure I understand the scenario you are saying. If you are doing Auth only, this missing dependency will only be reported in Kuadrant and AuthPolicy CR status. If you didn't apply Kuadrant CR, then it will be reported in AuthPolicy CR status only. Kuadrant CR cares about limitador and authorino since it creates an instance of those CRs. The current changes should already provide what you are saying - CRs that kuadrant provide should only report missing dependencies that each CR depend on - unless I'm missing something and you are saying this is not the case 🤔

Okay what i was meaning is the kuadrant CR only gave an error related to Limitador and no error related to authorino. Authorino was what I was trying to use. The issue really is the kuadrant CR status will only list one error at a time. I the case that I looked at the user would have seen the limitador error fixed that error, and restarted kuadrant to only find out there was more errors. If there is more than one error these errors need to be list at the same time.

Signed-off-by: KevFan <[email protected]>

…icy status Signed-off-by: KevFan <[email protected]>

…atus Signed-off-by: KevFan <[email protected]>

Signed-off-by: KevFan <[email protected]>

Boomatang

I have one nit pick around the exit logging statement and few questions around styling that you might change.

Every thing is working as expected, and I think the messaging is much better now.

controllers/state_of_the_world.go

Boomatang · 2025-01-15T15:12:09Z

controllers/state_of_the_world.go

 		},
-		Postcondition: finalStepsWorkflow(b.client, b.isIstioInstalled, b.isGatewayAPIInstalled).Run,
+		Postcondition: finalStepsWorkflow(b.client, b.isGatewayAPIInstalled, b.isIstioInstalled, b.isEnvoyGatewayInstalled).Run,


Could we have a future design issue here, the order of the boolean values mean something. I am wonder is a list of options a better thing here. Not asking for this to be changed, more asking what do you think.

Yes, agreed, I don't quite like this. I had an initial thought of potentially separating the dependencies flags in BootOptionsBuilder into it's own struct:

type BootOptionsBuilder struct { logger logr.Logger manager ctrlruntime.Manager client *dynamic.DynamicClient dependencies *Dependencies } type Dependencies struct { // Internal configurations isGatewayAPIInstalled bool isEnvoyGatewayInstalled bool isIstioInstalled bool isCertManagerInstalled bool isConsolePluginInstalled bool isDNSOperatorInstalled bool isLimitadorOperatorInstalled bool isAuthorinoOperatorInstalled bool }

And potentially just pass this into the subsequent reconcilers that require this information. Granted just doing this, would leak unnecessary info to respective reconcilers (e.g. auth would not care about limitador for reporting).

What's your thoughts?

I was thinking about this, and I am wondering what map[string]bool would be like. It would remove the leaking of unnecessary information to the respective reconcilers. How they are created that I am not sure.

Boomatang · 2025-01-15T15:16:43Z

controllers/tls_workflow.go

@@ -40,9 +40,9 @@ var (
 //+kubebuilder:rbac:groups="cert-manager.io",resources=clusterissuers,verbs=get;list;watch;
 //+kubebuilder:rbac:groups="cert-manager.io",resources=certificates,verbs=get;list;watch;create;update;patch;delete

-func NewTLSWorkflow(client *dynamic.DynamicClient, scheme *runtime.Scheme, isCertManagerInstalled bool) *controller.Workflow {
+func NewTLSWorkflow(client *dynamic.DynamicClient, scheme *runtime.Scheme, isGatewayAPIInstalled, isCertManagerInstalled bool) *controller.Workflow {


Just to expand on the comment about the options object, this is an example of where I think we could be running to an issue. There is a lot of passing around of flags, and I think when we start to expand the support, this gets messy. Again not asking for a change. Looking for your feedback.

controllers/tlspolicies_validator.go

Boomatang · 2025-01-15T15:28:37Z

pkg/authorino/conditions.go

+func IsAuthorinoOperatorInstalled(restMapper meta.RESTMapper, logger logr.Logger) (bool, error) {
+	if ok, err := utils.IsCRDInstalled(restMapper, kuadrantv1beta1.AuthorinoGroupKind.Group, kuadrantv1beta1.AuthorinoGroupKind.Kind, authorinooperatorv1beta1.GroupVersion.Version); !ok || err != nil {
+		logger.V(1).Error(err, "Authorino Operator CRD was not installed", "group", kuadrantv1beta1.AuthorinoGroupKind.Group, "kind", kuadrantv1beta1.AuthorinoGroupKind.Kind, "version", authorinooperatorv1beta1.GroupVersion.Version)
+		return false, err
+	}
+
+	if ok, err := utils.IsCRDInstalled(restMapper, AuthConfigGroupKind.Group, AuthConfigGroupKind.Kind, authorinov1beta3.GroupVersion.Version); !ok || err != nil {
+		logger.V(1).Error(err, "Authorino Operator CRD was not installed", "group", AuthConfigGroupKind.Group, "kind", AuthConfigGroupKind.Kind, "version", authorinov1beta3.GroupVersion.Version)
+		return false, err
+	}
+
+	return true, nil
+}


This has the same issue of only reporting one missing CRD, does that matter in this case? It is seeming like we don't use the error in reporting in CRs, only a log message.

Hmm, still probably better to still log all the missing CRDs but yes, we do not use/report the error in the CR status

Signed-off-by: KevFan <[email protected]>

Boomatang

I am happy with how this is working now. There was that on debug log that we talked about. If you want to change that here, I am happy to look at it again.

KevFan force-pushed the issues/730 branch 7 times, most recently from d01d7fc to 90b4385 Compare November 15, 2024 09:18

KevFan self-assigned this Nov 15, 2024

KevFan added kind/enhancement New feature or request size/medium labels Nov 15, 2024

KevFan changed the title ~~Issues/730~~ enhancement: report dependency not installed to kuadrant & policy status Nov 15, 2024

KevFan marked this pull request as ready for review November 15, 2024 15:37

KevFan force-pushed the issues/730 branch 2 times, most recently from ae8b800 to 997a078 Compare December 2, 2024 12:52

KevFan requested a review from a team December 2, 2024 15:50

jasonmadigan mentioned this pull request Dec 3, 2024

In console, provide user assistance to installing Kuadrant's dependencies Kuadrant/kuadrant-console-plugin#14

Open

KevFan added 10 commits January 14, 2025 16:46

feat: validate dns operator crd is installed for dnspolicy status

bada5f6

Signed-off-by: KevFan <[email protected]>

feat: validate limitador operator crd is installed for rate limit pol…

1ad64d1

…icy status Signed-off-by: KevFan <[email protected]>

feat: validate authorino operator crd is installed for auth policy st…

7cea6f0

…atus Signed-off-by: KevFan <[email protected]>

feat: report missing Limitator/Authorino Operator to kuadrant status

b8e66dc

Signed-off-by: KevFan <[email protected]>

feat: gateway api detection on kuadrant and policy status

cce9630

Signed-off-by: KevFan <[email protected]>

refactor: common missing dependency errors

5a544e9

Signed-off-by: KevFan <[email protected]>

refactor: IsAuthorinoOperatorInstalled function

495da29

Signed-off-by: KevFan <[email protected]>

fix: integration tests after rebase

c530224

Signed-off-by: KevFan <[email protected]>

fixup: specify kuadrant operator pod in status message

e1f802c

Signed-off-by: KevFan <[email protected]>

refactor: list missing dependencies in status message

b446956

Signed-off-by: KevFan <[email protected]>

KevFan force-pushed the issues/730 branch from 997a078 to b446956 Compare January 14, 2025 16:46

KevFan requested a review from Boomatang January 15, 2025 13:01

Boomatang requested changes Jan 15, 2025

View reviewed changes

KevFan added 2 commits January 15, 2025 16:52

refactor: remove else conditioning for returning opts

efcaf7e

Signed-off-by: KevFan <[email protected]>

refactor: use defer for log

e32bc81

Signed-off-by: KevFan <[email protected]>

Boomatang approved these changes Jan 17, 2025

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

enhancement: report dependency not installed to kuadrant & policy status #994

enhancement: report dependency not installed to kuadrant & policy status #994

KevFan commented Nov 8, 2024 •

edited

Loading

Boomatang commented Jan 8, 2025

KevFan commented Jan 9, 2025 •

edited

Loading

Boomatang commented Jan 13, 2025

Boomatang left a comment

Boomatang Jan 15, 2025

KevFan Jan 15, 2025

Boomatang Jan 16, 2025

Boomatang Jan 15, 2025

Boomatang Jan 15, 2025

KevFan Jan 15, 2025

Boomatang left a comment

enhancement: report dependency not installed to kuadrant & policy status #994

Are you sure you want to change the base?

enhancement: report dependency not installed to kuadrant & policy status #994

Conversation

KevFan commented Nov 8, 2024 • edited Loading

Description

Verification

Boomatang commented Jan 8, 2025

KevFan commented Jan 9, 2025 • edited Loading

Boomatang commented Jan 13, 2025

Boomatang left a comment

Choose a reason for hiding this comment

Boomatang Jan 15, 2025

Choose a reason for hiding this comment

KevFan Jan 15, 2025

Choose a reason for hiding this comment

Boomatang Jan 16, 2025

Choose a reason for hiding this comment

Boomatang Jan 15, 2025

Choose a reason for hiding this comment

Boomatang Jan 15, 2025

Choose a reason for hiding this comment

KevFan Jan 15, 2025

Choose a reason for hiding this comment

Boomatang left a comment

Choose a reason for hiding this comment

KevFan commented Nov 8, 2024 •

edited

Loading

KevFan commented Jan 9, 2025 •

edited

Loading