Disable ingress session flag update by Rajiv91 · Pull Request #935 · livekit/protocol

Rajiv91 · 2024-12-30T23:27:13Z

New Enabled flags on createIngress and updateIngress to have the ability to disable ingress sessions without having to remove the entire session and lose the streamkey

changeset-bot · 2024-12-30T23:27:16Z

⚠️ No Changeset found

Latest commit: 1933f25

Merging this PR will not cause a version bump for any packages. If these changes should not result in a new version, you're good to go. If these changes should result in a version bump, you need to add a changeset.

Click here to learn what changesets are, and how to add one.

Click here if you're a maintainer who wants to add a changeset to this PR

💥 An error occurred when fetching the changed packages and changesets in this PR

Some errors occurred when validating the changesets config:
The package or glob expression "github.com/livekit/protocol" specified in the `fixed` option does not match any package in the project. You may have misspelled the package name or provided an invalid glob expression. Note that glob expressions must be defined according to https://www.npmjs.com/package/micromatch.

CLAassistant · 2024-12-30T23:27:18Z

All committers have signed the CLA.

biglittlebigben · 2025-01-01T14:20:21Z

  string participant_metadata = 14;
  bool reusable = 11;
  IngressState state = 12; // Description of error/stream non compliance and debug info for publisher otherwise (received bitrate, resolution, bandwidth)
+  optional bool session_enabled = 16;


I would simply use enabled here, rather than session_enabled as we currently do not use a session semantic in this API. Please also add a comment specifying that when false, new connection attempts to this ingress will fail, and that the default value is true.

biglittlebigben · 2025-01-01T14:21:27Z

  IngressVideoOptions video = 7;
-
-  // NEXT_ID: 11
+  optional bool session_enabled = 11;


Please update this similarly to above.

biglittlebigben · 2025-01-01T14:29:44Z

The validation code in ingress/validation.go:ValidateForSerialization also needs to fail if the flag is set to false on a non reusable ingress.

biglittlebigben · 2025-01-01T14:51:29Z

 	ErrNoResponse              = psrpc.NewErrorf(psrpc.Unavailable, "no response from ingress service")
 	ErrInvalidOutputDimensions = NewInvalidVideoParamsError("invalid output media dimensions")
 	ErrInvalidIngressType      = psrpc.NewErrorf(psrpc.InvalidArgument, "invalid ingress type")
+	ErrSessionDisabled         = psrpc.NewErrorf(psrpc.PermissionDenied, "session is disabled")


Please also rename the error to ErrIngressDisabled.

Got it, thanks for the feedback. I'll be working on it.

Hey @biglittlebigben I updated the branch with the feedback you requested:

ErrIngressDisabled for the new error variable

ValidateForSerialization in ingress/validation.go checking the enabled flag.

session_enabled now is just enabled.

CreateIngressRequest message includes the flag too.

And i updated the bootstrap script with the versions i see they are being used in main for those protoc dependencies.
And I have a question about the compiled proto that is causing conflicts here: replay/cloud_replay.pb.go. The conflict is because is the only file compiled with different versions of protoc and protoc-gen-go:
// protoc-gen-go v1.31.0
// protoc v4.24.3
Was this on purpose?
Do you want me to compile that single file with those old versions and push it or will the same versions be used now as in the rest of the project?
Let me know if something is missing, thanks in advance.

… enabled flag (useful for ingressWithUrl), ValidateForSerialization throws error if url input and enabled flag set false

biglittlebigben

Please see further comments. With regard to the conflict, the files generated by the Github action are the ones that should end up merged in the repository, and any conflict should be resolved accordingly.

biglittlebigben · 2025-01-06T16:58:47Z

-    go install google.golang.org/protobuf/cmd/protoc-gen-go@v1.31.0
-    go install google.golang.org/grpc/cmd/protoc-gen-go-grpc@v1.3
-    go install github.com/livekit/psrpc/protoc-gen-psrpc@v0.5.1
+    go install google.golang.org/protobuf/cmd/protoc-gen-go@v1.36.0


Please remove this change from this PR and create a separate PR for this version change if you believe this is needed, since this is unrelated to the initial PR purpose. Keeping unrelated changes to separate PRs help debugging and backing out a broken patch when needed.

biglittlebigben · 2025-01-06T17:01:16Z

 			return ErrInvalidIngress("no source URL")
 		}
+		if info.Enabled != nil && !*info.Enabled {
+			return ErrIngressDisabled


Please return ErrInvalidIngress("disabled non reusable ingress"), or a similar message. The issue here is that the ingress has an inconsistent set of parameters, not that is it merely disabled.

biglittlebigben · 2025-01-06T17:50:31Z

  string participant_metadata = 14;
  bool reusable = 11;
  IngressState state = 12; // Description of error/stream non compliance and debug info for publisher otherwise (received bitrate, resolution, bandwidth)
+  optional bool enabled = 16; // The default value is true and when set to false, the new connection attempts will be rejected


Also, just realizing while looking at the livekit SFU patch: this should not be optional as it is an output, so the returned value should always be explicit.

Got it, makes sense
Thanks

…ingress fixed, ingressInfo enabled not longer an optional

Rajiv91 · 2025-01-06T22:43:36Z

Hey @biglittlebigben I just updated the pr with your feedback.
Thanks

biglittlebigben

This looks good to me. Please approve the CLA and fix the conflict and I'll merge.

…oid conflicts

Rajiv91 · 2025-01-07T16:29:50Z

This looks good to me. Please approve the CLA and fix the conflict and I'll merge.

Thanks.
I already fixed the conflicts and aproved the CLA. And a reminder that this pr is linked with these other 2:
livekit/livekit#3293
livekit/ingress#319

Rajiv91 added 6 commits December 20, 2024 23:30

adding flag

2e01138

cleaning code

8c68e22

protoc changed to 4.234

3e3466c

changing protoc-gen-go version

8313f06

changing protoc-gen-go version

3165692

removing -I flag from magefile

14d9f92

This was referenced Dec 31, 2024

Reject ingress if Enabled flag is false livekit/livekit#3293

Merged

Reject ingress if Enabled flag is false livekit/ingress#319

Merged

biglittlebigben reviewed Jan 1, 2025

View reviewed changes

Rajiv91 added 6 commits January 2, 2025 21:30

session_enabled flag changed to enabled, CreateIngressReques includes…

dc49762

… enabled flag (useful for ingressWithUrl), ValidateForSerialization throws error if url input and enabled flag set false

protoc-ge-go updated to 1.36.0 in boostrat script

206432a

protoc-ge-go updated to 1.36.0 in boostrat script

c11aae3

protoc-ge-go-grpc updated in bootstrap.sh

f5f90a3

updating protoc-gen-psrpc

e668862

updating protoc-gen-psrpc in bootstrap.sh

4a81b4b

biglittlebigben reviewed Jan 6, 2025

View reviewed changes

removed bootsrap versions update, validation error for non reusabled …

480a622

…ingress fixed, ingressInfo enabled not longer an optional

biglittlebigben reviewed Jan 7, 2025

View reviewed changes

cloud_replay.pb.go updated to the used versions in remote repo and av…

1933f25

…oid conflicts

biglittlebigben approved these changes Jan 7, 2025

View reviewed changes

biglittlebigben merged commit e58b0bf into livekit:main Jan 7, 2025

Conversation

Rajiv91 commented Dec 30, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

changeset-bot Bot commented Dec 30, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

⚠️ No Changeset found

Uh oh!

CLAassistant commented Dec 30, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

biglittlebigben commented Jan 1, 2025

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!

biglittlebigben 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!

Rajiv91 commented Jan 6, 2025

Uh oh!

biglittlebigben left a comment

Choose a reason for hiding this comment

Uh oh!

Rajiv91 commented Jan 7, 2025

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

Rajiv91 commented Dec 30, 2024 •

edited

Loading

changeset-bot Bot commented Dec 30, 2024 •

edited

Loading

CLAassistant commented Dec 30, 2024 •

edited

Loading