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
Open
Show file tree
Hide file tree
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
89 changes: 87 additions & 2 deletions tool/tctl/common/collection.go
Original file line number Diff line number Diff line change
Expand Up @@ -1618,11 +1618,15 @@ func (p *pluginResourceWrapper) UnmarshalJSON(data []byte) error {
settingsEntraID = "entra_id"
settingsDatadogIncidentManagement = "datadog_incident_management"
settingsEmailAccessPlugin = "email_access_plugin"
settingsAWSIdentityCenter = "aws_ic"
)
type unknownPluginType struct {
Spec struct {
Settings map[string]json.RawMessage `json:"Settings"`
} `json:"spec"`
Status struct {
Details map[string]json.RawMessage `json:"Details"`
} `json:"status"`
Credentials struct {
Credentials map[string]json.RawMessage `json:"Credentials"`
} `json:"credentials"`
Expand Down Expand Up @@ -1658,8 +1662,7 @@ func (p *pluginResourceWrapper) UnmarshalJSON(data []byte) error {
}
}

for k := range unknownPlugin.Spec.Settings {

for k, value := range unknownPlugin.Spec.Settings {
switch k {
case settingsSlackAccessPlugin:
p.PluginV1.Spec.Settings = &types.PluginSpecV1_SlackAccessPlugin{}
Expand Down Expand Up @@ -1689,17 +1692,99 @@ func (p *pluginResourceWrapper) UnmarshalJSON(data []byte) error {
p.PluginV1.Spec.Settings = &types.PluginSpecV1_Datadog{}
case settingsEmailAccessPlugin:
p.PluginV1.Spec.Settings = &types.PluginSpecV1_Email{}
case settingsAWSIdentityCenter:
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)
}
Comment on lines +1696 to +1704
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 ?

default:
return trace.BadParameter("unsupported plugin type: %v", k)
}
}

if len(unknownPlugin.Status.Details) > 1 {
return trace.BadParameter("malformed status details")
}
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 ?

}
}

if err := json.Unmarshal(data, &p.PluginV1); err != nil {
return err
}
return nil
}

// icSettingsWrapper is a wrapper around the Identity Center plugin settings to
// provide custom unmarshalling.
type icSettingsWrapper struct {
inner *types.PluginAWSICSettings
}

// UnmarshalJSON implements custom JSON-unmarshaling for the Identity Center
// plugin settings. This custom unmarshaler is required to unpack the structure
// of the polymorphic filters in the plugin settings, which otherise cannot be
// unpacked.
func (s *icSettingsWrapper) UnmarshalJSON(data []byte) error {
type resourceFilter struct {
Include map[string]json.RawMessage `json:"Include"`
}

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

}

// unpackFilters only creates the structure of the filters so that the
// normal JSON unmarshaller knows how to fill in the actual values
unpackFilters := func(src []resourceFilter) ([]*types.AWSICResourceFilter, error) {
var dst []*types.AWSICResourceFilter
for _, f := range src {
if len(f.Include) != 1 {
return nil, trace.BadParameter("Malformed filter")
}
for k := range f.Include {
switch k {
case "id":
dst = append(dst, &types.AWSICResourceFilter{Include: &types.AWSICResourceFilter_Id{}})

case "name_regex":
dst = append(dst, &types.AWSICResourceFilter{Include: &types.AWSICResourceFilter_NameRegex{}})

default:
return nil, trace.BadParameter("Unexpected filter key: %s", k)
}
}
}
return dst, nil
}

if err := json.Unmarshal(data, &settings); err != nil {
return trace.Wrap(err)
}

var err error
s.inner.AwsAccountsFilters, err = unpackFilters(settings.AccountFilters)
if err != nil {
return trace.Wrap(err)
}

s.inner.GroupSyncFilters, err = unpackFilters(settings.GroupFilters)
if err != nil {
return trace.Wrap(err)
}

return nil
}

func (c *pluginCollection) resources() []types.Resource {
r := make([]types.Resource, len(c.plugins))
for i, resource := range c.plugins {
Expand Down
48 changes: 48 additions & 0 deletions tool/tctl/common/resource_command_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -46,6 +46,7 @@ import (
headerv1 "github.com/gravitational/teleport/api/gen/proto/go/teleport/header/v1"
userprovisioningpb "github.com/gravitational/teleport/api/gen/proto/go/teleport/userprovisioning/v2"
"github.com/gravitational/teleport/api/types"
apicommon "github.com/gravitational/teleport/api/types/common"
"github.com/gravitational/teleport/api/types/discoveryconfig"
"github.com/gravitational/teleport/api/types/header"
"github.com/gravitational/teleport/entitlements"
Expand Down Expand Up @@ -2577,6 +2578,53 @@ func TestPluginResourceWrapper(t *testing.T) {
},
},
},
{
name: "identity center",
plugin: types.PluginV1{
Metadata: types.Metadata{
Name: apicommon.OriginAWSIdentityCenter,
Labels: map[string]string{
"teleport.dev/hosted-plugin": "true",
},
},
Spec: types.PluginSpecV1{
Settings: &types.PluginSpecV1_AwsIc{
AwsIc: &types.PluginAWSICSettings{
IntegrationName: apicommon.OriginAWSIdentityCenter,
Region: "ap-south-2",
Arn: "some:arn",
ProvisioningSpec: &types.AWSICProvisioningSpec{
BaseUrl: "https://scim.example.com/v2",
},
AccessListDefaultOwners: []string{"root"},
CredentialsSource: types.AWSICCredentialsSource_AWSIC_CREDENTIALS_SOURCE_SYSTEM,
UserSyncFilters: []*types.AWSICUserSyncFilter{
{Labels: map[string]string{types.OriginLabel: types.OriginOkta}},
{Labels: map[string]string{types.OriginLabel: types.OriginEntraID}},
},
GroupSyncFilters: []*types.AWSICResourceFilter{
{Include: &types.AWSICResourceFilter_NameRegex{NameRegex: `^Group #\\d+$`}},
{Include: &types.AWSICResourceFilter_Id{Id: "42"}},
},
AwsAccountsFilters: []*types.AWSICResourceFilter{
{Include: &types.AWSICResourceFilter_Id{Id: "314159"}},
{Include: &types.AWSICResourceFilter_NameRegex{NameRegex: `^Account #\\d+$`}},
},
},
},
},
Status: types.PluginStatusV1{
Code: types.PluginStatusCode_RUNNING,
Details: &types.PluginStatusV1_AwsIc{
AwsIc: &types.PluginAWSICStatusV1{
GroupImportStatus: &types.AWSICGroupImportStatus{
StatusCode: types.AWSICGroupImportStatusCode_DONE,
},
},
},
},
},
},
}

for _, tc := range tests {
Expand Down
Loading