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

Support v3 APIs and basic auth in Kafka Contact Point #28

Merged
merged 7 commits into from Jan 13, 2023
Merged

Support v3 APIs and basic auth in Kafka Contact Point #28

merged 7 commits into from Jan 13, 2023

Conversation

ghost
Copy link

@ghost ghost commented Jan 6, 2023

What?

This commit does the following,

  • Adds basic auth to the Kafka Contact Point.
  • Adds support for v3 Kafka APIs

Basic Auth

When creating a Kafka Rest Proxy Contact Point on Grafana, users should now be able to use basic auth.
They can specific a username and can choose to either specify the password or let the password be read from a file on
the grafana server.
Like this,

Screenshot 2023-01-08 at 11 34 10 PM

Additionally, when the user chooses to supply the password as a file on the server, the password is re-read from the file on
an API failure. This is to allow the password to be changed without needing to restart the grafana server.

v3 Kafka APIs

The existing Kafka Contact Point only supported v2 APIs. While this was fine till now - newer systems are moving towards v3. Confluent is already only deploying v3 APIs for their Kafka Cloud Clusters.
This commit adds support to allow using v3 APIs to publish messages to kafka.

When creating the contact point, the user can now choose the API version they would like to use. By default, v2 will be used.

Screenshot 2023-01-08 at 11 41 19 PM

Testing

  1. Added tests in kafka_tests.go
  2. Manually tested this against my grafana deployment. Existing v2 APIs continued to work fine. Tested this using the "Test Contact Point" in the "Edit Contact Point" page.
  3. Manually tested that I was able to messages to Kafka v3
  4. [Done] TODO: Verify that the contact points created before this change continue to load fine after upgrading to this. Tested upgrade from 9.3.1 to main(v9.4.0-pre (d9be87c22d)) The previously provisioned kafka contact point was working as expected after the upgrade.

Things I need help with

1. How do I build and test this with Grafana?
2. How do I know/specify which Grafana's release will this go into?

CHANGELOG

  • Kafka Contact Point now supports basic auth.
  • Kafka Contact Point now supports v3 APIs.

References

  1. Kafka v2/v3 API comparison doc
  2. Corresponding changes in Grafana UI: Alerting: UI changes required to support v3 and Auth in Kafka Contact Point grafana#61123

Credits

Something similar was done in this PR. Which I referenced: grafana/grafana#41227

@CLAassistant
Copy link

CLAassistant commented Jan 6, 2023

CLA assistant check
All committers have signed the CLA.

@ghost
Copy link
Author

ghost commented Jan 6, 2023

Hey Guys!
Do I need a corresponding change in available_channels.go as well?

@ghost ghost marked this pull request as ready for review January 6, 2023 20:24
@ghost
Copy link
Author

ghost commented Jan 6, 2023

Related issue: grafana/grafana#41176

@alexweav
Copy link
Contributor

alexweav commented Jan 6, 2023

Hi, thanks very much for this contribution 👋

At first glance, this seems like a reasonable change!

Do I need a corresponding change in available_channels.go as well?

Yes, this will automatically create UI elements to match your new fields, so they will be visible and editable in the UI.

Regarding password in the DB vs. in a file, there are a few additional considerations to take into account:

  • Grafana encrypts secrets stored in the database.
  • Network-based provisioning systems (Terraform, etc) do not have access to the disk of the instance, and will not be able to create a password file.
  • Many users expect all of Grafana's data to be in the database, and many Grafana deployments have totally volatile filesystems.
  • In Grafana's High-Availability mode, a password file would have to be synchronized among all instances, where a password in the database only has to be set once.

Additionally, some pointers that may be of help here:

a password changes shouldn't require the user to restart the grafana server

We offer a way around this, sending a POST request to /api/admin/provisioning/alerting/reload should reload and re-sync the provisioning files without a server restart.

I would prefer to store the kafka password in a kubernates secret and mount it onto the Grafana pod as a file.

The provisioning files also allow environment variables to be referenced. A common pattern that we see is to mount the secret as an environment variable rather than a file, then have the provisioning file point to it.

@ghost
Copy link
Author

ghost commented Jan 7, 2023

Thanks for the review @alexweav !

Yes, this will automatically create UI elements to match your new fields, so they will be visible and editable in the UI.

Sounds good, I will raise another PR for this.

I think I agree with your points of needing to allow for a password.
Would it be okay if I allowed both options? Either supply the password or supply a path to a file containing the password?

The provisioning files also allow environment variables to be referenced. A common pattern that we see is to mount the secret as an environment variable rather than a file, then have the provisioning file point to it

The problem with this is that changes in the secret will not propagate to the container's ENV variable.
From the Kubernetes doc,

Note: If a container already consumes a Secret in an environment variable, a Secret update will not be seen by the container unless it is restarted. There are third party solutions for triggering restarts when secrets change.

This is where I am coming from: My workplace wants us rotating passwords very frequently - every 15 mins or so. So, I would prefer the password be in a secret and mounted as a file. I would like to avoid frequent restarts of Grafana.

Given that there are uses for both the approaches, shall I go ahead with supporting them both please?

@ghost ghost changed the title [Draft] Add auth to Kafka Contact Point [Draft] Support v3 APIs and basic auth in Kafka Contact Point Jan 8, 2023
@ghost
Copy link
Author

ghost commented Jan 8, 2023

@alexweav Here are the corresponding changes for the UI: grafana/grafana#61123

@ghost
Copy link
Author

ghost commented Jan 9, 2023

Given grafana/grafana#60655, I am assuming that the earliest release this change can only go into is 9.4. Is that correct?

@ghost ghost changed the title [Draft] Support v3 APIs and basic auth in Kafka Contact Point Support v3 APIs and basic auth in Kafka Contact Point Jan 10, 2023
@ghost
Copy link
Author

ghost commented Jan 12, 2023

Added upgrade test results to the PR description.

Copy link
Contributor

@yuri-tceretian yuri-tceretian left a comment

Choose a reason for hiding this comment

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

Generally, good change. Only serious concern is about loading secrets from file.

alerting/notifier/channels/kafka.go Outdated Show resolved Hide resolved
alerting/notifier/channels/kafka.go Outdated Show resolved Hide resolved
alerting/notifier/channels/kafka.go Outdated Show resolved Hide resolved
alerting/notifier/channels/kafka.go Show resolved Hide resolved
@yuri-tceretian
Copy link
Contributor

yuri-tceretian commented Jan 12, 2023

LGTM after the suggestion above is committed.

Copy link
Contributor

@yuri-tceretian yuri-tceretian left a comment

Choose a reason for hiding this comment

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

LGTM. Thanks!

@ghost
Copy link
Author

ghost commented Jan 12, 2023

Thank you for reviewing @yuri-tceretian !
Am I good to merge this?

@ghost
Copy link
Author

ghost commented Jan 12, 2023

Oh, I can't merge it! It says "Only those with write access to this repository can merge pull requests."

@ghost
Copy link
Author

ghost commented Jan 12, 2023

Can someone also look at the corresponding UI changers on grafana please? grafana/grafana#61123

@ghost ghost requested a review from gotjosh January 12, 2023 20:56
@yuri-tceretian yuri-tceretian merged commit e287d43 into grafana:main Jan 13, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

None yet

5 participants