-
Notifications
You must be signed in to change notification settings - Fork 0
feat: support multiple hosts and different public port #10
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
base: main
Are you sure you want to change the base?
Conversation
xamcost
left a comment
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.
Hi @thomasBousselin ! Just dropping a review pointing towards some aspects I doubt about. I am not an expert of the connector so @juanrasantana and @vgonzalez7 should have a look and confirm.
mvm/connector.yaml
Outdated
| WEB_HTTP_PATH: ${CONN_WEB_HTTP_PATH:-/api} | ||
| EDC_CONTROL_ENDPOINT: http://connector-dataplane:${CONN_WEB_HTTP_CONTROL_PORT}${CONN_WEB_HTTP_CONTROL_PATH} | ||
| EDC_DSP_CALLBACK_ADDRESS: http://${SED_PUBLIC_HOSTNAME}:${CONN_CP_PUBLIC_DSP_PORT}${CONN_WEB_HTTP_PROTOCOL_PATH} | ||
| EDC_DSP_CALLBACK_ADDRESS: https://${CONN_DATA_PLANE_HOSTNAME}:${CONN_DATA_PLANE_PUBLIC_PORT}${CONN_DATA_PLANE_PUBLIC_PATH} |
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.
Two comments here:
- as you mentioned in your original post, it would be better to parametrize the protocol (
http/https), so everyone gains in flexibility. - I think this should be the following, considering that it is called DSP callback. @juanrasantana or @vgonzalez7 should have a look to confirm
| EDC_DSP_CALLBACK_ADDRESS: https://${CONN_DATA_PLANE_HOSTNAME}:${CONN_DATA_PLANE_PUBLIC_PORT}${CONN_DATA_PLANE_PUBLIC_PATH} | |
| EDC_DSP_CALLBACK_ADDRESS: https://${CONN_CONTROL_PLANE_HOSTNAME}:${CONN_CONTROL_PLANE_PORT}${CONN_CONTROL_PLANE_PATH} |
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.
Yes, the previous variable were indeed pointing to the control-plane. (easy to get mixed up with some of this names.)
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.
What Maxime said is correct, the DSP is used by the control plane.
| EDC_DATAPLANE_API_PUBLIC_BASEURL: "http://${SED_PUBLIC_HOSTNAME}:${CONN_WEB_HTTP_PUBLIC_PORT}${CONN_WEB_HTTP_PUBLIC_PATH}" | ||
| WEB_HTTP_PUBLIC_PORT: ${CONN_DATA_PLANE_PUBLIC_PORT:-8185} | ||
| WEB_HTTP_PUBLIC_PATH: ${CONN_DATA_PLANE_PUBLIC_PATH:-/api/public} | ||
| EDC_DATAPLANE_API_PUBLIC_BASEURL: "https://${CONN_DATA_PLANE_HOSTNAME}:${CONN_DATA_PLANE_PUBLIC_PORT}${CONN_DATA_PLANE_PUBLIC_PATH}" |
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.
Same comment here about the protocol
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.
About the protocol it could be one variable total or one variable by service. Which one make more sense to you?
| EXTERNAL_ENDPOINT: https://${OFFERING_MANAGER_HOSTNAME}:${OFFERING_MANAGER_PUBLIC_API_PORT:-8080} | ||
| ENABLE_CONNECTOR_SERVICE: true | ||
| EXTERNAL_CONNECTOR_ENDPOINT: http://${SED_PUBLIC_HOSTNAME}:${CONN_WEB_HTTP_PUBLIC_PORT:-8185}${CONN_WEB_HTTP_PUBLIC_PATH:-/api/public} | ||
| EXTERNAL_CONNECTOR_ENDPOINT: https://${CONN_DATA_PLANE_HOSTNAME}:${CONN_DATA_PLANE_PUBLIC_PORT:-8185}${CONN_DATA_PLANE_PUBLIC_PATH:-/api/public} |
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 as well, I find weird that the INTERNAL_CONNECTOR_ENDPOINT points towards the control plane, but the external one to the data plane. Moreover, is the offering manager even interacting with the data plane ? @juanrasantana and @vgonzalez7
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.
yes, it is strange. But this one should be the equivalent to what the previous variables were doing.
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.
What Thomas wrote is correct, this endpoint is used by the Offering Manager to fill the external endpoint when creating the offering (i.e. where the consumer will have to go to access the data), which is the data plane.
On the other hand, the internal endpoint is the one used for internal communication during the creation of the offerings, which is the control plane (this one was also correct).
Co-authored-by: Maxime Costalonga <[email protected]>
|
After installind tcpdump and netstat in the docker. I have find the issue, the port defined are used inside the container so my control-plane and data-plane application are bind to 443. |
a9ddc9c to
35cd311
Compare
SED_PUBLIC_HOSTNAMEby service because my services have different hostnamesOFFERING_MANAGER_PUBLIC_API_PORTbecause my public port will be 443 (different to the external port from the container)Edit: The configuration work on our side.
second commit contain the addition of https but i didn't bother making it configurable. (But it should be.)