Add validation for credentials in SB update to be not blank#2939
Conversation
svkrieger
force-pushed
the
issue-2937-fix-broker-lock-on-missing-credentials
branch
from
1c9a2fe to
b0a9989
Compare
| context 'when authentication is not valid' do | ||
| let(:request_body) do | ||
| valid_body[:authentication] = valid_body[:authentication].except(:type) | ||
| valid_body | ||
| end | ||
|
|
||
| before do | ||
| allow_any_instance_of(VCAP::CloudController::Validators::AuthenticationValidator).to receive(:validate) do |_, record| | ||
| record.errors.add(:authentication, 'this authentication is not valid!') | ||
| end | ||
| it 'is not valid' do |
There was a problem hiding this comment.
This is just a fix of a test which was wrong.
Context is 'when authentication is not valid' but the test ran with a valid_body. It was just green because the passed body didn't matter, because the validation was mocked. I changed the body to include an invalid authentication object by removing the mandatory type field. Also I removed the mock and checked for the correct error message, which is thrown by the validation in this case.
| context 'when authentication is not valid' do | ||
| let(:request_body) do | ||
| valid_body[:authentication] = valid_body[:authentication].except(:type) | ||
| valid_body | ||
| end | ||
|
|
||
| before do | ||
| allow_any_instance_of(VCAP::CloudController::Validators::AuthenticationValidator).to receive(:validate) do |_, record| | ||
| record.errors.add(:authentication, 'this authentication is not valid!') | ||
| end | ||
| it 'is not valid' do | ||
| expect(message).not_to be_valid | ||
| expect(message.errors_on(:authentication)).to include('authentication.type must be one of ["basic"]') | ||
| end |
There was a problem hiding this comment.
Same fix as in the other test file.
| # Make setting the state to 'AVAILABLE' fail, but nothing else | ||
| if record.state == 'AVAILABLE' | ||
| # Make setting the name to 'new-name' fail, but nothing else | ||
| if record.name == 'new-name' |
There was a problem hiding this comment.
So far this test tested the only case in which the state of the SB could be reset to its previous state after a failed model validation. The broker update failed here. From then on the broker object will still have state = "AVAILABLE". In the rescue call, the broker's state will be explicitly set to the previous state and will overwrite the state AVAILABLE, which caused the model violation to fail (see here).
After changing the cause for the model violation to the name new-name, the test failed. This is because the broker.name will still be set to new-name, which previously caused the model violation. This leads to a failed broker.update in the rescue block as well. Therefore, the state cannot be reset to the previous state.
| rescue => e | ||
| begin | ||
| broker.update(state: previous_broker_state) | ||
| initial_broker.update(state: previous_broker_state) |
There was a problem hiding this comment.
This PR fixes the issue, that the previous state could not be reset. Though I think it does not always make sense to let the broker return to the previous state.
For example when @catalog_updater.refresh fails to contact the broker to update the catalog and the previous_state was AVAILABLE, the broker would return to state AVAILABLE even it is not reachable at the moment. I think in this case setting the broker's state to SYNCHRONIZATION_FAILED would be more appropriate. This still allows for broker.update requests, but will let all service related operations fail fast.
svkrieger
force-pushed
the
issue-2937-fix-broker-lock-on-missing-credentials
branch
2 times, most recently
from
b645146 to
cd23682
Compare
svkrieger
force-pushed
the
issue-2937-fix-broker-lock-on-missing-credentials
branch
from
cd23682 to
c43a572
Compare
3 participants
Thanks for contributing to cloud_controller_ng. To speed up the process of reviewing your pull request please provide us with:
This PR consists of two fixes.
One is adding a validation check to make sure that credentials, which have been sent with a SB update request, are not empty strings. From now on
usernameandpasswordmust be non-empty strings, otherwise the validation will fail. This coding is executed by the API processes.The other ensures that after a model layer validation error, the SB can be reset to its previous state. This coding is executed by the CC worker processes and is run during the delayed job execution of the broker.update job.
The issue is explained in #2937.
Registering a broker in CF with empty credentials never worked. In V2 and V3 an error will be returned. So it is okay to make non-empty credentials obligatory. The OSBAPI Spec says "While the communication between a Platform and Service Broker MAY be unsecure, it is RECOMMENDED that all communications between a Platform and a Service Broker are secured via TLS and authenticated."(source)
I have reviewed the contributing guide
I have viewed, signed, and submitted the Contributor License Agreement
I have made this pull request to the
mainbranchI have run all the unit tests using
bundle exec rakeI have run CF Acceptance Tests