Wizard: add satellite registration #2963

avitova · 2025-03-05T09:24:22Z

This adds an option to paste the Satellite registration command + CA. Saving this information into the blueprint is still not done; I will create a follow-up; this is big PR as is.

avitova · 2025-03-05T11:43:29Z

Screencast.From.2025-03-05.10-51-23.mp4

So this is the UI change. We try to find the JWT token, in case it is not there, we disable the "next" button. We also show an error if the token is expired, but in that case, we let users through.
So far, we only allow some certificates. If you have any ideas how to validate this differently, please let me know.

codecov · 2025-03-05T12:22:10Z

Codecov Report

Attention: Patch coverage is 30.08130% with 172 lines in your changes missing coverage. Please review.

Project coverage is 81.53%. Comparing base (978a37f) to head (e3f1eb1).
Report is 10 commits behind head on main.

Files with missing lines	Patch %	Lines
...izard/steps/Registration/SatelliteRegistration.tsx	10.38%	69 Missing ⚠️
...ents/CreateImageWizard/utilities/useValidation.tsx	11.11%	48 Missing ⚠️
...ration/components/SatelliteRegistrationCommand.tsx	20.00%	32 Missing ⚠️
...ateImageWizard/steps/Registration/Registration.tsx	60.71%	11 Missing ⚠️
src/store/wizardSlice.ts	66.66%	7 Missing ⚠️
...onents/CreateImageWizard/utilities/certificates.ts	20.00%	4 Missing ⚠️
...nents/CreateImageWizard/utilities/requestMapper.ts	91.66%	1 Missing ⚠️

@@            Coverage Diff             @@
##             main    #2963      +/-   ##
==========================================
- Coverage   82.09%   81.53%   -0.57%     
==========================================
  Files         209      212       +3     
  Lines       23517    23754     +237     
  Branches     2329     2339      +10     
==========================================
+ Hits        19306    19367      +61     
- Misses       4182     4358     +176     
  Partials       29       29

Files with missing lines	Coverage Δ
...nts/CreateImageWizard/steps/Registration/index.tsx	`100.00% <100.00%> (ø)`
src/store/service/contentSourcesApi.ts	`82.60% <ø> (ø)`
src/store/service/imageBuilderApi.ts	`86.88% <ø> (ø)`
...t/Components/CreateImageWizard/wizardTestUtils.tsx	`96.06% <100.00%> (ø)`
...nents/CreateImageWizard/utilities/requestMapper.ts	`94.87% <91.66%> (-0.07%)`	⬇️
...onents/CreateImageWizard/utilities/certificates.ts	`20.00% <20.00%> (ø)`
src/store/wizardSlice.ts	`87.95% <66.66%> (-0.52%)`	⬇️
...ateImageWizard/steps/Registration/Registration.tsx	`88.26% <60.71%> (-7.87%)`	⬇️
...ration/components/SatelliteRegistrationCommand.tsx	`20.00% <20.00%> (ø)`
...ents/CreateImageWizard/utilities/useValidation.tsx	`74.47% <11.11%> (-9.18%)`	⬇️
... and 1 more

Continue to review full report in Codecov by Sentry.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 978a37f...e3f1eb1. Read the comment docs.

🚀 New features to boost your workflow:

❄ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

lucasgarfield · 2025-03-05T20:22:58Z

Love the validation guardrails!

lucasgarfield · 2025-03-05T21:47:51Z

package.json

@@ -19,6 +19,7 @@
    "@sentry/webpack-plugin": "3.2.1",
    "@unleash/proxy-client-react": "4.5.2",
    "classnames": "2.5.1",
+    "jwt-decode": "^4.0.0",


New dependency! Can you please remove the ^? We rely on Dependabot to handle everything.

Also, I'd be curious to know about the decision to use jwt-decode. What was your selection criteria?

I'm re-reading my comment and I realized it is a bit curt. 😅

jwt-decode is probably fine but it would be nice to know why we chose it, especially in the commit message for the future - is it the most popular or well maintained library for this? Did we look into any alternatives, etc...? I don't have any specific problem with jwt-decode itself I'm just totally ignorant so some extra context would be nice. 😄

Alright, that makes sense. I removed the ^, and added information into the commit, hopefully that's what you meant. :)

regexowl · 2025-03-06T07:59:47Z

+1 on pining the dep version, otherwise looks great! ❤️ One question - could isValidPEM go into validators.ts or are there more plans for certificates.ts?

Also can't see the flag in stage unleash, could you please add it there as well? We might even want to gate it behind a preview constraint as this will probably affect IQE 🤔

src/Components/CreateImageWizard/steps/Registration/components/SatelliteRegistrationCommand.tsx

ezr-ondrej

Great work! 🧡

ezr-ondrej · 2025-03-06T09:05:52Z

src/Components/CreateImageWizard/utilities/certificates.ts

@@ -0,0 +1,5 @@
+export const isValidPEM = (cert: string): boolean => {
+  return /-----BEGIN CERTIFICATE-----[\s\S]+-----END CERTIFICATE-----/.test(


Nit: isn't [\s\S]+ equal to .+?

Hmmm, I tried to figure this out as well, we'd need to add "s" at the end of the regex, something like this:
/-----BEGIN CERTIFICATE-----.+-----END CERTIFICATE-----/s

Otherwise it would not accept certificates with more lines. I can do that if it makes the regex a bit more clear.

src/Components/CreateImageWizard/utilities/useValidation.tsx

ezr-ondrej · 2025-03-06T09:15:30Z

src/Components/CreateImageWizard/utilities/useValidation.tsx

+        const decoded = jwtDecode(token);
+        if (decoded.exp) {
+          const currentTime = Date.now() / 1000;
+          if (decoded.exp < currentTime) {


Improvement idea, not a blocker: this could potentially have a buffer.
I mean the image build takes about 15 minutes, so token expiring in half an hour doesn't give you much room to use the image 😛
We might want to figure out a warning intead in a follow-up :)

I like this idea, and I can do that as a followup. I still need to add the Satellite registration to the review step, add the command + certificate to the blueprint, so that it is saved. This could fit in too. ✏️ 📓

ezr-ondrej · 2025-03-06T09:18:22Z

Ugh there is a conflict already, that was fast 🤔

avitova · 2025-03-06T09:19:25Z

+1 on pining the dep version, otherwise looks great! ❤️ One question - could isValidPEM go into validators.ts or are there more plans for certificates.ts?

Uhm, I'd like to add also isDer/isCer, but I need to look into that one. On top of that, the plan might be that users could paste more certificates into one textfield, and we'd parse them all. But I think we should do this as a followup. But yeah, it will likely be a bit more complicated.

Also can't see the flag in stage unleash, could you please add it there as well? We might even want to gate it behind a preview constraint as this will probably affect IQE 🤔

Done, that's a good point, thanks!

regexowl · 2025-03-06T09:51:51Z

+1 on pining the dep version, otherwise looks great! ❤️ One question - could isValidPEM go into validators.ts or are there more plans for certificates.ts?

Uhm, I'd like to add also isDer/isCer, but I need to look into that one. On top of that, the plan might be that users could paste more certificates into one textfield, and we'd parse them all. But I think we should do this as a followup. But yeah, it will likely be a bit more complicated.

Got it! I don't have anything against splitting validators into separate files. Just curious 👍 Thinking about it we should probably do a bit of a cleanup around utilities and unsorted files in the Wizard folder, but that's completely outside of this PR's scope 😅

ezr-ondrej · 2025-03-06T11:48:38Z

src/Components/CreateImageWizard/steps/Registration/components/SatelliteRegistrationCommand.tsx

+        <HelperText>
+          {/* TODO: Add link to our docs */}
+          <HelperTextItem>
+            To generate command from Satellite, follow the documentation.


Should we link the current Satellite registration for now? :)
https://docs.redhat.com/en/documentation/red_hat_satellite/6.16/html-single/managing_hosts/index#Customizing_the_Registration_Templates_managing-hosts

We could, both no link and using a temporary relevant link work for me. We just need to remember to open a follow up when the new documentation is ready. 👍

ezr-ondrej

All right then, I think we can merge, the only change with the feature flag turned off should be change to radios, which should be fine in the interim :)

regexowl · 2025-03-10T10:25:33Z

Switch from a single checkbox to two radios might impact IQE, @tkoscieln could you please check if the tests will need an adjustment?

ezr-ondrej · 2025-03-10T10:47:37Z

Ah, right, that's the ci.int failure :/

tkoscieln · 2025-03-10T10:58:30Z

@regexowl will do!

The jwt decode dependency helps us to keep track of the token that is present in the Satellite command. jwt-decode is the most popular dependency for the job, and very easy to use.

avitova · 2025-03-10T11:22:22Z

Oopsie, did not see the approval. I just put the docs link and fixed a bug, hope y'all do not mind. ✨

ezr-ondrej

I'm still fine, we need to fix the tests tho.
@tkoscieln would you do that and then merge this PR? or what's the process now?

tkoscieln · 2025-03-11T11:25:23Z

I'll deploy the PR locally and see if it breaks something. If not, we can merge, if yes, I will do the necessary adjustments and then we can merge

ezr-ondrej · 2025-03-11T13:32:40Z

the failures in the CI seems to be related :)

>               raise WidgetOperationFailed("Failed to set the checkbox to requested value.")
E               widgetastic.exceptions.WidgetOperationFailed: Failed to set the checkbox to requested value.

tkoscieln · 2025-03-11T13:57:31Z

Well, I'll do the adjustment then 😃

avitova force-pushed the sat-reg branch 3 times, most recently from cc5aed1 to 6c11792 Compare March 5, 2025 11:41

avitova marked this pull request as ready for review March 5, 2025 11:50

avitova requested review from ezr-ondrej, croissanne and regexowl March 5, 2025 11:52

avitova force-pushed the sat-reg branch from 6c11792 to bbc2f4f Compare March 5, 2025 12:10

lucasgarfield reviewed Mar 5, 2025

View reviewed changes

regexowl reviewed Mar 6, 2025

View reviewed changes

src/Components/CreateImageWizard/steps/Registration/components/SatelliteRegistrationCommand.tsx Outdated Show resolved Hide resolved

avitova force-pushed the sat-reg branch from bbc2f4f to cbd0d36 Compare March 6, 2025 09:11

ezr-ondrej reviewed Mar 6, 2025

View reviewed changes

avitova force-pushed the sat-reg branch from cbd0d36 to 47360ac Compare March 6, 2025 09:42

ezr-ondrej reviewed Mar 6, 2025

View reviewed changes

ezr-ondrej force-pushed the sat-reg branch from 47360ac to e7b64df Compare March 10, 2025 10:08

ezr-ondrej previously approved these changes Mar 10, 2025

View reviewed changes

Wizard: add satellite registration, add jwt-decode

e3f1eb1

The jwt decode dependency helps us to keep track of the token that is present in the Satellite command. jwt-decode is the most popular dependency for the job, and very easy to use.

avitova dismissed ezr-ondrej’s stale review via e3f1eb1 March 10, 2025 11:19

avitova force-pushed the sat-reg branch from e7b64df to e3f1eb1 Compare March 10, 2025 11:19

avitova requested a review from ezr-ondrej March 11, 2025 09:29

ezr-ondrej approved these changes Mar 11, 2025

View reviewed changes

regexowl added the 🧪 needs IQE update label Mar 13, 2025

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Wizard: add satellite registration #2963

Wizard: add satellite registration #2963

avitova commented Mar 5, 2025

avitova commented Mar 5, 2025 •

edited

Loading

codecov bot commented Mar 5, 2025 •

edited

Loading

lucasgarfield commented Mar 5, 2025

lucasgarfield Mar 5, 2025

lucasgarfield Mar 6, 2025

avitova Mar 6, 2025

regexowl commented Mar 6, 2025

ezr-ondrej left a comment

ezr-ondrej Mar 6, 2025

avitova Mar 6, 2025

ezr-ondrej Mar 6, 2025

avitova Mar 6, 2025

ezr-ondrej commented Mar 6, 2025

avitova commented Mar 6, 2025 •

edited

Loading

regexowl commented Mar 6, 2025

ezr-ondrej Mar 6, 2025

regexowl Mar 10, 2025

ezr-ondrej left a comment

regexowl commented Mar 10, 2025

ezr-ondrej commented Mar 10, 2025

tkoscieln commented Mar 10, 2025

avitova commented Mar 10, 2025

ezr-ondrej left a comment

tkoscieln commented Mar 11, 2025

ezr-ondrej commented Mar 11, 2025

tkoscieln commented Mar 11, 2025

		@@ -0,0 +1,5 @@
		export const isValidPEM = (cert: string): boolean => {
		return /-----BEGIN CERTIFICATE-----[\s\S]+-----END CERTIFICATE-----/.test(

Wizard: add satellite registration #2963

Are you sure you want to change the base?

Wizard: add satellite registration #2963

Conversation

avitova commented Mar 5, 2025

avitova commented Mar 5, 2025 • edited Loading

codecov bot commented Mar 5, 2025 • edited Loading

Codecov Report

lucasgarfield commented Mar 5, 2025

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

regexowl commented Mar 6, 2025

ezr-ondrej left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ezr-ondrej commented Mar 6, 2025

avitova commented Mar 6, 2025 • edited Loading

regexowl commented Mar 6, 2025

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ezr-ondrej left a comment

Choose a reason for hiding this comment

regexowl commented Mar 10, 2025

ezr-ondrej commented Mar 10, 2025

tkoscieln commented Mar 10, 2025

avitova commented Mar 10, 2025

ezr-ondrej left a comment

Choose a reason for hiding this comment

tkoscieln commented Mar 11, 2025

ezr-ondrej commented Mar 11, 2025

tkoscieln commented Mar 11, 2025

avitova commented Mar 5, 2025 •

edited

Loading

codecov bot commented Mar 5, 2025 •

edited

Loading

avitova commented Mar 6, 2025 •

edited

Loading