Support for pages authentication switches
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
https://gitlab.com/gitlab-org/gitlab-ce/merge_requests/18589
Merge request reports
Activity
mentioned in merge request gitlab-pages!94 (merged)
- Resolved by Nick Thomas
@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.@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.
-
added workflowblocked label
@ibaum thanks! I don't know how I managed to skip whole contributing guide.
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
-
a8861481...1bfd77c2 - 252 commits from branch
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.
changed milestone to %11.3
added 1 commit
- 71e65bc3 - Fixed usage of access control properties, previous implementation used always defaults.
Hi @Turmio,
Your latest commits are causing some valid spec failures. Can you take a look?
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.
added 1 commit
- 50106192 - Fix specs to take account of new subdomain for Pages
added 175 commits
-
50106192...f53c3e7f - 173 commits from branch
gitlab-org:master
- d3424c83 - Merge branch 'master' into pages_authentication
- f73ad3d6 - Some omnibus improvements
-
50106192...f53c3e7f - 173 commits from branch
@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 setgitlab_rails['pages_access_control']
based on thegitlab_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?
added Community contribution devopsverify labels and removed workflowblocked label
changed milestone to %11.4
@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. :)
- Resolved by Ian Baum
- Resolved by Turo Soisenniemi
- Resolved by Turo Soisenniemi
- Resolved by Turo Soisenniemi
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' CNote 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
mentioned in issue gitlab-development-kit#403
@tuomoa merge cutoff is the 7th, which falls on a sunday this month. So reasonably speaking, today is the last day.
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!
Edited by Nick Thomasadded 1 commit
- 4f051c4a - Fix the default test to not expect auth and fix custom config to set access control to true
added 1 commit
- 64912855 - Fix the default test to not expect auth and fix custom config to set access control to true
added 1 commit
- c004f95d - Only authorize pages if access control is enabled, add missing access control…
added 1 commit
- 77f29e7a - Only authorize pages if access control is enabled, add missing access control…
added 1 commit
- 84aa56ed - Only authorize pages if access control is enabled, add missing access control…
added 1 commit
- 2668a690 - Only authorize pages if access control is enabled, add missing access control…
added 1 commit
- b0a9d707 - Only authorize pages if access control is enabled, add missing access control…
added 1 commit
- e91d2e2c - Only authorize pages if access control is enabled, add missing access control…
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-Vannesluomaadded 1 commit
- b07d6283 - Only authorize pages if access control is enabled, add missing access control…
OK, this looks convincing to me, but I'm not clear on how to get a package out of this for testing! WDYT @ibaum ?
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- Resolved by Ian Baum
- Resolved by Ian Baum
I was able to test this on a dev instance by setting my
pages_external_url
, andgitlab_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 ofreconfigure
.Also, a couple of comments about the
authorize_with_gitlab
call. And documentation would be great as well.Edited by Stan Hu@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.mentioned in merge request gitlab-com/www-gitlab-com!15078 (merged)
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.
changed milestone to %11.5
added 1 commit
- 406f11e6 - Add guards to skip authorize if defined, add tests, try to use lazy evaluation
@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? :(
@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 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. 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
DangerEdited by 🤖 GitLab Bot 🤖Ok, I figured out the problem: the
authorize_with_gitlab
call changesGitLab
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 rungitlab-ctl reconfigure
.Edited by Stan Hu@stanhu good catch. I'll commit that change and remove the
lazy
stuff from therunit_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-Vannesluomaadded 1 commit
- 555e1a29 - Add re-populate configuration options in pages
- Resolved by DJ Mountney
- Resolved by Ian Baum
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
Thanks for confirmation @marin
added 1 commit
- 56e43475 - Update the warning message when app id and secret query fails
- Resolved by Ian Baum
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
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 :/
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
assigned to @twk3
added release post item label
mentioned in commit 7b1a22a1
Awesome, thanks @Turmio
mentioned in merge request gitlab-com/www-gitlab-com!15765 (merged)
added workflowstaging-ref label
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
added workflowproduction label and removed workflowstaging-ref label