-
Notifications
You must be signed in to change notification settings - Fork 5.8k
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
TechDocs: Support AWS S3 as TechDocs external storage #3794
Conversation
🦋 Changeset detectedLatest commit: 1517009 The changes in this PR will be included in the next version bump. This PR includes changesets to release 2 packages
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looking great so far @ayshiff ! Kudos for your work on this and supporting documentation as well.
Leaving some comments. Will take another look very soon. 🎉
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Excited this is being added.
awsS3: | ||
# An API key is required to write to a storage bucket. | ||
credentials: | ||
$file: '/path/to/aws_application_credentials.json', |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Curious, can the credentials be set with just environment variables? Having a physical secrets file might be difficult to setup in some environments.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I agree with you.
At first I wanted to put the credentials as environment variables:
credentials:
aws_access_key_id: ACCESS_KEY_ID
aws_secret_access_key: SECRET_ACCESS_KEY
But to keep some consistency with the configuration for GCS I thought it might be interesting to keep the same credentials file logic.
But since it is not very relevant for aws we can perhaps replace file credentials by environment variables 👍
@OrkoHunter I would like to know your opinion on that.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Curious, can the credentials be set with just environment variables?
Yes! We can use the contents of the "**credentials.json" as the value for the environment variable (e.g. TECHDOCS_AWSS3_CREDENTIALS
). And set
awsS3:
credentials:
$env: TECHDOCS_AWSS3_CREDENTIALS
I have used this with GCS on the demo site. But I see that it will be nice to write this ^ in the app-config.yaml
comments and the Config reference docs.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Wouldn't the environment variable need to be a JSON string in this case? I'm personally not a big fan of managing them that way. It can make it more challenging to deal with some secret management processes. I'd rather be able to split out each field separately. If possible that would be preferable imo.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@andrewthauer I agree with you that JSON strings are not pretty in environment variables.
Here is my concern. With GCS, their credentials JSON looks like this
{
"type": "",
"project_id": "",
"private_key_id": "",
"private_key": "-----BEGIN PRIVATE KEY-----<Insert 1600 characters>-----END PRIVATE KEY-----\n",
"client_email": "",
"client_id": "",
"auth_uri": "",
"token_uri": "",
"auth_provider_x509_cert_url": "",
"client_x509_cert_url": ""
}
Even if we split these out into individual fields, the private_key
will still be a huge JSON string. So, I do not see much of an improvement here over the existing ways i.e.
- Copy over the credentials file as a huge
$env
variable. - (Kubernetes) Inject the
$file
using Kubernetes secrets management. https://kubernetes.io/docs/concepts/configuration/secret/#overview-of-secrets - ...other typical ways to make a secret file available on a server.
That being said, if AWS allows smaller alphanumeric token-type secrets that can be generated and set as environment variables, I too would prefer that over the huge JSON string. It's just that, GCS is holding us back with how it authenticates service accounts.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If I run Backstage as a container in ECS will I be able to skip all of this config and use the role that is assigned to the task?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@mfrinnstrom Role and Region are not used as of this moment. Only bucket name and credentials are needed.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@OrkoHunter that was my concern. If I add credentials to my task in AWS ECS that would be picked up automatically by the AWS SDK. It seems like we bypass this here and require credentials to be provided from the app.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If I add credentials to my task in AWS ECS
Is this same as setting environment variables? We are open for suggestions. When the AWS sdk is initialized here, should it check some existing environment variables?
May I also ask what are the downsides of explicitly setting those environment variables in app-config.yaml
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@OrkoHunter not really, although the AWS SDK has some standard environment variables that are picked up automatically if available. Some more information can be found here.
We usually create roles and assign them to our different workloads, in this case an ECS task. This will then be picked up by the AWS SDK in the ECS task without having to create a user and generate credentials for that, and then having to securely manage and supply those credentials to the workload.
This works great when running in AWS but when running outside of AWS you will need something like what is implemented right now with the access and secret keys as environment variables. It should probably be possible to use the default environment variables (AWS_ACCESS_KEY_ID
and AWS_SECRET_ACCESS_KEY
) and have the SDK pick that up automatically, but that becomes a bit hidden and maybe not to easy to follow in the config.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have tested the setup with S3 and it works great! Thanks again @ayshiff for working on this.
I will fix the tests and fix the techdocs JSON schema by adding the |
Awesome! |
I don't know if you have reviewed the example diagram for aws. Link I used excalidraw to draw it but saw that the other graphs were made with draw.io. |
@ayshiff Good that you noticed it! If you make it with draw.io and save as an svg, it will be easier to maintain/update! Try the draw.io VS Code extension and open a file with |
Or |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Question - Will publishers support using nested directories? For instance, you want to share an s3 bucket for other purposes and the documentation sites are located in say /docs/sites
as the root.
@@ -42,6 +42,7 @@ | |||
"@google-cloud/storage": "^5.6.0", | |||
"@types/dockerode": "^3.2.1", | |||
"@types/express": "^4.17.6", | |||
"aws-sdk": "^2.813.0", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It looks like v3 was just released. Is it worth taking a look? Seems like it has better TS support - https://aws.amazon.com/about-aws/whats-new/2020/12/aws-sdk-javascript-version-3-generally-available/.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice! @ayshiff Would you this this a try? You can create an issue for the upgrade if you want to do it later or want someone else to do it - and that is totally fine.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes of course ! I'll look into it 👍
Great question @andrewthauer and big +1 for adding that support. I'll create an issue for this. I imagine it can be defined as a |
const techdocsMetadataJson = file?.Body?.toString(); | ||
|
||
if (!techdocsMetadataJson) { | ||
throw new Error( | ||
`Unable to parse the techdocs metadata file ${entityRootDir}/techdocs_metadata.json.`, | ||
); | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we have another way to get the Readable | ReadableStream | Blob
in a better way ?
@OrkoHunter Do you know why the |
a24f3d2
to
21d654d
Compare
a5a39d4
to
fd0a9d8
Compare
@ayshiff The failing test is gone (we had to rebase and fix merge conflict, that's all). Now there is a new I also had to run |
Thanks @OrkoHunter 👍 I'll look into it. |
I wonder why I didn't have conflicts while doing my rebase 🤔 |
It looks like an issue |
That's unfortunate. Maybe not worth the version bump until that is sorted out. |
I'm going to move the v3 bump commits to another branch and I'm going to create a Draft PR while waiting for the issue to be fixed in #1812. Does it sound good to you ? |
fd0a9d8
to
9b78b1a
Compare
also copy yarn.lock from master; and yarn again fix(techdocs-common): broken async test
acdcf94
to
3aa98a6
Compare
let secretAccessKey = null; | ||
let bucketName = ''; | ||
try { | ||
accessKeyId = config.getString( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Possible improvement for a future iteration (happy to contribute if we reach there first): this should use either EC2 instance profiles/Kubernetes IRSA to remove the need to provide an access key. These credentials are automatically rotated and support is built into the AWS SDK.
Typically in AWS SDKs if you credentials are omitted it uses the default precedence rules, which (from memory) is env vars, ~/.aws/credentials, instance profile/IRSA
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe have the credentials
as optional and if not present use the default credentials chain that @lowjoel mentions? Just new aws.S3()
basically.
We would also be happy to contribute this since we have a policy to never create users and use access keys. I can understand that this might be necessary if you are not running in AWS but still want to use S3 though.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'll be happy to add this to the existing PR !
@OrkoHunter Do we keep this PR in current state and iterate over it once merged to add optional credentials ? Or do we add that on this branch ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@ayshiff IMO let us iterate in the future! We shouldn't delay this PR more. Hopefully we can merge this next week. :)
@backstage/maintainers Please review when possible! |
@@ -76,7 +76,7 @@ techdocs: | |||
generators: | |||
techdocs: 'docker' # Alternatives - 'local' | |||
publisher: | |||
type: 'local' # Alternatives - 'googleGcs'. Read documentation for using alternatives. | |||
type: 'local' # Alternatives - 'googleGcs' or 'awsS3'. Read documentation for using alternatives. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not for now - But wondering if we can drop the provider prefix for these types? s3
and gcs
reads much better than awsS3
and googleGcs
respectively - thoughts @backstage/techdocs-core?
Thinking that maybe we would favour some form of configuration that looks like the below, where all the services below a provider live underneath a provider?
aws:
s3:
access_key_id: $S3_AWS_ACCESS_KEY_ID
secret_access_key: $S3_AWS_SECRET_ACCESS_KEY
redshift:
access_key_id: $REDSHIFT_AWS_ACCESS_KEY_ID
secret_access_key: $REDSHIFT_AWS_SECRET_ACCESS_KEY
default:
access_key_id: $AWS_ACCESS_KEY_ID
secret_access_key: $AWS_SECRET_ACCESS_KEY
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmm, interesting idea of unification of configs based on provider as you write above. I think we might do this just like how we did the integrations
with GitHub/GitLab/etc. ➕
As of now for techdocs.publisher.type
, IMHO googleGcs
and awsS3
are slightly better than gcs
and s3
. :)
# Required when techdocs.publisher.type is set to 'awsS3'. Skip otherwise. | ||
|
||
awsS3: | ||
# An API key is required to write to a storage bucket. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Does the sdk
also read from the instance profile or ~/.aws/credentials
too?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yep! There are some additional ways to handle S3 configuration apart from creating an access user agent (discussed above in various threads). Thinking we can address it in a separate PR (to also improve the GCS auth on similar patterns).
@ayshiff Do you think this is good to go? I'll do a final Quality Assurance on this and merge. Could you also highlight some future fixes/improvements? We can create issues for them. |
I think this is good to go, I’ll do a last check in the afternoon. Here are the future fixes/improvements that I noted in the comments:
|
@ayshiff Do let me know when it's ready according to you. |
It looks good for me ! I’ll be happy to create them. :) |
No, the commit history looks good to me!
Awesome and thank you! This is what I think -
|
You did awesome on this PR @ayshiff! 👏 I appreciate your attention to detail with the changes you did, and the reviews you addressed (sparked many good conversations). :) |
I think the problem with failed tests on master is due to bad path delimiters. |
@ayshiff New one would be good! (Windows paths 😬) |
You could use something like this in the tests. (Looking at existing examples)
|
Hey, I just made a Pull Request!
Closes #3714
This PR adds the feature to use AWS S3 as TechDocs external storage.
awsS3.ts
publisher Link✔️ Checklist