Read secrets for client-onboarding-token-validation #2827

mrudraia1 · 2024-09-30T09:51:14Z

This PR reads the secrets instead of reading the secrets from the volume mounts.
whenever the new onboarding secrets are created, it takes more time to read the secrets from the volume mounts,
The user clicks the rotate onboarding keys, the kubernetes still uses the old public, private keys , the new keys are mounted later, So this PR will read the secrets directly from the kubernetes secrets.

controllers/storagecluster/storageclient.go

controllers/util/provider.go

services/ux-backend/handlers/common.go

services/ux-backend/handlers/onboarding/clienttokens/handler.go

services/ux-backend/handlers/onboarding/peertokens/handler.go

services/ux-backend/main.go

tools/csv-merger/csv-merger.go

leelavg · 2024-10-01T05:42:26Z

a suggestion, we are seeing this PR for third time, second time it's fine that you weren't able to recover GH (remember you can't create new a/c every-time though) but last time it's better if you can focus on rebasing properly.

yes, GH doesn't have any issue w/ closing & opening a new PR but for reviewers it's kinda hard to relook from the start.

mrudraia1 · 2024-10-15T07:37:00Z

I have tested this PR, with the latest 4.18 build. I could see the keys getting exchanged when the rotate signing key is clicked in storageclient page.

services/ux-backend/handlers/onboarding/clienttokens/handler.go

leelavg · 2024-10-15T07:52:10Z

I have tested this PR, with the latest 4.18 build. I could see the keys getting exchanged when the rotate signing key is clicked in storageclient page.

ack, only minor comments now. @nb-ohad possible for a final review?

controllers/storagecluster/storageclient.go

nb-ohad · 2024-10-15T08:27:36Z

controllers/storagecluster/storagecluster_controller.go

@@ -225,7 +225,7 @@ func (r *StorageClusterReconciler) SetupWithManager(mgr ctrl.Manager) error {
 		Owns(&appsv1.Deployment{}, builder.WithPredicates(predicate.GenerationChangedPredicate{})).
 		Owns(&corev1.Service{}, builder.WithPredicates(predicate.GenerationChangedPredicate{})).
 		Owns(&corev1.ConfigMap{}, builder.MatchEveryOwner, builder.WithPredicates(predicate.GenerationChangedPredicate{})).
-		Owns(&corev1.Secret{}, builder.WithPredicates(predicate.GenerationChangedPredicate{})).
+		Owns(&corev1.Secret{}, builder.MatchEveryOwner, builder.WithPredicates(predicate.GenerationChangedPredicate{})).


Why do we need a MatchEveryOwner here? Isn't StorageCluster the controller owner of the secret we want to watch?

controller owner reference set to secrets, ack

This comment was not addressed as tall! You are matching by controller ownership, it does not make sense to match every owner

Owner reference were set during creation of secrets in on-boarding job, when the rotate signing key is clicked the re-consiliation is not happening, The onboarding-token needs to be created which is failing.

Any suggestion for this issue.

controllers/util/provider.go

services/ux-backend/handlers/onboarding/clienttokens/handler.go

services/ux-backend/main.go

openshift-ci · 2024-10-16T05:45:30Z

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: mrudraia1
Once this PR has been reviewed and has the lgtm label, please ask for approval from nb-ohad. For more information see the Kubernetes Code Review Process.

The full list of commands accepted by this bot can be found here.

Needs approval from an approver in each of these files:

OWNERS

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

deepsm007 · 2024-11-06T15:10:59Z

infra issue
/test images

nb-ohad · 2024-11-12T13:01:46Z

controllers/storagecluster/storagecluster_controller.go

@@ -225,7 +225,7 @@ func (r *StorageClusterReconciler) SetupWithManager(mgr ctrl.Manager) error {
 		Owns(&appsv1.Deployment{}, builder.WithPredicates(predicate.GenerationChangedPredicate{})).
 		Owns(&corev1.Service{}, builder.WithPredicates(predicate.GenerationChangedPredicate{})).
 		Owns(&corev1.ConfigMap{}, builder.MatchEveryOwner, builder.WithPredicates(predicate.GenerationChangedPredicate{})).
-		Owns(&corev1.Secret{}, builder.WithPredicates(predicate.GenerationChangedPredicate{})).
+		Owns(&corev1.Secret{}, builder.MatchEveryOwner, builder.WithPredicates(predicate.GenerationChangedPredicate{})).


This comment was not addressed as tall! You are matching by controller ownership, it does not make sense to match every owner

nb-ohad · 2024-11-12T13:10:59Z

controllers/util/provider.go

-	privateKey, err := x509.ParsePKCS1PrivateKey(Block.Bytes)
-	if err != nil {
-		return nil, fmt.Errorf("failed to parse private key: %v", err)
+	if privateSecret.Data != nil {


You don't need this check: Maps in golang, even when not initialized, support the key access operator.

Suggested change

if privateSecret.Data != nil {

nb-ohad · 2024-11-12T13:14:05Z

controllers/util/provider.go

+		if privateSecretKey, ok := privateSecret.Data["key"]; ok {
+			Block, _ := pem.Decode(privateSecretKey)
+			privateKey, err := x509.ParsePKCS1PrivateKey(Block.Bytes)
+			if err != nil {
+				return nil, fmt.Errorf("failed to parse private key: %v", err)
+			}
+			return privateKey, nil
+		}
 	}
-	return privateKey, nil
+
+	return nil, fmt.Errorf("No data found in secret")


I would prefer, from code readability and extensibility concerns, if we first handle error conditions and only at the end have the return that is not erroneous. This way you don't need nested returns and extending the function in the future with more checks become easier

Suggested change

if privateSecretKey, ok := privateSecret.Data["key"]; ok {

Block, _ := pem.Decode(privateSecretKey)

privateKey, err := x509.ParsePKCS1PrivateKey(Block.Bytes)

if err != nil {

return nil, fmt.Errorf("failed to parse private key: %v", err)

}

return privateKey, nil

}

}

return privateKey, nil

return nil, fmt.Errorf("No data found in secret")

privateSecretKey, ok := privateSecret.Data["key"];

if !ok {

return nil, fmt.Errorf("No data found in secret")

}

Block, _ := pem.Decode(privateSecretKey)

privateKey, err := x509.ParsePKCS1PrivateKey(Block.Bytes)

if err != nil {

return nil, fmt.Errorf("failed to parse private key: %v", err)

}

return privateKey, nil

nb-ohad · 2024-11-12T13:16:26Z

services/ux-backend/handlers/onboarding/clienttokens/handler.go

+		return
+	}
+
+	if onboardingToken, err := util.GenerateClientOnboardingToken(tokenLifetimeInHours, privateKey, storageQuotaInGiB, storageCluster.UID); err != nil {


This line is too long, please split over multi-lines

Suggested change

if onboardingToken, err := util.GenerateClientOnboardingToken(tokenLifetimeInHours, privateKey, storageQuotaInGiB, storageCluster.UID); err != nil {

if onboardingToken, err := util.GenerateClientOnboardingToken(

tokenLifetimeInHours,

privateKey,

storageQuotaInGiB,

storageCluster.UID,

); err != nil {

nb-ohad · 2024-11-12T13:16:52Z

services/ux-backend/handlers/onboarding/peertokens/handler.go

+		return
+	}
+
+	if onboardingToken, err := util.GeneratePeerOnboardingToken(tokenLifetimeInHours, privateKey, storageCluster.UID); err != nil {


Line too long, see last comment for breaking into multi lines

nb-ohad · 2024-11-12T13:17:52Z

services/ux-backend/handlers/onboarding/peertokens/handler.go

 	storageCluster, err := util.GetStorageClusterInNamespace(r.Context(), cl, namespace)
 	if err != nil {
 		http.Error(w, err.Error(), http.StatusInternalServerError)
 		return
 	}

-	if onboardingToken, err := util.GeneratePeerOnboardingToken(tokenLifetimeInHours, onboardingPrivateKeyFilePath, storageCluster.UID); err != nil {
+	ctx := context.TODO()


You should not use a TODO context but a background context

If there is a need to create a context for handling a request it should be created at the topmost level of the request handling and passed down

nb-ohad · 2024-11-12T13:18:48Z

services/ux-backend/main.go

@@ -87,10 +87,10 @@ func main() {
 		// Set the Deprecation header
 		w.Header().Set("Deprecation", "true") // Standard "Deprecation" header
 		w.Header().Set("Link", "/onboarding/client-tokens; rel=\"alternate\"")
-		clienttokens.HandleMessage(w, r, config.tokenLifetimeInHours, cl, namespace)
+		clienttokens.HandleMessage(r.Context(), w, r, config.tokenLifetimeInHours, cl, namespace)


You should not use the same context for main and for specific request handling. Each request should initialize its own background context that will be used during the entire request.

nb-ohad · 2024-11-12T13:18:55Z

services/ux-backend/main.go

 	})
 	http.HandleFunc("/onboarding/client-tokens", func(w http.ResponseWriter, r *http.Request) {
-		clienttokens.HandleMessage(w, r, config.tokenLifetimeInHours, cl, namespace)
+		clienttokens.HandleMessage(r.Context(), w, r, config.tokenLifetimeInHours, cl, namespace)


Same as last comment

Signed-off-by: Mrudraia1 <[email protected]>

rewantsoni suggested changes Oct 1, 2024

View reviewed changes

openshift-ci bot assigned rewantsoni Oct 1, 2024

mrudraia1 force-pushed the onboarding branch 3 times, most recently from 674b54c to dc8a56b Compare October 3, 2024 05:37

Nikhil-Ladha mentioned this pull request Oct 10, 2024

Read secrets for onboarding-token validation #2715

Closed

mrudraia1 requested review from leelavg and rewantsoni October 15, 2024 07:15

leelavg reviewed Oct 15, 2024

View reviewed changes

services/ux-backend/handlers/onboarding/clienttokens/handler.go Outdated Show resolved Hide resolved

services/ux-backend/handlers/onboarding/clienttokens/handler.go Outdated Show resolved Hide resolved

nb-ohad requested changes Oct 15, 2024

View reviewed changes

openshift-ci bot assigned nb-ohad Oct 15, 2024

mrudraia1 force-pushed the onboarding branch from dc8a56b to c90cc2f Compare October 16, 2024 05:45

mrudraia1 force-pushed the onboarding branch 6 times, most recently from 4dac720 to 8198c71 Compare October 16, 2024 07:06

mrudraia1 requested review from nb-ohad and leelavg October 16, 2024 09:05

sunilheggodu assigned mrudraia1 Oct 16, 2024

mrudraia1 force-pushed the onboarding branch 2 times, most recently from ecddbf0 to d31f826 Compare November 6, 2024 14:07

mrudraia1 force-pushed the onboarding branch from d31f826 to e627033 Compare November 6, 2024 15:21

nb-ohad requested changes Nov 12, 2024

View reviewed changes

mrudraia1 force-pushed the onboarding branch from e627033 to bbae1f7 Compare November 12, 2024 16:39

openshift-merge-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Nov 12, 2024

mrudraia1 force-pushed the onboarding branch from bbae1f7 to 36162f4 Compare November 12, 2024 16:59

openshift-merge-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Nov 12, 2024

mrudraia1 force-pushed the onboarding branch from 36162f4 to e3bba8e Compare November 12, 2024 17:08

mrudraia1 requested a review from nb-ohad November 13, 2024 08:00

Read secrets for client-onboarding-token-validation

f6ee295

Signed-off-by: Mrudraia1 <[email protected]>

mrudraia1 force-pushed the onboarding branch from e3bba8e to f6ee295 Compare November 13, 2024 10:40

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Read secrets for client-onboarding-token-validation #2827

Read secrets for client-onboarding-token-validation #2827

mrudraia1 commented Sep 30, 2024

leelavg commented Oct 1, 2024

mrudraia1 commented Oct 15, 2024

leelavg commented Oct 15, 2024

nb-ohad Oct 15, 2024

mrudraia1 Oct 16, 2024

nb-ohad Nov 12, 2024

mrudraia1 Nov 12, 2024

openshift-ci bot commented Oct 16, 2024

deepsm007 commented Nov 6, 2024

nb-ohad Nov 12, 2024

nb-ohad Nov 12, 2024

nb-ohad Nov 12, 2024

nb-ohad Nov 12, 2024

nb-ohad Nov 12, 2024

nb-ohad Nov 12, 2024

nb-ohad Nov 12, 2024

nb-ohad Nov 12, 2024

Read secrets for client-onboarding-token-validation #2827

Are you sure you want to change the base?

Read secrets for client-onboarding-token-validation #2827

Conversation

mrudraia1 commented Sep 30, 2024

leelavg commented Oct 1, 2024

mrudraia1 commented Oct 15, 2024

leelavg commented Oct 15, 2024

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

openshift-ci bot commented Oct 16, 2024

deepsm007 commented Nov 6, 2024

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