Skip to content
This repository has been archived by the owner on Oct 9, 2023. It is now read-only.

Fix double http in the Spark Driver UI Link #347

Closed
wants to merge 5 commits into from
Closed

Fix double http in the Spark Driver UI Link #347

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
9957030
Fix double http in the Spark Driver UI Link
Apr 26, 2023
796e245
fix test
Apr 26, 2023
a069bba
Apply suggestions from code review
mucahitkantepe Apr 27, 2023
6453fec
Update spark.go
mucahitkantepe Apr 27, 2023
9fb2b0d
fmt
Apr 29, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 3 additions & 2 deletions go/tasks/plugins/k8s/spark/spark.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -407,8 +407,9 @@ func getEventInfoForSpark(sj *sparkOp.SparkApplication) (*pluginsCore.TaskInfo,
})
}
} else if sj.Status.AppState.State == sparkOp.RunningState && sj.Status.DriverInfo.WebUIIngressAddress != "" {
// Append https as the operator doesn't currently.
customInfoMap[sparkDriverUI] = fmt.Sprintf("https://%s", sj.Status.DriverInfo.WebUIIngressAddress)
// Older versions of spark-operator does not append http:// but newer versions do.
uri := strings.TrimPrefix(sj.Status.DriverInfo.WebUIIngressAddress, "http://")
customInfoMap[sparkDriverUI] = fmt.Sprintf("https://%s", uri)
Comment on lines +411 to +412
Copy link
Contributor

Choose a reason for hiding this comment

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

To maintain backwards compatibility does it make sense to check for the prefix rather than removing it (noop if it doesn't exist) and then appending it? Maybe something like

uri := sj.Status.DriverInfo.WebUIIngressAddress
if !strings.HasPrefix("https://") {
    uri = fmt.Sprintf("https://%s", uri)
}
customInfoMap[sparkDriverUI] = uri

Copy link
Contributor

@EngHabu EngHabu May 12, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

We shouldn't assume SSL all the time. Does spark enforce ssl?

uri := sj.Status.DriverInfo.WebUIIngressAddress
if !strings.HasPrefix("https://") && !strings.HasPrefix("http://") {
    uri = fmt.Sprintf("https://%s", uri)
}

customInfoMap[sparkDriverUI] = uri

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I agree, so we should just pass the WebUIIngressAddress value along?

Copy link
Contributor

Choose a reason for hiding this comment

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

@mucahitkantepe yes, and then if it does not contain https or http we can prepend with https as Haytham suggests.

// Custom doesn't work unless the UI has a custom plugin to parse this, hence add to Logs as well.
taskLogs = append(taskLogs, &core.TaskLog{
Uri: customInfoMap[sparkDriverUI],
Expand Down
5 changes: 2 additions & 3 deletions go/tasks/plugins/k8s/spark/spark_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,6 @@ package spark

import (
"context"
"fmt"
"os"
"strconv"
"testing"
Expand Down Expand Up @@ -34,7 +33,7 @@ import (
const sparkMainClass = "MainClass"
const sparkApplicationFile = "local:///spark_app.py"
const testImage = "image://"
const sparkUIAddress = "spark-ui.flyte"
const sparkUIAddress = "http://spark-ui.flyte"

var (
dummySparkConf = map[string]string{
Expand Down Expand Up @@ -91,7 +90,7 @@ func TestGetEventInfo(t *testing.T) {
info, err := getEventInfoForSpark(dummySparkApplication(sj.RunningState))
assert.NoError(t, err)
assert.Len(t, info.Logs, 6)
assert.Equal(t, fmt.Sprintf("https://%s", sparkUIAddress), info.CustomInfo.Fields[sparkDriverUI].GetStringValue())
assert.Equal(t, "https://spark-ui.flyte", info.CustomInfo.Fields[sparkDriverUI].GetStringValue())
generatedLinks := make([]string, 0, len(info.Logs))
for _, l := range info.Logs {
generatedLinks = append(generatedLinks, l.Uri)
Expand Down