Fix e2e_client_server_connectivity_test for NoInstall #4708
Conversation
| @@ -442,6 +442,7 @@ func setMeshConfigToDefault(instOpts InstallOSMOpts, meshConfig *configv1alpha2. | |||
| meshConfig.Spec.Sidecar.LogLevel = instOpts.EnvoyLogLevel | |||
| meshConfig.Spec.Sidecar.MaxDataPlaneConnections = 0 | |||
| meshConfig.Spec.Sidecar.ConfigResyncInterval = "0s" | |||
| meshConfig.Spec.Sidecar.LocalProxyMode = instOpts.LocalProxyMode | |||
There was a problem hiding this comment.
@keithmattix isn't this set as the default? Wondering why this needs to be explicitly set without looking further at the test code.
There was a problem hiding this comment.
LocalProxyMode is changed for the "Test traffic flowing from client to a server with a podIP bind" part of the test to v1alpha2.LocalProxyModePodIP, the previous 2 ginkgo.contexts use the default Proxy Mode.
The way the test was before, the new value for LocalProxyMode was only being set for fresh installs. For the change to also be made for NoInstall, the value needs to be explicitly changed in setMeshConfigToDefault.
There was a problem hiding this comment.
Because we don't have a helper function to access the LocalProxyMode field, there's no default being set outside of a Helm install. So when the MeshConfig doesn't have it set, it's just an empty string
There was a problem hiding this comment.
@keithmattix Why don't we default its value in the CRD schema?
There was a problem hiding this comment.
We do. But unless we're using an openapi unmarshaler, those defaults won't take effect
There was a problem hiding this comment.
We could add a default in the json unmarshaler, but that also means we would need to change the API version if we change those defaults. This seems more like a test issue where the control plane is reused between tests.
There was a problem hiding this comment.
Yep, that's one of the downsides to adding not bumping a new version. This will also happen when users bump their OSM version, but because LocalProxyConfig is an opt-in behavior, we're fine with the 3rd case (unset) being the default.
|
@nshankar13 please add a commit description to share more context regarding the issue being fixed.. |
Currently, e2e_client_server_connectivity_test only changes the value of LocalProxyMode for fresh OSM installs, causing the test to fail for the podIP bind for NoInstall. To change the value of LocalProxyMode for NoInstall, it must be changed explicitly in setMeshConfigToDefault. Signed-off-by: nshankar13 <nshankar@microsoft.com>
ca8fa3a to
13f9d82
Compare
Currently, e2e_client_server_connectivity_test only changes the value of LocalProxyMode for fresh OSM installs, causing the test to fail for the podIP bind for NoInstall. To change the value of LocalProxyMode for NoInstall, it must be changed explicitly in setMeshConfigToDefault. Signed-off-by: nshankar13 <nshankar@microsoft.com>
Signed-off-by: nshankar13 nshankar@microsoft.com
Description: Fix e2e_client_server_connectivity_test for NoInstall by explictly setting LocalProxyMode in setMeshConfigToDefault Method.
Resolves #4707
Testing done: Tested locally on AKS cluster with OSM already installed.
Affected area:
Please answer the following questions with yes/no.
Does this change contain code from or inspired by another project?
Is this a breaking change?
Has documentation corresponding to this change been updated in the osm-docs repo (if applicable)?