Auth proxy status fix #465

jooseppi-luna · 2024-02-13T22:50:28Z

Description

Currently, the authorization proxy server status is always failed in csm-operator. This is for a couple reasons:

We are attempting to calculate the status of the daemonset, but since auth proxy has no daemonset, it always fails. To fix this, the daemonset is no longer considered when checking the auth proxy deployment.
Beyond that, the handleDeploymentUpdate function is now looking for a csm label on the deployment objects. If it doesn't find it, the handler exits without doing anything else. To fix this, labels were added to all of the auth proxy deployment objects.

GitHub Issues

List the GitHub issues impacted by this PR:

GitHub Issue #
dell/csm#1143

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
I have maintained backward compatibility

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
Ran auth proxy e2e test. Will run full e2e tests before fixed-release-v1.4.2 is released.

JacobGros

LGTM assuming checks will pass

bharathsreekanth · 2024-02-13T23:19:45Z

pkg/utils/status.go

 	// TODO: Currently commented this block of code as the API used to get the latest deployment status is not working as expected
 	// TODO: Can be uncommented once this issues gets sorted out
 	controllerReplicas, controllerStatus, controllerErr := getDeploymentStatus(ctx, instance, r)
 	if controllerErr != nil {
 		log.Infof("error from getDeploymentStatus: %s", controllerErr.Error())
 	}

+	// Auth proxy has no daemonset. Putting this if/else in here and setting nodeStatusGood to true by


How are we capturing this improvement, leave a comment in code is not a good practice but then also easy to lose sight of issues. Similarly there is TODO above as well.

@bharathsreekanth thanks for bringing that up, once v1.4.2 is out I will scope out the refactoring work and work on getting that into backlog

The TODO block above is from 6+ months ago but hopefully we will be able to address that as well

shaynafinocchiaro · 2024-02-14T13:01:59Z

pkg/utils/status.go

+	for _, deployment := range deploymentList.Items {
+		deployment := deployment
+		switch deployment.Name {
+		case "authorization-ingress-nginx-controller":


Will this check pass if the namespace isn't authorization?

thanks for catching that.
That will be a known issue for 1.9.2 and fixed in 1.10

* Add permissions for SC removal. (#431) * Bump_up_csm-operator_version_to_1.4.1 (#434) * Patch version support (#433) * Adding support of v2.9.1 for Pmax, Pscale, PFlex and unity * Updating module config version for Pmax, Pscale, PFlex and unity * Update storage_csm_powerflex.yaml * version changed to 2.9.1 * version changed to 2.9.1 * Enabling github actions for all branch * updated version to 2.9.1 for unity, powerflex, powerscale and powermax * Adding the support of PStore v2.9.1 * Adding the support of PFlex v2.9.1 * Version bumpup replication 1.7.1 * Bump up version for authorization, resiliency * update csi-metadata-retriever version to 1.6.1 * update reverseproxy version to 2.8.1 --------- Co-authored-by: KshitijaKakde <[email protected]> Co-authored-by: Kshitija Kakde <[email protected]> Co-authored-by: HarishH-DELL <[email protected]> Co-authored-by: panigs7 <[email protected]> * Change namespace name to match documentation. (#437) * go-yaml version update (#438) * go-yaml version update * update go-yaml package to latest * remove extra verbs * remove duplicate verbs and add correct test files for resiliency UT --------- Co-authored-by: Shayna Finocchiaro <[email protected]> * [bug-1120]: Update Authorization sidecar to use insecure flag (#442) * fix auth cert * remove constant * remove duplicate verbs * reverting #426 changes (#444) * Update final manifests for posting of Operator 1.4.1 (#445) * KRV-20573/csm status (#446) * add module status * Adding CSM Module status * Addressing the comments for PR * Making code reusable * Fix formatting issue * Resolving gosec issue * Final manifest update and apex client (#447) * Add csm watch permissions. (#448) * Merged CSM Module Status * Fix App-Mob node error (#457) Co-authored-by: abhi16394 <[email protected]> * Bump up CSM Operator version to 1.4.2 (#458) * update csm operator version to 1.4.2 * bump-up-op-version-1.4.2 * updated version in operator.yaml * adding missing arg to fix e2e * update authorization e2e (#459) * skip cert validation in e2e (#460) * Fixing FSGroupPolicy issue (#463) * Fixing FSGroupPolicy issue * Add unit test --------- Co-authored-by: Akshay Saini <[email protected]> * Fix for controller race, daemon status, and obs namespace (#464) * test controller state calc * add debug logs * check for driver * add daemonnset update * adds ns fix for when pod ns dis not csm ns * add ns check for daemon and deployment as well * fix typo * fix typo * explicitly return nil * add missing replace statements --------- Co-authored-by: grosnj1 <[email protected]> * Auth proxy status fix (#465) * actions * working on auth proxy status check * finished status check * fix 1 * fix or * code compiles * keep auth proxy out of daemonset check * trying to figure out deployment name * formatting * fix auth name in if condition * no daemonset * fix * added log statements * added another log statement * fix log statement * logs logs logs * compile * more logs * add logs and try updating status from handlesuccess * remove UpdateStatus call for now * adding csm label to auth * remove extraneous string replacement * removing commented code * Update actions.yml * fix merge issue * fix fmt * fix pscale + obs issue (#467) * fix some issues * fiix label issue, add debug logs * add more debug * more debugg lines * fix pscale issue * remmove debug logs * gofmt * set available to 0 en deployment is not checked * fix if check --------- Co-authored-by: grosnj1 <[email protected]> * update final manifests (#468) * fix vetting issues (#477) Co-authored-by: grosnj1 <[email protected]> --------- Co-authored-by: Don Khan <[email protected]> Co-authored-by: HarishH-DELL <[email protected]> Co-authored-by: Adarsh Kumar Yadav <[email protected]> Co-authored-by: KshitijaKakde <[email protected]> Co-authored-by: Kshitija Kakde <[email protected]> Co-authored-by: HarishH-DELL <[email protected]> Co-authored-by: panigs7 <[email protected]> Co-authored-by: Shayna Finocchiaro <[email protected]> Co-authored-by: Aaron Tye <[email protected]> Co-authored-by: Rishabh Raj <[email protected]> Co-authored-by: panigs7 <[email protected]> Co-authored-by: abhi16394 <[email protected]> Co-authored-by: Chiman Jain <[email protected]> Co-authored-by: Jooseppi Luna <[email protected]> Co-authored-by: abhi16394 <[email protected]> Co-authored-by: Akshay Saini <[email protected]> Co-authored-by: grosnj1 <[email protected]>

* Verified merge fixed release v1.4.2 (#479) * Add permissions for SC removal. (#431) * Bump_up_csm-operator_version_to_1.4.1 (#434) * Patch version support (#433) * Adding support of v2.9.1 for Pmax, Pscale, PFlex and unity * Updating module config version for Pmax, Pscale, PFlex and unity * Update storage_csm_powerflex.yaml * version changed to 2.9.1 * version changed to 2.9.1 * Enabling github actions for all branch * updated version to 2.9.1 for unity, powerflex, powerscale and powermax * Adding the support of PStore v2.9.1 * Adding the support of PFlex v2.9.1 * Version bumpup replication 1.7.1 * Bump up version for authorization, resiliency * update csi-metadata-retriever version to 1.6.1 * update reverseproxy version to 2.8.1 --------- Co-authored-by: KshitijaKakde <[email protected]> Co-authored-by: Kshitija Kakde <[email protected]> Co-authored-by: HarishH-DELL <[email protected]> Co-authored-by: panigs7 <[email protected]> * Change namespace name to match documentation. (#437) * go-yaml version update (#438) * go-yaml version update * update go-yaml package to latest * remove extra verbs * remove duplicate verbs and add correct test files for resiliency UT --------- Co-authored-by: Shayna Finocchiaro <[email protected]> * [bug-1120]: Update Authorization sidecar to use insecure flag (#442) * fix auth cert * remove constant * remove duplicate verbs * reverting #426 changes (#444) * Update final manifests for posting of Operator 1.4.1 (#445) * KRV-20573/csm status (#446) * add module status * Adding CSM Module status * Addressing the comments for PR * Making code reusable * Fix formatting issue * Resolving gosec issue * Final manifest update and apex client (#447) * Add csm watch permissions. (#448) * Merged CSM Module Status * Fix App-Mob node error (#457) Co-authored-by: abhi16394 <[email protected]> * Bump up CSM Operator version to 1.4.2 (#458) * update csm operator version to 1.4.2 * bump-up-op-version-1.4.2 * updated version in operator.yaml * adding missing arg to fix e2e * update authorization e2e (#459) * skip cert validation in e2e (#460) * Fixing FSGroupPolicy issue (#463) * Fixing FSGroupPolicy issue * Add unit test --------- Co-authored-by: Akshay Saini <[email protected]> * Fix for controller race, daemon status, and obs namespace (#464) * test controller state calc * add debug logs * check for driver * add daemonnset update * adds ns fix for when pod ns dis not csm ns * add ns check for daemon and deployment as well * fix typo * fix typo * explicitly return nil * add missing replace statements --------- Co-authored-by: grosnj1 <[email protected]> * Auth proxy status fix (#465) * actions * working on auth proxy status check * finished status check * fix 1 * fix or * code compiles * keep auth proxy out of daemonset check * trying to figure out deployment name * formatting * fix auth name in if condition * no daemonset * fix * added log statements * added another log statement * fix log statement * logs logs logs * compile * more logs * add logs and try updating status from handlesuccess * remove UpdateStatus call for now * adding csm label to auth * remove extraneous string replacement * removing commented code * Update actions.yml * fix merge issue * fix fmt * fix pscale + obs issue (#467) * fix some issues * fiix label issue, add debug logs * add more debug * more debugg lines * fix pscale issue * remmove debug logs * gofmt * set available to 0 en deployment is not checked * fix if check --------- Co-authored-by: grosnj1 <[email protected]> * update final manifests (#468) * fix vetting issues (#477) Co-authored-by: grosnj1 <[email protected]> --------- Co-authored-by: Don Khan <[email protected]> Co-authored-by: HarishH-DELL <[email protected]> Co-authored-by: Adarsh Kumar Yadav <[email protected]> Co-authored-by: KshitijaKakde <[email protected]> Co-authored-by: Kshitija Kakde <[email protected]> Co-authored-by: HarishH-DELL <[email protected]> Co-authored-by: panigs7 <[email protected]> Co-authored-by: Shayna Finocchiaro <[email protected]> Co-authored-by: Aaron Tye <[email protected]> Co-authored-by: Rishabh Raj <[email protected]> Co-authored-by: panigs7 <[email protected]> Co-authored-by: abhi16394 <[email protected]> Co-authored-by: Chiman Jain <[email protected]> Co-authored-by: Jooseppi Luna <[email protected]> Co-authored-by: abhi16394 <[email protected]> Co-authored-by: Akshay Saini <[email protected]> Co-authored-by: grosnj1 <[email protected]> * PR feedback --------- Co-authored-by: Don Khan <[email protected]> Co-authored-by: HarishH-DELL <[email protected]> Co-authored-by: Adarsh Kumar Yadav <[email protected]> Co-authored-by: KshitijaKakde <[email protected]> Co-authored-by: Kshitija Kakde <[email protected]> Co-authored-by: HarishH-DELL <[email protected]> Co-authored-by: panigs7 <[email protected]> Co-authored-by: Shayna Finocchiaro <[email protected]> Co-authored-by: Aaron Tye <[email protected]> Co-authored-by: Rishabh Raj <[email protected]> Co-authored-by: panigs7 <[email protected]> Co-authored-by: abhi16394 <[email protected]> Co-authored-by: Chiman Jain <[email protected]> Co-authored-by: Jooseppi Luna <[email protected]> Co-authored-by: abhi16394 <[email protected]> Co-authored-by: Akshay Saini <[email protected]> Co-authored-by: grosnj1 <[email protected]>

* Test branch (#482) * Verified merge fixed release v1.4.2 (#479) * Add permissions for SC removal. (#431) * Bump_up_csm-operator_version_to_1.4.1 (#434) * Patch version support (#433) * Adding support of v2.9.1 for Pmax, Pscale, PFlex and unity * Updating module config version for Pmax, Pscale, PFlex and unity * Update storage_csm_powerflex.yaml * version changed to 2.9.1 * version changed to 2.9.1 * Enabling github actions for all branch * updated version to 2.9.1 for unity, powerflex, powerscale and powermax * Adding the support of PStore v2.9.1 * Adding the support of PFlex v2.9.1 * Version bumpup replication 1.7.1 * Bump up version for authorization, resiliency * update csi-metadata-retriever version to 1.6.1 * update reverseproxy version to 2.8.1 --------- Co-authored-by: KshitijaKakde <[email protected]> Co-authored-by: Kshitija Kakde <[email protected]> Co-authored-by: HarishH-DELL <[email protected]> Co-authored-by: panigs7 <[email protected]> * Change namespace name to match documentation. (#437) * go-yaml version update (#438) * go-yaml version update * update go-yaml package to latest * remove extra verbs * remove duplicate verbs and add correct test files for resiliency UT --------- Co-authored-by: Shayna Finocchiaro <[email protected]> * [bug-1120]: Update Authorization sidecar to use insecure flag (#442) * fix auth cert * remove constant * remove duplicate verbs * reverting #426 changes (#444) * Update final manifests for posting of Operator 1.4.1 (#445) * KRV-20573/csm status (#446) * add module status * Adding CSM Module status * Addressing the comments for PR * Making code reusable * Fix formatting issue * Resolving gosec issue * Final manifest update and apex client (#447) * Add csm watch permissions. (#448) * Merged CSM Module Status * Fix App-Mob node error (#457) Co-authored-by: abhi16394 <[email protected]> * Bump up CSM Operator version to 1.4.2 (#458) * update csm operator version to 1.4.2 * bump-up-op-version-1.4.2 * updated version in operator.yaml * adding missing arg to fix e2e * update authorization e2e (#459) * skip cert validation in e2e (#460) * Fixing FSGroupPolicy issue (#463) * Fixing FSGroupPolicy issue * Add unit test --------- Co-authored-by: Akshay Saini <[email protected]> * Fix for controller race, daemon status, and obs namespace (#464) * test controller state calc * add debug logs * check for driver * add daemonnset update * adds ns fix for when pod ns dis not csm ns * add ns check for daemon and deployment as well * fix typo * fix typo * explicitly return nil * add missing replace statements --------- Co-authored-by: grosnj1 <[email protected]> * Auth proxy status fix (#465) * actions * working on auth proxy status check * finished status check * fix 1 * fix or * code compiles * keep auth proxy out of daemonset check * trying to figure out deployment name * formatting * fix auth name in if condition * no daemonset * fix * added log statements * added another log statement * fix log statement * logs logs logs * compile * more logs * add logs and try updating status from handlesuccess * remove UpdateStatus call for now * adding csm label to auth * remove extraneous string replacement * removing commented code * Update actions.yml * fix merge issue * fix fmt * fix pscale + obs issue (#467) * fix some issues * fiix label issue, add debug logs * add more debug * more debugg lines * fix pscale issue * remmove debug logs * gofmt * set available to 0 en deployment is not checked * fix if check --------- Co-authored-by: grosnj1 <[email protected]> * update final manifests (#468) * fix vetting issues (#477) Co-authored-by: grosnj1 <[email protected]> --------- Co-authored-by: Don Khan <[email protected]> Co-authored-by: HarishH-DELL <[email protected]> Co-authored-by: Adarsh Kumar Yadav <[email protected]> Co-authored-by: KshitijaKakde <[email protected]> Co-authored-by: Kshitija Kakde <[email protected]> Co-authored-by: HarishH-DELL <[email protected]> Co-authored-by: panigs7 <[email protected]> Co-authored-by: Shayna Finocchiaro <[email protected]> Co-authored-by: Aaron Tye <[email protected]> Co-authored-by: Rishabh Raj <[email protected]> Co-authored-by: panigs7 <[email protected]> Co-authored-by: abhi16394 <[email protected]> Co-authored-by: Chiman Jain <[email protected]> Co-authored-by: Jooseppi Luna <[email protected]> Co-authored-by: abhi16394 <[email protected]> Co-authored-by: Akshay Saini <[email protected]> Co-authored-by: grosnj1 <[email protected]> * PR feedback --------- Co-authored-by: Don Khan <[email protected]> Co-authored-by: HarishH-DELL <[email protected]> Co-authored-by: Adarsh Kumar Yadav <[email protected]> Co-authored-by: KshitijaKakde <[email protected]> Co-authored-by: Kshitija Kakde <[email protected]> Co-authored-by: HarishH-DELL <[email protected]> Co-authored-by: panigs7 <[email protected]> Co-authored-by: Shayna Finocchiaro <[email protected]> Co-authored-by: Aaron Tye <[email protected]> Co-authored-by: Rishabh Raj <[email protected]> Co-authored-by: panigs7 <[email protected]> Co-authored-by: abhi16394 <[email protected]> Co-authored-by: Chiman Jain <[email protected]> Co-authored-by: Jooseppi Luna <[email protected]> Co-authored-by: abhi16394 <[email protected]> Co-authored-by: Akshay Saini <[email protected]> Co-authored-by: grosnj1 <[email protected]> * add KUBE_NODE_NAME back in * add KUBE_NODE_NAME back in * add KUBE_NODE_NAME back into tests * Update dell-csm-operator.clusterserviceversion.yaml --------- Co-authored-by: Don Khan <[email protected]> Co-authored-by: HarishH-DELL <[email protected]> Co-authored-by: Adarsh Kumar Yadav <[email protected]> Co-authored-by: KshitijaKakde <[email protected]> Co-authored-by: Kshitija Kakde <[email protected]> Co-authored-by: HarishH-DELL <[email protected]> Co-authored-by: panigs7 <[email protected]> Co-authored-by: Shayna Finocchiaro <[email protected]> Co-authored-by: Aaron Tye <[email protected]> Co-authored-by: Rishabh Raj <[email protected]> Co-authored-by: panigs7 <[email protected]> Co-authored-by: abhi16394 <[email protected]> Co-authored-by: Chiman Jain <[email protected]> Co-authored-by: Jooseppi Luna <[email protected]> Co-authored-by: abhi16394 <[email protected]> Co-authored-by: Akshay Saini <[email protected]> Co-authored-by: grosnj1 <[email protected]>

jooseppi-luna added 23 commits February 12, 2024 13:58

actions

579cbbb

working on auth proxy status check

204ad55

finished status check

c50c068

fix 1

d4bb334

fix or

9e920ba

code compiles

1cf11eb

keep auth proxy out of daemonset check

b7bd55c

trying to figure out deployment name

d477487

formatting

d16a842

fix auth name in if condition

cb0b8c3

no daemonset

9c225bb

fix

8a5d31c

added log statements

0a336f7

added another log statement

6c8629f

fix log statement

0a6f9d9

logs logs logs

c68cc0e

compile

c9f9c91

more logs

dd1e1ad

add logs and try updating status from handlesuccess

ae54129

remove UpdateStatus call for now

f6e5c33

adding csm label to auth

a5793e0

remove extraneous string replacement

8aa9080

removing commented code

c2f5f41

jooseppi-luna requested review from abhi16394, mbasha-dell, alikdell, bharathsreekanth, chimanjain, coulof and Deepak-Ghivari as code owners February 13, 2024 22:50

jooseppi-luna requested review from karthikk92, kumarkgosa, bandak2, mjsdell, nitesh3108, Prabhu-Dell, rajendraindukuri, rajkumar-palani, shefali-malhotra, panigs7, tdawe, shaynafinocchiaro, atye, sharmilarama and shanmydell as code owners February 13, 2024 22:50

Merge branch 'fixed-release-v1.4.2' into auth-proxy-status-fix

7fb9492

JacobGros approved these changes Feb 13, 2024

View reviewed changes

jooseppi-luna added 3 commits February 13, 2024 18:04

Update actions.yml

32408be

fix merge issue

b387b64

fix fmt

0bd0124

bharathsreekanth approved these changes Feb 13, 2024

View reviewed changes

jooseppi-luna merged commit d57f869 into fixed-release-v1.4.2 Feb 13, 2024
7 of 8 checks passed

shaynafinocchiaro reviewed Feb 14, 2024

View reviewed changes

JacobGros mentioned this pull request Feb 19, 2024

Merge in 1.4.2 fixes - verified #480

Closed

10 tasks

JacobGros mentioned this pull request Feb 20, 2024

Merge in 1.4.2 fixes - verified #483

Merged

10 tasks

mjsdell deleted the auth-proxy-status-fix branch October 17, 2024 19:07

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Auth proxy status fix #465

Auth proxy status fix #465

jooseppi-luna commented Feb 13, 2024

JacobGros left a comment •

edited

Loading

bharathsreekanth Feb 13, 2024

jooseppi-luna Feb 13, 2024

jooseppi-luna Feb 13, 2024

shaynafinocchiaro Feb 14, 2024

JacobGros Feb 15, 2024

Auth proxy status fix #465

Auth proxy status fix #465

Conversation

jooseppi-luna commented Feb 13, 2024

Description

GitHub Issues

Checklist:

How Has This Been Tested?

JacobGros left a comment • edited Loading

Choose a reason for hiding this comment

bharathsreekanth Feb 13, 2024

Choose a reason for hiding this comment

jooseppi-luna Feb 13, 2024

Choose a reason for hiding this comment

jooseppi-luna Feb 13, 2024

Choose a reason for hiding this comment

shaynafinocchiaro Feb 14, 2024

Choose a reason for hiding this comment

JacobGros Feb 15, 2024

Choose a reason for hiding this comment

JacobGros left a comment •

edited

Loading