-
Notifications
You must be signed in to change notification settings - Fork 4
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
feat: add CLI support for Agent Hub #82
base: main
Are you sure you want to change the base?
Conversation
6247ab4
to
8b99231
Compare
be296f0
to
fcd0ecd
Compare
@matewolf As discussed, I think we could improve the PR with the following suggestions:
|
1c35845
to
db6fb79
Compare
plugins: | ||
- remote: buf.build/grpc/go:v1.5.1 | ||
out: . | ||
opt: paths=source_relative | ||
- remote: buf.build/protocolbuffers/go:v1.36.5 | ||
out: . | ||
opt: paths=source_relative |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
here you are using the same plugins that we use for the core/store/routing APIs, so we could generate hub code with the same api/buf.gen.yaml
file. this would also avoid simplify the changes in Taskfile.yml
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I wouldn't like to use the same buf file and integrate deps:api:gen
task because running this task would need a permission to reach the private hub repo. Maybe it is possible to exclude some module from buf generate
command.
@@ -39,6 +39,8 @@ vars: | |||
GOLANGCI_LINT_BIN: '{{ .BIN_DIR }}/golangci-lint-{{.GOLANGCI_LINT_VERSION}}' | |||
LICENSEI_VERSION: '0.9.0' | |||
LICENSEI_BIN: '{{ .BIN_DIR }}/licensei-{{.LICENSEI_VERSION}}' | |||
HUB_API_VERSION: 'main' | |||
HUB_REPO_URL: 'https://github.com/cisco-eti/phoenix-saas-be.git' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
should we keep this reference to an internal repo in the open repo? the git clone
below will fail for users without access
unpublish.Command, | ||
publish.NewCommand(baseOption), | ||
list.NewCommand(baseOption), | ||
unpublish.NewCommand(baseOption), | ||
network.Command, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this new command needs to be refactored as well
@@ -17,7 +16,6 @@ func main() { | |||
ctx, cancel := signal.NotifyContext(context.Background(), os.Interrupt, syscall.SIGHUP, syscall.SIGTERM) | |||
|
|||
if err := cmd.Run(ctx); err != nil { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
was this print removed by mistake?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No. It was intended. It resulted that the error message was printed twice. Once on stderr and once on stdout.
This command handles the build process for agent data models | ||
from source code. It generates a JSON object that | ||
describes an agent and satisfies the **Open Agent Schema Framework** specification. | ||
|
||
Usage examples: | ||
|
||
1. When build config is present under the agent source code: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
let's keep the formatting here, since the same numbered formatting is used in the push and pull commands
# List summary about our published data. | ||
dir list info | ||
|
||
2. List summary about published data across the network: | ||
|
||
# List summary about published data by a specific peer. | ||
dir list info --peer <peer-id> | ||
|
||
# List summary about published data by the whole network. | ||
# NOTE: This starts a DHT walk, so it may take a while. | ||
# NOTE: Results are not guaranteed to be complete and up-to-date. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
there might have been a merge conflict issue here, please check
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This PR is big, so it makes sense to add my reviews step by step. I continue my review.
- proto_file: saas/v1alpha1/agent_id_response.proto | ||
- proto_file: saas/v1alpha1/agent.proto | ||
- proto_file: saas/v1alpha1/category.proto | ||
- proto_file: saas/v1alpha1/locator.proto |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There is no locator.proto
in the saas/v1alpha1
package. We're using the `core/v1alpha1/locator.prot
return fmt.Errorf("failed to parse repo id: %w", err) | ||
} | ||
|
||
ctx := metadata.NewOutgoingContext(context.Background(), metadata.Pairs("authorization", "Bearer "+session.Tokens[session.CurrentTenant].AccessToken)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ctx := metadata.NewOutgoingContext(context.Background(), metadata.Pairs("authorization", "Bearer "+session.Tokens[session.CurrentTenant].AccessToken)) | |
if session.Tokens == nil { | |
return nil, errors.New("session tokens not initialized") | |
} | |
tokens, exists := session.Tokens[session.CurrentTenant] | |
if !exists { | |
return nil, errors.New("no tokens found for current tenant") | |
} | |
if tokens == nil { | |
return nil, errors.New("tokens for current tenant are nil") | |
} | |
if tokens.AccessToken == "" { | |
return nil, errors.New("access token is empty") | |
} | |
ctx := metadata.NewOutgoingContext(context.Background(), metadata.Pairs("authorization", "Bearer "+tokens.AccessToken)) |
if err = json.Unmarshal(body, &authConfig); err != nil { | ||
return nil, fmt.Errorf("%w: %w", ErrParsingConfig, err) | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Possible nil pointer
if err = json.Unmarshal(body, &authConfig); err != nil { | |
return nil, fmt.Errorf("%w: %w", ErrParsingConfig, err) | |
} | |
if err = json.Unmarshal(body, &authConfig); err != nil { | |
return nil, fmt.Errorf("%w: %w", ErrParsingConfig, err) | |
} | |
if authConfig == nil { | |
return nil, fmt.Errorf("%w: config is nil", ErrParsingConfig) | |
} |
} | ||
|
||
func runCmd(outStream io.Writer, opts *options.HubOptions, session *sessionstore.HubSession, secretStore sessionstore.SessionStore, oktaClient okta.Client) error { | ||
resp, err := oktaClient.Logout(&okta.LogoutRequest{IDToken: session.Tokens[session.CurrentTenant].IDToken}) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
resp, err := oktaClient.Logout(&okta.LogoutRequest{IDToken: session.Tokens[session.CurrentTenant].IDToken}) | |
if session == nil { | |
return errors.New("session is nil") | |
} | |
if session.Tokens == nil { | |
return errors.New("session tokens are nil") | |
} | |
tokens, ok := session.Tokens[session.CurrentTenant] | |
if !ok { | |
return fmt.Errorf("no tokens found for tenant: %s", session.CurrentTenant) | |
} | |
if tokens == nil { | |
return errors.New("tokens for current tenant are nil") | |
} | |
if tokens.IDToken == "" { | |
return errors.New("ID token is empty") | |
} | |
resp, err := oktaClient.Logout(&okta.LogoutRequest{IDToken: tokens.IDToken}) |
if session.Tokens != nil && session.Tokens[session.CurrentTenant].AccessToken != "" { | ||
ctx = metadata.NewOutgoingContext(ctx, metadata.Pairs("authorization", "Bearer "+session.Tokens[session.CurrentTenant].AccessToken)) | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
if session.Tokens != nil && session.Tokens[session.CurrentTenant].AccessToken != "" { | |
ctx = metadata.NewOutgoingContext(ctx, metadata.Pairs("authorization", "Bearer "+session.Tokens[session.CurrentTenant].AccessToken)) | |
} | |
if session.Tokens != nil { | |
tokens, exists := session.Tokens[session.CurrentTenant] | |
if exists && tokens != nil && tokens.AccessToken != "" { | |
ctx = metadata.NewOutgoingContext(ctx, metadata.Pairs("authorization", "Bearer "+tokens.AccessToken)) | |
} | |
} |
return nil, fmt.Errorf("%w: %w", ErrFetchingConfig, err) | ||
} | ||
|
||
resp, err := http.DefaultClient.Do(req) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We could use later something more secure http client as we did in panoptica/policy-framework. For example
func createSecureHTTPClient() *http.Client {
return &http.Client{
Timeout: 30 * time.Second,
Transport: &http.Transport{
TLSClientConfig: &tls.Config{
MinVersion: tls.VersionTLS12,
InsecureSkipVerify: false,
},
MaxIdleConns: 100,
MaxIdleConnsPerHost: 10,
IdleConnTimeout: 90 * time.Second,
},
}
}
} | ||
|
||
if resp.StatusCode != http.StatusOK { | ||
return nil, fmt.Errorf("%w: %w: (%d) %s", ErrFetchingConfig, errors.New("unexpected status code"), resp.StatusCode, resp.Body) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe this error messages expose internal details that could be useful for attackers.
} | ||
|
||
func FetchAuthConfig(frontedURL string) (*AuthConfig, error) { | ||
if !urlutil.IsURL(frontedURL) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As I see, IsURL
only checks the Schema. You can make the same with Go-s url package url.Scheme,
parsedURL, err := url.Parse(url)
if err != nil {
return "", ErrInvalidFrontendURL
}
if parsedURL.Scheme != "https" {
return "", fmt.Errorf("URL scheme must be HTTPS")
}
but it would make sense to create a method validateAndSanitizeURL
to validate and sanitize URLs
Signed-off-by: matewolf <[email protected]>
Signed-off-by: matewolf <[email protected]>
Signed-off-by: matewolf <[email protected]>
Signed-off-by: matewolf <[email protected]>
Signed-off-by: matewolf <[email protected]>
Signed-off-by: matewolf <[email protected]>
Signed-off-by: matewolf <[email protected]>
Signed-off-by: matewolf <[email protected]>
Signed-off-by: matewolf <[email protected]>
Signed-off-by: matewolf <[email protected]>
Signed-off-by: matewolf <[email protected]>
Signed-off-by: matewolf <[email protected]>
Signed-off-by: matewolf <[email protected]>
Signed-off-by: matewolf <[email protected]>
Signed-off-by: matewolf <[email protected]>
Signed-off-by: matewolf <[email protected]>
Signed-off-by: Zsolt Rappi <[email protected]>
Signed-off-by: matewolf <[email protected]>
Signed-off-by: matewolf <[email protected]>
Signed-off-by: matewolf <[email protected]>
Signed-off-by: matewolf <[email protected]>
Signed-off-by: matewolf <[email protected]>
Signed-off-by: matewolf <[email protected]>
Signed-off-by: matewolf <[email protected]>
Signed-off-by: matewolf <[email protected]>
Signed-off-by: matewolf <[email protected]>
Signed-off-by: matewolf <[email protected]>
Signed-off-by: matewolf <[email protected]>
Signed-off-by: matewolf <[email protected]>
Signed-off-by: matewolf <[email protected]>
Signed-off-by: matewolf <[email protected]>
No description provided.