Add EKS access entry and policy association resources #1273

mbbush · 2024-04-21T03:34:07Z

Description of your changes

Add support for api-based auth in EKS, using eks access entries and access policy associations.

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

Uptest run (the only difference between this and the latest commit is a spelling fix in a code comment)

I'm ~~planning to update the uptest example manifest to include an update test as well.~~ not going to fight with uptest about the current hard-to-use update syntax, and instead will just wait for chainsaw to offer a better way.

johnathan-sq · 2024-05-08T21:57:00Z

Are there any updates on this PR? Keen to see this in main

fernandezcuesta · 2024-05-30T10:25:59Z

I guess the underlying version constraint is gone with 1.5?

haarchri · 2024-05-30T11:20:22Z

Yes

mbbush · 2024-05-30T22:15:16Z

/test-examples="examples/eks/v1beta1/accessentry.yaml"

mbbush · 2024-05-31T05:15:53Z

/test-examples="examples/eks/v1beta1/accessentry.yaml"

package/crds/eks.aws.upbound.io_accesspolicyassociations.yaml

config/externalname.go

mbbush · 2024-05-31T05:31:45Z

config/externalname.go

+// This function sets the keys as IdentifierFields, which means that they are always required, even for observe-only
+// resources. Because the id is constructed exclusively from the keys, omitting them (even if the external name
+// annotation is set) leaves the provider unable to find the terraform id to use to observe the resource.
+func FormattedIdentifierFromProviderWithIdentifierFields(separator string, keys ...string) config.ExternalName {


These function names are getting excessively verbose, but I couldn't think of another option that preserved the "dsl-like" semantics of (most of) the external name config file, didn't introduce a schema change, and could handle the ...string variable arguments.

If anyone has a better idea I'd love to hear it.

When I examined the import sections in the two resources you added, the parameters used in id can be taken from spec.forProvider. In other words, since these are not resources whose identifiers are assigned by the remote client, I think it is better to use TemplatedStringAsIdentifier here if it works.

This function also sets the keys as IdentifierFields, so extra configuration is unnecessary.

That's exactly what I tried as my first external name configuration; see 24a791a. The problem with TemplatedStringAsIdentifier is that because it has DisableNameInitializer: false it initializes the external name to the metadata.name, before updating it to use the value from the template. When I was testing, this caused something to not work; I don't remember what it was.

Your comment about the identifier not really coming from the provider is a good one; that means this is not a good name for this function. Really the essence of what I need is to disable the name initializer. I think the best solution is to add WithDisabledNameInitializer to the ExternalNameFrom helper functions @ulucinar added in crossplane/upjet@47cb5c5

The trouble is that I couldn't find any actual examples of usages of the ExternalNameFrom helpers, so I'll probably have to fumble around a bit in order to figure out the syntax.

If the problem is just DisableNameInitializer: false, we may use this function?

TemplatedStringAsIdentifierWithNoName is only used by two resources, both of which have a provider-generated component to their terraform id which is separated out and stored in the external name. I recall running into some problems with them in the past related to initial creation, which prompted the TODO I put in that function's docs, and I'm re-running their uptest examples to refresh my memory. I'll check back once those tests finish to see what they did.

Do you have a reason for preferring the "Templated" syntax in the external name config over specifying the parameters and the separator? It seems like that just creates more work to reverse-engineer the template string to do things like find identifier fields and look for {{ .external_name }}.

I think that whatever problem I was observing on initial creation for TemplatedStringAsIdentifierWithNoName was only a problem with the cli-forking architecture, as I saw no issues with those resources. I'll remove the TODO comment from that external name config.

Do you have a reason for preferring the "Templated" syntax in the external name config over specifying the parameters and the separator?

I don't have any special reason or desire to use the templated syntax. I don't find it appropriate to add a new function for consistency reasons if we can configure the resource without any problems with the existing external name functions/configurations.

I think that whatever problem I was observing on initial creation for TemplatedStringAsIdentifierWithNoName was only a problem with the cli-forking architecture, as I saw no issues with those resources. I'll remove the TODO comment from that external name config.

From your comment, am I to understand that you will try to continue with TemplatedStringAsIdentifierWithNoName?

mbbush · 2024-05-31T05:40:33Z

config/eks/config.go

+				// Use the terraform id instead of the external name because the external name is set before the cluster
+				// has been created.
+				Extractor: common.PathTerraformIDExtractor,


When I was testing this resource (see the console logs from https://github.com/crossplane-contrib/provider-upjet-aws/actions/runs/9309885236 with the state of the MRs before the cluster creation finishes), I was surprised to see that with the default external name extractor, the reference to the eks cluster was being resolved even before the MR posted a Ready condition. I thought that the Ready check was part of all the cross-resource references, but when I went looking for the code that implemented it I couldn't find anything.

I'm not sure if this is something special about the eks cluster resource, something that would be true of all resources with an external name config that has the name-based initializer enabled, something I'm just misremembering, or a sign of a larger problem.

It seems like it would be especially problematic for those resources which use an external name which is not the same as the metadata.name, but which have an external name config that has the initializer enabled, because then references would resolve first to the metadata.name (in the external name field) and then once the real external name was set, switch to the right value.

turkenf · 2024-06-24T20:27:55Z

/test-examples="examples/eks/v1beta1/accessentry.yaml"

turkenf · 2024-06-25T22:21:05Z

config/externalname.go

 	// my_cluster:my_eks_addon
 	// "aws_eks_addon": config.TemplatedStringAsIdentifier("addon_name", "{{ .parameters.cluster_name }}:{{ .external_name }}"),
 	"aws_eks_addon": FormattedIdentifierFromProvider(":", "cluster_name", "addon_name"),
+	// import EKS access entry using the cluster_name and principal_arn separated by a colon (:).
+	"aws_eks_access_entry": FormattedIdentifierFromProviderWithIdentifierFields(":", "cluster_name", "principal_arn"),


As I mentioned here, could you please try the following:

Suggested change

"aws_eks_access_entry": FormattedIdentifierFromProviderWithIdentifierFields(":", "cluster_name", "principal_arn"),

"aws_eks_access_entry": config.TemplatedStringAsIdentifier("", "{{ .parameters.cluster_name }}:{{ .parameters.principal_arn }}"),

turkenf · 2024-06-25T22:21:39Z

config/externalname.go

+	// import EKS access entry using the cluster_name and principal_arn separated by a colon (:).
+	"aws_eks_access_entry": FormattedIdentifierFromProviderWithIdentifierFields(":", "cluster_name", "principal_arn"),
+	// import EKS access entry using the cluster_name principal_arn and policy_arn separated by a (#) which the tf provider docs incorrectly describe as a colon.
+	"aws_eks_access_policy_association": FormattedIdentifierFromProviderWithIdentifierFields("#", "cluster_name", "principal_arn", "policy_arn"),


Suggested change

"aws_eks_access_policy_association": FormattedIdentifierFromProviderWithIdentifierFields("#", "cluster_name", "principal_arn", "policy_arn"),

"aws_eks_access_policy_association": config.TemplatedStringAsIdentifier("", "{{ .parameters.cluster_name }}#{{ .parameters.principal_arn }}#{{ .parameters.policy_arn }}"),

r-nasiri · 2024-07-13T00:18:08Z

any updates on this? this is a very useful feature specially if you have multiple node groups or need to grant access to multiple IAM roles. updating config map is not fun :)

fhochleitner · 2024-07-22T20:18:09Z

plus one from my side this feature/provider would realy make our lives a lot easier

turkenf · 2024-07-25T10:33:48Z

Hey @mbbush, are you still interested in taking this PR forward?

mbbush · 2024-07-28T02:51:20Z

/test-examples="examples/eks/v1beta1/accessentry.yaml"

mbbush · 2024-07-28T03:10:27Z

/test-examples="examples/eks/v1beta1/accessentry.yaml"

mbbush · 2024-07-29T16:47:51Z

/test-examples="examples/eks/v1beta1/accessentry.yaml"

mbbush · 2024-07-29T17:59:53Z

/test-examples="examples/eks/v1beta1/accessentry.yaml"

mbbush · 2024-07-29T18:51:02Z

@turkenf I've rebased this, updated the schema to use embedded objects, and reran the tests. I renamed the ...FromProvider external name configs which only actually used values from the parameters to be named ...FromParameters, and gave the shorter name to the one that does include identifier fields, as that's what I would expect to be more correct for new resources. We can update the existing resource configurations as needed.

As I mentioned above, the main difference between the current external name config and the TemplatedString one is that this disables the name initializer, which I see as a good thing, since it would only ever set the external name to an incorrect value.

turkenf · 2024-07-29T18:59:36Z

As I mentioned above, the main difference between the current external name config and the TemplatedString one is that this disables the name initializer, which I see as a good thing, since it would only ever set the external name to an incorrect value.

I left a comment above.

@turkenf I've rebased this, updated the schema to use embedded objects, and reran the tests. I renamed the ...FromProvider external name configs which only actually used values from the parameters to be named ...FromParameters, and gave the shorter name to the one that does include identifier fields, as that's what I would expect to be more correct for new resources. We can update the existing resource configurations as needed.

I really appreciate your effort @mbbush, but I think the externalname change should be handled in another PR, even if we are just changing the name.

mbbush · 2024-07-29T19:01:24Z

/test-examples="examples/cloudformation/v1beta1/stack.yaml"

mbbush · 2024-07-29T19:02:10Z

/test-examples="examples/appsync/v1beta1/apikey.yaml"

turkenf · 2024-08-01T18:56:21Z

Hey @mbbush, kindly reminder: #1273 (comment)

turkenf · 2024-08-06T20:49:01Z

/test-examples="examples/eks/v1beta1/accessentry.yaml"

mbbush · 2024-08-07T04:53:18Z

@turkenf My plan for handling this was actually to remove TemplatedStringAsIdentifierWithNoName, as the two resources that use it work just as well (or better) using TemplatedStringAsProviderDefinedIdentifier, which has the added benefits of a name that makes sense with what it does, and not setting the external name to an invalid value (the template with an empty string in the .external_name part) during creation, as TemplatedStringAsIdentifierWithNoName does for templates that contain {{ .external_name }}.

I guess it's true that those changes can be done later in a non-breaking way, even if we release this code as it is after your 8a301f3 commit. I just find that I'm having less time lately for such improvements, and that when I do, those sorts of "general improvement" PRs often linger unreviewed for a long time.

Maybe the best thing to do at this point is to just merge this and release it so we can finally support this feature that I started implementing 3 months ago.

Signed-off-by: Matt Bush <[email protected]>

So that the refs don't resolve until after the cluster exists Signed-off-by: Matt Bush <[email protected]>

So that crossplane doesn't initialize the external name with the incorrect value before updating it to the correct value. Signed-off-by: Matt Bush <[email protected]>

Signed-off-by: Matt Bush <[email protected]>

…nd AccessEntry resources Signed-off-by: Fatih Türken <[email protected]>

turkenf · 2024-08-08T11:40:51Z

/test-examples="examples/eks/v1beta1/accessentry.yaml"

https://github.com/crossplane-contrib/provider-upjet-aws/actions/runs/10301382270

mergenci

Thanks @mbbush and @turkenf. LGTM.

mbbush force-pushed the eks-access branch 2 times, most recently from 8fc48f7 to 1e85602 Compare May 30, 2024 21:11

mbbush commented May 31, 2024

View reviewed changes

package/crds/eks.aws.upbound.io_accesspolicyassociations.yaml Show resolved Hide resolved

mbbush commented May 31, 2024

View reviewed changes

config/externalname.go Outdated Show resolved Hide resolved

mbbush commented May 31, 2024

View reviewed changes

mbbush marked this pull request as ready for review May 31, 2024 05:51

mbbush requested review from ulucinar, sergenyalcin and turkenf as code owners May 31, 2024 05:51

mbbush force-pushed the eks-access branch from 171ad95 to 3aa9039 Compare June 5, 2024 18:10

turkenf reviewed Jun 25, 2024

View reviewed changes

mbbush force-pushed the eks-access branch from 3aa9039 to 497ed16 Compare July 28, 2024 02:51

mbbush force-pushed the eks-access branch from 6b32c2b to cf6a41e Compare July 29, 2024 17:55

mbbush force-pushed the eks-access branch from cf6a41e to edd741b Compare July 29, 2024 18:03

mbbush requested a review from turkenf July 29, 2024 18:48

mbbush force-pushed the eks-access branch from edd741b to 0971d23 Compare July 29, 2024 20:40

mbbush and others added 10 commits August 8, 2024 14:39

Add eks_access_entry and eks_access_policy_association

e39a9f3

Signed-off-by: Matt Bush <[email protected]>

Configure references for eks access policy/entry

49f5cf1

Signed-off-by: Matt Bush <[email protected]>

example for accessentry

33ca0da

Signed-off-by: Matt Bush <[email protected]>

fixup example names

1df4ce3

Signed-off-by: Matt Bush <[email protected]>

switch to terraform id extractor for cluster references

65821af

So that the refs don't resolve until after the cluster exists Signed-off-by: Matt Bush <[email protected]>

Use FormattedIdentifierFromProvider external name config

2940690

So that crossplane doesn't initialize the external name with the incorrect value before updating it to the correct value. Signed-off-by: Matt Bush <[email protected]>

codegen

4f19e2f

Signed-off-by: Matt Bush <[email protected]>

fix formatting

4c5321f

Signed-off-by: Matt Bush <[email protected]>

Rename new external name function

ca8d73c

Signed-off-by: Matt Bush <[email protected]>

Change external name configurations for the AccessPolicyAssociation a…

0d8e1c4

…nd AccessEntry resources Signed-off-by: Fatih Türken <[email protected]>

turkenf force-pushed the eks-access branch from 8a301f3 to 0d8e1c4 Compare August 8, 2024 11:39

mergenci approved these changes Aug 8, 2024

View reviewed changes

turkenf merged commit 5a775c6 into crossplane-contrib:main Aug 8, 2024
12 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add EKS access entry and policy association resources #1273

Add EKS access entry and policy association resources #1273

mbbush commented Apr 21, 2024 •

edited

Loading

johnathan-sq commented May 8, 2024

fernandezcuesta commented May 30, 2024

haarchri commented May 30, 2024

mbbush commented May 30, 2024

mbbush commented May 31, 2024

mbbush May 31, 2024

turkenf Jun 25, 2024 •

edited

Loading

mbbush Jul 28, 2024

mbbush Jul 28, 2024

turkenf Jul 29, 2024

mbbush Jul 29, 2024

mbbush Jul 29, 2024

turkenf Jul 30, 2024

mbbush May 31, 2024 •

edited

Loading

turkenf commented Jun 24, 2024

turkenf Jun 25, 2024

turkenf Jun 25, 2024

r-nasiri commented Jul 13, 2024

fhochleitner commented Jul 22, 2024

turkenf commented Jul 25, 2024

mbbush commented Jul 28, 2024

mbbush commented Jul 28, 2024

mbbush commented Jul 29, 2024

mbbush commented Jul 29, 2024

mbbush commented Jul 29, 2024 •

edited

Loading

turkenf commented Jul 29, 2024

mbbush commented Jul 29, 2024

mbbush commented Jul 29, 2024

turkenf commented Aug 1, 2024

turkenf commented Aug 6, 2024

mbbush commented Aug 7, 2024 •

edited

Loading

turkenf commented Aug 8, 2024 •

edited

Loading

mergenci left a comment

	"aws_eks_access_entry": FormattedIdentifierFromProviderWithIdentifierFields(":", "cluster_name", "principal_arn"),
	"aws_eks_access_entry": config.TemplatedStringAsIdentifier("", "{{ .parameters.cluster_name }}:{{ .parameters.principal_arn }}"),

	"aws_eks_access_policy_association": FormattedIdentifierFromProviderWithIdentifierFields("#", "cluster_name", "principal_arn", "policy_arn"),
	"aws_eks_access_policy_association": config.TemplatedStringAsIdentifier("", "{{ .parameters.cluster_name }}#{{ .parameters.principal_arn }}#{{ .parameters.policy_arn }}"),

Add EKS access entry and policy association resources #1273

Add EKS access entry and policy association resources #1273

Conversation

mbbush commented Apr 21, 2024 • edited Loading

Description of your changes

How has this code been tested

johnathan-sq commented May 8, 2024

fernandezcuesta commented May 30, 2024

haarchri commented May 30, 2024

mbbush commented May 30, 2024

mbbush commented May 31, 2024

Choose a reason for hiding this comment

turkenf Jun 25, 2024 • edited Loading

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

mbbush May 31, 2024 • edited Loading

Choose a reason for hiding this comment

turkenf commented Jun 24, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

r-nasiri commented Jul 13, 2024

fhochleitner commented Jul 22, 2024

turkenf commented Jul 25, 2024

mbbush commented Jul 28, 2024

mbbush commented Jul 28, 2024

mbbush commented Jul 29, 2024

mbbush commented Jul 29, 2024

mbbush commented Jul 29, 2024 • edited Loading

turkenf commented Jul 29, 2024

mbbush commented Jul 29, 2024

mbbush commented Jul 29, 2024

turkenf commented Aug 1, 2024

turkenf commented Aug 6, 2024

mbbush commented Aug 7, 2024 • edited Loading

turkenf commented Aug 8, 2024 • edited Loading

mergenci left a comment

Choose a reason for hiding this comment

mbbush commented Apr 21, 2024 •

edited

Loading

turkenf Jun 25, 2024 •

edited

Loading

mbbush May 31, 2024 •

edited

Loading

mbbush commented Jul 29, 2024 •

edited

Loading

mbbush commented Aug 7, 2024 •

edited

Loading

turkenf commented Aug 8, 2024 •

edited

Loading