Bug 1888958: Store secrets in one place and utilize mutex #765

jcantrill · 2020-10-29T20:35:28Z

Description

This PR updates the certificate generation code to:

Add info message why certs are being (re)generated
Adds a mutex to the cert code to ensure changes are made in a single threaded fashion
Moves stashing of generated certs to the "master-certs" secret

Links

https://bugzilla.redhat.com/show_bug.cgi?id=1888958

openshift-ci-robot · 2020-10-29T20:35:35Z

@jcantrill: This pull request references Bugzilla bug 1888958, which is valid. The bug has been moved to the POST state. The bug has been updated to refer to the pull request using the external bug tracker.

3 validation(s) were run on this bug

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

In response to this:

Bug 1888958: Store secrets in one place and utilize mutex

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.

jcantrill · 2020-10-29T20:36:46Z

/hold

scripts/cert_generation.sh

pkg/k8shandler/certificates.go

syedriko · 2020-10-29T21:28:13Z

pkg/k8shandler/certificates.go

 		return
 	}

 	scriptsDir := utils.GetScriptsDir()
-	if err = GenerateCertificates(clusterRequest.Cluster.Namespace, scriptsDir, "elasticsearch", utils.DefaultWorkingDir); err != nil {
+	if err = GenerateCertificates(clusterRequest.Cluster.Namespace, scriptsDir, "elasticsearch", utils.GetWorkingDir()); err != nil {


If the script could indicate by its return value that it didn't change anything, we could skip writing the secret and cut down on API server traffic quite a bit.

updated as we discussed

syedriko · 2020-10-29T21:37:43Z

pkg/k8shandler/certificates.go

-func (clusterRequest *ClusterLoggingRequest) extractSecretToFile(secretName string, key string, toFile string) (err error) {
-	secret, err := clusterRequest.GetSecret(secretName)
+func (clusterRequest *ClusterLoggingRequest) extractMasterCerts() (err error) {
+	secret, err := clusterRequest.GetSecret(constants.MasterCASecretName)


When the new code runs for the first time after an upgrade, it'll dump files "masterca" and "masterkey" on disk and keep them circulating. Just might account for that and skip them altogether.

pkg/k8shandler/certificates.go

syedriko · 2020-10-29T21:49:32Z

pkg/k8shandler/certificates.go

-	// Pull master signing cert out from secret in logging.Spec.SecretName
-	if err = clusterRequest.readSecrets(); err != nil {
+	mutex.Lock()
+	defer mutex.Unlock()


Checking for 0-length files while populating per-component secrets was one of the bullet points, too.

i believe the changes to the script resolve this

syedriko · 2020-10-30T01:43:35Z

@jcantrill Added error checking to reading of ES certs:
https://github.com/syedriko/cluster-logging-operator/blob/bz_1888958/pkg/k8shandler/logstore.go#L155

syedriko · 2020-10-30T15:53:17Z

pkg/k8shandler/certificates.go

-		}
+	results := map[string][]byte{}
+	for _, f := range files {
+		results[f.Name()] = utils.GetFileContents(path.Join(workDir, f.Name()))


We could use a check for GetFileContents() failing here.

The called function logs an error and returns nil. Adding check for nil and log message. Skipping adding the file if the content is nil

pkg/k8shandler/curation.go

syedriko · 2020-10-30T16:22:32Z

pkg/k8shandler/curation.go

-		map[string][]byte{
+	var secrets = map[string][]byte{}
+	Syncronize(func() error {
+		secrets = map[string][]byte{


We're not checking for possible errors from GetWorkingDirFileContents()
I took a stab at adding that in https://github.com/syedriko/cluster-logging-operator/blob/bz_1888958/pkg/k8shandler/logstore.go#L155

I think this is OK not to check missing here. We need a bunch of secrets and we should fail and reconcile again. The cert gen code should regen if something is missing. If it does not that is a bug in the regen code IMO

But if we don't check we don't fail and stick empty buffers into secrets.

What is the action we can take in this situation? What do we believe will change during a subsequent reconciliation that would resolve the issue? The best we can do is really generate a log message and/or update status. This particular file is probably moot as we want to remove curation from CLO but if we are to do anything then it should be applied to the other places where we take similar action

We can abort, refuse to update the secret and to potentially make our situation worse. GetWorkingDirFileContents() is not very nuanced in how it reports errors, but there can be a genuine I/O failure that will make GetWorkingDirFileContents() return nil while there's a good cert in the secret as well as on disk. By proceeding we make things worse by wiping out a good cert from the secret. We will recover in time, but this is a disruption we can avoid.

jcantrill · 2020-10-30T16:33:29Z

/test unit

jcantrill · 2020-11-03T18:16:11Z

/test unit

jcantrill · 2020-11-03T18:25:02Z

/refresh

openshift-ci-robot · 2020-11-03T18:25:08Z

@jcantrill: The following test failed, say /retest to rerun all failed tests:

Test name	Commit	Details	Rerun command
ci/prow/unit	f4f34e14294d97a58a0d94b02a38e47755c85d46	link	`/test unit`

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.

jcantrill · 2020-11-03T22:00:04Z

/hold cancel

syedriko · 2020-11-03T22:52:46Z

scripts/cert_generation.sh

+
+if [ ! -s "${WORKING_DIR}/kibana-session-secret" ] ; then
+  info "Generating kibana session secret"
+  result=$(python -c "import string,random;print(''.join(random.choice(string.ascii_letters + string.digits) for i in range(32)))")


This achieves almost the same end, but with a shorter alphabet of letters, without the Python dependency

[root@e33867c18033 cluster-logging-operator]# dd if=/dev/urandom count=1 ibs=16 status=none | hexdump -e '"%02X"' 4306BCE91BC0C99B7AB482ECC3E73047

updated to remove python dep

syedriko · 2020-11-04T03:07:48Z

/test e2e-operator

jcantrill · 2020-11-04T15:07:56Z

/retest

jcantrill · 2020-11-05T15:53:01Z

/retest

syedriko · 2020-11-07T13:46:48Z

/lgtm

openshift-ci-robot · 2020-11-07T13:47:11Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: jcantrill, syedriko

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

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

openshift-ci-robot · 2020-11-07T13:49:44Z

@jcantrill: All pull requests linked via external trackers have merged:

openshift/cluster-logging-operator#765

Bugzilla bug 1888958 has been moved to the MODIFIED state.

In response to this:

Bug 1888958: Store secrets in one place and utilize mutex

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.

jcantrill · 2020-11-07T13:51:45Z

/cherrypick release-4.6

openshift-cherrypick-robot · 2020-11-07T13:51:51Z

@jcantrill: #765 failed to apply on top of branch "release-4.6":

Applying: Bug 1888958: Store secrets in one place and utilize mutex
Using index info to reconstruct a base tree...
M	Dockerfile
M	Makefile
A	pkg/certificates/certificates.go
M	pkg/k8shandler/certificates.go
M	pkg/k8shandler/curation.go
M	pkg/k8shandler/fluentd.go
M	pkg/k8shandler/logstore.go
M	pkg/k8shandler/visualization.go
M	pkg/utils/utils.go
A	test/functional/framework.go
M	test/helpers/framework.go
Falling back to patching base and 3-way merge...
Auto-merging test/helpers/framework.go
CONFLICT (content): Merge conflict in test/helpers/framework.go
CONFLICT (modify/delete): test/functional/framework.go deleted in HEAD and modified in Bug 1888958: Store secrets in one place and utilize mutex. Version Bug 1888958: Store secrets in one place and utilize mutex of test/functional/framework.go left in tree.
Auto-merging pkg/utils/utils.go
Auto-merging pkg/k8shandler/visualization.go
Auto-merging pkg/k8shandler/logstore.go
Auto-merging pkg/k8shandler/fluentd.go
Auto-merging pkg/k8shandler/curation.go
Auto-merging pkg/k8shandler/certificates.go
CONFLICT (content): Merge conflict in pkg/k8shandler/certificates.go
CONFLICT (modify/delete): pkg/certificates/certificates.go deleted in HEAD and modified in Bug 1888958: Store secrets in one place and utilize mutex. Version Bug 1888958: Store secrets in one place and utilize mutex of pkg/certificates/certificates.go left in tree.
Auto-merging Makefile
CONFLICT (content): Merge conflict in Makefile
Auto-merging Dockerfile
error: Failed to merge in the changes.
hint: Use 'git am --show-current-patch=diff' to see the failed patch
Patch failed at 0001 Bug 1888958: Store secrets in one place and utilize mutex
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:

/cherrypick release-4.6

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.

Bug 1888958: Store secrets in one place and utilize mutex

openshift-ci-robot assigned alanconway and syedriko Oct 29, 2020

openshift-ci-robot added bugzilla/severity-high Referenced Bugzilla bug's severity is high for the branch this PR is targeting. bugzilla/valid-bug Indicates that a referenced Bugzilla bug is valid for the branch this PR is targeting. labels Oct 29, 2020

openshift-ci-robot requested review from ewolinetz and syedriko October 29, 2020 20:35

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

openshift-ci-robot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Oct 29, 2020

syedriko reviewed Oct 29, 2020

View reviewed changes

scripts/cert_generation.sh Outdated Show resolved Hide resolved

syedriko reviewed Oct 29, 2020

View reviewed changes

scripts/cert_generation.sh Outdated Show resolved Hide resolved

syedriko reviewed Oct 29, 2020

View reviewed changes

pkg/k8shandler/certificates.go Outdated Show resolved Hide resolved

syedriko reviewed Oct 29, 2020

View reviewed changes

pkg/k8shandler/certificates.go Outdated Show resolved Hide resolved

syedriko reviewed Oct 29, 2020

View reviewed changes

jcantrill force-pushed the 1888958 branch from d1fb99b to f835e1e Compare October 30, 2020 15:28

syedriko reviewed Oct 30, 2020

View reviewed changes

pkg/k8shandler/curation.go Outdated Show resolved Hide resolved

syedriko reviewed Oct 30, 2020

View reviewed changes

jcantrill force-pushed the 1888958 branch 2 times, most recently from c2cbd29 to 1ac04cd Compare October 30, 2020 17:43

jcantrill added the release/4.7 label Oct 30, 2020

jcantrill force-pushed the 1888958 branch 2 times, most recently from 507e12a to f4f34e1 Compare November 3, 2020 18:00

jcantrill force-pushed the 1888958 branch 2 times, most recently from de17373 to 1420be7 Compare November 3, 2020 21:59

openshift-ci-robot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Nov 3, 2020

syedriko reviewed Nov 3, 2020

View reviewed changes

jcantrill force-pushed the 1888958 branch from 1420be7 to b3e905c Compare November 4, 2020 12:04

jcantrill force-pushed the 1888958 branch 3 times, most recently from 5127e56 to 6214207 Compare November 5, 2020 18:42

Bug 1888958: Store secrets in one place and utilize mutex

02e1cea

jcantrill force-pushed the 1888958 branch from 6214207 to 02e1cea Compare November 6, 2020 19:13

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

openshift-merge-robot merged commit 3af50bd into openshift:master Nov 7, 2020

jcantrill deleted the 1888958 branch November 7, 2020 13:51

jcantrill mentioned this pull request Nov 9, 2020

Bug 1895607: Store secrets in one place and utilize mutex #775

Merged

pmoogi-redhat pushed a commit to pmoogi-redhat/cluster-logging-operator that referenced this pull request Apr 26, 2022

Merge pull request openshift#765 from jcantrill/1888958

64740bf

Bug 1888958: Store secrets in one place and utilize mutex

Bug 1888958: Store secrets in one place and utilize mutex #765

Bug 1888958: Store secrets in one place and utilize mutex #765

jcantrill commented Oct 29, 2020

openshift-ci-robot commented Oct 29, 2020

jcantrill commented Oct 29, 2020

syedriko Oct 29, 2020

jcantrill Oct 30, 2020

syedriko Oct 29, 2020

syedriko Oct 29, 2020

jcantrill Oct 30, 2020

syedriko commented Oct 30, 2020

syedriko Oct 30, 2020

jcantrill Nov 3, 2020

syedriko Oct 30, 2020 •

edited

jcantrill Nov 3, 2020

syedriko Nov 3, 2020

jcantrill Nov 3, 2020

syedriko Nov 3, 2020 •

edited

jcantrill commented Oct 30, 2020

jcantrill commented Nov 3, 2020

jcantrill commented Nov 3, 2020

openshift-ci-robot commented Nov 3, 2020

jcantrill commented Nov 3, 2020

syedriko Nov 3, 2020 •

edited

jcantrill Nov 4, 2020

syedriko commented Nov 4, 2020

jcantrill commented Nov 4, 2020

jcantrill commented Nov 5, 2020

syedriko commented Nov 7, 2020

openshift-ci-robot commented Nov 7, 2020

openshift-ci-robot commented Nov 7, 2020

jcantrill commented Nov 7, 2020

openshift-cherrypick-robot commented Nov 7, 2020

Bug 1888958: Store secrets in one place and utilize mutex #765

Bug 1888958: Store secrets in one place and utilize mutex #765

Conversation

jcantrill commented Oct 29, 2020

Description

Links

openshift-ci-robot commented Oct 29, 2020

jcantrill commented Oct 29, 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

syedriko commented Oct 30, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

syedriko Oct 30, 2020 • edited

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

syedriko Nov 3, 2020 • edited

Choose a reason for hiding this comment

jcantrill commented Oct 30, 2020

jcantrill commented Nov 3, 2020

jcantrill commented Nov 3, 2020

openshift-ci-robot commented Nov 3, 2020

jcantrill commented Nov 3, 2020

syedriko Nov 3, 2020 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

syedriko commented Nov 4, 2020

jcantrill commented Nov 4, 2020

jcantrill commented Nov 5, 2020

syedriko commented Nov 7, 2020

openshift-ci-robot commented Nov 7, 2020

openshift-ci-robot commented Nov 7, 2020

jcantrill commented Nov 7, 2020

openshift-cherrypick-robot commented Nov 7, 2020

syedriko Oct 30, 2020 •

edited

syedriko Nov 3, 2020 •

edited

syedriko Nov 3, 2020 •

edited