Read secrets for onboarding-token validation #2715

Mrudraia · 2024-07-29T05:21:57Z

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.

openshift-ci · 2024-07-29T05:22:01Z

Skipping CI for Draft Pull Request.
If you want CI signal for your change, please convert it to an actual PR.
You can still manually trigger a test run with /test all

rchikatw · 2024-07-30T05:59:24Z

controllers/util/provider.go

+	privateSecret := &corev1.Secret{}
+	privateSecret.Name = onboardingValidationPrivateKeySecretName
+	privateSecret.Namespace = operatorNamespace
+	err = cl.Get(ctx, types.NamespacedName{Name: storageClusterName, Namespace: operatorNamespace}, privateSecret)


why do we need to get the storageClusterName?

storageClusterName is removed

rchikatw · 2024-07-30T06:00:15Z

controllers/util/provider.go

+
+	pubKeyBytes := privateSecret.Data["key"]
+
+	Block, _ := pem.Decode(pubKeyBytes)


pem.Decode(privateSecret.Data["key"])

rchikatw · 2024-07-30T06:06:13Z

controllers/util/provider.go

+func newClient() (client.Client, error) {
+	klog.Info("Setting up k8s client")
+	scheme := runtime.NewScheme()
+	if err := v1.AddToScheme(scheme); err != nil {


I think clientgoscheme already registers Kube native APIs. Let's try without the scheme.

rchikatw · 2024-07-30T06:09:03Z

Do you think it would be a good idea to remove the mounted secret from the ocs-operator? The mounted secret is used to create the default client. @leelavg

leelavg · 2024-07-30T08:12:31Z

Do you think it would be a good idea to remove the mounted secret from the ocs-operator? The mounted secret is used to create the default client. @leelavg

Sure, I see no point in having two implementations for the same requirement.

rchikatw · 2024-08-02T08:59:41Z

/retest

rchikatw · 2024-08-02T09:05:48Z

/test go test

openshift-ci · 2024-08-02T09:05:51Z

@rchikatw: The specified target(s) for /test were not found.
The following commands are available to trigger required jobs:

/test ci-bundle-ocs-operator-bundle
/test images
/test ocs-operator-bundle-e2e-aws

Use /test all to run all jobs.

In response to this:

/test go test

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.

rchikatw · 2024-08-02T09:06:10Z

/test all

nb-ohad · 2024-08-04T06:27:14Z

controllers/util/provider.go

+	cl, err := newClient()
 	if err != nil {
-		return nil, fmt.Errorf("failed to read private key: %v", err)
+		klog.Exitf("failed to create client: %v", err)
+	}
+	ctx := context.Background()
+	operatorNamespace, err := GetOperatorNamespace()
+	if err != nil {
+		klog.Exitf("unable to get operator namespace: %v", err)
 	}

-	Block, _ := pem.Decode(pemString)
+	privateSecret := &corev1.Secret{}
+	privateSecret.Name = onboardingValidationPrivateKeySecretName
+	privateSecret.Namespace = operatorNamespace
+	err1 := cl.Get(ctx, types.NamespacedName{Name: onboardingValidationPrivateKeySecretName, Namespace: operatorNamespace}, privateSecret)
+	if err1 != nil {
+		return nil, fmt.Errorf("failed to get private secret: %v", err1)
+	}


There should be no need to create a new client. In most cases this code is being used there is already a client that is used on the call sire of GenerateOnboardingToken. It is important to use that client instead of the new client for many reasons, some of which are:

The client is a cached client and the object is already loaded

The client is a mock/testing client used for testing and does not execute to a valid k8s API server

I am not sure you are taking the right approach by trying to load the secret here. This code path was and should be a compute-only code path, this means it will benefit us to not introduce any k8s-specific concerns into it. My approach will be to load the secret outside of the GenerateOnboardingToken util and provide the private key payload an argument

Used existing client on the call site on GenerateOnboardingToken,

The secrets are loaded in a separate function and the private key is decoded and passed to the GenerateOnboardingToken

openshift-ci · 2024-08-05T11:01:52Z

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: Mrudraia
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

rchikatw · 2024-08-09T05:54:28Z

i see the rebase is not done properly. Can you rebase correctly? Basically, I see 2 commits and one of the commits is unrelated to your changes.

leelavg · 2024-08-09T07:48:02Z

services/ux-backend/handlers/onboardingtokens/handler.go

@@ -36,6 +33,8 @@ func handlePost(w http.ResponseWriter, r *http.Request, tokenLifetimeInHours int
 	// When ContentLength is 0 that means request body is empty and
 	// storage quota is unlimited
 	var err error
+	var cl client.Client


where is the kubeclient initialized?

leelavg · 2024-08-09T07:48:52Z

controllers/storagecluster/odfinfoconfig.go

 	"sigs.k8s.io/controller-runtime/pkg/reconcile"
-	"sigs.k8s.io/yaml"


why is this pkg being removed and different pkg added?

This change is not mine, It accured due to improper rebasing

leelavg · 2024-08-09T07:49:54Z

controllers/util/provider.go

 // GenerateOnboardingToken generates a token valid for a duration of "tokenLifetimeInHours".
 // The token content is predefined and signed by the private key which'll be read from supplied "privateKeyPath".
 // The storageQuotaInGiB is optional, and it is used to limit the storage of PVC in the application cluster.


pls change or remove irrelevant parts of the comments.

removed the irrelevant comments

leelavg · 2024-08-09T07:50:20Z

controllers/util/provider.go

@@ -64,16 +74,28 @@ func GenerateOnboardingToken(tokenLifetimeInHours int, privateKeyPath string, st
 	return fmt.Sprintf("%s.%s", encodedPayload, encodedSignature), nil
 }

-func readAndDecodePrivateKey(privateKeyPath string) (*rsa.PrivateKey, error) {
-	pemString, err := os.ReadFile(privateKeyPath)
+func DecodePemKey(cl client.Client) (*rsa.PrivateKey, error) {


does this func needs to be exported?

This function not to be exported, changed the function

leelavg · 2024-08-14T10:05:53Z

services/ux-backend/handlers/onboardingtokens/handler.go

+	client, err := newClient()
+	if err != nil {
+		klog.Errorf("failed to create new client. %v", err)
+	}


you shouldn't be creating kubeclient for every request, create it in main and use that.

@Mrudraia I believe main already creates a client and some other endpoints accept that client. Can you please follow that pattern

Acknowledge

leelavg · 2024-08-14T10:08:02Z

controllers/util/provider.go

+	privateSecret.Name = onboardingValidationPrivateKeySecretName
+	privateSecret.Namespace = operatorNamespace
+
+	err = cl.Get(ctx, types.NamespacedName{Name: onboardingValidationPrivateKeySecretName, Namespace: operatorNamespace}, privateSecret)


you have already set ObjectMeta on privateSecret and could do

Suggested change

err = cl.Get(ctx, types.NamespacedName{Name: onboardingValidationPrivateKeySecretName, Namespace: operatorNamespace}, privateSecret)

err = cl.Get(ctx, client.ObjectKeyFromObject(privateSecret), privateSecret)

leelavg · 2024-08-14T10:09:33Z

controllers/util/provider.go

-	pemString, err := os.ReadFile(privateKeyPath)
+func decodePemKey(cl client.Client) (*rsa.PrivateKey, error) {
+	klog.Info("Decoding the Pem key")
+	ctx := context.Background()


better to get the context from caller rather than creating one here everytime.

leelavg · 2024-08-14T10:10:16Z

services/ux-backend/handlers/onboardingtokens/handler.go

@@ -86,3 +90,19 @@ func handleUnsupportedMethod(w http.ResponseWriter, r *http.Request) {
 		klog.Errorf("failed write data to response writer: %v", err)
 	}
 }
+
+func newClient() (client.Client, error) {


this is a handler and possibly creation of client shouldn't be here.

Ack, removed the code in handler.go and as suggested called from the main.

nb-ohad · 2024-08-15T11:38:25Z

controllers/util/provider.go

+	privateSecret := &corev1.Secret{}
+	privateSecret.Name = onboardingValidationPrivateKeySecretName
+	privateSecret.Namespace = operatorNamespace
+
+	err = cl.Get(ctx, types.NamespacedName{Name: onboardingValidationPrivateKeySecretName, Namespace: operatorNamespace}, privateSecret)


As I stated in my previous review, getting the secret content should be done outside of the flow for GenerateOnboardingToken. The GenerateOnboardingToken (and all sub-calls under it) should not concern themselves with k8s. This means you need to get the private key string outside this flow and pass it instead of passing in a client

Changed the GenerateOnboardingToken method, privateKey is passed to the function, no k8s query is done inside GenerateOnboardingToken.

rchikatw · 2024-08-19T10:22:21Z

services/ux-backend/main.go

+	return k8sClient, nil
+}
+
+func readPrivateKey(cl client.Client) (*rsa.PrivateKey, error) {


Do not read the private key in the main.go file as it is only called once at the start. It will be a problem if the storage admin rotates the key and the private key changes.

Removed the reading of private key from main.go, and moved to utils.

Mrudraia · 2024-09-04T17:36:21Z

@Mrudraia: The following test failed, say /retest to rerun all failed tests or /retest-required to rerun all mandatory failed tests:

Test name Commit Details Required Rerun command
ci/prow/ocs-operator-bundle-e2e-aws 496fd5e link unknown /test ocs-operator-bundle-e2e-aws
Full PR test history. Your PR dashboard.

/retest

rchikatw · 2024-09-05T07:18:03Z

services/ux-backend/handlers/onboardingtokens/handler.go

+
+	privateKey, err := util.ReadPrivateKey(cl)
+	if err != nil {
+		fmt.Println("Failed to get private key", err)


it should be of type http.Error

rchikatw · 2024-09-05T07:20:00Z

controllers/util/util.go

@@ -5,6 +5,8 @@ import (
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
 )

+const onboardingValidationPrivateKeySecretName = "onboarding-private-key"


its also defined in storageclient.go

removed unused code

rchikatw · 2024-09-05T07:20:54Z

controllers/util/provider.go

 	"time"

 	"github.com/google/uuid"
 	"github.com/red-hat-storage/ocs-operator/v4/services"
 )

 // GenerateOnboardingToken generates a token valid for a duration of "tokenLifetimeInHours".
-// The token content is predefined and signed by the private key which'll be read from supplied "privateKeyPath".


you are updating the parameter type so I would suggest this comment should be update accordingly.

rchikatw · 2024-09-05T07:22:37Z

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 this change is needed.

This will trigger reconcilation to all owners whenever their is change in secrets.

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

rewantsoni · 2024-09-10T05:55:42Z

controllers/storagecluster/storageclient.go

+		r.Log.Info("Unable to get privatekey:")
+		return reconcile.Result{}, nil


If there is an err while reading the private key we should return the err

openshift-merge-robot · 2024-09-28T23:28:25Z

PR needs rebase.

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.

leelavg · 2024-09-29T01:50:33Z

Close this as a copy is raised already.

Nikhil-Ladha · 2024-10-10T07:34:53Z

Opened a new PR #2827

openshift-ci bot added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Jul 29, 2024

Mrudraia force-pushed the onboarding branch from 5266fbf to 30e2f00 Compare July 29, 2024 05:24

Mrudraia marked this pull request as ready for review July 29, 2024 14:47

openshift-ci bot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Jul 29, 2024

rchikatw reviewed Jul 30, 2024

View reviewed changes

Mrudraia force-pushed the onboarding branch 5 times, most recently from 60022a5 to a98859f Compare August 2, 2024 08:03

Mrudraia force-pushed the onboarding branch from a98859f to ffe11fd Compare August 2, 2024 10:12

nb-ohad requested changes Aug 4, 2024

View reviewed changes

openshift-ci bot assigned nb-ohad Aug 4, 2024

Mrudraia force-pushed the onboarding branch from ffe11fd to 0ca00c9 Compare August 5, 2024 11:01

Mrudraia force-pushed the onboarding branch 7 times, most recently from 52d15b8 to c72f700 Compare August 7, 2024 11:52

leelavg reviewed Aug 9, 2024

View reviewed changes

Mrudraia force-pushed the onboarding branch 2 times, most recently from e84e5e2 to e9c31d4 Compare August 9, 2024 08:28

leelavg reviewed Aug 14, 2024

View reviewed changes

nb-ohad requested changes Aug 15, 2024

View reviewed changes

Mrudraia force-pushed the onboarding branch 3 times, most recently from fdf2e37 to 859cfef Compare August 19, 2024 08:26

rchikatw reviewed Aug 19, 2024

View reviewed changes

Mrudraia force-pushed the onboarding branch 2 times, most recently from d16fedf to 36a17e7 Compare September 4, 2024 10:59

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

Mrudraia force-pushed the onboarding branch from 36a17e7 to 6a56e87 Compare September 4, 2024 11:20

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

Mrudraia force-pushed the onboarding branch from 6a56e87 to 496fd5e Compare September 4, 2024 11:25

Mrudraia requested review from rchikatw and nb-ohad September 5, 2024 04:45

rchikatw reviewed Sep 5, 2024

View reviewed changes

Mrudraia force-pushed the onboarding branch from 496fd5e to b740c0f Compare September 5, 2024 08:41

rewantsoni suggested changes Sep 10, 2024

View reviewed changes

openshift-ci bot assigned rewantsoni Sep 10, 2024

mrudraia1 mentioned this pull request Sep 10, 2024

Read secrets for onboarding-token validation #2790

Closed

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

Nikhil-Ladha closed this Oct 10, 2024


		pubKeyBytes := privateSecret.Data["key"]

		Block, _ := pem.Decode(pubKeyBytes)

		"sigs.k8s.io/controller-runtime/pkg/reconcile"
		"sigs.k8s.io/yaml"

	err = cl.Get(ctx, types.NamespacedName{Name: onboardingValidationPrivateKeySecretName, Namespace: operatorNamespace}, privateSecret)
	err = cl.Get(ctx, client.ObjectKeyFromObject(privateSecret), privateSecret)

		r.Log.Info("Unable to get privatekey:")
		return reconcile.Result{}, nil

Read secrets for onboarding-token validation #2715

Read secrets for onboarding-token validation #2715

Conversation

Mrudraia commented Jul 29, 2024

openshift-ci bot commented Jul 29, 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

rchikatw commented Jul 30, 2024

leelavg commented Jul 30, 2024

rchikatw commented Aug 2, 2024

rchikatw commented Aug 2, 2024

openshift-ci bot commented Aug 2, 2024

rchikatw commented Aug 2, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

openshift-ci bot commented Aug 5, 2024

rchikatw commented Aug 9, 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

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

Mrudraia commented Sep 4, 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

Choose a reason for hiding this comment

openshift-merge-robot commented Sep 28, 2024

leelavg commented Sep 29, 2024

Nikhil-Ladha commented Oct 10, 2024