[vnet] `vnet_config` resource #41399

nklaassen · 2024-05-10T00:39:33Z

This PR adds a storage layer, gRPC service, and tctl support for vnet_config resources https://github.com/gravitational/teleport/blob/master/rfd/0163-vnet.md

TODO: support resource bootstrap and add to the cache, tracked in #39330

zmb3 · 2024-05-10T14:21:41Z

lib/auth/crownjewel/crownjewelv1/service_test.go

@@ -180,7 +180,7 @@ type fakeChecker struct {

 func (f fakeChecker) CheckAccessToRule(_ services.RuleContext, _ string, resource string, verb string) error {
 if resource == types.KindCrownJewel {
- for slices.Contains(f.allowedVerbs, verb) {
+ if slices.Contains(f.allowedVerbs, verb) {


How did you end up finding this? Linter?

I copied the test and noticed it while making edits, in practice it makes no difference

lib/auth/vnetconfig/v1/service.go

lib/services/resource.go

nklaassen · 2024-05-10T20:25:46Z

lib/services/kubewaitingcontainer.go

@@ -41,57 +39,21 @@ type KubeWaitingContainer interface {

 // MarshalKubeWaitingContainer marshals a KubernetesWaitingContainer resource to JSON.
 func MarshalKubeWaitingContainer(in *kubewaitingcontainerpb.KubernetesWaitingContainer, opts ...MarshalOption) ([]byte, error) {
- if in == nil {
- return nil, trace.BadParameter("message is nil")
- }
 if err := kubewaitingcontainer.ValidateKubeWaitingContainer(in); err != nil {


this also does a nil check

nklaassen · 2024-05-14T14:44:35Z

friendly ping @tigrato and @GavinFrazar

lib/services/local/vnet_config.go

lib/auth/vnetconfig/v1/service.go

rosstimothy · 2024-05-14T20:26:13Z

lib/auth/vnetconfig/v1/service.go

+ // Opting out of forward compatibility, this service must implement all service methods.
+ vnet.UnsafeVnetConfigServiceServer
+
+ storage *local.VnetConfigService


We generally have some kind of interface to decouple the RPC service from the storage layer. Any reason not to follow that pattern as well here?

this is one area where I'm not really sure what we want to do going forward

we could have an interface here but I don't really see why, there is only one storage layer implementation that we'll use, is it to break the (api layer) -> (storage layer) dependency?

some other types use that same interface and implement it with both the storage layer and an api client. Just to take the most recent example, the CrownJewel service uses this interface in the gRPC api layer to abstract the storage layer, and also makes this gRPC client wrapper implement that same interface

teleport/lib/auth/crownjewel/crownjewelv1/service.go

Line 40 in 127184f

Backend services.CrownJewels

teleport/lib/auth/authclient/clt.go

Line 639 in 127184f

return c.APIClient.CrownJewelServiceClient()

I do see why it's useful to have authclient.Client.<SomeService>Client return an interface rather than a concrete type, so that you can mock authclient.Client and have it return a mocked <SomeService>Client. What I don't see is why you should need the storage layer to implement that same interface, but maybe it has something to do with the cache needing an interface too and it's just easier to have a single common interface? I just haven't dug that deep yet. Would be nice to have some guidance on this in RFD153 I just haven't figured it out

The interest I see here is to be able to mock the underlying storage and test error handling.
Usually we don't do it 😞

alright, switch to interface and added tests for storage-layer errors

lib/services/resource.go

rosstimothy · 2024-05-14T20:29:44Z

lib/services/resource.go

+ resource = proto.Clone(resource).(T)
+ //nolint:staticcheck // SA1019. Id is deprecated, but still needed.
+ resource.GetMetadata().Id = 0
+ resource.GetMetadata().Revision = ""


Can we use maybeResetProtoResourceID here?

I wrote a maybeResetProtoResourceIDv2 that works with the new types, in #41562

lib/services/resource.go

tigrato · 2024-05-15T08:45:20Z

lib/auth/vnetconfig/v1/service.go

+ // Opting out of forward compatibility, this service must implement all service methods.
+ vnet.UnsafeVnetConfigServiceServer
+
+ storage *local.VnetConfigService


The interest I see here is to be able to mock the underlying storage and test error handling.
Usually we don't do it 😞

lib/auth/vnetconfig/v1/service.go

lib/services/local/vnet_config.go

tool/tctl/common/collection.go

tool/tctl/common/resource_command.go

nklaassen · 2024-05-15T20:42:58Z

@tigrato this is ready for another look

tool/tctl/common/collection.go

tool/tctl/common/resource_command.go

lib/services/local/vnet_config.go

Co-authored-by: Tiago Silva <[email protected]>

nklaassen added the no-changelog Indicates that a PR does not require a changelog entry label May 10, 2024

github-actions bot added size/lg tctl tctl - Teleport admin tool labels May 10, 2024

github-actions bot requested review from GavinFrazar and tigrato May 10, 2024 00:40

nklaassen force-pushed the nklaassen/vnet-config branch from fc8694a to 4392e49 Compare May 10, 2024 02:44

zmb3 reviewed May 10, 2024

View reviewed changes

tigrato reviewed May 10, 2024

View reviewed changes

lib/auth/vnetconfig/v1/service.go Outdated Show resolved Hide resolved

lib/auth/vnetconfig/v1/service.go Outdated Show resolved Hide resolved

lib/auth/vnetconfig/v1/service.go Outdated Show resolved Hide resolved

lib/services/resource.go Outdated Show resolved Hide resolved

Base automatically changed from nklaassen/vnet-config-proto to master May 10, 2024 16:10

nklaassen commented May 10, 2024

View reviewed changes

nklaassen requested a review from tigrato May 14, 2024 14:40

nklaassen mentioned this pull request May 14, 2024

[vnet][6] IPv4 support #41542

Open

nklaassen requested a review from rosstimothy May 14, 2024 18:38

rosstimothy reviewed May 14, 2024

View reviewed changes

nklaassen force-pushed the nklaassen/vnet-config branch from aa7e4aa to e618b23 Compare May 14, 2024 22:30

nklaassen changed the base branch from master to nklaassen/marshalprotoresource May 14, 2024 22:30

tigrato reviewed May 15, 2024

View reviewed changes

nklaassen force-pushed the nklaassen/vnet-config branch from e618b23 to 3ffbe7e Compare May 15, 2024 18:18

Base automatically changed from nklaassen/marshalprotoresource to master May 15, 2024 18:35

nklaassen force-pushed the nklaassen/vnet-config branch from 5055d14 to 53ab4a0 Compare May 15, 2024 18:45

nklaassen requested review from rosstimothy and tigrato May 15, 2024 19:09

rosstimothy approved these changes May 15, 2024

View reviewed changes

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

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

nklaassen force-pushed the nklaassen/vnet-config branch from a2c4355 to ddc93ce Compare May 15, 2024 20:42

GavinFrazar reviewed May 15, 2024

View reviewed changes

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

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

lib/services/local/vnet_config.go Show resolved Hide resolved

nklaassen requested a review from GavinFrazar May 15, 2024 21:35

GavinFrazar approved these changes May 15, 2024

View reviewed changes

lib/services/local/vnet_config.go Outdated Show resolved Hide resolved

nklaassen enabled auto-merge May 15, 2024 22:34

nklaassen and others added 20 commits May 15, 2024 16:03

add vnet_config local storage service

21a5a13

add client to api/client

cfbd83d

grpc service wip

c0930c9

fix update verb

7d3f0ae

add permissions to editor role

3bd3c31

register grpc server

fa4527b

admin action + tests

43f24fe

tctl command

b8ac118

cidr_range -> ipv4_cidr_range

27b7afb

just do basic verb authz checks

041f435

godocs

d7532df

use interface implemented by storage layer

3d785df

allow reused mfa for upsert

7412d80

wrap errors

3c93d98

test storage-layer errors

0cfdf63

comma-separate

3c1ef05

Co-authored-by: Tiago Silva <[email protected]>

authclient

b7d7609

use table in text output

c8fa3b8

validate custom dns zones

8112d01

better zone validation

66aa928

nklaassen force-pushed the nklaassen/vnet-config branch from eb9d66d to 66aa928 Compare May 15, 2024 23:06

make fix-license

663e418

nklaassen added this pull request to the merge queue May 15, 2024

github-merge-queue bot removed this pull request from the merge queue due to failed status checks May 15, 2024

nklaassen added this pull request to the merge queue May 16, 2024

Merged via the queue into master with commit 64d5d25 May 16, 2024
37 checks passed

nklaassen deleted the nklaassen/vnet-config branch May 16, 2024 01:41

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[vnet] `vnet_config` resource #41399

[vnet] `vnet_config` resource #41399

nklaassen commented May 10, 2024 •

edited

zmb3 May 10, 2024

nklaassen May 10, 2024

nklaassen May 10, 2024

nklaassen commented May 14, 2024

rosstimothy May 14, 2024

nklaassen May 14, 2024 •

edited

tigrato May 15, 2024

nklaassen May 15, 2024

rosstimothy May 14, 2024

nklaassen May 14, 2024

tigrato May 15, 2024

nklaassen commented May 15, 2024

[vnet] vnet_config resource #41399

[vnet] vnet_config resource #41399

Conversation

nklaassen commented May 10, 2024 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nklaassen commented May 14, 2024

Choose a reason for hiding this comment

nklaassen May 14, 2024 • edited

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

nklaassen commented May 15, 2024

[vnet] `vnet_config` resource #41399

[vnet] `vnet_config` resource #41399

nklaassen commented May 10, 2024 •

edited

nklaassen May 14, 2024 •

edited