-
Notifications
You must be signed in to change notification settings - Fork 501
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
[JUJU-3866] UpdateOffer: validate endpoints before running DB transaction #15677
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you.
Does the update offer cli print a nice error message like for adding an offer?
Adds logic to the validateOfferArgs method to check that the application and endpoints exist in state. No need for the explicit names.IsValidApplication check anymore, as this is already done inside state.Application. Also add a regression test and improve our test coverage here, to ensure we are rejecting offers which reference invalid applications or endpoints.
// Sanity checks. | ||
if !names.IsValidApplication(offer.ApplicationName) { | ||
return errors.NotValidf("application name %q", offer.ApplicationName) | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This check is no longer needed - as it is done inside state.Application
@@ -476,7 +491,7 @@ func (s *applicationOffersSuite) TestUpdateApplicationOfferNotFound(c *gc.C) { | |||
owner := s.Factory.MakeUser(c, nil) | |||
_, err := sd.UpdateOffer(crossmodel.AddApplicationOfferArgs{ | |||
OfferName: "hosted-mysql", | |||
ApplicationName: "foo", | |||
ApplicationName: "mysql", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This test began to fail for a different reason (application "foo" doesn't exist), due to the added validation checks. We need to ensure the application actually exists but the offer does not.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, ty!
/merge |
#15710 Forward ports: - #15676 - #15672 - #15683 - #15673 - #15677 - #15691 - #15701 Conflicts: - apiserver/errors/errors.go - apiserver/facades/client/application/application.go - apiserver/facades/client/application/application_unit_test.go - apiserver/restrict_newer_client_test.go - go.mod - go.sum - scripts/win-installer/setup.iss - snap/snapcraft.yaml - state/migration_import_test.go - state/mocks/description_mock.go - version/version.go
#15717 Merge 3.1 #15676 [from wallyworld/newer-clients-migrate](7c1f884) #15672 [from hpidcock/bump-juju-description-v3.0.15](acec126) #15683 [from hpidcock/fix-persistent-storage-test](460dd21) #15673 [from barrettj12/check-merge](5c253c3) #15677 [from barrettj12/invalid-offer](3cb3f8b) #15654 [from ycliuhw/fix/backendRefCount](4e5ae3c) #15692 [from wallyworld/fix-secrets-cmr](a1fb0c4) [t](840bc09) #15709 [from wallyworld/hook-secret-revison](840bc09) #15701 [from hpidcock/fix-upgrade-podspec-sidecar](d465c93) #15681 [from anvial/JUJU-3882-fix-test-deploy-test-…](f54c1ad) #15714 [from wallyworld/offer-consume=sameapp](6390036) Conflicts were upgrade steps - the 3.1.3 step has been moved to 3.2.1. Also an auth tweak to crossmodelrelaltions. ``` # Conflicts: # apiserver/common/crossmodel/auth_test.go # apiserver/facades/controller/crossmodelrelations/crossmodelrelations.go # rpc/params/apierror.go # state/upgrades.go # state/upgrades_test.go # upgrades/backend.go # ``` ## QA steps See PRs
#15719 Merge 3.2 No conflicts. Upgrade steps were deleted. #15636 [from jack-w-shaw/JUJU-3822_replace_ubuntu_w…](1a2759d) #15650 [from SimonRichardson/fix-machine-test-logs-aws](c316413) #15652 [from barrettj12/ojson](4436840) #15653 [from barrettj12/teardown-logs](ee886dd) #15633 [from anvial/JUJU-3821-fix-test-deploy-aks-c…](0bb6df2) #15657 [from anvial/JUJU-3842-fix-deploy-aks-clean-…](f16dd80) #15660 [from hpidcock/ignore-delete-network-interface](686b6f2) #15661 [from hpidcock/fix-data-race-getosfromseries](f639f09) #15662 [from hpidcock/fix-relation-departing-test](68ef945) #15663 [from hpidcock/fix-test-branch](c14bcb3) #15313 [from tlm/teardown-2.0](243fd66) #15542 [from masnax/project-fix](8ba03ec) #15676 [from wallyworld/newer-clients-migrate](7c1f884) #15672 [from hpidcock/bump-juju-description-v3.0.15](acec126) #15683 [from hpidcock/fix-persistent-storage-test](460dd21358378e7d1204348c37a9ba26e49b5871)https://github.com/juju/juju/pull/15673 [from barrettj12/check-merge](5c253c3) #15677 [from barrettj12/invalid-offer](3cb3f8b) #15654 [from ycliuhw/fix/backendRefCount](4e5ae3c) #15692 [from wallyworld/fix-secrets-cmr](a1fb0c4) #15709 [from wallyworld/hook-secret-revison](840bc09) #15701 [from hpidcock/fix-upgrade-podspec-sidecar](d465c93) #15681 [from anvial/JUJU-3882-fix-test-deploy-test-…](f54c1ad) #15573 [from jack-w-shaw/JUJU-3447_implement_ModelF…](f7082ca) #15714 [from wallyworld/offer-consume=sameapp](6390036) ## QA steps See PRs
#15721 Merge 3.3 #15636 [from jack-w-shaw/JUJU-3822_replace_ubuntu_w…](1a2759d) #15650 [from SimonRichardson/fix-machine-test-logs-aws](c316413) #15652 [from barrettj12/ojson](4436840) #15653 [from barrettj12/teardown-logs](ee886dd) #15633 [from anvial/JUJU-3821-fix-test-deploy-aks-c…](0bb6df2) #15657 [from anvial/JUJU-3842-fix-deploy-aks-clean-…](f16dd80) #15660 [from hpidcock/ignore-delete-network-interface](686b6f2) #15661 [from hpidcock/fix-data-race-getosfromseries](f639f09) #15662 [from hpidcock/fix-relation-departing-test](68ef945) #15663 [from hpidcock/fix-test-branch](c14bcb3) #15313 [from tlm/teardown-2.0](243fd66) #15542 [from masnax/project-fix](8ba03ec) #15676 [from wallyworld/newer-clients-migrate](7c1f884) #15672 [from hpidcock/bump-juju-description-v3.0.15](acec126) #15683 [from hpidcock/fix-persistent-storage-test](460dd21358378e7d1204348c37a9ba26e49b5871)https://github.com/juju/juju/pull/15673 [from barrettj12/check-merge](5c253c3) #15677 [from barrettj12/invalid-offer](3cb3f8b) #15654 [from ycliuhw/fix/backendRefCount](4e5ae3c) #15692 [from wallyworld/fix-secrets-cmr](a1fb0c4) #15709 [from wallyworld/hook-secret-revison](840bc09) #15701 [from hpidcock/fix-upgrade-podspec-sidecar](d465c93) #15681 [from anvial/JUJU-3882-fix-test-deploy-test-…](f54c1ad) #15573 [from jack-w-shaw/JUJU-3447_implement_ModelF…](f7082ca) #15714 [from wallyworld/offer-consume=sameapp](6390036) Conflicts ``` # Conflicts: # apiserver/facades/client/client/client_test.go # featuretests/package_test.go # ``` ## QA steps See PRs
When updating an existing offer using an invalid endpoint, Juju was returning the expected error
However, it was still internally updating the offer in the database. This left the database in an inconsistent state, where the
applicationOffers
table contained an offer which referenced a nonexistent endpoint. This pretty much breaks the model to the point where you can't even runjuju status
. See LP#1954830.We already had a function
validateOfferArgs
to validate the offer, but this didn't include checking that the application and endpoints exist in state. That responsibility was left to themakeApplicationOffer
method, but forUpdateOffer
, this is run after the DB transaction - so it was no good.This PR fixes the
validateOfferArgs
method, so it now also checks that the requested application and endpoints exist in state. We had an explicitnames.IsValidApplication
check which is no longer needed, as this is already done insidestate.Application
.I've also improved our test coverage here, adding tests to check that
AddOffer
andUpdateOffer
fail if the application or endpoints don't exist. This includes a regression test for the bug.Checklist
[ ] Comments saying why design decisions were made[ ] Integration tests, with comments saying what you're testing[ ] doc.go added or updated in changed packagesQA steps
Then, do a db dump:
and check that the offer in the
applicationOffers
table still refers to the original valid endpointsink
.Bug reference
https://bugs.launchpad.net/juju/+bug/1954830