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

Adds edit support for IC plugin to tctl edit #52243

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

tcsc
Copy link
Contributor

@tcsc tcsc commented Feb 18, 2025

This patch expands the plugin resource unmarshaler in tctl to handle the
heterogenous filters used by the Identity Center plugin settings. It also adds
support for the IC status block.

These changes allow a user to edit the plugin resource via tctl edit after
installation.

Changelog: Added tctl edit support for Identity Center plugin resources

This patch expands the plugin resource unmarshaler in `tctl` to handle the
heterogenous filters used by the Identity Center plugin settings. It also adds
support for the IC status block.

These changes allow a user to edit the plugin resource via `tctl edit` after
installation.
Comment on lines +1696 to +1704
settings := &types.PluginSpecV1_AwsIc{
AwsIc: &types.PluginAWSICSettings{},
}
p.PluginV1.Spec.Settings = settings

unmshallingWrapper := icSettingsWrapper{inner: settings.AwsIc}
if err := json.Unmarshal(value, &unmshallingWrapper); err != nil {
return trace.Wrap(err)
}
Copy link
Contributor

@smallinsky smallinsky Feb 18, 2025

Choose a reason for hiding this comment

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

Can we use a proto jsonpb package where oneof marhsling/unmershaling is already implemented ?


var settings struct {
AccountFilters []resourceFilter `json:"aws_accounts_filters"`
GroupFilters []resourceFilter `json:"group_sync_filters"`
Copy link
Contributor

@smallinsky smallinsky Feb 18, 2025

Choose a reason for hiding this comment

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

The current AWS IC flow does not allow updating groups after the AWS IC plugin is enrolled. Groups are fetched only once during plugin installation, and even if a filter is updated afterward, the plugin skips the group sync.

For GroupFilters, we need to add additional logic to the enrollment flow. Also  test the filter updates against corner cases. - for instance, what happens if the Account and Group filters further restrict the current configuration. 

So if we want to proceed with this change:

  • Backend code needs to be aligned to support GroupFilters update.
  • Manually the flow agains corner cases.
  • Add basic tests coverage for gorup/account ic plugin update flow

Copy link
Contributor Author

Choose a reason for hiding this comment

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

So, if i'm reading you correctly, in the case of a changed group filters we would basically want to re-run the import?

Copy link
Contributor

@smallinsky smallinsky Feb 24, 2025

Choose a reason for hiding this comment

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

Yes, we discussed this in a 1vs1. The final update implementation should ensure that the backend allows updating GroupFilters once the initial AWS enrollment is completed.

Since this is not directly related to the tctl update flow, the backend alignment can be handled in a separate PR. However, we need to ensure that if we advise customers to use tctl edit plugin aws, the backend flow properly supports updating GroupFilters

for k := range unknownPlugin.Status.Details {
switch k {
case settingsAWSIdentityCenter:
p.PluginV1.Status.Details = &types.PluginStatusV1_AwsIc{}
Copy link
Contributor

@smallinsky smallinsky Feb 18, 2025

Choose a reason for hiding this comment

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

Does reseting plugin details is safe ? Does the Backend will allow overwrite this settings ?

Could you add a comment answering "Why" the PluginV1.Status.Details field is reseted for settingsAWSIdentityCenter plugin ?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants