Ability to override ID operations #132

muvaf · 2021-11-02T18:45:49Z

Description of your changes

Allows users to override the default TF State ID -> External Name and External Name -> TF State ID flows. Note that we already have SetIdentifierArgumentFn endpoint but that's for overriding External Name -> Main TF File. Details about why we're doing this are in the issue.

Depends on #128 . See the last 3 commits for actual diff.

Fixes #119
Fixes #104

I have:

Read and followed Crossplane's [contribution process].
Run make reviewable to ensure this PR is ready for review.
Added backport release-x.y labels to auto-backport this PR if necessary.

How has this code been tested

Unit tests and see crossplane-contrib/provider-jet-azure#84

turkenh · 2021-11-04T20:06:25Z

@muvaf would this mean, users no longer reliably use Terraform import id as an external name? I was liking that what to use as an external name is documented for all resources, especially for import cases.

If we chose to still use Terraform import ids as external names, I believe we could cover all cases with two functions as below:

CR (metadata.name + spec) => identifier attributes (e.g. attributes["id"] and attributes["bucket-name"] set)
attributes["id"] -> CR (external-name and spec)

I think this would mean us not marking some (required) fields as not required so that they can be late initialized during import.

muvaf · 2021-11-11T22:14:08Z

Synced with @turkenh about his proposal and we decided not to proceed with it because it'd mean to make external name same as ID which is not acceptable in some cases especially in composition where you may not have some parts of the information stored in the ID, such as subscriptionID in Azure.

@ulucinar @turkenh I added your suggestion about the provider config content and opened crossplane-contrib/provider-jet-azure#84 to show how it all would look like.

…nversions Signed-off-by: Muvaffak Onus <me@muvaf.com>

…Terraformed interface Signed-off-by: Muvaffak Onus <me@muvaf.com>

… flow now Signed-off-by: Muvaffak Onus <me@muvaf.com>

…f the id to handle cases where project, region, subscription id etc. is included in the id Signed-off-by: Muvaffak Onus <me@muvaf.com>

Signed-off-by: Muvaffak Onus <me@muvaf.com>

pkg/config/defaults.go

turkenh · 2021-11-17T11:33:40Z

pkg/resource/sensitive.go

+	// just a nice-to-have, hence we don't handle cases where the ID is given
+	// in a field other than "id". cfg.ExternalName.GetNameFn wouldn't really
+	// help because we need the raw ID.
+	if id, ok := attr["id"]; conn != nil && ok {


Should we consider dropping including id to connection secrets?

I remember we've added this to support cases where id field is required as a connection detail, e.g. id in aws_iam_access_key. But now we have a way to include any information from the tf schema as a conn detail with AdditionalConnectionDetailsFn.

Looks like we pass Terraform attribute map to AdditionalConnectionDetailsFn. id might incorporate addition info, such as the subscription id for Azure resources, which may not be available in any other attributes. We should consider this in the discussion.

@ulucinar The attribute map we pass to AdditionalConnectionDetailsFn is the full TFState, which is where we pull the id from in this logic. So it should be available in that map. Maybe we can rename the argument name of that function from attr to tfstate to make it more clear.

@turkenh I thought about removing it but then ended up publishing it only if connection detail secret has some content so that we don't publish a connection secret for every resource, thinking that id of RDS Instance could be helpful for connecting for example but I do expect developers to write an AdditionalConnectionDetailsFn for connectible resources in all cases anyway. I'll just remove it.

pkg/config/resource.go

turkenh · 2021-11-17T12:34:39Z

pkg/config/resource.go

+}
+
+// GetNameFn returns the external name extracted from the TF State.
+type GetNameFn func(tfstate map[string]interface{}) string


I believe returning errors in these functions would be helpful for debugging by making what went wrong more explicit, i.e. instead returning an empty string.

:) I have similar concerns here as I detailed above.

Added it. I also added ctx to GetIDFn so that we make queries there, like getting account id by making get-caller-identity call in AWS to construct the full ARN.

ulucinar · 2021-11-17T13:37:47Z

pkg/config/resource.go

+
+// GetIDFn returns the ID to be used in TF State file, i.e. "id" field in
+// terraform.tfstate.
+type GetIDFn func(name string, parameters map[string]interface{}, providerConfig map[string]interface{}) string


I was thinking of the providerConfig map as a more generalized key-value map from which specific implementations can extract the needed information, i.e., a "producer" puts key,value pairs to be consumed by a specific instance of the GetIDFn via known keys. I think we currently augment the context only using the ProviderConfigs so the parameter is named as providerConfig. We may rename it with something like configMap so that it conveys a more generalized meaning.

Also here my understanding is that an error case (probably a programming error case in which a known key is not put into this map and the consumer, GetIDFn function instance, cannot construct the ID) signals it by returning an empty string. This will cause FileProducer.WriteTFState to set the generated tfstate's id attribute to an empty string. How does Terraform behave in this case? If Terraform errors in the above case, it's a better scenario but the error message will be indirect, i.e., instead of stating that an expected key could not be found in a configuration map, we might observe something like "resource not exists", or similar. Or, does it behave in undesired ways? Do we have an observation for this situation? It makes sense to detect and properly report a missing piece of expected information while calculating the id attribute via an error. Thus we may consider returning an error here and breaking early before running any Terraform commands in FileProducer.WriteTFState.

i.e., a "producer" puts key,value pairs to be consumed by a specific instance of the GetIDFn via known keys.

Could you elaborate? Right now, we can only give generic information to this function like those given, and if users want to add more they can do that in runtime by returning anonymous function that take value from somewhere else.

Thus we may consider returning an error here and breaking early before running any Terraform commands in FileProducer.WriteTFState.

Thanks, added.

ulucinar · 2021-11-17T13:42:17Z

pkg/config/resource.go

+type GetIDFn func(name string, parameters map[string]interface{}, providerConfig map[string]interface{}) string
+
+// NameAsID returns the name to be used as ID in TF State file.
+var NameAsID GetIDFn = func(name string, _ map[string]interface{}, _ map[string]interface{}) string {


nit: Type specification in the initializer is redundant.

I added them on purpose actually so that we get compilation errors here right away if they don't adhere to those types. Otherwise, they'd remain as anonymous functions until used somewhere to return compile error.

ulucinar · 2021-11-17T13:45:04Z

pkg/config/resource.go

+}
+
+// GetNameFn returns the external name extracted from the TF State.
+type GetNameFn func(tfstate map[string]interface{}) string


:) I have similar concerns here as I detailed above.

pkg/config/resource.go

ulucinar · 2021-11-17T13:57:24Z

pkg/config/resource.go

+
+// IDAsName returns the TF State ID as external name.
+var IDAsName GetNameFn = func(tfstate map[string]interface{}) string {
+	if id, ok := tfstate["id"].(string); ok {


nit: The ok bool checks the type assertion here, i.e., the string assertion. We will be returning an empty string if id does not exist, or the map value is not of the expected type. This is not an issue under our current assumptions, which may be broken in the future due to bugs or breaking changes. This is related to the above discussion on error reporting. It may prove to be useful to differentiate missing keys and unexpected value types here.

Added error reporting and made sure we return error if id is ""

ulucinar · 2021-11-17T14:18:57Z

pkg/config/resource.go

+
+	// GetIDFn returns the string that will be used as "id" key in TF state. In
+	// most cases, it should return given external name as is.
+	GetIDFn GetIDFn


nit: The immediate context here is (the enclosing type) the ExternalName struct, which is a Crossplane concept. I think it will be more explicit to rename this field as GetTerraformIDFn.

I'd actually prefer to keep it as ID since that's what it's called in the map that this function processes, similarly how ExternalName corresponds to crossplane.io/external-name annotation.

ulucinar · 2021-11-17T14:25:51Z

pkg/resource/lateinit.go

@@ -53,7 +54,7 @@ type GenericLateInitializer struct {
 }

 // LateInitializeAnnotations late initializes annotations of the resource
-func LateInitializeAnnotations(tr Terraformed, attr map[string]interface{}, privateRaw string) (bool, error) {
+func LateInitializeAnnotations(tr metav1.Object, name string, privateRaw string) (bool, error) {


nit: As mentioned above in one of @turkenh's comments, it makes sense to rename name parameter as externalName to make it more explicit.

ulucinar · 2021-11-17T14:30:04Z

pkg/resource/sensitive.go

 	if err != nil {
 		return nil, errors.Wrap(err, "cannot get connection details")
 	}

-	var custom map[string][]byte
-	// TODO(turkenh): Once we have automatic defaulting, remove this if check.


Is the idea here that we will always have a non-zero AdditionalConnectionDetailsFn because of the defaulting? I would still prefer to keep this check so that a nil AdditionalConnectionDetailsFn continues to behave as a noop.

See my comment in #132 (comment)

ulucinar · 2021-11-17T14:37:35Z

pkg/resource/sensitive.go

+	// just a nice-to-have, hence we don't handle cases where the ID is given
+	// in a field other than "id". cfg.ExternalName.GetNameFn wouldn't really
+	// help because we need the raw ID.
+	if id, ok := attr["id"]; conn != nil && ok {


Looks like we pass Terraform attribute map to AdditionalConnectionDetailsFn. id might incorporate addition info, such as the subscription id for Azure resources, which may not be available in any other attributes. We should consider this in the discussion.

ulucinar · 2021-11-17T14:43:12Z

pkg/terraform/files.go

-	if fp.Config.ExternalName.SetIdentifierArgumentFn != nil {
-		fp.Config.ExternalName.SetIdentifierArgumentFn(params, meta.GetExternalName(tr))
-	}
+	fp.Config.ExternalName.SetIdentifierArgumentFn(params, meta.GetExternalName(tr))


I think this was a common pattern we wanted to switch to after defaulting. Please see the above comment.

…ecret since it is already available to be added by additional connections fn Signed-off-by: Muvaffak Onus <me@muvaf.com>

…ample cases Signed-off-by: Muvaffak Onus <me@muvaf.com>

Signed-off-by: Muvaffak Onus <me@muvaf.com>

muvaf · 2021-11-17T22:40:07Z

@ulucinar @turkenh I think I have responded to all comments and made changes, ready for another look 🙂

turkenh

LGTM, thanks @muvaf !

pkg/config/defaults_test.go

…to be made, it can be done. For example, account ID in AWS to construct the full ARN Signed-off-by: Muvaffak Onus <me@muvaf.com>

muvaf requested review from ulucinar and turkenh November 2, 2021 18:45

muvaf force-pushed the name-me-well branch 3 times, most recently from c970efb to ba2a5cd Compare November 9, 2021 11:36

This was referenced Nov 11, 2021

Proper error reporting and eventing #44

Closed

Override ID operations crossplane-contrib/provider-jet-azure#84

Closed

muvaf mentioned this pull request Nov 15, 2021

Generating a Provider guide #130

Merged

3 tasks

muvaf added 5 commits November 17, 2021 13:37

external name: added customization endpoints for id<>external name co…

ef337ae

…nversions Signed-off-by: Muvaffak Onus <me@muvaf.com>

external name: fix tests and remove GetTerraformResourceIDField from …

157dcaf

…Terraformed interface Signed-off-by: Muvaffak Onus <me@muvaf.com>

config: remove id field configuration since we can override the whole…

0da8e09

… flow now Signed-off-by: Muvaffak Onus <me@muvaf.com>

config.externalname: supply provider config as well for calculation o…

0055e70

…f the id to handle cases where project, region, subscription id etc. is included in the id Signed-off-by: Muvaffak Onus <me@muvaf.com>

config: add unit tests for DefaultResource

c077333

Signed-off-by: Muvaffak Onus <me@muvaf.com>

muvaf force-pushed the name-me-well branch from ea22c96 to c077333 Compare November 17, 2021 10:40

turkenh reviewed Nov 17, 2021

View reviewed changes

pkg/config/resource.go Outdated Show resolved Hide resolved

turkenh reviewed Nov 17, 2021

View reviewed changes

pkg/config/resource.go Outdated Show resolved Hide resolved

turkenh reviewed Nov 17, 2021

View reviewed changes

ulucinar reviewed Nov 17, 2021

View reviewed changes

muvaf added 4 commits November 18, 2021 00:44

resource.sensitive: remove addition of id information to connection s…

16ae3f3

…ecret since it is already available to be added by additional connections fn Signed-off-by: Muvaffak Onus <me@muvaf.com>

config.externalname: expand documentation of the fields to include ex…

e684934

…ample cases Signed-off-by: Muvaffak Onus <me@muvaf.com>

config.externalname: rename usages of name to externalName

924507c

Signed-off-by: Muvaffak Onus <me@muvaf.com>

config.externalname: make external name overrides return error

36c9793

Signed-off-by: Muvaffak Onus <me@muvaf.com>

turkenh approved these changes Nov 18, 2021

View reviewed changes

pkg/config/defaults_test.go Show resolved Hide resolved

config.externalname: add context to GetIDFn so that if a query needs …

8e96bd1

…to be made, it can be done. For example, account ID in AWS to construct the full ARN Signed-off-by: Muvaffak Onus <me@muvaf.com>

muvaf force-pushed the name-me-well branch from 4d1ef88 to 8e96bd1 Compare November 18, 2021 09:59

muvaf merged commit 6abd89a into crossplane:main Nov 18, 2021

muvaf deleted the name-me-well branch November 18, 2021 10:05

turkenh mentioned this pull request Jan 7, 2022

Update Configuring a Resource guide #187

Merged

2 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Ability to override ID operations #132

Ability to override ID operations #132

muvaf commented Nov 2, 2021 •

edited

Loading

turkenh commented Nov 4, 2021

muvaf commented Nov 11, 2021

turkenh Nov 17, 2021

ulucinar Nov 17, 2021

muvaf Nov 17, 2021

turkenh Nov 17, 2021

ulucinar Nov 17, 2021

muvaf Nov 17, 2021

ulucinar Nov 17, 2021

muvaf Nov 17, 2021

ulucinar Nov 17, 2021

muvaf Nov 17, 2021

ulucinar Nov 17, 2021

ulucinar Nov 17, 2021

muvaf Nov 17, 2021

ulucinar Nov 17, 2021

muvaf Nov 17, 2021

ulucinar Nov 17, 2021

ulucinar Nov 17, 2021

muvaf Nov 17, 2021

ulucinar Nov 17, 2021

ulucinar Nov 17, 2021

muvaf commented Nov 17, 2021

turkenh left a comment

Ability to override ID operations #132

Ability to override ID operations #132

Conversation

muvaf commented Nov 2, 2021 • edited Loading

Description of your changes

How has this code been tested

turkenh commented Nov 4, 2021

muvaf commented Nov 11, 2021

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

muvaf commented Nov 17, 2021

turkenh left a comment

Choose a reason for hiding this comment

muvaf commented Nov 2, 2021 •

edited

Loading