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

forwarder: customize the listener port #1460

Merged
merged 5 commits into from
Dec 14, 2023
Merged

forwarder: customize the listener port #1460

merged 5 commits into from
Dec 14, 2023

Conversation

Amulyam24
Copy link
Contributor

By default the port is 15150, this adds the support to customize it to a specific port. Currently changes added for AWS, Azure and IBM Cloud PowerVS providers.

Fixes: #1457

@Amulyam24 Amulyam24 force-pushed the forwarder branch 2 times, most recently from 8a06d96 to ef82eb9 Compare September 22, 2023 12:48
@Amulyam24 Amulyam24 changed the title forwarder: customize the forwarder port forwarder: customize the listener port Sep 22, 2023
yoheiueda
yoheiueda reviewed Sep 26, 2023
View reviewed changes
Copy link
Member

@yoheiueda yoheiueda left a comment

Choose a reason for hiding this comment

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

I think FORWARDER_ADDRESS can also be configuration.

How about providing FORWARDER_LISTEN or FORWARDER_LISTEN_ADDR instead of FORWARDER_PORT only?

The default value of FORWARDER_LISTEN will be 0.0.0.0:15150.

@Amulyam24
Copy link
Contributor Author

The default value of FORWARDER_LISTEN will be 0.0.0.0:15150.

Hi @yoheiueda, are you suggesting we pass FORWARDER_LISTEN(addr+port) by default to the agent-protocol-forwarder?

I think FORWARDER_ADDRESS can also be configuration.

Okay

@yoheiueda
Copy link
Member

@Amulyam24

My point was that it is preferable to make the 0.0.0.0 part configurable in addition to the 15150 part.

are you suggesting we pass FORWARDER_LISTEN(addr+port) by default to the agent-protocol-forwarder?

So, when a user want to use the default 0.0.0.0:15150, I don't think we need to explicitly specify the value in the command line argument of agent-protocol-forwarder.

@Amulyam24
Copy link
Contributor Author

@yoheiueda, changes applied, PTAL!

yoheiueda
yoheiueda approved these changes Sep 28, 2023
View reviewed changes
Copy link
Member

@yoheiueda yoheiueda left a comment

Choose a reason for hiding this comment

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

LGTM. Thank you very much!

@bpradipt
Copy link
Member

Is using a customisable listen address (not port) applicable ? Since CAA uses the node IP (public or private) and the port to connect to the APF.
@yoheiueda @Amulyam24 is there a scenario that I'm missing?

@Amulyam24
Copy link
Contributor Author

Amulyam24 commented Sep 29, 2023 •
edited

applicable ? Since CAA uses the node IP (public or private) and the port to connect to the APF.

That's right @bpradipt. I missed this point. I will let @yoheiueda answer if he has any specific scenario case for it

@yoheiueda
Copy link
Member

@bpradipt @Amulyam24 Oh, that's right. I don't come up with a specific scenario. I think we can go ahead without FORWARDER_ADDRESS.

When a pod VM image supports multiple network interfaces in the future, we will possibly provide an option to select a specific network address. But, the current deployment mechanism only supports a single network, so there are no appropriate value to be set toFORWARDER_ADDRESS.

@bpradipt
Copy link
Member

bpradipt commented Oct 6, 2023

@Amulyam24 can you please fix the commit check

bpradipt
bpradipt reviewed Oct 6, 2023
View reviewed changes
podvm/Makefile.inc Outdated Show resolved Hide resolved
@Amulyam24 Amulyam24 force-pushed the forwarder branch 6 times, most recently from dcd3e6d to 1f93c1b Compare October 10, 2023 13:02
@Amulyam24
Copy link
Contributor Author

Hi @bpradipt, did you get a chance to verify this option for AWS/Azure?

@bpradipt
Copy link
Member

bpradipt commented Dec 3, 2023

@Amulyam24 sorry this PR got missed. Can you please rebase and resolve the conflicts ?

@Amulyam24 Amulyam24 force-pushed the forwarder branch 2 times, most recently from 83979b4 to 610c78d Compare December 6, 2023 05:59
bpradipt
bpradipt approved these changes Dec 11, 2023
View reviewed changes
Copy link
Member

@bpradipt bpradipt left a comment

Choose a reason for hiding this comment

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

/lgtm
Thanks @Amulyam24

stevenhorsman
stevenhorsman approved these changes Dec 11, 2023
View reviewed changes
Copy link
Member

@stevenhorsman stevenhorsman left a comment

Choose a reason for hiding this comment

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

Looks okay to me.

@Amulyam24
forwarder: customize the listener port
615fb75 
By default the port is 15150, this PR adds the flexibility to customize it to a specific port.

Fixes: confidential-containers#1457

Signed-off-by: Amulyam24 <amulmek1@in.ibm.com>
@Amulyam24
install: add option to pass FORWARDER_PORT
83afc77 
Currently option added for AWS, Azure and IBM Cloud PowerVS.

Fixes: confidential-containers#1457

Signed-off-by: Amulyam24 <amulmek1@in.ibm.com>
@Amulyam24
aws: add forwarder_port variable to configure it to a specific port
af0c41e 
Adds option to configure port for agent-protocol-forwarder.

Fixes: confidential-containers#1457

Signed-off-by: Amulyam24 <amulmek1@in.ibm.com>
@Amulyam24
azure: add forwarder_port variable to configure it to a specific port
589511c 
Adds option to configure port for agent-protocol-forwarder.

Fixes: confidential-containers#1457

Signed-off-by: Amulyam24 <amulmek1@in.ibm.com>
@Amulyam24
powervs: add forwarder_port variable to configure it to a specific port
b523374 
Adds option to configure port for agent-protocol-forwarder.

Fixes: confidential-containers#1457

Signed-off-by: Amulyam24 <amulmek1@in.ibm.com>
@Amulyam24
Copy link
Contributor Author

Amulyam24 commented Dec 14, 2023 •
edited

Rebased the PR and resolved conflicts.

@bpradipt bpradipt merged commit f700de4 into confidential-containers:main Dec 14, 2023
16 checks passed
@Amulyam24 Amulyam24 deleted the forwarder branch December 14, 2023 12:21
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@bpradipt bpradipt bpradipt approved these changes

@yoheiueda yoheiueda yoheiueda approved these changes

@stevenhorsman stevenhorsman stevenhorsman approved these changes

@surajssd surajssd Awaiting requested review from surajssd

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

Successfully merging this pull request may close these issues.

make the agent-protocl-forwarder port customisable
4 participants
@Amulyam24 @yoheiueda @bpradipt @stevenhorsman