Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix: Adding missing operator metrics service #186

Closed
wants to merge 2 commits into from

Conversation

R-Lawton
Copy link

Adding missing service to expose authorino operator metrics & adding flag to fix panic that happens with manifests make target

Verification

kind create cluster --name authorino-operator
kubectl create namespace authorino-operator
make install
make run

Create a authorino cr

kubectl apply -f -<<EOF             
apiVersion: operator.authorino.kuadrant.io/v1beta1
kind: Authorino
metadata:
  name: authorino
  namespace: authorino-operator
spec:
  clusterWide: true
  healthz: {}
  listener:
    ports: {}
    tls:
      enabled: false
  metrics: {}
  oidcServer:
    tls:
      enabled: false
  supersedingHostSubsets: true
  tracing:
    endpoint: ''
  volumes: {}
EOF

You should see multiple services come up
k get services -n authorino-operator
ensure you get the following

authorino-metrics                                      ClusterIP   10.96.96.154   <none>        8080/TCP             8s
authorino-authorino-authorization                       ClusterIP   10.96.218.59   <none>        50051/TCP,5001/TCP   8s
authorino-authorino-oidc                                ClusterIP   10.96.25.6     <none>        8083/TCP             8s
authorino-operator-controller-manager-metrics-service   ClusterIP   10.96.197.85   <none>        8080/TCP             8s
authorino-webhooks                                      ClusterIP   10.96.168.70   <none>        443/TCP              113s

@R-Lawton R-Lawton requested a review from guicassolato June 28, 2024 10:49
Copy link
Collaborator

@guicassolato guicassolato left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the PR, @R-Lawton.

I think what we want is something along the lines this what we have already for Limitador Operator:

No changes needed in the AuthorinoReconciler IMO. Just a simple 1-off config for when installing the operator itself.

Comment on lines +670 to +675
desiredServices = append(desiredServices, authorinoResources.NewOperatorMetricsService(
authorinoInstanceName,
authorinoInstanceNamespace,
httpPort,
authorinoOperatorLabels,
))
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Confused about this. This will create a metrics service for the operator after each Authorino CR, which is meant to deploy the operand. I don't think that's what we want.

@guicassolato
Copy link
Collaborator

Just pushed #187 as an alternative.


In time – one acknowledgement though to the bad suffix controller-metrics in the operands' metric services' names that this PR would help fix:

return newService("metrics", serviceNamespace, authorinoName, labels, ports...)

I think this bit is very welcome, though probably in a separate PR since it's a breaking change for the user.

@R-Lawton
Copy link
Author

Ok cool, i wasnt sure best way of going about it so i went with how the rest of the services are created but i think keeping it cohesive with limitador makes sense so ill close this out in favour of your pr

@R-Lawton R-Lawton closed this Jun 28, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants