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 new bastionzero_db_target resource #67

Merged
merged 46 commits into from
Nov 15, 2023
Merged

Conversation

ymarcus93
Copy link
Member

@ymarcus93 ymarcus93 commented Nov 6, 2023

  • Add support for managing Db targets with the new bastionzero_db_target resource
  • Add bastionzero_supported_database_configs data source which retrieves list of supported database auth configurations
  • End-to-end tests
  • Update docs

@ymarcus93 ymarcus93 added the enhancement New feature or request label Nov 6, 2023
@ymarcus93 ymarcus93 self-assigned this Nov 6, 2023
@ymarcus93 ymarcus93 marked this pull request as draft November 6, 2023 21:56
@ymarcus93 ymarcus93 force-pushed the feat/db-target-resource branch 2 times, most recently from c447d7f to f7c652b Compare November 10, 2023 17:58
@ymarcus93 ymarcus93 changed the base branch from master to feat/environments-as-proxies November 10, 2023 18:03
@ymarcus93 ymarcus93 requested a review from lipmas November 13, 2023 13:13
@ymarcus93 ymarcus93 marked this pull request as ready for review November 13, 2023 18:00
@ymarcus93 ymarcus93 changed the title <WIP> Add new bastionzero_db_target resource Add new bastionzero_db_target resource Nov 13, 2023
### Db target via proxy target

Create a Db target with the default authentication configuration
(non-passwordless), and use a Bzero target to proxy the connection to the

Choose a reason for hiding this comment

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

We should modify this to say "Linux target" or something similar. We shouldn't refer to targets any longer by "bzero" since we separated out targets vs. agents.

for each in data.bastionzero_environments.example.environments
: each if each.name == "example-env"
])
# Find Bzero target with name "ubuntu". `proxy_target` is null if not found

Choose a reason for hiding this comment

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

Find the Linux target


Create a Db target with the default authentication configuration
(non-passwordless) and a proxy environment. When a user connects to this target,
the Bzero or Cluster target with the least number of open connections in this

Choose a reason for hiding this comment

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

Gets a little wonky here since we still call it a proxy target, but in theory I think it should be proxy agent... Maybe we can just say "Linux, Windows, or Kubernetes target"

for each in data.bastionzero_environments.example.environments
: each if each.name == "example-env"
])
# Find Bzero target with name "ubuntu". `proxy_target` is null if not found

Choose a reason for hiding this comment

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

Same comment as above - "Find Linux target"

for each in data.bastionzero_environments.example.environments
: each if each.name == "example-env"
])
# Find Bzero target with name "ubuntu". `proxy_target` is null if not found

Choose a reason for hiding this comment

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

Find Linux target

Copy link
Member Author

Choose a reason for hiding this comment

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

This data source list can include Linux or Windows targets, so I've changed it to that. Lmk if that's okay.

- `environment_id` (String) The target's environment's ID.
- `name` (String) The target's name.
- `remote_host` (String) The target's hostname or IP address.
- `remote_port` (Number) The port of the Db server accessible via the target. This field is required for all databases; however, if `database_authentication_config.cloud_service_provider` is equal to `GCP`, then the value will be ignored when connecting to the database.

Choose a reason for hiding this comment

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

Let's add in here that for GCP, we recommend using value 0

page_title: "bastionzero_db_target Resource - terraform-provider-bastionzero"
subcategory: "Target"
description: |-
Provides a BastionZero database target. Database targets configure remote access to database servers running on Bzero bzero_target targets or Cluster cluster_target targets.

Choose a reason for hiding this comment

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

Windows, Linux, or Kubernetes targets


# bastionzero_db_target (Resource)

Provides a BastionZero database target. Database targets configure remote access to database servers running on [Bzero](bzero_target) targets or [Cluster](cluster_target) targets.

Choose a reason for hiding this comment

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

Linux, Windows, or Kubernetes targets

- `database_authentication_config` (Attributes) Information about the db target's database authentication configuration. If this attribute is left unconfigured, the target is configured with the default, non-passwordless database configuration. (see [below for nested schema](#nestedatt--database_authentication_config))
- `local_port` (Number) The port of the Db daemon's localhost server that is spawned on the user's machine on connect. If this attribute is left unconfigured, an available port will be chosen when the target is connected to.
- `proxy_environment_id` (String) The target's proxy environment's ID (ID of the backing proxy environment).
- `proxy_target_id` (String) The target's proxy target's ID (ID of a [Bzero](bzero_target) or [Cluster](cluster_target) target).

Choose a reason for hiding this comment

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

Linux, Windows, or Kubernetes target

@ymarcus93 ymarcus93 requested a review from asamborski November 15, 2023 17:55
@@ -19,7 +19,7 @@ func TestAccADBashDataSource_Basic(t *testing.T) {

acctest.SkipIfNotInAcceptanceTestMode(t)
acctest.PreCheck(ctx, t)
acctest.FindNEnvironmentsOrSkip(t, env)
acctest.FindNEnvironmentsOrSkipAsPolicyEnvironment(t, env)
Copy link
Member

Choose a reason for hiding this comment

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

I definitely missed the reason for transitioning a bunch of tests to use environment policy objects in this PR- what was the reason for doing this?

Copy link
Member Author

Choose a reason for hiding this comment

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

In the policy TF acctests, we need policies.Environment object. But honestly, looking at this code again I could actually get away with just one func and re-map from environments.Envrionment to policies.Environment where needed

bastionzero/target/dbtarget/resource_dbtarget_test.go Outdated Show resolved Hide resolved
lipmas
lipmas previously approved these changes Nov 15, 2023
Copy link
Member

@lipmas lipmas left a comment

Choose a reason for hiding this comment

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

Finished manually testing the db target resource and looking good!

Comment on lines +211 to +214
"last_agent_update": resource_schema.StringAttribute{
Computed: true,
Description: fmt.Sprintf("The time this target's proxy agent last had a transition change in status %s. Null if there has not been a single transition change.", internal.PrettyRFC3339Timestamp()),
},
Copy link
Member

@lipmas lipmas Nov 15, 2023

Choose a reason for hiding this comment

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

Actually one more question about this attribute of the resource. Since this is just a "computed" value returned from the api and could be constantly changing and shows up in the tf plan when other changes are made does it really make sense to include it in the resource? What advanatges are there for having it in the resource? Just to use as an output value? Same argument could be made for the status, region attributes as well

Terraform will perform the following actions:

  # bastionzero_db_target.example will be updated in-place
  ~ resource "bastionzero_db_target" "example" {
      ~ agent_public_key               = "n/a" -> (known after apply)
      ~ agent_version                  = "n/a" -> (known after apply)
        id                             = "3eb923b8-ccf0-4457-8f07-46a5becdac2b"
      + last_agent_update              = (known after apply)
        name                           = "tf-example-psql-db-target"
      - proxy_environment_id           = "312d0fc0-5679-4a47-9f5a-af4884d3bee5" -> null
      + proxy_target_id                = "b0c0a199-8e74-4a05-ac07-e904a33a0e6f"
      ~ region                         = "n/a" -> (known after apply)
      ~ status                         = "Online" -> (known after apply)
        # (5 unchanged attributes hidden)
    }

Plan: 0 to add, 1 to change, 0 to destroy.

Copy link
Member Author

@ymarcus93 ymarcus93 Nov 15, 2023

Choose a reason for hiding this comment

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

Since this is just a "computed" value returned from the api and could be constantly changing and shows up in the tf plan when other changes are made

Just to reiterate what I think you're talking about above...It will only show (known after apply) like in the example you posted when one of the bastionzero_db_target resource's configurable attributes is modified. If you refer to one of the resource's read-only/computed attributes in an output or somewhere else (like another resource or data source) without changing the resource's configurable attributes, it should show the refreshed value (if changed). Here is an example when I tried to restart the backing agent of a db target (using zli) with some outputs that refer to last_agent_update and status (outputs start with foo below) and then running terraform apply:

Note: Objects have changed outside of Terraform

Terraform detected the following changes made outside of Terraform since the last "terraform apply" which may have affected this plan:

  # bastionzero_db_target.example has changed
  ~ resource "bastionzero_db_target" "example" {
        id                             = "c7e9f2c5-e078-4699-94a5-f2ac20108b40"
      ~ last_agent_update              = "2023-11-15T19:29:04Z" -> "2023-11-15T19:34:28Z"
        name                           = "example-splitcert-db-target"
      ~ status                         = "Online" -> "Restarting"
        # (9 unchanged attributes hidden)
    }


Unless you have made equivalent changes to your configuration, or ignored the relevant attributes using ignore_changes, the following plan may include
actions to undo or respond to these changes.

─────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────

Changes to Outputs:
  ~ foo_status                      = "Online" -> "Restarting"
  ~ foo_update                      = "2023-11-15T19:29:04Z" -> "2023-11-15T19:34:28Z"

You can apply this plan to save these new output values to the Terraform state, without changing any real infrastructure.

does it really make sense to include it in the resource?

All of these attributes: status, last_agent_update, region, agent_public_key, etc. are part of the CRUD BastionZero API responses and Hashicorp recommends making the TF resource/attribute schema map the underlying API as close as possible:

A Terraform resource and associated schema should follow the naming and structure of the API, unless the it degrades the user experience or works in a way counter to a user's expectations of Terraform.

Source: https://developer.hashicorp.com/terraform/plugin/best-practices/hashicorp-provider-design-principles#resource-and-attribute-schema-should-closely-match-the-underlying-api

That link includes the benefits for following this practice.

Furthermore, it would be a bit jarring for the bastionzero_db_target data source to include these attributes, but not in the resource itself. Meaning if a user wanted to refer to any of these fields, they would have to do an extra API call / use the data source for a resource they've already created in TF. But under the hood, the resource should already have these fields since the refresh/planning procedure calls the GET API already for the resource (and that API returns those fields anyways).


Also, if you're wondering why some of the other computed/read-only values do not display (known after apply), this is because I've added plan modifiers that prevent this behavior for attributes that cannot change even after update--id and type are examples of this.

I agree though it can look a bit noisy. But this is also what happens in bastionzero_environment.targets as well (I think you had a similar offline comment about that a long time ago).

Lastly, in the example you mentioned above, all of those computed attributes can potentially change after update, especially when going from proxy_environment_id --> proxy_target_id (or changing proxy_target_id to another proxy target):

  • agent_public_key will be different if proxy_target_id changes to a different underlying agent or if you switch from proxy env --> proxy target (n/a --> to actual key)
  • agent_version same logic as above
  • last_agent_update same logic as above
  • region same logic as above
  • status same logic as above (e.g. switching to a diff proxy target whose status is no longer Online)

Copy link
Member

Choose a reason for hiding this comment

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

Thanks for the detailed explanation and links those were helpful. I definitely agree with you overall we should keep them in the resource.

It will only show (known after apply) like in the example you posted when one of the bastionzero_db_target resource's configurable attributes is modified.

Yup true so that does makes this considerably less annoying since it wont ever show up as a update in the terraform plan unless there is a real configurable change.

All of these attributes: status, last_agent_update, region, agent_public_key, etc. are part of the CRUD BastionZero API responses and Hashicorp recommends making the TF resource/attribute schema map the underlying API as close as possible:

Alright i guess that makes sense. I do see the value in that but was still wondering if making it a subset of the api would also make sense and reduce noise. I was struggling to think of any practical reasons why last_agent_update or status would need to be referenced in terraform (other than as output values for display purposes maybe). Its a little easier to see how something like agent_public_key may need to be referenced in another resource. I guess technically you could have other resources depend on the status or last_agent_update attributes but then its going to always cascade into plan changes when those attributes change. Practically speaking i dont think anyone would want that. All that said i guess there really isnt much harm in including them just in case.

Furthermore, it would be a bit jarring for the bastionzero_db_target data source to include these attributes, but not in the resource itself. Meaning if a user wanted to refer to any of these fields, they would have to do an extra API call / use the data source for a resource they've already created in TF. But under the hood, the resource should already have these fields since the refresh/planning procedure calls the GET API already for the resource (and that API returns those fields anyways).

yeah point very well taken here - it would be annoying to have to use a data source to reference one of these attributes when you the resource is already part of your terraform model. So if we have the attributes in the data source we should also have them in the resource.

asamborski
asamborski previously approved these changes Nov 15, 2023
Copy link

@asamborski asamborski left a comment

Choose a reason for hiding this comment

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

LGTM

Base automatically changed from feat/environments-as-proxies to master November 15, 2023 23:43
@ymarcus93 ymarcus93 dismissed stale reviews from asamborski and lipmas November 15, 2023 23:43

The base branch was changed.

+ New helper flatexpand func: internal.ExpandFrameworkObject() which
  converts a Terraform types.object to an arbitrary type
+ Add generator for the DatabaseAuthenticationConfig type
There is unfortunately some duplication in the field names and
descriptions between the data source and resource schema definitions. Will try at a later
point to remove this duplication
@ymarcus93 ymarcus93 force-pushed the feat/db-target-resource branch from 698e04f to 69ed9c6 Compare November 15, 2023 23:45
@ymarcus93 ymarcus93 requested a review from lipmas November 15, 2023 23:45
@ymarcus93 ymarcus93 merged commit 17232da into master Nov 15, 2023
11 checks passed
@ymarcus93 ymarcus93 deleted the feat/db-target-resource branch November 15, 2023 23:51
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants