Skip to content

Conversation

@infraredgirl
Copy link
Contributor

@infraredgirl infraredgirl commented Jan 6, 2021

https://gruntwork.atlassian.net/browse/IAC-1641

Remaining work:

@netlify
Copy link

netlify bot commented Jan 6, 2021

Deploy preview for keen-clarke-470db9 ready!

Built with commit 8ada2d5

https://deploy-preview-386--keen-clarke-470db9.netlify.app

@infraredgirl
Copy link
Contributor Author

Having the version number in the title makes the guide not appear in the list of guides - I guess this is a bug.

Copy link
Contributor

@bwhaley bwhaley left a comment

Choose a reason for hiding this comment

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

Great start! Left a few comments for your consideration on grammar/use of wording. Looking forward to doing a complete review!

@brikis98
Copy link
Member

brikis98 commented Jan 7, 2021

Having the version number in the title makes the guide not appear in the list of guides - I guess this is a bug.

Yes, this is a known issue. I think something in the code struggles with non alphanumeric characters (e.g., decimal points)? @eak12913 @oredavids Any chance of getting this fixed, as it's hard to publish a guide for CIS v1.3.0 without using decimal points 😁

Copy link
Member

@brikis98 brikis98 left a comment

Choose a reason for hiding this comment

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

Thanks for putting this together!

Having seen this, I think the right thing to do is to update the Acme CIS repos too, so customers have a concrete upgrade example to look at. We need to add that to the Jira epic.

@ina-stoyanova
Copy link
Contributor

Thanks for making a start on this @infraredgirl 🙂 I"m having a look to add bits about IAM access analyzer now

@eak12913
Copy link
Contributor

eak12913 commented Jan 7, 2021

Having the version number in the title makes the guide not appear in the list of guides - I guess this is a bug.

Yes, this is a known issue. I think something in the code struggles with non alphanumeric characters (e.g., decimal points)? @eak12913 @oredavids Any chance of getting this fixed, as it's hard to publish a guide for CIS v1.3.0 without using decimal points 😁

Would this be a blocker for you folks? If at all possible - I'd prefer to not switch context right now but I am worried that this will hold y'all up.

The last time I looked at the code it was somewhat of a tangled mess of wires with at some amount of assumptions baked in about what Jekyll will do too (in addition to whatever our mess of logic is doing). In order to dive into that - I'd have to page out what I'm working on now.

Ina Stoyanova added 2 commits January 7, 2021 16:26
This will need to be updated once we've updated the ACME repos for the Ref Arch.
@brikis98
Copy link
Member

brikis98 commented Jan 8, 2021

Having the version number in the title makes the guide not appear in the list of guides - I guess this is a bug.

Yes, this is a known issue. I think something in the code struggles with non alphanumeric characters (e.g., decimal points)? @eak12913 @oredavids Any chance of getting this fixed, as it's hard to publish a guide for CIS v1.3.0 without using decimal points 😁

Would this be a blocker for you folks? If at all possible - I'd prefer to not switch context right now but I am worried that this will hold y'all up.

The last time I looked at the code it was somewhat of a tangled mess of wires with at some amount of assumptions baked in about what Jekyll will do too (in addition to whatever our mess of logic is doing). In order to dive into that - I'd have to page out what I'm working on now.

I think the guide needs to have "1.2" and "1.3" in the title. If there's some workaround way to get that without forcing you to context switch, we're happy to use that... But otherwise, this won't have the right title, or the right SEO benefits, and changing it later would change the URL, which would require redirects, which we'd prob forget, leading to 404s... So, yea, I think we need some way to have the proper title on this guide.

@ina-stoyanova
Copy link
Contributor

Having the version number in the title makes the guide not appear in the list of guides - I guess this is a bug.

Yes, this is a known issue. I think something in the code struggles with non alphanumeric characters (e.g., decimal points)? @eak12913 @oredavids Any chance of getting this fixed, as it's hard to publish a guide for CIS v1.3.0 without using decimal points 😁

Would this be a blocker for you folks? If at all possible - I'd prefer to not switch context right now but I am worried that this will hold y'all up.
The last time I looked at the code it was somewhat of a tangled mess of wires with at some amount of assumptions baked in about what Jekyll will do too (in addition to whatever our mess of logic is doing). In order to dive into that - I'd have to page out what I'm working on now.

I think the guide needs to have "1.2" and "1.3" in the title. If there's some workaround way to get that without forcing you to context switch, we're happy to use that... But otherwise, this won't have the right title, or the right SEO benefits, and changing it later would change the URL, which would require redirects, which we'd prob forget, leading to 404s... So, yea, I think we need some way to have the proper title on this guide.

I was curious to have a play around with this bug, and I have to say it's very annoying and unfortunately my lack of experience with Jekyll and playing around didn't really help!

Anyway, I thought I'd share a few things I looked into, and hopefully it could be a very simple fix when combined with more knowledgeable people in how this works!

https://jekyllrb.com/docs/liquid/filters/
https://shopify.github.io/liquid/filters/escape/

Basically, as far as I could gather, none of the ;, ,, ., : characters work, so I assume it's some sort of escaping of them that needs to happen!

@eak12913
Copy link
Contributor

eak12913 commented Jan 8, 2021

Thanks for taking a look @ina-stoyanova! - I will just finish up a few things and can try to take a look as well. If memory serves me correctly from last time, the problem is somewhere in the interplay between the following things:

  • The title of the article
  • The search behavior which is responsible for showing/hiding the main sections on that index page
  • Scroll-spy - which is a library that's linking up the nav on the left with what's displayed in the main section

There's interplay between these things and the last time I looked - I was able to figure out Jim's issue - but it still didn't solve this one.

If you'd like - we can pair code? I'm no Jekyll expert and only have cursory exposure to this code from the troubleshooting I did on it last time. Otherwise I'll take it up either later today (probably too late for you) or Monday depending on how today's other tasks go.

@ina-stoyanova
Copy link
Contributor

Thanks for taking a look @ina-stoyanova! - I will just finish up a few things and can try to take a look as well. If memory serves me correctly from last time, the problem is somewhere in the interplay between the following things:

  • The title of the article
  • The search behavior which is responsible for showing/hiding the main sections on that index page
  • Scroll-spy - which is a library that's linking up the nav on the left with what's displayed in the main section

There's interplay between these things and the last time I looked - I was able to figure out Jim's issue - but it still didn't solve this one.

If you'd like - we can pair code? I'm no Jekyll expert and only have cursory exposure to this code from the troubleshooting I did on it last time. Otherwise I'll take it up either later today (probably too late for you) or Monday depending on how today's other tasks go.

Yeah - I'd love to pair code if it's on Monday. Although, it's not my domain - it would certainly be useful to pair code with you! Either way - thanks for filling in my gaps :) I knew there must be more to it, as simple fixes obviously didn't work 😜Loads to learn as always! Feel free to pick it up whenever's convenient for you - but if Monday, I'll make time to work with you :)

@ina-stoyanova
Copy link
Contributor

ina-stoyanova commented Jan 11, 2021

Just trying to understand what's still left on this PR? Is the following correct?

[x] Fix title bug (not showing '1.3' in the guide title)
[ ] Add section for NACL
[x] Add section for IAM access analyzer
[ ] Add section for updates to existing modules:
- [ ] EBS encryption
- [ ] Add section for object-level logging functionality
- [ ] Add section for log metrics & alarms
- [ ] Add section S3 bucket (using private buckets)

N.B.: This PR could be reviewed, but not merged until this is also done: https://gruntwork.atlassian.net/browse/IAC-1683

Screenshot 2021-01-11 at 12 27 08

@infraredgirl
Copy link
Contributor Author

infraredgirl commented Jan 11, 2021

Do we need to have dedicated section for the items that were just upgrades to the existing modules? I understand the need for dedicated sections for new modules, but I'm not sure that e.g. the S3 bucket upgrades (which are just refactors to use a different modules under the hood) need a section each?

@eak12913
Copy link
Contributor

@infraredgirl Could you please try to change your title to what you originally wanted it to be and double check. @ina-stoyanova and I have fixed the issue and we believe that it should no longer be a problem.

This reverts commit 3aba818.
@infraredgirl
Copy link
Contributor Author

@infraredgirl Could you please try to change your title to what you originally wanted it to be and double check. @ina-stoyanova and I have fixed the issue and we believe that it should no longer be a problem.

Works now! Thanks so much for the fix!

@eak12913
Copy link
Contributor

I was originally dreading this one because I thought it was going to be a can of worms. It ended up being pretty straight forward and to correct my earlier assumption, had nothing to do with ScrollSpy or the search logic. The issue was with the id property. We were basing the id property off of the title of the article but were not slugifying.

@brikis98
Copy link
Member

Updated for Acme CIS repos are here:
gruntwork-io/cis-infrastructure-modules-acme#6
gruntwork-io/cis-infrastructure-live-acme#8

Should we make a mention of these in the guide?

Yes! We should make these a first-class part of the guide: "See the Acme CIS examples to see what the code changes may look like."

@infraredgirl
Copy link
Contributor Author

Updated for Acme CIS repos are here:
gruntwork-io/cis-infrastructure-modules-acme#6
gruntwork-io/cis-infrastructure-live-acme#8

Should we make a mention of these in the guide?

Yes! We should make these a first-class part of the guide: "See the Acme CIS examples to see what the code changes may look like."

Added in e80a4ee. I've linked directly to the update PRs - if there's a better location to link to please LMK!

@infraredgirl
Copy link
Contributor Author

I think we can open this up for reviews!

@infraredgirl infraredgirl changed the title [WIP] Add migration guide for CIS AWS 1.3.0 Add migration guide for CIS AWS 1.3.0 Jan 25, 2021
Copy link
Member

@brikis98 brikis98 left a comment

Choose a reason for hiding this comment

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

Getting super close!

allow_administrative_remote_access_cidrs_private_app_subnets = { app_vpc_cidrs = module.app_vpc.vpc_cidr_block }
allow_administrative_remote_access_cidrs_private_persistence_subnets = { app_vpc_cidrs = module.app_vpc.vpc_cidr_block }
}
----
Copy link
Member

Choose a reason for hiding this comment

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

If you're switching from vpc-app-network-acls in module-vpc to this new one, you'll also need to run state mv commands! As all the original resources are now nested one module deeper.

Copy link
Contributor

Choose a reason for hiding this comment

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

@brikis98
Copy link
Member

brikis98 commented Jan 26, 2021 via email

Co-authored-by: Ina Stoyanova <ina@gruntwork.io>
@ina-stoyanova
Copy link
Contributor

Hey @marinalimeira @infraredgirl I believe with the last few comments, this is ready to go?

@josh-padnick josh-padnick removed their request for review February 2, 2021 17:22
brikis98
brikis98 previously approved these changes Feb 3, 2021
Copy link
Member

@brikis98 brikis98 left a comment

Choose a reason for hiding this comment

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

This is also good to go after a few minor wording / typo tweaks!

Co-authored-by: Yevgeniy Brikman <brikis98@users.noreply.github.com>
@infraredgirl
Copy link
Contributor Author

Woohoo!

Many thanks to everyone who contributed to this PR, both in terms of content and reviews!

Merging now!

@infraredgirl infraredgirl merged commit 1491b6c into master Feb 3, 2021
@robmorgan robmorgan deleted the cis-upgrade-guide branch February 3, 2021 13:56
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

8 participants