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.

Sign up for GitHub

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

Jump to bottom

Improve Terraform Provider integration testing #1329

Merged
merged 2 commits into from
Aug 28, 2024
Merged

Improve Terraform Provider integration testing #1329

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
3be45e8
tweaks to improve integration testing
kian99 Aug 27, 2024
16aa360
Merge branch 'v3' into further-integration-tweaks
kian99 Aug 28, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
11 changes: 9 additions & 2 deletions .github/actions/test-server/action.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -80,8 +80,11 @@ runs:
run: echo "name=$CONTROLLER_NAME" >> $GITHUB_OUTPUT
shell: bash

- name: Install jimmctl and yq
run: sudo snap install jimmctl --channel=3/stable && sudo snap install yq
- name: Install jimmctl, jaas plugin and yq
run: |
sudo snap install jimmctl --channel=3/stable && \
sudo snap install jaas --channel=3/stable &&
sudo snap install yq
shell: bash

- name: Authenticate Juju CLI
Expand All @@ -97,3 +100,7 @@ runs:
env:
JIMM_CONTROLLER_NAME: "jimm"
CONTROLLER_NAME: ${{ steps.lxd-controller.outputs.name }}

- name: Provide service account with cloud-credentials
run: ./local/jimm/setup-service-account.sh
shell: bash
2 changes: 1 addition & 1 deletion compose-common.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -37,7 +37,7 @@ services:
JIMM_OAUTH_CLIENT_SECRET: "SwjDofnbDzJDm9iyfUhEp67FfUFMY8L4"
JIMM_OAUTH_SCOPES: "openid profile email" # Space separated list of scopes
JIMM_DASHBOARD_FINAL_REDIRECT_URL: "https://jaas.ai" # Example URL
JIMM_ACCESS_TOKEN_EXPIRY_DURATION: 1h
JIMM_ACCESS_TOKEN_EXPIRY_DURATION: 100h
Copy link
Contributor

Choose a reason for hiding this comment

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

Why?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

To make local dev less tedious, you don't need to repeatedly login.

Copy link
Contributor

Choose a reason for hiding this comment

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

Ah oki I'm with you

JIMM_SECURE_SESSION_COOKIES: false
JIMM_SESSION_COOKIE_MAX_AGE: 86400
JIMM_SESSION_SECRET_KEY: Xz2RkR9g87M75xfoumhEs5OmGziIX8D88Rk5YW8FSvkBPSgeK9t5AS9IvPDJ3NnB
Expand Down
6 changes: 3 additions & 3 deletions internal/jimm/cache.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -44,7 +44,7 @@ func (d *cacheDialer) Dial(ctx context.Context, ctl *dbmodel.Controller, mt name
return d.dialer.Dial(ctx, ctl, mt, requiredPermissions)
}
rc := d.sfg.DoChan(ctl.Name, func() (interface{}, error) {
return d.dial(ctx, ctl)
return d.dial(ctx, ctl, requiredPermissions)
Copy link
Contributor

Choose a reason for hiding this comment

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

How did this work before without the permissions?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

There are default permissions set in the dialer so that probably resolves most issues because we don't add extra permissions in many places, and I believe the cache dialer is not used unless actually running JIMM?
We also probably want to get rid of the cache dialer, but this is still better than before.

Copy link
Contributor

Choose a reason for hiding this comment

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

I think I need to look over the default permissions, never really fully understood the permission map and what goes where... But oki thank you!

})
select {
case r := <-rc:
Expand All @@ -57,7 +57,7 @@ func (d *cacheDialer) Dial(ctx context.Context, ctl *dbmodel.Controller, mt name
}
}

func (d *cacheDialer) dial(ctx context.Context, ctl *dbmodel.Controller) (interface{}, error) {
func (d *cacheDialer) dial(ctx context.Context, ctl *dbmodel.Controller, requiredPermissions map[string]string) (interface{}, error) {
d.mu.Lock()
capi, ok := d.conns[ctl.Name]
if ok {
Expand All @@ -73,7 +73,7 @@ func (d *cacheDialer) dial(ctx context.Context, ctl *dbmodel.Controller) (interf
d.mu.Unlock()

// We don't have a working connection to the controller, so dial one.
api, err := d.dialer.Dial(ctx, ctl, names.ModelTag{}, nil)
api, err := d.dialer.Dial(ctx, ctl, names.ModelTag{}, requiredPermissions)
if err != nil {
return nil, err
}
Expand Down
6 changes: 4 additions & 2 deletions internal/jujuclient/applicationoffers.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -209,13 +209,15 @@ func (c Connection) GetApplicationOfferConsumeDetails(ctx context.Context, user
OfferURLs: []string{info.Offer.OfferURL},
BakeryVersion: v,
},
UserTag: user.String(),
// Do not include a user in the args, Juju will opt to use the user authenticated in the connection.
// There is a bug where setting the user tag does not behave as expected.
UserTag: "",
}

resp := jujuparams.ConsumeOfferDetailsResults{
Results: make([]jujuparams.ConsumeOfferDetailsResult, 1),
}
err := c.CallHighestFacadeVersion(ctx, "ApplicationOffers", []int{4, 3}, "", "GetConsumeDetails", &args, &resp)
err := c.CallHighestFacadeVersion(ctx, "ApplicationOffers", []int{5, 4}, "", "GetConsumeDetails", &args, &resp)
kian99 marked this conversation as resolved.
Show resolved Hide resolved
if err != nil {
return errors.E(op, jujuerrors.Cause(err))
}
Expand Down
4 changes: 3 additions & 1 deletion internal/rpc/proxy.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -515,7 +515,9 @@ func checkPermissionsRequired(ctx context.Context, msg *message) (map[string]any

// Check for errors that may be a result of a bulk request.
for _, e := range er.Results {
zapctx.Debug(ctx, "received error", zap.Any("error", e))
if e.Error != nil {
zapctx.Debug(ctx, "received error", zap.Any("error", e.Error))
}
if e.Error != nil && e.Error.Code == accessRequiredErrorCode {
for k, v := range e.Error.Info {
accessLevel, ok := v.(string)
Expand Down
13 changes: 13 additions & 0 deletions local/jimm/setup-service-account.sh
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,13 @@
#!/bin/bash

# This script is used to setup a service account by adding a set of cloud-credentials.
# Default values below assume a lxd controller is added to JIMM.

set -eux

SERVICE_ACCOUNT_ID="${SERVICE_ACCOUNT_ID:-test-client-id}"
CLOUD="${CLOUD:-localhost}"
CREDENTIAL_NAME="${CREDENTIAL_NAME:-localhost}"

juju add-service-account "$SERVICE_ACCOUNT_ID"
juju update-service-account-credential "$SERVICE_ACCOUNT_ID" "$CLOUD" "$CREDENTIAL_NAME"
Loading