jcolemorrison / vault-on-aws

A secure HashiCorp Vault for secrets, tokens, keys, passwords, and more. Automated deployment with Terraform on AWS. Configurable options for security and scalability. Usable with any applications and services hosted anywhere.
MIT License
339 stars 60 forks source link

Removed unsupported apply_server_side_encryption #5

Closed ghost closed 2 years ago

ghost commented 2 years ago

Not sure if you're open to pull requests on this but I picked up a bit of a bug in the code.

https://registry.terraform.io/providers/hashicorp/aws/latest/docs/resources/s3_bucket#enable-default-server-side-encryption

https://github.com/hashicorp/terraform-provider-aws/issues/23106

This is read-only and so the terraform apply fails as it's unable to use this feature.

Removing this code so that any other noobs like myself going through the project/YouTube series don't get tripped up on this.

jcolemorrison commented 2 years ago

Heyy THANK YOU so much for the catching this!! And you're right, this property has been changed around so that, in order to add it we have to use a separate resource, the aws_s3_bucket_server_side_encryption_configuration resource.

Although this PR fixes the initial failure, it does leave the S3 bucket without that server side encryption. Would you be interested in adding the new resource to this PR @conormccullough1 ? It'd be the equivalent of adding the following to the s3.tf file:

# S3

## S3 Bucket for Vault Data
resource "aws_s3_bucket" "vault_data" {
  bucket_prefix = "${var.main_project_tag}-"

  tags = merge({ "Project" = var.main_project_tag })
}

# THIS would be the NEW resource
resource "aws_s3_bucket_server_side_encryption_configuration" "vault_data_sse" {
  bucket = aws_s3_bucket.vault_data.bucket

  rule {
    apply_server_side_encryption_by_default {
      sse_algorithm     = "AES256"
    }
  }
}

## S3 Bucket Public Access Block
resource "aws_s3_bucket_public_access_block" "vault_data" {
  bucket = aws_s3_bucket.vault_data.id
  block_public_acls = true
  block_public_policy = true
  ignore_public_acls = true
  restrict_public_buckets = true
}

I just need someone to give it a run through and test. It'd be nice to keep this all in your PR, but if not, I appreciate you bringing this to my attention either way!!!

ghost commented 2 years ago

Hey, just trying to test this now, but for some reason after a terraform destroy / re-apply, although Terraform creates the S3 bucket, it's now failing to place the encrypted credentials in there. Tried both with the new code block and without, so I guess it's something else that's been created along the way and is blocking it.

jcolemorrison commented 2 years ago

Interesting (also, thanks for trying it). What's the full error message it's giving?

ghost commented 2 years ago

Hi @jcolemorrison - committed the change + left an explanation of the previous behaviour. Cheers for the feedback.

jcolemorrison commented 2 years ago

Hey awesome @conormccullough1 and thanks for the detailed explanation and code! Everything looks good so I'll go ahead and merge it in.

As far as the first hiccup of it not working, I have a feeling there's a race condition going on somewhere in which some resources are being created faster than others that depend upon them. It might be the case where a depends_on argument is needed somewhere. My bet is currently the Auto Scaling Groups creating the instances in the private subnet and attempting to install packages before the NAT gateway is set up..but that's a convo and PR for a different time. For now, thanks a ton for your contribution!