Skip to content

Support additional expected instance roles.#25718

Merged
mdwn merged 3 commits intomasterfrom
mike.wilson/additional-services
May 5, 2023
Merged

Support additional expected instance roles.#25718
mdwn merged 3 commits intomasterfrom
mike.wilson/additional-services

Conversation

@mdwn
Copy link
Copy Markdown
Contributor

@mdwn mdwn commented May 5, 2023

For enterprise specific services, there's no way to attach the Okta role to the current instance so that it's known to the inventory. This new config option will allow enterprise services to set this option to ensure that, when querying the inventory, these roles/services will display properly.

For enterprise specific services, there's no way to attach the Okta role to
the current instance so that it's known to the inventory. This new config
option will allow enterprise services to set this option to ensure that,
when querying the inventory, these roles/services will display properly.
Comment thread lib/service/service.go Outdated
Comment thread lib/service/service.go Outdated
Comment thread lib/service/servicecfg/config.go Outdated
@mdwn mdwn enabled auto-merge May 5, 2023 20:21
@mdwn mdwn added this pull request to the merge queue May 5, 2023
Copy link
Copy Markdown
Collaborator

@r0mant r0mant left a comment

Choose a reason for hiding this comment

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

Overall code looks good, just have a question about usage.

Comment thread lib/service/service.go
Comment on lines +4688 to +4692
// Register additional expected services for this Teleport instance.
// Meant for enterprise support.
for _, r := range cfg.AdditionalExpectedRoles {
process.SetExpectedInstanceRole(r.Role, r.IdentityEvent)
}
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Super dumb question but if these are "additional roles", then why is this block right at the front of the function rather than after all "regular expected roles" below? :)

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Truth be told I was taking a cue from AdditionalReadyEvents, but thinking on it it should probably be this way so that the OSS process can overwrite any conflicting instances with the ones it expects. Not that I expect that to be a frequent occurrence.

Comment thread lib/service/servicecfg/config.go Outdated
Comment thread lib/service/service.go
func (process *TeleportProcess) registerExpectedServices(cfg *servicecfg.Config) {
// Register additional expected services for this Teleport instance.
// Meant for enterprise support.
for _, r := range cfg.AdditionalExpectedRoles {
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Also, why do we need to pass this to OSS via AdditionalExpectedRoles, can't enterprise code just use SetExpectedInstanceRole(...) on the process when initializing appropriate service?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

It cannot, unfortunately. Basically the inventory status message is created at the very beginning of NewTeleport, so the enterprise statuses never actually get the roles from SetExpectedInstanceRole.

Copy link
Copy Markdown
Collaborator

@r0mant r0mant left a comment

Choose a reason for hiding this comment

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

Overall code looks good, just have a question about usage.

@r0mant r0mant removed this pull request from the merge queue due to a manual request May 5, 2023
@mdwn mdwn enabled auto-merge May 5, 2023 20:37
@mdwn mdwn added this pull request to the merge queue May 5, 2023
Merged via the queue into master with commit 38e5f57 May 5, 2023
@mdwn mdwn deleted the mike.wilson/additional-services branch May 5, 2023 20:57
@public-teleport-github-review-bot
Copy link
Copy Markdown

@mdwn See the table below for backport results.

Branch Result
branch/v12 Failed
branch/v13 Create PR

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

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants