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

Merge 3.1 #15717

Merged
merged 46 commits into from
Jun 8, 2023
Merged

Merge 3.1 #15717

merged 46 commits into from
Jun 8, 2023

Conversation

wallyworld
Copy link
Member

Merge 3.1

#15676 from wallyworld/newer-clients-migrate
#15672 from hpidcock/bump-juju-description-v3.0.15
#15683 from hpidcock/fix-persistent-storage-test
#15673 from barrettj12/check-merge
#15677 from barrettj12/invalid-offer
#15654 from ycliuhw/fix/backendRefCount
#15692 from wallyworld/fix-secrets-cmr
t #15709 from wallyworld/hook-secret-revison
#15701 from hpidcock/fix-upgrade-podspec-sidecar
#15681 from anvial/JUJU-3882-fix-test-deploy-test-…
#15714 from wallyworld/offer-consume=sameapp

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

hpidcock and others added 30 commits May 30, 2023 10:28
juju#15676

We now allow a juju 3.1 CLI to run things like status and migrations against a 2.9 controller.
Also tweak the error message to use more accurate language when trying to use an incompatible client.

## QA steps

bootstrap a 2.9 controller and add a model
install the 3.1 snap
use the 3.1 snap cli to run status
check the 3.1 client cannot deploy etc
bootstrap a 3.1 controller
use the 3.1 snap cli to migrate a model to a 3.1 controller
…0.15

juju#15672

Bumps juju/description/v3 dependency to v3.0.15 as it corrects the application description version to be compatible with juju/description/v4

## QA steps

migrate model to another controller

## Documentation changes

N/A

## Bug reference

N/A
juju#15683

Trusty doesn't seem to work anymore on AWS with 2.9, so this moves to preferring jammy for persistent storage tests.

## QA steps

`BOOTSTRAP_CLOUD=aws BOOTSTRAP_PROVIDER=ec2 ./main.sh -v -s '"test_charm_storage"' storage test_persistent_storage`

## Documentation changes

N/A

## Bug reference

https://jenkins.juju.canonical.com/job/test-storage-test-persistent-storage-aws/760/consoleText
juju#15673

When a PR is merged, check if it creates merge conflicts. If so, notify the PR author on Mattermost.
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.
juju#15677

When updating an existing offer using an invalid endpoint, Juju was returning the expected error
```
ERROR cannot update application offer "prometheus": getting relation endpoint for relation "http" and application "prometheus": application "prometheus" has no "http" relation
```
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 run `juju status`. See [LP#1954830](https://bugs.launchpad.net/juju/+bug/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 the `makeApplicationOffer` method, but for `UpdateOffer`, 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 explicit `names.IsValidApplication` check which is no longer needed, as this is already done inside `state.Application`.

I've also improved our test coverage here, adding tests to check that `AddOffer` and `UpdateOffer` fail if the application or endpoints don't exist. This includes a regression test for the bug.

## Checklist

- [x] Code style: imports ordered, good names, simple structure, etc
- ~[ ] Comments saying why design decisions were made~
- [x] Go unit tests, with comments saying what you're testing
- ~[ ] [Integration tests](https://github.com/juju/juju/tree/main/tests), with comments saying what you're testing~
- ~[ ] [doc.go](https://discourse.charmhub.io/t/readme-in-packages/451) added or updated in changed packages~

## QA steps

```sh
# rebuild juju, update microk8s operator image, etc
juju bootstrap (lxd|microk8s) c
juju deploy juju-qa-dummy-source d
juju offer d:sink # valid endpoint
juju offer d:sink1 # invalid endpoint
juju status # should work as normal
```

Then, do a db dump:
```
JUJU_DEV_FEATURE_FLAGS='developer-mode' juju dump-db > db-dump.yaml
```
and check that the offer in the `applicationOffers` table still refers to the original valid endpoint `sink`.


## Bug reference

https://bugs.launchpad.net/juju/+bug/1954830
…not be removed unless the backend gets removed;
juju#15654

This PR fixes secret backend refCount issues for:
- model import(increment on target controller)/export(decrement on source controller) during model migration;
- model removal;
- model migration abort;
- secret drain;

drive-by: 
- added upgrade steps to create at least one refCount for each secret backend;
- fixed a secret backend model config issue for model migration(including the backend ID in the model data for importing process to handle a potential backend renaming in the target controller);
- fixed a potential data race for the secret drain worker that could happen if the secret backend changed too frequently in a short time period.

## Checklist

- [x] Code style: imports ordered, good names, simple structure, etc
- [x] Comments saying why design decisions were made
- [x] Go unit tests, with comments saying what you're testing
- [ ] ~[Integration tests](https://github.com/juju/juju/tree/main/tests), with comments saying what you're testing~
- [ ] ~[doc.go](https://discourse.charmhub.io/t/readme-in-packages/451) added or updated in changed packages~

## QA steps

```sh
juju add-secret-backend myvault vault --config ./vault.yaml

juju deploy snappass-test

for i in {1..5}; do juju exec --unit snappass-test/0 -- secret-add --owner unit owned-by=easyrsa/0; done

for i in {1..5}; do juju exec --unit snappass-test/0 -- secret-add owned-by=easyrsa; done

juju model-config secret-backend=myvault

juju model-config secret-backend
myvault

export backend_id=$(juju show-secret-backend myvault | yq .myvault.id)

juju bootstrap microk8s k2

juju add-secret-backend myvault1 vault --config ./vault.yaml --import-id $backend_id

juju switch k1:t1

juju migrate k1:t1 k2

juju switch k2:t1

juju model-config secret-backend
myvault1

# check refCount in mongo on the source controller;
juju:PRIMARY> db.globalRefcounts.find({_id: "secretbackend#revisions#64782241fe5c050027770143"}).pretty()
{
 "_id" : "secretbackend#revisions#64782241fe5c050027770143",
 "refcount" : 0,
 "txn-revno" : NumberLong(11)
}

# check refCount in mongo on the target controller;
juju:PRIMARY> db.globalRefcounts.find({_id: "secretbackend#revisions#64782241fe5c050027770143"}).pretty()
{
 "_id" : "secretbackend#revisions#64782241fe5c050027770143",
 "refcount" : 8,
 "txn-revno" : NumberLong(3)
}

juju remove-secret-backend myvault1
ERROR backend "myvault1" still contains secret content

juju destroy-model -y --debug --destroy-storage k2:t1

# check refCount in mongo on target controller again;
juju:PRIMARY> db.globalRefcounts.find({_id: "secretbackend#revisions#64782241fe5c050027770143"}).pretty()
{
 "_id" : "secretbackend#revisions#64782241fe5c050027770143",
 "refcount" : 0,
 "txn-revno" : NumberLong(3)
}

juju remove-secret-backend myvault1

```

## Documentation changes

No

## Bug reference

No
jujubot and others added 15 commits June 2, 2023 15:40
juju#15692

The consumer secrets watcher used in the remote relations worker for cmr is broken due to fixes made to the offer side remote application proxy. The offer uuid is not stored on the proxy any more as the offer uuid must be determined from the relation. To that end, the relation tag is now passed when setting up the watcher. A small cleanup of the auth context is also done, requiring that source model and offer uuids be passed into the auth calls.

To allow for backwards compatibility with older controllers in a multi-controller cmr scenario, we extract the offer uuid from the macaroon.

## QA steps

create a model for an offer

juju deploy juju-qa-dummy-source
juju offer dummy-source:sink
juju config dummy-source token=foo

create a model for the consumer 

juju deploy juju-qa-dummy-sink
juju relate dummy-sink controller.dummy-source

check that the apps are idle and active

create a secret in the offering model

juju exec --unit dummy-source/0 -- secret-add foo=bar
juju exec --unit dummy-source/0 -- secret-grant -r 0 secret://dcbb8270-42ff-4d15-8e42-843a6a8e49d8/chsn02ip43ljshc2j7j0

check that it can be read

juju exec --unit dummy-sink/0 -- secret-get secret://dcbb8270-42ff-4d15-8e42-843a6a8e49d8/chsn02ip43ljshc2j7j0

update the secret

juju exec --unit dummy-source/0 -- secret-set secret://dcbb8270-42ff-4d15-8e42-843a6a8e49d8/chsn02ip43ljshc2j7j0 foo=bar2

check the consuming charm has run the secret-changed hook and can ee the new value

 juju show-status-log dummy-sink/0
 juju exec --unit dummy-sink/0 -- secret-get secret://dcbb8270-42ff-4d15-8e42-843a6a8e49d8/chsn02ip43ljshc2j7j0 --peek


Also deploy a 3.1.2 controller and add consuming app and relate to offer.

## Bug reference

https://bugs.launchpad.net/bugs/2021969
juju#15709

When creating the hook content for secret hooks, we were only setting the revision if the hook was secret-remove. However, it also needs to be set for secret-expired.

The logic was a little wrong - it has been changed so that if the hook info has revision set, it will always be used for any secret hook. We were also incorrectly setting revision for the secret-changed hook, so this has been fixed.

## QA steps

Deploy a charm and create a secret which expires in a minute.
Echo the JUJU_SECRET_REVISION env var in the secret-expired hook and see that it is set correctly.

## Bug reference

https://bugs.launchpad.net/juju/+bug/2023120
juju#15701

Upgrading from pospec charms to sidecar charms can be problematic when the unit numbers don't match the ordinal pod numbers. This one time change forces the users to scale the application to 0 before performing this particular charm upgrade. This has the side benefit of surfacing previous behaviour where the application would be unavailable during the charm upgrade from podspec to sidecar. This is now obvious because the admin must first scale the application down.

## QA steps

```
$ juju bootstrap microk8s
$ juju add-model a
$ juju deploy oidc-gatekeeper --revision 176 --channel ckf-1.7/stable
$ # wait for oidc-gatekeeper to settle down
$ juju remove-application oidc-gatekeeper
$ # wait for oidc-gatekeeper to disappear
$ juju deploy oidc-gatekeeper --revision 176 --channel ckf-1.7/stable
$ # wait for oidc-gatekeeper to settle down
$ juju refresh oidc-gatekeeper --channel latest/edge
Added charm-hub charm "oidc-gatekeeper", revision 213 in channel latest/edge, to the model
ERROR Upgrading from an older PodSpec style charm to a newer Sidecar charm requires that
the application be scaled down to 0 units.

Before refreshing the application again, you must scale it to 0 units and wait for
all those units to disappear before continuing.

 juju scale-application oidc-gatekeeper 0
$ juju scale-application oidc-gatekeeper 0
$ # wait for oidc-gatekeeper units to disappear
$ juju refresh oidc-gatekeeper --channel latest/edge
$ juju scale-application oidc-gatekeeper 1
$ # app should be upgraded
```

## Documentation changes

Document upgrade from podspec to sidecar charm that they need to be scaled to 0 first.

## Bug reference

https://bugs.launchpad.net/juju/+bug/2023117
…est-deploy-bundles-aws-in-3-1

juju#15681

This PR updates the test environment by replacing the usage of third-party charms with juju-team managed charms. 

## Checklist


- [x] Code style: imports ordered, good names, simple structure, etc
- [ ] Comments saying why design decisions were made

## QA steps

```sh
cd tests
./main.sh -v -p ec2 deploy run_deploy_cmr_bundle
```
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
juju#15710

Forward ports:
- juju#15676
- juju#15672
- juju#15683
- juju#15673
- juju#15677
- juju#15691
- juju#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
juju#15714

Fixes a cross model relations corner case:

Model 1
app A
offer app A

Model 2
app B
app C
offer app C

Model 1
relate A->C

Model 2
relate B->A

The unit relation hooks for C do not fire.
The root cause is that only one cmr token for app A is created and used for both the offer relation and consumer relation. So model 2 gets a duplicate remote entity token and the resolution of token to entity tag fails for the C->A relation.

The fix is to create the token for offering relations keyed on application offer tag, not application tag. This allows the same app to be in both an offering and consuming relation to a given model since different tokens are created for the different roles of the app.

There was also an unused app token parameter in the ingress address change watcher which was removed. This is lucky as it avoids the need to look up offers in the firewaller worker.

## QA steps

I used modified dummy-source and dummy-sink charms with extra endpoints to set up a cross model scenario as per the description. After the relations were created, show-status-log on each of the various units showed relation created and joined hooks being run. Previously, the relation joined hooks for one of the units would have been missing.


## Bug reference

https://bugs.launchpad.net/juju/+bug/2022855
juju#15716

Merge 2.9

[Merge pull request](juju@6390036) juju#15714 [from wallyworld/offer-consume=sameapp](juju@6390036)

Conflicts was new auth param.

```
# Conflicts:
# apiserver/common/crossmodel/crossmodel.go
# apiserver/facades/controller/crossmodelrelations/crossmodelrelations.go
# apiserver/facades/controller/remoterelations/remoterelations.go
#
```

## QA steps

See PRs
@wallyworld wallyworld force-pushed the merge-3.1-20230608 branch 2 times, most recently from cccf72e to 46ec088 Compare June 8, 2023 13:12
@wallyworld
Copy link
Member Author

/merge

@jujubot jujubot merged commit b53560a into juju:3.2 Jun 8, 2023
17 of 19 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
6 participants