Refactor registry-console setup and add support for SSL #4256
Conversation
|
Can one of the admins verify this patch?
|
1 similar comment
|
Can one of the admins verify this patch?
|
dmsimard
force-pushed
the
registry_console_refactor
branch
from
6003d3f to
1fbc8cf
Compare
|
Note the usage of "openshift_hosted" prefix for variables instead of "cockpit_ui". I was on the verge of "merging" the cockpit and cockpit-ui roles into the openshift_hosted role but I decided against it because I am not knowledgeable enough of the role composition to understand what are the implications. Would like opinions on that. |
|
I'd previously almost renamed things |
|
We prefix variable names with the name of the role they are used in although there are older roles where this rule isn't followed. I think we would ultimately want to rename the |
|
@abutcher @sdodson so at least it looks like we agree that cockpit/cockpit-ui could/should be merged together and renamed to something more appropriate ? I don't know enough about cockpit/cockpit-ui (are they used for anything else than the registry console? It looks like it...) That's why I didn't move it. I guess it would also be a backwards-incompatible change (changing role name, changing var names) and I don't know how okay this is. |
roles/cockpit-ui/tasks/main.yml
Outdated
| - name: Generate certificate bundle | ||
| copy: | ||
| content: "{{ certificate_files.results | map(attribute='content') | map('b64decode') | join('') }}" | ||
| dest: "{{ openshift_master_config_dir }}/named_certificates/registry-console.pem" |
There was a problem hiding this comment.
Do we need to retain the created/provided certificate within /etc/origin/master/named_certificates? We might want to just plop it in a temporary directory long enough to create the secret and then remove the temporary directory once we're done with it.
There was a problem hiding this comment.
Not a bad idea to put it somewhere temporary. That said, I guess we don't need to keep any certificate around once they have been configured either directly on the route (passthrough) or as a secret (mounted as a volume).
That said, I guess you sort of lose some idempotency since you'll need to upload certs in tmpdir/generate bundle/check against what is configured/delete tmpdir.
The fact that the bundle is stored means the task should not be "changed" because the source files are already there and the bundle ends up the same.
There was a problem hiding this comment.
Makes sense to keep the files for idempotency. /etc/origin/master/named_certificates isn't a good folder due to the overwrite var so sticking with convention and using /etc/origin/master would be fine for files like /etc/origin/master/registry-console.*.
I'd be worried about cluttering /etc/origin/master storing files that could have names defined by the user - also potential file name conflicts like ca.crt overwriting the OpenShift CA. What if we add a new directory for registry-console and store everything related there? @sdodson Thoughts on that?
There was a problem hiding this comment.
Ack. Note that I've taken the same approach in the docker-registry refactor here #4254 so whatever solution we agree on would likely apply to both.
There was a problem hiding this comment.
another directory is fine with me
roles/cockpit-ui/tasks/main.yml
Outdated
| namespace: "{{ openshift_hosted_registry_console_namespace }}" | ||
| files: | ||
| - name: registry-console.cert | ||
| path: "{{ openshift_master_config_dir }}/named_certificates/registry-console.pem" |
There was a problem hiding this comment.
Will we create a new certificate every run and modify the secret in this task? If so, wonder if we can detect that a certificate secret is already populated (or certificate exists) and skip these steps.
There was a problem hiding this comment.
There shouldn't be a new certificate every run, no, given that oc_adm_ca_server_cert and oc_secret are idempotent (from my understanding).
| # is optional | ||
| - name: Configure provided ca certificate file path | ||
| set_fact: | ||
| docker_registry_console_cacert_path: "{{ openshift_master_config_dir }}/named_certificates/{{ openshift_hosted_registry_console_routecertificates['cafile'] | basename }}" |
There was a problem hiding this comment.
Same question re: keeping these in the named_certificates directory. For more context, we currently only keep SNI certificates specified via openshift_master_named_certificates in this directory and there are other tasks which will clean this dir out based on setting openshift_master_overwrite_named_certificates=true so I'd be worried about losing them if we want to keep them but not sure that we want to keep them.
There was a problem hiding this comment.
Unless mistaken, at least in the case of the registry generated certificate (prior to my pull request), the certificate was stored in /etc/origin/master, not in /etc/origin/master/named_certificates. I moved it to named_certificates but we can keep it somewhere else (if we want to keep it)
|
@dmsimard I'll dig into the renaming and see if I can list everything we'd need to account for. |
|
Looks like we only call into the current Note: I've left the |
|
@abutcher ok, I'll include abutcher@68fe744 in my next rebase/commit, thanks. |
dmsimard
force-pushed
the
registry_console_refactor
branch
from
1fbc8cf to
0f18e24
Compare
|
Nothing in particular yet, just rebasing this on top of current master, it was getting stale. I'll try to take a new look at this in the near future. |
|
Can one of the admins verify this patch?
|
1 similar comment
|
Can one of the admins verify this patch?
|
openshift-merge-robot
added
the
needs-rebase
|
Hi @dmsimard, great job. It would be really helpful to document this in the openshift-docs repo: Also this #4254 Im gonna open two issue there to keep track of both, just FYI. |
|
Can one of the admins verify this patch?
|
dmsimard
force-pushed
the
registry_console_refactor
branch
from
0f18e24 to
06ee306
Compare
openshift-merge-robot
removed
the
needs-rebase
openshift-ci-robot
added
the
size/L
|
Hi @jtudelag, thanks for reminding me I still needed to land this... I just rebased on top of current master and addressed @sdodson and @abutcher's comments about moving certificates from /etc/origin/master/named_certificates to another directory, /etc/origin/master/registry_console_certificates. I have very little available bandwidth and asking me to write docs will likely leave you waiting for a long time -- if anyone else wants to pick that up, that's fine with me. |
This renames the role 'cockpit-ui' to 'openshift_hosted_registry_console' to bring it in line with the other roles related to openshift_hosted. This is a refactor to significantly improve the configurability of the registry console service and route in order to bring it in line with the way we configure docker-registry. Effectively, this adds the ability to select a hostname for the registry console and adds support for securing the registry-console route with a provided SSL certificate either with passthrough or reencrypt termination. We maintain backwards compatibility by keeping the same default which provides a default registry-console hostname and self-signed certificates on a passthrough route.
dmsimard
force-pushed
the
registry_console_refactor
branch
from
06ee306 to
cea1cd4
Compare
|
This now includes the rename from 'cockpit-ui' to 'openshift_hosted_registry_console' as per discussed in this PR. |
|
@dmsimard no worries, I just created the issue in openshift-docs in order not to forget the documentation ;). |
|
/ok-to-test |
|
@dmsimard PR needs rebase |
openshift-merge-robot
added
the
needs-rebase
|
Can one of the admins verify this patch?
|
1 similar comment
|
Can one of the admins verify this patch?
|
|
@dmsimard Are you planning on rebasing this at some point? These roles have changed dramatically since last update. |
|
@abutcher @michaelgugino I got the pull request rebase fatigue.. I sent this more than a year ago now and we're still using it for our deployments. I can rebase once more if we're still interested in this. |
|
@dmsimard Thanks for the update. I don't think we'll be merging this any time soon. We're in the middle of a bug-fix sprint for the 3.10 release, so this wouldn't land until earliest 3.11. |
|
@michaelgugino I hope this could be picked up and pulled into OpenShift-Ansible soon. I'm pulling in a variant of this change for my fork of this to deploy Origin 3.9 with this for our use, and it'd be handy to have it in mainline. |
|
@dmsimard Could you please rebase this for the current master? |
|
@dmsimard Now that Origin 3.10 is out, can you please rebase this for current git master (which will become 3.11)? |
|
@dmsimard: The following tests failed, say
Full PR test history. Your PR dashboard. Please help us cut down on flakes by linking to an open issue when you hit one in your PR. Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. I understand the commands that are listed here. |
|
Master branch is closed! A major refactor is ongoing in devel-40. Changes for 3.x should be made directly to the latest release branch they're relevant to and backported from there. |
This is a refactor to significantly improve the configurability
of the registry console service and route in order to bring it
in line with the way we configure docker-registry.
Effectively, this adds the ability to select a hostname for the
registry console and adds support for securing the registry-console
route with a provided SSL certificate either with passthrough or
reencrypt termination.
We maintain backwards compatibility by keeping the same default
which provides a default registry-console hostname and self-signed
certificates on a passthrough route.
It uses the same approach as with the docker registry refactor in this
other PR: #4254