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

Add workload Id and job ACLs #314

Merged
merged 8 commits into from
May 16, 2023

Conversation

IamTheFij
Copy link
Contributor

Adds support for passing ACLs for workloads when defining a policy.

I'm not sure how to write unit tests for this as the test section seemed like it was for acceptance testing. I wasn't able to get acceptance testing to run locally on my machine yet.

Fixes #306

Adds support for passing ACLs for workloads when defining a policy.
@hashicorp-cla
Copy link

hashicorp-cla commented Mar 22, 2023

CLA assistant check
All committers have signed the CLA.

Copy link
Contributor

@lgfa29 lgfa29 left a comment

Choose a reason for hiding this comment

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

Thanks for the PR @IamTheFij, and apologies for the delay in review it.

I left a few comments but it would also be nice to have some tests and documentation updates about this. What problems were you facing in running the tests locally?

I'm not sure if you've seen it, but we have some instructions in our README:
https://github.com/hashicorp/terraform-provider-nomad#developing-the-provider

For this work I think we can update testResourceACLPolicy_initialConfig to include some JobACL config:

func testResourceACLPolicy_initialConfig(name string) string {
return fmt.Sprintf(`
resource "nomad_acl_policy" "test" {
name = "%s"
description = "A Terraform acctest ACL policy"
rules_hcl = <<EOT
namespace "default" {
policy = "read"
capabilities = ["submit-job"]
}
EOT
}
`, name)
}

And then update testResourceACLPolicy_initialCheck to verify the job identity was set.

A more complete test would be to test the validation I mentioned. For this you can add a ExpectError to your TestStep to verify that the resource is invalid.

For the docs, we would need to update https://github.com/hashicorp/terraform-provider-nomad/blob/main/website/docs/r/acl_policy.html.markdown to include the new field. If you run make website you can preview your changes.

Let us know if you need help with any of this 🙂

nomad/resource_acl_policy.go Outdated Show resolved Hide resolved
nomad/resource_acl_policy.go Show resolved Hide resolved
nomad/resource_acl_policy.go Outdated Show resolved Hide resolved
This fails due to an error in the import validation
@IamTheFij
Copy link
Contributor Author

I managed to get acceptance testing running with some tweaks to the shell scripts used. I can cut another PR for those later, if interested. Right now I'm getting a test failure and I haven't quite been able to figure out what's wrong.

I'm seeing

=== RUN   TestResourceACLPolicy_import
    testing.go:705: Step 1 error: ImportStateVerify attributes not equivalent. Difference is shown below. Top is actual, bottom is expected.
        
        (map[string]string) {
        }
        
        
        (map[string]string) (len=5) {
         (string) (len=9) "job_acl.#": (string) (len=1) "1",
         (string) (len=15) "job_acl.0.group": (string) "",
         (string) (len=16) "job_acl.0.job_id": (string) (len=3) "job",
         (string) (len=19) "job_acl.0.namespace": (string) (len=7) "default",
         (string) (len=14) "job_acl.0.task": (string) ""
        }

I'm not sure where the expected and actual values come from. Given that the Check step passes, I'm confident the parsing is working correctly. I did some reading on how ImportStateVerify works and I've found that a resource that uses Importer.State = schema.ImportStatePassthrough should determine the state from the read function. I've verified the read function by spitting out a panic with a message including the entire resource and it appears to have the data as well. (as an aside, I'm not sure how to get it to spit out debug logs when running tests)

I've pushed the latest. Any ideas on what I'm missing?

@IamTheFij
Copy link
Contributor Author

Any advice on getting this test working so it can get across the finish line?

Copy link
Contributor

@lgfa29 lgfa29 left a comment

Choose a reason for hiding this comment

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

Looking good! Sorry for the delay in getting back to you here.

After the read method is fixed I think the only bits missing are updating the documentation and adding a CHANGELOG entry. Something like:

## 1.4.21 (Unreleased)
+ IMPROVEMENTS:
+ * resource/nomad_acl_policy: add support for `job_acl` ([#314](https://github.com/hashicorp/terraform-provider-nomad/pull/314))

## 1.4.20 (April 20, 2023)

nomad/resource_acl_policy.go Outdated Show resolved Hide resolved
nomad/resource_acl_policy.go Outdated Show resolved Hide resolved
@IamTheFij
Copy link
Contributor Author

I haven't tested the website because the make target is broken. It executes website-provider on https://github.com/hashicorp/terraform-website, but that target doesn't exist there.

It looks like the target was added back in 2018 with this PR, but as of 2022 it was reportedly absent: hashicorp/terraform-provider-tfe#503.

It looks like this is because the whole system was rewritten and the Makefile removed with this PR: hashicorp/terraform-website#2010 and then added back without this ability in this PR: hashicorp/terraform-website#2013

Copy link
Contributor

@lgfa29 lgfa29 left a comment

Choose a reason for hiding this comment

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

Hum...yeah, that is weird. I'm not sure what happened with the website preview but I ran the changes through https://registry.terraform.io/tools/doc-preview and they look good. I pushed a commit with minor touch-ups.

I also realized that we may need to update the nomad_acl_policy data source, but I have bother you enough in this PR 😄

Thank you for the contribution!

@lgfa29 lgfa29 merged commit 49ba9ac into hashicorp:main May 16, 2023
1 check passed
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.

Request: Support applying ACLs to workloads
3 participants