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

feat(onboarding): Datasource for regulatory trusted identity #561

Merged
merged 3 commits into from
Oct 25, 2024

Conversation

ravinadhruve10
Copy link
Contributor

@ravinadhruve10 ravinadhruve10 commented Oct 16, 2024

Change summary:

  • Adding new fields in existing datasource secure_trusted_cloud_identity with identities for onboarding regulatory workloads such as aws gov workloads.
  • updated acc test and docs for the datasource.

@ravinadhruve10 ravinadhruve10 marked this pull request as ready for review October 22, 2024 04:43
Copy link
Contributor

@cgeers cgeers left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nothing really blocking, but I have two related points.

The term "assets" was only intended to indicate a bundled collection of data relevant to a specific feature. I know it's pedantic, but I don't think "assets" is some new naming convention.

Did you consider adding these fields to the existing data source? Does that reduce any work, or create new problems?

@ravinadhruve10
Copy link
Contributor Author

Nothing really blocking, but I have two related points.

The term "assets" was only intended to indicate a bundled collection of data relevant to a specific feature. I know it's pedantic, but I don't think "assets" is some new naming convention.

So "assets" was added to the data source to indicate returning all information relevant to regulatory/gov framework types, and to keep the possibility open if we want to return additional set of attributes not just the trusted gov role arn. Perhaps could've chosen another word.

Did you consider adding these fields to the existing data source? Does that reduce any work, or create new problems?

Yes like I had mentioned before, the existing data source returns a single string response type. For the regulation/gov use case, we will need to additionally return another string, because the same backend should be able to have both commercial trusted role arn and gov trusted role arn if supported. On changing the response type shape (string to map), it could break backwards compatibility. Another reason is to be future looking and allow additional set of attributes to be returned for gov use case, so a map is most flexible there.

@cgeers
Copy link
Contributor

cgeers commented Oct 24, 2024

So "assets" was added to the data source to indicate returning all information relevant to regulatory/gov framework types, and to keep the possibility open if we want to return additional set of attributes not just the trusted gov role arn.

but that's not what this PR actually does. the new datasource accepts a var cloud_provider, where only aws is allowed. aws is not framework type.

the existing data source returns a single string response type

no, the API returns a string, the datasource returns an object with parts of that string response decomposed into various fields.

On changing the response type shape (string to map), it could break backwards compatibility

you don't have to break backwards compat. one datasource can call two APIs, and populate new fields corresponding to the added values can users can choose to use discriminately.

I'm not necessary suggesting the organization I describe is better overall, but it it has these positive attributes:

  • adds less stuff
  • keeps the datasources as feature aligned as they are today, as opposed to "environment" aligned

There might be bigger considerations though that I'm undervaluing. My question is, has this approach I describe been considered and how does it compare?

@ravinadhruve10
Copy link
Contributor Author

So "assets" was added to the data source to indicate returning all information relevant to regulatory/gov framework types, and to keep the possibility open if we want to return additional set of attributes not just the trusted gov role arn.

but that's not what this PR actually does. the new datasource accepts a var cloud_provider, where only aws is allowed. aws is not framework type.

the existing data source returns a single string response type

no, the API returns a string, the datasource returns an object with parts of that string response decomposed into various fields.

On changing the response type shape (string to map), it could break backwards compatibility

you don't have to break backwards compat. one datasource can call two APIs, and populate new fields corresponding to the added values can users can choose to use discriminately.

I'm not necessary suggesting the organization I describe is better overall, but it it has these positive attributes:

  • adds less stuff
  • keeps the datasources as feature aligned as they are today, as opposed to "environment" aligned

There might be bigger considerations though that I'm undervaluing. My question is, has this approach I describe been considered and how does it compare?

Ah I now understand what you mean! basically the same datasource can be leveraged for giving the populated results from 2 different APIs. That can be done, however I do have some concerns, we can chat offline.

Change summary:
-----------------
- Adding a new datasource secure_trusted_cloud_regulation_assets
  with identities for onboarding regulatory workloads such as aws
  gov workloads.
- added acc test and docs for the new datasource.
@ravinadhruve10
Copy link
Contributor Author

Ah I now understand what you mean! basically the same datasource can be leveraged for giving the populated results from 2 different APIs. That can be done, however I do have some concerns, we can chat offline.

We discussed this offline. Since currently the only use case / information we need from the new api endpoint is to consume the trusted identity arn for regulatory environments (such as gov), it makes sense to use the existing datasource. Updated the PR.

return diag.FromErr(err)
}
}

d.SetId(identity)
Copy link
Contributor Author

@ravinadhruve10 ravinadhruve10 Oct 25, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ideally we should be setting this data object id to a generic string. Refraining from changing it to avoid TF state drifts of existing customer installs.

@ravinadhruve10 ravinadhruve10 merged commit 95cf692 into master Oct 25, 2024
23 checks passed
@ravinadhruve10 ravinadhruve10 deleted the onboarding-gov-datasource branch October 25, 2024 23:22
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.

2 participants