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

Jump to bottom

[WIP]OSSM-5881 Do not require ingress and egress gateways when spec.cluster.multiCluster is enabled #1643

Open
wants to merge 10 commits into
base: maistra-2.4
Choose a base branch
from
Open

[WIP]OSSM-5881 Do not require ingress and egress gateways when spec.cluster.multiCluster is enabled #1643

Show file tree
Hide file tree
Changes from 9 commits
Commits
Show all changes
10 commits
Select commit Hold shift + click to select a range
7177a0e
OSSM-5881 Do not require ingress and egress gateways when `spec.clust…
mayleighnmyers Feb 21, 2024
8cdf0a3
Make gen with updating the file
mayleighnmyers Apr 18, 2024
f43ecc0
Rerun make gen
mayleighnmyers Apr 18, 2024
5418648
Removing tests
mayleighnmyers Jun 13, 2024
3e62d89
Adding test case
mayleighnmyers Jul 16, 2024
e79f64d
Fixing test case
mayleighnmyers Jul 18, 2024
963529d
Remove test imports
mayleighnmyers Jul 18, 2024
e891d97
Readded valmap
mayleighnmyers Jul 25, 2024
25a13be
Readd tests and fix test
mayleighnmyers Aug 30, 2024
1de63a9
Moved and edited test
mayleighnmyers Oct 18, 2024
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
236 changes: 0 additions & 236 deletions pkg/apis/maistra/conversion/cluster.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,6 @@ package conversion

import (
"fmt"
"regexp"
"strings"

corev1 "k8s.io/api/core/v1"
Expand All @@ -19,8 +18,6 @@ const (
clusterDomainDefault = "cluster.local"
)

var externalRequestedNetworkRegex = regexp.MustCompile("(^|,)external(,|$)")

// populateClusterValues popluates values.yaml specific to clustering. this
// function will also update fields in other settings that are related to
// clustering, e.g. MeshExpansionPorts on Ingress gateway and DNS search
Expand Down Expand Up @@ -390,241 +387,8 @@ func populateClusterConfig(in *v1.HelmValues, out *v2.ControlPlaneSpec) error {
} else if err != nil {
return err
}

// patchup gateways
if rawMultiClusterOverrides, ok, err := in.GetMap("global.multiCluster.multiClusterOverrides"); ok && len(rawMultiClusterOverrides) > 0 {
multiClusterOverrides := v1.NewHelmValues(rawMultiClusterOverrides)
if gateways, ok, err := in.GetMap("gateways"); ok && len(gateways) > 0 {
updateGateways := false
if gatewaysEnabled, ok, err := multiClusterOverrides.GetFieldNoCopy("gatewaysEnabled"); ok {
if gatewaysEnabled == nil {
delete(gateways, "enabled")
} else {
gateways["enabled"] = gatewaysEnabled
}
updateGateways = true
} else if err != nil {
return err
}
if ingressEnabled, ok, err := multiClusterOverrides.GetFieldNoCopy("ingressEnabled"); ok {
if ingressGateway, ok, err := v1.NewHelmValues(gateways).GetMap("istio-ingressgateway"); ok && len(ingressGateway) > 0 {
if ingressEnabled == nil {
delete(ingressGateway, "enabled")
} else {
ingressGateway["enabled"] = ingressEnabled
}
if shouldDeleteGatewayValues(ingressGateway) {
// only element should be name
delete(gateways, "istio-ingressgateway")
} else {
gateways["istio-ingressgateway"] = ingressGateway
}
updateGateways = true
} else if err != nil {
return nil
}
} else if err != nil {
return nil
}
if egressGateway, ok, err := v1.NewHelmValues(gateways).GetMap("istio-egressgateway"); ok && len(egressGateway) > 0 {
updateEgress := false
if egressEnabled, ok, err := multiClusterOverrides.GetFieldNoCopy("egressEnabled"); ok {
if egressEnabled == nil {
delete(egressGateway, "enabled")
} else {
egressGateway["enabled"] = egressEnabled
}
updateEgress = true
} else if err != nil {
return nil
}
if addedExternal, ok, err := multiClusterOverrides.GetAndRemoveBool("addedExternal"); ok && addedExternal {
if requestedNetworkView, ok, err := v1.NewHelmValues(egressGateway).GetAndRemoveString("env.ISTIO_META_REQUESTED_NETWORK_VIEW"); ok {
newRequestedNetworkView := externalRequestedNetworkRegex.ReplaceAllString(requestedNetworkView, "$1")
if newRequestedNetworkView != requestedNetworkView && newRequestedNetworkView != "" {
updateEgress = true
if err := setHelmStringValue(egressGateway, "env.ISTIO_META_REQUESTED_NETWORK_VIEW", newRequestedNetworkView); err != nil {
return err
}
}
// cleanup for to avoid extraneous empty ClusterEgress
if env, ok, err := v1.NewHelmValues(egressGateway).GetMap("env"); ok && len(env) == 0 {
delete(egressGateway, "env")
updateEgress = true
} else if err != nil {
return err
}
} else if err != nil {
return err
}
} else if err != nil {
return err
}
if updateEgress {
if shouldDeleteGatewayValues(egressGateway) {
// only element should be name
delete(gateways, "istio-egressgateway")
} else {
gateways["istio-egressgateway"] = egressGateway
}
updateGateways = true
}
} else if err != nil {
return nil
}
if ilbEnabled, ok, err := multiClusterOverrides.GetFieldNoCopy("ilbEnabled"); ok {
if ilbGateway, ok, err := v1.NewHelmValues(gateways).GetMap("istio-ilbgateway"); ok && len(ilbGateway) > 0 {
if ilbEnabled == nil {
delete(ilbGateway, "enabled")
} else {
ilbGateway["enabled"] = ilbEnabled
}
if shouldDeleteGatewayValues(ilbGateway) {
// only element should be name
delete(gateways, "istio-ilbgateway")
} else {
gateways["istio-ilbgateway"] = ilbGateway
}
updateGateways = true
} else if err != nil {
return nil
}
} else if err != nil {
return nil
}
if updateGateways {
if len(gateways) == 0 {
delete(in.GetContent(), "gateways")
} else {
if err := in.SetField("gateways", gateways); err != nil {
return err
}
}
}
} else if err != nil {
return err
}
if k8sIngressEnabled, ok, err := multiClusterOverrides.GetFieldNoCopy("k8sIngressEnabled"); ok {
if k8sIngressValues, ok, err := in.GetMap("global.k8sIngress"); ok && len(k8sIngressValues) > 0 {
if k8sIngressEnabled == nil {
delete(k8sIngressValues, "enabled")
} else {
k8sIngressValues["enabled"] = k8sIngressEnabled
}
if err := in.SetField("global.k8sIngress", k8sIngressValues); err != nil {
return err
}
} else if err != nil {
return nil
}
} else if err != nil {
return nil
}
if expansionEnabled, ok, err := multiClusterOverrides.GetFieldNoCopy("expansionEnabled"); ok {
if expansionEnabled == nil {
in.RemoveField("global.meshExpansion.enabled")
} else if err := in.SetField("global.meshExpansion.enabled", expansionEnabled); err != nil {
return err
}
} else if err != nil {
return nil
}
if multiClusterEnabled, ok, err := multiClusterOverrides.GetFieldNoCopy("multiClusterEnabled"); ok {
if multiClusterEnabled == nil {
in.RemoveField("global.multiCluster.enabled")
} else if err := in.SetField("global.multiCluster.enabled", multiClusterEnabled); err != nil {
return err
}
} else if err != nil {
return nil
}
} else if err != nil {
return err
}
in.RemoveField("global.multiCluster.multiClusterOverrides")

// multi-cluster
multiCluster := &v2.MultiClusterConfig{}
setMultiCluster := false
if multiClusterEnabled, ok, err := in.GetAndRemoveBool("global.multiCluster.enabled"); ok {
multiCluster.Enabled = &multiClusterEnabled
setMultiCluster = true
} else if err != nil {
return err
}
if rawMeshNetworks, ok, err := in.GetMap("global.meshNetworks"); ok && len(rawMeshNetworks) > 0 {
multiCluster.MeshNetworks = make(map[string]v2.MeshNetworkConfig)
if err := decodeAndRemoveFromValues(rawMeshNetworks, &multiCluster.MeshNetworks); err != nil {
return err
}
if len(rawMeshNetworks) == 0 {
in.RemoveField("global.meshNetworks")
} else if err := in.SetField("global.meshNetworks", rawMeshNetworks); err != nil {
return err
}
// remove defaulted mesh network
if addedLocalNetwork, ok, err := in.GetAndRemoveString("global.multiCluster.addedLocalNetwork"); ok {
delete(multiCluster.MeshNetworks, addedLocalNetwork)
} else if err != nil {
return nil
}
if len(multiCluster.MeshNetworks) == 0 {
multiCluster.MeshNetworks = nil
} else {
setMultiCluster = true
}
} else if err != nil {
return err
}

if setMultiCluster {
clusterConfig.MultiCluster = multiCluster
setClusterConfig = true
}

meshExpansionConfig := &v2.MeshExpansionConfig{}
setMeshExpansion := false
if expansionEnabled, ok, err := in.GetAndRemoveBool("global.meshExpansion.enabled"); ok {
setMeshExpansion = true
meshExpansionConfig.Enabled = &expansionEnabled
} else if err != nil {
return err
}
if ilbGatewayValues, ok, err := in.GetMap("gateways.istio-ilbgateway"); ok && len(ilbGatewayValues) > 0 {
in.RemoveField("gateways.istio-ilbgateway")
setMeshExpansion = true
meshExpansionConfig.ILBGateway = &v2.GatewayConfig{}
if err := gatewayValuesToConfig(v1.NewHelmValues(ilbGatewayValues), meshExpansionConfig.ILBGateway); err != nil {
return err
}
} else if err != nil {
return err
}

if setMeshExpansion {
clusterConfig.MeshExpansion = meshExpansionConfig
setClusterConfig = true
}

// clear out defaults
in.RemoveField("gateways.istio-ilbgateway.enabled")
in.RemoveField("global.meshExpansion.enabled")
in.RemoveField("global.meshExpansion.useILB")

if setClusterConfig {
out.Cluster = clusterConfig
}

return nil
}

func shouldDeleteGatewayValues(gateway map[string]interface{}) bool {
minGatewaySize := 0
if _, ok := gateway["name"]; ok {
minGatewaySize++
}
if _, ok := gateway["gatewayType"]; ok {
minGatewaySize++
}
return len(gateway) == minGatewaySize
}
64 changes: 64 additions & 0 deletions pkg/apis/maistra/conversion/security_test.go
View file
Open in desktop
Copy link
Member

Choose a reason for hiding this comment

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

You modified cluster.go, so I would expect to see appropriate tests in cluster_test.go.

Original file line number Diff line number Diff line change
Expand Up @@ -1034,3 +1034,67 @@ func TestSecurityConversionFromV2ToV1(t *testing.T) {
})
}
}

type multiClusterTestCase struct {
name string
spec *v2.ControlPlaneSpec
expectedHelmValues v1.HelmValues
}

var multiClusterTestCases = []multiClusterTestCase{
{
name: "MultiCluster_v2_4",
spec: &v2.ControlPlaneSpec{
Version: versions.V2_4.String(),
Cluster: &v2.ControlPlaneClusterConfig{
Name: "my-cluster",
Network: "my-network",
MultiCluster: &v2.MultiClusterConfig{
Enablement: v2.Enablement{
Enabled: &featureEnabled,
},
},
},
Gateways: &v2.GatewaysConfig{
Enablement: v2.Enablement{
Enabled: &featureDisabled,
},
},
},
expectedHelmValues: buildHelmValues(`
global:
multiCluster:
enabled: true
gateways:
enabled: false
`),
Copy link
Member

Choose a reason for hiding this comment

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

These values are obviously incorrect, so this test fails as expected.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

When I run the test locally it is asking me to add a bunch of stuff, should I keep it to what you had put into the jira?

Copy link
Member

Choose a reason for hiding this comment

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

When I run the test locally it is asking me to add a bunch of stuff

You should never adjust expected test data to the actual result just to make the test pass. Your test and data should drive the implementation, so first fix the expected helm values and the input (SMCP spec) and then work on the implementation until it passes for the correct input and output.

should I keep it to what you had put into the jira?

In JIRA I added only SMCP spec. You can use that YAML to properly configure input SMCP in your test.

},
}

func TestMultiClusterGatewaysDisabled(t *testing.T) {
for _, tc := range multiClusterTestCases {
t.Run(tc.name+"-v2_to_v1", func(t *testing.T) {
var specV1 v1.ControlPlaneSpec
if err := Convert_v2_ControlPlaneSpec_To_v1_ControlPlaneSpec(tc.spec.DeepCopy(), &specV1, nil); err != nil {
t.Errorf("failed to convert SMCP v2 to v1: %s", err)
}

if !reflect.DeepEqual(tc.expectedHelmValues.DeepCopy(), specV1.Istio.DeepCopy()) {
t.Errorf("unexpected output converting v2 to values:\n\texpected:\n%#v\n\tgot:\n%#v", tc.expectedHelmValues.GetContent(), specV1.Istio.GetContent())
}
})

t.Run(tc.name+"-v1_to_v2", func(t *testing.T) {
specV1 := v1.ControlPlaneSpec{
Istio: tc.expectedHelmValues.DeepCopy(),
}
specV2 := v2.ControlPlaneSpec{}
if err := Convert_v1_ControlPlaneSpec_To_v2_ControlPlaneSpec(&specV1, &specV2, nil); err != nil {
t.Errorf("failed to convert SMCP v1 to v2: %s", err)
}

assertEquals(t, tc.spec.Cluster, specV2.Cluster)
assertEquals(t, tc.spec.Gateways, specV2.Gateways)
})
}
}