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

Ability to use object store for images #1814

Closed
jpds opened this issue Jan 9, 2018 · 11 comments
Closed

Ability to use object store for images #1814

jpds opened this issue Jan 9, 2018 · 11 comments
Assignees
Labels
status: accepted This issue has been accepted for implementation
Milestone

Comments

@jpds
Copy link

jpds commented Jan 9, 2018

Issue type

[x] Feature request
[ ] Bug report
[ ] Documentation

Environment

  • NetBox version: v2.2.9

Description

I'm running Netbox in a Kubernetes cluster. It currently runs as a single container as I have a block device mounted into the container so that there's persistent storage for the image attachments which were introduced in #152.

This works OK, but I cannot run multiple instances of netbox in a clustered fashion as it's tied to the block device.

Could there be some integration with https://github.com/jschneier/django-storages so that S3 can be used as a backend for the image attachments?

@jeremystretch jeremystretch added status: accepted This issue has been accepted for implementation type: minor feature labels Jan 31, 2018
@jeremystretch jeremystretch added status: under review Further discussion is needed to determine this issue's scope and/or implementation and removed status: accepted This issue has been accepted for implementation labels Dec 5, 2018
@jeremystretch
Copy link
Member

Per the discussion in PR #2466, the configuration scheme for this needs to be fleshed out. While I don't have a strong preference, whatever solution we decide on must be platform- and vendor-agnostic, and easily extendable to accommodate similar services.

@nward
Copy link

nward commented Apr 22, 2019

Hi, I've been directed here from #3095 - thanks for that @jeremystretch.

I have implemented something as a PoC for this.
Additionally, I implemented a way to optionally load another application for display. "django-storages" doesn't require this, as it directs the user for the application to a direct download URL. This has the effect that images are not behind the NetBox access control anymore. Writing an application for this would allow an administrator to use S3/etc. for only storage, not for serving the files.

For the configuration, I toyed with several approaches:

  1. Have configuration for all the various options (S3_* etc.). This doesn't work, as in NetBox we have only specific settings passed through from configuration.py to settings.py, and we would need settings.py to support every possible option - doesn't really fly.
  2. Have a dictionary defined in configuration.py which the "external" applications, storage classes etc. can use for settings. This requires that these applications, storage classes etc. are written custom for NetBox (or at the very least, with NetBox support). This is not untenable as potentially some sort of wrapper classes could be written but it's not ideal.
  3. Have a dictionary defined in configuration.py which NetBox's settings.py takes and translates in to settings.py parameters.
  4. Have settings in configuration.py which are found by settings.py with a prefix, and then the prefix stripped off and defined as settings.py parameters. This is OK, but a bit clunky and hard to read.

In my implementation, I opted for (3) which I found this to be a good balance between (1) and (2). It has some things I'd like to improve. Primarily reduce or eliminate the ability for a configuration parameter to be named the same as an existing NetBox parameter and override it - perhaps it needs to scan a list of known settings used by NetBox and error if the administrator attempts to pass them in this dictionary. That seems fairly easy to implement.

Perhaps (3) and (2) at the same time would be a good option - "NetBox Native" storage systems and applications could use (2) while existing systems (django-storages, etc.) could use (3).

@chaomodus
Copy link

Note I have very simply implemented the above option 3 for our use since we need to use a Swift back-end to save images (and there are numerous configuration settings that have to be imported) in case anyone is interested.

https://gerrit.wikimedia.org/r/c/operations/software/netbox/+/518785

wmfgerrit pushed a commit to wikimedia/operations-software-netbox that referenced this issue Jun 25, 2019
This is an implementation of the strategy 3 from this
comment: netbox-community/netbox#1814 (comment)

Bug: T209182
Change-Id: Icbdf212b7b1f9ff59fef4f5ecf64bfd9fdc41238
@jeremystretch jeremystretch added status: needs owner This issue is tentatively accepted pending a volunteer committed to its implementation and removed help wanted status: under review Further discussion is needed to determine this issue's scope and/or implementation labels Oct 17, 2019
@steffann
Copy link
Contributor

steffann commented Nov 3, 2019

I'd be happy to take this issue.

Because of the separation between settings.py and configuration.py I suggest to abstract the implementation from the configuration and start with the most common one: AWS S3 and compatible.

The configuration could look like this:

MEDIA_STORAGE = {
  'BACKEND': 'S3',
  'ACCESS_KEY_ID': 'Secret',
  'SECRET_ACCESS_KEY': 'Secret',
  'REGION_NAME': 'eu-west-1',
  'ENDPOINT_URL': 'https://example/',
  'BUCKET_NAME': 'netbox',
  'DEFAULT_ACL': 'public-read',
}

For the implementation I suggest adding django-storages as a dependency when the S3 back-end is used.

Any objections to this approach?

@nward
Copy link

nward commented Nov 3, 2019

Hi @steffann ,

In my scenario, S3 is not appropriate, so I don't think it should be the only option.

I understand that Django-storages has various backends, however, none were suitable for my environment or tool set.

I implemented a similar configuration system to you because of the configuration/settings.py split, however, it allows any storage system to be used through an additional application. I encourage you to have a look at it, before going ahead with other solutions:
#3095

Django-storages could be implemented through this interface I believe, and it would allow any storage system to be used.

Personally, I implemented a storage layer which uses postgres - which is of course not a good idea for many people as has been noted in a number of other discussions, however, for my environment this is the right choice right now for a number of reasons. I think giving people the ability to choose what they do here is best. I think it would be good to include "here's how to use django-storages to get S3/whatever" as a good "default" though.

@steffann
Copy link
Contributor

steffann commented Nov 4, 2019

Hi @nward,

I completely understand. Based on the existing code I have the feeling that @jeremystretch prefers explicit support and configuration though. I'll leave it up to him to decide whether "officially supported options with corresponding configuration" or "flexibility to use anything you want with less clearly defined configuration boundaries" is the way forward.

Cheers,
Sander

@stale
Copy link

stale bot commented Dec 6, 2019

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. NetBox is governed by a small group of core maintainers which means not all opened issues may receive direct feedback. Please see our contributing guide.

@stale stale bot added the pending closure Requires immediate attention to avoid being closed for inactivity label Dec 6, 2019
@nward
Copy link

nward commented Dec 6, 2019

Hi @steffann I think my solution is a good compromise between both those extremes. The defaults can implement officially supported options, while still allowing people to use alternative options.

(Sorry, though I had replied to this back in Nov!)

@stale stale bot removed the pending closure Requires immediate attention to avoid being closed for inactivity label Dec 6, 2019
@steffann
Copy link
Contributor

steffann commented Dec 6, 2019

@nward I did see it back in November but got confused with all the proposed solutions. Yours does indeed look like a good middle ground!

Because it took me a while to find it, here is the link for others to help evaluate it: SearchLightNZ@074ace6

I haven't tested it yet, but it does look well designed.

@jeremystretch jeremystretch added status: accepted This issue has been accepted for implementation and removed status: needs owner This issue is tentatively accepted pending a volunteer committed to its implementation labels Dec 9, 2019
@jeremystretch
Copy link
Member

@steffann I've taken some more time to dig into this. I have to say that maintaining a barrage of settings specific to each storage backend within NetBox isn't realistic. Even if we were to only support S3 (which IMO is underwhelming), it would require NetBox to essentially sync with the development of django-storages, which isn't a burden I'm willing to accept on behalf of the other maintainers.

Based on the existing code I have the feeling that @jeremystretch prefers explicit support and configuration though.

This is correct: We should avoid opening an avenue for users to import unsanitized settings for various reasons.

I recommend a more lean approach, where we introduce support for the optional use of django-storages, but completely offload the configuration details into a single opaque setting. We can expose two NetBox configuration parameters:

  • STORAGE_BACKEND - A wrapper around Django's built-in DEFAULT_FILE_STORAGE setting, used to indicate the specific file storage mechanism
  • STORAGE_CONFIG - An arbitrary dictionary of settings to be passed to the storage backend

In the case of django-storages, which I imagine is what most users will use, we can monkey patch its utils.setting getter to first look for keys inside settings.STORAGE_CONFIG (and fall back to settings if the key is not found). Something like this:

if STORAGE_BACKEND is not None:

    import storages

    def _setting(name, default=None):
        if name in settings.STORAGE_CONFIG:
            return getattr(settings.STORAGE_CONFIG, name, default)
        return getattr(settings, name, default)
    
    storages.utils.setting = _setting

This would allow the user complete control over configuring the storage engine, while still keeping the global NetBox configuration gated.

I will also note that I'd prefer to avoid making django-storages a required dependency, as it would pull in each of its supported backends. It would probably be sufficient to simply require django-storages if STORAGE_BACKEND is defined.

@steffann
Copy link
Contributor

That was actually a lot easier than expected. Simple solutions FTW!

#3666 has been rebased to develop2.7, and the new implementation is ready for merge.

@jeremystretch jeremystretch added this to the v2.7 milestone Dec 11, 2019
wmfgerrit pushed a commit to wikimedia/operations-software-netbox that referenced this issue Feb 12, 2020
This is an implementation of the strategy 3 from this
comment: netbox-community/netbox#1814 (comment)

Bug: T209182
Change-Id: Icbdf212b7b1f9ff59fef4f5ecf64bfd9fdc41238
(cherry picked from commit 60c58bd)
wmfgerrit pushed a commit to wikimedia/operations-software-netbox that referenced this issue Feb 28, 2020
This is an implementation of the strategy 3 from this
comment: netbox-community/netbox#1814 (comment)

Bug: T209182
Change-Id: Icbdf212b7b1f9ff59fef4f5ecf64bfd9fdc41238
(cherry picked from commit 60c58bd)
(cherry picked from commit b46b469)
@lock lock bot locked as resolved and limited conversation to collaborators Mar 10, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
status: accepted This issue has been accepted for implementation
Projects
None yet
Development

No branches or pull requests

5 participants