feat(onboarding,cloud-host-scanner): v2 enables cloud_account GCP WIF usage [SSPROD-35921] #487
Conversation
# Conflicts: # sysdig/internal/client/v2/cloudauth/go/cloud_account.pb.go # sysdig/internal/client/v2/cloudauth/go/cloud_organization.pb.go # sysdig/resource_sysdig_secure_cloud_auth_account.go # sysdig/resource_sysdig_secure_cloud_auth_account_test.go
… usage [SSPROD-35921]
| @@ -555,10 +574,12 @@ func componentsToResourceData(components []*cloudauth.AccountComponent) []map[st | |||
|
|
|||
| // internal type redefintion for GCP service principals. | |||
| // This exists because in terraform, the key is originally provided in the form of a base64 encoded json string | |||
|
|
|||
| // note; caution with order of fields, they have to go in alphabetical ASC so that the json marshalled on the tf read phase produces no drift https://github.com/golang/go/issues/27179 | |||
There was a problem hiding this comment.
cc @cgeers just so you are aware of this.
for me it's ok to merge with this
There was a problem hiding this comment.
I'm still scratching my head on they why here, but I'm willing to accept it as a benign change that spoon feeds our tf tests
There was a problem hiding this comment.
haha, yeah... if you want to dig more into the issue feel free to do so, but let's agree on some criteria to accept this PR and unblock us; you can work on digging more anytime later.
i was there for couple of days and i'm ok letting it go...
gotta say though, i'm not expert on go, neither the json libraries. maybe you do see some other option
| } | ||
|
|
||
| component.Metadata = metadata |
There was a problem hiding this comment.
I don't see the functional difference in this change set, starting at L390. If there is one, I don't see it. I think if you look closer, this same behavior was more elegantly expressed previously, with less opportunity for tedium and failure.
This provider should not be in the business of asserting component metadata validity, or otherwise looking for ways to error. The API it talks to does that. This provider code should be as thin and agnostic as it possibly can be, so that future states are compatible out of the box and we don't have to baby sit this code.
There was a problem hiding this comment.
ok, i reverted the whole block.
i see your point after validating that the master branch code passes tests just with the > change.
probably it was due to figuring out the error on the drift (caused by the json marshalling) that i wanted to put the two use-cases separatedly
- key data is present > we populate key
- wif data is present (+email as dependency) > we populate wif
but i agree that with the code on the block
component.Metadata = &cloudauth.AccountComponent_ServicePrincipalMetadata{ServicePrincipalMetadata: &cloudauth.ServicePrincipalMetadata{}}
err = protojson.Unmarshal([]byte(value.(string)), component.GetServicePrincipalMetadata())
for our use case, is more than enough.
it is misleading though, that if the key is present the wif data is written, again.
| @@ -555,10 +574,12 @@ func componentsToResourceData(components []*cloudauth.AccountComponent) []map[st | |||
|
|
|||
| // internal type redefintion for GCP service principals. | |||
| // This exists because in terraform, the key is originally provided in the form of a base64 encoded json string | |||
|
|
|||
| // note; caution with order of fields, they have to go in alphabetical ASC so that the json marshalled on the tf read phase produces no drift https://github.com/golang/go/issues/27179 | |||
There was a problem hiding this comment.
I'm still scratching my head on they why here, but I'm willing to accept it as a benign change that spoon feeds our tf tests
| @@ -387,26 +387,42 @@ func constructAccountComponents(data *schema.ResourceData) []*cloudauth.AccountC | |||
| if data.Get(SchemaCloudProviderType).(string) == cloudauth.Provider_PROVIDER_GCP.String() { | |||
| spGcp := &internalServicePrincipalMetadata{} | |||
| err = json.Unmarshal([]byte(value.(string)), spGcp) | |||
| if len(spGcp.Gcp.Key) >= 0 { | |||
There was a problem hiding this comment.
>= is a legitimate, but the only bug here. Should be >, which I'd like to see be the only change within this section
duplicated from #480, after resolving conflicts on master (to include #482).
had to add a small fix since tests were not passing