grafana / oncall

Developer-friendly incident response with brilliant Slack integration
GNU Affero General Public License v3.0
3.44k stars 276 forks source link

:bug: Allow external redis/rabbitmq secret creation even if the broke… #3903

Closed afreyermuth98 closed 7 months ago

afreyermuth98 commented 7 months ago

What this PR does

This PR allows the chart to create the secret of your redis / rabbitmq even if it's not the broker. Actually, this is blocking if we want to have a redis as cache and a rabbitmq as broker for example

Which issue(s) this PR fixes

Closes https://github.com/grafana/oncall/issues/2979

Checklist

CLAassistant commented 7 months ago

CLA assistant check
All committers have signed the CLA.

afreyermuth98 commented 7 months ago

Hey ! It's done :D Btw I've modified a bit my code as we only use rabbitmq as a broker I guess :)

joeyorlando commented 7 months ago

@afreyermuth98 lastly you just need to sign the CLA 🙂

https://github.com/grafana/oncall/pull/3903#issuecomment-1945687387

afreyermuth98 commented 7 months ago

@joeyorlando Hmm weird i've signed it just after my commit 🤔

You have agreed to the CLA for grafana/oncall
joeyorlando commented 7 months ago

@joeyorlando Hmm weird i've signed it just after my commit 🤔


You have agreed to the CLA for grafana/oncall

looks to be fine now, thank you!

joeyorlando commented 7 months ago

@afreyermuth98 one last thing. One of the new helm unit tests is failing:

https://github.com/grafana/oncall/actions/runs/7971753982/job/21762105142#step:5:1

afreyermuth98 commented 7 months ago

@joeyorlando Yeah I don't really understand why. The test fails because the password is not set but it's the normal functioning isn't it ? If I'm wrong I would be glad to be highlighted :)

UPDATE : it should be fixed

afreyermuth98 commented 7 months ago

Hey @joeyorlando I've modified the tests. It passed but it's not perfect and I would be glad to be highlighted. Firstly, I had to comment my last lines of the broker_secret_test.yaml and I can't understand why it doesn't pass with. Also, on secrets.yaml - L64 , I've put (.Values.externalRedis.host) as a condition to pass (as you put the host for sure if you set an externalRedis) but I would have prefered to have (.Values.externalRedis.password) as before but I've investigated for hours and I can't understand why the test doesn't pass, everything is ok for me. Thanks by advance !

joeyorlando commented 7 months ago

Hey @joeyorlando I've modified the tests. It passed but it's not perfect and I would be glad to be highlighted. Firstly, I had to comment my last lines of the broker_secret_test.yaml and I can't understand why it doesn't pass with. Also, on secrets.yaml - L64 , I've put (.Values.externalRedis.host) as a condition to pass (as you put the host for sure if you set an externalRedis) but I would have prefered to have (.Values.externalRedis.password) as before but I've investigated for hours and I can't understand why the test doesn't pass, everything is ok for me. Thanks by advance !

One trick I just figured out is if you add a matchSnapshot assertion it will spit out the rendered .yaml as such:

# test file
- it: externalRedis.password and externalRabbitmq.password -> should create secret -redis-external and -rabbitmq-external
    templates:
      - engine/deployment.yaml
      - celery/deployment.yaml
    set:
      rabbitmq.enabled: false
      redis.enabled: false
      broker.type: rabbitmq
      externalRedis:
        host: redis.example.com
        username: user123
        password: abcd123
      externalRabbitmq:
        host: custom-host
        user: custom-user
        password: custom-password
    asserts:
      ... other assertions
      - matchSnapshot: {}
         template: secrets.yaml

then in ./helm/oncall/tests/__snapshot__ you will have a broker_secret_test.yaml.snap as such:

externalRedis.password and externalRabbitmq.password -> should create secret -redis-external and -rabbitmq-external:
  1: |
    apiVersion: v1
    data:
      MIRAGE_SECRET_KEY: aEV4RmNqYm5sdllPVUtqVmZ2ZG1oc3N4NXRQNGpuc0E2dzV5eXJrOA==
      SECRET_KEY: WjNGRVUzak9IMkM3TTBiQ0dyZUpDTmZ5dnQ0aEticGFFaEdXUk5IcQ==
    kind: Secret
    metadata:
      labels:
        app.kubernetes.io/instance: oncall
        app.kubernetes.io/managed-by: Helm
        app.kubernetes.io/name: oncall
        app.kubernetes.io/version: v1.3.99
        helm.sh/chart: oncall-1.3.99
      name: oncall
    type: Opaque
  2: |
    apiVersion: v1
    data:
      rabbitmq-password: Y3VzdG9tLXBhc3N3b3Jk
    kind: Secret
    metadata:
      name: oncall-rabbitmq-external
    type: Opaque
  3: |
    apiVersion: v1
    data:
      redis-password: YWJjZDEyMw==
    kind: Secret
    metadata:
      name: oncall-redis-external
    type: Opaque

turns out it was simply the documentIndex on the test you had commented out. It should be "2" instead of "1" 😄

afreyermuth98 commented 7 months ago

Ooook I was wondering about this documentIndex but didn't find a conclusion about it ahah. Thanks a lot ! :D