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

docstore/mongodocstore: Update Mongo dialer when MONGO_SERVER_URL rotates #3429

Merged
merged 1 commit into from
May 1, 2024

Conversation

concaf
Copy link
Contributor

@concaf concaf commented Apr 30, 2024

Prior to this commit, the dialer for MongoDB was generated once from MONGO_SERVER_URL environment variable but was never updated even when the environment variable was updated in subsequent calls. While this works fine when MONGO_SERVER_URL is not expected to update, but as MONGO_SERVER_URL also contains the credentials to connect to MongoDB, it's a fairly common use case to rotate these credentials (and hence the environment variable) at regular intervals.

This commit fixes that and updates the dialer when MONGO_SERVER_URL is updated.

This PR blocks tektoncd/chains#1089

}

func (o *defaultDialer) OpenCollectionURL(ctx context.Context, u *url.URL) (*docstore.Collection, error) {
o.init.Do(func() {
serverURL := os.Getenv("MONGO_SERVER_URL")
serverURL := os.Getenv("MONGO_SERVER_URL")
Copy link
Contributor

Choose a reason for hiding this comment

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

This is not thread-safe (OpenCollectionURL can be called concurrently on the same o).

You can protect it with a sync.Mutex.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

makes sense, updated 👍🏼

o.init.Do(func() {
serverURL := os.Getenv("MONGO_SERVER_URL")
serverURL := os.Getenv("MONGO_SERVER_URL")
if serverURL != o.mongoServerURL {
Copy link
Contributor

Choose a reason for hiding this comment

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

This doesn't work. If MONGO_SERVER_URL isn't set, then both of these will be the empty string and we'll fall through without returning an error.

How about something like if o.opener == nil || serverURL != o.openerServerURL ... ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

good catch 👍🏼

i've updated this to check for an empty MONGO_SERVER_URL outside the if condition, so it should fail each time the env var is empty

	if currentEnv == "" {
		o.err = errors.New("MONGO_SERVER_URL environment variable is not set")
		return nil, fmt.Errorf("open collection %s: %v", u, o.err)
	}

	// If MONGO_SERVER_URL has been updated, then update o.opener as well
	if currentEnv != o.mongoServerURL {
		client, err := Dial(ctx, currentEnv)
...
...

wdyt?

@concaf concaf force-pushed the concaf/fix/mongo-server-url-rotation branch 2 times, most recently from 18ee57d to cd0e696 Compare May 1, 2024 14:23
@concaf
Copy link
Contributor Author

concaf commented May 1, 2024

@vangent thanks for the quick review 🚀 🙂 - i've updated the PR and also added some tests.

Prior to this commit, the dialer for MongoDB was generated once from
MONGO_SERVER_URL environment variable but was never updated even when
the environment variable was updated in subsequent calls. While this
works fine when MONGO_SERVER_URL is not expected to update, but as
MONGO_SERVER_URL also contains the credentials to connect to MongoDB,
it's a fairly common use case to rotate these credentials (and hence the
environment variable) at regular intervals.

This commit fixes that and updates the dialer when MONGO_SERVER_URL is
updated.
@concaf concaf force-pushed the concaf/fix/mongo-server-url-rotation branch from cd0e696 to 024ea8c Compare May 1, 2024 14:25
Copy link

codecov bot commented May 1, 2024

Codecov Report

Attention: Patch coverage is 81.81818% with 2 lines in your changes are missing coverage. Please review.

Project coverage is 73.20%. Comparing base (3f641fd) to head (024ea8c).

Files Patch % Lines
docstore/mongodocstore/urls.go 81.81% 2 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##           master    #3429      +/-   ##
==========================================
+ Coverage   73.17%   73.20%   +0.03%     
==========================================
  Files         113      113              
  Lines       14872    14873       +1     
==========================================
+ Hits        10882    10888       +6     
+ Misses       3216     3213       -3     
+ Partials      774      772       -2     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@vangent vangent merged commit d8b9c94 into google:master May 1, 2024
5 checks passed
concaf added a commit to concaf/chains that referenced this pull request May 6, 2024
This commit bumps gocloud.dev/docstore/mongodocstore to the commit at
google/go-cloud#3429 that allows
MONGO_SERVER_URL rotation.
concaf added a commit to concaf/chains that referenced this pull request May 8, 2024
This commit bumps gocloud.dev/docstore/mongodocstore to the commit at
google/go-cloud#3429 that allows
MONGO_SERVER_URL rotation.
concaf added a commit to concaf/chains that referenced this pull request May 14, 2024
This commit bumps gocloud.dev/docstore/mongodocstore to the commit at
google/go-cloud#3429 that allows MONGO_SERVER_URL rotation.
concaf added a commit to concaf/chains that referenced this pull request May 15, 2024
This commit bumps gocloud.dev/docstore/mongodocstore to the commit at
google/go-cloud#3429 that allows MONGO_SERVER_URL rotation.
PuneetPunamiya pushed a commit to concaf/chains that referenced this pull request Jun 18, 2024
This commit bumps gocloud.dev/docstore/mongodocstore to the commit at
google/go-cloud#3429 that allows MONGO_SERVER_URL rotation.
PuneetPunamiya pushed a commit to concaf/chains that referenced this pull request Jun 25, 2024
This commit bumps gocloud.dev/docstore/mongodocstore to the commit at
google/go-cloud#3429 that allows MONGO_SERVER_URL rotation.
tekton-robot pushed a commit to tektoncd/chains that referenced this pull request Jul 13, 2024
* Allow supplying MONGO_SERVER_URL via chains-config

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.

* Bump gocloud.dev/docstore/mongodocstore

This commit bumps gocloud.dev/docstore/mongodocstore to the commit at
google/go-cloud#3429 that allows MONGO_SERVER_URL rotation.
renovate bot referenced this pull request in open-feature/flagd Sep 11, 2024
This PR contains the following updates:

| Package | Change | Age | Adoption | Passing | Confidence |
|---|---|---|---|---|---|
| [gocloud.dev](https://redirect.github.com/google/go-cloud) | `v0.37.0`
-> `v0.39.0` |
[![age](https://developer.mend.io/api/mc/badges/age/go/gocloud.dev/v0.39.0?slim=true)](https://docs.renovatebot.com/merge-confidence/)
|
[![adoption](https://developer.mend.io/api/mc/badges/adoption/go/gocloud.dev/v0.39.0?slim=true)](https://docs.renovatebot.com/merge-confidence/)
|
[![passing](https://developer.mend.io/api/mc/badges/compatibility/go/gocloud.dev/v0.37.0/v0.39.0?slim=true)](https://docs.renovatebot.com/merge-confidence/)
|
[![confidence](https://developer.mend.io/api/mc/badges/confidence/go/gocloud.dev/v0.37.0/v0.39.0?slim=true)](https://docs.renovatebot.com/merge-confidence/)
|

---

### Release Notes

<details>
<summary>google/go-cloud (gocloud.dev)</summary>

###
[`v0.39.0`](https://redirect.github.com/google/go-cloud/releases/tag/v0.39.0)

[Compare
Source](https://redirect.github.com/google/go-cloud/compare/v0.38.0...v0.39.0)

#### BREAKING CHANGE (AWS only, V1 vs V2 SDK)

Context: AWS has [announced maintenance
mode](https://aws.amazon.com/blogs/developer/announcing-end-of-support-for-aws-sdk-for-go-v1-on-july-31-2025/)
for the Go V1 SDK.

Go CDK has changed the default SDK for URLs across all modules except
`docstore/awsdynamodb` to be V2 (previously you needed to add
`awssdk=v2` to the URL to get V2). Most URLs should continue to work,
but in some cases you may need to add `awssdk=v1` to force V1
explicitly.

Also, concrete type constructors (e.g., `OpenBucket`) for V1 (again,
except `docstore/awsdynamodb`) have been marked deprecated; please
migrate to using the V2 versions (e.g., `OpenBucketV2`).

Our tentative plan is to remove support for V1 in early 2025; please
[file a
bug](https://redirect.github.com/google/go-cloud/issues/new/choose) if
you have concerns about that.

#### What's Changed

- pubsub: Make batch request results independent by
[@&#8203;mitsos1os](https://redirect.github.com/mitsos1os) in
[https://github.com/google/go-cloud/pull/3457](https://redirect.github.com/google/go-cloud/pull/3457)
- docstore/all: Add support for boolean filter by
[@&#8203;ybourgery](https://redirect.github.com/ybourgery) in
[https://github.com/google/go-cloud/pull/3464](https://redirect.github.com/google/go-cloud/pull/3464)
- aws/all: Mark V1 constructors deprecated. by
[@&#8203;vangent](https://redirect.github.com/vangent) in
[https://github.com/google/go-cloud/pull/3466](https://redirect.github.com/google/go-cloud/pull/3466)
- aws/all: Change the default for AWS URLs from V1 to V2. by
[@&#8203;vangent](https://redirect.github.com/vangent) in
[https://github.com/google/go-cloud/pull/3465](https://redirect.github.com/google/go-cloud/pull/3465)
- all: update to go version 1.23 by
[@&#8203;vangent](https://redirect.github.com/vangent) in
[https://github.com/google/go-cloud/pull/3467](https://redirect.github.com/google/go-cloud/pull/3467)

#### New Contributors

- [@&#8203;mitsos1os](https://redirect.github.com/mitsos1os) made their
first contribution in
[https://github.com/google/go-cloud/pull/3457](https://redirect.github.com/google/go-cloud/pull/3457)
- [@&#8203;dependabot](https://redirect.github.com/dependabot) made
their first contribution in
[https://github.com/google/go-cloud/pull/3448](https://redirect.github.com/google/go-cloud/pull/3448)

**Full Changelog**:
google/go-cloud@v0.38.0...v0.39.0

###
[`v0.38.0`](https://redirect.github.com/google/go-cloud/releases/tag/v0.38.0)

[Compare
Source](https://redirect.github.com/google/go-cloud/compare/v0.37.0...v0.38.0)

**blob**

- **all**: Fix panics if reader recreation fails after Seek by
[@&#8203;vangent](https://redirect.github.com/vangent) in
[https://github.com/google/go-cloud/pull/3425](https://redirect.github.com/google/go-cloud/pull/3425)
- **all**: Convert errors in `Open()` into appropriate fs errors by
[@&#8203;milescrabill](https://redirect.github.com/milescrabill) in
[https://github.com/google/go-cloud/pull/3443](https://redirect.github.com/google/go-cloud/pull/3443)
- **s3blob**: Fix Copy to work with keys that need escaping by
[@&#8203;vangent](https://redirect.github.com/vangent) in
[https://github.com/google/go-cloud/pull/3403](https://redirect.github.com/google/go-cloud/pull/3403)
- **azureblob**: Do not panic if Content-Length and Content-Range are
missing by [@&#8203;chancez](https://redirect.github.com/chancez) in
[https://github.com/google/go-cloud/pull/3445](https://redirect.github.com/google/go-cloud/pull/3445)
- **fileblob**: Allow customization of the FileMode by
[@&#8203;vangent](https://redirect.github.com/vangent) in
[https://github.com/google/go-cloud/pull/3426](https://redirect.github.com/google/go-cloud/pull/3426)

**pubsub**

- **awssnssqs**: Add support for setting FIFO message metadata by
[@&#8203;bartventer](https://redirect.github.com/bartventer) in
[https://github.com/google/go-cloud/pull/3435](https://redirect.github.com/google/go-cloud/pull/3435)
- **kafkapubsub**: Configuring key_name when OpenTopicURL by
[@&#8203;ssetin](https://redirect.github.com/ssetin) in
[https://github.com/google/go-cloud/pull/3404](https://redirect.github.com/google/go-cloud/pull/3404)
- **rabbitpubsub**: Add query string set the qos prefetch count by
[@&#8203;peczenyj](https://redirect.github.com/peczenyj) in
[https://github.com/google/go-cloud/pull/3431](https://redirect.github.com/google/go-cloud/pull/3431)
- **rabbitpubsub**: Add query string to set the routing key from
metadata by [@&#8203;peczenyj](https://redirect.github.com/peczenyj) in
[https://github.com/google/go-cloud/pull/3433](https://redirect.github.com/google/go-cloud/pull/3433)
- **rabbitpubsub**: Wrap pubsub rabbitmq errors by
[@&#8203;peczenyj](https://redirect.github.com/peczenyj) in
[https://github.com/google/go-cloud/pull/3437](https://redirect.github.com/google/go-cloud/pull/3437)

**docstore**

- **all**: Fix offset handling and extend test coverage by
[@&#8203;bartventer](https://redirect.github.com/bartventer) in
[https://github.com/google/go-cloud/pull/3409](https://redirect.github.com/google/go-cloud/pull/3409)
- **awsdynamodb**: Ensure Next returns EOF when no more items by
[@&#8203;bartventer](https://redirect.github.com/bartventer) in
[https://github.com/google/go-cloud/pull/3406](https://redirect.github.com/google/go-cloud/pull/3406)
- **mongodocstore**: Update Mongo dialer when MONGO_SERVER_URL rotates
by [@&#8203;concaf](https://redirect.github.com/concaf) in
[https://github.com/google/go-cloud/pull/3429](https://redirect.github.com/google/go-cloud/pull/3429)

#### New Contributors

- [@&#8203;ssetin](https://redirect.github.com/ssetin) made their first
contribution in
[https://github.com/google/go-cloud/pull/3404](https://redirect.github.com/google/go-cloud/pull/3404)
- [@&#8203;concaf](https://redirect.github.com/concaf) made their first
contribution in
[https://github.com/google/go-cloud/pull/3429](https://redirect.github.com/google/go-cloud/pull/3429)
- [@&#8203;peczenyj](https://redirect.github.com/peczenyj) made their
first contribution in
[https://github.com/google/go-cloud/pull/3431](https://redirect.github.com/google/go-cloud/pull/3431)
- [@&#8203;chancez](https://redirect.github.com/chancez) made their
first contribution in
[https://github.com/google/go-cloud/pull/3445](https://redirect.github.com/google/go-cloud/pull/3445)
- [@&#8203;milescrabill](https://redirect.github.com/milescrabill) made
their first contribution in
[https://github.com/google/go-cloud/pull/3443](https://redirect.github.com/google/go-cloud/pull/3443)
- [@&#8203;samlaf](https://redirect.github.com/samlaf) made their first
contribution in
[https://github.com/google/go-cloud/pull/3450](https://redirect.github.com/google/go-cloud/pull/3450)

**Full Changelog**:
google/go-cloud@v0.37.0...v0.38.0

</details>

---

### Configuration

📅 **Schedule**: Branch creation - At any time (no schedule defined),
Automerge - At any time (no schedule defined).

🚦 **Automerge**: Enabled.

♻ **Rebasing**: Whenever PR is behind base branch, or you tick the
rebase/retry checkbox.

🔕 **Ignore**: Close this PR and you won't be reminded about this update
again.

---

- [ ] <!-- rebase-check -->If you want to rebase/retry this PR, check
this box

---

This PR was generated by [Mend Renovate](https://mend.io/renovate/).
View the [repository job
log](https://developer.mend.io/github/open-feature/flagd).

<!--renovate-debug:eyJjcmVhdGVkSW5WZXIiOiIzOC43NC4xIiwidXBkYXRlZEluVmVyIjoiMzguNzQuMSIsInRhcmdldEJyYW5jaCI6Im1haW4iLCJsYWJlbHMiOltdfQ==-->

Co-authored-by: renovate[bot] <29139614+renovate[bot]@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants