converter for istio in-cluster operator config to sail operator config #616

ctartici · 2025-02-05T12:47:16Z

What type of PR is this?

What this PR does / why we need it:

Which issue(s) this PR fixes:

Fixes #

Related Issue/PR #

Additional information:

istio-testing · 2025-02-05T12:47:33Z

Hi @ctartici. Thanks for your PR.

I'm waiting for a istio-ecosystem or istio member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository.

fjglira · 2025-02-05T12:50:45Z

/ok-to-test

fjglira

LGTM adding this we will give to the users the possibility to easily convert their configuration to sail operator Istio resource. Also, this is needed to execute the upstream Istio integration test later against Sail Operator installed control plane increasing the testing coverage. Adding a hold in case someone else want to review it

docs/README.md

sridhargaddam · 2025-02-07T05:51:40Z

tools/configuration-converter.sh

+cp "$ISTIO_CONFIG_FILE" "$SAIL_CONFIG_FILE"
+
+function add_mandatory_fields(){
+    sail_api=".apiVersion=\"sailoperator.io/v1alpha1\""


Sail Operator APIs are now v1

Suggested change

sail_api=".apiVersion=\"sailoperator.io/v1alpha1\""

sail_api=".apiVersion=\"sailoperator.io/v1\""

tools/configuration-converter.sh

ctartici · 2025-02-07T13:06:35Z

/retest

ctartici · 2025-02-10T08:29:53Z

/retest

tests/e2e/controlplane/control_plane_test.go

tools/configuration-converter.sh

luksa · 2025-02-11T12:12:42Z

docs/README.md

+export ISTIO_VERSION=v1.24.0
+export ISTIO_NAMESPACE=istio-system
+./tools/configuration-converter.sh <ISTIO_OPERATOR_CONFIG_YAML_WITH_PATH>


BTW, why use environment variables if the version and namespace are required? Why not pass them as arguments to the script?

This script will be downloaded and executed from integ-suite-ocp.sh We are planning to run downstream integration tests with it and in there these env variables are already defined. Also since integ-suite-ocp.sh already gets many arguments(tests suite to run, tests to skip) It would have been so confusing.

OK, but can't integ-suite-ocp.sh simply pass the environment variables as arguments to the converter? It's weird that a shell script takes one required input as an argument, and other required inputs as an env var.

Also, won't users try to use this script as well? To convert examples from the upstream docs, for example.

Yes I can modify integ-suite-ocp.sh to pass arguments to the converter. You are right it will be cleaner for the users using directly the converter. Will update the readme also according to this.

pkg/converter/converter_test.go

luksa · 2025-02-11T12:25:17Z

tools/configuration-converter.sh

+    | del(.spec.tag) 
+    | del(.spec.values.gateways)
+    ' "$SAIL_CONFIG_FILE" > tmp.yaml
+    mv tmp.yaml $SAIL_CONFIG_FILE


I think you can use yq eval -i to update the original file directly; no need to write to a temporary file.

yq -i is not able to be executed for the files in root directories like /tmp if installed with snap. Since /tmp is default directory for integration tests it is necessary to create a temporary file.

implemented a workaround since yq -i is not working on root directories

Done now using yq -i

luksa · 2025-02-11T12:26:23Z

tools/configuration-converter.sh

+        cat $SAIL_CONFIG_FILE | yq '(.spec.values.[].env.[] | select(. == true)) |= "true"'  > tmp.yaml
+        mv tmp.yaml $SAIL_CONFIG_FILE
+        cat $SAIL_CONFIG_FILE | yq '(.spec.values.[].env.[] | select(. == false)) |= "false"' | sed '/env: \[\]/d'  > tmp.yaml
+        mv tmp.yaml $SAIL_CONFIG_FILE


This, too, can probably be done with a single yq command.

luksa · 2025-02-11T12:27:35Z

tools/configuration-converter.sh

+            cat $SAIL_CONFIG_FILE | yq 'del(.spec.components.[] | keys[] | select(. != "enabled"))' | yq '.spec.values *= .spec.components | del (.spec.components)' > tmp.yaml 
+            mv tmp.yaml $SAIL_CONFIG_FILE


Also could be a single yq command (+ no need for a temporary file).

implemented a workaround since yq -i is not working on root directories

Done now using yq -i

luksa · 2025-02-11T12:29:48Z

tools/configuration-converter.sh

+            cat $SAIL_CONFIG_FILE | yq 'del (.spec.components)' > tmp.yaml  
+            mv tmp.yaml $SAIL_CONFIG_FILE                          


Suggested change

cat $SAIL_CONFIG_FILE | yq 'del (.spec.components)' > tmp.yaml

mv tmp.yaml $SAIL_CONFIG_FILE

yq -i 'del (.spec.components)' "$SAIL_CONFIG_FILE"

Signed-off-by: ctartici <[email protected]> Signed-off-by: ctartici <[email protected]>

MaxBab · 2025-02-12T09:11:56Z

tools/configuration-converter.sh

+cp "$WORKDIR"/"$ISTIO_CONFIG_FILE" "$SAIL_CONFIG_FILE" || exit 1
+
+function add_mandatory_fields(){
+    yq -i eval ".apiVersion = \"sailoperator.io/v1\" 


I would add a check for the yq binary installed.
Otherwise, the command will fail without specifying a reason.

It will fail with yq command not found. You think I need to catch command not found and inform user with better error message?

You could add the following function.
It will check for the yq command available.
And if missing, will download it and use.

function verify_yq() { if ! command -v yq &> /dev/null; then if [[ "${OS}" == "darwin" ]]; then ERROR "Perform 'brew install yq' and try again." elif [[ "${OS}" == "linux" ]]; then WARNING "Missing yq command. Installing..." mkdir -p "$HOME"/.local/bin wget -qO- https://github.com/mikefarah/yq/releases/download/v4.16.2/yq_linux_amd64 \ -O "$HOME"/.local/bin/yq && chmod +x "$HOME"/.local/bin/yq # Add local BIN dir to PATH [[ ":$PATH:" = *":$HOME/.local/bin:"* ]] || export PATH="$HOME/.local/bin:$PATH" fi INFO "The yq command installed" fi INFO "The yq command is found" }

Or just check and explicitly tell the user what is missing.

Done. added an error message for yq installation

MaxBab · 2025-02-12T09:12:17Z

tools/configuration-converter.sh

+# Note that if there is an entry except spec.components.<component>.enabled: true/false converter will delete them and warn user 
+function validate_spec_components(){
+    yq -i 'del(.spec.components.[] | keys[] | select(. != "enabled")) | .spec.values *= .spec.components | del (.spec.components)' $SAIL_CONFIG_FILE
+    echo "Converter can only be used values with spec.components.<component>.enabled: true/false. Please see https://github.com/istio-ecosystem/sail-operator/tree/main/docs#components-field"


Converted can only use values with....

Done. changed the sentence

luksa · 2025-02-12T09:16:59Z

docs/README.md

+ISTIO_OPERATOR_CONFIG_YAML_WITH_PATH: Path to the Istio operator configuration YAML file.
+ISTIO_NAMESPACE: The Kubernetes namespace for the Istio deployment.
+ISTIO_VERSION: The version of Istio to be used.


Since the ISTIO_OPERATOR_CONFIG_YAML_WITH_PATH, ISTIO_NAMESPACE, and ISTIO_VERSION aren't shown in the example, you can probably just remove them here and just explain the arguments (in the correct order):

Suggested change

ISTIO_OPERATOR_CONFIG_YAML_WITH_PATH: Path to the Istio operator configuration YAML file.

ISTIO_NAMESPACE: The Kubernetes namespace for the Istio deployment.

ISTIO_VERSION: The version of Istio to be used.

- The path to the Istio operator configuration YAML file.

- The Kubernetes namespace for the Istio deployment.

- The version of Istio to be used.

Done modified readme

luksa · 2025-02-12T09:17:38Z

docs/README.md

@@ -303,6 +303,25 @@ Note the quotes around the value of `spec.values.pilot.env.PILOT_ENABLE_STATUS`.

 Sail Operator's Istio resource does not have a `spec.components` field. Instead, you can enable and disable components directly by setting `spec.values.<component>.enabled: true/false`. Other functionality exposed through `spec.components` like the k8s overlays is not currently available.

+### Converter Script
+This script is used to convert an Istio in-cluster operator configuration to a Sail Operator configuration. Upon execution, a new YAML file will be created in the same directory which ISTIO-OPERATOR-CONFIG-YAML is present. The new file will be named: sail-operator-config.yaml


Suggested change

This script is used to convert an Istio in-cluster operator configuration to a Sail Operator configuration. Upon execution, a new YAML file will be created in the same directory which ISTIO-OPERATOR-CONFIG-YAML is present. The new file will be named: sail-operator-config.yaml

This script is used to convert an Istio in-cluster operator configuration to a Sail Operator configuration. Upon execution, a new YAML file will be created in the same directory as the source YAML file. The new file will be named: sail-operator-config.yaml

Also, wouldn't it be better to generate the new file name from the original file name, and just adding sail.yaml as the suffix? If someone has multiple files to convert, it would be a hassle to have to rename the output file every time. And if they don't rename it, the file gets overwritten; what if the person doesn't notice?

Done. added an optional argument for output file. If not present it will add -sail.yaml suffix to input file

luksa · 2025-02-12T09:20:27Z

pkg/converter/converter_test.go

+var (
+	controlPlaneNamespace = "istio-system"
+	// Test converter with latest istio version
+	istioVersion = supportedversion.List[len(supportedversion.List)-1].Name


I believe there's a supportedversion.Default or something like that? No need to get the version from the list.

luksa · 2025-02-12T09:21:45Z

pkg/converter/converter_test.go

+func TestSimpleYamlConversion(t *testing.T) {
+	g := NewWithT(t)
+
+	istioYamlText := `apiVersion: install.istio.io/v1alpha1


Can rename this to istioYaml. Same for sailYamlText. Shorter names are usually better, especially if the variable's context is very small. It makes reading the code faster.

luksa · 2025-02-12T09:24:09Z

pkg/converter/converter_test.go

+	err = os.Remove(filepath.Join(project.RootDir, "tools", "istioConfig.yaml"))
+	g.Expect(err).To(Succeed(), "Unable to delete istioConfig.yaml")
+	err = os.Remove(filepath.Join(project.RootDir, "tools", "sail-operator-config.yaml"))
+	g.Expect(err).To(Succeed(), "Unable to delete sail-operator-config.yaml")


Does this get executed if the assertions above fail?

Added a cleanup function for removing created files.

luksa · 2025-02-12T09:33:54Z

pkg/converter/converter_test.go

+
+	g.Expect(executeConfigConverter(istioYamlFileWithPath, controlPlaneNamespace, istioVersion)).To(Succeed(),
+		"error in execution of ./configuration-converter.sh")
+	isConversionValidated, err := validateYamlContent(sailYamlText)


One thing I don't like about the validateYamlContent function is that it isn't clear what it's comparing to the sailYamlText. It would be clearer if you passed in both YAMLs. Then, when someone sees validateYamlContent(actual, expected), it would immediately clear what the function is doing. Right now, seeing this function causes confusion.

luksa · 2025-02-12T09:38:06Z

tests/e2e/util/shell/shell.go

@@ -53,3 +54,34 @@ func ExecuteShell(command string, input string) (string, error) {

 	return stdout.String(), nil
 }
+
+// Execute bash script with optional arguments
+func ExecuteBashScript(scriptPath string, args ...string) (string, error) {


Why do we need this function? Doesn't ExecuteShell have everything we need already?

Done. removed function

luksa · 2025-02-12T09:40:16Z

tools/configuration-converter.sh

+ISTIO_NAMESPACE="$3"
+SAIL_CONFIG_FILE="sail-operator-config.yaml"
+
+cp "$WORKDIR"/"$ISTIO_CONFIG_FILE" "$SAIL_CONFIG_FILE" || exit 1


I'd move this after the functions. Currently, we copy the file here, but then modify it in a totally different place in the script. It would be best to keep all the steps together, possibly even in a function that is then called at the end of the script.

luksa · 2025-02-12T09:41:21Z

tools/configuration-converter.sh

+    validate_spec_components
+fi 
+
+chmod +x $SAIL_CONFIG_FILE


Is this really necessary?

It is to be able to use created yaml in oc/kubectl command afterwards. oc apply -f sail-config.yaml

Done. removed command since I couldn't reproduce the error.

luksa · 2025-02-12T09:41:53Z

tools/configuration-converter.sh

+
+chmod +x $SAIL_CONFIG_FILE
+
+if ! mv "$SAIL_CONFIG_FILE" "$WORKDIR" 2>&1 | grep -q "are identical"; then


Is it a good idea to assume what the output text can be?

Removed after adding output as argument

Signed-off-by: ctartici <[email protected]>

luksa · 2025-02-13T09:49:51Z

docs/README.md

 ```bash
-./tools/configuration-converter.sh tools/istio_config.yaml istio-system v1.24.3
+./tools/configuration-converter.sh -i /home/user/input/istio_config.yaml -o /home/user/output/output.yaml -n custom-namespace -v v1.24.3


Does it make sense to have to specify -i instead of just the file name? You always need to specify the input file, so why make the user type -i? Similar for -o, even though that one is optional. The input and output seem like the most important arguments, so we don't need to specify them via flags. The namespace and version arguments are not at the same level (much more optional than the output file), so it makes sense to set them via flags.

Done. I have removed -i and -o arguments and updated readme

ctartici · 2025-02-13T11:58:48Z

/retest

Signed-off-by: ctartici <[email protected]>

ctartici requested review from jwendell, dgn, howardjohn, ericvn and a team as code owners February 5, 2025 12:47

istio-testing added size/M needs-ok-to-test labels Feb 5, 2025

istio-testing added ok-to-test and removed needs-ok-to-test labels Feb 5, 2025

ctartici force-pushed the main branch 4 times, most recently from 35345e0 to 50d9b8b Compare February 5, 2025 13:33

fjglira added the do-not-merge/hold label Feb 5, 2025

fjglira approved these changes Feb 5, 2025

View reviewed changes

ctartici force-pushed the main branch from 50d9b8b to 5fedfec Compare February 5, 2025 14:05

fjglira approved these changes Feb 6, 2025

View reviewed changes

fjglira added do-not-merge/hold and removed do-not-merge/hold labels Feb 6, 2025

sridhargaddam reviewed Feb 7, 2025

View reviewed changes

tools/configuration-converter.sh Show resolved Hide resolved

ctartici force-pushed the main branch from 5fedfec to 9e839d7 Compare February 7, 2025 11:47

istio-testing added size/XL and removed size/M labels Feb 7, 2025

ctartici force-pushed the main branch 3 times, most recently from 6579d00 to 3f32f60 Compare February 7, 2025 13:02

luksa reviewed Feb 10, 2025

View reviewed changes

tests/e2e/controlplane/control_plane_test.go Outdated Show resolved Hide resolved

luksa reviewed Feb 10, 2025

View reviewed changes

tests/e2e/controlplane/control_plane_test.go Outdated Show resolved Hide resolved

luksa reviewed Feb 10, 2025

View reviewed changes

tools/configuration-converter.sh Outdated Show resolved Hide resolved

ctartici force-pushed the main branch from 427c845 to 251ef4c Compare February 11, 2025 10:51

luksa reviewed Feb 11, 2025

View reviewed changes

ctartici force-pushed the main branch 7 times, most recently from e8b4436 to e4f4b4a Compare February 11, 2025 23:17

converter for istio in-cluster operator config to sail operator config

c9c8706

Signed-off-by: ctartici <[email protected]> Signed-off-by: ctartici <[email protected]>

ctartici force-pushed the main branch from e4f4b4a to c9c8706 Compare February 12, 2025 09:05

MaxBab reviewed Feb 12, 2025

View reviewed changes

luksa reviewed Feb 12, 2025

View reviewed changes

Added output argument and a new test for optional arguments

610ea23

Signed-off-by: ctartici <[email protected]>

ctartici force-pushed the main branch from 5469741 to 610ea23 Compare February 13, 2025 08:46

luksa reviewed Feb 13, 2025

View reviewed changes

ctartici force-pushed the main branch from 01a2e27 to 467cab5 Compare February 13, 2025 11:41

ctartici force-pushed the main branch 6 times, most recently from 1daa88c to f0c5825 Compare February 13, 2025 12:34

removed -i and -o from arguments

af099c7

Signed-off-by: ctartici <[email protected]>

ctartici force-pushed the main branch from f0c5825 to af099c7 Compare February 13, 2025 12:37

	sail_api=".apiVersion=\"sailoperator.io/v1alpha1\""
	sail_api=".apiVersion=\"sailoperator.io/v1\""

		cat $SAIL_CONFIG_FILE \| yq 'del(.spec.components.[] \| keys[] \| select(. != "enabled"))' \| yq '.spec.values *= .spec.components \| del (.spec.components)' > tmp.yaml
		mv tmp.yaml $SAIL_CONFIG_FILE

		cat $SAIL_CONFIG_FILE \| yq 'del (.spec.components)' > tmp.yaml
		mv tmp.yaml $SAIL_CONFIG_FILE

	cat $SAIL_CONFIG_FILE \| yq 'del (.spec.components)' > tmp.yaml
	mv tmp.yaml $SAIL_CONFIG_FILE
	yq -i 'del (.spec.components)' "$SAIL_CONFIG_FILE"

	This script is used to convert an Istio in-cluster operator configuration to a Sail Operator configuration. Upon execution, a new YAML file will be created in the same directory which ISTIO-OPERATOR-CONFIG-YAML is present. The new file will be named: sail-operator-config.yaml
	This script is used to convert an Istio in-cluster operator configuration to a Sail Operator configuration. Upon execution, a new YAML file will be created in the same directory as the source YAML file. The new file will be named: sail-operator-config.yaml


		chmod +x $SAIL_CONFIG_FILE

		if ! mv "$SAIL_CONFIG_FILE" "$WORKDIR" 2>&1 \| grep -q "are identical"; then

converter for istio in-cluster operator config to sail operator config #616

Are you sure you want to change the base?

converter for istio in-cluster operator config to sail operator config #616

Conversation

ctartici commented Feb 5, 2025 • edited Loading

What type of PR is this?

What this PR does / why we need it:

Which issue(s) this PR fixes:

Additional information:

istio-testing commented Feb 5, 2025

fjglira commented Feb 5, 2025

fjglira left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ctartici commented Feb 7, 2025

ctartici commented Feb 10, 2025

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ctartici Feb 11, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ctartici commented Feb 13, 2025

ctartici commented Feb 5, 2025 •

edited

Loading

ctartici Feb 11, 2025 •

edited

Loading