Add helm install to e2e test framework

[ritazh]

Please describe the Improvement and/or Feature Request

In addition to installation via the osm cli, helm install needs to be added as another installation process to the e2e test framework regardless of specific scenarios we are testing.

Scope (please mark with X where applicable)

• New Functionality [ ]
• Install [ ]
• SMI Traffic Access Policy [ ]
• SMI Traffic Specs Policy [ ]
• SMI Traffic Split Policy [ ]
• Permissive Traffic Policy [ ]
• Ingress [ ]
• Egress [ ]
• Envoy Control Plane [ ]
• CLI Tool [ ]
• Metrics [ ]
• Certificate Management [ ]
• Sidecar Injection [ ]
• Logging [ ]
• Debugging [ ]
• Tests [X]
• CI System [ ]
• Project Release [ ]

Possible use cases

[nojnhuh]

@ritazh How were you thinking this should work? The ways I can think of:

1. Configure the installation method directly in each test's code
2. Add a new command line flag to make all the tests run with a particular installation method
3. Start building a matrix and always run all the tests with both methods

[ritazh]

@nojnhuh Ideally we should have every test run against a cluster with helm install and against a cluster with the osm cli to ensure both deployment methods have full coverage.

[nojnhuh]

@ritazh If we do the same thing for all the other install parameters like cert manager, egress enabled/disabled, permissive traffic policy, etc., I worry that every combination of different parameters would make the suite way too big. And since osm install is essentially a thin wrapper over helm install with some custom validation, I'm not optimistic we would find any issues in the e2e suite that are neither unique to one method nor can be replicated with a unit test for the osm install command.

I would suggest we add one or two tests that use Helm directly, but not run every single test with both methods.

[ritazh]

I worry that every combination of different parameters would make the suite way too big

@nojnhuh This is definitely a valid concern. I think at a minimum, we need to test if the installation via helm install installed all the components with the default values provided in the values.yaml and that they are running successfully. We want to catch issues like #1829 before a PR is merged or before a release is cut.

[nojnhuh]

@ritazh I don't disagree with anything you're saying, but I think the mechanics of testing that are a little tricky.

We want to catch issues like #1829 before a PR is merged or before a release is cut.

I'm just thinking out loud here, but isn't this kind of a chicken-egg problem? Since helm install with no value overrides will use the last tagged osm-controller image, even if an osm-controller bug like that is fixed in a PR, the test will use an older osm-controller image without the fix, so the test will still fail. To update the osm-controller, we would need to make a release, but we shouldn't make a release if tests are failing.

I think at a minimum, we need to test if the installation via helm install installed all the components with the default values provided in the values.yaml and that they are running successfully.

I think if we want to ensure installing with the default values works for a tagged release, we need to override at least the image.tag value in CI to simulate the behavior of the default values for a tagged release. Testing without any overrides would need to be done only after the tagged images have been pushed.

[ritazh]

The test will use an older osm-controller image without the fix

To test the binary changes in a PR, the CI can build an image from the PR, load it to the kind cluster, then set the image value to the local image we just built for helm

[nojnhuh]

Yeah, that's the flow we have implemented right now for the other tests. To test with the default values though (i.e. not overriding the image) we could really only do that on the tagged commits or else we'll get version skew.

[ritazh]

To test with the default values though (i.e. not overriding the image) we could really only do that on the tagged commits or else we'll get version skew.

Can you please expand on this? Why do we not want to override the image? I was suggesting that we could build the image locally in the pipeline, load the image to the kind cluster, then override the image in the chart to test the PR.

[nojnhuh]

Perhaps I wasn't fully understanding your earlier comment:

I think at a minimum, we need to test if the installation via helm install installed all the components with the default values provided in the values.yaml and that they are running successfully.

I think I was reading that as "We should test that a plain helm install osm ./charts/osm without any overrides works for each PR.

But yes, I agree we should override the image tag to use one built with the source from the particular PR we're testing.