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

Support CA certificate provided by user #448

Merged
merged 1 commit into from Jan 21, 2020

Conversation

dulek
Copy link
Contributor

@dulek dulek commented Jan 16, 2020

In order to allow users to use self-signed certificates when
authenticating into OpenStack cloud, we need to make sure CA certificate
provided to the installer by the user is actually used by CNO and Kuryr.

In case of CNO this commit makes sure to configure gophercloud's HTTP
client to use the user CA when provided.

In case of Kuryr the CA certificate is injected into kuryr-controller's
/etc/ssl/certs.

@openshift-ci-robot openshift-ci-robot added do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Jan 16, 2020
@MaysaMacedo
Copy link
Contributor

Looks good. Just a question, should we mount the configmap even if the cert is not provided?

@openshift-ci-robot openshift-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jan 17, 2020
@openshift-ci-robot openshift-ci-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jan 17, 2020
@dulek
Copy link
Contributor Author

dulek commented Jan 17, 2020

Looks good. Just a question, should we mount the configmap even if the cert is not provided?

Yup, I was actually forced to make it not mount it in that case, so this is handled now.

@dulek dulek changed the title WiP: Support CA certificate provided by user Support CA certificate provided by user Jan 17, 2020
@openshift-ci-robot openshift-ci-robot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Jan 17, 2020
@dulek
Copy link
Contributor Author

dulek commented Jan 17, 2020

/retest

@gryf
Copy link
Member

gryf commented Jan 17, 2020

/lgtm

@openshift-ci-robot openshift-ci-robot added the lgtm Indicates that a PR is ready to be merged. label Jan 17, 2020
@MaysaMacedo
Copy link
Contributor

/lgtm

@dulek
Copy link
Contributor Author

dulek commented Jan 17, 2020

/retest


// We need to fetch user-provided OpenStack cloud CA certificate and make gophercloud use it.
// Also it'll get injected into a ConfigMap mounted into kuryr-controller later on.
userCACert, err := getUserCACert(kubeClient)
Copy link
Contributor

Choose a reason for hiding this comment

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

do you need here : if err != nil?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Not necessarily, the idea is that when error happens this just proceeds as the cert was not set. And indeed that's the case as the ConfigMap will not exist when user had not set the certificates in install-config.yaml.

Copy link
Contributor

Choose a reason for hiding this comment

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

awesome! thanks!

# There's no good way to just "append" user-provided certs to system ones,
# so just configure openstacksdk to use it.
cafile = /etc/ssl/certs/user-ca-bundle.crt
{{ end }}
Copy link
Contributor

Choose a reason for hiding this comment

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

pro tip: if you put a - after the {{ on the ``{{ if ... }}and{{ end }}` lines, it will delete the extra blank lines you'd otherwise get

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks, I fixed all the places.

@openshift-ci-robot openshift-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Jan 17, 2020
@openshift-ci-robot openshift-ci-robot removed the lgtm Indicates that a PR is ready to be merged. label Jan 17, 2020
@dulek
Copy link
Contributor Author

dulek commented Jan 19, 2020

/retest

@dulek
Copy link
Contributor Author

dulek commented Jan 20, 2020

/hold

Seems like the name of the ConfgMap might change, I'll try updating this once I know the new one.

@openshift-ci-robot openshift-ci-robot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Jan 20, 2020
In order to allow users to use self-signed certificates when
authenticating into OpenStack cloud, we need to make sure CA certificate
provided to the installer by the user is actually used by CNO and Kuryr.

In case of CNO this commit makes sure to configure gophercloud's HTTP
client to use the user CA when provided.

In case of Kuryr the CA certificate is injected into kuryr-controller's
/etc/ssl/certs.
@luis5tb
Copy link
Contributor

luis5tb commented Jan 20, 2020

/lgtm

1 similar comment
@gryf
Copy link
Member

gryf commented Jan 20, 2020

/lgtm

@dulek
Copy link
Contributor Author

dulek commented Jan 20, 2020

/hold cancel

@openshift-ci-robot openshift-ci-robot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Jan 20, 2020
@openshift-ci-robot openshift-ci-robot added the lgtm Indicates that a PR is ready to be merged. label Jan 20, 2020
@openshift-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: danwinship, dulek, gryf, luis5tb, MaysaMacedo

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@openshift-bot
Copy link
Contributor

/retest

Please review the full test history for this PR and help us cut down flakes.

7 similar comments
@openshift-bot
Copy link
Contributor

/retest

Please review the full test history for this PR and help us cut down flakes.

@openshift-bot
Copy link
Contributor

/retest

Please review the full test history for this PR and help us cut down flakes.

@openshift-bot
Copy link
Contributor

/retest

Please review the full test history for this PR and help us cut down flakes.

@openshift-bot
Copy link
Contributor

/retest

Please review the full test history for this PR and help us cut down flakes.

@openshift-bot
Copy link
Contributor

/retest

Please review the full test history for this PR and help us cut down flakes.

@openshift-bot
Copy link
Contributor

/retest

Please review the full test history for this PR and help us cut down flakes.

@openshift-bot
Copy link
Contributor

/retest

Please review the full test history for this PR and help us cut down flakes.

@dulek
Copy link
Contributor Author

dulek commented Jan 21, 2020

/retest

@openshift-bot
Copy link
Contributor

/retest

Please review the full test history for this PR and help us cut down flakes.

6 similar comments
@openshift-bot
Copy link
Contributor

/retest

Please review the full test history for this PR and help us cut down flakes.

@openshift-bot
Copy link
Contributor

/retest

Please review the full test history for this PR and help us cut down flakes.

@openshift-bot
Copy link
Contributor

/retest

Please review the full test history for this PR and help us cut down flakes.

@openshift-bot
Copy link
Contributor

/retest

Please review the full test history for this PR and help us cut down flakes.

@openshift-bot
Copy link
Contributor

/retest

Please review the full test history for this PR and help us cut down flakes.

@openshift-bot
Copy link
Contributor

/retest

Please review the full test history for this PR and help us cut down flakes.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. lgtm Indicates that a PR is ready to be merged. size/M Denotes a PR that changes 30-99 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

8 participants