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.

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

docs: permissions & reference roles #443

Conversation

0x2b3bfa0
Copy link
Member

@0x2b3bfa0 0x2b3bfa0 commented Mar 14, 2022

Closes #75; may need extra eyes and opinions on documentation and some awkward kubectl scripting.

Potential follow–ups

  • Validate all this code as part of our CI/CD smoke test workflow1
  • Migrate Kubernetes YAML to HCL for consistency2
  • Provide tighter ABAC apart from basic RBAC permissions3

Exporting credentials (one–liner)

eval "$(terraform output --json | jq --raw-output 'to_entries[]|"export \(.key|ascii_upcase)=\(.value.value|@sh)"')"

Note: added eval to support multi–line variables like gcp service account credentials.

Originally posted by @0x2b3bfa0 in #75 (comment)

Footnotes

  1. Requires some internal infrastructure work to create the appropriate sandboxes.

  2. Doesn't seem possible without a https://github.com/hashicorp/terraform-provider-kubernetes/issues/1641

  3. https://twitter.com/michaelgat/status/1017166804139954176

@0x2b3bfa0 0x2b3bfa0 added security Sensitive flaws cloud-common Applies to every cloud vendor ui/ux User interface/experience labels Mar 14, 2022
@0x2b3bfa0 0x2b3bfa0 requested a review from a team March 14, 2022 16:13
@0x2b3bfa0 0x2b3bfa0 self-assigned this Mar 14, 2022
@0x2b3bfa0 0x2b3bfa0 linked an issue Mar 14, 2022 that may be closed by this pull request
4 tasks
@casperdcl
Copy link
Contributor

I presume the permissions root folder is WIP? Would it be placed in docs/, tests/, .github/workflows or somewhere else?

@0x2b3bfa0
Copy link
Member Author

0x2b3bfa0 commented Mar 14, 2022

Either tests/ or examples/ could be a good shoehorn–in–hand fit. Using docs/permissions/ might be also appropriate.

@casperdcl
Copy link
Contributor

casperdcl commented Mar 14, 2022

Yes in any case could you also add a page docs/guides/permissions.md? (Actually I'd suggest also deleting the current permissions/README.md to avoid duplication/maintenance sync troubles).

Could then cross-ref the main.tf example files.

@0x2b3bfa0
Copy link
Member Author

Then,

  • Rename permissions/README.md to docs/guides/permissions.md
  • Rename permissions to docs/permissions

@0x2b3bfa0
Copy link
Member Author

d2b9e1a

Copy link
Contributor

@casperdcl casperdcl left a comment

Choose a reason for hiding this comment

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

actually can we merge permissions.md with authentication.md? They shouldn't be separate pages IMO. The latter name is probably better to keep.

docs/guides/permissions.md Outdated Show resolved Hide resolved
docs/guides/permissions.md Outdated Show resolved Hide resolved
@casperdcl casperdcl changed the title Provide reference roles and permissions for every cloud ...provider docs: permissions & reference roles Mar 15, 2022
Copy link
Contributor

@casperdcl casperdcl left a comment

Choose a reason for hiding this comment

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

getting there!

docs/guides/authentication.md Outdated Show resolved Hide resolved
docs/guides/authentication.md Outdated Show resolved Hide resolved
docs/guides/authentication.md Outdated Show resolved Hide resolved
docs/permissions/aws/main.tf Outdated Show resolved Hide resolved
Copy link
Contributor

@casperdcl casperdcl left a comment

Choose a reason for hiding this comment

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

minor

docs/guides/authentication.md Outdated Show resolved Hide resolved
docs/guides/authentication.md Outdated Show resolved Hide resolved
docs/guides/authentication.md Outdated Show resolved Hide resolved
docs/guides/authentication.md Outdated Show resolved Hide resolved
Co-authored-by: Casper da Costa-Luis <casper.dcl@physics.org>
Copy link
Contributor

@casperdcl casperdcl left a comment

Choose a reason for hiding this comment

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

one slight concern I have is the syntax won't work on all shells (e.g. windows) but is probably understandable to the sort of users it targets.

@0x2b3bfa0 0x2b3bfa0 force-pushed the 75-provide-reference-roles-and-permissions-for-every-cloud-vendor branch from 56390d8 to abd6742 Compare March 15, 2022 19:58
@0x2b3bfa0 0x2b3bfa0 requested a review from casperdcl March 15, 2022 20:00
Copy link
Contributor

@casperdcl casperdcl left a comment

Choose a reason for hiding this comment

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

lgtm.

(minor) could we remove as many of the empty newlines in the *.tf/*.sh files as possible?

@0x2b3bfa0
Copy link
Member Author

  • When both arguments and blocks appear together inside a block body, place all of the arguments together at the top and then place nested blocks below them. Use one blank line to separate the arguments from the blocks [...]
  • Use empty lines to separate logical groups of arguments within a block [...]
  • Top-level blocks should always be separated from one another by one blank line. Nested blocks should also be separated by blank lines [...]

(Infimum) Not at the cost of breaking the Style Conventions?

@casperdcl
Copy link
Contributor

Not at the cost of breaking the Style Conventions

I think those conventions were written without deep thought.

If terraform fmt is happy we should be too, no?

@0x2b3bfa0
Copy link
Member Author

I think those conventions were written without deep thought.

Deep Thought, I presume?

If terraform fmt is happy we should be too, no?

Inversely, I believe that terraform fmt was written without deep thought™

@0x2b3bfa0
Copy link
Member Author

0x2b3bfa0 commented Mar 15, 2022

adding and removing newlines is relatively easier to do [compared to horizontal whitespace] and often done for subjective reasons that an automatic tool cannot reason about

I was right, terraform fmt doesn't alter vertical whitespace because it was written without Deep Thought

Also removes subshell, only required when sourcing or copying/pasting the code into a shell
@0x2b3bfa0

This comment was marked as off-topic.

@0x2b3bfa0
Copy link
Member Author

0x2b3bfa0 commented Mar 15, 2022

I'd prefer not to remove these newlines myself, on principle. 😅 Still, a commit with that change would be welcome.

@0x2b3bfa0
Copy link
Member Author

0x2b3bfa0 commented Mar 16, 2022

Terraform configuration is usually formatted like in this example, following the style guide. Neither abd6742 nor omitting newlines are common practices.

Anyway, this boils down to a matter of taste, and adherence (or not) to official guidelines; I don't want to block a pull request because of an existential quandary. ⚔️ 😅

🔔 @casperdcl, please make the changes you deem opportune and merge this pull request.

@casperdcl casperdcl force-pushed the 75-provide-reference-roles-and-permissions-for-every-cloud-vendor branch from 802da6a to 76bb6a9 Compare March 17, 2022 01:03
@casperdcl
Copy link
Contributor

casperdcl commented Mar 17, 2022

Hope b74a784 is fine (specifically set -euxo pipefail).

About style, there are two options: 1) aim for comprehension-over-rules, or 2) use a comprehensive linter1. Since as you say terraform fmt isn't quite (2) then I'd revert to (1) i.e. aid first-time readers 76bb6a9.

Footnotes

  1. e.g. I detest black & yapf but tolerate them since at least they're comprehensive

@0x2b3bfa0
Copy link
Member Author

Yes, there is no canonical representation for HCL apart from @casperdcl–taste-format and @0x2b3bfa0-style-guide-format. In absence of machines there is no clear way of make a neutral choice and differences are minimal, so my pragmatic vote is for the former. 🏳️

set -euxo pipefail in b74a784 is great to have, although I don't know if users will be misled somehow by set -x to stderr 🤔


Post scriptum: I adore black... --line-length=79.

@0x2b3bfa0
Copy link
Member Author

:shipit: Ready to merge unless you want to sed '/^$/d' any other file. 😄

@casperdcl casperdcl closed this Mar 17, 2022
@casperdcl casperdcl reopened this Mar 17, 2022
@casperdcl casperdcl merged commit fff90c1 into master Mar 17, 2022
@casperdcl casperdcl deleted the 75-provide-reference-roles-and-permissions-for-every-cloud-vendor branch March 17, 2022 14:48
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cloud-common Applies to every cloud vendor security Sensitive flaws ui/ux User interface/experience
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Provide reference roles and permissions for every cloud vendor
3 participants