Skip to content

Fix yum repo cleanup#17334

Merged
wadells merged 4 commits into
masterfrom
walt/fix-yum-publish
Oct 12, 2022
Merged

Fix yum repo cleanup#17334
wadells merged 4 commits into
masterfrom
walt/fix-yum-publish

Conversation

@wadells
Copy link
Copy Markdown
Contributor

@wadells wadells commented Oct 12, 2022

Previously, "${ARTIFACT_PATH}" was interpreted as Drone variable subsitution, resulting in "rm -rf ${ARTIFACT_PATH}/*" becoming "rm -rf /*", which deleted credentials on the filesystem. This causes the error seen here:

+ rm -rf "/*"
+ aws s3 sync --no-progress --delete --exclude "*" --include "*.rpm*" s3://$AWS_S3_BUCKET/teleport/tag/10.3.2/ "$ARTIFACT_PATH"
fatal error: An error occurred (AuthorizationHeaderMalformed) when calling the ListObjectsV2 operation: The authorization header is malformed; a non-empty Access Key (AKID) must be provided in the credential.

https://drone.platform.teleport.sh/gravitational/teleport/16371/2/5

This has been latent in the publishing logic for since https://github.com/gravitational/teleport/pull/14203/files#diff-b54b39f1afced2465e1f3641db9d5bbf4f3a7fcf890996dfedd3c197bcb7f8c7R5472, but it happened to not matter because we weren't storing anything important in the container's filesystem. I do wonder how the aws command was still available following the delete.

Backports:

Testing Done

None -- a development build won't ever hit these codepaths. I'm happy to take suggestions of how we might test it.

@wadells wadells requested review from fheinecke and r0mant October 12, 2022 15:53
@wadells wadells marked this pull request as ready for review October 12, 2022 15:53
@github-actions github-actions Bot requested a review from klizhentas October 12, 2022 15:53
Previously, "${ARTIFACT_PATH}" was interpreted as Drone variable
subsitution, resulting in "rm -rf ${ARTIFACT_PATH}/*" becoming
"rm -rf /*", which deleted credentials on the filesystem.
@wadells wadells force-pushed the walt/fix-yum-publish branch from 16a942d to 94bee39 Compare October 12, 2022 15:59
Copy link
Copy Markdown
Collaborator

@zmb3 zmb3 left a comment

Choose a reason for hiding this comment

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

😱

@fheinecke
Copy link
Copy Markdown
Contributor

Yea I definitely missed that on my initial implementation in the linked PR

I would recommend just doing $$ instead of removing braces. If I remember right Drone will still interpret $VAR the same as it would interpret ${VAR}. If you look at dronegen/promote.go that's how var escaping is handled.

Regarding testing... the only real way to test it is to inject "testing code" (like different secret names) that point to AWS resources in the dev account and then update the trigger to execute on branch push. Then revert those testing changes before merge. Very messy but it's the best way I'm aware of.

@wadells
Copy link
Copy Markdown
Contributor Author

wadells commented Oct 12, 2022

I would recommend just doing $$ instead of removing braces. If I remember right Drone will still interpret $VAR the same as it would interpret ${VAR}. If you look at dronegen/promote.go that's how var escaping is handled.

$ARTIFACT_PATH is what is used in the rest of the pipeline. See the mkdir on the line immediately above or the s3 command on the line after. I can use $$ if you prefer, but it'll be inconsistent with the other usages.

Regarding testing... the only real way to test it is to inject "testing code" (like different secret names) that point to AWS resources in the dev account and then update the trigger to execute on branch push. Then revert those testing changes before merge. Very messy but it's the best way I'm aware of.

Do you want me to test this? I can do so. I don't feel it is needed, but that is also how I got into this situation 😆 😭

@fheinecke
Copy link
Copy Markdown
Contributor

$ARTIFACT_PATH is what is used in the rest of the pipeline. See the mkdir on the line immediately above or the s3 command on the line after. I can use $$ if you prefer, but it'll be inconsistent with the other usages.

Sorry yes you are correct. After reviewing the docs here it looks like $ARTIFACT_PATH and $${ARTIFACT_PATH} will be executed as expected, where ${ARTIFACT_PATH} will trigger Drone's substitution. Your changes look good to me as is.

Do you want me to test this?

I think it's relatively safe to assume that this will work and fix the problem as expected. Quite frankly I think it'd take longer to test than it would to open/merge another PR with a follow-up fix should something go wrong during migration of the latest release.

wadells added a commit that referenced this pull request Oct 12, 2022
This helps test a couple more changes from this pipeline when cutting a
dev build.  Particularly, we saw the download and role assumption steps
fail in #17334, and this
change would have allowed us to catch that error during testing.
@wadells wadells enabled auto-merge (squash) October 12, 2022 17:17
@wadells wadells merged commit 5e5a323 into master Oct 12, 2022
@wadells wadells deleted the walt/fix-yum-publish branch October 12, 2022 19:06
wadells added a commit that referenced this pull request Oct 21, 2022
This helps test a couple more changes from this pipeline when cutting a
dev build.  Particularly, we saw the download and role assumption steps
fail in #17334, and this
change would have allowed us to catch that error during testing.
wadells added a commit that referenced this pull request Oct 21, 2022
This helps test a couple more changes from this pipeline when cutting a
dev build.  Particularly, we saw the download and role assumption steps
fail in #17334, and this
change would have allowed us to catch that error during testing.
wadells added a commit that referenced this pull request Oct 21, 2022
This helps test a couple more changes from this pipeline when cutting a
dev build.  Particularly, we saw the download and role assumption steps
fail in #17334, and this
change would have allowed us to catch that error during testing.
wadells added a commit that referenced this pull request Oct 21, 2022
This helps test a couple more changes from this pipeline when cutting a
dev build.  Particularly, we saw the download and role assumption steps
fail in #17334, and this
change would have allowed us to catch that error during testing.
wadells added a commit that referenced this pull request Oct 21, 2022
This helps test a couple more changes from this pipeline when cutting a
dev build.  Particularly, we saw the download and role assumption steps
fail in #17334, and this
change would have allowed us to catch that error during testing.
wadells added a commit that referenced this pull request Oct 21, 2022
This helps test a couple more changes from this pipeline when cutting a
dev build.  Particularly, we saw the download and role assumption steps
fail in #17334, and this
change would have allowed us to catch that error during testing.
wadells added a commit that referenced this pull request Oct 21, 2022
This helps test a couple more changes from this pipeline when cutting a
dev build.  Particularly, we saw the download and role assumption steps
fail in #17334, and this
change would have allowed us to catch that error during testing.
wadells added a commit that referenced this pull request Oct 21, 2022
* Serialize apt/yum promote pipelines

These were running in parallel, but we want them to run serially.
Therefore, we add a dependency between each step and its previous step.

* Allow dev build promotes to proceed in deb/rpm pipelines

This helps test a couple more changes from this pipeline when cutting a
dev build.  Particularly, we saw the download and role assumption steps
fail in #17334, and this
change would have allowed us to catch that error during testing.

* Fix globbing bug

This bug does not appear to affect anything currently.  However it
should be fixed in case the rm is important at some point in the future.

The bug is: when a wildcard is inside quotes, it is treated as a literal
filename.  So rm -rf "$ARTIFACT_PATH/*" tries to remove the file named
'*' instead of trying to remove everything in artifact path.

* Swap YUM_REPO_NEW_ROLE to YUM_REPO_NEW_AWS_ROLE

All other roles environment variables end in AWS_ROLE, and consistency
is our friend here.
wadells added a commit that referenced this pull request Oct 21, 2022
* Serialize apt/yum promote pipelines

These were running in parallel, but we want them to run serially.
Therefore, we add a dependency between each step and its previous step.

* Allow dev build promotes to proceed in deb/rpm pipelines

This helps test a couple more changes from this pipeline when cutting a
dev build.  Particularly, we saw the download and role assumption steps
fail in #17334, and this
change would have allowed us to catch that error during testing.

* Fix globbing bug

This bug does not appear to affect anything currently.  However it
should be fixed in case the rm is important at some point in the future.

The bug is: when a wildcard is inside quotes, it is treated as a literal
filename.  So rm -rf "$ARTIFACT_PATH/*" tries to remove the file named
'*' instead of trying to remove everything in artifact path.

* Swap YUM_REPO_NEW_ROLE to YUM_REPO_NEW_AWS_ROLE

All other roles environment variables end in AWS_ROLE, and consistency
is our friend here.
wadells added a commit that referenced this pull request Oct 21, 2022
* Serialize apt/yum promote pipelines

These were running in parallel, but we want them to run serially.
Therefore, we add a dependency between each step and its previous step.

* Allow dev build promotes to proceed in deb/rpm pipelines

This helps test a couple more changes from this pipeline when cutting a
dev build.  Particularly, we saw the download and role assumption steps
fail in #17334, and this
change would have allowed us to catch that error during testing.

* Fix globbing bug

This bug does not appear to affect anything currently.  However it
should be fixed in case the rm is important at some point in the future.

The bug is: when a wildcard is inside quotes, it is treated as a literal
filename.  So rm -rf "$ARTIFACT_PATH/*" tries to remove the file named
'*' instead of trying to remove everything in artifact path.
wadells added a commit that referenced this pull request Oct 21, 2022
* Serialize apt/yum promote pipelines

These were running in parallel, but we want them to run serially.
Therefore, we add a dependency between each step and its previous step.

* Allow dev build promotes to proceed in deb/rpm pipelines

This helps test a couple more changes from this pipeline when cutting a
dev build.  Particularly, we saw the download and role assumption steps
fail in #17334, and this
change would have allowed us to catch that error during testing.

* Fix globbing bug

This bug does not appear to affect anything currently.  However it
should be fixed in case the rm is important at some point in the future.

The bug is: when a wildcard is inside quotes, it is treated as a literal
filename.  So rm -rf "$ARTIFACT_PATH/*" tries to remove the file named
'*' instead of trying to remove everything in artifact path.
wadells added a commit that referenced this pull request Oct 21, 2022
* Serialize apt/yum promote pipelines

These were running in parallel, but we want them to run serially.
Therefore, we add a dependency between each step and its previous step.

* Allow dev build promotes to proceed in deb/rpm pipelines

This helps test a couple more changes from this pipeline when cutting a
dev build.  Particularly, we saw the download and role assumption steps
fail in #17334, and this
change would have allowed us to catch that error during testing.

* Fix globbing bug

This bug does not appear to affect anything currently.  However it
should be fixed in case the rm is important at some point in the future.

The bug is: when a wildcard is inside quotes, it is treated as a literal
filename.  So rm -rf "$ARTIFACT_PATH/*" tries to remove the file named
'*' instead of trying to remove everything in artifact path.

* Swap YUM_REPO_NEW_ROLE to YUM_REPO_NEW_AWS_ROLE

All other roles environment variables end in AWS_ROLE, and consistency
is our friend here.
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.

3 participants