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

Feature: Secrets integration, including Secrets Providers and Secrets Groups #868

Merged
merged 25 commits into from
Nov 15, 2021

Conversation

glennmatthews
Copy link
Contributor

Towards: #541

This is the initial implementation of a generic/abstract Secret data model, including basic UI, REST API, and generic testing. At this time it doesn't include support for any concrete secret provider (environment variables, Hashicorp Vault, AWS Secrets Manager, etc.); my plan is to do incremental PRs into this feature branch as I proceed for ease of incremental reviewing. This PR, as it currently stands, is essentially all of the generic "boilerplate" needed to add a new, functional data model to Nautobot - it's a fair bit of code but there shouldn't be anything "surprising" in here so it should be a pretty quick/easy review.

image

image

image

image

image



class SecretForm(BootstrapMixin, CustomFieldModelForm, RelationshipModelForm):
"""Create/update form for `Secret` objects."""
Copy link
Contributor

Choose a reason for hiding this comment

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

Comment for me: This seems to be the only page where we use Markdown ticks? I see that there are other forms in this file that do the same thing, but it doesn’t seem to be universal. Which way if preferred?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fair question. I don't think we have an official position on this yet. In general I'd like to see a lot of improvement to our docstrings (many of the forms in this file don't even have docstrings at all) and that's maybe something we should revisit then.

@hellerve
Copy link
Contributor

hellerve commented Sep 2, 2021

This looks reasonable to me as of now, bearing in mind that I know next to nothing of the actual spec for this.

Copy link
Contributor

@jathanism jathanism 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. My only gripe right now is that parameters are required and depending on the provider, that might not always be the case? I assume you've already considered that?

@glennmatthews
Copy link
Contributor Author

I can't think of a case offhand where we'd have a "singleton" provider that has no configurable parameters (because it only ever returns a single specific secret?). If you can think of one, I'm happy to reconsider this part of the design. :-)

@jathanism
Copy link
Contributor

I can't think of a case offhand where we'd have a "singleton" provider that has no configurable parameters (because it only ever returns a single specific secret?). If you can think of one, I'm happy to reconsider this part of the design. :-)

Nah, just covering bases. :)

…der implementations (#887)

* Add Secret.value property, add EnvironmentVariable provider, add dummy-plugin Constant provider, add tests

* Add TextFileSecretProvider

* Add docs

* Improve display of secret providers in the UI

* Refactor SecretsProvider registration to use the Nautobot registry instead of python entry_points

* Refactor slightly

* Add ability for secrets providers to define an HTML form for parameter inputs

* Fix default value for JSONField and add error handling in JS
Secrets refinement, docs, and GitRepository integration
@dgarros
Copy link
Contributor

dgarros commented Sep 30, 2021

@glennmatthews great work

With this implementation is it possible to have difference secret per device, or group of devices ?

For example, as Nelly the Network Engineer, I want to be have a different login/password for my Cisco devices and my Arista devices or between my Console devices and my network devices etc ...

@glennmatthews
Copy link
Contributor Author

With this implementation is it possible to have difference secret per device, or group of devices ?

That's the next piece of the design that we're actively thinking about. :-) Attaching a secret to an individual device is useful but definitely doesn't scale as well as being able to define secrets for groups of devices. We've talked about maybe building something atop config-contexts as an existing model in this area, but it's not definite yet.

@dgarros
Copy link
Contributor

dgarros commented Oct 3, 2021

That's the next piece of the design that we're actively thinking about. :-) Attaching a secret to an individual device is useful but definitely doesn't scale as well as being able to define secrets for groups of devices. We've talked about maybe building something atop config-contexts as an existing model in this area, but it's not definite yet.

As a first step, a SecretsProvider class could define a model/content type and expect an object of this type to be passed when get_value_for_secret is called.

Something like that.

class DeviceSpecificSecretsProvider(SecretsProvider):
    """
    Example of a plugin-provided SecretsProvider that could return a value different per device or group of devices
    """
    model = "dcim.device" 

    @classmethod
    def get_value_for_secret(cls, secret, context=None):
        """
        Return a different secret based on the name of the device.
        """
        return secret.parameters.get(context.name)

* WIP

* More WIP

* WIP remove SecretType model

* Such WIP. Wow

* WIP: working secretsgroup-edit UI

* More WIP

* Change Category/Meaning to Access Type/Secret Type

* Add SecretsGroup key to Device model; get tests passing

* Add test coverage for REST API and filters

* Add SecretsGroup view tests

* Linting fixes

* Docs updates

* Cleanup leftover SecretType cruft

* Update nautobot/docs/user-guides/git-data-source.md

Co-authored-by: Jathan McCollum <jathan@gmail.com>

* Fix egregious issues

Co-authored-by: Jathan McCollum <jathan@gmail.com>
* Support Jinja2 templating of secret parameters

* Add secrets providers to plugin detail view

* Doc updates
@glennmatthews glennmatthews changed the title Initial model, UI, and REST API for Secrets Feature: Secrets integration, including Secrets Providers and Secrets Groups Nov 9, 2021
@glennmatthews glennmatthews marked this pull request as ready for review November 9, 2021 17:44
@glennmatthews
Copy link
Contributor Author

glennmatthews commented Nov 9, 2021

This should be nearly feature-complete at this point - please review. :-)

Possible remaining TODOs:

  • Improve GraphQL representation of SecretsGroupAssociation "through" table
  • Improve REST API for creating/updating SecretsGroup's contained SecretsGroupAssociations
  • Move "Secrets" menu items out of "Extensibility" into a top-level "Secrets" menu
  • Author release-notes content

image

image

image

image

image

image

image

Devices and Git Repositories can now use Secrets Groups:

image

image

@glennmatthews
Copy link
Contributor Author

Improved GraphQL representation to allow for inspection of the "through" model:

image

image

@glennmatthews
Copy link
Contributor Author

image

…atures registry (#1065)

* Fix TypeError when trying to sort `SecretsProvider` class objects
* Don't sort `secrets_providers` when added to features.
Copy link
Contributor

@jathanism jathanism left a comment

Choose a reason for hiding this comment

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

Ship this beautiful beast!

Copy link
Member

@lampwins lampwins left a comment

Choose a reason for hiding this comment

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

This looks awesome! Just a couple of comments/questions.

nautobot/dcim/models/devices.py Outdated Show resolved Hide resolved
nautobot/extras/apps.py Show resolved Hide resolved
#


class SecretsGroupAccessTypeChoices(ChoiceSet):
Copy link
Member

Choose a reason for hiding this comment

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

Perhaps a premature optimization, but given the relative specificity defined here, should this be its own model to allow people to define their own?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The hope was that we could keep the design simpler by not introducing two additional data models here. Additionally, they kind of need to be constants because we need to be able to look them up in some fashion - if the user can dynamically define which AccessType means "http(s)" (or even not define that type at all), how can a GitRepository look up the HTTP(S) secret that it needs?

Copy link
Member

Choose a reason for hiding this comment

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

That's fair, but it is important to note we already have this problem with status, related to the imbedded logic for slacc and container. Not saying we should perpetuate that problem, but something to think about.

nautobot/extras/choices.py Show resolved Hide resolved
nautobot/extras/models/secrets.py Outdated Show resolved Hide resolved
nautobot/extras/views.py Outdated Show resolved Hide resolved
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.

None yet

5 participants