Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

extend the onboarding struct to make it modular #2791

Closed
wants to merge 1 commit into from
Closed

extend the onboarding struct to make it modular #2791

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 3 additions & 1 deletion controllers/storagecluster/storageclient.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,7 @@ import (
ocsclientv1a1 "github.com/red-hat-storage/ocs-client-operator/api/v1alpha1"
ocsv1 "github.com/red-hat-storage/ocs-operator/api/v4/v1"
"github.com/red-hat-storage/ocs-operator/v4/controllers/util"
"github.com/red-hat-storage/ocs-operator/v4/services"
kerrors "k8s.io/apimachinery/pkg/api/errors"
"sigs.k8s.io/controller-runtime/pkg/controller/controllerutil"
"sigs.k8s.io/controller-runtime/pkg/reconcile"
Expand All @@ -31,7 +32,8 @@ func (s *storageClient) ensureCreated(r *StorageClusterReconciler, storagecluste
storageClient.Name = storagecluster.Name
_, err := controllerutil.CreateOrUpdate(r.ctx, r.Client, storageClient, func() error {
if storageClient.Status.ConsumerID == "" {
token, err := util.GenerateOnboardingToken(tokenLifetimeInHours, onboardingPrivateKeyFilePath, nil)
roleSpec := services.OnboardingSubjectRoleSpec{Role: services.ClientRole}
token, err := util.GenerateOnboardingToken(tokenLifetimeInHours, onboardingPrivateKeyFilePath, roleSpec)
if err != nil {
return fmt.Errorf("unable to generate onboarding token: %v", err)
}
Expand Down
9 changes: 4 additions & 5 deletions controllers/util/provider.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,19 +19,18 @@ import (

// 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.
func GenerateOnboardingToken(tokenLifetimeInHours int, privateKeyPath string, storageQuotaInGiB *uint) (string, error) {
// The roleSpec contains the SubjectRole and the RoleOptions for the ticket.
func GenerateOnboardingToken(tokenLifetimeInHours int, privateKeyPath string, roleSpec services.OnboardingSubjectRoleSpec) (string, error) {
tokenExpirationDate := time.Now().
Add(time.Duration(tokenLifetimeInHours) * time.Hour).
Unix()

ticket := services.OnboardingTicket{
ID: uuid.New().String(),
ExpirationDate: tokenExpirationDate,
SubjectRole: roleSpec,
}
if storageQuotaInGiB != nil {
ticket.StorageQuotaInGiB = *storageQuotaInGiB
}

payload, err := json.Marshal(ticket)
if err != nil {
return "", fmt.Errorf("failed to marshal the payload: %v", err)
Expand Down
20 changes: 15 additions & 5 deletions services/provider/server/server.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -110,13 +110,13 @@ func (s *OCSProviderServer) OnboardConsumer(ctx context.Context, req *pb.Onboard
return nil, status.Errorf(codes.Internal, "failed to get public key to validate onboarding ticket for consumer %q. %v", req.ConsumerName, err)
}

onboardingTicket, err := decodeAndValidateTicket(req.OnboardingTicket, pubKey)
onboardingTicket, err := decodeAndValidateTicket(req.OnboardingTicket, pubKey, services.ClientRole)
if err != nil {
klog.Errorf("failed to validate onboarding ticket for consumer %q. %v", req.ConsumerName, err)
return nil, status.Errorf(codes.InvalidArgument, "onboarding ticket is not valid. %v", err)
}

storageConsumerUUID, err := s.consumerManager.Create(ctx, req, int(onboardingTicket.StorageQuotaInGiB))
storageConsumerUUID, err := s.consumerManager.Create(ctx, req, int(onboardingTicket.SubjectRole.ClientOptions.StorageQuotaInGiB))
if err != nil {
if !kerrors.IsAlreadyExists(err) && err != errTicketAlreadyExists {
return nil, status.Errorf(codes.Internal, "failed to create storageConsumer %q. %v", req.ConsumerName, err)
Expand Down Expand Up @@ -470,7 +470,7 @@ func getSubVolumeGroupClusterID(subVolumeGroup *rookCephv1.CephFilesystemSubVolu
return hex.EncodeToString(hash[:16])
}

func decodeAndValidateTicket(ticket string, pubKey *rsa.PublicKey) (*services.OnboardingTicket, error) {
func decodeAndValidateTicket(ticket string, pubKey *rsa.PublicKey, expectedRole services.OnboardingSubjectRole) (*services.OnboardingTicket, error) {
ticketArr := strings.Split(string(ticket), ".")
if len(ticketArr) != 2 {
return nil, fmt.Errorf("invalid ticket")
Expand All @@ -487,8 +487,18 @@ func decodeAndValidateTicket(ticket string, pubKey *rsa.PublicKey) (*services.On
return nil, fmt.Errorf("failed to unmarshal onboarding ticket message. %v", err)
}

if ticketData.StorageQuotaInGiB > math.MaxInt {
return nil, fmt.Errorf("invalid value sent in onboarding ticket, storage quota should be greater than 0 and less than %v: %v", math.MaxInt, ticketData.StorageQuotaInGiB)
if ticketData.SubjectRole.Role != expectedRole {
return nil, fmt.Errorf("unsupported role sent in onboarding ticket %s, expectedRole is %s", ticketData.SubjectRole.Role, expectedRole)
}
Comment on lines +490 to +492
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

isn't this repeating the default case of below switch?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, I am expecting this function to be called while onboarding (onboarding clients and onboarding peers). This check is required so that we don't enter the peer role ticket while onboarding the client and vice-versa.

I could remove the default case in the switch as the code would never reach it.


switch ticketData.SubjectRole.Role {
case services.ClientRole:
clientOptions := ticketData.SubjectRole.ClientOptions
if ticketData.SubjectRole.ClientOptions.StorageQuotaInGiB > math.MaxInt {
return nil, fmt.Errorf("invalid value sent in onboarding ticket, storage quota should be greater than 0 and less than %v: %v", math.MaxInt, clientOptions.StorageQuotaInGiB)
}
default:
return nil, fmt.Errorf("invalid role sent in onboarding ticket %s, expectedRole is %s", ticketData.SubjectRole.Role, expectedRole)
}

signature, err := base64.StdEncoding.DecodeString(ticketArr[1])
Expand Down
23 changes: 20 additions & 3 deletions services/types.go
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,7 +1,24 @@
package services

type OnboardingSubjectRole string

type OnboardingSubjectRoleSpec struct {
Role OnboardingSubjectRole `json:"role"`
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

you are trying to make role as mandatory but the original could be empty quota, thought of having OnboardingSubjectRoleSpec as a pointer in parent struct?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

original could be empty quota Could you elaborate on what you mean by original could be empty quota?

ClientOptions ClientRoleOptions `json:"clientRoleOptions,omitempty"`
}

const (
ClientRole OnboardingSubjectRole = "ocs-client"
)

type ClientRoleOptions struct {
StorageQuotaInGiB uint `json:"storageQuotaInGiB,omitempty"`
}

type OnboardingTicket struct {
ID string `json:"id"`
ExpirationDate int64 `json:"expirationDate,string"`
StorageQuotaInGiB uint `json:"storageQuotaInGiB,omitempty"`
ID string `json:"id"`
ExpirationDate int64 `json:"expirationDate,string"`

// SubjectRole specifies the role and options for the role
SubjectRole OnboardingSubjectRoleSpec `json:"subjectRole"`
}
13 changes: 7 additions & 6 deletions services/ux-backend/handlers/onboardingtokens/handler.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,9 +7,10 @@ import (
"net/http"

"github.com/red-hat-storage/ocs-operator/v4/controllers/util"
"github.com/red-hat-storage/ocs-operator/v4/services"
"github.com/red-hat-storage/ocs-operator/v4/services/ux-backend/handlers"

"k8s.io/klog/v2"
"k8s.io/utils/ptr"
)

const (
Expand All @@ -32,9 +33,9 @@ func HandleMessage(w http.ResponseWriter, r *http.Request, tokenLifetimeInHours
}

func handlePost(w http.ResponseWriter, r *http.Request, tokenLifetimeInHours int) {
var storageQuotaInGiB *uint
// When ContentLength is 0 that means request body is empty and
// storage quota is unlimited

var roleSpec services.OnboardingSubjectRoleSpec
roleSpec.Role = services.ClientRole
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

setting fields at multiple places forces us to look for where all these are set, better to move this to line 59 only where you are setting the options?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Since we have only one role up to this point, I am always setting the role as clientRole to avoid further changes to console. With 4.18, we will explicitly set the role while calling this endpoint

var err error
if r.ContentLength != 0 {
var quota = struct {
Expand All @@ -55,9 +56,9 @@ func handlePost(w http.ResponseWriter, r *http.Request, tokenLifetimeInHours int
http.Error(w, fmt.Sprintf("invalid Unit type sent in request body, Valid types are [Gi,Ti,Pi]: %v", quota.Unit), http.StatusBadRequest)
return
}
storageQuotaInGiB = ptr.To(unitAsGiB * quota.Value)
roleSpec.ClientOptions = services.ClientRoleOptions{StorageQuotaInGiB: unitAsGiB * quota.Value}
}
if onboardingToken, err := util.GenerateOnboardingToken(tokenLifetimeInHours, onboardingPrivateKeyFilePath, storageQuotaInGiB); err != nil {
if onboardingToken, err := util.GenerateOnboardingToken(tokenLifetimeInHours, onboardingPrivateKeyFilePath, roleSpec); err != nil {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

if contentLength is 0 then you'll be setting empty roleSpec (after doing above change), is that still fine?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If that is done, the ticket will be generated for an empty role if the body is empty, which will always lead to failure while onboarding since we cannot verify the role

klog.Errorf("failed to get onboardig token: %v", err)
w.WriteHeader(http.StatusInternalServerError)
w.Header().Set("Content-Type", handlers.ContentTypeTextPlain)
Expand Down
Loading