gha: k8s: prepare AKS workflow to install the CoCo KBS #9116
Conversation
| # Set the KBS ingress handler (empty string disables handling) | ||
| KBS_INGRESS: ${{ matrix.kbs == 'true' && 'aks' || '' }} |
There was a problem hiding this comment.
| # Set the KBS ingress handler (empty string disables handling) | |
| KBS_INGRESS: ${{ matrix.kbs == 'true' && 'aks' || '' }} | |
| # Set the KBS ingress handler to "aks" if we install the KBS, otherwise pass an empty string to disable it | |
| KBS_INGRESS: ${{ matrix.kbs == 'true' && 'aks' || '' }} |
| - kbs: true | ||
| vmm: qemu | ||
| instance-type: small |
There was a problem hiding this comment.
Now that I'm thinking about it, it'll change the name of the test, which will require the dance of making tests non-required, and asking folks to rebase.
I'm okay with it, but I'd like to hear from @stevenhorsman on this.
There was a problem hiding this comment.
I think it'd be clearer to have the logic inside the shell scripts (with a possibility to override things) as the need for kbs comes from the test suite rather than from the workflow (although the line is pretty thin as the workflow defines the suite). So +1 for having it as part of the shell scripts from me.
There was a problem hiding this comment.
I think that having that as part of the shell scripts simply goes against what we've been doing as part of the Ci.
We've defined whether or not we install devmapper or nydus-snapshotter as part of the action, this one would be inconsistent.
There was a problem hiding this comment.
I see, TBH I'm not that familiar with the kata-containers style and I reacted based on my expectations (rather than magic variable I tend to favour logic to enable dependencies and kbs seems like a dependency, rather than module, to me; especially for the KBS_INGRESS I'd expect the scripts to enable it when KBS is enabled and on AKS)
There was a problem hiding this comment.
@fidencio one option to avoid the jobs renaming is to remove the new matrix entry; we filter by vmm on the if conditional of the step that install the KBS. All in all, we want to install kbs to run additional tests, if we create a new jobs we will be re-running tests already covered (increasing the odds of intermittent failures).
|
I just updated this with a fixup commit (that I should squash before merge) where I move the logic out of the matrix to avoid renaming of all jobs. Once the attestation tests are rock solid running on CI we can think on using the matrix configuration. What do you think? |
wainersm
added
no-backport-needed
area/ci
| @@ -58,9 +53,9 @@ jobs: | |||
| KATA_HOST_OS: ${{ matrix.host_os }} | |||
| KATA_HYPERVISOR: ${{ matrix.vmm }} | |||
| # Set to install the KBS for attestation tests | |||
| KBS: ${{ matrix.kbs }} | |||
| KBS: ${{ (matrix.vmm == 'qemu' && matrix.host_os == 'ubuntu') && 'true' || 'false' }} | |||
There was a problem hiding this comment.
Does this condition really depends on host_os? As far as I understood the previous version only cared about vmm...
There was a problem hiding this comment.
Hi @ldoktor ,
You are right, in the previous version I wasn't checking host_os. I thought in use that variable too to really restrict further the jobs that will have KBS installed. With the old condition, if someone adds qemu vmm to the mariner configuration than it would enable attestation to that combination, which we don't want (not at this point at least).
BTW, perhaps we should enable KBS on only one instance_type too...
There was a problem hiding this comment.
I'd go with no unnecessary conditions and I'd leave it on those who add the new types to decide. Anyway either way is fine by me.
There was a problem hiding this comment.
hi @ldoktor , I kept the most restrictive condition.
wainersm
force-pushed
the
ci_install_kbs-workflow
branch
from
4c98cf1
to
2f40279
Compare
|
Only rebased to main and squashed the two commits. No code changes. |
|
As suggested by @ldoktor in #9114 (comment) , it will require a new step on the workflow to install the kbs-client. I've added a new commit that should be squashed before the merge. |
Changed the "run k8s tests on AKS" workflows to get the CoCo KBS installed so that we can run attestation tests. The plan is to run attestation tests only on a subset of non-TEE jobs initially, so this commit restricts to install KBS only on kata-qemu configuration. Actually at this point it is added only stubs commands to tests/integration/kubernetes/gha-run.sh that should be implemented in a future commit. Fixes kata-containers#9058 Signed-off-by: Wainer dos Santos Moschetta <wainersm@redhat.com>
wainersm
force-pushed
the
ci_install_kbs-workflow
branch
from
b7807bc
to
b44e0c4
Compare
Done. |
|
/test |
Changed the "run k8s tests on AKS" workflows to get the CoCo KBS installed so that we can run attestation tests.
The plan is to run attestation tests only on a subset of non-TEE jobs initially, so this commit restricts to install KBS only on kata-qemu configuration. Actually at this point it is added only stubs commands to tests/integration/kubernetes/gha-run.sh that should be implemented in a future commit.
Fixes #9058
Signed-off-by: Wainer dos Santos Moschetta wainersm@redhat.com
This is a split of PR #9065. We merge this commit first to then allow to run CI on #9065.
@sprt @jodh-intel suggested on #9065 to move the logic about whether to install or not the KBS out of the workflow and into the shell script as this is easier for testing and future extending. However, talking with @fidencio today, despite agreeing with @sprt and @jodh-intel that it would be easier for testing; conceptually speaking it should be the workflow concerned about taking (or not) some action like install the kbs. So I moved the logic back to the workflow, however, unlike on the original version, this time I added the flag to enable/disable KBS installation on the matrix.
I ran the
actionlintprogram on the new workflow file, so I can ensure it has no syntax errors :)