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

Add a CI Workflow to push docker image of multiubuntu on a change #1943

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

Conversation

Manik2708
Copy link

@Manik2708 Manik2708 commented Jan 17, 2025

Purpose of PR?:

Fixes #1876

Does this PR introduce a breaking change? No

If the changes in this PR are manually verified, list down the scenarios covered:: Will be tested in CI

Additional information for reviewer? :

I have introduced one change in multiubuntu directory to see if CI is working fine, as it is very difficult to test ci in fork. Once this PR is ready to merge, that change will be reverted!

Checklist:

  • Bug fix. Fixes #
  • 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

@Manik2708
Copy link
Author

@Aryan-sharma11 Sorry for not looking up for the issue. Will look into the CI failures and fix them!

@Manik2708
Copy link
Author

@Aryan-sharma11 I could understand the failure of ginkgo tests but can't get why crio tests are failing. Can you help with this?

@Aryan-sharma11
Copy link
Member

@Aryan-sharma11 I could understand the failure of ginkgo tests but can't get why crio tests are failing. Can you help with this?

@Manik2708 It looks like syscall test for BPFLSM runner are failing on some other PRs too, I'll look into it. Will update you on the issue soon.

@Manik2708
Copy link
Author

Manik2708 commented Jan 27, 2025

Thanks for your time @Aryan-sharma11. I think workflows were re-ran and now every test has passed. Please review cc @rksharma95

@rksharma95
Copy link
Collaborator

@Manik2708 thanks for the PR, can you check on your fork if path filter is working properly?

@Manik2708
Copy link
Author

@Manik2708 thanks for the PR, can you check on your fork if path filter is working properly?

@rksharma95 Please see this report of CI: https://github.com/kubearmor/KubeArmor/actions/runs/12970813140/job/36201280553?pr=1943#step:11:1. It was becoming difficult for me to test the changes in fork, so I introduced a dummy change in multiubuntu, when this PR will be ready to merge, I will revert this change!

@rksharma95
Copy link
Collaborator

@rksharma95 can you please check the other scenario as well? if no change on path examples/multiubuntu/build/** it skips the ubuntu build step.

@Manik2708
Copy link
Author

@rksharma95 Please run the workflow, now it should test the other scenario

@Manik2708
Copy link
Author

@rksharma95 Please run the workflow, now it should test the other scenario

@rksharma95 Please can you help me with the vulnerability failures? I have no idea why are they failing and how it is connected to this PR?

@rksharma95
Copy link
Collaborator

@Manik2708 can you upgrade the dependencies to their fixed version as suggested in the test result?

@Manik2708
Copy link
Author

@Manik2708 can you upgrade the dependencies to their fixed version as suggested in the test result?

@rksharma95 https://github.com/kubearmor/KubeArmor/actions/runs/13025109360/job/36333266926?pr=1943#step:4:35 For this I think we need to upgrade the go version. Am I correct?

@Manik2708
Copy link
Author

After fixing the versions, the security checks are failing. @rksharma95 How can we fix it?

@rksharma95
Copy link
Collaborator

can you check if you're able to build locally, check if running go mod tidy helps?

image

@Manik2708
Copy link
Author

can you check if you're able to build locally, check if running go mod tidy helps?

image

I ran go mod tidy, but let me try building it locally!

@Manik2708
Copy link
Author

Manik2708 commented Jan 29, 2025

@rksharma95 I have built locally successfully with these changes (tested the core KubeArmor code) and go mod tidy is not doing anything. But another thing which I could see is in KubeArmorOperator go.mod, versions are requiring updates (deployments, protobuf and core KubeArmor). And when I update them and run go mod tidy many dependencies get removed. Could it be related to this?

@rksharma95
Copy link
Collaborator

@Manik2708 let's push the changes.

@Manik2708
Copy link
Author

@rksharma95 Actually the PR is becoming very large, and I am concerned if it could be a breaking change if dependencies are upgraded directly. @Aryan-sharma11 helped me in the PR: kubearmor/kubearmor-client#471 which was giving similar errors even after running go mod tidy. Would be thankful if I can get details of error or if @Aryan-sharma11 can help us!

@rksharma95
Copy link
Collaborator

@rksharma95 Actually the PR is becoming very large, and I am concerned if it could be a breaking change if dependencies are upgraded directly. @Aryan-sharma11 helped me in the PR: kubearmor/kubearmor-client#471 which was giving similar errors even after running go mod tidy. Would be thankful if I can get details of error or if @Aryan-sharma11 can help us!

It's a snyk issue, let's upgrade to 1.23.5 we can ignore snyk failures.

@Manik2708
Copy link
Author

@rksharma95 Actually the PR is becoming very large, and I am concerned if it could be a breaking change if dependencies are upgraded directly. @Aryan-sharma11 helped me in the PR: kubearmor/kubearmor-client#471 which was giving similar errors even after running go mod tidy. Would be thankful if I can get details of error or if @Aryan-sharma11 can help us!

It's a snyk issue, let's upgrade to 1.23.5 we can ignore snyk failures.

Then can you please run the workflows, so that we could see if tests are passing on upgrading to 1.23.5?

.github/workflows/ci-test-ginkgo.yml Outdated Show resolved Hide resolved
@Manik2708 Manik2708 requested a review from rksharma95 January 30, 2025 07:33
@Manik2708
Copy link
Author

@rksharma95 Have made the change, please review!

@rksharma95
Copy link
Collaborator

@Manik2708 please squash all the commits.

@Manik2708
Copy link
Author

@rksharma95 Squashed! Please review!

@rksharma95
Copy link
Collaborator

@Manik2708 please handle these as well.
image

@Manik2708
Copy link
Author

@Manik2708 please handle these as well. image

@rksharma95 For this I am thinking of moving with this approach:

  1. Changing the 0.1 to latest in all files.
  2. Find for kubearmor/ubuntu-w-utils:latest in the directory tests/k8s_env
  3. Adding imagePyllPolicy: Always in every file.
  4. Changing imagePullPolicy from Always to Never everywhere in CI.

Now it rises a question: What if in future a contributor adds a new ubuntu-deployment file but then tests might not run against the local image for deployment if these conventions are not followed:

  1. Using kubearmor/ubuntu-w-utils:latest instead of kubearmor/ubuntu-w-utils:0.1.
  2. Always adding imagePullPolicy: Always to this.

Therefore I am thinking to throw an error if any of these conventions are not followed for any of the ubuntu-deployment file, so that the contributor who is adding a new multiubuntu deployment could follow the conventions. But then there could be an exception where these conventions might need to be bypassed, for that, those exceptions could be added in the CI to ignore the error from a particular file. Am I going in correct direction?

@rksharma95
Copy link
Collaborator

@Manik2708 I don't think we need to enforce this convention by any validation, we can add a comment in the workflow step name: make changes in multiubuntu-deployment itself to keep a note of this for reviewers.

@Manik2708
Copy link
Author

@rksharma95 Done! Please review!

@Manik2708
Copy link
Author

@rksharma95 Please can you run the workflows!

@Manik2708
Copy link
Author

Manik2708 commented Feb 4, 2025

Vulnerability fixed in the commit: 5882551 @rksharma95 Can you please run the workflows?

@Manik2708
Copy link
Author

@rksharma95 @Aryan-sharma11 Is it good to go?

@rksharma95
Copy link
Collaborator

@Manik2708 squash commits

@Manik2708
Copy link
Author

Manik2708 commented Feb 7, 2025

@Manik2708 squash commits

@rksharma95 It's long time I have squashed the commits but still GitHub is showing Processing Updates and haven't updated the changes. Can you help with this? My guess is it is not able to clean the commit history!

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
3 participants