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

Resource: Add LBAC for datasources data_source_config_lbac_rules #1797

Merged
merged 21 commits into from
Dec 2, 2024

Conversation

eleijonmarck
Copy link
Contributor

@eleijonmarck eleijonmarck commented Sep 9, 2024

why/what
We have seen that users would love to provision their lbac rules using terraform. this implements a experimental feature that will be highly developed. this is the first part of LBAC rules resource for terraform.

dependancies:

ability to run terraform plan, terraform apply and terraform destroy of the rules resource

Example terraform:

resource "grafana_data_source" "test" {
  type = "loki"
  name = "loki"
  url  = "http://localhost:3100"
  basic_auth_enabled  = true
  basic_auth_username = "username"

  lifecycle {
    ignore_changes = [json_data_encoded, http_headers]
  }
}

resource "grafana_team" "test" {
	name = "test"
}

resource "grafana_data_source_config_lbac_rules" "test" {
    datasource_uid = grafana_data_source.test.uid
    rules = jsonencode({
        "${grafana_team.test.uid}" = [
            "{ foo != \"bar\", foo !~ \"baz\" }",
            "{ foo = \"qux\" }"
        ]
    })
}

why is this difficult

lbac for datasources terraform resource, why is it a bit tricky?

the bidirection of lbac_rules -> data_source:jsonData, meaning we are updating the jsondata with another resource impacting another resource
Image


proposals and thought process

there is no ID for the json field teamHttpHeaders

  • so the state is only “stored” in the datasource as a whole, therefore leaning on copying the data_source_config resource more
    this is a resource that will be heavily used
  • i would like to have the internal state for terraform (some customers create about 500 rules and more), which clogs the datasource, i would like to copy the setup from grafana_data_source_permission_item for this. as it suits well for how we would like to setup this resource
  • instead we make use of the api as it is. it is bulk update anyway. so we assume that all of the teams will be provided in the resource of resource_data_source_config_lbac_rules

proposal of all rules combined into one resource

benefits

  • one api call to grafana
  • mimicing the get/update apis

tradeoffs

  • clonky json encoding (could be fixed)
  • not ideal for teams managing their own set of rules
resource "grafana_data_source_config_lbac_rules" "test" {
    datasource_uid = grafana_data_source.test.uid
	org_id = grafana_team.test.org_id
    rules = jsonencode({
        "${grafana_team.test.id}" = [
            "{ foo != \"bar\", foo !~ \"baz\" }",
            "{ foo = \"qux\" }"
        ]
    })
}

proposal of each rule being a resource

benefits

  • each team rules, would be able to be owned by a team. this is preferred from a user perspective

tradeoffs

  • too many api calls to grafana, if we have 500 rules we would have 500 api calls to the datasource endpoint
resource "grafana_data_source_config_lbac_rule" "test" {
    datasource_uid = grafana_data_source.test.uid
    org_id = grafana_team.test.org_id
    team_id = "1"
    rules = [
            "{ foo != \"bar\", foo !~ \"baz\" }",
            "{ foo = \"qux\" }"
        ]
}

proposal of embedding it into a resource and then add it to the json

benefits:

  • easier to implement

tradeoff

  • management of lbac rules would lie inside of the json and we would not be able to move the storage of the lbac rules

It's a tradeoff of where you want the complexity to be. In Grafana with a new way to store this, or in the provider extracting it from json data, and possibly running into versioning issues.

I guess the less complex option is to leave everything as-is, isn't it? Have people manage those new fields in the existing json data? Perhaps provide a datasource helper for this, if the value is not trivial to build/represent

data "grafana_datasource_team_headers" "helper" { ... }

resource "grafana_datasource" "test" {
  ...
  json_data_encoded = jsonencode({
     teamHeaders = data.grafana_datasource_team_headers.helper.value
     otherfields
  })
}

run tests via

GRAFANA_VERSION=11.1.0 
GRAFANA_URL=http://localhost:3000 
GRAFANA_AUTH=admin:admin 
TESTARGS="-run TestAccDataSourceLBAC_inOrg" make testacc-enterprise

The teamHttpHeaders resource is a datasource json field.

epic

basically this introduced a way to add/update a datasource json field
lbac-api

here is the doc for it. https://grafana.com/docs/grafana/latest/administration/data-source-management/teamlbac/


next steps

improvements:

  • API of the resource could be made so that there is no need of escaped JSON. To enforces types and schema
  1. Create a template file (logql_query.tmpl):
{f="alice", b="bob", a="abc"}
resource "grafana_data_source_config_lbac_rules" "test" {
    datasource_uid = grafana_data_source.test.uid
    rules = jsonencode({
        "1" = [
            "{ foo != \"bar\", foo !~ \"baz\" }",
            "{ foo = \"qux\" }"
        ],
        "2" = [
            templatefile("${path.module}/logql_query.tmpl", {})
        ]
    })
}

```[tasklist]
- [ ] fix the lifecycle of the jsondata within grafana
  - [ ] make sure you cannot set teamhttpheaders from the json api (updateDatasource), ONLY through lbac/team api
  - [ ] magic diffsuprressfunc to ignore changes on json_data_encoded for the specific field teamHttpHeaders

@eleijonmarck eleijonmarck self-assigned this Sep 9, 2024
Copy link

github-actions bot commented Sep 9, 2024

In order to lower resource usage and have a faster runtime, PRs will not run Cloud tests automatically.
To do so, a Grafana Labs employee must trigger the cloud acceptance tests workflow manually.

@eleijonmarck eleijonmarck changed the title [WIP]: LBAC for datasources Resource: LBAC for datasources data_source_lbac_rules Sep 17, 2024
@eleijonmarck eleijonmarck changed the title Resource: LBAC for datasources data_source_lbac_rules Resource: Add LBAC for datasources data_source_lbac_rules Sep 17, 2024
@eleijonmarck eleijonmarck marked this pull request as ready for review September 17, 2024 09:01
@eleijonmarck eleijonmarck requested review from a team as code owners September 17, 2024 09:01
@undefinedhuman
Copy link

Hey there, is there an ETA when this feature will be shipped? Or is there anything that still needs to be done?

@eleijonmarck
Copy link
Contributor Author

@undefinedhuman we are working on the backend side of things for this. So this will wait for clear resource handling

@undefinedhuman
Copy link

Thank you very much!

@eleijonmarck eleijonmarck requested a review from a team as a code owner November 8, 2024 15:43
@Robsta86
Copy link

Robsta86 commented Nov 18, 2024

What's the status of this? I was using teamHttpHeaders to configure lbac via the grafana_datasource_config, but that seems to be broken after these 2 are merged:

#1857
#1856

Error: teamHttpHeaders is a reserved key and cannot be used in JSON data. Use the data_source_config_lbac_rules resource instead

Now I am no longer able to manage my loki lbac rules via terraform, resulting in new onboarded teams to have full query access to my log datasource.

@eleijonmarck eleijonmarck force-pushed the eleijonmarck/lbac-for-datasources/bulk-api branch from 9d782ee to 0f08921 Compare November 26, 2024 10:32
@eleijonmarck eleijonmarck force-pushed the eleijonmarck/lbac-for-datasources/bulk-api branch from 0f08921 to dbe2076 Compare November 27, 2024 09:33
@eleijonmarck eleijonmarck force-pushed the eleijonmarck/lbac-for-datasources/bulk-api branch from dbe2076 to 692ac09 Compare November 27, 2024 09:41
@eleijonmarck eleijonmarck changed the title Resource: Add LBAC for datasources data_source_lbac_rules Resource: Add LBAC for datasources data_source_config_lbac_rules Nov 27, 2024
@eleijonmarck eleijonmarck force-pushed the eleijonmarck/lbac-for-datasources/bulk-api branch 2 times, most recently from eb413b9 to 0c46185 Compare November 27, 2024 13:49
@eleijonmarck eleijonmarck force-pushed the eleijonmarck/lbac-for-datasources/bulk-api branch from 0c46185 to 26e8f05 Compare November 27, 2024 13:51
@eleijonmarck eleijonmarck force-pushed the eleijonmarck/lbac-for-datasources/bulk-api branch from ec3db57 to fb29a0a Compare November 27, 2024 14:02
@eleijonmarck eleijonmarck force-pushed the eleijonmarck/lbac-for-datasources/bulk-api branch from 0869822 to 3b3c344 Compare November 27, 2024 14:59
Copy link
Contributor

@IevaVasiljeva IevaVasiljeva left a comment

Choose a reason for hiding this comment

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

Mostly looks good, but I've added a few questions.

})
}

# TODO: add back 5th of dec
Copy link
Contributor

Choose a reason for hiding this comment

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

This commended out code block also appears in the docs, so we should really remember to uncomment the code before we do the next Terraform release.

Or maybe just track this as an action item in an issue rather than having it here.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

il track it in an item instead 👍

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Copy link
Contributor

@IevaVasiljeva IevaVasiljeva left a comment

Choose a reason for hiding this comment

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

Nice, I've added some small comments, but once those are fixed it looks ready to me 👌

Manages LBAC rules for a data source.
!> Warning: The resource is experimental and will be subject to change. This resource manages the entire LBAC rules tree, and will overwrite any existing rules.
Official documentation https://grafana.com/docs/grafana/latest/administration/data-source-management/teamlbac/HTTP API https://grafana.com/docs/grafana/latest/developers/http_api/datasource_lbac_rules/
This resource requires Grafana >=11.4.0.
Copy link
Contributor

Choose a reason for hiding this comment

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

I think we actually need to set it to 11.5.0 - 11.4.0 will not include the changes that we need (see this internal thread).


json_data_encoded = jsonencode({
authType = "default"
## basicAuthPassword = "<>"
Copy link
Contributor

Choose a reason for hiding this comment

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

Is this line needed?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

it's not needed at all. just showing that you need to have a password to connect with the datasource from basicAuth

Comment on lines 44 to 48
type LBACRule struct {
TeamID types.String `tfsdk:"team_id"`
TeamUID types.String `tfsdk:"team_uid"`
Rules []types.String `tfsdk:"rules"`
}
Copy link
Contributor

Choose a reason for hiding this comment

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

Is this actually used?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

wait it is unused, i should really start to make my structs private on first creation. how do you spot this? with the linter

},
"rules": schema.StringAttribute{
Required: true,
Description: "JSON-encoded LBAC rules for the data source. Map of team IDs to lists of rule strings.",
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
Description: "JSON-encoded LBAC rules for the data source. Map of team IDs to lists of rule strings.",
Description: "JSON-encoded LBAC rules for the data source. Map of team UIDs to lists of rule strings.",

)

func TestAccDataSourceConfigLBACRules_basic(t *testing.T) {
testutils.CheckEnterpriseTestsEnabled(t, ">=11.4.0")
Copy link
Contributor

Choose a reason for hiding this comment

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

Should also be 11.5.0.

@eleijonmarck eleijonmarck force-pushed the eleijonmarck/lbac-for-datasources/bulk-api branch from dd4f1cf to fd76b29 Compare December 2, 2024 11:04
@eleijonmarck
Copy link
Contributor Author

Tested feature fully ✅ :

  • rejects the teamHttpHeaders field in json when trying to update a json field in the datasource resource.
  • do not change state when data source was provisioned via terraform and users manually adds a rule / removes rules
  • correctly adds resource
  • correctly delete resource
  • correctly updates/changes the resource, if user adds more rules

This feature would be issued together with the release of Grafana version 11.5 in 28th of January

@eleijonmarck eleijonmarck merged commit a2ff67e into main Dec 2, 2024
26 checks passed
@eleijonmarck eleijonmarck deleted the eleijonmarck/lbac-for-datasources/bulk-api branch December 2, 2024 12:19
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.

4 participants