Skip to content
Snippets Groups Projects

Support for pages authentication switches

Merged Turo Soisenniemi requested to merge Turmio/omnibus-gitlab:pages_authentication into master
All threads resolved!

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

  • added 1 commit

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

    Compare with previous version

  • I did some work on this one, I added more tests but one of them is still failing (gitlab::gitlab-pages with user settings authorizes pages with gitlab). I think the actual implementation is okay but there is something wrong with the test. I'll take a look when I have more time.

    Edit: The tests are now passing.

    Edited by Tuomo Ala-Vannesluoma
  • added 1 commit

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

    Compare with previous version

  • OK, this looks convincing to me, but I'm not clear on how to get a package out of this for testing! WDYT @ibaum ?

  • Author Contributor

    Sorry about radio silence. I have had some family issues (our second son was born week ago and my wife had and still has health issues) so I have had zero free time.

    I am glad that this MR is moving on!

  • Maintainer

    Our Trigger:package job doesn't work for forks it looks like. I pushed to dev.gitlab.org instead, pipeline is https://dev.gitlab.org/gitlab/omnibus-gitlab/pipelines/96318

  • Ian Baum
  • Ian Baum
  • Maintainer

    I was able to test this on a dev instance by setting my pages_external_url, and gitlab_pages['access_control'] = true in my /etc/gitlab/gitlab.rb.

    On the initial run of reconfigure, -auth-client-id and -auth-client-secret were empty. They did get populated on the second run of reconfigure.

    Also, a couple of comments about the authorize_with_gitlab call. And documentation would be great as well.

    Edited by Stan Hu
  • Jason Yavorska added 1 deleted label

    added 1 deleted label

  • Hmm. Any ideas what could be the problem with the initial run?

  • Maintainer

    @tuomoa I think if we use lazy evaluation for the runit_service gitlab::gitlab-pages recipe, that should populate the correct values on initial run.

  • I understand from @nick.thomas that this the primary feature here was reverted in 11.4 due to regression gitlab-pages#170 (closed). Updated this item to 11.5 since we are now past feature freeze.

  • Jason Yavorska changed milestone to %11.5

    changed milestone to %11.5

  • added 1 commit

    • 406f11e6 - Add guards to skip authorize if defined, add tests, try to use lazy evaluation

    Compare with previous version

  • @ibaum I tried adding the lazy evaluation to the runit_service in the pages recipe, however I'm not sure if I did it right (I'm struggling to create a test for it).

    Is there a way to test the package myself as I can't access the http://dev.gitlab.org? :(

  • Maintainer

    @tuomoa Our development document has instructions on how to get setup developing with our cookbooks. This should help you test everything out.

    Let me know if you have any issues with it.

  • @ibaum thanks I'll take a look when I have time. :)

  • 2 Warnings
    :warning: This merge request does not have any assignee yet. Setting an assignee clarifies who needs to take action on the merge request at any given time.
    :warning: You’ve made some changes at the locations which contain user facing configuration.
    That’s OK as long as you’re refactoring existing code and not adding any new
    configuration. If you are adding new user facing configuration, consider adding
    to gitlab.rb.template located in files/gitlab-config-template/gitlab.rb.template .
    Otherwise, please consider adding the ~backstage label in that case.

    Generated by :no_entry_sign: Danger

    Edited by 🤖 GitLab Bot 🤖
  • Ok, I figured out the problem: the authorize_with_gitlab call changes GitLab attributes after Omnibus has already populated the node attributes at the start (see https://gitlab.com/gitlab-org/omnibus-gitlab/blob/3019267cbfa2c3c371c1dc6cf875a3e497cd3474/files/gitlab-cookbooks/gitlab/recipes/config.rb#L26 and https://gitlab.com/gitlab-org/omnibus-gitlab/blob/3019267cbfa2c3c371c1dc6cf875a3e497cd3474/files/gitlab-cookbooks/package/libraries/helpers/settings_helper.rb#L166-179).

    A quick fix is to do what the Mattermost recipe does and reload the entire config:

    diff --git a/files/gitlab-cookbooks/gitlab/recipes/gitlab-pages.rb b/files/gitlab-cookbooks/gitlab/recipes/gitlab-pages.rb
    index 383ccd681..4a33fe81f 100644
    --- a/files/gitlab-cookbooks/gitlab/recipes/gitlab-pages.rb
    +++ b/files/gitlab-cookbooks/gitlab/recipes/gitlab-pages.rb
    @@ -42,6 +42,13 @@ ruby_block "authorize pages with gitlab" do
       only_if { node['gitlab']['gitlab-pages']['access_control'] }
     end
     
    +# Options may have changed in the previous step
    +ruby_block "re-populate GitLab Pages configuration options" do
    +  block do
    +    node.consume_attributes(Gitlab.hyphenate_config_keys)
    +  end
    +end
    +
     file File.join(working_dir, "VERSION") do
       content VersionHelper.version("/opt/gitlab/embedded/bin/gitlab-pages -version")
       notifies :restart, "service[gitlab-pages]"

    Note: to reproduce the problem, you just need to wipe the gitlab-pages entry from /etc/gitlab/gitlab-secrets.json and run gitlab-ctl reconfigure.

    Edited by Stan Hu
  • @stanhu good catch. I'll commit that change and remove the lazy stuff from the runit_service. I'm setting up the development VM at the moment to be able to test this.

    Edit: I was able to test it and it worked. @nick.thomas has already started on some documentation in gitlab-ce!22146. I suppose we should update that to match what's been done on this MR (gitlab.rb stuff etc.).

    Edited by Tuomo Ala-Vannesluoma
  • added 1 commit

    • 555e1a29 - Add re-populate configuration options in pages

    Compare with previous version

  • Stan Hu
  • Stan Hu
  • I kicked off a package to test here: https://gitlab.com/gitlab-org/omnibus-gitlab/-/jobs/107770680

    It seems to work for me. Great work here.

  • assigned to @ibaum

  • How is this coming? Do we expect this (and thereby the rest of the feature) to make it in for 11.4?

  • This won't make it to 11.4, it is too late in the cycle.

  • Thanks for confirmation @marin

  • added 1 commit

    • 56e43475 - Update the warning message when app id and secret query fails

    Compare with previous version

  • Stan Hu
  • added 1 commit

    Compare with previous version

  • Ian Baum approved this merge request

    approved this merge request

  • Maintainer

    I think this looks ok now. My testing went well (latest dev pipeline is https://dev.gitlab.org/gitlab/omnibus-gitlab/pipelines/97159).

    The pages administration documentation should be updated with how to enable/disable this.

    And we do need a definitive decision on whether or not to enable by default for 11.5.

    This is pretty complicated, so I definitely think it could use a second pass.

  • assigned to @twk3

  • Stan Hu approved this merge request

    approved this merge request

  • Thanks @ibaum! I will get onto the documentation Monday.

    If it weren't for the HA scenario, I'd be strongly for this being enabled by default, but that complicates matters somewhat :/

  • DJ Mountney resolved all discussions

    resolved all discussions

  • Contributor

    I ran into issues testing this here: !2583 (comment 110426292)

  • assigned to @Turmio

  • added 1 commit

    • 37f43947 - Update files/gitlab-cookbooks/gitlab/templates/default/sv-gitlab-pages-run.erb

    Compare with previous version

  • Turo Soisenniemi resolved all discussions

    resolved all discussions

  • DJ Mountney marked the checklist item Tests added as completed

    marked the checklist item Tests added as completed

  • DJ Mountney marked the checklist item MR targeting master branch as completed

    marked the checklist item MR targeting master branch as completed

  • assigned to @twk3

  • DJ Mountney marked the checklist item MR has a green pipeline on GitLab.com as completed

    marked the checklist item MR has a green pipeline on GitLab.com as completed

  • DJ Mountney mentioned in commit 7b1a22a1

    mentioned in commit 7b1a22a1

  • merged

  • Contributor

    Awesome, thanks @Turmio

  • Due to an issue the workflowstaging-ref label was incorrectly applied to this merge request. Re-setting back to workflowproduction using https://gitlab.com/gitlab-org/release-tools/-/merge_requests/1620

  • Please register or sign in to reply
    Loading