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.

Sign up for GitHub

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

Update error msgs for OSImageURL validation: #7769

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open

Update error msgs for OSImageURL validation: #7769

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
4 changes: 2 additions & 2 deletions pkg/providers/tinkerbell/assert_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -337,7 +337,7 @@ func TestK8sVersionInCPMachineConfigOSImageURL_Error(t *testing.T) {
clusterSpec.MachineConfigs[builder.ControlPlaneMachineName].Spec.OSImageURL = "test-url"
clusterSpec.MachineConfigs[builder.ExternalEtcdMachineName].Spec.OSImageURL = "test-url-122"
clusterSpec.MachineConfigs[builder.WorkerNodeGroupMachineName].Spec.OSImageURL = "test-url-122"
g.Expect(tinkerbell.AssertOSImageURL(clusterSpec)).To(gomega.MatchError(gomega.ContainSubstring("missing kube version from control plane machine config OSImageURL:")))
g.Expect(tinkerbell.AssertOSImageURL(clusterSpec)).To(gomega.MatchError(gomega.ContainSubstring("missing kube version from control plane machine config OSImageURL")))
}

func TestK8sVersionInWorkerMachineConfigOSImageURL_Error(t *testing.T) {
Expand All @@ -350,7 +350,7 @@ func TestK8sVersionInWorkerMachineConfigOSImageURL_Error(t *testing.T) {
clusterSpec.MachineConfigs[builder.ControlPlaneMachineName].Spec.OSImageURL = "test-url-122"
clusterSpec.MachineConfigs[builder.ExternalEtcdMachineName].Spec.OSImageURL = "test-url-122"
clusterSpec.MachineConfigs[builder.WorkerNodeGroupMachineName].Spec.OSImageURL = "test-url"
g.Expect(tinkerbell.AssertOSImageURL(clusterSpec)).To(gomega.MatchError(gomega.ContainSubstring("missing kube version from worker node group machine config OSImageURL:")))
g.Expect(tinkerbell.AssertOSImageURL(clusterSpec)).To(gomega.MatchError(gomega.ContainSubstring("missing kube version from worker node group machine config OSImageURL")))
}

func TestK8sVersionForBRAutoImport_Succeed(t *testing.T) {
Expand Down
80 changes: 74 additions & 6 deletions pkg/providers/tinkerbell/validate.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -132,8 +132,8 @@ func validateK8sVersionInOSImageURLs(spec *ClusterSpec) error {
kvs := spec.Cluster.KubernetesVersions()
for _, v := range kvs {
if !containsK8sVersion(spec.DatacenterConfig.Spec.OSImageURL, string(v)) {
return fmt.Errorf("missing kube version from OSImageURL: url=%v, version=%v",
spec.DatacenterConfig.Spec.OSImageURL, v)
return fmt.Errorf("missing kube version from OSImageURL, the image url must include %v: url=%v, version=%v",
permutations(string(v), []string{"-", "_", ""}), spec.DatacenterConfig.Spec.OSImageURL, v)
}
}
} else {
Expand All @@ -150,8 +150,8 @@ func validateK8sVersionInOSImageURLs(spec *ClusterSpec) error {
}

if !containsK8sVersion(spec.ControlPlaneMachineConfig().Spec.OSImageURL, string(spec.Cluster.Spec.KubernetesVersion)) {
return fmt.Errorf("missing kube version from control plane machine config OSImageURL: url=%v, version=%v",
spec.ControlPlaneMachineConfig().Spec.OSImageURL, spec.Cluster.Spec.KubernetesVersion)
return fmt.Errorf("missing kube version from control plane machine config OSImageURL, the image url must include %v: url=%v, version=%v",
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
return fmt.Errorf("missing kube version from control plane machine config OSImageURL, the image url must include %v: url=%v, version=%v",
return fmt.Errorf("missing kube version from control plane machine config OSImageURL, the image url must include %s: url=%v, version=%v",

permutations(string(spec.Cluster.Spec.KubernetesVersion), []string{"-", "_", ""}), spec.ControlPlaneMachineConfig().Spec.OSImageURL, spec.Cluster.Spec.KubernetesVersion)
}

for _, wng := range spec.WorkerNodeGroupConfigurations() {
Expand All @@ -162,14 +162,82 @@ func validateK8sVersionInOSImageURLs(spec *ClusterSpec) error {
}

if !containsK8sVersion(url, string(version)) {
return fmt.Errorf("missing kube version from worker node group machine config OSImageURL: url=%v, version=%v",
url, version)
return fmt.Errorf("missing kube version from worker node group machine config OSImageURL, the image url must include %v: url=%v, version=%v",
permutations(string(version), []string{"-", "_", ""}), url, version)
}
}
}
return nil
}

// permutations takes a version in dot format and
// returns a human readable string with the permutations of the version
// using the passed in separators.
//
// For example, when v = 1.29 and separators = []string{"-", "_", ""}
// the result will be "1.29, 1-29, 1_29, or 129".
func permutations(v string, separators []string) string {
result := []string{v}
split := strings.Split(v, ".")
if len(split) == 1 {
return v
}

seps := remove(".", deduplicate(separators))
for idx, sep := range seps {
Copy link
Member

Choose a reason for hiding this comment

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

can we simplify this logic to just parsing in the version and concatenating the seps to list out all accepted versions in a single for loop

Suggested change
for idx, sep := range seps {
version := strings.Split(v, ".")
result := []string{v}
major := version[0]
minor := version[1]
for idx, sep := range seps {
elem := fmt.Sprintf("%s%s%s",major,sep,minor)
if idx == len(seps)-1 {
result = append(result, "or")
} else if idx != len(seps)-2 {
elem = elem + ","
}
result = append(result, elem)
}
return strings.Join(result, " ")
}

Copy link
Member Author

Choose a reason for hiding this comment

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

this changes the function to only support a single . as a separator. In my opinion, this more generic implementation is basic, flexible, and simple enough to be future proof. I'm not convinced of the value of the trade off of a few less lines of code.

var elem string
for idx, s := range split {
if idx != len(split)-1 {
elem = elem + s + sep
} else {
elem = elem + s
}
}

if idx == len(seps)-1 {
result = append(result, "or")
} else if idx != len(seps)-2 {
elem = elem + ","
}
result = append(result, elem)
}

if len(seps) > 1 {
result[0] = result[0] + ","
}

return strings.Join(result, " ")
}

// deduplicate returns a new slice with duplicates values removed.
func deduplicate(s []string) []string {
Copy link
Member

Choose a reason for hiding this comment

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

wondering if this is needed, since we have the control over the input and this is not coming from user?

Copy link
Member Author

Choose a reason for hiding this comment

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

the control we have over the input is "business" level logic, this function is the "application" level logic. Coupling those together creates for us future issues when business logic changes. I don't recommend we couple them.

if len(s) <= 1 {
return s
}
result := []string{}
seen := make(map[string]struct{})
for _, val := range s {
val := strings.ToLower(val)
Copy link
Member

Choose a reason for hiding this comment

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

since these are all symbols we might not need this too!

Copy link
Member Author

Choose a reason for hiding this comment

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

this function doesnt limit the type of strings in the slice. the values could be characters. coupling the inputs to our business logic creates a rigid implementation.

if _, ok := seen[val]; !ok {
result = append(result, val)
seen[val] = struct{}{}
}
}
return result
}

// removes all values equal to val from in.
func remove(val string, in []string) []string {
result := []string{}
for _, i := range in {
if i != val {
result = append(result, i)
}
}

return result
}

func defaultBottlerocketOSImageURLs(spec *ClusterSpec) {
if spec.ControlPlaneMachineConfig().Spec.OSImageURL == "" {
spec.ControlPlaneMachineConfig().Spec.OSImageURL = spec.RootVersionsBundle().EksD.Raw.Bottlerocket.URI
Expand Down
31 changes: 31 additions & 0 deletions pkg/providers/tinkerbell/validate_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,31 @@
package tinkerbell

import (
"testing"

"github.com/google/go-cmp/cmp"
)

func TestPermuatations(t *testing.T) {
tests := []struct {
initial string
separators []string
want string
}{
{"125", []string{"-", "_", ""}, "125"},
{"1.26", []string{"-", "_", ""}, "1.26, 1-26, 1_26 or 126"},
{"1.27", []string{"."}, "1.27"},
{"1.28", []string{".", "."}, "1.28"},
{"1.29", []string{"-", "-", ""}, "1.29, 1-29 or 129"},
{"1.29.1", []string{"-", "_", "_", ""}, "1.29.1, 1-29-1, 1_29_1 or 1291"},
Copy link
Member

Choose a reason for hiding this comment

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

we don't support patch version on our spec, so we don't have to be covering this. hence a simpler approach might work?

Copy link
Member Author

Choose a reason for hiding this comment

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

what versions we support is business level logic. Coupling this with application logic will cause us future maintainability issues in the future. I don't recommend we couple them.

}

for _, tt := range tests {
t.Run(tt.initial, func(t *testing.T) {
got := permutations(tt.initial, tt.separators)
if diff := cmp.Diff(got, tt.want); diff != "" {
t.Errorf("permutations() mismatch (-got +want):\n%s", diff)
}
})
}
}
Loading