grafana-operator / grafana-operator-experimental

Testing the new reconciler for the Grafana Operator
Apache License 2.0
9 stars 4 forks source link

Deploying a Grafana CR without ingress configuration provisions invalid Ingress resource #78

Open tamcore opened 1 year ago

tamcore commented 1 year ago

When deploying a Grafana CR without having any Ingress configuration specified, the Operator will create an Ingress object without any host attached to it.

Expected behaviour should be to not provision an Ingress CR, as long as there's no valid host defined. One use case (our's, actually), where an Ingress is unwanted, is when Grafana is behind a separate proxy application and authentication is done via JWT for example.


Example Grafana CR

apiVersion: grafana.integreatly.org/v1beta1
kind: Grafana
metadata:
  name: grafana-test
spec:
  config:
    log:
      mode: "console"
    auth:
      disable_login_form: "false"
    security:
      admin_user: root
      admin_password: secret

Resulting Ingress

apiVersion: networking.k8s.io/v1
kind: Ingress
metadata:
  creationTimestamp: "2023-03-09T14:49:13Z"
  generation: 2
  name: grafana-test-ingress
  namespace: grafana
  ownerReferences:
  - apiVersion: grafana.integreatly.org/v1beta1
    kind: Grafana
    name: grafana-test
    uid: 49f3e944-f1d6-48ba-bb12-7a9de8dcbce0
  resourceVersion: "206126146"
  uid: f0676b71-15fd-4415-aaf7-c3c6692cbb74
spec:
  rules:
  - http:
      paths:
      - backend:
          service:
            name: grafana-test-service
            port:
              number: 3000
        path: /
        pathType: Prefix
status:
  loadBalancer: {}
NissesSenap commented 1 year ago

Hi @tamcore , thanks once again for trying things out. I have actually moved a bunch of open issues over the main repo because I'm preparing to migrate all the code over to it. You can find an existing issue around this here: https://github.com/grafana-operator/grafana-operator/issues/906

I wrote a small note about it in slack https://kubernetes.slack.com/archives/C019A1KTYKC/p1678293324043159 but I understand it's easy to not read everything that is posted there.

I think enabling and disabling ingress by a correct host will be a bit strange. I'm leaning more towards just not creating one unless you define it.

We can't do any of the assumptions that we do in OCP any way so why create one at all? But at the same time I think we should keep the same way of working for both OCP and k8s users so my current vote is to not create a ingress object unless it's defined.

Is it something that you would be willing to do as well?

NissesSenap commented 1 year ago

@tamcore, saw that I forgot to ping you in my response. Fully understand if you don't have the time to look in to the issue though.

tamcore commented 1 year ago

Sure, I'll happily have a look at it :)

not creating one unless you define it

Probably the best way. But if we simply don't provision something unless the user explicitly provides grafana.spec.ingress.spec by himself, we would expect him to also know the service name and port.

So I somewhat feel like, the user should be able to get away with just defining the ingress host (+ ingress labels/annotations, if he wishes) OR defining the full ingress object.

NissesSenap commented 1 year ago

That is a very good point.

But that should be solvable with us providing decent defaults. This how we do it in the deployment resource right? They don't have to define everything just the things they want to change.

So if grafana.spec.ingress != nil then do a few things, give them a suggested ingress object and merge there changes on top. We would be consistent with the way the user is using the CR:s.

So just as you say, more or less the only thing need to write is the host URL and then they will be good. For using https they of course have to define

      tls:
        - hosts:
            - example.com
          secretName: core-cert