src: Use jobs not plain pods for auxiliary tasks #1857

matejvasek · 2023-07-11T17:58:59Z

Changes

🧹 Use jobs instead of plain pods for auxiliary tasks.
Rationale: job should have security context set properly automatically.

use `job`s instead of plain `pod`s for auxiliary tasks

Job should have security context set properly automatically. Signed-off-by: Matej Vasek <mvasek@redhat.com>

knative-prow · 2023-07-11T17:59:01Z

Skipping CI for Draft Pull Request.
If you want CI signal for your change, please convert it to an actual PR.
You can still manually trigger a test run with /test all

knative-prow · 2023-07-11T17:59:07Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: matejvasek

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

~~OWNERS~~ [matejvasek]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

matejvasek · 2023-07-11T18:01:00Z

@grafvonb please review this and test it on AWS EKS. In particular try running on-cluster-build&re-build on EKS.

codecov · 2023-07-11T18:08:31Z

Codecov Report

Patch coverage: 51.04% and project coverage change: -2.36% ⚠️

Comparison is base (79c36ee) 61.90% compared to head (0d386b9) 59.54%.

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #1857      +/-   ##
==========================================
- Coverage   61.90%   59.54%   -2.36%     
==========================================
  Files         107      106       -1     
  Lines       13710    13710              
==========================================
- Hits         8487     8164     -323     
- Misses       4381     4722     +341     
+ Partials      842      824      -18

Flag	Coverage Δ
e2e-test	`35.52% <0.00%> (ø)`
e2e-test-oncluster	`30.50% <51.04%> (-0.06%)`	⬇️
e2e-test-oncluster-runtime	`25.67% <0.00%> (?)`
e2e-test-runtime-go	`25.79% <0.00%> (?)`
e2e-test-runtime-node	`26.88% <0.00%> (?)`
e2e-test-runtime-python	`26.88% <0.00%> (?)`
e2e-test-runtime-quarkus	`27.00% <0.00%> (?)`
e2e-test-runtime-springboot	`26.02% <0.00%> (?)`
e2e-test-runtime-typescript	`27.00% <0.00%> (?)`
integration-tests	`?`
unit-tests-macos-latest	`49.33% <0.00%> (ø)`
unit-tests-ubuntu-latest	`50.09% <0.00%> (-0.03%)`	⬇️
unit-tests-windows-latest	`49.33% <0.00%> (ø)`

Flags with carried forward coverage won't be shown. Click here to find out more.

Files Changed	Coverage Δ
pkg/k8s/dialer.go	`17.97% <16.32%> (-60.52%)`	⬇️
pkg/k8s/persistent_volumes.go	`60.57% <87.23%> (-11.22%)`	⬇️

... and 10 files with indirect coverage changes

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Signed-off-by: Matej Vasek <mvasek@redhat.com>

matejvasek · 2023-07-11T23:52:31Z

/retest

grafvonb · 2023-07-12T07:58:44Z

@grafvonb please review this and test it on AWS EKS. In particular try running on-cluster-build&re-build on EKS.

I will look at this however during my current work on #1466 I get problems with the deploy task on EKS (Kubernetes v1.24). The error says that the spec for Knative Service is wrong. Do you now the issue?

❯ f deploy --remote -v
Creating Pipeline resources
Running Pipeline Task: Building function image on the cluster
Running Pipeline Task: Deploying function to the cluster
Error: failed to run pipeline: function pipeline run has failed with message:

⬆️  Deploying function to the cluster
W0712 07:45:15.552757      15 warnings.go:70] Kubernetes default value is insecure, Knative may default this to secure in a future release: spec.template.spec.containers[0].securityContext.allowPrivilegeEscalation, spec.template.spec.containers[0].securityContext.capabilities, spec.template.spec.containers[0].securityContext.runAsNonRoot, spec.template.spec.containers[0].securityContext.seccompProfile
Error: knative deployer failed to deploy the Knative Service: admission webhook "validation.webhook.serving.knative.dev" denied the request: validation failed: missing field(s): spec.template.spec.containers[0].image
deploy error: knative deployer failed to deploy the Knative Service: admission webhook "validation.webhook.serving.knative.dev" denied the request: validation failed: missing field(s): spec.template.spec.containers[0].image

matejvasek · 2023-07-12T14:24:47Z

the error says that the spec for Knative Service is wrong.

@grafvonb I don't know about the issue. Try asking in knative serving slack. Can you reproduce the issue by running kn service create foo-ksvc --image=gcr.io/knative-samples/helloworld-go ?

matejvasek · 2023-07-12T14:52:19Z

The seccompProfile warning is probably knative serving issue, but it's just warning.
But the missing field(s): spec.template.spec.containers[0].image is really odd.

matejvasek · 2023-07-17T14:39:19Z

@grafvonb have you had opportunity to test this?

grafvonb · 2023-07-20T12:21:26Z

@matejvasek Due to the current workload at work, I will not be able to address this issue until the weekend. My plan is to set up a new cluster, since I can't do anything with the error mentioned above.

grafvonb · 2023-07-23T20:05:13Z

@matejvasek As discussed I have created today a new AWS EKS cluster with Kubernetes 1.27, latest Knative and Tekton to test this PR. The test was successful. Please find following logs with detailed information:

❯ k version --short
Flag --short has been deprecated, and will be removed in the future. The --short output will become the default.
Client Version: v1.27.2
Kustomize Version: v5.0.1
Server Version: v1.27.3-eks-a5565ad

❯ k get nodes -o wide
NAME                                             STATUS   ROLES    AGE   VERSION               INTERNAL-IP     EXTERNAL-IP    OS-IMAGE         KERNEL-VERSION                  CONTAINER-RUNTIME
ip-172-31-29-159.eu-central-1.compute.internal   Ready    <none>   27h   v1.27.3-eks-a5565ad   172.31.29.159   3.66.18.102    Amazon Linux 2   5.10.184-175.731.amzn2.x86_64   containerd://1.6.19
ip-172-31-40-124.eu-central-1.compute.internal   Ready    <none>   27h   v1.27.3-eks-a5565ad   172.31.40.124   35.157.82.57   Amazon Linux 2   5.10.184-175.731.amzn2.x86_64   containerd://1.6.19
ip-172-31-9-54.eu-central-1.compute.internal     Ready    <none>   13m   v1.27.3-eks-a5565ad   172.31.9.54     3.79.4.254     Amazon Linux 2   5.10.184-175.731.amzn2.x86_64   containerd://1.6.19

❯ kn version
Version:      v1.10.0
Build Date:   2023-04-26T07:22:44Z
Git Revision: 46dbf661
Supported APIs:
* Serving
  - serving.knative.dev/v1 (knative-serving v1.10.0)
* Eventing
  - sources.knative.dev/v1 (knative-eventing v1.10.0)
  - eventing.knative.dev/v1 (knative-eventing v1.10.0)
  
 ❯ tkn version
Client version: 0.31.1
Pipeline version: v0.49.0

❯ g fetch origin pull/1857/head:PR-1857
❯ g switch PR-1857
❯ make build
❯ ./func version
v0.37.0-105-g0d386b95
❯ ./func create -l node func-node-1-remote-pr-1857
Created node function in /Users/adam/Development/Workspace/Knative/projects/func/func-node-1-remote-pr-1857
❯ ./func deploy -v --remote -p func-node-1-remote-pr-1857
Creating Pipeline resources
Running Pipeline Task: Building function image on the cluster
Running Pipeline Task: Deploying function to the cluster
✅ Function deployed in namespace "knative-func" and exposed at URL:
   http://func-node-1-remote-pr-1857.knative-func.3.121.145.23.sslip.io

It would be good to have this version released asap as the current version does not work:

❯ f version -v
Version: v0.37.0-6-gf38444bf
Knative: v1.10.0
Commit: f38444bf
SocatImage: quay.io/boson/alpine-socat:1.7.4.3-r1-non-root
TarImage: quay.io/boson/alpine-socat:1.7.4.3-r1-non-root
❯ f deploy -v --remote -p func-node-1-remote-pr-1857
Creating Pipeline resources
W0723 21:55:39.924577   27269 reflector.go:347] github.com/tektoncd/pipeline/pkg/client/informers/externalversions/factory.go:117: watch of *v1.PipelineRun ended with: an error on the server ("unable to decode an event from the watch stream: unable to decode watch event: no kind \"PipelineRun\" is registered for version \"tekton.dev/v1\" in scheme \"github.com/tektoncd/pipeline/pkg/client/clientset/versioned/scheme/register.go:32\"") has prevented the request from succeeding
...

matejvasek · 2023-07-24T12:29:09Z

@grafvonb thaks for checking. So this PR is good to go, right?

matejvasek · 2023-07-24T12:29:54Z

@lance @lkingland @jrangelramos PTAL

grafvonb · 2023-07-24T12:41:05Z

It would be good to have this version released asap as the current version does not work:

See also #1872

matejvasek · 2023-07-24T12:44:04Z

@grafvonb there should be release this week.

matejvasek · 2023-07-24T19:27:52Z

It would be good to have this version released asap as the current version does not work:

See also #1872

@grafvonb and it works all right on main, right?

grafvonb · 2023-07-24T20:04:34Z

@grafvonb and it works all right on main, right?

@matejvasek Yeah, it works. To be 100% I've just built the current state of main:

❯ g log --oneline -1
8b46151b (HEAD -> main, origin/main, origin/HEAD) fix: resolve tkn task spec locally (#1885)
❯ ./func version -v
Version: v0.38.0-4-g8b46151b
Knative: v1.10.0
Commit: 8b46151b
SocatImage: quay.io/boson/alpine-socat:1.7.4.3-r1-non-root
TarImage: quay.io/boson/alpine-socat:1.7.4.3-r1-non-root
❯ ./func deploy -v --remote -p func-node-1-remote-main
Creating Pipeline resources
Running Pipeline Task: Building function image on the cluster
Running Pipeline Task: Deploying function to the cluster
✅ Function deployed in namespace "knative-func" and exposed at URL:
   http://func-node-1-remote-main.knative-func.3.121.145.23.sslip.io

lance

/lgtm

This reverts commit cb6f33d.

* Revert "src: Use jobs not plain pods for auxiliary tasks (#1857)" This reverts commit cb6f33d. * refactor: move code from openshift This is needed to avoid circular package dependencies. Signed-off-by: Matej Vasek <mvasek@redhat.com> * fix: set pod SC only on non-OpenShift Signed-off-by: Matej Vasek <mvasek@redhat.com> --------- Signed-off-by: Matej Vasek <mvasek@redhat.com>

src: Use jobs not plain pods for auxiliary tasks

517b38e

Job should have security context set properly automatically. Signed-off-by: Matej Vasek <mvasek@redhat.com>

matejvasek requested review from lance and jrangelramos July 11, 2023 17:58

knative-prow bot added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Jul 11, 2023

knative-prow bot requested review from maximilien and rhuss July 11, 2023 17:59

knative-prow bot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. approved Indicates a PR has been approved by an approver from all required OWNERS files. labels Jul 11, 2023

matejvasek requested review from zroubalik and lkingland and removed request for maximilien and rhuss July 11, 2023 18:04

fixup: remove unused code

0d386b9

Signed-off-by: Matej Vasek <mvasek@redhat.com>

lance added this to the Release 1.11 milestone Jul 12, 2023

matejvasek marked this pull request as ready for review July 24, 2023 12:29

knative-prow bot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Jul 24, 2023

knative-prow bot requested review from nainaz and vyasgun July 24, 2023 12:29

matejvasek requested review from salaboy and aslom and removed request for nainaz and vyasgun July 24, 2023 13:55

lance reviewed Jul 24, 2023

View reviewed changes

knative-prow bot assigned lance Jul 24, 2023

knative-prow bot added the lgtm Indicates that a PR is ready to be merged. label Jul 24, 2023

knative-prow bot merged commit cb6f33d into knative:main Jul 24, 2023
39 checks passed

grafvonb added a commit to NTTDATA-DACH/kn-plugin-func that referenced this pull request Jul 25, 2023

fix: add pod security context to avoid permission denied (knative#1857)

de25016

matejvasek added a commit to matejvasek/faas that referenced this pull request Jul 26, 2023

Revert "src: Use jobs not plain pods for auxiliary tasks (knative#1857)"

21bfa01

This reverts commit cb6f33d.

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

src: Use jobs not plain pods for auxiliary tasks #1857

src: Use jobs not plain pods for auxiliary tasks #1857

matejvasek commented Jul 11, 2023

knative-prow bot commented Jul 11, 2023

knative-prow bot commented Jul 11, 2023

matejvasek commented Jul 11, 2023

codecov bot commented Jul 11, 2023 •

edited

Loading

matejvasek commented Jul 11, 2023

grafvonb commented Jul 12, 2023

matejvasek commented Jul 12, 2023 •

edited

Loading

matejvasek commented Jul 12, 2023 •

edited

Loading

matejvasek commented Jul 17, 2023 •

edited

Loading

grafvonb commented Jul 20, 2023

grafvonb commented Jul 23, 2023

matejvasek commented Jul 24, 2023

matejvasek commented Jul 24, 2023

grafvonb commented Jul 24, 2023

matejvasek commented Jul 24, 2023

matejvasek commented Jul 24, 2023

grafvonb commented Jul 24, 2023 •

edited

Loading

lance left a comment

src: Use jobs not plain pods for auxiliary tasks #1857

src: Use jobs not plain pods for auxiliary tasks #1857

Conversation

matejvasek commented Jul 11, 2023

Changes

knative-prow bot commented Jul 11, 2023

knative-prow bot commented Jul 11, 2023

matejvasek commented Jul 11, 2023

codecov bot commented Jul 11, 2023 • edited Loading

Codecov Report

matejvasek commented Jul 11, 2023

grafvonb commented Jul 12, 2023

matejvasek commented Jul 12, 2023 • edited Loading

matejvasek commented Jul 12, 2023 • edited Loading

matejvasek commented Jul 17, 2023 • edited Loading

grafvonb commented Jul 20, 2023

grafvonb commented Jul 23, 2023

matejvasek commented Jul 24, 2023

matejvasek commented Jul 24, 2023

grafvonb commented Jul 24, 2023

matejvasek commented Jul 24, 2023

matejvasek commented Jul 24, 2023

grafvonb commented Jul 24, 2023 • edited Loading

lance left a comment

Choose a reason for hiding this comment

codecov bot commented Jul 11, 2023 •

edited

Loading

matejvasek commented Jul 12, 2023 •

edited

Loading

matejvasek commented Jul 12, 2023 •

edited

Loading

matejvasek commented Jul 17, 2023 •

edited

Loading

grafvonb commented Jul 24, 2023 •

edited

Loading