Refactor `grafana_datasource` and add `uid` property #301

alexjfisher · 2022-10-06T14:45:44Z

Refactor grafana_datasource

Property defaults are removed from the type and where values are needed
on datasource creation, the defaults are in the provider.

If updating an existing datasource, only properties that you want to
manage have to be specified. The API needs the post to contain several
more fields, but these can be pulled from the existing state.

uid is added as an optional property and, (in versions that support
it), is used when updating a datasource, (instead of updating by id
which has been deprecated.)

Fetching and deleting datasources by id has also been deprecated in
Grafana 9 so is replaced by fetching and deleting by name.

The managing of multiple datasources is now quicker as the previous
implementation made a number of API calls that grew exponentially with
the number of datasources.

Fixes #229

For users not using basic_auth or password properties in their
datasources restores idempotency when using Grafana 9 (see #289)

(From Grafana 9 onwards, users must use secure_json_data but this is
not idempotent and making it behave 100% correctly is currently
impossible as the Grafana API purposefully never exposes this data.)

There are still a number of issues open around using datasources in more
than one organization. None of these have been addressed in this
commit.

alexjfisher · 2022-10-06T16:03:18Z

Code changes are perhaps more or less done (for now), but tests still need work (and expanding), so leaving this as draft for now.

joernott · 2022-10-10T09:22:41Z

I just tested this branch on our servers. The module works fine but when I recreate the datasource, using the following code

    grafana_datasource { 'icinga2_influxdb':
      grafana_url         => $grafana_url,
      grafana_user        => $grafana_admin_user,
      grafana_password    => $grafana_admin_password.node_encrypt::secret,
      grafana_api_path    => $grafana_api_path,
      type                => 'influxdb',
      organization        => 'Main Org.',
      url                 => $influxdb_url,
      user                => $influxdb_grafana_user,
      password            => $influxdb_grafana_password.node_encrypt::secret,
      database            => $influxdb_icinga_database,
      access_mode         => 'proxy',
      is_default          => true,
      basic_auth          => true,
      basic_auth_user     => $influxdb_grafana_user,
      basic_auth_password => $influxdb_grafana_password.node_encrypt::secret,
    }

I get the following error when using a dashboard which uses that datasource

message:"InfluxDB Error: Bad Request"
data:Object
message:"Authentication to data source failed"
error:"Bad Request"
response:"Authentication to data source failed"
config:Object
method:"GET"
url:"api/datasources/proxy/43/query"
params:Object
data:null
precision:"ms"
inspect:Object
paramSerializer:serializeParams(e){…}
headers:Object
retry:0
hideFromInspector:false

I need to update the basic-auth passwords for influxdb manually in the UI for it to work. With the postgres datasource, it is the regular password which needs top be provided again. But I've seen the same behaviour also with the old version of the module.

Concerning the idempotence:
I have seen a similar issue in the elasticsearch API in the past. They disregard certain things reported back from the api (or from the initial object) when checking for idempotence.

Passwords can be checked by separate api calls against a login url (which means tedious extra calls). I have used something similar

alexjfisher · 2022-10-10T12:46:29Z

@joernott Which version of grafana are you using?

joernott · 2022-10-10T14:06:49Z

We use v9.0.7

alexjfisher · 2022-10-10T14:33:11Z

We use v9.0.7

I think that from version 9 onwards, the API will ignore the database and basicAuthPassword fields. You have to specify them in the secure json data instead.

Try the following. It should work, but unfortunately won't appear to be idempotent.

    grafana_datasource { 'icinga2_influxdb':
      grafana_url         => $grafana_url,
      grafana_user        => $grafana_admin_user,
      grafana_password    => $grafana_admin_password.node_encrypt::secret,
      grafana_api_path    => $grafana_api_path,
      type                => 'influxdb',
      organization        => 'Main Org.',
      url                 => $influxdb_url,
      user                => $influxdb_grafana_user,
      database            => $influxdb_icinga_database,
      access_mode         => 'proxy',
      is_default          => true,
      basic_auth          => true,
      basic_auth_user     => $influxdb_grafana_user,
      secure_json_data    => {
        basicAuthPassword => $influxdb_grafana_password.node_encrypt::secret,
        password          => $influxdb_grafana_password.node_encrypt::secret,
      },
    }

joernott · 2022-10-11T15:25:57Z

Try the following. It should work, but unfortunately won't appear to be idempotent.

This works if I leave out the .node_encrypt::secret in secure_json_data.

alexjfisher · 2022-10-11T16:30:57Z

Try the following. It should work, but unfortunately won't appear to be idempotent.

This works if I leave out the .node_encrypt::secret in secure_json_data.

Is wonder if that's a limitation of Deferred?
Currently, node_encrypt won't accept things other than strings, but if it could be made to, then perhaps

secure_json_data    => {
        basicAuthPassword => $influxdb_grafana_password,
        password          => $influxdb_grafana_password,
      }.node_encrypt::secret,

would work???

joernott · 2022-10-12T06:45:29Z

Of course, I tried that first, but .node_encrypt::secret does not support hashes. AFAIK, it is limited to strings.

alexjfisher · 2022-10-12T07:18:13Z

Of course, I tried that first, but .node_encrypt::secret does not support hashes. AFAIK, it is limited to strings.

Indeed. Doesn't look that tricky to fix that. I've been discussing this with @binford2k . Meanwhile, what version of Puppet are you using? We're fairly convinced your first attempt should have worked too.

joernott · 2022-10-12T07:33:46Z

We are currently using Puppet Enterprise LTS, which uses puppet 6.28.0. As the next LTS is around the corner, we are currently testing our code against puppet 7 as well, but we don't use it in production yet.

My first try was using node_encrypt on the two strings inside the hash. I deleted thje datasources in Grafana and let puppet recreate them. Opening a dashboard which uses the fixed UID, gave a data access error.

I retried without the node_encrypt::secret and it worked. Then I tried to use node_encrypt on the hash which did not work as it only handles strings.

alexjfisher · 2022-10-12T11:18:24Z

Looks like the issue is that the decryption works, but returns a sensitive and nothing unwraps that.

I added a bit of debug to see what data we were sending to the grafana api.

Warning: {"foo"=>#<Sensitive [value redacted]>}

I don't suppose fixing this is going to be that hard. Will get back to you, (and meanwhile found another bug I need to fix anyway)

bastelfreak · 2022-10-12T11:42:26Z

lib/puppet/provider/grafana_datasource/grafana.rb

  end

  def secure_json_data
-    datasource[:secure_json_data]
+    # The API never returns `secure` data, so we won't ever be able to tell if the current state is correct.
+    # TODO: Figure this out!!


Property defaults are removed from the type and where values are needed on datasource creation, the defaults are in the provider. If updating an existing datasource, only properties that you want to manage have to be specified. The API needs the post to contain several more fields, but these can be pulled from the existing state. `uid` is added as an optional property and, (in versions that support it), is used when updating a datasource, (instead of updating by `id` which has been deprecated.) Fetching and deleting datasources by `id` has also been deprecated in Grafana 9 so is replaced by fetching and deleting by `name`. The managing of multiple datasources is now quicker as the previous implementation made a number of API calls that grew exponentially with the number of datasources. Fixes voxpupuli#229 For users not using `basic_auth` or `password` properties in their datasources restores idempotency when using Grafana 9 (see voxpupuli#289) (From Grafana 9 onwards, users must use `secure_json_data` but this is not idempotent and making it behave 100% correctly is currently impossible as the Grafana API purposefully never exposes this data.) There are still a number of issues open around using datasources in more than one organization. None of these have been addressed in this commit.

fixes voxpupuli#304

alexjfisher · 2022-10-12T11:43:59Z

@joernott Fixed. (I think!)
See https://github.com/voxpupuli/puppet-grafana/pull/301/files#diff-421b295d220e3e337962d097276114357c34b43ba09ffc15ca5c7e97ab6a5476R113-R125

Also fixed is fetching and deleting of datasources when the datasource name contains spaces.

joernott · 2022-10-12T15:55:12Z

The fix did it. using .node_encrypt::secret on the strings inside the json does no longer break my datasources.

bastelfreak · 2022-10-17T13:45:47Z

spec/acceptance/grafana_datasource_spec.rb

+
+supported_versions.each do |grafana_version|
+  describe "grafana_datasource with Grafana version #{grafana_version}" do
+    prepare_host


you cann prepare_host here and also in line 21. Isn't that redundant?

bastelfreak · 2022-10-17T13:47:22Z

spec/acceptance/grafana_datasource_spec.rb

+            apply_manifest_on(default, manifest, catch_failures: true)
+          end
+
+          it 'is idempotent' do


@alexjfisher now with the existing code, is it idempotent but only works on Grafana 8 and older? And the new code isn't idempotent for security data but works on Grafana 9?

secure_json_data was never idempotent, but before grafana 9 you could use the other non secure fields instead.

Removing the property defaults (empty strings) from password and basic_auth_password means that if you were never setting these, upgrading to grafana 9 won't result in the type becoming non idempotent for no reason.

alexjfisher force-pushed the refactor_datasource branch from a75d100 to ef2a665 Compare October 6, 2022 16:00

alexjfisher added the enhancement New feature or request label Oct 6, 2022

This was referenced Oct 6, 2022

Feature/datasource uid #300

Closed

WIP: Add version 9 acceptance tests supported_versions #299

Closed

Fix idempotent issue in grafana_datasource provider #297

Closed

alexjfisher force-pushed the refactor_datasource branch from ef2a665 to 48ce6e8 Compare October 10, 2022 09:29

alexjfisher force-pushed the refactor_datasource branch from 48ce6e8 to ccbcaf9 Compare October 10, 2022 13:00

alexjfisher force-pushed the refactor_datasource branch 3 times, most recently from 531387c to 1f38f9c Compare October 11, 2022 13:08

alexjfisher marked this pull request as ready for review October 11, 2022 16:41

alexjfisher force-pushed the refactor_datasource branch from 1f38f9c to 11d432e Compare October 12, 2022 11:38

bastelfreak reviewed Oct 12, 2022

View reviewed changes

alexjfisher added 2 commits October 12, 2022 12:43

Fix grafana_folder permissions idempotency

8baa53c

fixes voxpupuli#304

alexjfisher force-pushed the refactor_datasource branch from 11d432e to 8baa53c Compare October 12, 2022 11:43

bastelfreak reviewed Oct 17, 2022

View reviewed changes

bastelfreak approved these changes Oct 17, 2022

View reviewed changes

alexjfisher changed the title ~~Refactor grafana_datasource~~ Refactor grafana_datasource and add uid property Oct 18, 2022

alexjfisher merged commit 958726d into voxpupuli:master Oct 18, 2022

alexjfisher deleted the refactor_datasource branch October 18, 2022 12:27

tobias-urdin mentioned this pull request Oct 20, 2022

grafana_datasource keeps updating basic_auth_password, no longer idempotent since grafana 9.0 #289

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Refactor `grafana_datasource` and add `uid` property #301

Refactor `grafana_datasource` and add `uid` property #301

alexjfisher commented Oct 6, 2022 •

edited

Loading

alexjfisher commented Oct 6, 2022

joernott commented Oct 10, 2022 •

edited

Loading

alexjfisher commented Oct 10, 2022

joernott commented Oct 10, 2022

alexjfisher commented Oct 10, 2022

joernott commented Oct 11, 2022

alexjfisher commented Oct 11, 2022

joernott commented Oct 12, 2022

alexjfisher commented Oct 12, 2022

joernott commented Oct 12, 2022

alexjfisher commented Oct 12, 2022

bastelfreak Oct 12, 2022

alexjfisher commented Oct 12, 2022

joernott commented Oct 12, 2022

bastelfreak Oct 17, 2022

bastelfreak Oct 17, 2022

alexjfisher Oct 17, 2022

alexjfisher Oct 17, 2022

Refactor grafana_datasource and add uid property #301

Refactor grafana_datasource and add uid property #301

Conversation

alexjfisher commented Oct 6, 2022 • edited Loading

alexjfisher commented Oct 6, 2022

joernott commented Oct 10, 2022 • edited Loading

alexjfisher commented Oct 10, 2022

joernott commented Oct 10, 2022

alexjfisher commented Oct 10, 2022

joernott commented Oct 11, 2022

alexjfisher commented Oct 11, 2022

joernott commented Oct 12, 2022

alexjfisher commented Oct 12, 2022

joernott commented Oct 12, 2022

alexjfisher commented Oct 12, 2022

bastelfreak Oct 12, 2022

Choose a reason for hiding this comment

alexjfisher commented Oct 12, 2022

joernott commented Oct 12, 2022

bastelfreak Oct 17, 2022

Choose a reason for hiding this comment

bastelfreak Oct 17, 2022

Choose a reason for hiding this comment

alexjfisher Oct 17, 2022

Choose a reason for hiding this comment

alexjfisher Oct 17, 2022

Choose a reason for hiding this comment

Refactor `grafana_datasource` and add `uid` property #301

Refactor `grafana_datasource` and add `uid` property #301

alexjfisher commented Oct 6, 2022 •

edited

Loading

joernott commented Oct 10, 2022 •

edited

Loading