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 EC checks for StepActions #1640

Merged
merged 2 commits into from
Dec 2, 2024
Merged

Add EC checks for StepActions #1640

merged 2 commits into from
Dec 2, 2024

Conversation

lcarva
Copy link
Contributor

@lcarva lcarva commented Nov 22, 2024

Ref: https://issues.redhat.com/browse/EC-1010

Before you complete this pull request ...

Look for any open pull requests in the repository with the title "e2e-tests update" and
see if there are recent e2e-tests updates that will be applicable to your change.

@lcarva
Copy link
Contributor Author

lcarva commented Nov 22, 2024

Draft until enterprise-contract/ec-policies#1232 is merged.

@lcarva lcarva force-pushed the EC-1010 branch 3 times, most recently from fab55c6 to 18437f6 Compare November 22, 2024 20:20
policies/step-actions.yaml Outdated Show resolved Hide resolved
@lcarva lcarva force-pushed the EC-1010 branch 3 times, most recently from 9cf5335 to 6dc26fe Compare November 25, 2024 17:01
@lcarva lcarva marked this pull request as ready for review November 25, 2024 17:01
@openshift-ci openshift-ci bot requested review from tkdchen and tnevrlka November 25, 2024 17:01
@tnevrlka
Copy link
Contributor

/retest

.tekton/tasks/ec-checks.yaml Outdated Show resolved Hide resolved
Copy link
Contributor

@chmeliik chmeliik left a comment

Choose a reason for hiding this comment

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

/tekton/scripts/script-3-swb4p: line 11: find: command not found
[DEBUG] Files parameter: 
Error: required flag(s) "file" not set

The ec-cli image doesn't have find, so something like Zoran's suggestion will be needed

@lcarva lcarva force-pushed the EC-1010 branch 3 times, most recently from 9929789 to cdec316 Compare November 27, 2024 16:27
@@ -18,9 +18,14 @@ spec:
# the cluster will set imagePullPolicy to IfNotPresent
workingDir: $(workspaces.source.path)/source
script: |
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Extra changes needed to make checkton happy.

@lcarva
Copy link
Contributor Author

lcarva commented Nov 27, 2024

/retest

Trying to pull registry.access.redhat.com/ubi9/ubi:latest...
error creating build container: Source image rejected: Error reading signature from https://access.redhat.com/webassets/docker/content/sigstore/ubi9/ubi@sha256=9ac75c1a392429b4a087971cdf9190ec42a854a169b6835bc9e25eecaf851258/signature-7: status 500 (Internal Server Error)
time="2024-11-27T16:28:41Z" level=error msg="exit status 125"

@lcarva lcarva marked this pull request as draft November 27, 2024 16:47
@lcarva lcarva marked this pull request as ready for review November 27, 2024 18:50
@openshift-ci openshift-ci bot requested review from chmeliik and rcerven November 27, 2024 18:50
@lcarva
Copy link
Contributor Author

lcarva commented Nov 27, 2024

/retest

@lcarva
Copy link
Contributor Author

lcarva commented Nov 27, 2024

Hmm EC reporting a failure for stepactions-ec/0.1-eaas-get-ephemeral-cluster-credentials.yaml.

  - metadata:
      code: stepaction.image.accessible
    msg: Image ref "registry.redhat.io/openshift4/ose-cli@sha256:15da03b04318bcc842060b71e9dd6d6c2595edb4e8fdd11b0c6781eeb03ca182"
      is inaccessible

It requires authentication which we can't do from a PR. How important is the image accessible check? 🤔

UPDATE: Ah! Need a new version of the ec-clli that understands the "severity" result metadata.
UPDATE2: Nope. That's not it. (Got mislead by a local bogus version of EC I was hacking on.)

@lcarva
Copy link
Contributor Author

lcarva commented Nov 27, 2024

There is a similar check for image accessibility for Task definitions which should be triggering a violation here (no secret to access that image from a pull request). But EC is currently passing due to a bug enterprise-contract/ec-policies#1237.

Not sure how to proceed here.

@lcarva
Copy link
Contributor Author

lcarva commented Nov 27, 2024

Because the check happens in Konflux, there is probably a way to pipe the secrets to the ec-check Task.

@lcarva lcarva force-pushed the EC-1010 branch 10 times, most recently from ad93d5e to 9be9717 Compare November 27, 2024 21:18
@lcarva
Copy link
Contributor Author

lcarva commented Nov 27, 2024

Ok. This seems to be sorted out now. Please have another looks 🙏

# in the namespace, but that is currently disabled so it's wild west.
runAsUser: 0
env:
- name: HOME
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is needed so EC finds the authfile 🤷
And it also, for some reason, makes runAsUser required. Quite odd.

@chmeliik
Copy link
Contributor

There is a similar check for image accessibility for Task definitions which should be triggering a violation here (no secret to access that image from a pull request). But EC is currently passing due to a bug enterprise-contract/ec-policies#1237.

Not sure how to proceed here.

Because the check happens in Konflux, there is probably a way to pipe the secrets to the ec-check Task.

Hmm, maybe we want it to fail. A while back, Gal went around changing all the images to publicly accessible ones for the community version of Konflux.

Maybe add an exception for this rule for now (for the specific tasks/stepactions if possible) and we should gradually move them away from using registry.redhat.io images

Copy link
Member

@zregvart zregvart left a comment

Choose a reason for hiding this comment

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

LGTM, nitpickery inside

hack/ec-checks.sh Outdated Show resolved Hide resolved
fi
local d=$1

while IFS= read -r -d '' f; do
Copy link
Member

Choose a reason for hiding this comment

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

Just by using bash this can be done like:

Suggested change
while IFS= read -r -d '' f; do
shopt -s globstar
for f in stepactions/**/*.yaml; do

hack/ec-checks.sh Outdated Show resolved Hide resolved
@lcarva
Copy link
Contributor Author

lcarva commented Dec 2, 2024

There is a similar check for image accessibility for Task definitions which should be triggering a violation here (no secret to access that image from a pull request). But EC is currently passing due to a bug enterprise-contract/ec-policies#1237.
Not sure how to proceed here.

Because the check happens in Konflux, there is probably a way to pipe the secrets to the ec-check Task.

Hmm, maybe we want it to fail. A while back, Gal went around changing all the images to publicly accessible ones for the community version of Konflux.

Maybe add an exception for this rule for now (for the specific tasks/stepactions if possible) and we should gradually move them away from using registry.redhat.io images

The reason it was failing it's because EC wasn't seeing the registry auth file. That's fixed and things are working as expected.

@lcarva lcarva added this pull request to the merge queue Dec 2, 2024
Merged via the queue into konflux-ci:main with commit 86e02fa Dec 2, 2024
15 checks passed
@lcarva lcarva deleted the EC-1010 branch December 2, 2024 18:11
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.

4 participants