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

lock: update lock change resolution to fix mutating image collision #337

Merged
merged 1 commit into from
Aug 2, 2024

Conversation

jmt-lab
Copy link
Contributor

@jmt-lab jmt-lab commented Jul 26, 2024

Description of changes:

  • Changes lock change resolution to no longer only rely on the Twoliter.toml digest. Instead it will rerun the resolution and compare the result in order to verify that the remote images haven't mutated
  • Adds --update flag to twoliter fetch to allow for performing an update at fetch time
  • Removes release-version and root digest from Twoliter.toml
  • Consolidates the constructors

Testing done:

Tested that Twoliter.lock collision still correctly occurs including remote digest change.

Terms of contribution:

By submitting this pull request, I agree that this contribution is dual-licensed under the terms of both the Apache License, version 2.0, and the MIT license.

@jmt-lab jmt-lab requested review from bcressey and cbgbt July 26, 2024 20:54
@jmt-lab jmt-lab self-assigned this Jul 26, 2024
@jmt-lab jmt-lab force-pushed the lock-resolution-fix branch 4 times, most recently from 2f7b23d to 55855c6 Compare July 26, 2024 22:34
Comment on lines +42 to +45
fn eq(&self, other: &Self) -> bool {
self.source == other.source && self.digest == other.digest
}
}
Copy link
Contributor

Choose a reason for hiding this comment

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

I think the image digest should be sufficient for equivalency, no?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The source could change with vendor but the digest could theoretically be the same

@jmt-lab jmt-lab requested a review from sam-berning July 29, 2024 20:42
twoliter/src/lock.rs Outdated Show resolved Hide resolved
pub(crate) async fn create(project: &Project) -> Result<Self> {
let lock_file_path = project.project_dir().join(TWOLITER_LOCK);
if lock_file_path.exists() {
remove_file(&lock_file_path).await?;
Copy link
Contributor

Choose a reason for hiding this comment

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

do we need to remove the existing lockfile when create == true?

Copy link
Contributor

@cbgbt cbgbt left a comment

Choose a reason for hiding this comment

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

Ah, I just noticed I never clicked "submit" on this review. Sorry!

Comment on lines 265 to 266
pub schema_version: SchemaVersion<1>,
/// The workspace release version
pub release_version: String,
/// The resolved bottlerocket sdk
Copy link
Contributor

Choose a reason for hiding this comment

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

Does this require a SchemaVersion change?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

No not really the schema version here is more to track the schema version of Twoliter.toml

Copy link
Contributor

Choose a reason for hiding this comment

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

I'm imagining that it could also be a problem if you update twoliter and the lockfile no longer matches, despite the contents being the same. I believe that shouldn't be the case here, though.

twoliter/src/lock.rs Outdated Show resolved Hide resolved
twoliter/src/lock.rs Outdated Show resolved Hide resolved
twoliter/src/lock.rs Outdated Show resolved Hide resolved
@jmt-lab jmt-lab merged commit 6b086df into bottlerocket-os:develop Aug 2, 2024
1 check passed
vigh-m added a commit to vigh-m/bottlerocket-core-kit that referenced this pull request Aug 26, 2024
@jmt-lab jmt-lab deleted the lock-resolution-fix branch September 6, 2024 17:07
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