DiscoveryConfig: init service and add resource to `tctl` by marcoandredinis · Pull Request #32399 · gravitational/teleport

marcoandredinis · 2023-09-22T16:03:58Z

Context: #25494

This PR starts the DiscoveryConfig service in gRPC server and allows tctl to interact with those records.

It also adds access to the editor role.
Users should be able to RW any DiscoveryConfig.

DiscoveryService should be able to watch those resources, so that it can act upon any changes.

espadolini · 2023-09-29T08:13:00Z

This is a terrible approach to upsert, and it's not really justifiable for a brand new resource type IMO. Can we add a real upsert operation, outright drop the implementation of UpdateDiscoveryConfig in v14 and only support it in v15+ as a conditional update operation instead?

I've just added the upsert method.

outright drop the implementation of UpdateDiscoveryConfig in v14 and only support it in v15+ as a conditional update operation instead?

Sorry, not sure I followed.
Are you suggesting to remove the UpdateDiscoveryConfig in v15/master but keep it in v14?
But then clients using v14 will hit the UpdateDiscoveryConfig and it will fail

Could you please clear this up?

(maybe by v15+ you meant v16 and ownards, not sure if your + was inclusive)

This resource is not yet in use, but we should be careful anyway because it was backported to v14

Since v14.0.1 doesn't have an implementation for UpdateDiscoveryConfig yet, I propose that we remove UpdateDiscoveryConfig from branch/v14 and leave it as a not implemented RPC, add (and backport) UpsertDiscoveryConfig from this PR and use it in v14 clients, then in master and branch/v15 we can implement it as a conditional update operation.

remove UpdateDiscoveryConfig from branch/v14 and leave it as a not implemented RPC

It might get released before we are able to merge the backport.
I think, the Update method is the intended one for 99% of the cases.
That also holds true when we implement the condition updates/revisions.

What is "write-if-any-exists" useful for?

Well, honestly the only other client (for now) is going to be the WebAPI
I'm not sure we need it there.

This should ease the path for the revision logic we intend to implement later.

I would rather not remove it.

marcoandredinis · 2023-10-03T06:55:40Z

@fheinecke Can you please take a look?

tigrato · 2023-10-11T09:25:19Z

Will this require optimistic locking?

I think all resources will use it.

This is being used in tctl create -f and cache maintenance.

Upsert is always unconditional - we considered adding a primitive backend operation for "write if item doesn't exist or if it exists and it matches this revision" but we couldn't really think of uses for it. 🤷

If you meant in general then yeah, unfortunately we have an implemented and released UpdateDiscoveryConfig that no one should use so the conditionally updating RPC used by tctl edit will probably be UpdateDiscoveryConfigV2 or something like that. 🥲

I meant the Update endpoint but github doesn't let me add the comment to the correct place

I'm not sure I'm following here.

tctl edit should use the UpdateDiscoveryConfig which conditionally updates the resource.
It's not doing that right now, but the flow is there:

teleport/tool/tctl/common/edit_command.go

Lines 159 to 172 in 435e7d4

// Use the UpdateHandler if the resource has one, otherwise fallback to using

// the CreateHandler. UpdateHandlers are preferred over CreateHandler because an update

// will not forcibly overwrite a resource unlike with create which requires the force

// flag to be set to update an existing resource.

updator, found := rc.UpdateHandlers[ResourceKind(raw.Kind)]

if found {

return trace.Wrap(updator(ctx, client, raw))

}

// TODO(tross) remove the fallback to CreateHandlers once all the resources

// have been updated to implement an UpdateHandler.

if creator, found := rc.CreateHandlers[ResourceKind(raw.Kind)]; found {

return trace.Wrap(creator(ctx, client, raw))

}

This PR starts the DiscoveryConfig service in gRPC server and allows `tctl` to interact with those records. It also adds access to the `editor` role. Users should be able to RW any DiscoveryConfig. DiscoveryService should be able to watch those resources, so that it can act upon any changes.

public-teleport-github-review-bot · 2023-10-11T10:43:36Z

@marcoandredinis See the table below for backport results.

Branch	Result
branch/v14	Failed

* DiscoveryConfig: init service and add it to `tctl` This PR starts the DiscoveryConfig service in gRPC server and allows `tctl` to interact with those records. It also adds access to the `editor` role. Users should be able to RW any DiscoveryConfig. DiscoveryService should be able to watch those resources, so that it can act upon any changes. * add revision * add upsert method * improve tctl -f command

…3289) * DiscoveryConfig: init service and add it to `tctl` This PR starts the DiscoveryConfig service in gRPC server and allows `tctl` to interact with those records. It also adds access to the `editor` role. Users should be able to RW any DiscoveryConfig. DiscoveryService should be able to watch those resources, so that it can act upon any changes. * add revision * add upsert method * improve tctl -f command

marcoandredinis added discover Issues related to Teleport Discover backport/branch/v14 labels Sep 22, 2023

This was referenced Sep 22, 2023

Add ability to manage auto-discovery settings dynamically #25494

Closed

DiscoveryConfig: add service and client #32328

Merged

marcoandredinis force-pushed the marco/discovery_config_service_with_rbac branch from 4bd0512 to 09ada06 Compare September 26, 2023 16:09

marcoandredinis changed the title ~~DiscoveryConfig: init service and add it to tctl~~ DiscoveryConfig: init service and add resource to tctl Sep 26, 2023

marcoandredinis force-pushed the marco/discovery_config_init_resource_tctl branch from a7e849e to b3afbec Compare September 26, 2023 16:10

marcoandredinis force-pushed the marco/discovery_config_service_with_rbac branch from 09ada06 to aa4b7a9 Compare September 27, 2023 18:23

Base automatically changed from marco/discovery_config_service_with_rbac to master September 27, 2023 20:27

marcoandredinis force-pushed the marco/discovery_config_init_resource_tctl branch 4 times, most recently from 6dfa8be to 497c64f Compare September 28, 2023 10:43

marcoandredinis marked this pull request as ready for review September 28, 2023 13:20

github-actions Bot added size/md tctl tctl - Teleport admin tool labels Sep 28, 2023

github-actions Bot requested review from espadolini and fheinecke September 28, 2023 13:21

espadolini reviewed Sep 29, 2023

View reviewed changes

Comment thread lib/services/local/events.go Outdated

marcoandredinis force-pushed the marco/discovery_config_init_resource_tctl branch from 497c64f to 53736e0 Compare September 29, 2023 09:28

espadolini approved these changes Sep 29, 2023

View reviewed changes

Comment thread tool/tctl/common/resource_command.go Outdated

marcoandredinis force-pushed the marco/discovery_config_init_resource_tctl branch from 7fad84d to b809d6d Compare October 2, 2023 17:52

marcoandredinis force-pushed the marco/discovery_config_init_resource_tctl branch 4 times, most recently from 2a3c17c to 5af94ee Compare October 10, 2023 09:11

tigrato approved these changes Oct 11, 2023

View reviewed changes

public-teleport-github-review-bot Bot removed the request for review from fheinecke October 11, 2023 09:27

marcoandredinis added this pull request to the merge queue Oct 11, 2023

github-merge-queue Bot removed this pull request from the merge queue due to failed status checks Oct 11, 2023

marcoandredinis added 4 commits October 11, 2023 11:06

add revision

c5df96d

add upsert method

3ddd28d

improve tctl -f command

972995d

marcoandredinis force-pushed the marco/discovery_config_init_resource_tctl branch from 5af94ee to 972995d Compare October 11, 2023 10:06

marcoandredinis enabled auto-merge October 11, 2023 10:06

marcoandredinis added this pull request to the merge queue Oct 11, 2023

Merged via the queue into master with commit de3a0cc Oct 11, 2023

marcoandredinis deleted the marco/discovery_config_init_resource_tctl branch October 11, 2023 10:42

marcoandredinis mentioned this pull request Oct 11, 2023

[v14] DiscoveryConfig: init service and add resource to tctl (#32399) #33289

Merged

	// Use the UpdateHandler if the resource has one, otherwise fallback to using
	// the CreateHandler. UpdateHandlers are preferred over CreateHandler because an update
	// will not forcibly overwrite a resource unlike with create which requires the force
	// flag to be set to update an existing resource.
	updator, found := rc.UpdateHandlers[ResourceKind(raw.Kind)]
	if found {
	return trace.Wrap(updator(ctx, client, raw))
	}

	// TODO(tross) remove the fallback to CreateHandlers once all the resources
	// have been updated to implement an UpdateHandler.
	if creator, found := rc.CreateHandlers[ResourceKind(raw.Kind)]; found {
	return trace.Wrap(creator(ctx, client, raw))
	}

Conversation

marcoandredinis commented Sep 22, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

marcoandredinis commented Oct 3, 2023

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

public-teleport-github-review-bot Bot commented Oct 11, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

marcoandredinis commented Sep 22, 2023 •

edited

Loading