-
Notifications
You must be signed in to change notification settings - Fork 1.8k
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
Let tctl make plugins #41038
Let tctl make plugins #41038
Conversation
api/types/constants.go
Outdated
const ( | ||
// HostedPluginLabel defines the name for the hosted plugin label. | ||
// When this label is set to "true" on a Plugin resource, | ||
// it indicates that the Plugin should be run by the Cloud service, | ||
// rather than self-hosted plugin services. | ||
HostedPluginLabel = TeleportNamespace + "/hosted-plugin" | ||
) | ||
|
||
const ( | ||
// OktaOrgURLLabel is the label used by Okta-managed resources to indicate | ||
// the upstream Okta organization that they come from. | ||
OktaOrgURLLabel = "okta/org" | ||
|
||
// OktaCredPurposeLabel is used by Okta-managed PluginStaticCredentials to | ||
// indicate their purpose | ||
OktaCredPurposeLabel = "okta/purpose" | ||
|
||
// OktaCredPurposeAuth indicates that the credential is intended for | ||
// authenticating with the Okta REST API | ||
OktaCredPurposeAuth = "okta-auth" | ||
|
||
// OktaCredPurposeSCIMToken indicates that theis to be used for authenticating | ||
// SCIM requests from the upstream organization. The content of the credential | ||
// is a bcrypt hash of actual token. | ||
OktaCredPurposeSCIMToken = "scim-bearer-token" | ||
) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
All of these constants are defined in teleport.e. Is there a corresponding teleport.e PR that removes them from there and uses these?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not yet, but its on the way
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
gravitational/teleport.e#4098
tool/tctl/common/plugins_command.go
Outdated
Required(). | ||
StringVar(&p.install.okta.apiToken) | ||
p.install.okta.cmd. | ||
Flag("saml-connector", "SAML connector to link to plugin"). |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What does it mean to "link" connector? Will it try to create connector with this name, or somehow adopt an existing one? It's not clear from this description.
}, | ||
} | ||
|
||
if _, err := args.plugins.CreatePlugin(ctx, req); err != nil { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Web UI doesn't let you create more than 1 instance of a hosted plugin, incl. Okta. How will this behave if you already have an Okta integration running?
And similarly, our UI prompts user to cleanup old Okta integration before creating a new one if it detects there are resources left from the previous instance of Okta integration. Does this behave the same way? @smallinsky should have details about it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes It should behave the same way and the cleanup step will be retuned in error message.
Co-authored-by: Roman Tkachenko <[email protected]> Co-authored-by: Jakub Nyckowski <[email protected]>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let few minor comments.
tool/tctl/common/plugins_command.go
Outdated
if oktaSettings.samlConnector == "" { | ||
return trace.BadParameter("SCIM support requires saml-connector to be set") | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why samlConnector is only validated when SCIM is enabled ?
Don't we reference to samlConnector in our okta sync flow ? For instance:
https://github.com/gravitational/teleport.e/blob/5012ca517540f8d1d44bda375fc5de0e8113e04a/lib/okta/synchronize.go#L640
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok, will just make it flat-out required
tool/tctl/common/plugins_command.go
Outdated
} | ||
} | ||
|
||
if oktaSettings.samlConnector != "" { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
if samlConnector was provided can we get the okta org from connector spec ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We can try it, and give a warning to set it explicitly if it can't be found. The App ID label is only guaranteed to be set if the SAML Connector was created though the Okta install flow; if it was created manually it may not exist.
tool/tctl/common/plugins_command.go
Outdated
func (p *PluginsCommand) initInstall(parent *kingpin.CmdClause, config *servicecfg.Config) { | ||
p.install.cmd = parent.Command("install", "Install new plugin instance") | ||
|
||
p.install.okta.cmd = p.install.cmd.Command("okta", "Install an okta integration") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
p.install.okta.cmd = p.install.cmd.Command("okta", "Install an okta integration") | |
p.install.okta.cmd = p.install.cmd.Command("okta", "Install an Okta integration") |
tool/tctl/common/plugins_command.go
Outdated
fmt.Printf("Failed validating SAML connector: %v", err) | ||
return trace.Wrap(err) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
fmt.Printf("Failed validating SAML connector: %v", err) | |
return trace.Wrap(err) | |
return trace.Wrap(err, "failed validating SAML connector") |
tool/tctl/common/plugins_command.go
Outdated
fmt.Printf("Plugin creation failed: %v\n", err) | ||
return trace.Wrap(err) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
fmt.Printf("Plugin creation failed: %v\n", err) | |
return trace.Wrap(err) | |
return trace.Wrap(err, "plugin creation failed") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@tcsc Can you include the docs for this as well?
Co-authored-by: Roman Tkachenko <[email protected]>
Allows
tctl
to install an Okta integration without having the user resort to crafting a PluginRequest in JSON. It's not as automated as the Web-based installer, but is a reasonable mid-point if you need more control over integration behaviour.Example:
Note: Currently defaults to
--sync--users
and--sync-groups
being enabled for parity with the Web-based installer, but I'm not wedded to it. Perhaps defaulting to "off" would be less surprising for a CLI.Full option set:
Also moves some constants (e.g. Okta plugin label names) from Enterprise to OSS where they can be seen from
tctl
. A following PR will update the enterprse definitions to point back to the new constants created here.Changelog: Added support for creating Okta integrations in
tctl
.