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

Allow supplying MONGO_SERVER_URL via chains-config #1113

Merged
merged 2 commits into from
Jul 13, 2024

Conversation

concaf
Copy link
Contributor

@concaf concaf commented May 6, 2024

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 tekton-robot added the size/L Denotes a PR that changes 100-499 lines, ignoring generated files. label May 6, 2024
@tekton-robot
Copy link

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

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

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

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

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

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

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

@concaf 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
Copy link

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 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
@tekton-robot
Copy link

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

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

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 concaf force-pushed the concaf/feature/mongo-path branch 2 times, most recently from 59efc83 to c11bbb4 Compare May 6, 2024 14:09
@tekton-robot
Copy link

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
Copy link
Contributor Author

concaf commented May 7, 2024

/assign @wlynch @lcarva @PuneetPunamiya

@tekton-robot
Copy link

@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

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
Copy link
Contributor Author

concaf commented May 7, 2024

/assign @wlynch

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

concaf commented May 14, 2024

#1119 needs to be merged for tests to pass

@tekton-robot
Copy link

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

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

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

@tekton-robot tekton-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label May 16, 2024
@chitrangpatel chitrangpatel requested a review from wlynch May 16, 2024 19:49
Copy link
Member

@wlynch wlynch left a comment

Choose a reason for hiding this comment

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

One small readability comment, otherwise LGTM!

}
} 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 {
Copy link
Member

Choose a reason for hiding this comment

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

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

Comment on lines 140 to 167
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)
}
}
}
Copy link
Member

Choose a reason for hiding this comment

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

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
}

Copy link
Member

Choose a reason for hiding this comment

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

Updated 👍🏻

Comment on lines 301 to 314
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
}
}
Copy link
Member

Choose a reason for hiding this comment

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

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
}

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

// Set up the watcher only for mongo backends
if u.Scheme != "mongo" {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

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

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
Copy link
Member

/test pull-tekton-chains-unit-tests

@PuneetPunamiya
Copy link
Member

/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
Copy link

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
Copy link
Member

/test pull-tekton-chains-build-tests

2 similar comments
@PuneetPunamiya
Copy link
Member

/test pull-tekton-chains-build-tests

@PuneetPunamiya
Copy link
Member

/test pull-tekton-chains-build-tests

@tekton-robot
Copy link

[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

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
Copy link
Contributor

lcarva commented Jul 12, 2024

/lgtm

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

/test pull-tekton-chains-unit-tests

@PuneetPunamiya
Copy link
Member

/test pull-tekton-chains-integration-tests

@tekton-robot tekton-robot merged commit dcbad78 into tektoncd:main Jul 13, 2024
18 checks passed
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. size/XL Denotes a PR that changes 500-999 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Support for Mongo Token rotation
6 participants