Skip to content
Snippets Groups Projects

Add OIDC compatibility

Open Pablo Vigo Mas requested to merge wip/pvigo/add-oidc-compatibility into collabora/staging
2 unresolved threads

OIDC authentication requires a different Docker image from the one used for publishing repository content. This customized image loads the OIDC module to enable the connection and loads other modules listed in the Helm Chart values file by default.

Forcing the container to load the default Helm Chart value modules results in the error "module is built-in and can't be loaded," causing the container to stop.

This Helm Chart must be compatible with different Apache2 images. Therefore, a new conditional is added to avoid loading the default modules if CUSTOM_MODULES is defined.

Phabricator task: T10135

Signed-off-by: Pablo Vigo pvigo@collabora.com

Edited by Pablo Vigo Mas

Merge request reports

Merge request pipeline #123859 passed

Merge request pipeline passed for 2cebf728

Approval is optional
Merge blocked: 1 check failed
Unresolved discussions must be resolved.

Merge details

  • 1 commit will be added to collabora/staging.
  • Source branch will be deleted.

Activity

Filter activity
  • Approvals
  • Assignees & reviewers
  • Comments (from bots)
  • Comments (from users)
  • Commits & branches
  • Edits
  • Labels
  • Lock status
  • Mentions
  • Merge request status
  • Tracking
  • assigned to @refi64

  • Pablo Vigo Mas requested review from @ritesh

    requested review from @ritesh

  • Author Maintainer

    Tested in aptly-test-staging with no issues

    ansible-playbook -i inventories/production -l aptly-test-staging digital-ocean-k8s/aptly.yaml --diff

    ...
     data:
        httpd.conf: |
          ServerRoot "/usr/local/apache2"
          Listen 80
    
              
          LoadModule mpm_event_module modules/mod_mpm_event.so
          LoadModule authz_core_module modules/mod_authz_core.so
          LoadModule allowmethods_module modules/mod_allowmethods.so
          LoadModule reqtimeout_module modules/mod_reqtimeout.so
          LoadModule mime_module modules/mod_mime.so
    -     LoadModule log_config_module modules/mod_log_config.so
          LoadModule headers_module modules/mod_headers.so
          LoadModule proxy_module modules/mod_proxy.so
          LoadModule proxy_http_module modules/mod_proxy_http.so
    -     LoadModule unixd_module modules/mod_unixd.so
          LoadModule dir_module modules/mod_dir.so
          LoadModule autoindex_module modules/mod_autoindex.so
    +     <ifDefine !OIDC_CONFIG_PRESENT>
    +       LoadModule log_config_module modules/mod_log_config.so
    +       LoadModule unixd_module modules/mod_unixd.so
    +     </IfDefine>
    ...
15 15 LoadModule allowmethods_module modules/mod_allowmethods.so
16 16 LoadModule reqtimeout_module modules/mod_reqtimeout.so
17 17 LoadModule mime_module modules/mod_mime.so
18 LoadModule log_config_module modules/mod_log_config.so
19 18 LoadModule headers_module modules/mod_headers.so
20 19 LoadModule proxy_module modules/mod_proxy.so
21 20 LoadModule proxy_http_module modules/mod_proxy_http.so
22 LoadModule unixd_module modules/mod_unixd.so
23 21 LoadModule dir_module modules/mod_dir.so
24 22 LoadModule autoindex_module modules/mod_autoindex.so
23 <ifDefine !OIDC_CONFIG_PRESENT>
  • IMO this is really confusing to look at: it's basically implicitly tying the loading of these modules to what is, fundamentally, an entirely unrelated config option, and the only reason it still works is because of the existence of an image unassociated with this repo that happens to have that enabled.

    My ideal scenario would be that the OIDC config is actually in the playbooks and injected via before_modules / after_modules...buuuut I'm gonna guess that's not workable if this is coming from an external image? So instead, this could add a new item to values.yaml's publish:config: section to control whether or not these two modules are present. That way, the playbooks can set this in the same place where it chooses the docker image to use.

  • Author Maintainer

    What do you think about having an option to control all the modules that the "Chart" loads by default?

    Enable or disable all the modules.

    From ansible-playbook, we can add whichever we want, making the Helm Chart fully compatible with all current and future Docker images.

  • Enable or disable all the modules.

    Hmm maaaybe, my only concern would just be that the modules are basically soup and that runs the risk of them going wildly out-of-sync...but I guess that's a risk already incurred by the fact that the docker image being used has an existing module setup anyway, so it's probably not much of a loss...so yeah that would probably be fine.

    @em any extra thoughts here?

  • Pablo Vigo Mas changed this line in version 2 of the diff

    changed this line in version 2 of the diff

  • Author Maintainer

    Please, review the new proposal :)

  • Author Maintainer

    @em get a look at this

  • Author Maintainer

    I added a conditional to avoid loading default modules

  • Please register or sign in to reply
  • added 1 commit

    Compare with previous version

  • Pablo Vigo Mas added 2 commits

    added 2 commits

    • ae3635da - 1 commit from branch collabora/staging
    • c67d398e - Add OIDC compatibility

    Compare with previous version

  • Pablo Vigo Mas added 1 commit

    added 1 commit

    Compare with previous version

  • Pablo Vigo Mas added 1 commit

    added 1 commit

    Compare with previous version

  • Pablo Vigo Mas changed the description

    changed the description

  • Pablo Vigo Mas assigned to @em and unassigned @refi64

    assigned to @em and unassigned @refi64

  • Pablo Vigo Mas changed the description

    changed the description

  • Emanuele Aina
    Emanuele Aina @em started a thread on the diff
  • 28 28 pullPolicy: IfNotPresent
    29 29 tag: 2.4
    30 30 config:
    31 # Disable default modules can be set here, e.g.:
    32 # before_modules: 'Define CUSTOM_MODULES'
    31 33 before_modules: ''
    34 # Specify custom modules to load can be set here, e.g.:
    35 # after_modules: |
    36 # LoadModule authz_user_module modules/mod_authz_user.so
    37 # LoadModule authn_core_module modules/mod_authn_core.so
    32 38 after_modules: ''
    • Comment on lines +31 to 38

      I would rather move to a default config.modules value that people can override with whatever they want:

      Suggested change
      31 # Disable default modules can be set here, e.g.:
      32 # before_modules: 'Define CUSTOM_MODULES'
      33 before_modules: ''
      34 # Specify custom modules to load can be set here, e.g.:
      35 # after_modules: |
      36 # LoadModule authz_user_module modules/mod_authz_user.so
      37 # LoadModule authn_core_module modules/mod_authn_core.so
      38 after_modules: ''
      31 before_modules: ''
      32 modules:
      33 LoadModule mpm_event_module modules/mod_mpm_event.so
      34 LoadModule authz_core_module modules/mod_authz_core.so
      35 LoadModule allowmethods_module modules/mod_allowmethods.so
      36 LoadModule reqtimeout_module modules/mod_reqtimeout.so
      37 LoadModule mime_module modules/mod_mime.so
      38 LoadModule log_config_module modules/mod_log_config.so
      39 LoadModule headers_module modules/mod_headers.so
      40 LoadModule proxy_module modules/mod_proxy.so
      41 LoadModule proxy_http_module modules/mod_proxy_http.so
      42 LoadModule unixd_module modules/mod_unixd.so
      43 LoadModule dir_module modules/mod_dir.so
      44 LoadModule autoindex_module modules/mod_autoindex.so
      45 LoadModule log_config_module modules/mod_log_config.so
      46 LoadModule unixd_module modules/mod_unixd.so
      47 # Specify custom modules to load can be set here, e.g.:
      48 # after_modules: |
      49 # LoadModule authz_user_module modules/mod_authz_user.so
      50 # LoadModule authn_core_module modules/mod_authn_core.so
      51 after_modules: ''

      And then you can set:

        config:
          modules:
            LoadModule authz_user_module modules/mod_authz_user.so
            LoadModule authn_core_module modules/mod_authn_core.so
            LoadModule authn_file_module modules/mod_authn_file.so
            LoadModule auth_basic_module modules/mod_auth_basic.so
            LoadModule access_compat_module modules/mod_access_compat.so
            LoadModule authz_host_module modules/mod_authz_host.so

      Alternatively, can't we tweak the custom image to ensure that modules listed by default are actually built as modules?

    • I mean, the best solution would be to have both things :)

      1. modules are built as modules in the image
      2. the default list can be overridden in the chart
    • Please register or sign in to reply
    Please register or sign in to reply
    Loading