Add cluster autoupdate resources with enabled cache #45617

vapopov · 2024-08-20T04:53:16Z

In this PR implemented new resources for autoupdate configuration and version store with enabled cache on proxy side to be able serve these resources without making requests to auth service.
Also added commands for tctl edit/get/create

Related: #39805
changelog: Added resources for autoupdate (config and version)

api/proto/teleport/legacy/client/proto/authservice.proto

api/proto/teleport/legacy/types/types.proto

api/types/autoupdate_config.go

…ate-resources

api/proto/teleport/autoupdate/v1/autoupdate.proto

api/client/webclient/webclient.go

…ate-resources

github-actions · 2024-08-22T19:06:58Z

The PR changelog entry failed validation: Changelog entry not found in the PR body. Please add a "no-changelog" label to the PR, or changelog lines starting with changelog: followed by the changelog entries for the PR.

sclevine

LGTM, may want to double-check with @rosstimothy about whether the cache is configured suitably for serving the /find endpoint

tool/tctl/common/autoupdate_command.go

Fix interval first duration

rosstimothy

There is a lot going on in this PR - many layers of boilerplate all being implemented at once. I'd suggest trying to scope this to just adding the resource, the RPC service and persisting it in the backend and retrieving it from the cache. The tctl commands and webapi changes could probably both be in their own distinct and more focused PRs.

lib/auth/authclient/clt.go

rosstimothy · 2024-08-23T15:09:12Z

api/client/client.go

+// GetAutoupdateConfig gets autoupdate configuration.
+func (c *Client) GetAutoupdateConfig(ctx context.Context) (*autoupdate.AutoupdateConfig, error) {
+	resp, err := c.AutoupdateServiceClient().GetAutoupdateConfig(ctx, &autoupdate.GetAutoupdateConfigRequest{})
+	if err != nil {
+		return nil, trace.Wrap(err)
+	}
+	return resp, nil
+}
+
+// GetAutoupdateVersion gets autoupdate version.
+func (c *Client) GetAutoupdateVersion(ctx context.Context) (*autoupdate.AutoupdateVersion, error) {
+	resp, err := c.AutoupdateServiceClient().GetAutoupdateVersion(ctx, &autoupdate.GetAutoupdateVersionRequest{})
+	if err != nil {
+		return nil, trace.Wrap(err)
+	}
+	return resp, nil
+}


Consider consolidating the API here. If we offer AutoupdateServiceClient then let's have everyone use that instead of creating these helpers.

after adding get methods for fetching config and version in find endpoint

interface ReadRemoteProxyAccessPoint { GetAutoupdateConfig(ctx context.Context) (*autoupdate.AutoupdateConfig, error) GetAutoupdateVersion(ctx context.Context) (*autoupdate.AutoupdateVersion, error) }

cache.Cache and client.ClientI should implement these methods, otherwise if I replace this one to client get method I need to add some wrapper for https://github.com/gravitational/teleport/pull/45617/files#diff-3e237f973a30f7092dc18a706ecd95d8a7e1d99c5c50626b8bb0e7ac6cb7efd1R1899-R1920

api/proto/teleport/autoupdate/v1/autoupdate_service.proto

api/types/autoupdate/config.go

api/types/autoupdate/version.go

lib/services/autoupdates.go

lib/web/apiserver.go

rosstimothy · 2024-08-23T15:23:16Z

tool/tctl/common/autoupdate_command.go

+// AutoupdateCommand implements the `tctl autoupdate` command for managing
+// autoupdate process for tools and agents.
+type AutoupdateCommand struct {


Should this resource be supported via tctl create and tctl get? Any reason to prefer managing auto update resources via tctl autoupdate instead?

The autoupdate subcommand was proposed by RJ here: https://github.com/gravitational/teleport/pull/39805/files#diff-8d3c0408dd826f27e9f13cd15f09f4d7e89202862f0b13ce131a2e3072c2a40dR249

Seems reasonable for both the subcommand and tctl create/tctl get to work?

I believe that might be better from UX, when everything in one place and you able to configure autoupdates without having knowledge about resources responsible for this configuration.
I will additionally add these resources also to tctl create/get resource command

tool/tctl/common/autoupdate_command.go

lib/services/local/autoupdate.go

Add resource command for autoupdate config and version Limit to getter interface for client and cache part Add client wrapping

public-teleport-github-review-bot · 2024-08-27T04:42:52Z

@vapopov - this PR will require admin approval to merge due to its size. Consider breaking it up into a series smaller changes.

lib/auth/grpcserver.go

lib/web/apiserver.go

tool/tctl/common/resource_command.go

Add tests for resources command

vapopov · 2024-08-30T19:41:27Z

@rosstimothy could you please take another look at the PR?

rosstimothy

As mentioned before, I think it would be best if this PR was split up into several smaller, more focused PRs. I tend to recommend that for new resources folks try to split each layer of the boilerplate up into PRs in roughly the following order:

resource and audit events proto and generated code
backend storage service implementation
RPC service implementation
cache support
tctl support
audit event support

A good example to follow is the SPIFFEFederation resource that Noah recently added.

rosstimothy · 2024-09-01T21:34:47Z

lib/auth/autoupdate/v1/service.go

+	Backend services.AutoupdateService
+	// Cache is the cache used to store autoupdate resources.
+	Cache services.AutoupdateServiceGetter


Suggestion: consider using smaller, more well defined interfaces here for each of these containing only the methods expected to be called on each instead of relying on the services interface.

rosstimothy · 2024-09-01T21:35:29Z

lib/auth/autoupdate/v1/service.go

+	// Opting out of forward compatibility, this service must implement all service methods.
+	autoupdate.UnsafeAutoupdateServiceServer


Any particular reason to go with this instead of the UnimplementedAutoupdateServiceServer?

this is minimal requirement to be registered as service server, but we have to be sure that all methods implemented not to have nil pointer panic, will replace with UnimplementedAutoupdateServiceServer

rosstimothy · 2024-09-01T21:37:47Z

api/types/constants.go

+	// KindAutoupdateConfig is the resource with autoupdate configuration.
+	KindAutoupdateConfig = "autoupdate_config"
+
+	// KindAutoupdateVersion is the resource with autoupdate versions.
+	KindAutoupdateVersion = "autoupdate_version"
+
+	// MetaNameAutoupdateConfig is the name of a configuration resource for autoupdate config.
+	MetaNameAutoupdateConfig = "autoupdate-config"
+
+	// MetaNameAutoupdateVersion is the name of a resource for autoupdate version.
+	MetaNameAutoupdateVersion = "autoupdate-version"


Reading through the code I find the naming Autoupdate to look off - should we be using AutoUpdate instead?

We agreed with @sclevine to use Autoupdate instead camel case, both RFDs was modified to be consistent

No strong opinion about this. The RFDs had a mixture of autoupdate_ / Autoupdate and auto_update_ / AutoUpdate prefixes, so we went with the former spelling everywhere to be consistent with the resource and command naming in the original Client Tools proposal: https://github.com/gravitational/teleport/pull/39805/files

@rosstimothy if you feel strongly that we should move the other way, or break consistency for identifiers in the codebase, no objections.

rosstimothy · 2024-09-01T21:41:56Z

lib/services/local/autoupdate.go

+	autoupdateConfigPrefix  = "autoupdate_config"
+	autoupdateVersionPrefix = "autoupdate_version"


Same question about the naming of this resource. Is this entity an autoupdate or an auto update?

Suggested change

autoupdateConfigPrefix = "autoupdate_config"

autoupdateVersionPrefix = "autoupdate_version"

autoUpdateConfigPrefix = "auto_update_config"

autoUpdateVersionPrefix = "auto_update_version"

rosstimothy · 2024-09-01T21:47:45Z

api/client/autoupdate/autoupdate.go

+)
+
+// Client is an AutoupdateService client that conforms to the following lib/services interfaces:
+//   - services.AutoupdateService


I don't think we should mention services in the api package since API does not an cannot depend on gravitational/teleport without creating an import cycle.

ok, will remove this one, we have other 4 clients declaring the same

rosstimothy · 2024-09-01T21:53:30Z

api/client/autoupdate/autoupdate.go

+
+// Client is an AutoupdateService client that conforms to the following lib/services interfaces:
+//   - services.AutoupdateService
+type Client struct {


Do we need a dedicated client? There isn't much going on here besides hiding gRPC requests. Consider following the same approach that Noah took with the SPIFFE resource:

Add backend Service and Cache support for SPIFFE Federation resource #45054

Add SPIFFEFederation gRPC service and client support #45253

in codebase there are 3 ways how it is implemented, legacy one with interface in types, methods implementation and this one with wrapping grpc client to be interface compatible to service definition, this PR by iterations implemented all of them.

Last one was added because I was trying to eliminate methods declarations for client (as it was requested previously in this review)
1877695#diff-f5e514e352c12c01eab507e6d9bbdfa697791b35bf914d949b92245a8e950447R2873-R2889

and use only client wrapper:
1877695#diff-3bdff46f360deb2937ae725b5117a2462cf0bcaaff86400d08179c652a2ce2f3R2653

but its not possible right now because if we use cache wrapping, interfaces of the cache.Cache and authclient.ClientI must be compatible

interface ReadRemoteProxyAccessPoint { GetAutoupdateConfig(ctx context.Context) (*autoupdate.AutoupdateConfig, error) GetAutoupdateVersion(ctx context.Context) (*autoupdate.AutoupdateVersion, error) } type RemoteProxyAccessPoint interface { ReadRemoteProxyAccessPoint accessPoint } func NewRemoteProxyWrapper(base RemoteProxyAccessPoint, cache ReadRemoteProxyAccessPoint) RemoteProxyAccessPoint {...} // newLocalCacheForRemoteProxy returns new instance of access point configured for a remote proxy. func (process *TeleportProcess) newLocalCacheForRemoteProxy(clt authclient.ClientI, cacheName []string) (authclient.RemoteProxyAccessPoint, error) { // if caching is disabled, return access point if !process.Config.CachePolicy.Enabled { return clt, nil } cache, err := process.NewLocalCache(clt, cache.ForRemoteProxy, cacheName) if err != nil { return nil, trace.Wrap(err) } return authclient.NewRemoteProxyWrapper(clt, cache), nil }

vapopov · 2024-09-03T10:46:01Z

closed in prior to several PRs described in #45617 (review)

Add cluster autoupdate resources with enabled cache and

a3dd69d

vapopov requested a review from sclevine August 20, 2024 04:53

rosstimothy reviewed Aug 20, 2024

View reviewed changes

api/proto/teleport/legacy/client/proto/authservice.proto Outdated Show resolved Hide resolved

rosstimothy reviewed Aug 20, 2024

View reviewed changes

api/proto/teleport/legacy/types/types.proto Outdated Show resolved Hide resolved

rosstimothy reviewed Aug 20, 2024

View reviewed changes

api/types/autoupdate_config.go Outdated Show resolved Hide resolved

vapopov added 3 commits August 20, 2024 15:48

Refactor autoupdate service from legacy approach

4287f85

Merge remote-tracking branch 'origin/master' into vapopov/add-autoupd…

61c3443

…ate-resources

Add commands for autoupdate

db12db8

vapopov force-pushed the vapopov/add-autoupdate-resources branch from bd8e1d3 to db12db8 Compare August 21, 2024 17:42

vapopov added 3 commits August 22, 2024 00:53

Add test and fix CI

1e57600

Rename autoupdate config resource

e1aca13

Add cache test and comment fixes

0e45c8f

vapopov force-pushed the vapopov/add-autoupdate-resources branch from 4e2cd91 to 0e45c8f Compare August 22, 2024 07:03

sclevine reviewed Aug 22, 2024

View reviewed changes

api/proto/teleport/autoupdate/v1/autoupdate.proto Outdated Show resolved Hide resolved

api/client/webclient/webclient.go Outdated Show resolved Hide resolved

vapopov added 3 commits August 22, 2024 14:16

Rename tools_autoupdate

bb0e806

Merge remote-tracking branch 'origin/master' into vapopov/add-autoupd…

99a9e7e

…ate-resources

Fix events from latest change in backend lib

70b7c54

vapopov force-pushed the vapopov/add-autoupdate-resources branch from e24daff to 70b7c54 Compare August 22, 2024 18:36

vapopov marked this pull request as ready for review August 22, 2024 19:06

github-actions bot requested review from capnspacehook and fspmarshall August 22, 2024 19:06

github-actions bot added size/lg tctl tctl - Teleport admin tool labels Aug 22, 2024

vapopov added the backport/branch/v16 label Aug 22, 2024

sclevine approved these changes Aug 22, 2024

View reviewed changes

sclevine reviewed Aug 22, 2024

View reviewed changes

tool/tctl/common/autoupdate_command.go Outdated Show resolved Hide resolved

sclevine reviewed Aug 22, 2024

View reviewed changes

tool/tctl/common/autoupdate_command.go Outdated Show resolved Hide resolved

vapopov force-pushed the vapopov/add-autoupdate-resources branch from 6675c32 to 0184ab6 Compare August 22, 2024 20:15

Command flag naming adjustment

3b03379

Fix interval first duration

vapopov force-pushed the vapopov/add-autoupdate-resources branch from 0184ab6 to 3b03379 Compare August 22, 2024 21:48

rosstimothy reviewed Aug 23, 2024

View reviewed changes

vapopov added 4 commits August 23, 2024 16:10

Simplify client implementation for autoupdate

9796d4e

Add create/update api for autoupdate service

1877695

Add resource command for autoupdate config and version Limit to getter interface for client and cache part Add client wrapping

Add tests for service and resource validation

4e4962b

Drop autoupdate tctl command and find endpoint changes

12e1b79

Fix tests

b3a62d7

rosstimothy reviewed Aug 27, 2024

View reviewed changes

lib/auth/grpcserver.go Outdated Show resolved Hide resolved

rosstimothy reviewed Aug 27, 2024

View reviewed changes

lib/web/apiserver.go Outdated Show resolved Hide resolved

rosstimothy reviewed Aug 27, 2024

View reviewed changes

tool/tctl/common/resource_command.go Outdated Show resolved Hide resolved

Re-use cache for autoupdate service

4cceaac

Add tests for resources command

vapopov force-pushed the vapopov/add-autoupdate-resources branch from 083fb77 to 4cceaac Compare August 27, 2024 21:16

rosstimothy reviewed Sep 1, 2024

View reviewed changes

vapopov closed this Sep 3, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add cluster autoupdate resources with enabled cache #45617

Add cluster autoupdate resources with enabled cache #45617

vapopov commented Aug 20, 2024 •

edited

Loading

github-actions bot commented Aug 22, 2024

sclevine left a comment

rosstimothy left a comment

rosstimothy Aug 23, 2024

vapopov Aug 26, 2024

rosstimothy Aug 23, 2024

sclevine Aug 23, 2024

vapopov Aug 23, 2024

public-teleport-github-review-bot bot commented Aug 27, 2024

vapopov commented Aug 30, 2024

rosstimothy left a comment

rosstimothy Sep 1, 2024

rosstimothy Sep 1, 2024

vapopov Sep 2, 2024

rosstimothy Sep 1, 2024

vapopov Sep 2, 2024

sclevine Sep 3, 2024

rosstimothy Sep 1, 2024

rosstimothy Sep 1, 2024

vapopov Sep 2, 2024

rosstimothy Sep 1, 2024

vapopov Sep 2, 2024

vapopov commented Sep 3, 2024

		// Opting out of forward compatibility, this service must implement all service methods.
		autoupdate.UnsafeAutoupdateServiceServer

		autoupdateConfigPrefix = "autoupdate_config"
		autoupdateVersionPrefix = "autoupdate_version"

Add cluster autoupdate resources with enabled cache #45617

Add cluster autoupdate resources with enabled cache #45617

Conversation

vapopov commented Aug 20, 2024 • edited Loading

github-actions bot commented Aug 22, 2024

sclevine left a comment

Choose a reason for hiding this comment

rosstimothy left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

public-teleport-github-review-bot bot commented Aug 27, 2024

vapopov commented Aug 30, 2024

rosstimothy left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vapopov commented Sep 3, 2024

vapopov commented Aug 20, 2024 •

edited

Loading