Add X-Org-Id header for IoP requests #1092
Merged
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Reviewer's GuideThis PR centralizes certificate-based authentication and header injection by replacing the old GatewayRequest mixin with a unified CertAuth service that embeds the X-Org-Id header, and extends all IoP-related workflows—VMAAS reposcan sync, Foreman inventory upload, Insights API forwarder, and TagsAuth—to propagate the organization context via the X-Org-Id header. The deprecated gateway_request implementation is removed. Sequence diagram for X-Org-Id header propagation in cloud request executionsequenceDiagram
participant Caller
participant CertAuth
participant CloudService
Caller->>CertAuth: execute_cloud_request(params)
CertAuth->>CloudService: HTTP request with X-Org-Id header
CloudService-->>CertAuth: Response
CertAuth-->>Caller: Response
ER diagram for organization context propagationerDiagram
ORGANIZATION {
string label
int id
}
REPOSITORY {
int id
int organization_id
}
ORGANIZATION ||--o{ REPOSITORY : owns
Class diagram for CertAuth replacing GatewayRequest and header injectionclassDiagram
class CertAuth {
+execute_cloud_request(params)
+organization
}
class InsightsApiForwarder {
+forward_request(original_request, path, controller_name, user, organization)
}
class TagsAuth {
+update_tag()
}
class VmaasReposcanSync {
+plan(repo, *_args)
+run()
+organization
}
CertAuth <|.. InsightsApiForwarder
CertAuth <|.. TagsAuth
CertAuth <|.. VmaasReposcanSync
GatewayRequest <|.. InsightsApiForwarder : removed
GatewayRequest <|.. TagsAuth : removed
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
ShimShtein
force-pushed
the
force_iop_org
branch
from
dbe2753 to
be8cb01
Compare
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey there - I've reviewed your changes and they look great!
Prompt for AI Agents
Please address the comments from this code review:
## Individual Comments
### Comment 1
<location> `lib/foreman_inventory_upload/scripts/uploader.sh.erb:29-33` </location>
<code_context>
AUTH_VAL="\"$RH_USERNAME\":\"$RH_PASSWORD\""
fi
+if [ -n "$ORG_ID" ]
+then
+ ORG_HEADER="-H \"X-Org-Id: $ORG_ID\""
+fi
+
</code_context>
<issue_to_address>
**suggestion (bug_risk):** ORG_HEADER is not initialized when ORG_ID is unset, which may cause an empty argument to curl.
Initialize ORG_HEADER to an empty string before the conditional to avoid passing an unset variable to curl.
```suggestion
ORG_HEADER=""
if [ -n "$ORG_ID" ]
then
ORG_HEADER="-H \"X-Org-Id: $ORG_ID\""
fi
```
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
ShimShtein
force-pushed
the
force_iop_org
branch
from
be8cb01 to
a134304
Compare
ShimShtein
commented
Sep 17, 2025
There was a problem hiding this comment.
Hey there - I've reviewed your changes and they look great!
Prompt for AI Agents
Please address the comments from this code review:
## Individual Comments
### Comment 1
<location> `app/services/foreman_rh_cloud/cert_auth.rb:18-20` </location>
<code_context>
ssl_client_cert: OpenSSL::X509::Certificate.new(certs[:cert]),
ssl_client_key: OpenSSL::PKey.read(certs[:key]),
- }.deep_merge(params)
+ headers: {
+ 'X-Org-Id' => ForemanRhCloud.with_iop_smart_proxy? ? organization&.label : nil,
+ },
+ }.compact.deep_merge(params)
</code_context>
<issue_to_address>
**issue (bug_risk):** The conditional assignment of 'X-Org-Id' header may result in a nil value.
Omitting the header when not required will prevent sending a nil value, reducing the risk of issues for downstream consumers.
</issue_to_address>
### Comment 2
<location> `lib/foreman_inventory_upload/scripts/uploader.sh.erb:31-34` </location>
<code_context>
AUTH_VAL="\"$RH_USERNAME\":\"$RH_PASSWORD\""
fi
+ORG_HEADER=""
+if [ -n "$ORG_ID" ]
+then
+ ORG_HEADER="-H \"X-Org-Id: $ORG_ID\""
+fi
+
# /tmp/a b/x.pem
</code_context>
<issue_to_address>
**suggestion (bug_risk):** Quoting in the curl command may result in literal quotes in the header value.
Escaped double quotes in ORG_HEADER will be included in the header value. Remove them to avoid sending extra quotes in the request.
```suggestion
if [ -n "$ORG_ID" ]
then
ORG_HEADER='-H "X-Org-Id: '"$ORG_ID"'"'
fi
```
</issue_to_address>
### Comment 3
<location> `lib/foreman_inventory_upload/scripts/uploader.sh.erb:45` </location>
<code_context>
for f in $FILES
do
- curl -k -vvv -# --fail -F "file=@$f;type=application/vnd.redhat.qpc.tar+tgz" $DEST "$AUTH_KEY" "$AUTH_VAL"
+ curl -k -vvv -# --fail -F "file=@$f;type=application/vnd.redhat.qpc.tar+tgz" $DEST "$AUTH_KEY" "$AUTH_VAL" "$ORG_HEADER"
status=$?
if [ $status -eq 0 ]; then
</code_context>
<issue_to_address>
**suggestion (bug_risk):** Passing an empty string as a curl argument may cause unexpected behavior.
Conditionally add ORG_HEADER to the curl command only if it is non-empty to avoid potential errors.
</issue_to_address>
### Comment 4
<location> `lib/insights_cloud/async/vmaas_reposcan_sync.rb:26-28` </location>
<code_context>
end
- plan_self
+ organization_id = Katello::Repository.find(repo_id).organization_id
+
+ plan_self(organization_id: organization_id)
</code_context>
<issue_to_address>
**suggestion (bug_risk):** Potential for exception if repository is not found.
Consider using 'find_by' or adding error handling to prevent unhandled exceptions if the repository is missing.
```suggestion
repository = Katello::Repository.find_by(id: repo_id)
unless repository
Rails.logger.warn("Repository with id #{repo_id} not found. Skipping plan_self.")
return
end
organization_id = repository.organization_id
plan_self(organization_id: organization_id)
```
</issue_to_address>
### Comment 5
<location> `test/unit/lib/insights_cloud/async/vmaas_reposcan_sync_test.rb:80-81` </location>
<code_context>
params[:url] == @expected_url &&
params[:headers].is_a?(Hash) &&
- params[:headers]['Content-Type'] == 'application/json'
+ params[:headers]['Content-Type'] == 'application/json' &&
+ params[:organization] == @repo.organization
end
.returns(mock_response)
</code_context>
<issue_to_address>
**suggestion (testing):** Test does not verify that the X-Org-Id header is present and correct.
Add an assertion to verify that params[:headers]['X-Org-Id'] equals @repo.organization.label.
</issue_to_address>
### Comment 6
<location> `test/unit/lib/insights_cloud/async/vmaas_reposcan_sync_test.rb:10-17` </location>
<code_context>
- @repo_payload = { id: 123 }
+ @root = FactoryBot.build(:katello_root_repository, :fedora_17_x86_64_dev_root)
+ @root.save(validate: false)
+ @repo = FactoryBot.create(
+ :katello_repository,
+ :with_product,
+ distribution_family: 'Red Hat',
+ distribution_version: '7.5',
+ root: @root
+ )
+ @repo_payload = { id: @repo.id }
@expected_url = 'https://example.com/api/v1/vmaas/reposcan/sync'
InsightsCloud.stubs(:vmaas_reposcan_sync_url).returns(@expected_url)
</code_context>
<issue_to_address>
**suggestion (testing):** Edge cases for organization context are not covered.
Add tests for scenarios where the repository lacks an organization or the organization label is nil or blank to verify proper handling in header injection.
Suggested implementation:
```ruby
setup do
@root = FactoryBot.build(:katello_root_repository, :fedora_17_x86_64_dev_root)
@root.save(validate: false)
@repo = FactoryBot.create(
:katello_repository,
:with_product,
distribution_family: 'Red Hat',
distribution_version: '7.5',
root: @root
)
@repo_payload = { id: @repo.id }
@expected_url = 'https://example.com/api/v1/vmaas/reposcan/sync'
InsightsCloud.stubs(:vmaas_reposcan_sync_url).returns(@expected_url)
ForemanRhCloud.stubs(:with_iop_smart_proxy?).returns(true)
params[:method] == :put &&
params[:url] == @expected_url &&
params[:headers].is_a?(Hash) &&
params[:headers]['Content-Type'] == 'application/json' &&
params[:organization] == @repo.organization
end
.returns(mock_response)
end
test "header injection when repository has no organization" do
repo_without_org = FactoryBot.create(
:katello_repository,
:with_product,
distribution_family: 'Red Hat',
distribution_version: '7.5',
root: @root,
organization: nil
)
repo_payload = { id: repo_without_org.id }
expected_url = 'https://example.com/api/v1/vmaas/reposcan/sync'
InsightsCloud.stubs(:vmaas_reposcan_sync_url).returns(expected_url)
ForemanRhCloud.stubs(:with_iop_smart_proxy?).returns(true)
# Simulate header injection
headers = InsightsCloud.send(:build_headers, repo_without_org)
assert_nil headers['Organization'], "Organization header should be nil when repository has no organization"
end
test "header injection when organization label is nil" do
org = FactoryBot.create(:organization, label: nil)
repo_with_nil_label = FactoryBot.create(
:katello_repository,
:with_product,
distribution_family: 'Red Hat',
distribution_version: '7.5',
root: @root,
organization: org
)
headers = InsightsCloud.send(:build_headers, repo_with_nil_label)
assert_nil headers['Organization'], "Organization header should be nil when organization label is nil"
end
test "header injection when organization label is blank" do
org = FactoryBot.create(:organization, label: '')
repo_with_blank_label = FactoryBot.create(
:katello_repository,
:with_product,
distribution_family: 'Red Hat',
distribution_version: '7.5',
root: @root,
organization: org
)
headers = InsightsCloud.send(:build_headers, repo_with_blank_label)
assert_nil headers['Organization'], "Organization header should be nil when organization label is blank"
end
```
- Ensure that the `build_headers` method in `InsightsCloud` correctly handles cases where the organization or its label is nil or blank, returning nil for the 'Organization' header.
- If the header key is named differently (e.g., 'x-organization' or similar), adjust the test assertions accordingly.
- If FactoryBot traits or attributes differ in your codebase, update the organization and repository creation to match your conventions.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
ShimShtein
force-pushed
the
force_iop_org
branch
2 times, most recently
from
7afbfcc to
8838167
Compare
Member
Author
|
Ready for another round |
ShimShtein
force-pushed
the
force_iop_org
branch
from
8838167 to
f53ed87
Compare
ekohl
reviewed
Sep 25, 2025
ShimShtein
force-pushed
the
force_iop_org
branch
2 times, most recently
from
db3d34d to
4cab066
Compare
Member
Author
|
@ekohl ready for another round |
ekohl
reviewed
Sep 25, 2025
Member
ekohl
left a comment
ekohl
left a comment
There was a problem hiding this comment.
This was a really superficial review. Minor nits inline, but otherwise no objections. I'm not familiar enough with the rest of the codebase to know if this is complete or if anything is missing. I'd appreciate anyone else did a more thorough review.
ShimShtein
force-pushed
the
force_iop_org
branch
from
4cab066 to
7e08490
Compare
Member
Author
|
Done |
ekohl
reviewed
Sep 25, 2025
Member
ekohl
left a comment
ekohl
left a comment
There was a problem hiding this comment.
Consider my concerns addressed but I don't feel I know it well enough to ack
Collaborator
|
The uploader.sh hits some nightmare erb=> bash quoting issue diff --git a/lib/foreman_inventory_upload/scripts/uploader.sh.erb b/lib/foreman_inventory_upload/scripts/uploader.sh.erb
index fbadaf7..66858b1 100644
--- a/lib/foreman_inventory_upload/scripts/uploader.sh.erb
+++ b/lib/foreman_inventory_upload/scripts/uploader.sh.erb
@@ -27,10 +27,10 @@ else
AUTH_VAL="\"$RH_USERNAME\":\"$RH_PASSWORD\""
fi
-ORG_HEADER=""
+ORG_HEADER=()
if [ -n "$ORG_ID" ]
then
- ORG_HEADER='-H "X-Org-Id: '"$ORG_ID"'"'
+ ORG_HEADER=("-H" "X-Org-Id: $ORG_ID")
fi
# /tmp/a b/x.pem
@@ -42,7 +42,7 @@ mkdir -p $DONE_DIR
for f in $FILES
do
- curl -k -vvv -# --fail -F "file=@$f;type=application/vnd.redhat.qpc.tar+tgz" $DEST "$AUTH_KEY" "$AUTH_VAL" "$ORG_HEADER"
+ curl -k -vvv -# --fail -F "file=@$f;type=application/vnd.redhat.qpc.tar+tgz" $DEST "$AUTH_KEY" "$AUTH_VAL" "${ORG_HEADER[@]}"
status=$?
if [ $status -eq 0 ]; then
mv $f $DONE_DIR
|
ShimShtein
force-pushed
the
force_iop_org
branch
from
486677e to
83c0238
Compare
Member
Author
|
@parthaa incorporated your patch. Didn't test it though. |
parthaa
approved these changes
Oct 1, 2025
Collaborator
There was a problem hiding this comment.
APJ.
- updated the gateway container on my iop box
- rm'ed
/var/lib/foreman/redhat_inventory/uploads/uploader.sh - registered host again (which should regenerate the
uploader.shand run it) - then ran the following query
$ sudo -u postgres psql -d advisor_db -c "select id, org_id from inventory.hosts"
id | org_id
--------------------------------------+----------------------
86870b59-e45c-4f4e-9b15-914a44af4dbd | Default_Organization
(1 row)
- Saw all the recommendations etc for it.
$ curl -H "X-Org-Id: Default_Organization" --cert ./foreman-certs/client_cert.pem --key foreman-certs/client_key.pem "https://localhost:24443/api/inventory/v1/hosts"|jq
% Total % Received % Xferd Average Speed Time Time Time Current
Dload Upload Total Spent Left Speed
100 1226 100 1226 0 0 35028 0 --:--:-- --:--:-- --:--:-- 35028
{
"total": 1,
"count": 1,
"page": 1,
"per_page": 50,
"results": [
{
"insights_id": "86870b59-e45c-4f4e-92215-914a44af4dbd",
"subscription_manager_id": "868722259-e45c-4f4e-9b15-914a44af4dbd",
"satellite_id": "86870b59-e45c-4f4e-9225-914a44af4dbd",
"bios_uuid": "ee69b1d0-4622-494d-ac22-090e8664b85c",
"ip_addresses": [
"192.168.122.164"
],
"fqdn": "another-rhel9.paji.example.com",
"mac_addresses": [
"52:54:00:ef:72:7a"
],
"provider_id": null,
"provider_type": null,
"id": "86870b59-e45c-4f4e-9b15-914a44af4dbd",
"account": "",
"org_id": "Default_Organization",
"display_name": "another-rhel9.example.com",
"ansible_host": null,
"facts": [],
"reporter": "puptoo",
"per_reporter_staleness": {
"puptoo": {
"last_check_in": "2025-10-01T16:56:31.267338+00:00",
"stale_timestamp": "2025-10-02T21:56:31.267338+00:00",
"culled_timestamp": "2025-10-15T16:56:31.267338+00:00",
"check_in_succeeded": true,
"stale_warning_timestamp": "2025-10-08T16:56:31.267338+00:00"
}
},
"stale_timestamp": "2025-10-02T21:56:31.267338+00:00",
"stale_warning_timestamp": "2025-10-08T16:56:31.267338+00:00",
"culled_timestamp": "2025-10-15T16:56:31.267338+00:00",
"created": "2025-10-01T16:56:31.273217+00:00",
"updated": "2025-10-01T17:06:53.728003+00:00",
"last_check_in": "2025-10-01T16:56:31.267338+00:00",
"groups": []
}
]
}ACK on this but would like @ShimShtein to add more tests in a follow up PR.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
6 participants
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
What are the changes introduced in this pull request?
Considerations taken when implementing this change?
What are the testing steps for this pull request?
Summary by Sourcery
Include X-Org-Id headers in cloud API calls and scripts by extracting organization context, refactor auth modules to centralize header injection, and update tests accordingly.
New Features:
Enhancements:
Tests: