Powerflex pre-approved GUIDs #208

ashleyvjoy · 2023-01-27T12:26:49Z

Description

The sdcApproveHandler will allow/deny approve SDC request coming from driverside to the powerflex array based on the Tenant data
A new approvesdc boolean flag(default value is true) is added to karavictl tenant create
A new karavictl tenant update command to update the approvesdc flag value
karavictl tenant get updated

GitHub Issues

List the GitHub issues impacted by this PR:

| GitHub Issue # |
| dell/csm#402 |

Checklist:

I have performed a self-review of my own code to ensure there are no formatting, vetting, linting, or security issues
I have verified that new and existing unit tests pass locally with my changes
I have not allowed coverage numbers to degenerate
I have maintained at least 90% code coverage
I have commented my code, particularly in hard-to-understand areas
I have made corresponding changes to the documentation
I have added tests that prove my fix is effective or that my feature works
Backward compatibility is not broken

How Has This Been Tested?

Please describe the tests that you ran to verify your changes. Please also list any relevant details for your test configuration

Test A
Test B

logs.txt

csmbot · 2023-01-27T12:27:01Z

Can one of the admins verify this patch?

sharmilarama

Move merge target from main to a holding branch for 1.6, until patch release is in progress.

cmd/karavictl/cmd/tenant_update.go

cmd/karavictl/cmd/tenant_update_test.go

internal/tenantsvc/service.go

internal/sdc/sdc_approve.go

policies/sdc_approve.rego

atye · 2023-02-17T13:45:50Z

internal/proxy/powerflex_handler.go

+		}
+		defer r.Body.Close()
+
+		var requestBody map[string]json.RawMessage


It doesn't look like the requestBody is used or modified in this handler. If we're not doing that, we don't need to read it.

On a second look, we are doing this in other handler so it's ok to leave this. We will have the request body ready for the future incase we need to modify it.

Okay. Left this

atye · 2023-02-17T13:46:38Z

internal/sdc/sdc_approve.go

+
+// ApproveSdcField returns the redis formatted approved capacity field.
+func (r Request) ApproveSdcField() string {
+	return "Approvesdc"


The existing fields in Redis are lowercase with underscores. Can this be changed to approve_sdc?

cmd/karavictl/cmd/tenant.go

internal/tenantsvc/service.go

sharmilarama · 2023-02-17T14:07:38Z

cmd/karavictl/cmd/tenant_update.go

@@ -0,0 +1,78 @@
+// Copyright © 2021-2023 Dell Inc., or its subsidiaries. All Rights Reserved.


Please remove 2021

sharmilarama · 2023-02-17T14:08:11Z

cmd/karavictl/cmd/tenant_update_test.go

@@ -0,0 +1,165 @@
+// Copyright © 2021-2023 Dell Inc., or its subsidiaries. All Rights Reserved.


same comment as above

sharmilarama · 2023-02-17T15:27:53Z

internal/sdc/fake.go

@@ -0,0 +1,32 @@
+// Copyright © 2021-2023 Dell Inc., or its subsidiaries. All Rights Reserved.


Remove 2021

sharmilarama · 2023-02-17T15:28:01Z

internal/sdc/sdc_approve.go

@@ -0,0 +1,130 @@
+// Copyright © 2021-2023 Dell Inc., or its subsidiaries. All Rights Reserved.


Remove 2021

sharmilarama · 2023-02-17T15:28:29Z

internal/sdc/sdc_approve_test.go

@@ -0,0 +1,177 @@
+// Copyright © 2021-2023 Dell Inc., or its subsidiaries. All Rights Reserved.


remove 2021

shaynafinocchiaro · 2023-02-20T22:30:40Z

internal/proxy/powerflex_handler.go

+		ans, err := decision.Can(func() decision.Query {
+			return decision.Query{
+				Host: opaHost,
+				// TODO(ian): This will need to be namespaced under "powerflex".


can this comment be removed?

shaynafinocchiaro · 2023-02-20T22:31:27Z

internal/sdc/sdc_approve.go

+
+// Request is a request to redis.
+type Request struct {
+	//SystemID string `json:"systemId"`


if systemID isn't required here, can it be removed?

shaynafinocchiaro · 2023-02-20T22:33:48Z

policies/sdc_approve.rego

@@ -0,0 +1,26 @@
+package karavi.sdc.approve


are there plans to add this new sdc-approve policy to the helm-charts and csm operator authorization deployments?

Added the support in helm-charts and csm-operator

prablr79 · 2023-02-28T08:45:47Z

@coulof @bjiang27 @xuluna need your quick approval, since all the test are passed. see if you can approve this PR.

coulof

Giving an approval per request but I'm not qualified to audit the code.

internal/tenantsvc/service.go

cmd/karavictl/cmd/tenant.go

prablr79 · 2023-02-28T11:58:55Z

@ashleyvjoy @khareRajshree can you quickly update and revert ?

ashleyvjoy · 2023-02-28T12:25:54Z

@ashleyvjoy @khareRajshree can you quickly update and revert ?

The variable name is self explanatory in the context of the function and Novus had already reviewed it. Regarding the copyright on the tenant.go file, can raise a new PR for copyright update without affecting the progress of this PR. Discussed it with Bharat

tdawe · 2023-02-28T12:47:22Z

deploy/airgap-prepare.sh

@@ -13,7 +13,7 @@
 # limitations under the License.

 ARCH=amd64
-SIDECAR_DOCKER_TAG=1.5.1
+SIDECAR_DOCKER_TAG=nightly


Does this need to change to 1.6.0?

The last information I got from Novus yesterday was that to keep that as nightly

that will be addressed in another PR

sharmilarama · 2023-02-28T13:01:07Z

run e2e test

alikdell · 2023-02-28T13:31:48Z

run e2e test

ashleyvjoy requested review from shaynafinocchiaro, atye, IsaiasA1 and sharmilarama January 27, 2023 12:45

sharmilarama previously requested changes Jan 27, 2023

View reviewed changes

ashleyvjoy requested review from Deepak-Ghivari, Prabhu-Dell, VamsiSiddu-7 and sharmilarama January 30, 2023 06:40

atye reviewed Feb 17, 2023

View reviewed changes

cmd/karavictl/cmd/tenant.go Outdated Show resolved Hide resolved

atye reviewed Feb 17, 2023

View reviewed changes

internal/tenantsvc/service.go Outdated Show resolved Hide resolved

sharmilarama reviewed Feb 17, 2023

View reviewed changes

ashleyvjoy marked this pull request as ready for review February 20, 2023 13:01

ashleyvjoy requested review from gallacher, tdawe, alikdell, hoppea2, coulof, EvgenyUglov, bjiang27, aqu-dell and xuluna as code owners February 20, 2023 13:01

ashleyvjoy changed the base branch from main to release-v1.6.0 February 20, 2023 13:47

ashleyvjoy requested review from atye and sharmilarama February 20, 2023 13:48

IsaiasA1 previously approved these changes Feb 20, 2023

View reviewed changes

shaynafinocchiaro reviewed Feb 20, 2023

View reviewed changes

shaynafinocchiaro previously approved these changes Feb 27, 2023

View reviewed changes

squashing all commits into one

45ae196

ashleyvjoy dismissed stale reviews from shaynafinocchiaro and sharmilarama via 45ae196 February 28, 2023 05:44

ashleyvjoy force-pushed the approvesdc-poc branch from b291b3b to 45ae196 Compare February 28, 2023 05:44

Merge branch 'main' into approvesdc-poc

f475a9b

VamsiSiddu-7 approved these changes Feb 28, 2023

View reviewed changes

Deepak-Ghivari approved these changes Feb 28, 2023

View reviewed changes

khareRajshree approved these changes Feb 28, 2023

View reviewed changes

coulof approved these changes Feb 28, 2023

View reviewed changes

ashleyvjoy requested review from sharmilarama and atye February 28, 2023 09:35

bharathsreekanth reviewed Feb 28, 2023

View reviewed changes

internal/tenantsvc/service.go Outdated Show resolved Hide resolved

cmd/karavictl/cmd/tenant.go Show resolved Hide resolved

tdawe reviewed Feb 28, 2023

View reviewed changes

sharmilarama approved these changes Feb 28, 2023

View reviewed changes

tdawe approved these changes Feb 28, 2023

View reviewed changes

shaynafinocchiaro approved these changes Feb 28, 2023

View reviewed changes

alikdell approved these changes Feb 28, 2023

View reviewed changes

dell deleted a comment from shaynafinocchiaro Feb 28, 2023

atye approved these changes Feb 28, 2023

View reviewed changes

dell deleted a comment from atye Feb 28, 2023

atye closed this Feb 28, 2023

atye reopened this Feb 28, 2023

atye closed this Feb 28, 2023

anandrajak1 deleted the approvesdc-poc branch October 17, 2024 19:03

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Powerflex pre-approved GUIDs #208

Powerflex pre-approved GUIDs #208

ashleyvjoy commented Jan 27, 2023 •

edited

Loading

csmbot commented Jan 27, 2023

sharmilarama left a comment

atye Feb 17, 2023

atye Feb 17, 2023

ashleyvjoy Feb 20, 2023

atye Feb 17, 2023 •

edited

Loading

ashleyvjoy Feb 20, 2023

sharmilarama Feb 17, 2023

ashleyvjoy Feb 20, 2023

sharmilarama Feb 17, 2023

ashleyvjoy Feb 20, 2023

sharmilarama Feb 17, 2023

ashleyvjoy Feb 20, 2023

sharmilarama Feb 17, 2023

ashleyvjoy Feb 20, 2023

sharmilarama Feb 17, 2023

ashleyvjoy Feb 20, 2023

shaynafinocchiaro Feb 20, 2023

ashleyvjoy Feb 21, 2023

shaynafinocchiaro Feb 20, 2023

ashleyvjoy Feb 21, 2023

shaynafinocchiaro Feb 20, 2023

ashleyvjoy Feb 21, 2023 •

edited

Loading

prablr79 commented Feb 28, 2023

coulof left a comment

prablr79 commented Feb 28, 2023

ashleyvjoy commented Feb 28, 2023

tdawe Feb 28, 2023

ashleyvjoy Feb 28, 2023 •

edited

Loading

sharmilarama Feb 28, 2023

sharmilarama commented Feb 28, 2023

alikdell commented Feb 28, 2023

		@@ -0,0 +1,78 @@
		// Copyright © 2021-2023 Dell Inc., or its subsidiaries. All Rights Reserved.

		@@ -0,0 +1,165 @@
		// Copyright © 2021-2023 Dell Inc., or its subsidiaries. All Rights Reserved.

		@@ -0,0 +1,32 @@
		// Copyright © 2021-2023 Dell Inc., or its subsidiaries. All Rights Reserved.

		@@ -0,0 +1,130 @@
		// Copyright © 2021-2023 Dell Inc., or its subsidiaries. All Rights Reserved.

		@@ -0,0 +1,177 @@
		// Copyright © 2021-2023 Dell Inc., or its subsidiaries. All Rights Reserved.

Powerflex pre-approved GUIDs #208

Powerflex pre-approved GUIDs #208

Conversation

ashleyvjoy commented Jan 27, 2023 • edited Loading

Description

GitHub Issues

Checklist:

How Has This Been Tested?

csmbot commented Jan 27, 2023

sharmilarama 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

atye Feb 17, 2023 • edited Loading

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

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

ashleyvjoy Feb 21, 2023 • edited Loading

Choose a reason for hiding this comment

prablr79 commented Feb 28, 2023

coulof left a comment

Choose a reason for hiding this comment

prablr79 commented Feb 28, 2023

ashleyvjoy commented Feb 28, 2023

Choose a reason for hiding this comment

ashleyvjoy Feb 28, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sharmilarama commented Feb 28, 2023

alikdell commented Feb 28, 2023

ashleyvjoy commented Jan 27, 2023 •

edited

Loading

atye Feb 17, 2023 •

edited

Loading

ashleyvjoy Feb 21, 2023 •

edited

Loading

ashleyvjoy Feb 28, 2023 •

edited

Loading