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

Added ACL Support to the Terraform Provider #64

Merged
merged 10 commits into from
Feb 8, 2024

Conversation

unasra
Copy link
Collaborator

@unasra unasra commented Feb 6, 2024

No description provided.

@unasra unasra requested a review from mathewab February 6, 2024 11:26

func (r *AclResource) Schema(ctx context.Context, req resource.SchemaRequest, resp *resource.SchemaResponse) {
resp.Schema = schema.Schema{
MarkdownDescription: "",
Copy link
Collaborator

Choose a reason for hiding this comment

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

Please add description

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Added


func (d *AclDataSource) Schema(ctx context.Context, req datasource.SchemaRequest, resp *datasource.SchemaResponse) {
resp.Schema = schema.Schema{
MarkdownDescription: "",
Copy link
Collaborator

Choose a reason for hiding this comment

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

Please add description

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Added

Copy link
Collaborator

Choose a reason for hiding this comment

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

You also need to add examples/resources/bloxone_dns_acl.tf and examples/data-sources/bloxone_dns_acls.tf

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Both files are added


# bloxone_dns_acls (Data Source)

Retrieves information about existing Authoritative DNS ACLs.
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
Retrieves information about existing Authoritative DNS ACLs.
Retrieves information about existing named Access Control Lists.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Ahh, my comment vanished. @mathewab Please add the suggested change in the corresponding service file. Would make it easier for the addressee. Just a thought :)

Copy link
Collaborator

Choose a reason for hiding this comment

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

Sure :) Will keep that in mind next time .

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Addressed


# bloxone_dns_acl (Resource)

Manages an Access Control List (ACL).
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
Manages an Access Control List (ACL).
Manages a named Access Control List (ACL).

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Addressed

Comment on lines 20 to 22
resource "bloxone_dns_acl" "test" {
name = "test-acl"
}
Copy link
Collaborator

@mathewab mathewab Feb 6, 2024

Choose a reason for hiding this comment

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

Even though an empty ACL is possible, it is not useful. For our examples, it would be better to have an ACL with basic list, where the transfer_acl can follow the following order

deny any
allow acl 192.168.1.0/24
deny ip 192.168.1.1
allow tsig test-tsig.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Addressed

Copy link
Collaborator

Choose a reason for hiding this comment

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

I see that you have removed the nested acl altogether. Sorry if I was not clear in my comments. We do want the nested ACL as well, but not a nested ACL that is empty. So the whole resource should look something like this

resource "bloxone_dns_auth_zone" "example" {
  fqdn         = "domain.com."
  primary_type = "cloud"

  # Other optional fields
  comment = "Example of an Authoritative Zone"
  tags = {
    site = "Site A"
  }
  query_acl = [
    {
      access  = "deny"
      element = "ip"
      address = "192.168.1.1"
    },
    {
      element = "acl"
      acl = bloxone_dns_acl.example_acl.id
    },
    {
      access  = "allow"
      element = "tsig_key"
      tsig_key = {
        key = bloxone_keys_tsig.example_tsig.id
      }
    },
    {
      access  = "deny"
      element = "any"
    },
  ]
  update_acl = [
    {
      access  = "allow"
      element = "any"
    },
  ]
  transfer_acl = [
    {
      access  = "allow"
      element = "any"
    },
  ]

}

resource "bloxone_keys_tsig" "example_tsig" {
  name = "example_tsig.domain.com."
}

resource "bloxone_dns_acl" "example_acl" {
    name = "example_acl"
    elements = [
        {
        access  = "deny"
        element = "ip"
        address = "192.168.1.0/24"
        },
    ]
}

resource "bloxone_dns_acl" "test" {
name = "test-acl"
}

resource "bloxone_dns_auth_zone" "example" {
fqdn = "example.com."
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
fqdn = "example.com."
fqdn = "domain.com."

Copy link
Collaborator

Choose a reason for hiding this comment

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

I believe we should set standards for naming in the tf files. Maybe we can talk about this offline and come up with a standard.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Fixed

@@ -13,6 +13,14 @@ Manages an authoritative zone.
## Example Usage

```terraform
resource "bloxone_keys_tsig" "test" {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Don't use "test" as name in the examples

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Addressed

Comment on lines +558 to +561
Check: resource.ComposeTestCheckFunc(
testAccCheckAuthZoneExists(context.Background(), resourceName, &v),
resource.TestCheckResourceAttr(resourceName, "transfer_acl.0.access", "deny"),
resource.TestCheckResourceAttr(resourceName, "transfer_acl.0.element", "tsig_key"),
Copy link
Collaborator

Choose a reason for hiding this comment

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

Please add check to verify Tsig key ID as well.

Applies to all UTs

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Verification added

Comment on lines 550 to 553
Check: resource.ComposeTestCheckFunc(
testAccCheckAuthZoneExists(context.Background(), resourceName, &v),
resource.TestCheckResourceAttr(resourceName, "transfer_acl.0.element", "acl"),
),
Copy link
Collaborator

Choose a reason for hiding this comment

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

Please add check to verify ACL ID as well.

Applies to all UTs

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Done

@unasra unasra requested a review from mathewab February 7, 2024 17:53
@@ -1,5 +1,9 @@
resource "bloxone_keys_tsig" "example_tsig" {
name = "test-tsig."
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
name = "test-tsig."
name = "example_tsig.domain.com."

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Modified as instructed !

Comment on lines 20 to 22
resource "bloxone_dns_acl" "test" {
name = "test-acl"
}
Copy link
Collaborator

Choose a reason for hiding this comment

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

I see that you have removed the nested acl altogether. Sorry if I was not clear in my comments. We do want the nested ACL as well, but not a nested ACL that is empty. So the whole resource should look something like this

resource "bloxone_dns_auth_zone" "example" {
  fqdn         = "domain.com."
  primary_type = "cloud"

  # Other optional fields
  comment = "Example of an Authoritative Zone"
  tags = {
    site = "Site A"
  }
  query_acl = [
    {
      access  = "deny"
      element = "ip"
      address = "192.168.1.1"
    },
    {
      element = "acl"
      acl = bloxone_dns_acl.example_acl.id
    },
    {
      access  = "allow"
      element = "tsig_key"
      tsig_key = {
        key = bloxone_keys_tsig.example_tsig.id
      }
    },
    {
      access  = "deny"
      element = "any"
    },
  ]
  update_acl = [
    {
      access  = "allow"
      element = "any"
    },
  ]
  transfer_acl = [
    {
      access  = "allow"
      element = "any"
    },
  ]

}

resource "bloxone_keys_tsig" "example_tsig" {
  name = "example_tsig.domain.com."
}

resource "bloxone_dns_acl" "example_acl" {
    name = "example_acl"
    elements = [
        {
        access  = "deny"
        element = "ip"
        address = "192.168.1.0/24"
        },
    ]
}

@mathewab mathewab merged commit ed1fbde into infobloxopen:master Feb 8, 2024
3 checks 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.

3 participants