Skip to content
Snippets Groups Projects

Support for pages authentication switches

Merged Turo Soisenniemi requested to merge Turmio/omnibus-gitlab:pages_authentication into master

What does this MR do?

This merge request adds support for GitLab pages authentication switches implemented by @tuomoa in gitlab-pages!94 (merged).

Checklist

See Definition of done.

  • Change added to CHANGELOG.md. Not applicable for Documentation changes and minor changes.
  • Documentation created/updated
  • Tests added
  • Integration tests added to GitLab QA, if applicable
  • MR targeting master branch
  • MR has a green pipeline on GitLab.com

Reviewer Checklist

In addition to above, reviewer must:

  • Pipeline is green on dev.gitlab.org if the change is not touching documentation or internal cookbooks

References

https://gitlab.com/gitlab-org/gitlab-ce/issues/33422

gitlab-pages!94 (merged)

https://gitlab.com/gitlab-org/gitlab-ce/merge_requests/18589

Closes https://gitlab.com/gitlab-org/gitlab-ce/issues/33422

Edited by DJ Mountney

Merge request reports

Loading
Loading

Activity

Filter activity
  • Approvals
  • Assignees & reviewers
  • Comments (from bots)
  • Comments (from users)
  • Commits & branches
  • Edits
  • Labels
  • Lock status
  • Mentions
  • Merge request status
  • Tracking
  • added 1 commit

    • a8861481 - Support for pages authentication switches

    Compare with previous version

  • @tuomoa @Turmio what's the status of this? MR? Is it essential to get it into 11.2 as well?

    If mattermost uses exactly the same mechanism, I don't have any particular objections to merging it as-is, but it would need approval from an omnibus maintainer, so that's no guarantee.

  • @nick.thomas I think it's only missing the access_control hint that I added in the GitLab but other than that it should be okay. It might be a good idea to test it also, as I'm not sure how to test it.

  • Maintainer

    @Turmio Thanks for the merge request.

    A few quick notes:

    • It looks like this depends on gitlab-pages!94 (merged) which isn't yet merged. I'll mark this as blocked for now.

    • You will also want to enable shared runners as per our contributing guide so our automated tests can run.

    • I don't think @bjk-gitlab is necessary for an approval on this, it should be safe to remove him from the approvers list.

  • Author Contributor

    @ibaum thanks! I don't know how I managed to skip whole contributing guide.

  • Turo Soisenniemi added 254 commits

    added 254 commits

    • a8861481...1bfd77c2 - 252 commits from branch gitlab-org:master
    • c3992dc3 - Support for pages authentication switches
    • 653a4215 - Added access_control switch for pages and set it enabled by default

    Compare with previous version

  • Author Contributor

    I added access_control setting (defaults to true) and rebased to latest master.

    @nick.thomas I did not have time to create VM and test omnibus after @ibaum pointed me to right direction. I may have time to read up on testing guide and find out how to use @tuomoa's forks tomorrow but I cannot promise that. At least pipeline went to green.

    If everything works fine and secret can be generated as in this MR, I think this is ready.

  • Nick Thomas changed milestone to %11.3

    changed milestone to %11.3

  • added 1 commit

    • 71e65bc3 - Fixed usage of access control properties, previous implementation used always defaults.

    Compare with previous version

  • Maintainer

    Hi @Turmio,

    Your latest commits are causing some valid spec failures. Can you take a look?

  • Author Contributor

    Hi @ibaum,

    Yeah, I will. I just did not have time to fix those when I made changes with Web IDE and thought that I have time later that day. I will fix those failures later this week and rebase to latest master.

  • Nick Thomas added 1 commit

    added 1 commit

    • 50106192 - Fix specs to take account of new subdomain for Pages

    Compare with previous version

  • Nick Thomas resolved all discussions

    resolved all discussions

  • Nick Thomas added 175 commits

    added 175 commits

    Compare with previous version

  • @Turmio @ibaum @tuomoa I've taken the liberty of pushing some improvements to this MR, but I don't think it's ready yet.

    The failing test was easy to fix. What remains, I think, is fettling the gitlab.yml file. Like gitlab_rails['pages_enabled'], we need to correctly set gitlab_rails['pages_access_control'] based on the gitlab_pages['access_control'] attribute, and add specs to ensure it does the right thing.

    I'm happy with the feature itself, so I think I'm going to merge the pages and CE MRs for 11.4 anyway. Without omnibus support, it will be disabled by default. If we can get the omnibus MR in for 11.4 (I'm afraid I can't spend any more time on it myself), great. Otherwise, we can target it for 11.5 to be enabled by default.

  • mentioned in commit gitlab-pages@86ad3207

  • @ibaum some of the specs are now failing with:

    RuntimeError
    ------------
    The sidekiq_cluster queue_groups must be set in order to use the sidekiq-cluster service

    Presumably this is from a change in master that came in with the merge to latest?

  • Nick Thomas changed milestone to %11.4

    changed milestone to %11.4

  • (I'll keep this as 11.4 until the deadline is past, just in case someone is able to get it in by then)

  • Nick Thomas changed the description

    changed the description

  • @nick.thomas sounds reasonable. What is the timeline for 11.4? Maybe I or @Turmio could spend some time on this to get it finished and into the 11.4. :)

  • Nick Thomas
  • Nick Thomas
  • Nick Thomas
  • Nick Thomas
  • some of the specs are now failing with:

    RuntimeError
    ------------
    The sidekiq_cluster queue_groups must be set in order to use the sidekiq-cluster service

    @nick.thomas Huh. That is weird. That isn't the reason why tests fail. Please check the JUnit report on this MR to see what actually failed.

    I will open an issue to fix the test reports. EDIT: #3831 (closed)

    Edited by Balasankar 'Balu' C
  • Note that I doubt we can enable this feature on GitLab.com / gitlab.io without this MR. So we should keep https://gitlab.com/gitlab-org/gitlab-ce/issues/33422 open until it's finished.

  • added devopsrelease [DEPRECATED] label and removed devopsverify label

  • @tuomoa merge cutoff is the 7th, which falls on a sunday this month. So reasonably speaking, today is the last day.

  • Maintainer

    Some documentation for this would be great. Pages administration documentation should live in ce

  • I've got the start of some docs in https://gitlab.com/gitlab-org/gitlab-ce/merge_requests/22146 - it details what is now, rather than what should be, though! :scream:

    Edited by Nick Thomas
  • Maintainer

    The current spec failures I think are valid. Once those are clear, and the comments are addressed, we can build a package to test this out.

  • added 1 commit

    • 4f051c4a - Fix the default test to not expect auth and fix custom config to set access control to true

    Compare with previous version

  • added 1 commit

    • 64912855 - Fix the default test to not expect auth and fix custom config to set access control to true

    Compare with previous version

  • added 1 commit

    • c004f95d - Only authorize pages if access control is enabled, add missing access control…

    Compare with previous version

  • added 1 commit

    • 77f29e7a - Only authorize pages if access control is enabled, add missing access control…

    Compare with previous version

  • added 1 commit

    • 84aa56ed - Only authorize pages if access control is enabled, add missing access control…

    Compare with previous version

  • added 1 commit

    • 2668a690 - Only authorize pages if access control is enabled, add missing access control…

    Compare with previous version

  • added 1 commit

    • b0a9d707 - Only authorize pages if access control is enabled, add missing access control…

    Compare with previous version

  • Loading
  • Loading
  • Loading
  • Loading
  • Loading
  • Loading
  • Loading
  • Loading
  • Loading
  • Loading
  • Please register or sign in to reply
    Loading