Add cluster autoupdate resources with enabled cache by vapopov · Pull Request #45617 · gravitational/teleport

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)

…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

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.

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

+// 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

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

+// 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

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.

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

+	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

+	// 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

+	// 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

+	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

+)
+
+// 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

+
+// 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

Comment thread api/proto/teleport/legacy/client/proto/authservice.proto Outdated

rosstimothy reviewed Aug 20, 2024

View reviewed changes

Comment thread api/proto/teleport/legacy/types/types.proto Outdated

rosstimothy reviewed Aug 20, 2024

View reviewed changes

Comment thread api/types/autoupdate_config.go Outdated

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

Comment thread api/proto/teleport/autoupdate/v1/autoupdate.proto Outdated

Comment thread api/client/webclient/webclient.go Outdated

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

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

sclevine reviewed Aug 22, 2024

View reviewed changes

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

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

Comment thread lib/auth/grpcserver.go Outdated

rosstimothy reviewed Aug 27, 2024

View reviewed changes

Comment thread lib/web/apiserver.go Outdated

rosstimothy reviewed Aug 27, 2024

View reviewed changes

Comment thread tool/tctl/common/resource_command.go

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

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

		autoupdateConfigPrefix = "autoupdate_config"
		autoupdateVersionPrefix = "autoupdate_version"

Conversation

vapopov commented Aug 20, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

github-actions Bot commented Aug 22, 2024

Uh oh!

sclevine left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

rosstimothy left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

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!

Uh oh!

Uh oh!

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

Uh oh!

Uh oh!

Uh oh!

Uh oh!

vapopov commented Aug 30, 2024

Uh oh!

rosstimothy left a comment

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!

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!

vapopov commented Sep 3, 2024

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

vapopov commented Aug 20, 2024 •

edited

Loading