Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Add allow to discover #47

Merged
merged 2 commits into from
Mar 20, 2024
Merged

Add allow to discover #47

merged 2 commits into from
Mar 20, 2024

Conversation

uncleDecart
Copy link
Member

This flag will allow to expose app instance IP address
via metadata server. Could be useful when you have certain
topology and don't want to have static IP routes, i.e.
sidecar applications.

eriknordmark
eriknordmark reviewed Mar 14, 2024
View reviewed changes
Copy link
Contributor

@eriknordmark eriknordmark left a comment

Choose a reason for hiding this comment

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

I thought the idea was to only have setting on the app instance to allow discover. Am I confused?

@uncleDecart uncleDecart force-pushed the add-allow-to-discover branch from 9e91b58 to b541e02 Compare March 14, 2024 11:23
@uncleDecart
Copy link
Member Author

I thought the idea was to only have setting on the app instance to allow discover. Am I confused?

We can move it to app instance level, but this way we can give access granularly, i.e. expose specific port of app instance

@eriknordmark
Copy link
Contributor

I thought the idea was to only have setting on the app instance to allow discover. Am I confused?

We can move it to app instance level, but this way we can give access granularly, i.e. expose specific port of app instance

The initial proposal was to have it both on the network instance and the app instance, which seems overkill.
Only on the network instance means that anybody and everybody can discover, which seems to relaxed.
But only on the app instance should be sufficient for the initial use for the sidecar container, and less to configure.

@uncleDecart uncleDecart force-pushed the add-allow-to-discover branch from b541e02 to ef68bec Compare March 14, 2024 15:43
@uncleDecart
Copy link
Member Author

I thought the idea was to only have setting on the app instance to allow discover. Am I confused?

We can move it to app instance level, but this way we can give access granularly, i.e. expose specific port of app instance

The initial proposal was to have it both on the network instance and the app instance, which seems overkill.
Only on the network instance means that anybody and everybody can discover, which seems to relaxed.
But only on the app instance should be sufficient for the initial use for the sidecar container, and less to configure.

Makes sense, updated it with app instance. We should publish doc on lfedge wiki

@uncleDecart uncleDecart force-pushed the add-allow-to-discover branch from ef68bec to 459b601 Compare March 14, 2024 16:04
eriknordmark
eriknordmark reviewed Mar 15, 2024
View reviewed changes
proto/config/appconfig.proto Outdated
Comment on lines 210 to 211
// allow to be discovered by other app instances in the same network
// instance. Default is false
Copy link
Contributor

Choose a reason for hiding this comment

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

Comment is reversed; we want to grant the ability for an app instance to discover all the other app instances attached to its network instances.
Unless @gkodali-zededa tells me otherwise.

Copy link
Contributor

Choose a reason for hiding this comment

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

Yes. @eriknordmark is right.

eriknordmark
eriknordmark requested changes Mar 19, 2024
View reviewed changes
Copy link
Contributor

@eriknordmark eriknordmark left a comment

Choose a reason for hiding this comment

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

Please fix comment. Then ready to go.

@uncleDecart
Introduce allow_to_discover flag for AppInstanceConfig
07f656b 
This flag will allow app instance to discover other App Instances
connected to its network instances via metadata server.
Could be useful when you have certain topology and don't want
to have static IP routes, i.e. sidecar applications.

Signed-off-by: Pavel Abramov <uncle.decart@gmail.com>
@uncleDecart
Generate files for previous commit
303c42a 
Signed-off-by: Pavel Abramov <uncle.decart@gmail.com>
@uncleDecart uncleDecart force-pushed the add-allow-to-discover branch from 459b601 to 303c42a Compare March 20, 2024 08:39
@uncleDecart
Copy link
Member Author

Please fix comment. Then ready to go.

@eriknordmark done

eriknordmark
eriknordmark approved these changes Mar 20, 2024
View reviewed changes
Copy link
Contributor

@eriknordmark eriknordmark left a comment

Choose a reason for hiding this comment

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

LGTM

@eriknordmark eriknordmark merged commit bad6aa9 into lf-edge:main Mar 20, 2024
4 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@milan-zededa milan-zededa milan-zededa left review comments

@eriknordmark eriknordmark eriknordmark approved these changes

@gkodali-zededa gkodali-zededa Awaiting requested review from gkodali-zededa

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@uncleDecart @eriknordmark @gkodali-zededa @milan-zededa