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

Add ip address whitelist UI to manage satellites page for itless #2836

Merged

Conversation

florkbr
Copy link
Contributor

@florkbr florkbr commented May 15, 2024

Depends on: RedHatInsights/chrome-service-backend#668

Utilizes mbop changes here: RedHatInsights/mbop#105

Adds a simple UI to the manage satellites page to add/remove IP address(es) WRT to our itless environments.

Marking as draft until I manually try the API requests in this PR via postman on pre-prod env.
EDIT: verified API requests after deploying

@codecov-commenter
Copy link

Codecov Report

Attention: Patch coverage is 14.58333% with 82 lines in your changes are missing coverage. Please review.

Project coverage is 62.47%. Comparing base (3194dee) to head (c040274).
Report is 13 commits behind head on master.

❗ Current head c040274 differs from pull request most recent head ca648e6. Consider uploading reports for the commit ca648e6 to get more accurate results

Additional details and impacted files

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #2836      +/-   ##
==========================================
- Coverage   63.54%   62.47%   -1.07%     
==========================================
  Files         199      202       +3     
  Lines        4301     4397      +96     
  Branches      777      783       +6     
==========================================
+ Hits         2733     2747      +14     
- Misses       1559     1641      +82     
  Partials        9        9              
Files Coverage Δ
src/auth/setCookie.ts 100.00% <100.00%> (ø)
src/components/Header/UserToggle.tsx 93.93% <ø> (ø)
src/state/atoms/releaseAtom.ts 100.00% <100.00%> (ø)
src/state/atoms/utils.ts 100.00% <100.00%> (ø)
src/state/chromeStore.ts 100.00% <100.00%> (ø)
src/components/Header/Tools.tsx 63.76% <0.00%> (-0.94%) ⬇️
src/layouts/SatelliteToken.tsx 5.88% <0.00%> (-0.37%) ⬇️
src/utils/debugFunctions.ts 12.00% <0.00%> (-0.50%) ⬇️
src/components/Satellite/IPWhitelistTable.tsx 1.25% <1.25%> (ø)

@Hyperkid123
Copy link
Contributor

@florkbr do we want this to be baked into chrome? Can this be its own module?

@florkbr
Copy link
Contributor Author

florkbr commented May 15, 2024

@Hyperkid123 I don't think it necessarily needs to be baked into chrome - I just started from where the UI lived today.

Do you have any suggestions or thoughts on where the new module could/should be placed?

@Hyperkid123
Copy link
Contributor

@florkbr a new module :) Like the widget UI.

@florkbr florkbr force-pushed the add-ip-range-input-to-manage-sat branch from 174a06c to 8f7bab3 Compare September 9, 2024 18:19
@florkbr florkbr marked this pull request as ready for review September 9, 2024 18:20
@florkbr
Copy link
Contributor Author

florkbr commented Sep 9, 2024

When deploying this in our itless env I'm seeing the same pod crash loop as noted in: #2922

@florkbr florkbr force-pushed the add-ip-range-input-to-manage-sat branch from ccdf9ca to 5c92b08 Compare September 9, 2024 22:03
@@ -38,11 +47,6 @@ const SatelliteToken: React.FC = () => {
<Page
className="chr-c-all-services"
onPageResize={null} // required to disable PF resize observer that causes re-rendring issue
header={
<Masthead className="chr-c-masthead">
Copy link
Contributor Author

@florkbr florkbr Sep 9, 2024

Choose a reason for hiding this comment

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

@Hyperkid123 since we are now exposing this as a module from chrome at /insights/satellite - this is causing the insights masthead and nav to render around the module (hence removing the masthead here otherwise we see duplicates).

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 page require some unique layout?

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 - the content is just a bit odd/out of the ordinary as it is 3 utilities.

frontend.yml Outdated
@@ -19,6 +19,11 @@ objects:
manifestLocation: "/apps/chrome/js/fed-mods.json"
config:
ssoUrl: ${SSO_URL}
modules:
- id: 'satellite-token'
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@Hyperkid123 I noticed this is duplicated with my changes in chrome-service-backend: RedHatInsights/chrome-service-backend#668. I know this is used for the frontend-operator - but can you explain why we need both? Thanks.

Copy link
Contributor

Choose a reason for hiding this comment

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

We don't need both. Only the chrome service. This will become relevant and the source of truth once we have the new FEO features.

@@ -259,7 +258,6 @@ const ScalprumRoot = memo(
}
/>
)}
{ITLess() && <Route path="/insights/satellite" element={<SatelliteToken />} />}
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Removed the react router route in ScalprumRoot in favor of the exposed module approach.

frontend.yml Outdated
@@ -19,6 +19,11 @@ objects:
manifestLocation: "/apps/chrome/js/fed-mods.json"
config:
ssoUrl: ${SSO_URL}
modules:
- id: 'satellite-token'
Copy link
Contributor

Choose a reason for hiding this comment

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

We don't need both. Only the chrome service. This will become relevant and the source of truth once we have the new FEO features.

@@ -38,11 +47,6 @@ const SatelliteToken: React.FC = () => {
<Page
className="chr-c-all-services"
onPageResize={null} // required to disable PF resize observer that causes re-rendring issue
header={
<Masthead className="chr-c-masthead">
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 page require some unique layout?

@Hyperkid123
Copy link
Contributor

/retest

@florkbr florkbr force-pushed the add-ip-range-input-to-manage-sat branch from 767fb92 to a69b2d8 Compare September 10, 2024 21:15
@florkbr
Copy link
Contributor Author

florkbr commented Sep 10, 2024

@Hyperkid123 I removed the changes to frontend.yml for now. Let me know if you think this needs anything else! I'll likely still need to follow up with some styling tweaks on the IP whitelist UI itself - but the major flows seems to be working for me.

Copy link
Contributor

@Hyperkid123 Hyperkid123 left a comment

Choose a reason for hiding this comment

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

LGTM

@florkbr
Copy link
Contributor Author

florkbr commented Sep 12, 2024

The PR build pipeline is still failing due to the CrashLoopBackOff issue mentioned earlier. I've started a thread in our dev-prod channel and will continue investigating on #2922

@florkbr florkbr merged commit 20d6a98 into RedHatInsights:master Sep 12, 2024
7 of 9 checks passed
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