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

fix: target option to disable extra identity defaulting in transport tool #1223

Closed
wants to merge 3 commits into from

Conversation

mandelsoft
Copy link
Contributor

@mandelsoft mandelsoft commented Jan 6, 2025

What this PR does / why we need it

The rework of the extra identity handling unfortunately also changed the transport logic.

Setting a (re)source for a new component version now defaults the version attribute of the extra identity if it is required
to make the identity unique. This was the first step towards getting rid of this implicit identity creation incorporating
the resource's version attribute into the effective extra identity. It makes this explicit, if the version is required as part of the identity of an element, it should be explicitly set for the extra identity in the future.

This defaulting has been made explicit by manipulating the extra identity when adding a new (re)resource. It was enabled by the modification option, which basically allows to do signature relevant changes.

Unfortunately this option is also used by the transport tool, after checking the delta to compose the target version.
Therefore, the transport now explicitly defaults the version into the extra identity, which causes the signature to break.

The solution is disable this defaulting for the transport. Therefore the methods accept a new option now (DisableExtraIdentityDefaulting), which tells the add handling to omit the defaulting and take the meta data as presented.

Which issue(s) this PR fixes

Does not default extra Identity when using ocm for transfer

@jakobmoellerdev
Copy link
Contributor

While this PR is important to fix newly transferred resources, existing component versions that had this defaulting happen need to have the same hash in v1 & v2 as they otherwise would break signature calculation. This is why v3 is necessary in addition to this defaulting change which was forgotten in the original PR for defaulting.

@jakobmoellerdev jakobmoellerdev changed the title target option to disable extra identity defaulting in transport tool fix: target option to disable extra identity defaulting in transport tool Jan 6, 2025
@github-actions github-actions bot added the kind/bugfix Bug label Jan 6, 2025
Copy link
Contributor

@jakobmoellerdev jakobmoellerdev left a comment

Choose a reason for hiding this comment

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

This fix looks mostly good, just the variable name is problematic for me.

@jakobmoellerdev
Copy link
Contributor

I checked current main out and cannot actually reproduce the extra identity defaulting happening during transport. I dont know how you did it, but I couldnt replicate the apparent fixable behavior.

If I transfer

component:
  componentReferences: []
  creationTime: "2025-01-07T11:35:56Z"
  name: github.com/jakobmoellerdev/hash
  provider: github.com/jakobmoellerdev
  repositoryContexts: []
  resources:
  - access:
      imageReference: ghcr.io/stefanprodan/podinfo:6.7.1
      type: ociArtifact
    name: podinfo
    extraIdentity: {}
    relation: external
    type: ociImage
    version: 6.7.1
  - access:
      imageReference: ghcr.io/stefanprodan/podinfo:6.3.1
      type: ociArtifact
    name: podinfo
    extraIdentity: {}
    relation: external
    type: ociImage
    version: 6.3.1
  sources: []
  version: 1.0.0
meta:
  schemaVersion: v2

from e.g. a component archive to a OCI registry, I cannot see any extra identity default:

crane blob ghcr.io/jakobmoellerdev/hashtest/component-descriptors/github.com/jakobmoellerdev/hash:1.0.0@sha256:08f3ad3809cd4a7f33997d52cb916c62255f0d05ecc69ee9682debb8c5b8436e
component-descriptor.yaml0000000000000000000000000000130214163714600014266 0ustar0000000000000000component:
  componentReferences: []
  creationTime: "2025-01-07T11:35:56Z"
  name: github.com/jakobmoellerdev/hash
  provider: github.com/jakobmoellerdev
  repositoryContexts:
  - baseUrl: ghcr.io
    componentNameMapping: urlPath
    subPath: jakobmoellerdev/hashtest
    type: OCIRegistry
  resources:
  - access:
      imageReference: ghcr.io/stefanprodan/podinfo:6.7.1
      type: ociArtifact
    name: podinfo
    relation: external
    type: ociImage
    version: 6.7.1
  - access:
      imageReference: ghcr.io/stefanprodan/podinfo:6.3.1
      type: ociArtifact
    name: podinfo
    relation: external
    type: ociImage
    version: 6.3.1
  sources: []
  version: 1.0.0
meta:
  schemaVersion: v2

How is this additional case triggered?

@mandelsoft
Copy link
Contributor Author

I did the following:

$ at comp-impl.yaml 

name: github.com/mandelsoft/test1
version: 1.0.0
provider:
  name: mandelsoft.org
resources:
  - name: multi
    type: plainText
    version: v1
    input:
      type: utf8
      text: test data
  - name: multi
    type: plainText
    version: v2
    input:
      type: utf8
      text: extended test data
$ ocm.v18 add cv -c -F ctf-impl comp-impl.yaml

processing comp-impl.yaml...
  processing document 1...
    processing index 1
found 1 component
adding component github.com/mandelsoft/test1:1.0.0...
  adding resource plainText: "name"="multi","version"="v1"...
  adding resource plainText: "name"="multi","version"="v2"...
$ ../../tmp/ocm.v18 get cv ctf-impl -o yaml

---
component:
  componentReferences: []
  creationTime: "2025-01-08T10:46:33Z"
  name: github.com/mandelsoft/test1
  provider: mandelsoft.org
  repositoryContexts: []
  resources:
  - access:
      localReference: sha256:916f0027a575074ce72a331777c3478d6513f786a591bd892da1a577bf2335f9
      mediaType: application/octet-stream
      type: localBlob
    digest:
      hashAlgorithm: SHA-256
      normalisationAlgorithm: genericBlobDigest/v1
      value: 916f0027a575074ce72a331777c3478d6513f786a591bd892da1a577bf2335f9
    name: multi
    relation: local
    type: plainText
    version: v1
  - access:
      localReference: sha256:920ce99fb13b43ca0408caee6e61f6335ea5156d79aa98e733e1ed2393e0f649
      mediaType: application/octet-stream
      type: localBlob
    digest:
      hashAlgorithm: SHA-256
      normalisationAlgorithm: genericBlobDigest/v1
      value: 920ce99fb13b43ca0408caee6e61f6335ea5156d79aa98e733e1ed2393e0f649
    name: multi
    relation: local
    type: plainText
    version: v2
  sources: []
  version: 1.0.0
meta:
  schemaVersion: v2

Because v18 was used to create the ctf, there is no explicit extra identity.

$ git fetch ocm main
$ git checkout ocm/main
$ make install

$ ocm transfer cv ctf-impl tgt-impl
transferring version "github.com/mandelsoft/test1:1.0.0"...
...resource 0 multi[plainText]...
...resource 1 multi[plainText]...
...adding component version...
1 versions transferred

This already uses the PR you unfortunately already merged.

$ ocm get cv tgt-impl -o yaml

---
component:
  componentReferences: []
  creationTime: "2025-01-08T10:46:33Z"
  name: github.com/mandelsoft/test1
  provider: mandelsoft.org
  repositoryContexts: []
  resources:
  - access:
      localReference: sha256:916f0027a575074ce72a331777c3478d6513f786a591bd892da1a577bf2335f9
      mediaType: application/octet-stream
      type: localBlob
    digest:
      hashAlgorithm: SHA-256
      normalisationAlgorithm: genericBlobDigest/v1
      value: 916f0027a575074ce72a331777c3478d6513f786a591bd892da1a577bf2335f9
    extraIdentity:
      version: v1
    name: multi
    relation: local
    type: plainText
    version: v1
  - access:
      localReference: sha256:920ce99fb13b43ca0408caee6e61f6335ea5156d79aa98e733e1ed2393e0f649
      mediaType: application/octet-stream
      type: localBlob
    digest:
      hashAlgorithm: SHA-256
      normalisationAlgorithm: genericBlobDigest/v1
      value: 920ce99fb13b43ca0408caee6e61f6335ea5156d79aa98e733e1ed2393e0f649
    extraIdentity:
      version: v2
    name: multi
    relation: local
    type: plainText
    version: v2
  sources: []
  version: 1.0.0
meta:
  schemaVersion: v2

As you can see, as expected, there is the extra identity, now.

You PR does not fix anything related to the root cause.

@jakobmoellerdev
Copy link
Contributor

Im starting to realize this is a different bug now. Even though you are correct with the identity defaulting on this specific component, the component version that was reported in the bug looks different after transport

    - access:
        imageReference: ...
      digest: null
      extraIdentity: {}
      labels: ...
      name: nginx-ingress-controller
      relation: external
      srcRefs: []
      type: ociImage
      version: v1.11.3
    - access:
        imageReference: ...
        type: ociRegistry
      digest: null
      extraIdentity: {}
      labels: ...
      name: nginx-ingress-controller
      relation: external
      srcRefs: []
      type: ociImage
      version: v1.9.6

I think you are fixing a bug, but not a bug thats related to the report.

jakobmoellerdev
jakobmoellerdev previously approved these changes Jan 8, 2025
Copy link
Contributor

@jakobmoellerdev jakobmoellerdev left a comment

Choose a reason for hiding this comment

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

Double checked: The bug cannot be related to transport because the team that reported it didnt use OCM for transfer, thus the bugs are not correlated. Nevertheless, this bug is valid and the fix here should be merged

@mandelsoft
Copy link
Contributor Author

Yes, that is what I also discovered yesterday, there are indeed two bugs, I added this to the issue. But both bugs are not related to the normalization, but the to the defaulting of the version for the extraIdentity.

The one reported in the issue, was just uncovered by v19 but was in the code since a very long time.

@jakobmoellerdev
Copy link
Contributor

closing in favor of #1236

jakobmoellerdev added a commit that referenced this pull request Jan 9, 2025
…1236)

<!-- markdownlint-disable MD041 -->
#### What this PR does / why we need it

replaces #1223 as it
does not contain the revert for the normalization algorithm

Introduces DisableExtraIdentityDefaulting which is enabled by default
during transport because IsModifyElement is not enough to differentiate
if the component version is allowed to be modified during transfer.

#### Which issue(s) this PR fixes

PR that tackles a case in which `ocm transfer cv` caused accidental
defaults to the extra identity.

---------

Co-authored-by: Uwe Krueger <[email protected]>
jakobmoellerdev added a commit to jakobmoellerdev/ocm that referenced this pull request Jan 9, 2025
…pen-component-model#1236)

<!-- markdownlint-disable MD041 -->
#### What this PR does / why we need it

replaces open-component-model#1223 as it
does not contain the revert for the normalization algorithm

Introduces DisableExtraIdentityDefaulting which is enabled by default
during transport because IsModifyElement is not enough to differentiate
if the component version is allowed to be modified during transfer.

#### Which issue(s) this PR fixes

PR that tackles a case in which `ocm transfer cv` caused accidental
defaults to the extra identity.

---------

Co-authored-by: Uwe Krueger <[email protected]>
jakobmoellerdev added a commit to jakobmoellerdev/ocm that referenced this pull request Jan 9, 2025
…pen-component-model#1236)

<!-- markdownlint-disable MD041 -->
#### What this PR does / why we need it

replaces open-component-model#1223 as it
does not contain the revert for the normalization algorithm

Introduces DisableExtraIdentityDefaulting which is enabled by default
during transport because IsModifyElement is not enough to differentiate
if the component version is allowed to be modified during transfer.

#### Which issue(s) this PR fixes

PR that tackles a case in which `ocm transfer cv` caused accidental
defaults to the extra identity.

---------

Co-authored-by: Uwe Krueger <[email protected]>
ikhandamirov pushed a commit that referenced this pull request Jan 9, 2025
…1236) (#1237)

<!-- markdownlint-disable MD041 -->
#### What this PR does / why we need it

replaces #1223 as it
does not contain the revert for the normalization algorithm

Introduces DisableExtraIdentityDefaulting which is enabled by default
during transport because IsModifyElement is not enough to differentiate
if the component version is allowed to be modified during transfer.

#### Which issue(s) this PR fixes

PR that tackles a case in which `ocm transfer cv` caused accidental
defaults to the extra identity.

---------

<!-- markdownlint-disable MD041 -->
#### What this PR does / why we need it

Backports this change to v0.19 as it occurred there first

#### Which issue(s) this PR fixes
<!--
Usage: `Fixes #<issue number>`, or `Fixes (paste link of issue)`.
-->

Ensures Transports also dont default with 0.19.z

Co-authored-by: Uwe Krueger <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants