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.

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

chore(build): Add a CI workflow to build and push multiubuntu image whenever a change detected #1878

Open
wants to merge 12 commits into
base: main
Choose a base branch
from

Conversation

sikehish
Copy link

Purpose of PR?:
Add a CI workflow to build and push the multi-architecture Ubuntu-based image for KubeArmor on changes to the main branch.

Fixes #1876

Does this PR introduce a breaking change?
No

If the changes in this PR are manually verified, list down the scenarios covered::

  • Verified that the CI workflow builds the Docker image successfully using multi-architecture (amd64 and arm64) support.
  • Confirmed that the Docker image pushes correctly to Docker Hub when the workflow is triggered by push or pull_request on the main branch.

Additional information for reviewer? :
This PR introduces a new CI workflow and is not dependent on any previous PR or design.

Checklist:

  • Bug fix. Fixes N/A
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to not work as expected)
  • This change requires a documentation update
  • PR Title follows the convention of <type>(<scope>): <subject>
  • Commit has unit tests
  • Commit has integration tests

.github/workflows/ci-build-and-push.yml Outdated Show resolved Hide resolved
.github/workflows/ci-build-and-push.yml Outdated Show resolved Hide resolved
.github/workflows/ci-build-and-push.yml Outdated Show resolved Hide resolved
password: ${{ secrets.DOCKER_AUTHTOK }}

- name: Build and push multi-architecture image
uses: docker/build-push-action@v6
Copy link
Collaborator

Choose a reason for hiding this comment

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

examples/multiubuntu/build/build.sh can handle build and push as well, no need to setup docker action for it.
build and push

Copy link
Member

Choose a reason for hiding this comment

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

We need multiarch builds, they are not handled in the script.

Copy link
Collaborator

Choose a reason for hiding this comment

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

my bad, in that case keep repo name same that used in the script

Copy link
Author

Choose a reason for hiding this comment

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

examples/multiubuntu/build/build.sh can handle build and push as well, no need to setup docker action for it. build and push

Do I need to make any changes? Do I need to use examples/multiubuntu/build/build.sh? A bit confused here. Would love for some clarity :))

Copy link
Collaborator

Choose a reason for hiding this comment

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

@sikehish no need to use build.sh script, changes with your PR looks good just change image tag kubearmor/ubuntu-w-utils:latest

Copy link
Author

Choose a reason for hiding this comment

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

Hi @rksharma95 . Ive made the change. Let me know if you need me to change anything else :)

@rksharma95
Copy link
Collaborator

@sikehish thanks for the PR, just some suggestions, looks good otherwise. also can you test it on your fork first??

@sikehish
Copy link
Author

@sikehish thanks for the PR, just some suggestions, looks good otherwise. also can you test it on your fork first??

Thanks for the oppurtunity too :) I've made all the other changes that you had requested for, barring #1878 (comment) . It's working on my fork.

image

rksharma95
rksharma95 previously approved these changes Oct 21, 2024
@rksharma95 rksharma95 requested a review from daemon1024 October 21, 2024 11:35
@rksharma95
Copy link
Collaborator

rksharma95 commented Oct 22, 2024

@sikehish can you please add examples/multiubuntu/build path as well here, to run the e2e tests for any change

edit: we'll need to handle the case that latest built ubuntu image should be used in the tests

Added examples/multiubuntu/build path in the CI workflow to run end-to-end tests for any changes.
@sikehish
Copy link
Author

@sikehish can you please add examples/multiubuntu/build path as well here, to run the e2e tests for any change

edit: we'll need to handle the case that latest built ubuntu image should be used in the tests

Yup. I've added it. Let me know if any other changes need to be made.

@rksharma95
Copy link
Collaborator

@sikehish can you please add examples/multiubuntu/build path as well here, to run the e2e tests for any change

edit: we'll need to handle the case that latest built ubuntu image should be used in the tests

Yup. I've added it. Let me know if any other changes need to be made.

we'll need handling that the locally built image with changes pushed with PR used in testing. at this point it will just pull the image from kubearmor repo.

@sikehish
Copy link
Author

@sikehish can you please add examples/multiubuntu/build path as well here, to run the e2e tests for any change

edit: we'll need to handle the case that latest built ubuntu image should be used in the tests

Yup. I've added it. Let me know if any other changes need to be made.

we'll need handling that the locally built image with changes pushed with PR used in testing. at this point it will just pull the image from kubearmor repo.

I didn't quite get you. Do I have make any change in any of the test yml file? What change do I need to make and in which file? Also, how do I handle the case that latest built ubuntu image be used in the tests? Could you elaborate a bit more.
Thank you!

@Manik2708
Copy link

Manik2708 commented Dec 4, 2024

@sikehish can you please add examples/multiubuntu/build path as well here, to run the e2e tests for any change

edit: we'll need to handle the case that latest built ubuntu image should be used in the tests

Yup. I've added it. Let me know if any other changes need to be made.

we'll need handling that the locally built image with changes pushed with PR used in testing. at this point it will just pull the image from kubearmor repo.

I didn't quite get you. Do I have make any change in any of the test yml file? What change do I need to make and in which file? Also, how do I handle the case that latest built ubuntu image be used in the tests? Could you elaborate a bit more. Thank you!

I guess he is trying to say that in e2e tests, it should pick the locally built image rather than pulling from docker hub. Am I right? @rksharma95? To achieve this need to save docker image of multibuntu with latest tag and then this image will be used for tests. See ci-test-controllers.yml for getting the context of using local images for testing!

@sikehish
Copy link
Author

sikehish commented Dec 4, 2024

@sikehish can you please add examples/multiubuntu/build path as well here, to run the e2e tests for any change

edit: we'll need to handle the case that latest built ubuntu image should be used in the tests

Yup. I've added it. Let me know if any other changes need to be made.

we'll need handling that the locally built image with changes pushed with PR used in testing. at this point it will just pull the image from kubearmor repo.

I didn't quite get you. Do I have make any change in any of the test yml file? What change do I need to make and in which file? Also, how do I handle the case that latest built ubuntu image be used in the tests? Could you elaborate a bit more. Thank you!

I guess he is trying to say that in e2e tests, it should pick the locally built image rather than pulling from docker hub. Am I right? @rksharma95? To achieve this need to save docker image of multibuntu with latest tag and then this image will be used for tests. See ci-test-controllers.yml for getting the context of using local images for testing!

Thanks for clarifying! I'll look into it

@sikehish
Copy link
Author

sikehish commented Dec 4, 2024

Hi @rksharma95 @Manik2708
I've made the requested change. Could you let me know if its okay? I'm currently failing a step (Generate KubeArmor artifacts) in the crio runtime

- name: Build multiubuntu image
run: |
cd examples/multiubuntu/build
docker build -t kubearmor/multiubuntu:latest .

Choose a reason for hiding this comment

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

Please change the name to kubearmor/ubuntu-w-utils:latest

@Manik2708
Copy link

Manik2708 commented Dec 4, 2024

Hi @rksharma95 @Manik2708 I've made the requested change. COuld you let me know if its okay? I'm currently failing a step (Generate KubeArmor artifacts) in the crio runtime

Can you share the logs or you local github action report or provide the error?

@@ -84,6 +91,12 @@ jobs:

- name: Run KubeArmor
run: |
if [[ ${{ matrix.runtime }} == "containerd" ]]; then

Choose a reason for hiding this comment

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

Why another if statement? Is there any problem for putting in the same if statement?

Copy link
Author

@sikehish sikehish Dec 4, 2024

Choose a reason for hiding this comment

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

Why another if statement? Is there any problem for putting in the same if statement?

I thought it'd make it a bit more readable(as the kubearmor related images would be in a different block and the multiubuntu docker save block would be prominent enough. I'll make that change right away :)

@sikehish
Copy link
Author

sikehish commented Dec 4, 2024

Hi @rksharma95 @Manik2708 I've made the requested change. COuld you let me know if its okay? I'm currently failing a step (Generate KubeArmor artifacts) in the crio runtime

Can you share the logs or you local github action report or provide the error?

image

It worked for containerd, but failed for crio.

@sikehish
Copy link
Author

sikehish commented Dec 4, 2024

@rksharma95 @Manik2708 I've made those minor fixes. Is there anything else that needs to be changed?

@Manik2708
Copy link

Hi @rksharma95 @Manik2708 I've made the requested change. COuld you let me know if its okay? I'm currently failing a step (Generate KubeArmor artifacts) in the crio runtime

Can you share the logs or you local github action report or provide the error?

image

It worked for containerd, but failed for crio.

Please share the action logs, it's difficult to conclude from the image!

@Manik2708
Copy link

Also some more changes are required! The tests which is using multiubuntu image is ksp_test. This test is applying the yaml file in test/k8s_env/ksp/multiubuntu/multiubuntu-deployment.yaml which is using the 0.1 version of image. Either we have to change it permanently to latest or you can write a command in CI to change it to latest and revert it after the test. Not doing this will always pull image from docker hub instead of using the local one. @rksharma95 Please tell which will be the correct way!

@Manik2708
Copy link

Also some more changes are required! The tests which is using multiubuntu image is ksp_test. This test is applying the yaml file in test/k8s_env/ksp/multiubuntu/multiubuntu-deployment.yaml which is using the 0.1 version of image. Either we have to change it permanently to latest or you can write a command in CI to change it to latest and revert it after the test. Not doing this will always pull image from docker hub instead of using the local one. @rksharma95 Please tell which will be the correct way!

You might have to change the imagePullPolicy if required!

@rksharma95
Copy link
Collaborator

Also some more changes are required! The tests which is using multiubuntu image is ksp_test. This test is applying the yaml file in test/k8s_env/ksp/multiubuntu/multiubuntu-deployment.yaml which is using the 0.1 version of image. Either we have to change it permanently to latest or you can write a command in CI to change it to latest and revert it after the test. Not doing this will always pull image from docker hub instead of using the local one. @rksharma95 Please tell which will be the correct way!

change it to latest in the deployment,
conditionally build the multiubuntu image based on the check if there's any change, use filter to detect it i.e.

- name: Check what paths were updated

and if local image is supposed to be used in the tests use imagePullPolicy: Never in deployment.

@sikehish
Copy link
Author

sikehish commented Dec 7, 2024

@rksharma95 @Manik2708 I'm looking into the issues flagged. Apologize for the delay; a bit caught up with uni work

@Manik2708
Copy link

@rksharma95 @Manik2708 I'm looking into the issues flagged. Apologize for the delay; a bit caught up with uni work

Hi! Are you still working on this PR? Asking if you are facing any problem!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

add CI workflow to build and push multiubuntu image on change
4 participants