From dd079182705c6515cbf62acdd51b614711e039cd Mon Sep 17 00:00:00 2001 From: Roman Dodin Date: Tue, 2 Aug 2022 09:39:05 +0200 Subject: [PATCH 1/2] refactored version detection for license and lic update procedure --- api/types/v1alpha1/srlinux_types.go | 5 +- api/types/v1alpha1/srlinux_types_test.go | 37 +++--- api/types/v1alpha1/zz_generated.deepcopy.go | 5 - .../crd/bases/kne.srlinux.dev_srlinuxes.yaml | 14 --- config/manager/kustomization.yaml | 2 +- controllers/secret.go | 106 ++++++++++++++++-- controllers/srlinux_controller.go | 12 +- 7 files changed, 114 insertions(+), 67 deletions(-) diff --git a/api/types/v1alpha1/srlinux_types.go b/api/types/v1alpha1/srlinux_types.go index 5247bba..525b9a2 100644 --- a/api/types/v1alpha1/srlinux_types.go +++ b/api/types/v1alpha1/srlinux_types.go @@ -59,8 +59,6 @@ type Srlinux struct { Spec SrlinuxSpec `json:"spec,omitempty"` Status SrlinuxStatus `json:"status,omitempty"` - // parsed NOS version - NOSVersion *SrlVersion `json:"nos-version,omitempty"` // license key from license secret that contains a license file for this Srlinux LicenseKey string `json:"license_key,omitempty"` } @@ -154,12 +152,13 @@ func (s *SrlinuxSpec) GetImageVersion() (*SrlVersion, error) { func (s *Srlinux) InitLicenseKey( _ context.Context, secret *corev1.Secret, + version *SrlVersion, ) { if secret == nil { return } - versionSecretKey := fmt.Sprintf("%s-%s.key", s.NOSVersion.Major, s.NOSVersion.Minor) + versionSecretKey := fmt.Sprintf("%s-%s.key", version.Major, version.Minor) if _, ok := secret.Data[versionSecretKey]; ok { s.LicenseKey = versionSecretKey diff --git a/api/types/v1alpha1/srlinux_types_test.go b/api/types/v1alpha1/srlinux_types_test.go index 5f91df5..f57bd2b 100644 --- a/api/types/v1alpha1/srlinux_types_test.go +++ b/api/types/v1alpha1/srlinux_types_test.go @@ -135,16 +135,14 @@ func TestGetImageVersion(t *testing.T) { func TestInitVersion(t *testing.T) { tests := []struct { - desc string - srl *Srlinux - secret *corev1.Secret - want string + desc string + version *SrlVersion + secret *corev1.Secret + want string }{ { - desc: "secret key matches srl version", - srl: &Srlinux{ - NOSVersion: &SrlVersion{"22", "3", "", "", ""}, - }, + desc: "secret key matches srl version", + version: &SrlVersion{"22", "3", "", "", ""}, secret: &corev1.Secret{ Data: map[string][]byte{ "22-3.key": nil, @@ -154,10 +152,8 @@ func TestInitVersion(t *testing.T) { want: "22-3.key", }, { - desc: "wildcard secret key matches srl version", - srl: &Srlinux{ - NOSVersion: &SrlVersion{"22", "3", "", "", ""}, - }, + desc: "wildcard secret key matches srl version", + version: &SrlVersion{"22", "3", "", "", ""}, secret: &corev1.Secret{ Data: map[string][]byte{ "22-6.key": nil, @@ -167,24 +163,23 @@ func TestInitVersion(t *testing.T) { want: "all.key", }, { - desc: "secret does not exist", - srl: &Srlinux{ - NOSVersion: &SrlVersion{"22", "3", "", "", ""}, - }, - secret: nil, - want: "", + desc: "secret does not exist", + version: &SrlVersion{"22", "3", "", "", ""}, + secret: nil, + want: "", }, } for _, tt := range tests { t.Run(tt.desc, func(t *testing.T) { - tt.srl.InitLicenseKey(context.TODO(), tt.secret) + srl := &Srlinux{} + srl.InitLicenseKey(context.TODO(), tt.secret, tt.version) - if !cmp.Equal(tt.srl.LicenseKey, tt.want) { + if !cmp.Equal(srl.LicenseKey, tt.want) { t.Fatalf( "%s: actual and expected inputs do not match\nactual: %+v\nexpected:%+v", tt.desc, - tt.srl.LicenseKey, + srl.LicenseKey, tt.want, ) } diff --git a/api/types/v1alpha1/zz_generated.deepcopy.go b/api/types/v1alpha1/zz_generated.deepcopy.go index 5b0c633..f0ec889 100644 --- a/api/types/v1alpha1/zz_generated.deepcopy.go +++ b/api/types/v1alpha1/zz_generated.deepcopy.go @@ -113,11 +113,6 @@ func (in *Srlinux) DeepCopyInto(out *Srlinux) { in.ObjectMeta.DeepCopyInto(&out.ObjectMeta) in.Spec.DeepCopyInto(&out.Spec) out.Status = in.Status - if in.NOSVersion != nil { - in, out := &in.NOSVersion, &out.NOSVersion - *out = new(SrlVersion) - **out = **in - } } // DeepCopy is an autogenerated deepcopy function, copying the receiver, creating a new Srlinux. diff --git a/config/crd/bases/kne.srlinux.dev_srlinuxes.yaml b/config/crd/bases/kne.srlinux.dev_srlinuxes.yaml index ae9a4be..3196ff2 100644 --- a/config/crd/bases/kne.srlinux.dev_srlinuxes.yaml +++ b/config/crd/bases/kne.srlinux.dev_srlinuxes.yaml @@ -44,20 +44,6 @@ spec: type: string metadata: type: object - nos-version: - description: parsed NOS version - properties: - build: - type: string - commit: - type: string - major: - type: string - minor: - type: string - patch: - type: string - type: object spec: description: SrlinuxSpec defines the desired state of Srlinux. properties: diff --git a/config/manager/kustomization.yaml b/config/manager/kustomization.yaml index 3621a31..5161c05 100644 --- a/config/manager/kustomization.yaml +++ b/config/manager/kustomization.yaml @@ -13,4 +13,4 @@ kind: Kustomization images: - name: controller newName: ghcr.io/srl-labs/srl-controller - newTag: 0.4.0 + newTag: 0.4.2 diff --git a/controllers/secret.go b/controllers/secret.go index 8b13b56..995a5a6 100644 --- a/controllers/secret.go +++ b/controllers/secret.go @@ -2,31 +2,60 @@ package controllers import ( "context" + "errors" + "fmt" "github.com/go-logr/logr" + "github.com/google/go-cmp/cmp" typesv1alpha1 "github.com/srl-labs/srl-controller/api/types/v1alpha1" corev1 "k8s.io/api/core/v1" - "k8s.io/apimachinery/pkg/api/errors" + k8serrors "k8s.io/apimachinery/pkg/api/errors" + metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "k8s.io/apimachinery/pkg/types" ) +var ErrLicenseProvisioning = errors.New("license provisioning failed") + // createSecrets creates secrets such as srlinux-licenses. func (r *SrlinuxReconciler) createSecrets( ctx context.Context, s *typesv1alpha1.Srlinux, log logr.Logger, ) error { - secret, err := r.copyLicenseSecret(ctx, s, log) + secret, err := r.addOrUpdateLicenseSecret(ctx, s, log) + if err != nil { + return err + } + + v, err := s.Spec.GetImageVersion() if err != nil { return err } + log.Info("SR Linux image version parsed", "version", v) + // set license key matching image version - s.InitLicenseKey(ctx, secret) + s.InitLicenseKey(ctx, secret, v) return err } +func (r *SrlinuxReconciler) addOrUpdateLicenseSecret( + ctx context.Context, + s *typesv1alpha1.Srlinux, + log logr.Logger, +) (*corev1.Secret, error) { + secret := &corev1.Secret{} + + // if secret is already present in the s.Namespace, we need to update it + if err := r.Get(ctx, types.NamespacedName{Name: srlLicenseSecretName, Namespace: s.Namespace}, secret); err == nil { + return r.updateLicenseSecret(ctx, s, log, secret) + } + + // otherwise we need to copy a secret from controller's namespace + return r.copyLicenseSecret(ctx, s, log) +} + // copyLicenseSecret finds a secret with srlinux licenses in srlinux-controller namespace // and copies it to the srlinux CR namespace and returns the pointer to the newly created secret. // If original secret doesn't exist (i.e. when no licenses were provisioned by a user) @@ -40,24 +69,36 @@ func (r *SrlinuxReconciler) copyLicenseSecret( // find license secret in controller' ns to copy from // return silently if not found. - err := r.Get(ctx, types.NamespacedName{Name: srlLicenseSecretName, Namespace: controllerNamespace}, secret) - if err != nil && errors.IsNotFound(err) { - log.Info("secret with licenses is not found in controller's namespace, skipping copy to lab namespace", - "secret name", srlLicenseSecretName, - "controller namespace", controllerNamespace) + err := r.Get( + ctx, + types.NamespacedName{Name: srlLicenseSecretName, Namespace: controllerNamespace}, + secret, + ) + if err != nil && k8serrors.IsNotFound(err) { + log.Info( + "secret with licenses is not found in controller's namespace, skipping copy to lab namespace", + "secret name", + srlLicenseSecretName, + "controller namespace", + controllerNamespace, + ) return nil, nil } // copy secret obj from controller's ns to a new secret // that we put in the lab's ns - newSecret := secret.DeepCopy() - newSecret.Namespace = s.Namespace - newSecret.ResourceVersion = "" + newSecret := &corev1.Secret{ + ObjectMeta: metav1.ObjectMeta{ + Name: srlLicenseSecretName, + Namespace: s.Namespace, + }, + Data: secret.Data, + } log.Info("creating secret", "secret name", srlLicenseSecretName, - "namespace", s.Namespace) + ) err = r.Create(ctx, newSecret) if err != nil { @@ -66,3 +107,44 @@ func (r *SrlinuxReconciler) copyLicenseSecret( return newSecret, err } + +// updateLicenseSecret updates the secret present in Srlinux namespace with the currently +// present secret data. +func (r *SrlinuxReconciler) updateLicenseSecret( + ctx context.Context, + _ *typesv1alpha1.Srlinux, + log logr.Logger, + secret *corev1.Secret, +) (*corev1.Secret, error) { + mainSecret := &corev1.Secret{} // Secret in controller' namespace we treat as a source of truth + + // get license secret from controller' ns to copy from + // error if not found. + err := r.Get( + ctx, + types.NamespacedName{Name: srlLicenseSecretName, Namespace: controllerNamespace}, + mainSecret, + ) + if err != nil && k8serrors.IsNotFound(err) { + return nil, fmt.Errorf( + "%w: couldn't find Secret in controller's namespace", + ErrLicenseProvisioning, + ) + } + + // if secrets match, don't update the resource + if cmp.Equal(secret.Data, mainSecret.Data) { + log.Info("secret already exists, not updating") + + return secret, nil + } + + log.Info("updating the secret") + + err = r.Update(ctx, secret) + if err != nil { + return nil, err + } + + return secret, err +} diff --git a/controllers/srlinux_controller.go b/controllers/srlinux_controller.go index 1d9f2a7..2d31f7e 100644 --- a/controllers/srlinux_controller.go +++ b/controllers/srlinux_controller.go @@ -155,17 +155,7 @@ func (r *SrlinuxReconciler) checkSrlinuxPod( srlinux *typesv1alpha1.Srlinux, found *corev1.Pod, ) (ctrl.Result, bool, error) { - // get NOS version - v, err := srlinux.Spec.GetImageVersion() - if err != nil { - return ctrl.Result{}, true, err - } - - log.Info("SR Linux image version parsed", "version", v) - - srlinux.NOSVersion = v - - err = r.Get(ctx, types.NamespacedName{Name: srlinux.Name, Namespace: srlinux.Namespace}, found) + err := r.Get(ctx, types.NamespacedName{Name: srlinux.Name, Namespace: srlinux.Namespace}, found) if err != nil && errors.IsNotFound(err) { err = createConfigMaps(ctx, r, srlinux, log) if err != nil { From 7bc35c1741ad33f4733db98bf054f253689ca8f8 Mon Sep 17 00:00:00 2001 From: Roman Dodin Date: Tue, 2 Aug 2022 10:09:20 +0200 Subject: [PATCH 2/2] udpate clientset to use pointers for option structs --- api/clientset/v1alpha1/srlinux.go | 38 +++++++++++++++++-------------- 1 file changed, 21 insertions(+), 17 deletions(-) diff --git a/api/clientset/v1alpha1/srlinux.go b/api/clientset/v1alpha1/srlinux.go index d702ef6..772eb21 100644 --- a/api/clientset/v1alpha1/srlinux.go +++ b/api/clientset/v1alpha1/srlinux.go @@ -25,21 +25,25 @@ var ErrUpdateFailed = errors.New("operation update failed") // SrlinuxInterface provides access to the Srlinux CRD. type SrlinuxInterface interface { - List(ctx context.Context, opts metav1.ListOptions) (*typesv1alpha1.SrlinuxList, error) - Get(ctx context.Context, name string, options metav1.GetOptions) (*typesv1alpha1.Srlinux, error) + List(ctx context.Context, opts *metav1.ListOptions) (*typesv1alpha1.SrlinuxList, error) + Get( + ctx context.Context, + name string, + options *metav1.GetOptions, + ) (*typesv1alpha1.Srlinux, error) Create(ctx context.Context, srlinux *typesv1alpha1.Srlinux) (*typesv1alpha1.Srlinux, error) - Delete(ctx context.Context, name string, opts metav1.DeleteOptions) error - Watch(ctx context.Context, opts metav1.ListOptions) (watch.Interface, error) + Delete(ctx context.Context, name string, opts *metav1.DeleteOptions) error + Watch(ctx context.Context, opts *metav1.ListOptions) (watch.Interface, error) Unstructured( ctx context.Context, name string, - opts metav1.GetOptions, + opts *metav1.GetOptions, subresources ...string, ) (*unstructured.Unstructured, error) Update( ctx context.Context, obj *unstructured.Unstructured, - opts metav1.UpdateOptions, + opts *metav1.UpdateOptions, ) (*typesv1alpha1.Srlinux, error) } @@ -104,14 +108,14 @@ type srlinuxClient struct { // List gets a list of SRLinux resources. func (s *srlinuxClient) List( ctx context.Context, - opts metav1.ListOptions, + opts *metav1.ListOptions, ) (*typesv1alpha1.SrlinuxList, error) { result := typesv1alpha1.SrlinuxList{} err := s.restClient. Get(). Namespace(s.ns). Resource(gvr.Resource). - VersionedParams(&opts, scheme.ParameterCodec). + VersionedParams(opts, scheme.ParameterCodec). Do(ctx). Into(&result) @@ -122,7 +126,7 @@ func (s *srlinuxClient) List( func (s *srlinuxClient) Get( ctx context.Context, name string, - opts metav1.GetOptions, + opts *metav1.GetOptions, ) (*typesv1alpha1.Srlinux, error) { result := typesv1alpha1.Srlinux{} err := s.restClient. @@ -130,7 +134,7 @@ func (s *srlinuxClient) Get( Namespace(s.ns). Resource(gvr.Resource). Name(name). - VersionedParams(&opts, scheme.ParameterCodec). + VersionedParams(opts, scheme.ParameterCodec). Do(ctx). Into(&result) @@ -156,7 +160,7 @@ func (s *srlinuxClient) Create( func (s *srlinuxClient) Watch( ctx context.Context, - opts metav1.ListOptions, + opts *metav1.ListOptions, ) (watch.Interface, error) { opts.Watch = true @@ -164,16 +168,16 @@ func (s *srlinuxClient) Watch( Get(). Namespace(s.ns). Resource(gvr.Resource). - VersionedParams(&opts, scheme.ParameterCodec). + VersionedParams(opts, scheme.ParameterCodec). Watch(ctx) } -func (s *srlinuxClient) Delete(ctx context.Context, name string, opts metav1.DeleteOptions) error { +func (s *srlinuxClient) Delete(ctx context.Context, name string, opts *metav1.DeleteOptions) error { return s.restClient. Delete(). Namespace(s.ns). Resource(gvr.Resource). - VersionedParams(&opts, scheme.ParameterCodec). + VersionedParams(opts, scheme.ParameterCodec). Name(name). Do(ctx). Error() @@ -182,7 +186,7 @@ func (s *srlinuxClient) Delete(ctx context.Context, name string, opts metav1.Del func (s *srlinuxClient) Update( ctx context.Context, obj *unstructured.Unstructured, - opts metav1.UpdateOptions, + _ *metav1.UpdateOptions, ) (*typesv1alpha1.Srlinux, error) { result := typesv1alpha1.Srlinux{} @@ -202,10 +206,10 @@ func (s *srlinuxClient) Update( func (s *srlinuxClient) Unstructured( ctx context.Context, name string, - opts metav1.GetOptions, + opts *metav1.GetOptions, subresources ...string, ) (*unstructured.Unstructured, error) { - return s.dInterface.Namespace(s.ns).Get(ctx, name, opts, subresources...) + return s.dInterface.Namespace(s.ns).Get(ctx, name, *opts, subresources...) } func init() {