[release-4.5] Bug 1835090: Collect certificates #70

martinkunc · 2020-01-27T09:30:04Z

The task should add collection of Certificate features, which could be used to verify their validity in further stages.

openshift-ci-robot · 2020-01-27T09:30:19Z

Hi @martinkunc. Thanks for your PR.

I'm waiting for a openshift member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

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.

tisnik · 2020-01-27T10:09:16Z

pkg/gather/clusterconfig/clusterconfig.go

+		func() ([]record.Record, []error) {
+			requests, err := i.certClient.CertificateSigningRequests().List(metav1.ListOptions{})
+			if errors.IsNotFound(err) {
+				return nil, nil


I know this is used in previous gathering functions, but is the nil handled properly later?

Fortunately returning nil in a slice will return empty slice and wont fail. It looks even empty Record wont fail as its just a structure. I have just verified both.

tisnik · 2020-01-27T10:10:08Z

pkg/gather/clusterconfig/csr.go

+	*certificatesv1b1api.CertificateSigningRequest
+}
+
+func (a CSRAnonymizer) Marshal(_ context.Context) ([]byte, error) {


nitpick - all public methods should have proper docstring

these comments are still pending, would be nice to add some docs in the whole file.

tisnik · 2020-01-27T10:10:22Z

pkg/gather/clusterconfig/csr.go

+	"k8s.io/apimachinery/pkg/util/json"
+)
+
+type CSRAnonymizer struct {


nitpick - all public data types should have proper docstring

tisnik · 2020-01-27T10:11:23Z

pkg/gather/clusterconfig/csr.go

+	}
+	csr, err := x509.ParseCertificateRequest(block.Bytes)
+	if err != nil {
+		return


don't we need to log these issues?

tisnik

In overall it is a very nice work!

tisnik · 2020-01-27T11:30:28Z

pkg/gather/clusterconfig/csr.go

+	c.Spec.Request.PublicKeyAlgorithm = csr.PublicKeyAlgorithm.String()
+	c.Spec.Request.DNSNames = utils.Map(csr.DNSNames, anonymizeURL)
+	c.Spec.Request.EmailAddresses = utils.Map(csr.EmailAddresses, anonymizeURL)
+	ipsl := []string{}


if we know len(csr.IPAddresses), you can allocate an array, but it's up to you

tisnik · 2020-01-27T11:31:43Z

pkg/utils/strings.go

+package utils
+
+// Map applies each of functions to passed slice
+func Map(it []string, fn func(string) string) []string {


in the future, we might create a common utils package outside operator

in the future, Go may add map-reduce functions, but there already libraries like this available.

tisnik

LGTM, but I think that golinter will fail on current sources ;/

iNecas · 2020-01-31T15:55:02Z

/ok-to-test

tisnik · 2020-01-31T15:58:36Z

/ok-to-test

martinkunc · 2020-02-03T07:35:49Z

/retest

martinkunc · 2020-02-03T12:40:05Z

/retest

tisnik · 2020-02-04T08:11:18Z

/lgtm

martinkunc · 2020-02-04T16:24:11Z

/assign iNecas

iNecas · 2020-02-05T14:42:53Z

Great job on adding tests as well.

I'm a bit concerned about the possible amounts of the records, so I would suggest filtering out Approved CSRs, as they would probably not be very valuable for troubleshooting.

@smarterclayton any suggestion for somebody close to CSRs to give us some hinds on what kinds of CSRs types are useful for troubleshooting?

onmete · 2020-02-05T15:01:55Z

Well, we don't need the actual certificate, we are just using that to extract expiration date. If the cert is expired or will be expired on week or so.

I'm totally fine if the data extraction will be part of the collection mechanism.

So basically we just need info if the cert is approved and expiration date.

If it is still too much data, we can move all logic to the operator and collect just unapproved, expired and those, which will expire in 7 (we can discuss this number) days.

martinkunc · 2020-02-05T16:49:04Z

I can either filter all, or just gather certain data from Approved. Do you have any idea about numbers here ?

One idea was that we might start thinking about maybe different profiles of small and bigger size.
While one could be really statistical, the other could be all-in-one for detailed investigation.

Which way would be the best for now ?

iNecas · 2020-02-05T20:09:40Z

My main question is, whether we care (have a use-case) for the approved ones, or we are interested into some specific cases?

onmete · 2020-02-06T08:11:18Z

As far as I know, we don't care about approved ones (at least from the Insights perspective).

smarterclayton · 2020-02-07T01:07:28Z

Can you describe in more detail why we are collecting this? What scenarios will this enable us to detect (or which bugs are we seeing in the field that this info would return)?

onmete · 2020-02-07T08:53:12Z

The unapproved or expired certificates were a root issue in some customer cases.
Hence it seems useful to collect this info in advance and possibly show it to the customer.

Are we missing something?

alexandrevicenzi · 2020-02-18T13:21:38Z

pkg/gather/clusterconfig/csr.go

+func IncludeCSR(c *CSRAnonymizedFeatures, opts ...FilterOptFunc) bool {
+	opt := &FilterOpt{time: time.Now()}
+	for _, o := range opts {
+		o(opt)


these variables names are not good, e, t, o, i, etc, it could be a little more descriptive. In this case it could be optFn.

martinkunc · 2020-02-18T15:00:50Z

/retest

tisnik · 2020-02-19T10:19:54Z

/lgtm

openshift-ci-robot · 2020-02-19T10:20:28Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: martinkunc, tisnik

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~~ [tisnik]

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

openshift-bot · 2020-02-19T12:35:05Z

/retest

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

openshift-bot · 2020-02-19T12:47:34Z

/retest

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

smarterclayton · 2020-03-13T13:05:16Z

pkg/utils/strings.go

@@ -0,0 +1,10 @@
+package utils
+


In general in go we do not create utility packages like this. This is a trivial method and would inline privately in a package. Utility packages cause dependency creep and attract unrelated use cases, which cause coupling.

Please move this to a private method at the package call site.

Thank you for the comment. It makes sense, I will remove this in some further PR.

martinkunc · 2020-05-13T05:39:46Z

/retitle [release-4.3] Bug 1835090: Collect certificates

openshift-ci-robot · 2020-05-13T05:40:03Z

@martinkunc: All pull requests linked via external trackers have merged: . Bugzilla bug 1835090 has been moved to the MODIFIED state.

In response to this:

[release-4.3] Bug 1835090: Collect certificates

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.

martinkunc · 2020-05-13T05:45:58Z

/retitle [release-4.5] Bug 1835090: Collect certificates

openshift-ci-robot · 2020-05-29T10:24:33Z

@martinkunc: Bugzilla bug 1835090 is in an unrecognized state (MODIFIED) and will not be moved to the MODIFIED state.

In response to this:

/bugzilla 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.

martinkunc · 2020-05-29T10:27:26Z

/bugzilla refresh

openshift-ci-robot · 2020-05-29T10:27:30Z

@martinkunc: All pull requests linked via external trackers have merged: . Bugzilla bug 1835090 has been moved to the MODIFIED state.

In response to this:

/bugzilla 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.

openshift-ci-robot · 2020-06-01T04:18:16Z

@martinkunc: Bugzilla bug 1835090 is in an unrecognized state (MODIFIED) and will not be moved to the MODIFIED state.

In response to this:

/bugzilla 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.

Collect certificates

7788e21

martinkunc requested a review from tisnik January 27, 2020 09:30

openshift-ci-robot added needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. labels Jan 27, 2020

openshift-ci-robot requested review from iNecas and mfojtik January 27, 2020 09:31

tisnik reviewed Jan 27, 2020

View reviewed changes

Incorporate comments + make for lint/vet

70e91e5

tisnik approved these changes Jan 27, 2020

View reviewed changes

openshift-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Jan 27, 2020

Add logging

6c10612

openshift-ci-robot added ok-to-test Indicates a non-member PR verified by an org member that is safe to test. and removed needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. labels Jan 31, 2020

openshift-ci-robot assigned tisnik Feb 4, 2020

openshift-ci-robot added the lgtm Indicates that a PR is ready to be merged. label Feb 4, 2020

openshift-ci-robot assigned iNecas Feb 4, 2020

Don't store approved certificates

8cb8576

openshift-ci-robot removed the lgtm Indicates that a PR is ready to be merged. label Feb 18, 2020

alexandrevicenzi reviewed Feb 18, 2020

View reviewed changes

openshift-ci-robot added the lgtm Indicates that a PR is ready to be merged. label Feb 19, 2020

openshift-merge-robot merged commit 392fb20 into openshift:master Feb 19, 2020

smarterclayton reviewed Mar 13, 2020

View reviewed changes

openshift-ci-robot changed the title ~~Collect certificates~~ [release-4.3] Bug 1835090: Collect certificates May 13, 2020

openshift-ci-robot changed the title ~~[release-4.3] Bug 1835090: Collect certificates~~ [release-4.5] Bug 1835090: Collect certificates May 13, 2020

martinkunc mentioned this pull request May 13, 2020

[release-4.3] Bug 1835094: Collect certificates #104

Merged

[release-4.5] Bug 1835090: Collect certificates #70

[release-4.5] Bug 1835090: Collect certificates #70

Conversation

martinkunc commented Jan 27, 2020 • edited

openshift-ci-robot commented Jan 27, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tisnik left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tisnik left a comment

Choose a reason for hiding this comment

iNecas commented Jan 31, 2020

tisnik commented Jan 31, 2020

martinkunc commented Feb 3, 2020

martinkunc commented Feb 3, 2020

tisnik commented Feb 4, 2020

martinkunc commented Feb 4, 2020

iNecas commented Feb 5, 2020

onmete commented Feb 5, 2020

martinkunc commented Feb 5, 2020

iNecas commented Feb 5, 2020

onmete commented Feb 6, 2020

smarterclayton commented Feb 7, 2020

onmete commented Feb 7, 2020

Choose a reason for hiding this comment

martinkunc commented Feb 18, 2020

tisnik commented Feb 19, 2020

openshift-ci-robot commented Feb 19, 2020

openshift-bot commented Feb 19, 2020

openshift-bot commented Feb 19, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

martinkunc commented May 13, 2020

openshift-ci-robot commented May 13, 2020

martinkunc commented May 13, 2020

openshift-ci-robot commented May 29, 2020

martinkunc commented May 29, 2020

openshift-ci-robot commented May 29, 2020

openshift-ci-robot commented Jun 1, 2020

martinkunc commented Jan 27, 2020 •

edited