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.

Sign up for GitHub

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

Jump to bottom

Add data source for secrets #455

Merged
merged 6 commits into from
Apr 19, 2024
Merged

Add data source for secrets #455

merged 6 commits into from
Apr 19, 2024

Conversation

Aflynn50
Copy link
Contributor

@Aflynn50 Aflynn50 commented Apr 16, 2024 •
edited
Loading

Description

This PR only has very basic acceptance tests right now. It will not be possible to do an end to end test until #454 is in as without this it is not possible for applications to make use of the secret.

To access a secret in juju we are already adding in a new resource that allows you to create a new secret. But there may often be cases where the user does not wish to store the raw secret values in the terraform plan, and add them instead some other way. In this use case, a data source is needed.

A data source is added that allows a secret specified by name in a particular model to be referenced in the terraform plan.

Type of change

  • Add new data source

Environment

  • Juju controller version:
    3.3 and above

QA steps

Manual QA steps should be done to test this PR.

  • juju bootstrap lxd
  • juju add-model default
  • `juju add-secret test1 token=123123
  • Run the terraform plan below and observe no errors.
terraform {
  required_providers {
    juju = {
      version = "~> 0.11.0"
      source  = "juju/juju"
    }
  }
}

provider "juju" {}

data "juju_model" "default" {
  name = "default"
}

data "juju_secret" "datasource-secretName" {
  name = "test1"
  model = data.juju_model.default.name
}

@Aflynn50 Aflynn50 changed the title [WIP] Add data source for secrets Add data source for secrets Apr 17, 2024
cderici
cderici approved these changes Apr 17, 2024
View reviewed changes
Copy link
Contributor

@cderici cderici left a comment

Choose a reason for hiding this comment

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

LGTM, modulo some nits. QA went well, I see the id of the secret in the terraform state 👍

internal/provider/data_source_secrets.go Show resolved Hide resolved
internal/provider/data_source_secrets.go Show resolved Hide resolved
internal/provider/data_source_secrets.go Show resolved Hide resolved
internal/provider/data_source_secrets.go Show resolved Hide resolved
internal/provider/data_source_secrets_test.go Outdated Show resolved Hide resolved
@Aflynn50 Aflynn50 force-pushed the secrets-data-source branch 2 times, most recently from 11c6fd9 to 44836b5 Compare April 18, 2024 08:25
hmlanigan
hmlanigan requested changes Apr 18, 2024
View reviewed changes
Copy link
Member

@hmlanigan hmlanigan left a comment

Choose a reason for hiding this comment

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

A small thing.

internal/provider/data_source_secrets.go Outdated Show resolved Hide resolved
hmlanigan
hmlanigan reviewed Apr 18, 2024
View reviewed changes
examples/data-sources/juju_secret/data-souce.tf Outdated
Comment on lines 1 to 4
data "juju_secret" "this" {
model = "model-name"
name = "secret-name"
}
Copy link
Member

Choose a reason for hiding this comment

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

This is a good start, but please add an application example where the data.juju_secret.this.secret_id is provided to an application config.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Added, though it won't be runnable until #454 is in.

Copy link
Member

Choose a reason for hiding this comment

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

That's okay, we'll land that before RC.

examples/data-sources/juju_secret/data-souce.tf Outdated Show resolved Hide resolved
internal/provider/data_source_secrets.go Show resolved Hide resolved
internal/provider/data_source_secrets_test.go Outdated Show resolved Hide resolved
internal/provider/data_source_secrets_test.go Show resolved Hide resolved
internal/provider/data_source_secrets_test.go
Comment on lines +62 to +64
data "juju_secret" "secret_data_source" {
name = juju_secret.secret_resource.name
model = juju_model.{{.ModelName}}.name
Copy link
Member

Choose a reason for hiding this comment

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

This test is a bit contrived. But we need a good charm for this and full implementation of all 3 pieces for secrets to do that.

Please add a ToDo.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Added, there is also one at the top of this PR description, am not planning on merging until its done

internal/provider/data_source_secrets_test.go Show resolved Hide resolved
internal/provider/data_source_secrets.go Outdated Show resolved Hide resolved
hmlanigan
hmlanigan reviewed Apr 18, 2024
View reviewed changes
Copy link
Member

@hmlanigan hmlanigan left a comment

Choose a reason for hiding this comment

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

Sorry Alastair, I gave another look and found more.

docs/resources/secret.md
A resource that represents a Juju secret.
---

# juju_secret (Resource)
Copy link
Member

Choose a reason for hiding this comment

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

I wonder if we're going to have a merge conflict somewhere, you shouldn't be picking up this file. The original commit needs an example.tf too.

internal/provider/data_source_secrets.go Outdated Show resolved Hide resolved
@Aflynn50
Improve example, sort imports and minor code changes
f13ebcb 
Add a full example with a charm using the secret
Sort import statements into stanzas
Call os.Getenv only once for tests
Check if secret id is emepty by directly comparing to empty string.
@Aflynn50
Do make docs
5934da1
@Aflynn50
Update docs
609e7ed
@Aflynn50 Aflynn50 requested a review from hmlanigan April 19, 2024 14:28
hmlanigan
hmlanigan approved these changes Apr 19, 2024
View reviewed changes
Copy link
Member

@hmlanigan hmlanigan 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 for all the last minute changes!

@hmlanigan hmlanigan merged commit df009ac into juju:main Apr 19, 2024
25 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@cderici cderici cderici approved these changes

@hmlanigan hmlanigan hmlanigan approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@Aflynn50 @cderici @hmlanigan