OTA-915: Serve signatures through a new container #794

PratikMahajan · 2023-05-18T22:28:53Z

This PR adds a new container to serve the signatures.

Signatures should be added to the directory in order to
serve the signatures.
The dir structure should look like {ALGO}/{DIGEST}/{SIGNATURE}

Add this directory to toml file in-order to read the signatures.

[signatures]
dir = <DIR-HERE>

In the absence of dir in toml file, it'll create a temp directory and
read from it

wking · 2023-05-18T23:10:31Z

metadata-helper/src/status.rs

+///
+/// Status:
+///  * Ready (200 code): the application has been initialized and is available to accept connections.
+///  * Not Ready (503 code): no JSON graph available yet.


I'm fuzzy on what app_data.is_ready is actually about, but the blocking issue for serving signatures out of this process is whether the local signature filesystem is present. With init-container data sourcing, that's true from before this container comes up. In a future where one of these containers is trying to poll graph-data image from the registry, it would be true after the first successful retrieval and unpacking. Neither of those are directly about the JSON graph (that graph-builder is serving?).

wking · 2023-05-25T01:18:42Z

metadata-helper/src/signatures.rs

Looking at RFC 3986's sections 3.3 and 5.2.4, I don't think we need to worry about .. path segments being used to access locations outside of signatures_data_path, but it's probably worth testing just to make sure.

petr-muller · 2023-05-29T15:23:30Z

Do we plan to address the issues with deps with rust 1.54? If not, should we remove the jobs? If yes, why do we need to support 1.54? :)

petr-muller

I'm very green in reviewing Rust code, so take the comments with a grain of salt and feel free to ignore them, I'm not yet familiar with Rust idioms and accepted practice :)

In general there seems to be quite a lot of duplicate code around the processing of parameters (merging params from cli and config file etc) that stems from how this was apparently created by making a gutted copy of policy-engine. It feels like we could DRY these pieces now when we have two or even three copies of them?

commons/src/lib.rs

metadata-helper/src/signatures.rs

petr-muller · 2023-05-29T16:14:52Z

metadata-helper/src/status.rs

This is a duplicate of policy-engine/src/status.rs, would it make sense to extract these simple probe backends to a helper that would simply take app_data: actix_web::web::Data<AppState> as a parameter?

the struct, though it has same name, the struct is a bit different in the packages, thats the reason for duplicate code

metadata-helper/src/config/cli.rs

metadata-helper/src/config/settings.rs

metadata-helper/src/signatures.rs

PratikMahajan · 2023-06-23T20:35:02Z

/retest

openshift-ci · 2023-06-23T21:34:54Z

@PratikMahajan: The following tests failed, say /retest to rerun all failed tests or /retest-required to rerun all mandatory failed tests:

Test name	Commit	Details	Required	Rerun command
ci/prow/rustfmt-1.58.0	1fcff3cd3cf3f6c210a1535f16f63c1ad015fd8e	link	true	`/test rustfmt-1.58.0`
ci/prow/customrust-rustfmt-1.54.0	1fcff3cd3cf3f6c210a1535f16f63c1ad015fd8e	link	true	`/test customrust-rustfmt-1.54.0`
ci/prow/rustfmt-1.54.0	1fcff3cd3cf3f6c210a1535f16f63c1ad015fd8e	link	true	`/test rustfmt-1.54.0`
ci/prow/customrust-rustfmt-1.58.0	1fcff3cd3cf3f6c210a1535f16f63c1ad015fd8e	link	true	`/test customrust-rustfmt-1.58.0`

Full PR test history. Your PR dashboard.

Details

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. I understand the commands that are listed here.

wking · 2023-06-23T22:44:18Z

metadata-helper/src/config/cli.rs

I'm fuzzy on details, but Status service options for service and Main service options for status seems like the comments may be flipped here?

openshift-ci-robot · 2023-06-26T15:09:06Z

@PratikMahajan: This pull request references OTA-915 which is a valid jira issue.

Details

In response to this:

This PR adds a new container to serve the signatures.

Signatures should be added to the directory in order to
serve the signatures.
The dir structure should look like {ALGO}/{DIGEST}/{SIGNATURE}

Add this directory to toml file in-order to read the signatures.
[signatures]
dir = <DIR-HERE>
In the absence of dir in toml file, it'll create a temp directory and
read from it

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

this commit adds a new container to help with serving the signatures. this commits adds all the code required to make the new container work, including liveness/readiness checks, taking user input from CLI and through TOML files. initializing the api and accepting connections.

petr-muller

/lgtm
/hold

I'm not the greatest Rust reviewer ever, hence holding for Trevor or Lala to have a chance if you want them to review. Feel free to unhold at will.

openshift-ci · 2023-06-28T13:56:53Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: petr-muller, PratikMahajan

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Details

Needs approval from an approver in each of these files:

~~OWNERS~~ [PratikMahajan,petr-muller]

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

LalatenduMohanty · 2023-07-07T05:43:02Z

metadata-helper/Cargo.toml

 version = "0.1.0"
 authors = ["Pratik Mahajan <pmahajan@redhat.com>"]
-edition = "2021"
+edition = "2018"


Is it an intentional change?

LalatenduMohanty · 2023-07-07T05:46:17Z

/hold cancel

openshift/cincinnati-graph-data@9e9e97cf2a (OTA-949: README: Define a 1.2.0 filesystem schema for release signatures, 2023-04-19, openshift/cincinnati-graph-data#3509) defined that graph-data version, adding only: signatures/{algorithm}/{digest}/signature-{number} Cincinnati learned how to process those paths already in fb20669 (add signatures serving module to metadata_helper, 2023-06-26, openshift#794). This commit just catches up the supported-version set, to avoid failing like [1]: $ curl -s https://gcsweb-ci.apps.ci.l2s4.p1.openshiftapps.com/gcs/origin-ci-test/pr-logs/pull/openshift_cincinnati-operator/176/pull-ci-openshift-cincinnati-operator-master-operator-e2e-hypershift-local-graph-data/1722001220597452800/artifacts/operator-e2e-hypershift-local-graph-data/e2e-test/artifacts/inspect/namespaces/openshift-updateservice/pods/example-5cd78cdf8b-vqcgv/graph-builder/graph-builder/logs/current.log | tail -n3 2023-11-07T21:50:33.646877602Z [2023-11-07T21:50:33Z TRACE cincinnati::plugins] Running next plugin 'openshift-secondary-metadata-parse' 2023-11-07T21:50:33.657154577Z [2023-11-07T21:50:33Z ERROR graph_builder::graph] unrecognized graph-data version 1.2.0 2023-11-07T21:50:33.657154577Z ; supported versions: ["1.0.0", "1.1.0"] when run against graph-data that declares itself to use the new version. [1]: https://prow.ci.openshift.org/view/gs/origin-ci-test/pr-logs/pull/openshift_cincinnati-operator/176/pull-ci-openshift-cincinnati-operator-master-operator-e2e-hypershift-local-graph-data/1722001220597452800

Exposing the config setting from fb20669 (add signatures serving module to metadata_helper, 2023-05-18, openshift#794) as a command-line option, so folks can pass it though without having to drop into TOML or hitting [1,2]: 2023-11-08T00:01:57.621922383Z [2023-11-08T00:01:57Z INFO metadata_helper] application settings: 2023-11-08T00:01:57.621922383Z AppSettings { 2023-11-08T00:01:57.621922383Z verbosity: Trace, 2023-11-08T00:01:57.621922383Z address: ::, 2023-11-08T00:01:57.621922383Z port: 8082, 2023-11-08T00:01:57.621922383Z path_prefix: "/api/upgrades_info", 2023-11-08T00:01:57.621922383Z status_address: ::, 2023-11-08T00:01:57.621922383Z status_port: 9082, 2023-11-08T00:01:57.621922383Z signatures_dir: "", 2023-11-08T00:01:57.621922383Z tracing_endpoint: None, 2023-11-08T00:01:57.621922383Z backlog: 10, 2023-11-08T00:01:57.621922383Z max_connections: 10, 2023-11-08T00:01:57.621922383Z max_connection_rate: 64, 2023-11-08T00:01:57.621922383Z keep_alive: None, 2023-11-08T00:01:57.621922383Z client_timeout: 5s, 2023-11-08T00:01:57.621922383Z } 2023-11-08T00:01:57.622031074Z [2023-11-08T00:01:57Z INFO metadata_helper] signatures data directory not provided, using /tmp/.tmp2r89fA [1]: https://prow.ci.openshift.org/view/gs/origin-ci-test/pr-logs/pull/openshift_cincinnati-operator/176/pull-ci-openshift-cincinnati-operator-master-operator-e2e-hypershift-local-graph-data/1722037234359603200 [2]: https://gcsweb-ci.apps.ci.l2s4.p1.openshiftapps.com/gcs/origin-ci-test/pr-logs/pull/openshift_cincinnati-operator/176/pull-ci-openshift-cincinnati-operator-master-operator-e2e-hypershift-local-graph-data/1722037234359603200/artifacts/operator-e2e-hypershift-local-graph-data/e2e-test/artifacts/inspect/namespaces/openshift-updateservice/pods/example-695fd68b74-wjtv6/metadata/metadata/logs/current.log

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 May 18, 2023

openshift-ci bot requested review from LalatenduMohanty and petr-muller May 18, 2023 22:29

openshift-ci bot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label May 18, 2023

wking reviewed May 18, 2023

View reviewed changes

PratikMahajan force-pushed the metadata-skeleton branch 10 times, most recently from 7ea86b7 to daeaf26 Compare May 22, 2023 17:27

openshift-merge-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label May 22, 2023

PratikMahajan force-pushed the metadata-skeleton branch from daeaf26 to 72cf2c1 Compare May 22, 2023 17:30

openshift-merge-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label May 22, 2023

PratikMahajan force-pushed the metadata-skeleton branch 4 times, most recently from 06e62cb to 4083aea Compare May 22, 2023 18:43

PratikMahajan changed the title ~~[WIP] Serve signatures through a new container~~ Serve signatures through a new container May 23, 2023

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 May 23, 2023

wking reviewed May 25, 2023

View reviewed changes

petr-muller reviewed Jun 5, 2023

View reviewed changes

PratikMahajan force-pushed the metadata-skeleton branch 3 times, most recently from 19863b4 to 8ac5b48 Compare June 6, 2023 20:14

PratikMahajan force-pushed the metadata-skeleton branch 2 times, most recently from 00a2769 to 454176f Compare June 12, 2023 17:23

wking reviewed Jun 23, 2023

View reviewed changes

PratikMahajan changed the title ~~Serve signatures through a new container~~ OTA-915: Serve signatures through a new container Jun 26, 2023

openshift-ci-robot added the jira/valid-reference Indicates that this PR references a valid Jira ticket of any type. label Jun 26, 2023

PratikMahajan added 2 commits June 26, 2023 11:12

add signatures serving module to metadata_helper

fb20669

PratikMahajan force-pushed the metadata-skeleton branch from 1fcff3c to fb20669 Compare June 26, 2023 15:12

petr-muller approved these changes Jun 28, 2023

View reviewed changes

openshift-ci bot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Jun 28, 2023

openshift-ci bot assigned petr-muller Jun 28, 2023

openshift-ci bot added the lgtm Indicates that a PR is ready to be merged. label Jun 28, 2023

LalatenduMohanty reviewed Jul 7, 2023

View reviewed changes

openshift-ci bot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Jul 7, 2023

openshift-merge-robot merged commit 65793c2 into openshift:master Jul 7, 2023

wking mentioned this pull request Nov 7, 2023

OTA-946: openshift_secondary_metadata_parser: Support graph-data 1.2.0 #876

Merged

wking mentioned this pull request Nov 8, 2023

OTA-915: metadata-helper/src/config/cli: Expose --signatures.dir #877

Merged

OTA-915: Serve signatures through a new container #794

OTA-915: Serve signatures through a new container #794

Uh oh!

Conversation

PratikMahajan commented May 18, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

wking May 18, 2023

Choose a reason for hiding this comment

Uh oh!

wking May 25, 2023

Choose a reason for hiding this comment

Uh oh!

petr-muller commented May 29, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

petr-muller left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

petr-muller May 29, 2023

Choose a reason for hiding this comment

Uh oh!

PratikMahajan Jun 6, 2023

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

PratikMahajan commented Jun 23, 2023

Uh oh!

openshift-ci bot commented Jun 23, 2023

Uh oh!

wking Jun 23, 2023

Choose a reason for hiding this comment

Uh oh!

openshift-ci-robot commented Jun 26, 2023 • edited by openshift-ci bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

petr-muller left a comment

Choose a reason for hiding this comment

Uh oh!

openshift-ci bot commented Jun 28, 2023

Uh oh!

LalatenduMohanty Jul 7, 2023

Choose a reason for hiding this comment

Uh oh!

LalatenduMohanty commented Jul 7, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

PratikMahajan commented May 18, 2023 •

edited

Loading

petr-muller commented May 29, 2023 •

edited

Loading

openshift-ci-robot commented Jun 26, 2023 •

edited by openshift-ci bot

Loading