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

logformatter: link to logs using Cirrus API #14608

Merged
merged 1 commit into from
Jun 22, 2022

Conversation

edsantiago
Copy link
Member

@edsantiago edsantiago commented Jun 15, 2022

One day we may use AWS for part of CI. Do you want to maintain
two separate code paths in this script for linking to artifacts
in multiple cloud providers? Can you say no? I knew you could.

Cirrus already knows the location of the artifacts and provides
a transparent mechanism for accessing them. Use it.

This PR exposed a nasty bug in our environment-variable handling:
envariables passed through to the containerized environment were
being double-space-escaped, so "FOO=a b" ended up as "FOO=a\ b"
(with a backslash), with one consequence being invalid URLs.
The solution is simple: run 'podman -e FOO', not '-e FOO=value'.

Finally, reinstate the environment-variable dump (in comments).
I had removed this in a moment of panic over leaking secrets,
but no, that doesn't happen.

See #14569 for discussion.

Signed-off-by: Ed Santiago [email protected]

None

@openshift-ci openshift-ci bot added release-note-none approved Indicates a PR has been approved by an approver from all required OWNERS files. labels Jun 15, 2022
@edsantiago
Copy link
Member Author

Yay, seems to work: see link here: https://cirrus-ci.com/task/4690118979092480?logs=main#L53

@edsantiago
Copy link
Member Author

@cevich this is failing in two cases: the two container tests. E.g. int-podman-f36-root-container. Reason seems to be spurious backslashes before the spaces: int\ podman\ fedora-36\ etc. Root cause is this escape:

# Properly escape values to prevent injection
printf -- "$envname=%q\n" "$envval"

Sure, it's easy for me to fix in logformatter, but I think this might actually be showing us that there's a problem with the variable-escaping mechanism: the variables seen in container environment are subtly different from those in other environments.

@edsantiago edsantiago marked this pull request as draft June 15, 2022 21:25
@openshift-ci openshift-ci bot added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Jun 15, 2022
@edsantiago edsantiago force-pushed the logformatter_new_url branch 9 times, most recently from 77cc07f to 0019b27 Compare June 16, 2022 12:21
@edsantiago edsantiago marked this pull request as ready for review June 16, 2022 12:31
@openshift-ci openshift-ci bot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Jun 16, 2022
@edsantiago
Copy link
Member Author

Followup: this is ready for review. I've confirmed that all the links are correct, via:

$ cirrus-pr-timing --download-logs 14608
Downloading into cirrus-pr-timing.6633932056690688:
...
$ grep -ah '  https://api.cirrus-ci.com/v1/' cirrus-pr-timing.6633932056690688/* | xargs -l1 browser-open

Every one of the links worked. (Yes, I also tested by manually clicking a handful of CI log links, but I have only so much tolerance for tedium)

@cevich
Copy link
Member

cevich commented Jun 16, 2022

I think this might actually be showing us that there's a problem with the variable-escaping mechanism

Ack. I'm not surprised about this. Your fix looks good, and overall this LGTM. Thanks for taking this on.

One day we may use AWS for part of CI. Do you want to maintain
two separate code paths in this script for linking to artifacts
in multiple cloud providers? Can you say no? I knew you could.

Cirrus already knows the location of the artifacts and provides
a transparent mechanism for accessing them. Use it.

This PR exposed a nasty bug in our environment-variable handling:
envariables passed through to the containerized environment were
being double-space-escaped, so "FOO=a b" ended up as "FOO=a\ b"
(with a backslash), with one consequence being invalid URLs.
The solution is simple: run 'podman -e FOO', not '-e FOO=value'.

Finally, reinstate the environment-variable dump (in comments).
I had removed this in a moment of panic over leaking secrets,
but no, that doesn't happen. Exclude scary-sounding vars anyway.

Signed-off-by: Ed Santiago <[email protected]>
@edsantiago edsantiago force-pushed the logformatter_new_url branch from 0019b27 to ef563c5 Compare June 22, 2022 17:27
@edsantiago
Copy link
Member Author

@cevich I've folded in my SECRET_ENV_RE filter and re-pushed, in case that was the blocker for merging this. You can confirm by view-source on this log. TOKEN does not appear in the list, but SECRET does because SECRET is not actually part of SECRET_ENV_RE.

@cevich
Copy link
Member

cevich commented Jun 22, 2022

Oh sorry and thanks for mentioning me. I completely forgot about this. I can't really read perl so I'll just trust that you've got it right.

/lgtm

@openshift-ci openshift-ci bot added the lgtm Indicates that a PR is ready to be merged. label Jun 22, 2022
@openshift-ci
Copy link
Contributor

openshift-ci bot commented Jun 22, 2022

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: cevich, edsantiago

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:

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

@openshift-ci openshift-ci bot merged commit 8e88abd into containers:main Jun 22, 2022
@edsantiago edsantiago deleted the logformatter_new_url branch June 23, 2022 00:08
@github-actions github-actions bot added the locked - please file new issue/PR Assist humans wanting to comment on an old issue or PR with locked comments. label Sep 21, 2023
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Sep 21, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. lgtm Indicates that a PR is ready to be merged. locked - please file new issue/PR Assist humans wanting to comment on an old issue or PR with locked comments. release-note-none
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants