feat: add juju_access_offer resource #633

amandahla · 2024-11-19T21:22:54Z

Description

This is a PR for adding the juju_access_offer resource based on schema described in #627

Type of change

<What type of a change is this? Please keep one or more relevant lines from below and delete the others.>

Add new resource

Environment

Juju controller version: 3.5
Terraform version: 1.7.4

QA steps

Manual QA steps should be done to test this PR.

provider juju {}
...

Additional notes

<Please add relevant notes & information, links to mattermost chats, other related issues/PRs, anything to help understand and QA the PR.>

Since Update changes the ID, I had to set Access as a RequiresReplace. Otherwise, tests fail with:

=== RUN   TestAcc_ResourceAccessOffer
=== PAUSE TestAcc_ResourceAccessOffer
=== CONT  TestAcc_ResourceAccessOffer
    resource_access_offer_test.go:27: Step 3/4 error: Error running apply: exit status 1
        
        Error: Provider produced inconsistent result after apply
        
        When applying changes to juju_access_offer.access_prometheus_endpoint,
        provider "provider[\"registry.terraform.io/hashicorp/juju\"]" produced an
        unexpected new value: .id: was
        cty.StringVal("admin/tf-access-model-7830324085062068677.prometheus-k8s:consume:tfuser-4861434009413829244"),
        but now
        cty.StringVal("admin/tf-access-model-7830324085062068677.prometheus-k8s:admin:tfuser-4861434009413829244").
        
        This is a bug in the provider, which should be reported in the provider's own
        issue tracker.
--- FAIL: TestAcc_ResourceAccessOffer (40.70s)
FAIL
FAIL	github.com/juju/terraform-provider-juju/internal/provider	40.719s
FAIL

alesstimec

some initial comments. happy to talk about this, if needed.

internal/provider/resource_access_offer.go

alesstimec · 2024-12-06T14:18:34Z

internal/provider/resource_access_offer.go

+			},
+			"users": schema.SetAttribute{
+				Description: "List of users to grant access.",
+				Optional:    true,


why is this optional? without users, does an access offer resource make sense?

All attributes regarding users are optional but there is one validator for checking if at least one is set.

alesstimec · 2024-12-06T14:30:43Z

internal/provider/resource_access_offer.go

+	}
+
+	// Get information from ID
+	offerURL, _, _ := retrieveAccessOfferDataFromID(ctx, state.ID, state.Users, &resp.Diagnostics)


why ignore the access level from offer ID. i think you should use it to filter out users that don't have the matching access level.

alesstimec · 2024-12-06T14:32:14Z

internal/provider/resource_access_offer.go

+			continue
+		}
+		users = append(users, offerUserDetail.UserName)
+		if access != "" && access != string(offerUserDetail.Access) {


again, i don't think this is quite right. imagine an application offer where alice has admin access, bob has consume access and eve has read access, which is quite a valid state for an application offer. the way this condition is written, this would produce an error here, because we see three different access levels for the same offer.

internal/provider/resource_access_offer.go

alesstimec · 2024-12-06T14:53:21Z

internal/juju/offers.go

+	return nil
+}
+
+// This function revokes access to an offer.


This function revokes a specific level of access to an offer.

I think (correct me if i'm wrong), but access levels for offers work a bit differently in juju.
e.g. if a user has admin access to an offer and we revoke admin access for that user, the user is left with consume access to the offer.. if we revoke consume access from an admin user, that user is left with read access. and if we revoke read access, user is left with no access to the offer.

@alesstimec you are correct in how revoke on offer works.

The provider should decide which level to grant or revoke, not the internal juju code making the API call.

@hmlanigan yes, but with the current schema, where juju_access_offer only lists users for a single access level, this is nearly impossible, since you can have a juju_access_offer resource for the consume level and another one for read level and the two don't "know" about eachother.

Also the godoc is not formatted correctly.

.github/workflows/test_integration.yml

hmlanigan · 2024-12-06T21:42:30Z

docs/resources/access_offer.md

+
+A resource that represent a Juju Access Offer.
+
+


todo: add examples/resource/juju_access_offer directory with resource.tf and import.sh files, content will be added to this markdown file during make install.

hmlanigan · 2024-12-06T21:44:16Z

internal/provider/resource_access_offer.go

+	"github.com/juju/juju/core/crossmodel"
+	"github.com/juju/names/v5"


todo: move the github.com/juju imports to the second stanza, in sorted order.

hmlanigan · 2024-12-06T21:50:30Z

internal/provider/resource_access_offer.go

+		resp.Diagnostics.AddError(
+			"ImportState Failure",
+			fmt.Sprintf("Malformed AccessOffer ID %q, "+
+				"please use format '<offer URL>:<access>:<user1,user1>'", IDstr),


thought: we're having issues with cross controller CMR today in this provider, however an offer URL with a controller uses a colon, e.g. foo:admin/another-model.postgresql. A different delineator would be a good idea.

hmlanigan · 2024-12-06T22:11:52Z

internal/provider/resource_access_offer.go

+	// Get user/access info from Offer
+	response, err := a.client.Offers.ReadOffer(&juju.ReadOfferInput{
+		OfferURL: offerURL,
+	})
+	if err != nil {
+		resp.Diagnostics.AddError("Client Error", fmt.Sprintf("Unable to read offer, got error: %s", err))
+		return
+	}
+	a.trace(fmt.Sprintf("read juju offer %q", offerURL))
+	offerUsers := response.Users


This data is available in state:

var state accessOfferResourceOffer // Get the current state resp.Diagnostics.Append(req.State.Get(ctx, &state)...)

Doing it this way has the potential to change access levels for the offer which were not set via this resource.

hmlanigan · 2024-12-06T22:21:49Z

internal/juju/offers.go

+	return nil
+}
+
+// This function revokes access to an offer.


@alesstimec you are correct in how revoke on offer works.

The provider should decide which level to grant or revoke, not the internal juju code making the API call.

hmlanigan · 2024-12-06T22:24:04Z

internal/provider/resource_access_offer.go

+		offerUserNames[offerUser.UserName] = struct{}{}
+		if !slices.Contains(planUsers, offerUser.UserName) {
+			a.trace(fmt.Sprintf("revoke user %q for offer %q", offerUser.UserName, offerURL))
+			err := a.client.Offers.RevokeOffer(&juju.GrantRevokeOfferInput{


If a user has been removed from the resource, then to remove their privileges, you'd Revoke read. Afaik, it's not necessary to step through the levels.

I've also suggested not allowing the admin user as a valid user to avoid problems there.

hmlanigan · 2024-12-06T22:26:36Z

internal/juju/offers.go

+	_, err = client.ApplicationOffer(input.OfferURL)
+	if err != nil {
+		return err
+	}


No need to do this. RevokeOffer will validate that the offer exists.

hmlanigan · 2024-12-06T22:26:57Z

internal/juju/offers.go

+	_, err = client.ApplicationOffer(input.OfferURL)
+	if err != nil {
+		return err
+	}


No need to do this. GrantOffer will validate that the offer exists.

hmlanigan · 2024-12-06T22:34:20Z

internal/provider/resource_access_offer.go

+	users := []string{}
+	var access string
+	for _, offerUserDetail := range response.Users {
+		if offerUserDetail.UserName == "everyone@external" || offerUserDetail.UserName == "admin" {


todo: please document in the schema description the admin user will not be handled via this resource. Suggest failing the validation of the user set as well if admin is specified.

ale8k · 2024-12-10T08:38:44Z

internal/juju/offers.go

+type GrantRevokeOfferInput struct {
+	User     string
+	Access   string
+	OfferURL string


I think this should be URL's, you can revoke many at once

Since we discussed changing the schema in the last terraform office hours, I think this no longer apply, WDYT?

ale8k · 2024-12-10T08:39:09Z

internal/juju/offers.go

+	return nil
+}
+
+// This function revokes access to an offer.


Also the godoc is not formatted correctly.

ale8k · 2024-12-10T08:40:20Z

internal/juju/offers.go

+		return err
+	}
+
+	err = client.RevokeOffer(input.User, input.Access, input.OfferURL)


This should probably be (if we can make the schema look nice) input.OfferURLs

amandahla marked this pull request as draft November 19, 2024 21:23

amandahla force-pushed the feat/offer-access branch from 99e33e1 to 3b7b636 Compare November 21, 2024 21:16

amandahla marked this pull request as ready for review November 28, 2024 21:21

amandahla changed the title ~~WIP feat: add juju_access_offer resource~~ feat: add juju_access_offer resource Nov 28, 2024

amandahla force-pushed the feat/offer-access branch 2 times, most recently from 29c1c47 to df0e20b Compare December 4, 2024 13:40

alesstimec requested review from hmlanigan and anvial December 6, 2024 14:13

alesstimec requested changes Dec 6, 2024

View reviewed changes

hmlanigan reviewed Dec 6, 2024

View reviewed changes

ale8k reviewed Dec 10, 2024

View reviewed changes

amandahla force-pushed the feat/offer-access branch from df0e20b to 3fa9531 Compare December 11, 2024 13:10

amandahla added 10 commits December 12, 2024 15:11

feat: add juju_access_offer resource

aa8035f

feat: add create and read to access_offer

851a95c

feat: add delete

cd7da1a

feat: add update

540c520

feat: add tests

0ccd379

fix: fix jaas test

2119bba

fix: replace prometheus charm for juju-qa-dummy-source

42cd2c6

fix: fix resourcename and offerurl

ee944cd

fix: add godocs

c21ac36

feat: change the schema (proposal)

35e236a

amandahla force-pushed the feat/offer-access branch from 3fa9531 to 35e236a Compare December 12, 2024 20:55

fix: add configvalidator

138874f

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: add juju_access_offer resource #633

feat: add juju_access_offer resource #633

amandahla commented Nov 19, 2024 •

edited

Loading

alesstimec left a comment

alesstimec Dec 6, 2024

amandahla Dec 13, 2024

alesstimec Dec 6, 2024

alesstimec Dec 6, 2024

alesstimec Dec 6, 2024

hmlanigan Dec 6, 2024

alesstimec Dec 9, 2024

ale8k Dec 10, 2024

hmlanigan Dec 6, 2024

hmlanigan Dec 6, 2024

hmlanigan Dec 6, 2024

hmlanigan Dec 6, 2024

hmlanigan Dec 6, 2024

hmlanigan Dec 6, 2024

hmlanigan Dec 6, 2024

hmlanigan Dec 6, 2024

hmlanigan Dec 6, 2024

ale8k Dec 10, 2024

amandahla Dec 13, 2024

ale8k Dec 10, 2024

ale8k Dec 10, 2024

		"github.com/juju/juju/core/crossmodel"
		"github.com/juju/names/v5"

feat: add juju_access_offer resource #633

Are you sure you want to change the base?

feat: add juju_access_offer resource #633

Conversation

amandahla commented Nov 19, 2024 • edited Loading

Description

Type of change

Environment

QA steps

Additional notes

alesstimec left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

amandahla commented Nov 19, 2024 •

edited

Loading