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.

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

types: slices and maps of secret key selector should not have pointer #135

Closed
wants to merge 1 commit into from

Conversation

muvaf
Copy link
Member

@muvaf muvaf commented Nov 6, 2022

Description of your changes

*map[string]v1.SecretKeySelector and *[]v1.SecretKeySelector are not compatible with marshaling and slices & maps are already pointers, so, no need to make them pointer of pointers to have them optional in OpenAPI spec. The only functional change in this PR is to not make those two cases pointer but only string one.

Also opened #134 since map with struct value type is actually anti-pattern. So we should decide whether we should switch to something else now or later. @turkenh Do you know of CRDs that have sensitive map values like this one in other providers? I wonder how much breaking change we'd inflict if we were to switch to either of the options proposed in the issue.

Part of #126

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

Confirmed that #126 is fixed with this PR. It also results in rename & retype of PasswordSecretRef *[]v1.SecretKeySelector in User of elasticache to PasswordSecretRefs []v1.SecretKeySelector, which is a breaking change in the schema due to rename. I need to check if it was working in the first place.

… types since they are already pointers

Signed-off-by: Muvaffak Onus <me@muvaf.com>
@turkenh
Copy link
Member

turkenh commented Nov 7, 2022

@turkenh Do you know of CRDs that have sensitive map values like this one in other providers? I wonder how much breaking change we'd inflict if we were to switch to either of the options proposed in the issue.

Not top of my mind. You can print effected resources by running generate against a custom change.

@turkenh
Copy link
Member

turkenh commented Nov 7, 2022

Looks like unit tests are failing with a relevant issue, can you check that?

@muvaf
Copy link
Member Author

muvaf commented Nov 7, 2022

@turkenh I'll run and see the number. Meanwhile, it'd be great if you can share your thoughts on #134 . Ideally, I'd like to switch to either of those options instead of having to go through a breaking change later with aws_glue_connection.

@turkenh
Copy link
Member

turkenh commented Nov 11, 2022

Confirmed that #126 is fixed with this PR. It also results in rename & retype of PasswordSecretRef *[]v1.SecretKeySelector in User of elasticache to PasswordSecretRefs []v1.SecretKeySelector, which is a breaking change in the schema due to rename. I need to check if it was working in the first place.

@muvaf are there anything else you did to get the issue fixed with this PR? Because I cannot get it fixed with this PR as well:

panic: cannot run goimports for apis folder: ./glue/v1beta1/zz_connection_types.go:45:55: expected ';', found '/'
./glue/v1beta1/zz_connection_types.go:74:1: expected '}', found 'type'
: exit status 2

Generates the following

// +kubebuilder:validation:Optional
ConnectionPropertiesSecretRefMap map[string]github.com/crossplane/crossplane-runtime/apis/common/v1.SecretKeySelector%!(EXTRA string=v1) `json:"connectionPropertiesSecretRefMap,omitempty" tf:"-"`

@srust
Copy link

srust commented Nov 14, 2022

Hi,

I am attempting to create a Linode provider using upjet and hitting this issue as well. Specifically for the "linode_instance" resource via https://registry.terraform.io/providers/linode/linode/latest/docs/resources/instance

The errors are similar to here:

14:22:23 [ .. ] go generate linux_amd64
panic: cannot run goimports for apis folder: ./instance/v1alpha1/zz_instance_types.go:226:48: expected ';', found '/'
./instance/v1alpha1/zz_instance_types.go:235:1: expected '}', found 'type'
./instance/v1alpha1/zz_instance_types.go:425:48: expected ';', found '/'
./instance/v1alpha1/zz_instance_types.go:454:1: expected '}', found 'type'
: exit status 2
  226 StackscriptDataSecretRef *map[string]github.com/crossplane/crossplane-runtime/apis/common/v1.SecretKeySelector%!(EXTRA string=_v1common) `json:"stackscriptDataSecretRef,omitempty" tf:"-      "`
  425 StackscriptDataSecretRef *map[string]github.com/crossplane/crossplane-runtime/apis/common/v1.SecretKeySelector%!(EXTRA string=_v1common) `json:"stackscriptDataSecretRef,omitempty" tf:"-      "`

In this case, with this PR/patch, the StackscriptDataSecretRef becomes

202 StackscriptDataSecretRefMap map[string]_v1.SecretKeySelector json:"stackscriptDataSecretRefMap,omitempty" tf:"-"
389 StackscriptDataSecretRefMap map[string]_v1.SecretKeySelector json:"stackscriptDataSecretRefMap,omitempty" tf:"-"

But it is able to generate successfully with this patch.

@Upbound-CLA
Copy link

Upbound-CLA commented Nov 23, 2022

CLA assistant check
All committers have signed the CLA.

@turkenh
Copy link
Member

turkenh commented Dec 8, 2022

Closing this since the underlying problem fixed by #138

@turkenh turkenh closed this Dec 8, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants