Add REMOTE_AUTH_* configs

[shuichiro-makigaki]

Related Issue:

New Behavior

The remote authentication feature, which is available from Netbox v2.8, will be supported in the docker image.

Contrast to Current Behavior

...

Discussion: Benefits and Drawbacks

...

Changes to the Wiki

...

Proposed Release Note Entry

Add REMOVE_AUTH_* configurations #310

REMOTE_AUTH_ENABLED, REMOTE_AUTH_BACKEND, REMOTE_AUTH_HEADER, REMOTE_AUTH_AUTO_CREATE_USER and REMOTE_AUTH_DEFAULT_GROUPS can be configured via environment variables.

Double Check

• I have read the comments and followed the PR template.
• I have explained my PR according to the information in the comments.
• My PR targets the develop branch.

[shuichiro-makigaki]

@cimnine Netbox v2.8.6 was released a week ago. Could you make a new release of netbox-docker as well with some PRs (including my PR)?

[cimnine]

Sorry, it was – and still is – a busy time. I've not have had time to look into all the PRs.

From a first glance, what I don't like are the literal_eval statements which you make use of. My opinion is that if a configuration property is too complex to be a simple type (string, bool, number), then it should be loaded from a file. Because a file can be type-checked, formatted, commented, etc. Docker has perfect support for mounting (config) files, so I would prefer if REMOTE_AUTH_DEFAULT_PERMISSIONS was actually REMOTE_AUTH_DEFAULT_PERMISSION_CONFIG and the value a path to the respective file (Python, YAML, JSON, ...).

Since this is a problem in other open PRs as well, maybe – instead of loading X additional files (or eval X env variables for that matter) – we should aim for a more flexible option altogether, e.g. loading all files in a certain directory (e.g. /etc/netbox/config/*) and evaluate them in the context of the config file (i.e. changing the way configuration.docker.py works).

Independent of the above, I'm not sure whether I like the if statement with code for 2.8 and 2.9+. Maybe we should just wait until 2.9 is released and get rid of the condition. It is hard enough to keep up with the current versions, so I don't fancy implementing backwards-compatibility.

[cimnine]

I've started a discussion issue here: #318

[shuichiro-makigaki]

As for dynamic variables, your concern is reasonable, and I also have the same mind. I'd like to join the discussion in a separate issue.

Actually, REMOTE_AUTH_DEFAULT_PERMISSIONS is optional for my current requirement, at least, but I need other remote auth configs. Therefore, for now, I will delete lines of REMOTE_AUTH_DEFAULT_PERMISSIONS in this pull request. This pull request will have partial patches of only basic remote authentication configurations, and (hopefully) is expected to merge. After a discussion of dynamic variables, REMOTE_AUTH_DEFAULT_PERMISSIONS will be implemented. (Some comments to be implemented later may be written in the code.)

Independent of the above, I'm not sure whether I like the if statement with code for 2.8 and 2.9+. Maybe we should just wait until 2.9 is released and get rid of the condition. It is hard enough to keep up with the current versions, so I don't fancy implementing backwards-compatibility.

As you said, the if is not so "cool." I just added the line to pass all tests.
I believe this docker image should support v2.8's remote auth as the first citizen because the current stable release is v2.8.x. Support for v2.9 is optional though we need to change some default value when v2.9 is released. As a disadvantage, tests for the daily build will fail until v2.9 is released, but v2.9 tests are not required to pass all.

In summary, I will fix this pull request:

• Delete REMOTE_AUTH_DEFAULT_PERMISSIONS in this time, and wait for a decision about dynamic variables
• Delete v2.9 support in this time, and wait for the happy release of v2.9

[herbetom]

Since 2.9 was released it might be time to update this PR and get this merged.

[herbetom]

I just added the missing options from your PR into my PR #339 which just addressed REMOTE_AUTH_ENABLED and REMOTE_AUTH_BACKEND. I wasn't aware of this PR when i originally opened my PR.

My goal is to get this merged soon. But since my PR is now just basically your PR (exept the if) i would propose merging this PR without the if (and closing mine).

[herbetom]

Good, i'm gonna close my PR. You should probably squash those two commits. But what about REMOTE_AUTH_DEFAULT_PERMISSIONS? With the second commit you simply removed it. That doesn't seem right to me.

[shuichiro-makigaki]

The REMOTE_AUTH_DEFAULT_PERMISSIONS requires a dynamic variable system, which is implemented in #343.
In this PR, I want to avoid discussing the dynamic variable implementation and to enable remote authentication as soon as possible.
I delete REMOTE_AUTH_DEFAULT_PERMISSIONS at this time and wait for a decision about dynamic variables.

[herbetom]

Okay, I definitely agree with getting this merged as soon as possible.

(You might have noticed that I'm trying exactly that the last weeks 😅)

[shuichiro-makigaki]

@cimnine #343 is reasonable and maybe makes this PR unnecessary. However, the discussion takes a longer time than we expected... If the new configuration system may take a longer time to merge and release, I think this PR (without dynamic variables of REMOTE_AUTH_DEFAULT_PERMISSIONS) is required for the features of Netbox v2.9.