Feat: Support to accept the response of signing result from RADAS #294

yma955 · 2025-05-13T05:15:16Z

Let me know if we have other preferred branch rather than main, @ligangty

coveralls · 2025-05-13T05:48:03Z

Pull Request Test Coverage Report for Build 15177290271

Details

52 of 163 (31.9%) changed or added relevant lines in 6 files are covered.
No unchanged relevant lines lost coverage.
Overall coverage decreased (-1.8%) to 64.903%

Changes Missing Coverage	Covered Lines	Changed/Added Lines	%
charon/cmd/cmd_upload.py	0	1	0.0%
charon/config.py	15	22	68.18%
charon/pkgs/maven.py	5	18	27.78%
charon/pkgs/oras_client.py	10	29	34.48%
charon/pkgs/radas_signature_handler.py	20	91	21.98%

Totals
Change from base Build 15011136224:	-1.8%
Covered Lines:	2036
Relevant Lines:	3137

💛 - Coveralls

ligangty · 2025-05-14T00:54:06Z

charon/config.py

+        self.__quay_radas_auth_enabled: bool = data.get("quay_radas_auth_enabled", False)
+        self.__quay_radas_registry: str = data.get("quay_radas_registry", None)
+        self.__quay_radas_username: str = data.get("quay_radas_username", None)
+        self.__quay_radas_password: str = data.get("quay_radas_password", None)


These quay specified items should not be set through configuration. they should be specified through cmd flag.

@ligangty, if through cmd flags, might need to transfer the flags values when setup umb client(sign request send) with umb listener register, since this will be used for on_message. But could you help to explain why it needs to be specified through cmd flag?

This registry is a variable one but not fixed for different product packages. For example, eap will have different registry than quarkus one, and for different qurakus versions, the registry will also have different version tags in quay.

and for the username and password, maybe we will use another way like registry-config file. See "oras --registry-config" for details.

@ligangty Just for sure, you mean the registry host(https://quay.io/repository/signing/radas) will be changed for different products? This properties are just used for login, it means oras just needs hostname. For tags, it should be used for pull and we could get that from the result_reference url.
So,

Maybe we don't need quay_radas_registry, just get the hostname from the result_reference url extracting, CMIMW.

For the registry-config file, does it store in any env loc for different products, or still from user cmd input flags?

Maybe we don't need quay_radas_registry, just get the hostname from the result_reference url extracting, CMIMW.

Agree with this.

For the registry-config file, does it store in any env loc for different products, or still from user cmd input flags?

FWIK now, the registry is a public one which don't need auth. So maybe for first version we will not care auth issue. For the registry-config file, I think it will be a configmap or secret later in openshift managed workspace.

@ligangty So, I just file a quay_radas_registry_config.py and we could enhance more if it needs in future, or just ignore the login part, WDYT?

Maybe you can directly add them to the RadasConfig class in my new merged PR?

@ligangty Sure, will adjust that.

ligangty · 2025-05-14T00:55:22Z

charon/config.py

+        self.__quay_radas_password: str = data.get("quay_radas_password", None)
+        self.__radas_sign_enabled: bool = data.get("radas_sign_enabled", False)
+        self.__radas_sign_timeout_count: int = data.get("radas_sign_timeout_count", 10)
+        self.__radas_sign_wait_interval_sec: int = data.get("radas_sign_wait_interval_sec", 60)


What's the difference between the interval and timeout here?

@ligangty timeout_count means the number of retries, interval means the time for per sleep, maybe rename them as 'radas_sign_timeout_retry_count' and 'radas_sign_timeout_retry_interval' would be more explicit.

Sounds good.

ligangty · 2025-05-14T00:56:39Z

charon/config.py

        self.__aws_cf_enable: bool = data.get("aws_cf_enable", False)
+        self.__amqp_queue: str = data.get("amqp_queue", None)
+        self.__sign_result_loc: str = data.get("sign_result_loc", None)
+        self.__quay_radas_auth_enabled: bool = data.get("quay_radas_auth_enabled", False)


Is this auth_enabled needed? I think if the user/pass is not provided, it means the auth is not enabled, vice versa.

@ligangty ok, sure.

ligangty · 2025-05-14T07:37:38Z

charon/config.py

        self.__signature_command: str = data.get("detach_signature_command", None)
        self.__aws_cf_enable: bool = data.get("aws_cf_enable", False)
+        self.__amqp_queue: str = data.get("amqp_queue", None)
+        self.__sign_result_loc: str = data.get("sign_result_loc", None)


@yma96 I think this one is also needed to be passed by cmd flag. The cause is that we should store this in Konflux workspace storage, but the path to this workspace is a variable which is passed by Konflux for each release pipeline run. That means it will always change and we don't know the exact value. So the only way here is give it through flag and then we can specify it through Konflux shell environment.

@ligangty, ok, make sense.

@yma96 BTW, can I ask this "sign_result_loc" 's purpose? Is it used to store the sign result bundle or extracted signing files?

@ligangty, this's used to store the oras pull result json file from radas quay registry, and let maven-publish could detect this file to handle the following .asc files generation, the final singing files will be generated directly alongside the artifact.

@yma96 Oh ok. Where will you want to process this file in charon? During the maven uploading phase?
What I want to check here is because the "sign" and the "process" may happen in different command(Should it?), so we need to know how to pass this file between these two command.

@ligangty, this "sign_result_loc" should only be used for "sign"(when message received), oras pulls the files result to this loc and store the files result into the SignHandler:_downloaded_files, and we could use classmethod get_downloaded_files to detect the files result during "process"(when maven upload), since this kind of class method is bound to the class and not the instance of the class, so that could be shared by all instances/clsName call. Through this python feature, we may not need to transfer this value across two commands.

@yma96 one big issue is "sign" and "maven_upload" are not happend in same time. In later Konflux usage, they are called in different tasks with two command call. That means we could not use any memory way (like you said for the classmethod) to store these files. We should store them into a Konflux reachable storage between different tasks. That's why I proposed to use a flag to pass this file location.

@ligangty Ok, I just think of a flag to detect, not aware of the sign is a dependent task, so the SignHandler status management here should not be workable, and the corresponding process status handling should be adjusted too.

How about environ SIGN_RESULT_LOC for sign_result_loc after sign msg received? For maven upload, could it be able to fetch the env SIGN_RESULT_LOC across different tasks?

Or just open this flag for both of the two tasks, and Konflux could help to transfer that sequentially?

@yma96 the env var is not shared between two tekton tasks. That's why the task is using the "result"(see https://medium.com/@email2smohanty/passing-values-between-tekton-tasks-using-task-results-9ac89a44b2b9). What I'm thinking here is passing this sign_result_loc into sign command in sign task, and then store it in to task result. Then extract it in the maven upload task by passing it to maven upload command.

@ligangty, make sense, so both of the two tasks would open this loc as the cmd flag IIUC.

…terval

…load

ligangty · 2025-05-20T02:17:54Z

charon/config.py

        self.__ignore_signature_suffix: Dict = data.get("ignore_signature_suffix", None)
        self.__signature_command: str = data.get("detach_signature_command", None)
        self.__aws_cf_enable: bool = data.get("aws_cf_enable", False)
+        self.__radas_config_enable: bool = data.get("radas_config_enable", False)


After thinking, we don't need an extra "enable" here, instead we can use "radas_config and radas_config.validate" to decide if radas is enabled.

@ligangty sounds good, already update in the new commit, BTW I've committed code in the different commits with different aspects mentioned in the review, which might be more convenient to check.

ligangty · 2025-05-20T02:18:05Z

charon/config.py

+        self.__radas_config_enable: bool = data.get("radas_config_enable", False)
        radas_config: Dict = data.get("radas", None)
        if radas_config:
+            self.__radas_config_enable = True


ligangty · 2025-05-20T02:20:16Z

charon/config.py

        return self.__aws_cf_enable

+    def is_radas_config_enable(self) -> bool:
+        return self.__radas_config_enable


we can use "radas_config and radas_config.validate()" to decide if it is enabled.

…ssary for public

ligangty · 2025-05-20T04:56:45Z

charon/config.py

+            self.__quay_radas_registry_config, os.R_OK
+        ):
+            logger.error("The quay registry config for oras is not valid!")
+            return False


Maybe this should not fail the radas config, but just give a warn message and leave this registry_config as None, which means we will consider it as a public registry without authentication.

@ligangty done, 947f23c

…o read

ligangty · 2025-05-20T07:28:41Z

charon/config.py


    def is_aws_cf_enable(self) -> bool:
        return self.__aws_cf_enable



I'm thinking we should add a __is_radas_enabled field here for convenient, which set to self.__radas_config__ and self.__radas_config__.validate(). So then we can use this with a get method is_radas_enabled() for unified way in other place to check if radas is usable.

ligangty · 2025-05-20T07:55:18Z

charon/pkgs/radas_signature_handler.py

+        """
+        conf = get_config()
+        rconf = conf.get_radas_config() if conf else None
+        if not rconf:


so maybe here we can also use not conf.is_radas_enabled()?

@ligangty: da803dd

ligangty · 2025-05-20T08:59:33Z

charon/pkgs/radas_signature_handler.py

+        logger.info("Number of files pulled: %d, path: %s", len(files), files[0])
+
+
+def generate_radas_sign(top_level: str, sign_result_loc: str) -> Tuple[List[str], List[str]]:


Do you think we should add some utests for this method to make sure it works as expectation?

@ligangty Yeah, I want to add the unit test for this feat, sign result parse and generation, etc, might need some mock if more thorough , I could file a unit test JIRA for this, WDYT?

@yma96 sure ok.

ligangty · 2025-05-20T09:07:44Z

charon/pkgs/radas_signature_handler.py

+    wait_count = 0
+
+    # Wait until files appear in the sign_result_loc directory
+    while True:


I don't think we need to wait here, because this this gen_file method should make sure the result_file is ready. If it is not, that means the umblistener already failed with some reasons, which does not retrieve the result correctly.

@ligangty It takes time from sending the sign request to receiving and processing the message, if maven_upload is called immediately or in quite short time, no wait will lead to no sign result.

@yma96 we need to make sure that the sign command has fixed result, like has this result file downloaded as success state or got some error as failure state. If succeeded then we can proceed the maven upload, vice visa the maven upload will never be triggered. This means all wait control should happen in umb receiver processing but not in maven processing.

@ligangty You mean the sign task should have a state return for the sign result, which could be used to trigger the maven-upload, something like:
- name: maven-upload
when:
- input: "$(tasks.sign-cmd.results.is-signed-proccessed)"
operator: in
values: ["run"]
That might need the listener on_message is handled in a sync way, or I'm afraid sign task would not wait until the on_message is received and process ends.
Generally, proton client register the listener then container.run() automatically and keeps the event loop running until connections close. We may need another container.process() to perform the specific loop, when the sign receiver and process is not done. That is to say, we may need to consider the sign request and sign receive as a whole in the first sign task, CMIMW.

@yma96

You mean the sign task should have a state return for the sign result, which could be used to trigger the maven-upload, something like:
- name: maven-upload
when:
- input: "$(tasks.sign-cmd.results.is-signed-proccessed)"
operator: in
values: ["run"]

What I mean is similar but we will not bring a new stuff like this is-signed-processed. We just pass the sign_result_loc between task, or failed the sign task if error happened, which will break the whole process.

That might need the listener on_message is handled in a sync way, or I'm afraid sign task would not wait until the on_message is received and process ends.
Generally, proton client register the listener then container.run() automatically and keeps the event loop running until connections close. We may need another container.process() to perform the specific loop, when the sign receiver and process is not done. That is to say, we may need to consider the sign request and sign receive as a whole in the first sign task, CMIMW.

I'm thinking we can use the main thread to control the sync process like what you have done in the maven_upload now, which checks and waits the sign file downloaded or error happened. I really don't want to bring more complexity into the maven process as this complexity is introduced by radas but not maven itself. If possible we'd keep the complexity in radas_sign process itself.

@ligangty IIUC, move this timeout retry part to be closely after the umb send. If yes, I'll move this part out of the maven upload, and I've updated a commit f784b16 to remove the thread usage with on_message process, and the retry part we could move later after the sign send code is there.

@yma96 Yes. We will have a function like "sign_in_radas()" in main thread which will be called in the cli part. It will be responsible to do the overall controlling of the whole process, like trigger the send and register the receiver, and control the wait and timeout there.

I've created a PR which construct the skeleton of the sign command here: https://github.com/Commonjava/charon/pull/296/files

@ligangty Sounds good, we could try to migrate this control later. BTW, I've removed that from maven-upload: 83a92f9

ligangty

I think we can merge this. If any changes or fixes let's use separate PR later.

yma955 requested review from ligangty and shokakucarrier May 13, 2025 05:15

yma955 force-pushed the main branch 3 times, most recently from f017afb to 7a82d79 Compare May 13, 2025 05:47

yma955 force-pushed the main branch from 7a82d79 to 49dff60 Compare May 13, 2025 05:53

ligangty reviewed May 14, 2025

View reviewed changes

yma955 added 4 commits May 19, 2025 14:30

Feat: Support to accept the response of signing result from RADAS

e322b4e

Format Code and fix typo

07e43f3

Rename radas_sign_timeout_retry_count and radas_sign_timeout_retry_in…

42dbb01

…terval

Use oras registry_config and registry url parse to finalize login

89be82f

yma955 force-pushed the main branch 3 times, most recently from 74c94d4 to 8931a7f Compare May 20, 2025 02:01

Change sign_result_loc to cmd flag both for sign request and maven up…

d4eee68

…load

yma955 force-pushed the main branch from 8931a7f to d4eee68 Compare May 20, 2025 02:06

ligangty reviewed May 20, 2025

View reviewed changes

Update quay_radas_registry_config to default None since it's not nece…

d2e86c9

…ssary for public

yma955 force-pushed the main branch 2 times, most recently from 815dc4d to e906e92 Compare May 20, 2025 03:26

Use radas_config validate instead of is_radas_config_enable option

75740e5

yma955 force-pushed the main branch from e906e92 to 75740e5 Compare May 20, 2025 03:33

ligangty reviewed May 20, 2025

View reviewed changes

yma955 force-pushed the main branch 2 times, most recently from e391260 to 85fae74 Compare May 20, 2025 06:25

Ignore the registry config if the provided config path is not valid t…

947f23c

…o read

yma955 force-pushed the main branch from 85fae74 to 947f23c Compare May 20, 2025 06:27

ligangty reviewed May 20, 2025

View reviewed changes

yma955 force-pushed the main branch 5 times, most recently from 96684b7 to a4ad9e0 Compare May 20, 2025 08:26

Add is_radas_enabled unified method to check radas enablement

da803dd

yma955 force-pushed the main branch from a4ad9e0 to da803dd Compare May 20, 2025 08:30

ligangty reviewed May 20, 2025

View reviewed changes

Change on_message process method without using threads

f784b16

yma955 force-pushed the main branch from ef48cba to f784b16 Compare May 21, 2025 07:25

Remove timeout retry handling for sign result fetch from maven upload

83a92f9

yma955 force-pushed the main branch from 330725e to 83a92f9 Compare May 22, 2025 03:16

ligangty approved these changes May 22, 2025

View reviewed changes

yma955 merged commit cfdd37f into Commonjava:main May 22, 2025
17 checks passed


		def is_aws_cf_enable(self) -> bool:
		return self.__aws_cf_enable

		logger.info("Number of files pulled: %d, path: %s", len(files), files[0])


		def generate_radas_sign(top_level: str, sign_result_loc: str) -> Tuple[List[str], List[str]]:

Feat: Support to accept the response of signing result from RADAS #294

Feat: Support to accept the response of signing result from RADAS #294

Uh oh!

Conversation

yma955 commented May 13, 2025

Uh oh!

coveralls commented May 13, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Pull Request Test Coverage Report for Build 15177290271

Details

💛 - Coveralls

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ligangty May 14, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ligangty May 14, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

yma955 May 14, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ligangty May 15, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ligangty May 15, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

yma955 May 15, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

coveralls commented May 13, 2025 •

edited

Loading

ligangty May 14, 2025 •

edited

Loading

ligangty May 14, 2025 •

edited

Loading

yma955 May 14, 2025 •

edited

Loading

ligangty May 15, 2025 •

edited

Loading

ligangty May 15, 2025 •

edited

Loading

yma955 May 15, 2025 •

edited

Loading

ligangty May 20, 2025 •

edited

Loading

yma955 May 21, 2025 •

edited

Loading

ligangty May 21, 2025 •

edited

Loading