Optional Blobstore agent creds #2327
Conversation
| if_p('blobstore.agent.user') do | ||
| all_users[p('blobstore.agent.user')] = p('blobstore.agent.password') | ||
| end |
There was a problem hiding this comment.
I thought that we want to make these properties depend on blobstore.enable_signed_urls?
There was a problem hiding this comment.
If blobstore.agent.user is not set and blobstore.enable_signed_urls is false it should raise an error. In order to shorten the operator feedback loop in the case of a misconfiguration.
There was a problem hiding this comment.
I think we shouldn't configure the blobstore.agent.user if blobstore.enable_signed_urls is set to true.
The agent doesn't need this credential anymore in this case. And from a security perspective, it's better to not configure it so that you don't need to rotate it.
We currently have the issue that some CPIs still reference the blobstore.agent.user, blobstore.agent.password even though they don't use it if blobstore.enable_signed_urls is set to true.
Till these CPIs have been updated, we could set blobstore.agent.user, blobstore.agent.password in the manifest and still wouldn't need to rotate the password, since it is not configured in the blobstore.
There was a problem hiding this comment.
There was a problem hiding this comment.
there is a need for the blobstore to accept both, until all agents have received their new configuration.
Is this really the case?
I thought the agent only downloads blobs from the blobstore, when the director sends a request and doesn't store or reuse previous blobstore URLs.
I assume that after the blobstore.enable_signed_urls config is set to true on the director side, the agents only use signed blobstore URLs.
There was a problem hiding this comment.
When mixing old and new stemcells it could in theory be that both signed URLs and non agent blobstore credentials are used at the same time: https://github.com/cloudfoundry/bosh/blob/master/src/bosh-director/lib/bosh/blobstore_client/base.rb#L122-L125
Apparently the ubuntu-xenial/456.x line (which is still supported for VMware customers) does not support singed URLs (API version 2).
Stemcell builder was bumped to API version 3 in this commit: cloudfoundry/bosh-linux-stemcell-builder@f4de1ec
Co-authored-by: Oliver Mautschke <oliver.mautschke@sap.com>
bgandon
force-pushed
the
optional-blobstore-agent-creds
branch
from
911daaa
to
2edea39
Compare
|
I've just pushed these improvements:
|
jobs/blobstore/spec
Outdated
| @@ -34,9 +34,9 @@ properties: | |||
| description: Password director must use to access blobstore via HTTP Basic | |||
|
|
|||
| blobstore.agent.user: | |||
| description: Username agents must use to access blobstore via HTTP Basic | |||
| description: Username agents must use to access blobstore via HTTP Basic (optional, if provided the password is mandatory) | |||
There was a problem hiding this comment.
Should we say hier something like "optional if blobstore.enable_signed_urls is used, ..."
There was a problem hiding this comment.
Yes, sure. I suddenly realise how ambiguous is the “if provided the password is mandatory” sentence… and it's unnecessary to tell there anyway. So, I've changed that.
|
@bgandon having something about the ERB test is really good. We could even add a script for this into https://github.com/cloudfoundry/bosh/tree/master/scripts to make it more easier reference that in the docs. |
|
Good idea. I've added a dedicated script for running the ERB unit tests. |
docs/running_tests.md
Outdated
|
|
||
| Install the Gem dependencies. | ||
| ``` | ||
| (cd src && bundle install) |
There was a problem hiding this comment.
Why these commands are in parenthesis?
There was a problem hiding this comment.
In order for them to run in a sub-shell, so that on return the current shell remains in the same current working directory.
There was a problem hiding this comment.
What about using bundle install --gemfile=./src/Gemfile, which is more intuitive and it is closer to what we want to achieve here? I didn't know the effect of the parenthesis and I just executed the command without them. My confusion comes from there :-)
|
I've pushed some changes to take your remarks into account @beyhan |
|
LGTM, Thanks @bgandon |
What is this change about?
Here we have the agent credentials for local Blobstore become optional, because implementing signed URLs means they are not required anymore.
Please provide contextual information.
When operators opt for signed URLs, they should not be forced to specify any username and password for the agent to access the blobstore.
Maybe worth to note, Pivotal/VMware has had a similar related story, left unfinished though: https://www.pivotaltracker.com/n/projects/956238/stories/169966865
Related PRs
What tests have you run against this PR?
Unit tests for the BOSH Release templates are passing, as executed with
(cd src && bundle exec rspec ../spec).How should this change be described in bosh release notes?
Does this PR introduce a breaking change?
No breaking change is introduced here, but missing agent credentials with a
blobstore.enable_signed_urlsoffalseis a new potential pitfall.Tag your pair, your PM, and/or team!
Co-Authored-By: @OliverMautschke