WIP: feat: add the first component #1

gberenice · 2023-04-24T14:54:09Z

Info

This adds our first component tailscale so we could discuss and agree on the structure.

Questions that come up:

At this moment we don't have any intermediate tool or interface that could help us to work with mixins the way we used to. We need to decide what is the role of mixins in our components? If we use them - what is the workflow/structure/other rules that we should declare?
Should we add trunk?
Basically these components should be used as TF modules unless we add some custom layer. If so - what is our definition of component? When it should be created and when it's enough to use a root module (+ probably some locals, variables or data)?
Should we consider that these components will be used for current customers?
Who is the end user of these components?

kevcube · 2023-04-24T22:54:48Z

components/tailscale/data.tf

+  vpc_id             = data.aws_vpc.default.id
+  vpc_cidr_block     = data.aws_vpc.default.cidr_block


IMO these locals don't add any value

kevcube · 2023-04-24T22:56:04Z

components/tailscale/data.tf

+  private_subnet_ids = toset(data.aws_subnets.private.ids)
+}
+
+data "aws_vpc" "default" {


I don't think we should name this object "default" because we aren't looking for the default VPC

kevcube · 2023-04-24T22:58:10Z

components/tailscale/data.tf

+locals {
+  vpc_id             = data.aws_vpc.default.id
+  vpc_cidr_block     = data.aws_vpc.default.cidr_block
+  private_subnet_ids = toset(data.aws_subnets.private.ids)


data.aws_subnets.private.ids will already be a list(string) in which we don't expect to find duplicates, right? I don't think we need explicit conversion to set

kevcube · 2023-04-24T23:00:01Z

components/tailscale/main.tf

@@ -0,0 +1,25 @@
+module "tailscale_subnet_router" {
+  source  = "git::https://github.com/masterpointio/terraform-aws-tailscale.git?ref=tags/0.1.0"


side note: @Gowiem we should publish to Terraform Registry so we can reference this as "masterpointio/tailscale/aws"

But also we should be more specific about how that repo is named, instead of "tailscale" call it "tailscale-subnet-router"

Or if it is going to be a collection of tailscale resources, then make ./modules/subnet-router

kevcube · 2023-04-24T23:01:21Z

components/tailscale/main.tf

+  session_logging_enabled = var.tailscale_session_logging_enabled
+}
+
+module "ssh_key_pair" {


Suggested change

module "ssh_key_pair" {

module "ec2_key_pair" {

to differentiate that we aren't just creating a public/private key with something like tls_private_key

kevcube · 2023-04-24T23:05:07Z

components/tailscale/providers.tf

+locals {
+  aws_account_id = module.account.outputs.full_account_map[var.stage]
+  assume_role    = format("arn:aws:iam::${local.aws_account_id}:role/${var.namespace}-${var.stage}-%s", var.run_in_cicd ? "terraform" : "admin")
+}
+
+variable "run_in_cicd" {
+  type        = bool
+  description = "If run in CI/CD platform, use terraform delegated role, otherwise use admin."
+  default     = true
+}
+
+variable "assume_role_override" {
+  type        = string
+  description = "Overrides the assumed role for the AWS provider. Useful for local testing and imports."
+  default     = null
+}


All of this logic is pretty heavily dependent on someone already following the Cloudposse/Atmos account management blueprint, which marries us to their architecture. It makes sense to be prescriptive about their architecture to our customers because we are already deep in it, but public consumers of this "component" will likely not be using that same architecture.

cc @Gowiem

Mainly this is a trace of Masterpoint using Spacelift. The variable run_in_cicd was renamed from run_in_spacelift, for example.
The assumption here is that we still would want to separate local roles and roles used in CI/CD platforms. However, with tf-controller this all is very questionable, and the final working version could look very different from the pattern we're following now.

kevcube · 2023-04-24T23:05:16Z

components/tailscale/providers.tf

+module "account" {
+  source  = "cloudposse/stack-config/yaml//modules/remote-state"
+  version = "0.22.3"
+
+  component = "account-map"
+  stage     = "root"
+
+  context = module.this.context
+}


same as above

kevcube · 2023-04-24T23:08:05Z

components/tailscale/providers.tf

+provider "tailscale" {
+  tailnet             = var.tailnet
+  oauth_client_id     = local.secrets.tailscale_oauth_client_id
+  oauth_client_secret = local.secrets.tailscale_oauth_client_secret
+}


same here, I feel the way we are providing creds to the provider to be a little prescriptive, maybe I'm missing the point here and we really WANT to be prescriptive about this sort of stuff, but if I was managing infra I probably would have my stuff set up in a way that these are fed in via environment variables.

here we're placing a boundary to adoption on someone already storing these things in SSM

Yeah, it's an open question.
I like the idea of having some kind of secrets mixin (or probably module), that provides us an elegant way to work primarily with SOPS (and support any other manager we need). If we unfollow Atmos' way - we need to come up with something new. I don't know if we ever would want to pass a secret value as a Terraform variable, since usually it's configured as fetching secret value from some storage, but I'm open to any good example to reconsider.

kevcube · 2023-04-24T23:08:52Z

components/tailscale/variables.tf

+variable "ssh_public_key_path" {
+  type        = string
+  description = "Path to SSH public key directory (e.g. `/secrets`)"
+}


Suggested change

}

}

kevcube · 2023-04-24T23:11:11Z

components/tailscale/secrets.mixin.tf

same as above, this is prescriptive and dependent on implementations that we use but others might not - if our master plan is to integrate tightly into the cloudposse component ecosystem then this makes sense

if our plan here is to make modules for internal use then no problem

but if our plan is to make an attractive library of useful marketing materials then I feel like we're placing a big operational burden on potential users here to conform to our implementation details - one of my main gripes with cloudposse's components. These things make sense for our customers - partly because it's how things are already done there - but might not make sense for everyone

Yeah, agree here. We should have a clear understanding of who are our end users, and who do we expect to participate in evolution of this ecosystem.

Gowiem · 2024-02-09T18:16:16Z

@gberenice damn April 2023, I'm sorry this got full stopped! I'm surprised we did that to ourselves here. Let's chat through live and see if we should close or push through.

feat: add the first component

b7245ff

gberenice requested a review from Gowiem April 24, 2023 14:54

kevcube self-requested a review April 24, 2023 22:53

kevcube reviewed Apr 24, 2023

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

WIP: feat: add the first component #1

WIP: feat: add the first component #1

gberenice commented Apr 24, 2023 •

edited

Loading

kevcube Apr 24, 2023

kevcube Apr 24, 2023

kevcube Apr 24, 2023

kevcube Apr 24, 2023

kevcube Apr 24, 2023

kevcube Apr 24, 2023

gberenice Apr 25, 2023

kevcube Apr 24, 2023

kevcube Apr 24, 2023

gberenice Apr 25, 2023

kevcube Apr 24, 2023

kevcube Apr 24, 2023

gberenice Apr 25, 2023

Gowiem commented Feb 9, 2024

		vpc_id = data.aws_vpc.default.id
		vpc_cidr_block = data.aws_vpc.default.cidr_block

		@@ -0,0 +1,25 @@
		module "tailscale_subnet_router" {
		source = "git::https://github.com/masterpointio/terraform-aws-tailscale.git?ref=tags/0.1.0"

WIP: feat: add the first component #1

Are you sure you want to change the base?

WIP: feat: add the first component #1

Conversation

gberenice commented Apr 24, 2023 • edited Loading

Info

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Gowiem commented Feb 9, 2024

gberenice commented Apr 24, 2023 •

edited

Loading