Allow supplying MONGO_SERVER_URL via chains-config #1113
Conversation
tekton-robot
added
the
size/L
|
The following is the coverage report on the affected files.
|
concaf
force-pushed
the
concaf/feature/mongo-path
branch
from
4082944
to
2d7c733
Compare
|
The following is the coverage report on the affected files.
|
concaf
force-pushed
the
concaf/feature/mongo-path
branch
from
2d7c733
to
43c7e4a
Compare
|
The following is the coverage report on the affected files.
|
concaf
force-pushed
the
concaf/feature/mongo-path
branch
2 times, most recently
from
ffb7649
to
7cc40f5
Compare
|
The following is the coverage report on the affected files.
|
concaf
force-pushed
the
concaf/feature/mongo-path
branch
2 times, most recently
from
31aeeb0
to
b4230b7
Compare
|
The following is the coverage report on the affected files.
|
tekton-robot
added
size/XL
|
The following is the coverage report on the affected files.
|
concaf
force-pushed
the
concaf/feature/mongo-path
branch
from
e76e64a
to
ca0580f
Compare
|
The following is the coverage report on the affected files.
|
concaf
force-pushed
the
concaf/feature/mongo-path
branch
2 times, most recently
from
59efc83
to
c11bbb4
Compare
|
The following is the coverage report on the affected files.
|
|
/assign @wlynch @lcarva @PuneetPunamiya |
|
@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. In response to this:
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. |
|
/assign @wlynch |
concaf
force-pushed
the
concaf/feature/mongo-path
branch
2 times, most recently
from
922593d
to
6ebd8f1
Compare
|
The following is the coverage report on the affected files.
|
| } | ||
| } 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 { |
There was a problem hiding this comment.
I'm not a fan of setting global env state from the local config - I'd rather global options configure the local config (e.g. config.Config values are set based on env values if not set).
I'd also expect this ordering for config values, since they're most specific to least.
- ConfigMap values set directly
- MONGO_SERVER_URL
- MONGO_SERVER_URL_DIR
There was a problem hiding this comment.
I'm not a fan of setting global env state from the local config
makes sense and i agree, however there 2 things here:
os.Setenvwill only set the value ofMONGO_SERVER_URLwithin this context in code and not outside for the whole container and it can't update its parent process- we must set
MONGO_SERVER_URLenv var instead of a variable in config because that's what https://github.com/google/go-cloud/tree/master/docstore/mongodocstore consumes...
I'd also expect this ordering for config values, since they're most specific to least.
ConfigMap values set directly
MONGO_SERVER_URL
MONGO_SERVER_URL_DIR
- this is roughly the order that's set right now; ConfigMap values take precedence over
MONGO_SERVER_URL MONGO_SERVER_URL_DIRis not an env var that i've added 🤔
There was a problem hiding this comment.
bleh it'd be nicer if we could pass it through the URL or the context. I guess this is fine for now though 😅
concaf
force-pushed
the
concaf/feature/mongo-path
branch
from
6ebd8f1
to
9e59233
Compare
|
The following is the coverage report on the affected files.
|
concaf
force-pushed
the
concaf/feature/mongo-path
branch
from
9e59233
to
9295eed
Compare
|
The following is the coverage report on the affected files.
|
|
/test pull-tekton-chains-unit-tests |
tekton-robot
added
the
needs-rebase
concaf
force-pushed
the
concaf/feature/mongo-path
branch
from
9295eed
to
8336375
Compare
tekton-robot
removed
the
needs-rebase
|
#1119 needs to be merged for tests to pass |
|
The following is the coverage report on the affected files.
|
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.
concaf
force-pushed
the
concaf/feature/mongo-path
branch
from
8336375
to
fd62c7e
Compare
|
The following is the coverage report on the affected files.
|
|
@concaf: The following tests failed, say
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. |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: chitrangpatel 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:
Approvers can indicate their approval by writing |
tekton-robot
added
the
approved
| } | ||
| } 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 { |
There was a problem hiding this comment.
bleh it'd be nicer if we could pass it through the URL or the context. I guess this is fine for now though 😅
| 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) | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
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:
| 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 | |
| } |
| 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 | ||
| } | ||
| } |
There was a problem hiding this comment.
To fix lint error
| 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 | |
| } |
Changes
Submitter Checklist
As the author of this PR, please check off the items in this checklist:
functionality, content, code)
Release Notes
Fix #1089