Allow supplying MONGO_SERVER_URL via chains-config by concaf · Pull Request #1113 · tektoncd/chains

concaf · 2024-05-06T06:38:40Z

Changes

Currently, when using the Mongo docstore for docdb storage backend, the
only way to supply MONGO_SERVER_URL environment variable (which contains
the credentials to connect to MongoDB) is by adding an environment
variable to the Chains controller pod. It's a farily common practice to
update the MONGO_SERVER_URL at regular intervals when the credentials
are rotated.

To facilitate this, this commit adds 2 fields to Chains' configuration:
1. storage.docdb.mongo-server-url
2. storage.docdb.mongo-server-url-dir

`storage.docdb.mongo-server-url` simply allows supplying the value of
MONGO_SERVER_URL as a field. When this field is updated, the chains
controller pod does not restart, unlike when the MONGO_SERVER_URL
environment variable is updated.

`storage.docdb.mongo-server-url-dir` allows reading MONGO_SERVER_URL
from a file in the specified directory. This allows mounting the value
of MONGO_SERVER_URL from a secret or other mechanisms. When the value of
MONGO_SERVER_URL is updated in the path, the new value is automatically
picked up and applied.

Submitter Checklist

As the author of this PR, please check off the items in this checklist:

Has Docs included if any changes are user facing
Has Tests included if any functionality added or changed
Follows the commit message standard
Meets the Tekton contributor standards (including
functionality, content, code)
Release notes block below has been updated with any user facing changes (API changes, bug fixes, changes requiring upgrade notices or deprecation warnings)
Release notes contains the string "action required" if the change requires additional action from users switching to the new release

Release Notes

Allow supplying MONGO_SERVER_URL via chains-config to facilitate rotation

Fix #1089

tekton-robot · 2024-05-06T06:45:04Z

The following is the coverage report on the affected files.
Say /test pull-tekton-chains-go-coverage to re-run this coverage report

File	Old Coverage	New Coverage	Delta
pkg/chains/signing.go	74.3%	69.8%	-4.5
pkg/chains/storage/docdb/docdb.go	64.7%	20.0%	-44.7
pkg/chains/storage/storage.go	56.7%	41.1%	-15.6
pkg/reconciler/pipelinerun/controller.go	86.4%	82.1%	-4.2
pkg/reconciler/taskrun/controller.go	88.9%	83.3%	-5.6

tekton-robot · 2024-05-06T07:59:45Z

The following is the coverage report on the affected files.
Say /test pull-tekton-chains-go-coverage to re-run this coverage report

File	Old Coverage	New Coverage	Delta
pkg/chains/signing.go	74.3%	69.8%	-4.5
pkg/chains/storage/docdb/docdb.go	64.7%	20.0%	-44.7
pkg/chains/storage/storage.go	56.7%	41.1%	-15.6
pkg/reconciler/pipelinerun/controller.go	86.4%	82.1%	-4.2
pkg/reconciler/taskrun/controller.go	88.9%	83.3%	-5.6

tekton-robot · 2024-05-06T08:27:47Z

The following is the coverage report on the affected files.
Say /test pull-tekton-chains-go-coverage to re-run this coverage report

File	Old Coverage	New Coverage	Delta
pkg/chains/signing.go	74.3%	69.8%	-4.5
pkg/chains/storage/docdb/docdb.go	64.7%	19.5%	-45.2
pkg/chains/storage/storage.go	56.7%	39.7%	-17.0
pkg/reconciler/pipelinerun/controller.go	86.4%	82.1%	-4.2
pkg/reconciler/taskrun/controller.go	88.9%	83.3%	-5.6

tekton-robot · 2024-05-06T09:22:25Z

The following is the coverage report on the affected files.
Say /test pull-tekton-chains-go-coverage to re-run this coverage report

File	Old Coverage	New Coverage	Delta
pkg/chains/signing.go	74.3%	69.8%	-4.5
pkg/chains/storage/docdb/docdb.go	64.7%	19.5%	-45.2
pkg/chains/storage/storage.go	56.7%	39.7%	-17.0
pkg/reconciler/pipelinerun/controller.go	86.4%	82.1%	-4.2
pkg/reconciler/taskrun/controller.go	88.9%	83.3%	-5.6

tekton-robot · 2024-05-06T11:26:52Z

The following is the coverage report on the affected files.
Say /test pull-tekton-chains-go-coverage to re-run this coverage report

File	Old Coverage	New Coverage	Delta
pkg/chains/signing.go	74.3%	72.3%	-2.0
pkg/chains/storage/docdb/docdb.go	64.7%	20.0%	-44.7
pkg/chains/storage/storage.go	56.7%	36.4%	-20.3
pkg/reconciler/pipelinerun/controller.go	86.4%	82.1%	-4.2
pkg/reconciler/taskrun/controller.go	88.9%	83.3%	-5.6

tekton-robot · 2024-05-06T12:49:58Z

The following is the coverage report on the affected files.
Say /test pull-tekton-chains-go-coverage to re-run this coverage report

File	Old Coverage	New Coverage	Delta
pkg/chains/signing.go	74.3%	72.3%	-2.0
pkg/chains/storage/docdb/docdb.go	64.7%	40.9%	-23.8
pkg/chains/storage/storage.go	56.7%	36.4%	-20.3
pkg/reconciler/pipelinerun/controller.go	86.4%	82.1%	-4.2
pkg/reconciler/taskrun/controller.go	88.9%	83.3%	-5.6

tekton-robot · 2024-05-06T13:28:47Z

The following is the coverage report on the affected files.
Say /test pull-tekton-chains-go-coverage to re-run this coverage report

File	Old Coverage	New Coverage	Delta
pkg/chains/signing.go	74.3%	72.3%	-2.0
pkg/chains/storage/docdb/docdb.go	64.7%	44.1%	-20.6
pkg/chains/storage/storage.go	56.7%	36.4%	-20.3
pkg/reconciler/pipelinerun/controller.go	86.4%	82.1%	-4.2
pkg/reconciler/taskrun/controller.go	88.9%	83.3%	-5.6

tekton-robot · 2024-05-06T14:14:53Z

The following is the coverage report on the affected files.
Say /test pull-tekton-chains-go-coverage to re-run this coverage report

File	Old Coverage	New Coverage	Delta
pkg/chains/signing.go	74.3%	72.3%	-2.0
pkg/chains/storage/docdb/docdb.go	64.7%	44.1%	-20.6
pkg/chains/storage/storage.go	56.7%	36.4%	-20.3
pkg/reconciler/pipelinerun/controller.go	86.4%	82.1%	-4.2
pkg/reconciler/taskrun/controller.go	88.9%	83.3%	-5.6

concaf · 2024-05-07T17:09:25Z

/assign @wlynch @lcarva @PuneetPunamiya

tekton-robot · 2024-05-07T17:09:28Z

@concaf: GitHub didn't allow me to assign the following users: PuneetPunamiya.

Note that only tektoncd members, repo collaborators and people who have commented on this issue/PR can be assigned. Additionally, issues/PRs can only have 10 assignees at the same time.
For more information please see the contributor guide

Details

In response to this:

/assign @wlynch @lcarva @PuneetPunamiya

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.

concaf · 2024-05-07T17:09:51Z

/assign @wlynch

concaf · 2024-05-14T11:13:21Z

#1119 needs to be merged for tests to pass

tekton-robot · 2024-05-14T11:18:19Z

The following is the coverage report on the affected files.
Say /test pull-tekton-chains-go-coverage to re-run this coverage report

File	Old Coverage	New Coverage	Delta
pkg/chains/signing.go	74.3%	71.8%	-2.5
pkg/chains/storage/docdb/docdb.go	64.7%	79.8%	15.1
pkg/chains/storage/storage.go	56.7%	33.9%	-22.8
pkg/reconciler/pipelinerun/controller.go	86.4%	82.1%	-4.2
pkg/reconciler/taskrun/controller.go	88.9%	83.3%	-5.6

tekton-robot · 2024-05-15T05:09:16Z

The following is the coverage report on the affected files.
Say /test pull-tekton-chains-go-coverage to re-run this coverage report

File	Old Coverage	New Coverage	Delta
pkg/chains/signing.go	74.3%	71.8%	-2.5
pkg/chains/storage/storage.go	56.7%	33.9%	-22.8
pkg/reconciler/pipelinerun/controller.go	86.4%	82.1%	-4.2
pkg/reconciler/taskrun/controller.go	88.9%	83.3%	-5.6

wlynch

One small readability comment, otherwise LGTM!

wlynch · 2024-05-20T15:57:58Z

pkg/chains/storage/docdb/docdb.go

+		}
+	} else if cfg.Storage.DocDB.MongoServerURL != "" {
+		logger.Infof("setting %s from storage.docdb.mongo-server-url", mongoEnv)
+		if err := os.Setenv(mongoEnv, cfg.Storage.DocDB.MongoServerURL); err != nil {


bleh it'd be nicer if we could pass it through the URL or the context. I guess this is fine for now though 😅

wlynch · 2024-05-20T16:02:18Z

pkg/chains/storage/docdb/docdb.go

+				if event.Has(fsnotify.Create) || event.Has(fsnotify.Write) || event.Has(fsnotify.Remove) {
+					// event.Name captures the path that is updated
+					if slices.Contains(pathsToWatch, event.Name) {
+						// Do not reconfigure backend if env var has not changed
+						updatedEnv, err := getMongoServerURLFromDir(cfg.Storage.DocDB.MongoServerURLDir)
+						if err != nil {
+							logger.Error(err)
+							backendChan <- nil
+						}
+						if updatedEnv != os.Getenv("MONGO_SERVER_URL") {
+							logger.Infof("directory %s has been updated, reconfiguring backend...", cfg.Storage.DocDB.MongoServerURLDir)
+
+							// Now that MONGO_SERVER_URL has been updated, we should update docdb backend again
+							newDocDBBackend, err := NewStorageBackend(ctx, cfg)
+							if err != nil {
+								logger.Error(err)
+								backendChan <- nil
+							} else {
+								// Storing the backend in the signer so everyone has access to the up-to-date backend
+								backendChan <- newDocDBBackend
+							}
+						} else {
+							logger.Infof("MONGO_SERVER_URL has not changed in path: %s, backend will not be reconfigured", cfg.Storage.DocDB.MongoServerURLDir)
+						}
+					}
+				}


If there are short circuit conditions, break these out early with returns so the reader doesn't need to keep stack state in their head. e.g. this can be rewritten as:

Suggested change

if event.Has(fsnotify.Create) || event.Has(fsnotify.Write) || event.Has(fsnotify.Remove) {

// event.Name captures the path that is updated

if slices.Contains(pathsToWatch, event.Name) {

// Do not reconfigure backend if env var has not changed

updatedEnv, err := getMongoServerURLFromDir(cfg.Storage.DocDB.MongoServerURLDir)

if err != nil {

logger.Error(err)

backendChan <- nil

}

if updatedEnv != os.Getenv("MONGO_SERVER_URL") {

logger.Infof("directory %s has been updated, reconfiguring backend...", cfg.Storage.DocDB.MongoServerURLDir)

// Now that MONGO_SERVER_URL has been updated, we should update docdb backend again

newDocDBBackend, err := NewStorageBackend(ctx, cfg)

if err != nil {

logger.Error(err)

backendChan <- nil

} else {

// Storing the backend in the signer so everyone has access to the up-to-date backend

backendChan <- newDocDBBackend

}

} else {

logger.Infof("MONGO_SERVER_URL has not changed in path: %s, backend will not be reconfigured", cfg.Storage.DocDB.MongoServerURLDir)

}

}

}

if !(event.Has(fsnotify.Create) || event.Has(fsnotify.Write) || event.Has(fsnotify.Remove)) {

// Files have not been modified

continue

}

// event.Name captures the path that is updated

if !slices.Contains(pathsToWatch, event.Name) {

// No file that we're interested in has been modified.

continue

}

// Do not reconfigure backend if env var has not changed

updatedEnv, err := getMongoServerURLFromDir(cfg.Storage.DocDB.MongoServerURLDir)

if err != nil {

logger.Error(err)

backendChan <- nil

}

if updatedEnv == os.Getenv("MONGO_SERVER_URL") {

logger.Infof("MONGO_SERVER_URL has not changed in path: %s, backend will not be reconfigured", cfg.Storage.DocDB.MongoServerURLDir)

}

logger.Infof("directory %s has been updated, reconfiguring backend...", cfg.Storage.DocDB.MongoServerURLDir)

// Now that MONGO_SERVER_URL has been updated, we should update docdb backend again

newDocDBBackend, err := NewStorageBackend(ctx, cfg)

if err != nil {

logger.Error(err)

backendChan <- nil

} else {

// Storing the backend in the signer so everyone has access to the up-to-date backend

backendChan <- newDocDBBackend

}

Updated 👍🏻

wlynch · 2024-05-20T16:10:33Z

pkg/chains/storage/docdb/docdb.go

+		if os.IsNotExist(err) {
+			// If directory does not exist, then create it. This is needed for
+			// the fsnotify watcher.
+			// fsnotify does not receive events if the path that it's watching
+			// is created later.
+			if err := os.MkdirAll(dir, 0755); err != nil {
+				return "", err
+			}
+			return "", nil
+		} else {
+			return "", err
+		}
+	}


To fix lint error

Suggested change

if os.IsNotExist(err) {

// If directory does not exist, then create it. This is needed for

// the fsnotify watcher.

// fsnotify does not receive events if the path that it's watching

// is created later.

if err := os.MkdirAll(dir, 0755); err != nil {

return "", err

}

return "", nil

} else {

return "", err

}

}

if os.IsNotExist(err) {

// If directory does not exist, then create it. This is needed for

// the fsnotify watcher.

// fsnotify does not receive events if the path that it's watching

// is created later.

if err := os.MkdirAll(dir, 0755); err != nil {

return "", err

}

return "", nil

}

return "", err

}

concaf · 2024-06-06T06:56:02Z

pkg/chains/storage/docdb/docdb.go

+	}
+
+	// Set up the watcher only for mongo backends
+	if u.Scheme != "mongo" {


Suggested change

if u.Scheme != "mongo" {

if u.Scheme != mongodocstore.Scheme {

tekton-robot · 2024-06-18T07:35:52Z

The following is the coverage report on the affected files.
Say /test pull-tekton-chains-go-coverage to re-run this coverage report

File	Old Coverage	New Coverage	Delta
pkg/chains/signing.go	75.7%	73.3%	-2.4
pkg/chains/storage/docdb/docdb.go	64.7%	76.9%	12.2
pkg/chains/storage/storage.go	56.7%	33.9%	-22.8
pkg/reconciler/pipelinerun/controller.go	86.4%	82.1%	-4.2
pkg/reconciler/taskrun/controller.go	88.9%	83.3%	-5.6

PuneetPunamiya · 2024-06-18T07:43:49Z

/test pull-tekton-chains-unit-tests

PuneetPunamiya · 2024-06-18T07:44:12Z

/test pull-tekton-chains-integration-tests

Currently, when using the Mongo docstore for docdb storage backend, the only way to supply MONGO_SERVER_URL environment variable (which contains the credentials to connect to MongoDB) is by adding an environment variable to the Chains controller pod. It's a farily common practice to update the MONGO_SERVER_URL at regular intervals when the credentials are rotated. To facilitate this, this commit adds 2 fields to Chains' configuration: 1. storage.docdb.mongo-server-url 2. storage.docdb.mongo-server-url-dir `storage.docdb.mongo-server-url` simply allows supplying the value of MONGO_SERVER_URL as a field. When this field is updated, the chains controller pod does not restart, unlike when the MONGO_SERVER_URL environment variable is updated. `storage.docdb.mongo-server-url-dir` allows reading MONGO_SERVER_URL from a file in the specified directory. This allows mounting the value of MONGO_SERVER_URL from a secret or other mechanisms. When the value of MONGO_SERVER_URL is updated in the path, the new value is automatically picked up and applied.

This commit bumps gocloud.dev/docstore/mongodocstore to the commit at google/go-cloud#3429 that allows MONGO_SERVER_URL rotation.

tekton-robot · 2024-06-25T07:07:34Z

The following is the coverage report on the affected files.
Say /test pull-tekton-chains-go-coverage to re-run this coverage report

File	Old Coverage	New Coverage	Delta
pkg/chains/signing.go	75.7%	73.3%	-2.4
pkg/chains/storage/docdb/docdb.go	64.7%	76.0%	11.3
pkg/chains/storage/storage.go	56.7%	33.9%	-22.8
pkg/reconciler/pipelinerun/controller.go	86.4%	82.1%	-4.2
pkg/reconciler/taskrun/controller.go	88.9%	83.3%	-5.6

PuneetPunamiya · 2024-06-25T08:41:59Z

/test pull-tekton-chains-build-tests

PuneetPunamiya · 2024-06-26T05:58:19Z

/test pull-tekton-chains-build-tests

PuneetPunamiya · 2024-07-03T01:05:53Z

/test pull-tekton-chains-build-tests

tekton-robot · 2024-07-12T17:59:34Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: chitrangpatel, lcarva

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

The pull request process is described here

Details

Needs approval from an approver in each of these files:

~~OWNERS~~ [chitrangpatel,lcarva]

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

lcarva · 2024-07-12T17:59:37Z

/lgtm

PuneetPunamiya · 2024-07-13T05:36:35Z

/test pull-tekton-chains-unit-tests

PuneetPunamiya · 2024-07-13T05:36:43Z

/test pull-tekton-chains-integration-tests

tekton-robot requested review from chitrangpatel and lcarva May 6, 2024 06:38

tekton-robot added the size/L Denotes a PR that changes 100-499 lines, ignoring generated files. label May 6, 2024

concaf force-pushed the concaf/feature/mongo-path branch from 4082944 to 2d7c733 Compare May 6, 2024 07:51

concaf force-pushed the concaf/feature/mongo-path branch from 2d7c733 to 43c7e4a Compare May 6, 2024 08:20

concaf force-pushed the concaf/feature/mongo-path branch 2 times, most recently from ffb7649 to 7cc40f5 Compare May 6, 2024 09:14

concaf force-pushed the concaf/feature/mongo-path branch 2 times, most recently from 31aeeb0 to b4230b7 Compare May 6, 2024 11:20

tekton-robot added size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. and removed size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels May 6, 2024

concaf force-pushed the concaf/feature/mongo-path branch from e76e64a to ca0580f Compare May 6, 2024 13:22

concaf force-pushed the concaf/feature/mongo-path branch 2 times, most recently from 59efc83 to c11bbb4 Compare May 6, 2024 14:09

concaf mentioned this pull request May 6, 2024

^ in MONGO_SERVER_URL password causes null pointer panic #1114

Closed

tekton-robot assigned lcarva and wlynch May 7, 2024

concaf force-pushed the concaf/feature/mongo-path branch 2 times, most recently from 922593d to 6ebd8f1 Compare May 8, 2024 14:24

concaf force-pushed the concaf/feature/mongo-path branch from 8336375 to fd62c7e Compare May 15, 2024 05:03

chitrangpatel approved these changes May 16, 2024

View reviewed changes

tekton-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label May 16, 2024

chitrangpatel requested a review from wlynch May 16, 2024 19:49

wlynch reviewed May 20, 2024

View reviewed changes

tekton-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jun 5, 2024

concaf commented Jun 6, 2024

View reviewed changes

PuneetPunamiya force-pushed the concaf/feature/mongo-path branch from fd62c7e to 220339c Compare June 18, 2024 07:30

tekton-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jun 18, 2024

concaf added 2 commits June 25, 2024 12:26

Bump gocloud.dev/docstore/mongodocstore

ffd65fd

This commit bumps gocloud.dev/docstore/mongodocstore to the commit at google/go-cloud#3429 that allows MONGO_SERVER_URL rotation.

PuneetPunamiya force-pushed the concaf/feature/mongo-path branch from 220339c to ffd65fd Compare June 25, 2024 06:56

lcarva approved these changes Jul 12, 2024

View reviewed changes

tekton-robot added the lgtm Indicates that a PR is ready to be merged. label Jul 12, 2024

tekton-robot merged commit dcbad78 into tektoncd:main Jul 13, 2024

	if u.Scheme != "mongo" {
	if u.Scheme != mongodocstore.Scheme {

Conversation

concaf commented May 6, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Changes

Submitter Checklist

Release Notes

Uh oh!

tekton-robot commented May 6, 2024

Uh oh!

tekton-robot commented May 6, 2024

Uh oh!

tekton-robot commented May 6, 2024

Uh oh!

tekton-robot commented May 6, 2024

Uh oh!

tekton-robot commented May 6, 2024

Uh oh!

tekton-robot commented May 6, 2024

Uh oh!

tekton-robot commented May 6, 2024

Uh oh!

tekton-robot commented May 6, 2024

Uh oh!

concaf commented May 7, 2024

Uh oh!

tekton-robot commented May 7, 2024

Uh oh!

concaf commented May 7, 2024

Uh oh!

concaf commented May 14, 2024

Uh oh!

tekton-robot commented May 14, 2024

Uh oh!

tekton-robot commented May 15, 2024

Uh oh!

wlynch left a comment

Choose a reason for hiding this comment

Uh oh!

wlynch May 20, 2024

Choose a reason for hiding this comment

Uh oh!

wlynch May 20, 2024

Choose a reason for hiding this comment

Uh oh!

PuneetPunamiya Jun 25, 2024

Choose a reason for hiding this comment

Uh oh!

wlynch May 20, 2024

Choose a reason for hiding this comment

Uh oh!

concaf Jun 6, 2024

Choose a reason for hiding this comment

Uh oh!

tekton-robot commented Jun 18, 2024

Uh oh!

PuneetPunamiya commented Jun 18, 2024

Uh oh!

PuneetPunamiya commented Jun 18, 2024

Uh oh!

tekton-robot commented Jun 25, 2024

Uh oh!

PuneetPunamiya commented Jun 25, 2024

Uh oh!

PuneetPunamiya commented Jun 26, 2024

Uh oh!

PuneetPunamiya commented Jul 3, 2024

Uh oh!

tekton-robot commented Jul 12, 2024

Uh oh!

lcarva commented Jul 12, 2024

Uh oh!

PuneetPunamiya commented Jul 13, 2024

Uh oh!

PuneetPunamiya commented Jul 13, 2024

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

concaf commented May 6, 2024 •

edited

Loading