Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Router wildcard certificate created by default #4693

Merged

Conversation

enoodle
Copy link
Contributor

@enoodle enoodle commented Jul 6, 2017

I suggest setting this parameter default to True. Since #3821 It seems more useful then using arbitrary hostnames.

cc @kwoodson @simon3z

@openshift-bot
Copy link

Can one of the admins verify this patch?
I understand the following commands:

  • bot, add author to whitelist
  • bot, test pull request
  • bot, test pull request once

1 similar comment
@openshift-bot
Copy link

Can one of the admins verify this patch?
I understand the following commands:

  • bot, add author to whitelist
  • bot, test pull request
  • bot, test pull request once

@enoodle
Copy link
Contributor Author

enoodle commented Jul 12, 2017

ping @kwoodson @jcantrill PTAL

@kwoodson
Copy link
Contributor

@mtnbikenc @abutcher, do you see any reason as to why we should or shouldn't do this?

@jcantrill
Copy link
Contributor

@enoodle I don't have an opinion on why we would or would not do this.

@abutcher
Copy link
Member

abutcher commented Jul 14, 2017

@kwoodson @enoodle My only worry would be that defaulting openshift_hosted_router_create_certificate=true would stomp a user provided openshift_hosted_router_certificate. This block looks like it would stomp

# This is for when we desire a cluster signed cert
# The certificate is generated and placed in master_config_dir/
- block:
- name: generate a default wildcard router certificate
oc_adm_ca_server_cert:
signer_cert: "{{ openshift_master_config_dir }}/ca.crt"
signer_key: "{{ openshift_master_config_dir }}/ca.key"
signer_serial: "{{ openshift_master_config_dir }}/ca.serial.txt"
hostnames:
- "{{ openshift_master_default_subdomain }}"
- "*.{{ openshift_master_default_subdomain }}"
cert: "{{ ('/etc/origin/master/' ~ (item.certificate.certfile | basename)) if 'certfile' in item.certificate else ((openshift_master_config_dir) ~ '/openshift-router.crt') }}"
key: "{{ ('/etc/origin/master/' ~ (item.certificate.keyfile | basename)) if 'keyfile' in item.certificate else ((openshift_master_config_dir) ~ '/openshift-router.key') }}"
with_items: "{{ openshift_hosted_routers }}"
- name: set the openshift_hosted_router_certificate
set_fact:
openshift_hosted_router_certificate:
certfile: "{{ openshift_master_config_dir ~ '/openshift-router.crt' }}"
keyfile: "{{ openshift_master_config_dir ~ '/openshift-router.key' }}"
cafile: "{{ openshift_master_config_dir ~ '/ca.crt' }}"
# End Block
when: openshift_hosted_router_create_certificate | bool
.

@kwoodson
Copy link
Contributor

@abutcher, @enoodle, That's my fear as we don't use the default router certs on our operations team. We supply real certificates and would get default ones which would break our installs unless specifying openshift_hosted_router_create_certificate=false.

I understand the desire to have a default be turned on since a lot of installs do not require real certificates. I guess this is just a matter of preference unless someone has a good argument to update it.

@abutcher
Copy link
Member

We could probably default this if that linked section didn't create the default wildcard certificate when openshift_hosted_router_certificate was provided.

@enoodle enoodle force-pushed the create_router_certificate_by_default branch from 0d69bff to c1e654b Compare July 18, 2017 09:14
@enoodle
Copy link
Contributor Author

enoodle commented Jul 18, 2017

@abutcher I have added a condition on openshift_hosted_router_certificate being empty, PTAL

@enoodle
Copy link
Contributor Author

enoodle commented Jul 25, 2017

@abutcher @kwoodson I have added a condition on openshift_hosted_router_certificate being empty, Can you take another look?

@abutcher
Copy link
Member

@enoodle The conditional looks great thanks.

I did not notice that the wildcard cert will contain SANs for *.openshift_master_default_subdomain and openshift_master_default_subdomain without any defaults, so base install is "", "*.". If this is always on I think we should have a default for this like *.router.default.svc.cluster.local. @sdodson @kwoodson sound okay?

@kwoodson
Copy link
Contributor

@enoodle, sorry for the slow response. We've been locked down working on 3.6.

@abutcher, That sounds fine.

@enoodle
Copy link
Contributor Author

enoodle commented Jul 25, 2017

@kwoodson @abutcher I have added a default value for the certificate paths. Can you take another look?

@enoodle enoodle force-pushed the create_router_certificate_by_default branch from 1092eed to be59490 Compare July 25, 2017 16:56
Copy link
Member

@abutcher abutcher left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lgtm

@abutcher
Copy link
Member

aos-ci-test

@openshift-bot
Copy link

success: "aos-ci-jenkins/OS_3.6_NOT_containerized, aos-ci-jenkins/OS_3.6_NOT_containerized_e2e_tests" for be59490 (logs)

@openshift-bot
Copy link

success: "aos-ci-jenkins/OS_3.6_containerized, aos-ci-jenkins/OS_3.6_containerized_e2e_tests" for be59490 (logs)

@sdodson
Copy link
Member

sdodson commented Jul 25, 2017

[merge]

@openshift-bot
Copy link

[test]ing while waiting on the merge queue

@openshift-bot
Copy link

Evaluated for openshift ansible test up to be59490

@openshift-bot
Copy link

continuous-integration/openshift-jenkins/test FAILURE (https://ci.openshift.redhat.com/jenkins/job/test_pull_request_openshift_ansible/359/) (Base Commit: 14fbd4a) (PR Branch Commit: be59490)

@enoodle
Copy link
Contributor Author

enoodle commented Jul 26, 2017

"Error from server (NotFound): namespaces "logging" not found"
"/go/src/github.com/openshift/origin/_output/local/go/src/github.com/openshift/origin/vendor/k8s.io/kubernetes/test/e2e/scheduling/predicates.go:73
I0726 01:51:51.623994 17300 request.go:782] Error in request: resource name may not be empty"

  • I am not sure what jenkins is testing there exactly but it doesn't seem connected.

@abutcher
Copy link
Member

flake openshift/origin#15356

@abutcher
Copy link
Member

[merge]

@openshift-bot
Copy link

Evaluated for openshift ansible merge up to be59490

@openshift-bot
Copy link

continuous-integration/openshift-jenkins/merge FAILURE (https://ci.openshift.redhat.com/jenkins/job/merge_pull_request_openshift_ansible/744/) (Base Commit: b70e033) (PR Branch Commit: be59490)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants