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

LOG-1076: Opt-in cert generation in EO #727

Merged

Conversation

ewolinetz
Copy link
Contributor

@ewolinetz ewolinetz commented Jun 11, 2021

Description

For EO 5.2, adding the ability to opt-in for cert generation and management within EO.
Implementation of enhancement proposal (linked below).
Will also require OAL change to be merged in first since DN Organization and OrganizationUnit were adjusted to read correctly as part of this code change.

To opt in, annotate the es CR with:

logging.openshift.io/elasticsearch-cert-management: "true"

To have EO provide a tls cert/key ca-bundle for your component also annotate with:

logging.openshift.io/elasticsearch-cert.fluentd: "system.logging.fluentd"

or

logging.openshift.io/elasticsearch-cert.jaeger: "user.jaeger"

/cc @jcantrill @periklis @jpkrohling

Links

@openshift-ci openshift-ci bot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Jun 11, 2021
@ewolinetz
Copy link
Contributor Author

/hold
for reviews

@openshift-ci openshift-ci bot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Jun 11, 2021
@ewolinetz ewolinetz force-pushed the es_cert_management branch 2 times, most recently from c372cb4 to 1acca09 Compare June 11, 2021 21:08
hack/cr.yaml Show resolved Hide resolved
internal/k8shandler/cluster.go Outdated Show resolved Hide resolved
internal/k8shandler/configuration_tmpl.go Show resolved Hide resolved
internal/k8shandler/reconciler.go Outdated Show resolved Hide resolved
@ewolinetz ewolinetz force-pushed the es_cert_management branch 4 times, most recently from 78a31d2 to 9c6951b Compare June 14, 2021 19:53
@ewolinetz
Copy link
Contributor Author

/hold cancel

@openshift-ci openshift-ci bot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Jun 14, 2021
@ewolinetz
Copy link
Contributor Author

/hold
to update e2e tests to use EO cert generation

@openshift-ci openshift-ci bot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Jun 14, 2021
@ewolinetz
Copy link
Contributor Author

/test e2e-operator

@ewolinetz
Copy link
Contributor Author

/retest

2 similar comments
@ewolinetz
Copy link
Contributor Author

/retest

@ewolinetz
Copy link
Contributor Author

/retest

@ewolinetz
Copy link
Contributor Author

/hold
to fix tests

@ewolinetz ewolinetz force-pushed the es_cert_management branch 2 times, most recently from 0d61229 to 8ab9d2c Compare June 17, 2021 16:53
@ewolinetz
Copy link
Contributor Author

/hold cancel

@openshift-ci openshift-ci bot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Jun 17, 2021
@ewolinetz
Copy link
Contributor Author

/retest

Copy link
Contributor

@periklis periklis left a comment

Choose a reason for hiding this comment

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

Looks really awesome. Will give it an LGTM now. We can fix nits on a follow.

Comment on lines +748 to +756
if certOK {
decodedCert, err := pemDecodeCert(certBytes)
if err != nil {
return nil, err
}
x509Cert = decodedCert
} else {
return nil, fmt.Errorf("missing cert key from secret")
}
Copy link
Contributor

Choose a reason for hiding this comment

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

nit. Suggest to skip one level of indentation like:

if !certOK {
  return nil, fmt.Errorf("missing cert key from secret")
}
decodedCert, err := pemDecodeCert(certBytes)
if err != nil {
   return nil, err
}
x509Cert = decodedCert

Same applies below for keyOk and SerialOk.

@periklis
Copy link
Contributor

/lgtm

@openshift-ci openshift-ci bot added the lgtm Indicates that a PR is ready to be merged. label Jun 18, 2021
@openshift-ci
Copy link
Contributor

openshift-ci bot commented Jun 18, 2021

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: ewolinetz, periklis

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-merge-robot openshift-merge-robot merged commit c06152f into openshift:master Jun 18, 2021
@jpkrohling
Copy link

cc @rubenvp8510

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.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants