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.

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

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

Open
wants to merge 3 commits into
base: main
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from 1 commit
Commits
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
21 changes: 20 additions & 1 deletion docs/README.md
Original file line number Diff line number Diff line change
Expand Up @@ -293,7 +293,7 @@ spec:
pilot:
env:
PILOT_ENABLE_STATUS: "true"
version: v1.23.0
version: v1.24.0
namespace: istio-system
```

Expand All @@ -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
Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Contributor

Choose a reason for hiding this comment

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

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?

Copy link
Author

Choose a reason for hiding this comment

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

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


#### Usage
To run the configuration-converter.sh script, you need to provide three arguments:

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.
Copy link
Contributor

Choose a reason for hiding this comment

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

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.

Copy link
Author

Choose a reason for hiding this comment

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

Done modified readme


##### Example
```bash
./tools/configuration-converter.sh tools/istio_config.yaml istio-system v1.24.3
```

> [!WARNING]
> This script is still under development.
> Please verify the resulting configuration carefully after conversion to ensure it meets your expectations and requirements.

### CNI

The CNI plugin's lifecycle is managed separately from the control plane. You will have to create a [IstioCNI resource](#istiocni-resource) to use CNI.
Expand Down
176 changes: 176 additions & 0 deletions pkg/converter/converter_test.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,176 @@
// Copyright Istio Authors
//
// Licensed under the Apache License, Version 2.0 (the "License");
// you may not use this file except in compliance with the License.
// You may obtain a copy of the License at
//
// http://www.apache.org/licenses/LICENSE-2.0
//
// Unless required by applicable law or agreed to in writing, software
// distributed under the License is distributed on an "AS IS" BASIS,
// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
// See the License for the specific language governing permissions and
// limitations under the License.

package converter

import (
"fmt"
"os"
"path/filepath"
"testing"

"github.com/istio-ecosystem/sail-operator/pkg/test/project"
"github.com/istio-ecosystem/sail-operator/pkg/test/util/supportedversion"
"github.com/istio-ecosystem/sail-operator/tests/e2e/util/shell"
. "github.com/onsi/gomega"
)

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

Choose a reason for hiding this comment

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

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

Copy link
Author

Choose a reason for hiding this comment

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

Done

)

func TestSimpleYamlConversion(t *testing.T) {
g := NewWithT(t)

istioYamlText := `apiVersion: install.istio.io/v1alpha1
Copy link
Contributor

Choose a reason for hiding this comment

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

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.

Copy link
Author

Choose a reason for hiding this comment

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

Done

kind: IstioOperator
metadata:
name: default
spec:`

sailYamlText := `apiVersion: sailoperator.io/v1
kind: Istio
metadata:
name: default
spec:
version: %s
namespace: %s`
sailYamlText = fmt.Sprintf(sailYamlText, istioVersion, controlPlaneNamespace)

istioYamlFileWithPath, err := saveYamlToFile(istioYamlText)
g.Expect(err).To(Succeed(), "failed to write YAML file")

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

Choose a reason for hiding this comment

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

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.

Copy link
Author

Choose a reason for hiding this comment

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

Done

g.Expect(err).To(Succeed(), "Can not open file to compare")
g.Expect(isConversionValidated).To(BeTrue(), "Converted content is not as expected")

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")
Copy link
Contributor

Choose a reason for hiding this comment

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

Does this get executed if the assertions above fail?

Copy link
Author

Choose a reason for hiding this comment

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

Added a cleanup function for removing created files.

}

func TestComplexYamlConversion(t *testing.T) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Since this and the previous test both test the same function, it would be better if both tests were implemented in the same test function, with table-based sub-tests. Something like:

func TestConversion(t *testing.T) {
    var flagtests = []struct {
        name  string
        input string
        expectedOutput string
    }{
        {"simple", "simple input yaml", "simple output yaml"},
        {"complex", "complex input yaml", "complex output yaml"},
    }    

    for _, tt := range testcases {
        t.Run(tt.name, func(t *testing.T) {
            ...
        })
    }
}

This would reduce code duplication and allow the next person to easily add another sub-test.

g := NewWithT(t)

istioYamlText := `apiVersion: install.istio.io/v1alpha1
kind: IstioOperator
metadata:
name: default
spec:
components:
base:
enabled: true
pilot:
enabled: false
values:
global:
externalIstiod: true
operatorManageWebhooks: true
configValidation: false
base:
enableCRDTemplates: true
pilot:
env:
PILOT_ENABLE_STATUS: true`

sailYamlText := `apiVersion: sailoperator.io/v1
kind: Istio
metadata:
name: default
spec:
values:
global:
externalIstiod: true
operatorManageWebhooks: true
configValidation: false
base:
enableCRDTemplates: true
enabled: true
pilot:
env:
PILOT_ENABLE_STATUS: "true"
enabled: false
version: %s
namespace: %s`
sailYamlText = fmt.Sprintf(sailYamlText, istioVersion, controlPlaneNamespace)

istioYamlFileWithPath, err := saveYamlToFile(istioYamlText)
g.Expect(err).To(Succeed(), "failed to write YAML file")

g.Expect(executeConfigConverter(istioYamlFileWithPath, controlPlaneNamespace, istioVersion)).To(Succeed(),
"error in execution of ./configuration-converter.sh")
isConversionValidated, err := validateYamlContent(sailYamlText)
g.Expect(err).To(Succeed(), "Can not open file to compare")
g.Expect(isConversionValidated).To(BeTrue(), "Converted content is not as expected")

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")
}

// saveYamlToFile writes the given YAML content to IstioConfig.yaml
func saveYamlToFile(yamlText string) (string, error) {
// Create file in the same directory with the converter
istioYamlFile := "istioConfig.yaml"
istioYamlFileWithPath := filepath.Join(project.RootDir, "tools", istioYamlFile)

// Write to file
if err := os.WriteFile(istioYamlFileWithPath, []byte(yamlText), 0o644); err != nil {
return "", fmt.Errorf("failed to write YAML file: %w", err)
}

return istioYamlFileWithPath, nil
}

func executeConfigConverter(istioYamlFilePath, istioVersion, controlPlaneNamespace string) error {
// Define file path
configConverterPath := filepath.Join(project.RootDir, "tools", "configuration-converter.sh")

_, err := shell.ExecuteBashScript(configConverterPath, istioYamlFilePath, controlPlaneNamespace, istioVersion)
if err != nil {
return fmt.Errorf("error in execution of %s %s %s %s: %w", configConverterPath, istioYamlFilePath, controlPlaneNamespace, istioVersion, err)
}
return nil
}

// compareYamlContent checks if the provided YAML string matches the content of converted config
func validateYamlContent(yamlString string) (bool, error) {
sailYamlWithPath := filepath.Join(project.RootDir, "tools", "sail-operator-config.yaml")

// Write the input YAML string to a temporary file
tmpFile := filepath.Join(project.RootDir, "tools", "temp.yaml")
err := os.WriteFile(tmpFile, []byte(yamlString), 0o644)
if err != nil {
return false, fmt.Errorf("failed to write temporary YAML file: %w", err)
}
defer os.Remove(tmpFile)

// The command will check if the files are equal, ignoring order
cmd := fmt.Sprintf("diff <(yq -P 'sort_keys(..)' -o=props %s) <(yq -P 'sort_keys(..)' -o=props %s)", sailYamlWithPath, tmpFile)
output, err := shell.ExecuteCommand(cmd)
if err != nil {
return false, fmt.Errorf("error executing yq comparison: %w", err)
}
if output != "" {
return false, nil
}
// If no output from yq, the files are considered equal
return true, nil
}
34 changes: 33 additions & 1 deletion tests/e2e/util/shell/shell.go
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,7 @@ import (
"fmt"
"os"
"os/exec"
"path/filepath"
"strings"
)

Expand All @@ -34,7 +35,7 @@ func ExecuteCommand(command string) (string, error) {

// ExecuteShell executes a command given the input string and returns the output and err if any
func ExecuteShell(command string, input string) (string, error) {
cmd := exec.Command("sh", "-c", command)
cmd := exec.Command("bash", "-c", command)
if input != "" {
cmd.Stdin = strings.NewReader(input)
}
Expand All @@ -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) {
Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Author

Choose a reason for hiding this comment

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

Done. removed function

absScriptPath, err := filepath.Abs(scriptPath)
if err != nil {
return "", fmt.Errorf("error getting absolute path: %w", err)
}

if _, err := os.Stat(absScriptPath); os.IsNotExist(err) {
return "", fmt.Errorf("script file does not exist: %s", absScriptPath)
}

scriptDir := filepath.Dir(absScriptPath)
cmdArgs := append([]string{absScriptPath}, args...)
cmd := exec.Command("bash", cmdArgs...)

var stdout, stderr bytes.Buffer
cmd.Stdout = &stdout
cmd.Stderr = &stderr

// set dir to dir of the script before execution
cmd.Dir = scriptDir

// Run the script
err = cmd.Run()
if err != nil {
return "", fmt.Errorf("error executing script: %s, stderr: %s", err, stderr.String())
}

return stdout.String(), nil
}
77 changes: 77 additions & 0 deletions tools/configuration-converter.sh
Original file line number Diff line number Diff line change
@@ -0,0 +1,77 @@
#!/bin/bash

# Copyright Istio Authors
#
# Licensed under the Apache License, Version 2.0 (the "License");
# you may not use this file except in compliance with the License.
# You may obtain a copy of the License at
#
# http://www.apache.org/licenses/LICENSE-2.0
#
# Unless required by applicable law or agreed to in writing, software
# distributed under the License is distributed on an "AS IS" BASIS,
# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
# See the License for the specific language governing permissions and
# limitations under the License.

# This script is used to convert istio configuration to sail operator configuration.
# In the end of the execution new yaml file will be created with "sail-ISTIO_CONFIG_YAML" name.
# Usage: ./configuration-converter.sh ISTIO_CONFIG_YAML_WITH_PATH, example: ./configuration-converter.sh sample_config.yaml"

set -e

if [ -z "$1" ] || [ -z "$2" ] || [ -z "$3" ]; then
echo "Usage: $0 <ISTIO_OPERATOR_CONFIG_YAML_WITH_PATH> <ISTIO_NAMESPACE> <ISTIO_VERSION>"
exit 1
fi

ctartici marked this conversation as resolved.
Show resolved Hide resolved
WORKDIR=$(dirname "$1")
ISTIO_CONFIG_FILE=$(basename "$1")
ISTIO_VERSION="$2"
ISTIO_NAMESPACE="$3"
SAIL_CONFIG_FILE="sail-operator-config.yaml"

cp "$WORKDIR"/"$ISTIO_CONFIG_FILE" "$SAIL_CONFIG_FILE" || exit 1
Copy link
Contributor

Choose a reason for hiding this comment

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

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.

Copy link
Author

Choose a reason for hiding this comment

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

Done


function add_mandatory_fields(){
yq -i eval ".apiVersion = \"sailoperator.io/v1\"
Copy link
Collaborator

Choose a reason for hiding this comment

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

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

Copy link
Author

Choose a reason for hiding this comment

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

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

Copy link
Collaborator

Choose a reason for hiding this comment

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

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.

Copy link
Author

Choose a reason for hiding this comment

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

Done. added an error message for yq installation

| .kind = \"Istio\"
| (select(.spec.meshConfig) | .spec.values.meshConfig) = .spec.meshConfig
| (select(.spec.values.istio_cni) | .spec.values.pilot.cni) = .spec.values.istio_cni
| .metadata.name = \"default\"
| .spec.version = \"$ISTIO_VERSION\"
| .spec.namespace = \"$ISTIO_NAMESPACE\"
| del(.spec.values.istio_cni)
| del(.spec.meshConfig)
| del(.spec.hub)
| del(.spec.tag)
| del(.spec.values.gateways)" "$SAIL_CONFIG_FILE"
}

#Convert boolean values to string if they are under *.env
function boolean_2_string(){
yq -i -e '(.spec.values.[].env.[] | select(. == true)) |= "true"
| (.spec.values.[].env.[] | select(. == false)) |= "false"' "$SAIL_CONFIG_FILE"
yq -i -e 'del(.. | select(tag == "!!seq" and length == 0))' "$SAIL_CONFIG_FILE"
}

# 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"
Copy link
Collaborator

Choose a reason for hiding this comment

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

Converted can only use values with....

Copy link
Author

Choose a reason for hiding this comment

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

Done. changed the sentence


}

add_mandatory_fields
boolean_2_string
if [[ $(yq eval '.spec.components' "$SAIL_CONFIG_FILE") != "null" ]]; then
validate_spec_components
fi

chmod +x $SAIL_CONFIG_FILE
Copy link
Contributor

Choose a reason for hiding this comment

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

Is this really necessary?

Copy link
Author

Choose a reason for hiding this comment

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

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

Copy link
Author

Choose a reason for hiding this comment

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

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


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

Choose a reason for hiding this comment

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

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

Copy link
Author

Choose a reason for hiding this comment

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

Removed after adding output as argument

true
fi

echo "Sail configuration file created with name: ${SAIL_CONFIG_FILE}"