COCOS - 209 - Restructure agent and manager gRPC config #297

WashingtonKK · 2024-11-01T12:27:15Z

What type of PR is this?

This is a refactor of agent and manager grpc configs.

What does this do?

Introduced a new configuration structure for gRPC clients and servers, enhancing integration and management of settings.

Which issue(s) does this PR fix/relate to?

Related Issue #
Resolves Feature: Restructure Configuration for Manager and Agent Services #209

Have you included tests for your changes?

Yes

Did you document any new/modified feature?

Notes

SammyOina · 2024-11-01T12:32:04Z

.vscode/launch.json

should not be tracked

SammyOina · 2024-11-01T12:34:04Z

cocos-manager.env

these changes are not needed

SammyOina · 2024-11-01T12:34:46Z

internal/server/server.go

 	CertFile     string `env:"SERVER_CERT"        envDefault:""`
 	KeyFile      string `env:"SERVER_KEY"         envDefault:""`
 	ServerCAFile string `env:"SERVER_CA_CERTS"    envDefault:""`


should be part of server base config

SammyOina · 2024-11-01T12:38:04Z

pkg/clients/grpc/connect.go

+type BaseConfig struct {
+	URL     string        `env:"URL"             envDefault:"localhost:7001"`
+	Timeout time.Duration `env:"TIMEOUT"         envDefault:"60s"`
+}
+
+type ManagerConfig struct {
+	BaseConfig
+	ClientCert   string `env:"CLIENT_CERT"     envDefault:""`
+	ClientKey    string `env:"CLIENT_KEY"      envDefault:""`
+	ServerCAFile string `env:"SERVER_CA_CERTS" envDefault:""`
+	BackendInfo  string `env:"BACKEND_INFO"    envDefault:""`
+	ClientTLS  bool   `env:"CLIENT_TLS"    envDefault:"false"`
+
 }


you need cli config as well(agent client)

SammyOina · 2024-11-01T12:39:06Z

pkg/clients/grpc/connect.go

 var _ Client = (*client)(nil)

-func NewClient(cfg Config) (Client, error) {
+func NewClient(cfg ManagerConfig) (Client, error) {


should handle both manager and agent client configs

drasko · 2024-11-01T20:43:16Z

Fix the title

codecov · 2024-11-03T21:07:46Z

Codecov Report

Attention: Patch coverage is 76.92308% with 30 lines in your changes missing coverage. Please review.

Project coverage is 66.38%. Comparing base (92a4f8b) to head (58dc3ef).
Report is 1 commits behind head on main.

Files with missing lines	Patch %	Lines
internal/server/grpc/grpc.go	74.02%	13 Missing and 7 partials ⚠️
pkg/clients/grpc/connect.go	89.13%	4 Missing and 1 partial ⚠️
internal/server/server.go	0.00%	4 Missing ⚠️
cli/sdk.go	0.00%	1 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main     #297      +/-   ##
==========================================
+ Coverage   66.15%   66.38%   +0.23%     
==========================================
  Files          53       53              
  Lines        4328     4349      +21     
==========================================
+ Hits         2863     2887      +24     
+ Misses       1195     1192       -3     
  Partials      270      270

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

SammyOina · 2024-11-18T12:03:25Z

internal/server/grpc/grpc.go


-func New(ctx context.Context, cancel context.CancelFunc, name string, config server.Config, registerService serviceRegister, logger *slog.Logger, qp client.QuoteProvider, authSvc auth.Authenticator) server.Server {
-	listenFullAddress := fmt.Sprintf("%s:%s", config.Host, config.Port)
+func New(ctx context.Context, cancel context.CancelFunc, name string, config server.AgentConfig, registerService serviceRegister, logger *slog.Logger, qp client.QuoteProvider, authSvc auth.Authenticator) server.Server {


this function is used by computations server which is different from agent, use interface type and type assertion things specific to agent server

SammyOina · 2024-11-18T12:03:29Z

internal/server/grpc/grpc.go


-func New(ctx context.Context, cancel context.CancelFunc, name string, config server.Config, registerService serviceRegister, logger *slog.Logger, qp client.QuoteProvider, authSvc auth.Authenticator) server.Server {
-	listenFullAddress := fmt.Sprintf("%s:%s", config.Host, config.Port)
+func New(ctx context.Context, cancel context.CancelFunc, name string, config server.AgentConfig, registerService serviceRegister, logger *slog.Logger, qp client.QuoteProvider, authSvc auth.Authenticator) server.Server {


this function is used by computations server which is different from agent, use interface type and type assertion things specific to agent server

you can create a custom interface with a method such as getBaseConfig, and then do type assertion for specific fields like attested tls

SammyOina · 2024-11-18T12:07:23Z

pkg/clients/grpc/agent/agent.go

 // NewAgentClient creates new agent gRPC client instance.
-func NewAgentClient(ctx context.Context, cfg grpc.Config) (grpc.Client, agent.AgentServiceClient, error) {
-	client, err := grpc.NewClient(cfg)
+func NewAgentClient(ctx context.Context, cfg server.AgentConfig) (grpc.Client, agent.AgentServiceClient, error) {


client and server config are distinct

SammyOina · 2024-11-18T12:07:34Z

pkg/clients/grpc/agent/agent.go

 // NewAgentClient creates new agent gRPC client instance.
-func NewAgentClient(ctx context.Context, cfg grpc.Config) (grpc.Client, agent.AgentServiceClient, error) {
-	client, err := grpc.NewClient(cfg)
+func NewAgentClient(ctx context.Context, cfg server.AgentConfig) (grpc.Client, agent.AgentServiceClient, error) {


client and server config are distinct

SammyOina · 2024-11-18T12:07:39Z

pkg/clients/grpc/agent/agent.go

 // NewAgentClient creates new agent gRPC client instance.
-func NewAgentClient(ctx context.Context, cfg grpc.Config) (grpc.Client, agent.AgentServiceClient, error) {
-	client, err := grpc.NewClient(cfg)
+func NewAgentClient(ctx context.Context, cfg server.AgentConfig) (grpc.Client, agent.AgentServiceClient, error) {


client and server config are distinct

SammyOina · 2024-11-18T12:07:41Z

pkg/clients/grpc/agent/agent.go

 // NewAgentClient creates new agent gRPC client instance.
-func NewAgentClient(ctx context.Context, cfg grpc.Config) (grpc.Client, agent.AgentServiceClient, error) {
-	client, err := grpc.NewClient(cfg)
+func NewAgentClient(ctx context.Context, cfg server.AgentConfig) (grpc.Client, agent.AgentServiceClient, error) {


client and server config are distinct

SammyOina · 2024-11-18T12:09:10Z

pkg/clients/grpc/manager/manager.go


+type ManagerConfig struct {
+	grpc.BaseConfig
+	BackendInfo string `env:"BACKEND_INFO"    envDefault:""`


manager client config is not configured using backendinfo, this exclusive to agent client(CLI). also clienttls should be part of base client config

SammyOina · 2024-11-18T12:10:12Z

pkg/clients/grpc/connect.go

+	BackendInfo string `env:"BACKEND_INFO"    envDefault:""`
+	ClientTLS   bool   `env:"CLIENT_TLS"    envDefault:"false"`
+	AttestedTLS bool   `env:"ATTESTED_TLS"       envDefault:"false"`


backendinfo and attested tls are only for agent client(CLI). This needs to bbe distinguished. Use interfaces for configuration purposes. clienttls is common for all clients and should be part of base config

SammyOina · 2024-11-18T12:10:15Z

pkg/clients/grpc/connect.go

+	BackendInfo string `env:"BACKEND_INFO"    envDefault:""`
+	ClientTLS   bool   `env:"CLIENT_TLS"    envDefault:"false"`
+	AttestedTLS bool   `env:"ATTESTED_TLS"       envDefault:"false"`


backendinfo and attested tls are only for agent client(CLI). This needs to bbe distinguished. Use interfaces for configuration purposes. clienttls is common for all clients and should be part of base config

SammyOina · 2024-11-29T11:49:35Z

pkg/clients/grpc/connect.go

+
+type ManagerClientConfig struct {
+	ClientConfig
+	AttestedTLS bool `env:"ATTESTED_TLS"       envDefault:"false"`


manager cannot have attested TLS

SammyOina · 2024-11-29T11:50:57Z

test/computations/main.go

+	grpcServerConfig := server.ServerConfig{
+		BaseConfig: server.BaseConfig{
+			URL: fmt.Sprintf("localhost:%s", defaultPort),
+		},
+	}


env only has host and port, so set url in code after parsing config

SammyOina · 2024-11-29T11:51:47Z

pkg/clients/grpc/connect.go

+
+type AgentClientConfig struct {
+	ClientConfig
+	AttestationPolicy string `env:"ATTESTATION_POLICY" envDefault:""`


attested tls bool?

SammyOina · 2024-11-29T11:52:08Z

pkg/clients/grpc/connect.go

+	CertFile     string        `env:"SERVER_CERT"        envDefault:""`
+	KeyFile      string        `env:"SERVER_KEY"         envDefault:""`
+	ClientCAFile string        `env:"CLIENT_CA_CERTS"    envDefault:""`
+	ClientTLS    bool          `env:"CLIENT_TLS"    envDefault:"false"`


why was this added

SammyOina · 2024-11-29T11:52:37Z

pkg/clients/grpc/connect.go

+	CertFile     string        `env:"SERVER_CERT"        envDefault:""`
+	KeyFile      string        `env:"SERVER_KEY"         envDefault:""`


server certs are not used on client

SammyOina · 2024-11-29T11:53:23Z

pkg/clients/grpc/connect.go

-		}
+	switch s := cfg.(type) {
+	case AgentClientConfig:
+		if s.ClientTLS {


this code is breaking existing functionality

SammyOina · 2024-12-02T14:49:18Z

pkg/clients/grpc/connect.go

+	case AgentClientConfig:
+		if s.AttestedTLS {
+			err := ReadAttestationPolicy(s.AttestationPolicy, &quoteprovider.AttConfigurationSEVSNP)
+			if err != nil {
+				return nil, secure, errors.Wrap(fmt.Errorf("failed to read Attestation Policy"), err)
+			}

-		tlsConfig := &tls.Config{
-			InsecureSkipVerify:    true,
-			VerifyPeerCertificate: verifyPeerCertificateATLS,
+			tlsConfig := &tls.Config{
+				InsecureSkipVerify:    true,
+				VerifyPeerCertificate: verifyPeerCertificateATLS,
+			}
+			tc = credentials.NewTLS(tlsConfig)
+			opts = append(opts, grpc.WithContextDialer(CustomDialer))
+			secure = withaTLS
 		}


will fail if agentconfig is any other config other than atls

SammyOina · 2024-12-02T14:50:15Z

pkg/clients/grpc/connect.go

+func (s ClientConfig) GetBaseConfig() BaseConfig {
+	return s.BaseConfig
+}


why do you need both client and base config, clients are only two CLI(Agent) and manager

SammyOina · 2024-12-03T11:31:52Z

pkg/clients/grpc/connect.go

+		}
+	default:
+		conf := cfg.GetBaseConfig()
+		if conf.ServerCAFile != "" {


repeated in loadTLSConfig

SammyOina · 2024-12-03T11:32:02Z

pkg/clients/grpc/connect.go

+			opts = append(opts, grpc.WithContextDialer(CustomDialer))
+			secure = withaTLS
+		} else {
+			if s.ServerCAFile != "" {


Signed-off-by: WashingtonKK <washingtonkigan@gmail.com> enhance clients Signed-off-by: WashingtonKK <washingtonkigan@gmail.com> restructure config Signed-off-by: WashingtonKK <washingtonkigan@gmail.com> refactor Signed-off-by: WashingtonKK <washingtonkigan@gmail.com> rebase Signed-off-by: WashingtonKK <washingtonkigan@gmail.com> rebase Signed-off-by: WashingtonKK <washingtonkigan@gmail.com> use separate configuration Signed-off-by: WashingtonKK <washingtonkigan@gmail.com> fix tests Signed-off-by: WashingtonKK <washingtonkigan@gmail.com> fix config Signed-off-by: WashingtonKK <washingtonkigan@gmail.com> refactor Signed-off-by: WashingtonKK <washingtonkigan@gmail.com> Lint Signed-off-by: WashingtonKK <washingtonkigan@gmail.com> fix tests Signed-off-by: WashingtonKK <washingtonkigan@gmail.com> add tests Signed-off-by: WashingtonKK <washingtonkigan@gmail.com> add test case Signed-off-by: WashingtonKK <washingtonkigan@gmail.com> add test case Signed-off-by: WashingtonKK <washingtonkigan@gmail.com> refactor Signed-off-by: WashingtonKK <washingtonkigan@gmail.com> further refactor' Signed-off-by: WashingtonKK <washingtonkigan@gmail.com> add tests Signed-off-by: WashingtonKK <washingtonkigan@gmail.com> rebase Signed-off-by: WashingtonKK <washingtonkigan@gmail.com>

SammyOina · 2024-12-04T16:17:07Z

internal/server/grpc/grpc_test.go

 }

-func TestServerStartWithTLSFile(t *testing.T) {
+func TestServerStartWithTLS(t *testing.T) {


Do not rename the test name here

SammyOina · 2024-12-04T16:18:05Z

internal/server/grpc/grpc_test.go

+	assert.Contains(t, logContent, "TestServer service gRPC server listening at localhost:0 with TLS")
+}
+
+func TestServerStartWithMTLS(t *testing.T) {


Already in another test cas

Signed-off-by: WashingtonKK <washingtonkigan@gmail.com>

drasko

LGTM

SammyOina requested changes Nov 1, 2024

View reviewed changes

WashingtonKK changed the title ~~cocos 209~~ (DRAFT) COCOS - 209 - Restructure agent and manager gRPC config Nov 2, 2024

WashingtonKK marked this pull request as draft November 2, 2024 09:04

WashingtonKK force-pushed the cocos-209 branch from ed5284e to 335a90c Compare November 3, 2024 21:04

WashingtonKK changed the title ~~(DRAFT) COCOS - 209 - Restructure agent and manager gRPC config~~ COCOS - 209 - Restructure agent and manager gRPC config Nov 3, 2024

WashingtonKK marked this pull request as ready for review November 3, 2024 21:14

WashingtonKK self-assigned this Nov 3, 2024

WashingtonKK force-pushed the cocos-209 branch from 335a90c to 8c9e4fe Compare November 7, 2024 09:29

SammyOina requested changes Nov 18, 2024

View reviewed changes

WashingtonKK force-pushed the cocos-209 branch 2 times, most recently from 6e7b198 to 9cf4c5f Compare November 29, 2024 07:37

SammyOina requested changes Nov 29, 2024

View reviewed changes

SammyOina requested changes Dec 2, 2024

View reviewed changes

SammyOina requested changes Dec 3, 2024

View reviewed changes

WashingtonKK force-pushed the cocos-209 branch from 811e3df to 3553ddb Compare December 4, 2024 15:00

WashingtonKK force-pushed the cocos-209 branch from 3553ddb to 28c7511 Compare December 4, 2024 15:01

SammyOina requested changes Dec 4, 2024

View reviewed changes

remove redundant code

70a8ac5

Signed-off-by: WashingtonKK <washingtonkigan@gmail.com>

WashingtonKK requested a review from SammyOina December 4, 2024 16:49

fix test

58dc3ef

Signed-off-by: WashingtonKK <washingtonkigan@gmail.com>

SammyOina approved these changes Dec 4, 2024

View reviewed changes

drasko approved these changes Dec 4, 2024

View reviewed changes

drasko merged commit ec426e5 into ultravioletrs:main Dec 4, 2024
3 checks passed

		CertFile string `env:"SERVER_CERT" envDefault:""`
		KeyFile string `env:"SERVER_KEY" envDefault:""`

COCOS - 209 - Restructure agent and manager gRPC config #297

COCOS - 209 - Restructure agent and manager gRPC config #297

Uh oh!

Conversation

WashingtonKK commented Nov 1, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

What type of PR is this?

What does this do?

Which issue(s) does this PR fix/relate to?

Have you included tests for your changes?

Did you document any new/modified feature?

Notes

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!

drasko commented Nov 1, 2024

Uh oh!

codecov bot commented Nov 3, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

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!

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!

drasko left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

WashingtonKK commented Nov 1, 2024 •

edited

Loading

codecov bot commented Nov 3, 2024 •

edited

Loading