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.

Sign up for GitHub

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

Jump to bottom

Adding files and S3 bucket for Adding Log data to QS #127

Open
wants to merge 8 commits into
base: mainline
Choose a base branch
from
Open

Adding files and S3 bucket for Adding Log data to QS #127

wants to merge 8 commits into from

Conversation

DillonRanwala
Copy link
Contributor

No description provided.

@DillonRanwala
feat(cdk): add S3 bucket for logs to Pipeline.
ada8daa 
Problem:
Makerspace's Logs are currently stored in Google Sheets. This data has
useful information that can be used in Quicksight, but there is not a
simple way to connect Quicksight to Google Sheets.

Solution:
By creating this S3 bucket, we can store the Logs as csv files in our
AWS ecosystem. Quicksight can create a dataset from S3, so this Log data
can be visualized.

Testing:
Dev environment successfully deployed bucket with no external issues.

Issue:
N/A, No open issue.
@DillonRanwala
feat(cdk): adds apps script and json files needed for Log usage.
0924817 
Problem:
Need to store the relevant apps script code and json manifest file used
in the process of connecting Quicksight to S3.

Solution:
These files can be utilized by team members as needed. The apps script
is used in a Google Apps Script project and the manifest file can be
used when connecting Quicksight to an S3 data source.

Testing:
These additions do not directly affect the AWS system. The manifest file
worked properly for connecting a Quicksight bucket.

Issue:
N/A, No open issue
@DillonRanwala
docs(cdk): add readme for Log folder.
ffc52d0 
Problem:
Need documentation for this new folder and its items.

Solution:
Adds README.md file with basic instructions.

Testing:
Documentation only.

Issue:
N/A, No open issue
ophilli
ophilli reviewed Mar 10, 2023
View reviewed changes
cdk/log_storage/apps_script_code/log_upload_to_s3.gs Outdated

// replace with your own IAM User's access key and secret access key
// This IAM role should preferably only have access to your one target S3 bucket
var s3 = S3.getInstance("<AWS_ACCESS_KEY>", "<AWS_SECRET_KEY>");
Copy link
Member

Choose a reason for hiding this comment

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

Can you model this IAM user / role in CDK?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Just added IAM user as "cdk-log-s3-user"

ophilli
ophilli reviewed Mar 10, 2023
View reviewed changes
cdk/log_storage/__init__.py


def s3_log_bucket(self):
self.log_bucket = aws_s3.Bucket(self, 'quicksight-log-data',
Copy link
Member

Choose a reason for hiding this comment

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

Can you also enforce SSL? https://docs.aws.amazon.com/cdk/api/v1/docs/aws-s3-readme.html#enforcing-ssl

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Used enforce_ssl option in S3 bucket creation to address this.

ophilli
ophilli reviewed Mar 10, 2023
View reviewed changes
cdk/log_storage/__init__.py Outdated
self.log_bucket = aws_s3.Bucket(self, 'quicksight-log-data',
block_public_access=aws_s3.BlockPublicAccess.BLOCK_ALL,
encryption=aws_s3.BucketEncryption.S3_MANAGED,
versioned=True,
Copy link
Member

Choose a reason for hiding this comment

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

What do you think about leaving versioning turned off? The Google Sheet they're using will already track edit history & I think this could cause the size of stored data to balloon pretty quickly if someone started running the google apps script on a high frequency (every 5 minutes for example).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I agree that versioning is most likely not needed. Google sheets will keep backups of the log data if needed and I think we should avoid any potential problems with unnecessary data storage.

Turned versioned=False, in new commit.

@DillonRanwala
feat(cdk): enforces ssl and removes versioning from log S3.
6b6e401 
Problem:
The log S3 bucket needs to enable ssl verification for any requests for
best security practices. Versioning could create a lot of unnecessary
data which we don't need to backup (google sheets has this backup if
needed).

Solution:
By modifying the cdk creation of this s3 bucket we can enforce all
requests to use HTTPS and we can also turn off versioning.

Testing:
A dev S3 bucket that was created had a bucket policy that enforced ssl
and had versioning turned off. HTTPS requests were able to insert into
the bucket with proper credentials.

Issue:
N/A, no open issue.
@DillonRanwala
feat(cdk): add IAM user with log S3 access to Cloudformation.
352b977 
Problem:
Need to provide an IAM user with access to the correct bucket through
cdk. Without this, could raise problems with trying to create new IAM
users through AWS console only.

Solution:
By modeling an IAM user and access policy, this user will be created
automatically. This IAM user will have proper access to the right S3
bucket and its credentials can be created/viewed as needed.

Testing:
Dev environment created this IAM user which can access the log S3
bucket.

Issue:
N/A, no open issue.
@DillonRanwala
feat(cdk): add modified apps script library for HTTPS requests.
d1e7b79 
Problem:
The previous log_upload_to_s3.gs script used a third-party apps script
library to connect to S3. This library used HTTP requests which raise
security concerns and will not connect to our new S3 bucket (with SSL
enforced). We needed to modify this source library to allow for HTTPS
requests instead of HTTP.

Solution:
By cloning the S3.gs and S3Request.gs scripts and adjusting a few urls
to use https instead of http, we can send verified https requests to S3.
Also, the apps script project no longer needs to import the library, it
can just use the two scripts.

Testing:
This code was able to send an https request to a dev S3 bucket and
insert a csv properly.

Issue:
N/A, no open issue.
@DillonRanwala
feat(cdk): add log storage Cloudformation objects to Makerspace stack.
655b110 
Problem:
Need to update our current Cloudformation stack with all our object to
include the new log_storage items.

Solution:
Adding the dependency in makerspace.py will allow our new changes to be
modeled in Cloudformation.

Testing:
Dev cloudformation added new objects without issue.

Issue:
N/A, no open issue.
@DillonRanwala
docs(cdk): update log storage README for new files.
cdd6597 
Problem:
Need to add some comments about new .gs files.

Solution:
Updated README.md

Testing:
Only changed docs

Issue:
N/A, no open issue.
@DillonRanwala
Copy link
Contributor Author

I added two new .gs files that are cloned from the apps script library I was using to make S3 put requests. I changed them to use HTTPS and added them into our repo, so anyone using this code won't need to import the original library in their Google Apps Script project.

@Mjtlittle Mjtlittle added the pr: stale PR is either old or needs further attention before merging label Dec 13, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ophilli ophilli ophilli left review comments

At least 1 approving review is required to merge this pull request.

Assignees
No one assigned
Labels
pr: stale PR is either old or needs further attention before merging
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@DillonRanwala @ophilli @Mjtlittle