Skip to content

Fix navigation for the Sentinel config and clarify module usage#9588

Merged
sgmiller merged 10 commits into
masterfrom
sentinel-conf-doc-updates
Jul 27, 2020
Merged

Fix navigation for the Sentinel config and clarify module usage#9588
sgmiller merged 10 commits into
masterfrom
sentinel-conf-doc-updates

Conversation

@sgmiller
Copy link
Copy Markdown
Collaborator

This hopefully get's the Sentinel stanza docs into the sidebar, and tries to
address any confusion around which modules Vault generally supports and the
justification for additonal_enabled_modules.

be included could be overridden here.


~> **Warning**: Care should be taken when enabling modules which
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

For some reason when I tested this out the warning didn't format with the fancy box.

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

Hmm. How do we test this actually (ie render docs locally)

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

See the website/README.md, but you can just make in that dir and then see it on localhost:3000.

I made various improvements to this doc.
Note that we really should have called this property `additional_enabled_imports`.  I tried to describe the use of the property with what I view as correct terminology from a Sentinel point of view while also referencing "modules" since that is what is in the property name.  I'm going to submit an issue suggesting we change the property. For that reason, I put "modules" in parentheses rather than putting "imports" in parentheses.
Copy link
Copy Markdown
Contributor

@rberlind rberlind left a comment

Choose a reason for hiding this comment

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

It's unfortunate that the property is called "additional_enabled_modules" when it should have been called "additional_enabled_imports". Modules in Sentinel are technically something different since they are written in Sentinel itself instead of Golang as is the case for imports (or "import plugins"). https://docs.hashicorp.com/sentinel/extending/internals/#sentinel-s-import-model shows that modules and import plugins can be considered as two types of "imports"; of course, both are integrated into Sentinel policies with the "import" statement. I should have noticed this bigger issue in addition to the doc issue. But let's put that aside for now and focus on the doc. I will submit a separate issue suggesting we rename the setting.

I have made an edit in this branch suggesting how this should be written. Apologies if you object to my having done it on your branch. Feel free to modify or revert my changes. But I do think they are better.

Note that in addition to discussing the http import, I added the important note about customers being able to create their own custom import plugins in Golang.

@rberlind
Copy link
Copy Markdown
Contributor

I'm also going to do a PR to change https://docs.hashicorp.com/sentinel/extending/plugins/ to say that custom imports can be used with Nomad and Vault.

@sgmiller
Copy link
Copy Markdown
Collaborator Author

It's unfortunate that the property is called "additional_enabled_modules" when it should have been called "additional_enabled_imports". Modules in Sentinel are technically something different since they are written in Sentinel itself instead of Golang as is the case for imports (or "import plugins"). https://docs.hashicorp.com/sentinel/extending/internals/#sentinel-s-import-model shows that modules and import plugins can be considered as two types of "imports"; of course, both are integrated into Sentinel policies with the "import" statement. I should have noticed this bigger issue in addition to the doc issue. But let's put that aside for now and focus on the doc. I will submit a separate issue suggesting we rename the setting.

I have made an edit in this branch suggesting how this should be written. Apologies if you object to my having done it on your branch. Feel free to modify or revert my changes. But I do think they are better.

Note that in addition to discussing the http import, I added the important note about customers being able to create their own custom import plugins in Golang.

Does Vault support custom sentinel modules though? I'm not sure it does...

@rberlind
Copy link
Copy Markdown
Contributor

rberlind commented Jul 24, 2020

@sgmiller : regarding your comment "Does Vault support custom sentinel modules though? I'm not sure it does...".
That had been my understanding, but I now wonder if it is wrong. Can you check with engineering and @narayan-iyengar ?
Of course, he said to check with you. :). I now do suspect custom imports are NOT supported. Once confirmed, I can rewrite.

…on a blocklist, it does not actually load any modules. Vault does not support custom Sentinel plugins.
@sgmiller
Copy link
Copy Markdown
Collaborator Author

This feature for certain does not load any module. It merely removes items from a blocklist in Vault core setup. So this does not provide a route for adding custom plugins.

@sgmiller sgmiller merged commit 5b1b2ff into master Jul 27, 2020
@sgmiller sgmiller deleted the sentinel-conf-doc-updates branch July 27, 2020 14:52
pull Bot pushed a commit to sigtrap/vault that referenced this pull request Sep 24, 2025
…ashicorp#9588)

* use "redirect" instead of "afterModel"

* fix styling of radio group buttons

* remove redundant route redirect

* wrap mount dropdown in loading conditional

* reuse parent redirect logic, delete unused outlet

* minor padding adjustments

* force restart tests

Co-authored-by: claire bontempo <68122737+hellobontempo@users.noreply.github.com>
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.

3 participants