OCPBUGS-1684: Optimize certificate generation #486

tmshort · 2023-04-27T13:30:53Z

Reduce certificate generation to once-a-day.
Optimize loading of the certificate.
Rename operations to correctly identify what they do.

openshift-ci-robot · 2023-04-27T13:30:58Z

@tmshort: This pull request references Jira Issue OCPBUGS-1684, which is invalid:

expected the bug to target the "4.14.0" version, but it targets "4.12.z" instead

Comment /jira refresh to re-evaluate validity if changes to the Jira bug are made, or edit the title of this pull request to link to a different bug.

The bug has been updated to refer to the pull request using the external bug tracker.

In response to this:

Reduce certificate generation to once-a-day.
Optimize loading of the certificate.
Rename operations to correctly identify what they do.

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.

tmshort · 2023-04-27T13:43:26Z

/jira refresh

openshift-ci-robot · 2023-04-27T13:43:35Z

@tmshort: This pull request references Jira Issue OCPBUGS-1684, which is valid. The bug has been moved to the POST state.

3 validation(s) were run on this bug

bug is open, matching expected state (open)
bug target version (4.14.0) matches configured target version for branch (4.14.0)
bug is in the state New, which is one of the valid states (NEW, ASSIGNED, POST)

No GitHub users were found matching the public email listed for the QA contact in Jira ([email protected]), skipping review request.

In response to this:

/jira refresh

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.

tmshort · 2023-04-27T17:39:06Z

/retest

tmshort · 2023-04-27T19:47:38Z

The first job took 17s, subsequent jobs are taking 3~4s:

NAMESPACE                              NAME                        COMPLETIONS   DURATION   AGE
openshift-operator-lifecycle-manager   collect-profiles-28043685   1/1           17s        38m
openshift-operator-lifecycle-manager   collect-profiles-28043700   1/1           4s         24m
openshift-operator-lifecycle-manager   collect-profiles-28043715   1/1           4s         9m37s
openshift-operator-lifecycle-manager   collect-profiles-28043730   1/1           3s         8m46s
openshift-operator-lifecycle-manager   collect-profiles-28043745   1/1           4s         114s

Assuming all nodes are roughly equivalent; this is a significant time savings.
EDIT: This is also the behavior for non-modified olm.

tmshort · 2023-04-27T20:30:36Z

/retest

Reduce certificate generation to once-a-day. Optimize loading of the certificate. Rename operations to correctly identify what they do. Signed-off-by: Todd Short <[email protected]>

Signed-off-by: Todd Short <[email protected]>

tmshort · 2023-05-02T14:55:00Z

/hold

tmshort · 2023-05-02T18:04:33Z

/retest

tmshort · 2023-05-02T20:24:48Z

/retest

tmshort · 2023-05-02T20:26:04Z

/hold cancel

openshift-ci · 2023-05-02T23:02:07Z

@tmshort: all tests passed!

Full PR test history. Your PR dashboard.

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.

m1kola · 2023-05-05T15:12:22Z

cmd/collect-profiles/main.go

+						InsecureSkipVerify: true,
+						Certificates:       []tls.Certificate{*tlsCert},


I know it was like that originally in getHttpClient, but this thing makes me nervous.

Do you know why we have to use InsecureSkipVerify? Can't we set RootCAs to a x509.CertPool containing correct cert so that our self singed cert can be verified?

TL;DR: I think it's a bit beyond the scope of this Bug/PR to fix the certificate verification.

I was just focusing on the certificate generation and not the underlying security policy.

The pod template in the CronJob does not mount the olm-operator-serving-cert secret, which should be the certificate (and key) of the server. So it does not have access. I don't know what the CA is for that certificate (or if it's self-signed - as I have not figured out where it's generated - I'm guessing some openshift/cert-mgr magic. @awgreene do you know?).

(Note that there are two pods from which profiling information is collected, so both secrets, the other being catalog-operator-serving-cert, would need to be referenced.)

The collect-profiles pod collects from two different pods. It would require changing the manifests to mount the secret(s), adding a CLI option to reference the secret(s), then code to reference. As the it might require multiple arguments, and that doesn't scale. If I could figure out where the certificate comes from, and if the two certificates use a common CA, that would ease the burden.

I will note that the olm and catalog pods do appear to validate the client, as they mount the pprof secret.

Follow up: the catalog-operator-serving-cert and olm-operator-serving-cert are both signed by CN=openshift-service-serving-signer@1683311309 (presumably the number at the end is unique per instance). The signing cert is included in the certificate chain.

This certificate (and key) is located in the signing-key secret in the openshift-service-ca namespace; so no access. The key being there is a bit dangerous.

Optimally, the CA cert (only) would be made available somewhere, and I'm not sure that's the case. There are no secrets in the default namespace.

Service CA certificates describes the behavior of the OpenShift CA, and how to use it. Rotation would have to be handled (OLM does this already), and the collect-profiles pod really ought to use the same certificate generation mechanism, but it's designed for services (servers) not clients, AFAICT (a fake service could be defined, though).

Definitely beyond the scope of this bugfix/PR.

I'm fine with this change for now as it addresses the bug defined in the ticket. We should probably consider if we should disable this job by default as I don't believe anyone has actually utilized the pprof data collected by the job since this feature was introduced.

TL;DR: I think it's a bit beyond the scope of this Bug/PR to fix the certificate verification.

Absolutely. I implied that when I said it was like that in getHttpClient, but I'm sorry that I did not make it more clear.

awgreene

Thanks for all the variable name change improvements.
/approve

m1kola

I think we should create a ticket to address InsecureSkipVerify. It has potential for man in the middle attack.

But I agree that it is not in the scope of this change.

/lgtm

openshift-ci-robot · 2023-05-09T14:58:15Z

@tmshort: Jira Issue OCPBUGS-1684: All pull requests linked via external trackers have merged:

openshift/operator-framework-olm#486

Jira Issue OCPBUGS-1684 has been moved to the MODIFIED state.

In response to this:

Reduce certificate generation to once-a-day.
Optimize loading of the certificate.
Rename operations to correctly identify what they do.

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.

openshift-ci · 2023-05-09T15:03:05Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: awgreene, m1kola, tmshort

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:

~~OWNERS~~ [awgreene]

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

tmshort · 2023-05-09T15:50:21Z

/cherry-pick 4.13

openshift-cherrypick-robot · 2023-05-09T15:52:08Z

@tmshort: cannot checkout 4.13: error checking out 4.13: exit status 1. output: error: pathspec '4.13' did not match any file(s) known to git

In response to this:

/cherry-pick 4.13

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.

tmshort · 2023-05-09T15:55:06Z

/cherry-pick release-4.13

openshift-cherrypick-robot · 2023-05-09T15:55:46Z

@tmshort: #486 failed to apply on top of branch "release-4.13":

Applying: OCPBUGS-1684: Optimize certificate generation
Applying: Make 'ci/prow/verify' happy
Using index info to reconstruct a base tree...
M	go.mod
M	go.sum
M	vendor/modules.txt
Falling back to patching base and 3-way merge...
Auto-merging vendor/modules.txt
CONFLICT (content): Merge conflict in vendor/modules.txt
Auto-merging go.sum
CONFLICT (content): Merge conflict in go.sum
Auto-merging go.mod
CONFLICT (content): Merge conflict in go.mod
error: Failed to merge in the changes.
hint: Use 'git am --show-current-patch=diff' to see the failed patch
Patch failed at 0002 Make 'ci/prow/verify' happy
When you have resolved this problem, run "git am --continue".
If you prefer to skip this patch, run "git am --skip" instead.
To restore the original branch and stop patching, run "git am --abort".

In response to this:

/cherry-pick release-4.13

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.

tmshort · 2023-05-09T19:07:08Z

/jira cherrypick OCPBUGS-1684

openshift-ci-robot · 2023-05-09T19:07:14Z

@tmshort: Jira Issue OCPBUGS-1684 has been cloned as Jira Issue OCPBUGS-13321. Will retitle bug to link to clone.
/retitle OCPBUGS-13321: OCPBUGS-1684: Optimize certificate generation

In response to this:

/jira cherrypick OCPBUGS-1684

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.

openshift-ci-robot · 2023-05-09T19:07:20Z

@tmshort: Jira Issue OCPBUGS-13321: All pull requests linked via external trackers have merged:

Jira Issue OCPBUGS-13321 has been moved to the MODIFIED state.

In response to this:

Reduce certificate generation to once-a-day.
Optimize loading of the certificate.
Rename operations to correctly identify what they do.

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.

openshift-ci-robot · 2023-05-09T19:17:42Z

@tmshort: Jira Issue OCPBUGS-1684 is in an unrecognized state (ON_QA) and will not be moved to the MODIFIED state.

In response to this:

Reduce certificate generation to once-a-day.
Optimize loading of the certificate.
Rename operations to correctly identify what they do.

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.

openshift-ci bot requested review from grokspawn and perdasilva April 27, 2023 13:36

tmshort force-pushed the OCPBUGS-1684m branch from c02960a to 4cc9fe8 Compare April 27, 2023 14:18

tmshort added 2 commits May 2, 2023 10:54

OCPBUGS-1684: Optimize certificate generation

72600d2

Reduce certificate generation to once-a-day. Optimize loading of the certificate. Rename operations to correctly identify what they do. Signed-off-by: Todd Short <[email protected]>

Make 'ci/prow/verify' happy

cbd2325

Signed-off-by: Todd Short <[email protected]>

tmshort force-pushed the OCPBUGS-1684m branch from 4cc9fe8 to cbd2325 Compare May 2, 2023 14:54

openshift-ci bot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label May 2, 2023

openshift-ci bot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label May 2, 2023

m1kola reviewed May 5, 2023

View reviewed changes

awgreene approved these changes May 9, 2023

View reviewed changes

openshift-ci bot assigned awgreene May 9, 2023

openshift-ci bot added lgtm Indicates that a PR is ready to be merged. approved Indicates a PR has been approved by an approver from all required OWNERS files. labels May 9, 2023

m1kola approved these changes May 9, 2023

View reviewed changes

openshift-ci bot assigned m1kola May 9, 2023

openshift-merge-robot merged commit 969aa60 into openshift:master May 9, 2023

tmshort deleted the OCPBUGS-1684m branch May 9, 2023 15:50

openshift-ci bot changed the title ~~OCPBUGS-1684: Optimize certificate generation~~ OCPBUGS-13321: OCPBUGS-1684: Optimize certificate generation May 9, 2023

tmshort changed the title ~~OCPBUGS-13321: OCPBUGS-1684: Optimize certificate generation~~ OCPBUGS-1684: Optimize certificate generation May 9, 2023

m1kola mentioned this pull request Oct 24, 2024

OCPBUGS-43581: Fix TLS handshake error #884

Merged

		InsecureSkipVerify: true,
		Certificates: []tls.Certificate{*tlsCert},

OCPBUGS-1684: Optimize certificate generation #486

OCPBUGS-1684: Optimize certificate generation #486

Uh oh!

Conversation

tmshort commented Apr 27, 2023

Uh oh!

openshift-ci-robot commented Apr 27, 2023

Uh oh!

tmshort commented Apr 27, 2023

Uh oh!

openshift-ci-robot commented Apr 27, 2023

Uh oh!

tmshort commented Apr 27, 2023

Uh oh!

tmshort commented Apr 27, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

tmshort commented Apr 27, 2023

Uh oh!

tmshort commented May 2, 2023

Uh oh!

tmshort commented May 2, 2023

Uh oh!

tmshort commented May 2, 2023

Uh oh!

tmshort commented May 2, 2023

Uh oh!

openshift-ci bot commented May 2, 2023

Uh oh!

m1kola May 5, 2023

Choose a reason for hiding this comment

Uh oh!

tmshort May 5, 2023

Choose a reason for hiding this comment

Uh oh!

tmshort May 5, 2023

Choose a reason for hiding this comment

Uh oh!

tmshort May 8, 2023

Choose a reason for hiding this comment

Uh oh!

awgreene May 9, 2023

Choose a reason for hiding this comment

Uh oh!

m1kola May 9, 2023

Choose a reason for hiding this comment

Uh oh!

awgreene left a comment

Choose a reason for hiding this comment

Uh oh!

m1kola left a comment

Choose a reason for hiding this comment

Uh oh!

openshift-ci-robot commented May 9, 2023

Uh oh!

openshift-ci bot commented May 9, 2023

Uh oh!

tmshort commented May 9, 2023

Uh oh!

openshift-cherrypick-robot commented May 9, 2023

Uh oh!

tmshort commented May 9, 2023

Uh oh!

openshift-cherrypick-robot commented May 9, 2023

Uh oh!

tmshort commented May 9, 2023

Uh oh!

openshift-ci-robot commented May 9, 2023

Uh oh!

openshift-ci-robot commented May 9, 2023

Uh oh!

openshift-ci-robot commented May 9, 2023

Uh oh!

Uh oh!

tmshort commented Apr 27, 2023 •

edited

Loading