Cache secret info #2168

justinabrahms · 2025-02-12T17:41:17Z

Description of your changes

As best I can tell, we're not actually storing anything relevant in the ExternalObservation struct, so we end up with a secret with zero bytes.

I didn't actually see Endpoint in the underlying observer object. There were a list of cache nodes. I don't think this is a solution to all of the problems, but I hope this is a step in the right direction.

Refs #2167

I have:

Read and followed Crossplane's contribution process.
Run make reviewable test to ensure this PR is ready for review.

That ^^ isn't a valid command

How has this code been tested

I've not tested this. I'm not clear how. I'm actively seeking help and understanding here. I'm even open to running this in my dev cluster if there's a reasonable process to do that.

Signed-off-by: Justin Abrahms <[email protected]>

…ve method Signed-off-by: Justin Abrahms <[email protected]>

Signed-off-by: Justin Abrahms <[email protected]>

… is what we're after? Signed-off-by: Justin Abrahms <[email protected]>

anagarlau · 2025-02-12T21:26:49Z

Hi. The provider can be tested in and out of cluster.

For quick local testing/outside a k8s cluster:
Please refer to the following docs: https://github.com/crossplane-contrib/provider-aws/blob/master/INSTALL.md?plain=1#L47 (the make submodules task should be run first - though, cf. code-gen for details). You can also comment out any controllers which are not relevant to the test in the setup-file

In cluster:
For builds, use the make build task, cf. pipeline
Push to a package registry of your choice using the crossplane cli

Hope this helps!

justinabrahms · 2025-02-13T06:12:46Z

Thanks @anagarlau, that's helpful. When I run locally, I get linting errors unrelated to my code when running make reviewable. It seems to be some sort of dependency issue in addition to an issue around referencing incorrect types. Not clear if this is something on my end or something wrong with the master branch. I attempted to comment out things in the setup file, but it still built all the CRDs for it and fails on linting.

[..snip..]
21:49:46 [ .. ] golangci-lint
apis/servicediscovery/v1alpha1/custom_types.go:67:19: in.GetAnnotations undefined (type *HTTPNamespace has no field or method GetAnnotations) (typecheck)
	if val, ok := in.GetAnnotations()[AnnotationKeyOperationID]; ok {
	                 ^
apis/servicediscovery/v1alpha1/custom_types.go:95:19: in.GetAnnotations undefined (type *PrivateDNSNamespace has no field or method GetAnnotations) (typecheck)
	if val, ok := in.GetAnnotations()[AnnotationKeyOperationID]; ok {
	                 ^
apis/servicediscovery/v1alpha1/custom_types.go:126:19: in.GetAnnotations undefined (type *PublicDNSNamespace has no field or method GetAnnotations) (typecheck)
	if val, ok := in.GetAnnotations()[AnnotationKeyOperationID]; ok {
	                 ^
apis/v1alpha1/storeconfig_types.go:68:19: in.Status.GetCondition undefined (type StoreConfigStatus has no field or method GetCondition) (typecheck)
	return in.Status.GetCondition(ct)
	                 ^
apis/v1alpha1/storeconfig_types.go:73:12: in.Status.SetConditions undefined (type StoreConfigStatus has no field or method SetConditions) (typecheck)
	in.Status.SetConditions(c...)
	          ^
pkg/controller/batch/job/controller.go:294:13: undefined: smithy (typecheck)
	var awsErr smithy.APIError
	           ^
pkg/controller/batch/jobdefinition/controller.go:215:13: undefined: smithy (typecheck)
	var awsErr smithy.APIError
	           ^
pkg/controller/rds/dbinstance/setup.go:290:2: desiredPassword declared and not used (typecheck)
	desiredPassword, err := dbinstance.GetDesiredPassword(ctx, e.kube, cr)
	^
cmd/provider/main.go:50:22: undefined: kingpin (typecheck)
		app              = kingpin.New(filepath.Base(os.Args[0]), "AWS support for Crossplane.").DefaultEnvars()
		                   ^
cmd/provider/main.go:61:2: undefined: kingpin (typecheck)
	kingpin.MustParse(app.Parse(os.Args[1:]))
	^
cmd/provider/main.go:78:2: undefined: kingpin (typecheck)
	kingpin.FatalIfError(err, "Cannot get API server rest config")
	^
cmd/provider/main.go:98:2: undefined: kingpin (typecheck)
	kingpin.FatalIfError(err, "Cannot create controller manager")
	^
cmd/provider/main.go:99:2: undefined: kingpin (typecheck)
	kingpin.FatalIfError(apis.AddToScheme(mgr.GetScheme()), "Cannot add AWS APIs to scheme")
	^
cmd/provider/main.go:114:3: undefined: kingpin (typecheck)
		kingpin.FatalIfError(resource.Ignore(kerrors.IsAlreadyExists, mgr.GetClient().Create(context.Background(), &v1alpha1.StoreConfig{
		^
cmd/provider/main.go:133:2: undefined: kingpin (typecheck)
	kingpin.FatalIfError(metrics.SetupMetrics(), "Cannot setup AWS metrics hook")
	^
cmd/provider/main.go:134:2: undefined: kingpin (typecheck)
	kingpin.FatalIfError(controller.Setup(mgr, o), "Cannot setup AWS controllers")
	^
cmd/provider/main.go:135:2: undefined: kingpin (typecheck)
	kingpin.FatalIfError(mgr.Start(ctrl.SetupSignalHandler()), "Cannot start controller manager")
	^
pkg/controller/s3/testing/clientHelper.go:28:42: undefined: smithy (typecheck)
			return &awss3.GetBucketCorsOutput{}, &smithy.GenericAPIError{Code: clients3.CORSNotFoundErrCode}
			                                      ^
pkg/controller/s3/testing/clientHelper.go:31:60: undefined: smithy (typecheck)
			return &awss3.GetBucketLifecycleConfigurationOutput{}, &smithy.GenericAPIError{Code: clients3.LifecycleNotFoundErrCode}
			                                                        ^
pkg/controller/s3/testing/clientHelper.go:40:17: undefined: smithy (typecheck)
			return nil, &smithy.GenericAPIError{Code: clients3.ReplicationNotFoundErrCode}
			             ^
pkg/controller/s3/testing/clientHelper.go:46:17: undefined: smithy (typecheck)
			return nil, &smithy.GenericAPIError{Code: s3.SSENotFoundErrCode}
			             ^
pkg/controller/s3/testing/clientHelper.go:49:17: undefined: smithy (typecheck)
			return nil, &smithy.GenericAPIError{Code: clients3.TaggingNotFoundErrCode}
			             ^
pkg/controller/s3/testing/clientHelper.go:55:17: undefined: smithy (typecheck)
			return nil, &smithy.GenericAPIError{Code: clients3.WebsiteNotFoundErrCode}
			             ^
pkg/clients/ec2/address.go:33:13: undefined: smithy (typecheck)
	var awsErr smithy.APIError
	           ^
pkg/clients/ec2/flowlogs.go:16:13: undefined: smithy (typecheck)
	var awsErr smithy.APIError
	           ^
pkg/clients/ec2/instance.go:55:13: undefined: smithy (typecheck)
	var awsErr smithy.APIError
	           ^
pkg/clients/ec2/internetgateway.go:41:13: undefined: smithy (typecheck)
	var awsErr smithy.APIError
	           ^
pkg/clients/ec2/internetgateway.go:47:13: undefined: smithy (typecheck)
	var awsErr smithy.APIError
	           ^
pkg/clients/ec2/natgateway.go:38:13: undefined: smithy (typecheck)
	var awsErr smithy.APIError
	           ^
pkg/clients/ec2/routetable.go:58:13: undefined: smithy (typecheck)
	var awsErr smithy.APIError
	           ^
pkg/clients/ec2/routetable.go:64:13: undefined: smithy (typecheck)
	var awsErr smithy.APIError
	           ^
pkg/clients/ec2/routetable.go:70:13: undefined: smithy (typecheck)
	var awsErr smithy.APIError
	           ^
pkg/clients/ec2/securitygroup.go:46:13: undefined: smithy (typecheck)
	var awsErr smithy.APIError
	           ^
pkg/clients/ec2/securitygroup.go:52:13: undefined: smithy (typecheck)
	var awsErr smithy.APIError
	           ^
pkg/clients/ec2/subnet.go:38:13: undefined: smithy (typecheck)
	var awsErr smithy.APIError
	           ^
pkg/clients/ec2/vpc.go:40:13: undefined: smithy (typecheck)
	var awsErr smithy.APIError
	           ^
pkg/clients/ec2/vpccidrblock.go:47:13: undefined: smithy (typecheck)
	var awsErr smithy.APIError
	           ^
pkg/clients/ec2/ec2_test.go:57:5: undefined: smithy (typecheck)
			&smithy.GenericAPIError{Code: InternetGatewayIDNotFound},
			 ^
pkg/clients/ec2/ec2_test.go:91:5: undefined: smithy (typecheck)
			&smithy.GenericAPIError{Code: RouteTableIDNotFound},
			 ^
pkg/clients/ec2/ec2_test.go:125:5: undefined: smithy (typecheck)
			&smithy.GenericAPIError{Code: RouteNotFound},
			 ^
pkg/clients/ec2/ec2_test.go:159:5: undefined: smithy (typecheck)
			&smithy.GenericAPIError{Code: AssociationIDNotFound},
			 ^
pkg/clients/ec2/ec2_test.go:193:5: undefined: smithy (typecheck)
			&smithy.GenericAPIError{Code: InvalidGroupNotFound},
			 ^
pkg/clients/ec2/ec2_test.go:227:5: undefined: smithy (typecheck)
			&smithy.GenericAPIError{Code: SubnetIDNotFound},
			 ^
pkg/clients/ec2/ec2_test.go:261:5: undefined: smithy (typecheck)
			&smithy.GenericAPIError{Code: VPCIDNotFound},
			 ^
pkg/clients/eks/eks.go:458:2: token declared and not used (typecheck)
	token := v1Prefix + base64.RawURLEncoding.EncodeToString([]byte(getCallerIdentity.URL))
	^
pkg/clients/hostedzone/hostedzone_test.go:37:11: undefined: smithy (typecheck)
			err:  &smithy.GenericAPIError{Code: "something"},
			       ^
pkg/clients/redshift/redshift_test.go:320:11: undefined: smithy (typecheck)
			err:  &smithy.GenericAPIError{Code: "something"},
			       ^
pkg/clients/s3/bucket.go:166:13: undefined: smithy (typecheck)
	var awsErr smithy.APIError
	           ^
pkg/clients/s3/bucket.go:172:13: undefined: smithy (typecheck)
	var awsErr smithy.APIError
	           ^
pkg/clients/s3/bucket.go:178:13: undefined: smithy (typecheck)
	var awsErr smithy.APIError

anagarlau · 2025-02-13T18:31:34Z

Hi, as far as I can tell the errors are related to running the wrong linter version.
I currently have a PR open which reads it from the pipeline job but it hasn't been merged yet: https://github.com/crossplane-contrib/provider-aws/pull/2165/files#diff-76ed074a9305c04054cdebb9e9aad2d818052b07091de1f20cad0bbac34ffb52L41

Until then you could copy this line in your Makefile/override the variable in the file in the cli to ensure your linter is in sync with the checks (make reviewable GOLANGCILINT_VERSION=1.63.4).
make reviewable runs the test, lint & generate tasks so the generation of CRDs is normal. The setup file i referenced is not related to the generators, it serves to register the controllers with the manager.
The make reviewable task can be found in the build submodules (get with make submodules)
Given your changes you shouldn't see any diffs in the CRDs.

let me know if you have additional questions :)

MisterMX · 2025-02-14T16:42:51Z

pkg/controller/cache/cluster/controller.go

+		obs.ConnectionDetails = managed.ConnectionDetails{
+			xpv1.ResourceCredentialsSecretEndpointKey: []byte(
+				strings.Join(nodes, ","),
+			),
+		}


I think we should keep a single "default" endpoint and and keep the other endpoints in seperate keys, maybe endpoint2, endpoint3 or add a second key endpoints with the joined endpoints string.

MisterMX · 2025-02-14T16:43:57Z

pkg/controller/cache/cluster/controller.go


 	"github.com/aws/aws-sdk-go-v2/aws"
 	elasticacheservice "github.com/aws/aws-sdk-go-v2/service/elasticache"
 	xpv1 "github.com/crossplane/crossplane-runtime/apis/common/v1"
+	"github.com/crossplane-contrib/provider-aws/pkg/utils/pointer"


This import does not seem to be used anywhere.

justinabrahms added 4 commits February 12, 2025 09:43

fix: Add connection details handling for ElastiCache cluster

ce19979

Signed-off-by: Justin Abrahms <[email protected]>

fix: Add missing imports and fix obs variable initialization in Obser…

444373d

…ve method Signed-off-by: Justin Abrahms <[email protected]>

fix: Remove extraneous return statement in Observe method

2af10f9

Signed-off-by: Justin Abrahms <[email protected]>

I don't actually see an Endpoint option. Maybe a joined list of nodes…

6d809a8

… is what we're after? Signed-off-by: Justin Abrahms <[email protected]>

justinabrahms force-pushed the cache-secret-info branch from 6ca08d3 to 6d809a8 Compare February 12, 2025 17:43

MisterMX requested changes Feb 14, 2025

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Cache secret info #2168

Cache secret info #2168

justinabrahms commented Feb 12, 2025

anagarlau commented Feb 12, 2025

justinabrahms commented Feb 13, 2025

anagarlau commented Feb 13, 2025 •

edited

Loading

MisterMX Feb 14, 2025

MisterMX Feb 14, 2025

Cache secret info #2168

Are you sure you want to change the base?

Cache secret info #2168

Conversation

justinabrahms commented Feb 12, 2025

Description of your changes

How has this code been tested

anagarlau commented Feb 12, 2025

justinabrahms commented Feb 13, 2025

anagarlau commented Feb 13, 2025 • edited Loading

MisterMX Feb 14, 2025

Choose a reason for hiding this comment

MisterMX Feb 14, 2025

Choose a reason for hiding this comment

anagarlau commented Feb 13, 2025 •

edited

Loading