Allow full profile updates through the PATCH handler #2990

jhrozek · 2024-04-08T08:33:38Z

Summary

We used to use a bespoke PATCH handler to update profiles but the PATCH
method only allowed changing alert and remediate. We can make the code
simpler and allow more updates by modifying the existing profile using
the UpdateMask and then just relaying the update to the existing
UpdateProfile method of the Profile service.

Fixes: #2971

Change Type

Mark the type of change your PR introduces:

Bug fix (resolves an issue without affecting existing features)
Feature (adds new functionality without breaking changes)
Breaking change (may impact existing functionalities or require documentation updates)
Documentation (updates or additions to documentation)
Refactoring or test improvements (no bug fixes or new functionality)

Testing

There are unit tests but you can also test with curl:

Adding a rule to a profile:

curl -XPATCH -H "Authorization: Bearer $MINDER_BEARER_TOKEN"
'http://localhost:8080/api/v1/profile/ce746798-c30c-46f3-8a12-0654747b49ec?context.project=1900c9f6-ef0f-43e4-8af7-108beb72de0e&context.provider=github'
-d '{"name":"dependabot-enabled-profile", "labels":[], "buildEnvironment":[], "repository":[{"type":"dependabot_configured","params":null,"def":{"apply_if_file":"requirements.txt","package_ecosystem":"pypi","schedule_interval":"weekly"},"name":"dependabot_configured"},{"type":"dependabot_configured","params":null,"def":{"apply_if_file":"go.mod","package_ecosystem":"gomod","schedule_interval":"weekly"},"name":"dependabot_configured_go"}], "pullRequest":[], "remediate ":"on", "alert":"off", "type":"", "version":"", "displayName":"gah" }

Remove the rule back:

curl -XPATCH -H "Authorization: Bearer $MINDER_BEARER_TOKEN"
'http://localhost:8080/api/v1/profile/ce746798-c30c-46f3-8a12-0654747b49ec?context.project=1900c9f6-ef0f-43e4-8af7-108beb72de0e&context.provider=github'
-d '{"name":"dependabot-enabled-profile", "labels":[], "buildEnvironment":[], "repository":[{"type":"dependabot_configured","params":null,"def":{"apply_if_file":"requirements.txt","package_ecosystem":"pypi","schedule_interval":"weekly"},"name":"dependabot_configured"}], "pullRequest":[], "remediate":"on", "alert":"off", "type":"", "version":"", "displayName":"gah"}}'

Review Checklist:

Reviewed my own code for quality and clarity.
Added comments to complex or tricky code sections.
Updated any affected documentation.
Included tests that validate the fix or feature.
Checked that related changes are merged.

jhrozek · 2024-04-08T08:34:29Z

internal/controlplane/handlers_profile.go

-	if err != nil {
-		return nil, util.UserVisibleError(codes.Internal, "error updating profile: %v", err)
-	}
+	for _, attr := range updateMask.Paths {


should we explicitly disallow patching some fields here? I think it's OK to leave that to the Update but wanted to check explicitly...

Subscription ID is one which we do not want to allow to be changed.

coveralls · 2024-04-08T08:41:52Z

coverage: 49.69% (+1.8%) from 47.861%
when pulling ebc3a37 on jhrozek:patch
into 5964281 on stacklok:main.

internal/controlplane/handlers_profile.go

dmjb

Leaving this as "Request changes" until the question around editing bundle profiles is settled.

dmjb · 2024-04-08T14:13:51Z

After talking to @jhrozek offline, I understand why that check can be deleted. I would like to see a test added for the bundled subscription ID case though.

I forgot to issue a profile update message on patching a profile. Since the profile update message is currently nicely encapsulated in `sendNewProfileEvent` which is a private method of the Profile service and we already have PR mindersec#2990 that would move all of the Patch functionality to that service and remove the current Patch handler implementation, I decided to create a bespoke message for now. This code will be removed once we merge PR mindersec#2990.

I forgot to issue a profile update message on patching a profile. Since the profile update message is currently nicely encapsulated in `sendNewProfileEvent` which is a private method of the Profile service and we already have PR #2990 that would move all of the Patch functionality to that service and remove the current Patch handler implementation, I decided to create a bespoke message for now. This code will be removed once we merge PR #2990.

jhrozek · 2024-04-09T21:04:15Z

After talking to @jhrozek offline, I understand why that check can be deleted. I would like to see a test added for the bundled subscription ID case though.

I added a test and moved a bunch of the patch handler to profile service.

btw this PR is NOT important to merge at this moment

evankanderson

Only nits; I like the new code much better! Thanks!

evankanderson · 2024-04-22T13:30:47Z

internal/profiles/service.go

+	ctx context.Context,
+	projectID uuid.UUID,
+	profileID uuid.UUID,
+	subscriptionID uuid.UUID,


Do we want to allow PATCH with subscriptions? If not, could we drop this argument?

(It looks like we pass uuid.Nil as the argument to UpdateProfile from the other gRPC endpoint, so it looks like we the PATCH-on-Subscription codepath is effectively dead...)

probably not, you're right. We'll likely to only update subscription profiles through a replace, not patch.

evankanderson · 2024-04-22T13:35:27Z

internal/profiles/service.go

+		oldReflect := oldProfilePb.ProtoReflect()
+		patchReflect := patchPb.ProtoReflect()


Do you want to do these inside the for loop?

thanks, fixed

evankanderson · 2024-04-22T13:37:08Z

internal/profiles/service.go

+	updateMask *fieldmaskpb.FieldMask,
+	qtx db.Querier,
+) (*minderv1.Profile, error) {
+	oldProfilePb, err := getProfilePBFromDB(ctx, profileID, projectID, qtx)


Do you want to call old "base" here? It's slightly confusing, because we go through the steps and then pass old to the Update method.

thanks, renamed

evankanderson · 2024-04-22T13:37:52Z

internal/profiles/service.go

+}
+
+func patchProfilePb(oldProfilePb, patchPb *minderv1.Profile, updateMask *fieldmaskpb.FieldMask) {
+	// if there is no update mask, return the old profile. The grpc-rest gateway always sets the update mask


This is confusing, as it says "return", but there's no return value. Maybe you mean "an empty field mask means apply no changes"?

I think the original version of the code was trying to return a modified profile instead and I didn't update the comment.
Thanks, fixed.

We used to use a bespoke PATCH handler to update profiles but the PATCH method only allowed changing alert and remediate. We can make the code simpler and allow more updates by modifying the existing profile using the UpdateMask and then just relaying the update to the existing UpdateProfile method of the Profile service. Examples: Adding a rule to a profile: ``` curl -XPATCH -H "Authorization: Bearer $MINDER_BEARER_TOKEN" 'http://localhost:8080/api/v1/profile/ce746798-c30c-46f3-8a12-0654747b49ec?context.project=1900c9f6-ef0f-43e4-8af7-108beb72de0e&context.provider=github' -d '{"name":"dependabot-enabled-profile", "labels":[], "buildEnvironment":[], "repository":[{"type":"dependabot_configured","params":null,"def":{"apply_if_file":"requirements.txt","package_ecosystem":"pypi","schedule_interval":"weekly"},"name":"dependabot_configured"},{"type":"dependabot_configured","params":null,"def":{"apply_if_file":"go.mod","package_ecosystem":"gomod","schedule_interval":"weekly"},"name":"dependabot_configured_go"}], "pullRequest":[], "remediate ":"on", "alert":"off", "type":"", "version":"", "displayName":"gah" } ``` Remove the rule back: ``` curl -XPATCH -H "Authorization: Bearer $MINDER_BEARER_TOKEN" 'http://localhost:8080/api/v1/profile/ce746798-c30c-46f3-8a12-0654747b49ec?context.project=1900c9f6-ef0f-43e4-8af7-108beb72de0e&context.provider=github' -d '{"name":"dependabot-enabled-profile", "labels":[], "buildEnvironment":[], "repository":[{"type":"dependabot_configured","params":null,"def":{"apply_if_file":"requirements.txt","package_ecosystem":"pypi","schedule_interval":"weekly"},"name":"dependabot_configured"}], "pullRequest":[], "remediate":"on", "alert":"off", "type":"", "version":"", "displayName":"gah"}}' ``` Fixes: mindersec#2971

jhrozek · 2024-04-22T14:15:51Z

@evankanderson thanks for the review, I fixed your comments in separate patches for easier review.

jhrozek requested a review from a team as a code owner April 8, 2024 08:33

jhrozek commented Apr 8, 2024

View reviewed changes

dmjb reviewed Apr 8, 2024

View reviewed changes

internal/controlplane/handlers_profile.go Show resolved Hide resolved

dmjb requested changes Apr 8, 2024

View reviewed changes

jhrozek mentioned this pull request Apr 9, 2024

Issue a bespoke profile update message on patching a profile #3012

Merged

10 tasks

jhrozek force-pushed the patch branch from 2d8dbf5 to d340c14 Compare April 9, 2024 21:02

jhrozek mentioned this pull request Apr 18, 2024

Don't use SELECT * when joining profile tables #3130

Merged

10 tasks

evankanderson previously approved these changes Apr 22, 2024

View reviewed changes

jhrozek added 5 commits April 22, 2024 16:03

Drop the subscriptionID

17ead77

Move code that doesn't change in a for loop out of that for loop

a669f63

Rename variable to better signal its intent

95738e0

Reword a confusing comment

ebc3a37

jhrozek dismissed evankanderson’s stale review via ebc3a37 April 22, 2024 14:14

jhrozek force-pushed the patch branch from d340c14 to ebc3a37 Compare April 22, 2024 14:14

dmjb approved these changes Apr 22, 2024

View reviewed changes

jhrozek merged commit 17412a2 into mindersec:main Apr 22, 2024
20 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Allow full profile updates through the PATCH handler #2990

Allow full profile updates through the PATCH handler #2990

jhrozek commented Apr 8, 2024

jhrozek Apr 8, 2024

dmjb Apr 8, 2024

coveralls commented Apr 8, 2024 •

edited

Loading

dmjb left a comment

dmjb commented Apr 8, 2024

jhrozek commented Apr 9, 2024

evankanderson left a comment

evankanderson Apr 22, 2024

evankanderson Apr 22, 2024

jhrozek Apr 22, 2024

evankanderson Apr 22, 2024

jhrozek Apr 22, 2024

evankanderson Apr 22, 2024

jhrozek Apr 22, 2024

evankanderson Apr 22, 2024

jhrozek Apr 22, 2024

jhrozek commented Apr 22, 2024

		oldReflect := oldProfilePb.ProtoReflect()
		patchReflect := patchPb.ProtoReflect()

Allow full profile updates through the PATCH handler #2990

Allow full profile updates through the PATCH handler #2990

Conversation

jhrozek commented Apr 8, 2024

Summary

Change Type

Testing

Review Checklist:

Choose a reason for hiding this comment

Choose a reason for hiding this comment

coveralls commented Apr 8, 2024 • edited Loading

dmjb left a comment

Choose a reason for hiding this comment

dmjb commented Apr 8, 2024

jhrozek commented Apr 9, 2024

evankanderson 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

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

jhrozek commented Apr 22, 2024

coveralls commented Apr 8, 2024 •

edited

Loading