Skip to content

ssh: add api definitions #7

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

Merged
merged 2 commits into from
May 8, 2025
Merged

ssh: add api definitions #7

merged 2 commits into from
May 8, 2025

Conversation

kralicky
Copy link
Collaborator

@kralicky kralicky commented May 7, 2025

No description provided.

@kralicky kralicky requested a review from kenjenkins May 7, 2025 21:00
@kralicky kralicky force-pushed the kralicky/ssh-api-definitions branch from f9731c8 to fcaed2e Compare May 7, 2025 21:54
}

message RecordingData {
oneof data {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Would this be a good place for a few comments about the expected order of these data chunks?

Is it basically:

  • 1 chunk with the metadata
  • n chunks of the stream recording contents
  • 1 final chunk with the checksum?


option go_package = "github.com/pomerium/envoy-custom/api/extensions/filters/network/ssh";

message Config {}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this empty message required by the Generic Proxy architecture? If so it might be nice to add a comment mentioning that here.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This was an old unused message, will remove it

// temporarily becomes the upstream server, and delays the connection to the real upstream
// until this stream is closed. Afterwards, Envoy connects to the real upstream server and
// "hands off" the stream internally in a way that is transparent to the downstream.
rpc ServeChannel(stream ChannelMessage) returns (stream ChannelMessage);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In the interest of splitting things up into small chunks, should we hold off on adding this RPC and its associated messages for now?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No, I would prefer to keep this here for now

@@ -2,6 +2,6 @@ syntax = "proto3";

package pomerium.extensions;

option go_package = "github.com/pomerium/envoy-custom/source/extensions/http/early_header_mutation/trace_context";
option go_package = "github.com/pomerium/envoy-custom/api/extensions/http/early_header_mutation/trace_context";
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Definitely good to be consistent in the import path we use for these protos, but should this go in a separate PR?

}

enum PacketDirection {
NONE = 0;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Which packets do not have a direction? (Or should this be the standard UNKNOWN enum value?)

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think I was going to use this for something, but didn't end up needing it.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Would it be worth adding an UNKNOWN anyway, just in case we need to extend this enum later?

@kralicky kralicky force-pushed the kralicky/ssh-api-definitions branch from edc6033 to fbbbe6e Compare May 8, 2025 21:09
@kralicky kralicky force-pushed the kralicky/ssh-api-definitions branch from fbbbe6e to e4f16ca Compare May 8, 2025 21:09
@kralicky kralicky merged commit 13edfd6 into main May 8, 2025
1 check passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants