fix: correct url for virtual ws #170
Conversation
On-behalf-of: @SAP [email protected] Signed-off-by: Artem Shcherbatiuk <[email protected]>
On-behalf-of: @SAP [email protected] Signed-off-by: Artem Shcherbatiuk <[email protected]>
| pathURL, err := url.Parse(pathURLStr) | ||
| if err != nil { | ||
| return "", errors.Join(ErrInvalidURL, err) | ||
| } | ||
|
|
||
| path := pathURL.Path | ||
|
|
||
| if !strings.HasPrefix(path, "/") { | ||
| path = "/" + path | ||
| } | ||
|
|
||
| return baseURL.ResolveReference(&url.URL{Path: path}).String(), nil |
There was a problem hiding this comment.
can't you just use url.JoinPath(baseURLStr, pathURL) here instead?
There was a problem hiding this comment.
No, it does simple concatenation, and instead of:
"https://openmfp-kcp-front-proxy.openmfp-system:8443/services/apiexport/root/kubernetes.graphql.gateway"
It end up with
"https://openmfp-kcp-front-proxy.openmfp-system:8443/clusters/root/https:/kcp.dev.local:8443/services/apiexport/root/kubernetes.graphql.gateway"
(host from second param is not removed)
There was a problem hiding this comment.
Instead of the ResolveReference call (that was new to me tbh), I would probably do
finalURL := url.URL{
Scheme: baseURL.Scheme,
Host: baseURL.Host,
Path: path,
}
return finalURL.String(), nilbecause it is more obvious to me. But ResolveReference does more or less the same and covers more edge cases, so we can leave it.
You can add an optional early return after the baseURL parsing that avoids all the stuff whenever the pathURLStr is empty like so:
if pathURLStr == "" {
return baseURL.String() + "/", nil
}On-behalf-of: @SAP [email protected] Signed-off-by: Artem Shcherbatiuk <[email protected]>
| @@ -25,6 +27,9 @@ func TestNewManager(t *testing.T) { | |||
| "successful_manager_creation": {isKCPEnabled: false, expectErr: false}, | |||
| } | |||
|
|
|||
| log, err := logger.New(logger.DefaultConfig()) | |||
There was a problem hiding this comment.
Just as an information in case you are not aware: we also have a testlogger package that can be used to actually check log messages (not necessary here)
| return nil, errors.Join(ErrFailedToGetAPIExport, err) | ||
| } | ||
|
|
||
| log.Warn().Msg(fmt.Sprintf("failed to find %s ApiExport, listener will not watch ApiBinding changes in realtime", appCfg.ApiExportName)) |
There was a problem hiding this comment.
Any reason to not use structured logging here?
| log.Warn().Msg(fmt.Sprintf("failed to find %s ApiExport, listener will not watch ApiBinding changes in realtime", appCfg.ApiExportName)) | |
| log.Warn().Str("apiexport", appCfg.ApiExportName).Msg("failed to find ApiExport, listener will not watch ApiBinding changes in realtime")) |
When we create ApiExport, it assigns to virtual workspace url address with external host, which is not resolvable within openmfp cluster.
The solution is to take the host address that is present in kubeconfig and replace original virtual workspace url host.
It works in both cases - in openmpf and outside it.
Also, I added a change for local development - app is not failing in case of kubernetes.graphql.gateway apixexport absence, it starts but prints warning that ApiBindings are not watched.
On-behalf-of: @SAP [email protected]