-
Notifications
You must be signed in to change notification settings - Fork 5
[SREP-1814] - Initial working commit for protected labels. #94
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
base: main
Are you sure you want to change the base?
Conversation
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: Mhodesty The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #94 +/- ##
==========================================
+ Coverage 61.44% 62.95% +1.51%
==========================================
Files 6 6
Lines 708 737 +29
==========================================
+ Hits 435 464 +29
Misses 232 232
Partials 41 41
🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I added a few PR nits.
I also noticed tests are failing because of rpms-signature-scan
errors.
I think we already fixed that on main (had to merge a PR) - you may need to rebase?
internal/api/server.go
Outdated
// validateProtectedFields checks if the user is trying to modify protected fields/labels | ||
func validateProtectedFields(request *v1.UpdateProbeJSONRequestBody) error { | ||
// Check if user is trying to modify protected labels | ||
if err := validateProtectedLabels(request.Labels); err != nil { | ||
return err | ||
} | ||
|
||
// Check if user is trying to modify status (except for deletion) | ||
if request.Status != nil { | ||
// Only allow status change to "deleted" for deletion functionality | ||
if *request.Status != v1.Deleted { | ||
return fmt.Errorf("modification of status field is forbidden - it's managed by the system (only deletion via 'deleted' status is allowed)") | ||
} | ||
} | ||
|
||
return nil | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Modifying the "status" data object IS allowed.
We want to let RMO set a terminating
status and the Synthetic Agent changes state to active
/ failed
.
It's only the status label:
that we're concerned about protecting, and you did that above (aka: probeStatusLabelKey
).
It's a little confusing, but you'll see:
"status": "active"
in the `probe-config.json - this is the thing we want to allow to be modified.rhobs-synthetics/status: active
in thelabels:
- that's what we're protecting.
ocm backplane elevate "SREP-333" -- get cm -n rhobs-int probe-config-4dd39ee5-42d1-45cc-8b5c-49b1fd8907dc -o yaml
apiVersion: v1
data:
probe-config.json: '{"id":"4dd39ee5-42d1-45cc-8b5c-49b1fd8907dc","labels":{"cluster-id":"361327b1-5a11-4f06-b75f-268ba6e99dcc","private":"false","tenant":"hcp"},"static_url":"https://api.cs-ci-m7dqj.af4m.i3.devshift.org/livez","status":"active"}'
kind: ConfigMap
metadata:
creationTimestamp: "2025-09-16T01:20:45Z"
labels:
app: rhobs-synthetics-probe
cluster-id: 361327b1-5a11-4f06-b75f-268ba6e99dcc
private: "false"
rhobs-synthetics/static-url-hash: 47d80a225655474a41c076ff2f21fe6c940f93a8e3c848b08bc341afa0ebce0
rhobs-synthetics/status: active
tenant: hcp
name: probe-config-4dd39ee5-42d1-45cc-8b5c-49b1fd8907dc
namespace: rhobs-int
resourceVersion: "119427894"
uid: 407d596d-17a8-4985-9441-689ef1c169d8
TLDR; We don't need validateProtectedFields()
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi Jim.
I know this is a few weeks old but could you explain what you mean here because I am confused.
You're saying that you want us to still be able to run patch commands on the API to change the status of the probe, but you want me to put something in place so the configMap can't be modified?
internal/api/server.go
Outdated
defer metrics.RecordProbestoreRequest("update_probe", time.Now()) | ||
|
||
// Validate that protected fields/labels are not being modified - return 403 if they are | ||
if err := validateProtectedFields(request.Body); err != nil { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should be updated to only call validateProtectedLabels()
internal/api/server.go
Outdated
} | ||
|
||
// Now, update the fields from the request. | ||
// Handle status updates (only "deleted" is allowed, validated above) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Revert
@Mhodesty: The following tests failed, say
Full PR test history. Your PR dashboard. 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-sigs/prow repository. I understand the commands that are listed here. |
Initial working version of protected labels for the rhobs API. This prevents users from manually patching these values.