From fc572453a58f30ac09deb2a56427f4fd65be54f4 Mon Sep 17 00:00:00 2001 From: Vladimir Kozhukalov Date: Tue, 9 Mar 2021 16:41:00 +0300 Subject: [PATCH] Remove unnecessary code - Use opendev.org/airship/airshipctl/pkg/document.Selector instead of sigs.k8s.io/kustomize/api/types.Selector which makes selectors conversion code unnecessary - Use document.GetSecretDataKey instead of document.DecodeSecretData which removes code duplicates Change-Id: Ie2c6b8d8222b7acb1b657f8d786a8c3a06b0c6fd --- pkg/api/v1alpha1/isoconfiguration.go | 7 +- pkg/bootstrap/cloudinit/cloud-init.go | 74 +++++---- pkg/bootstrap/cloudinit/cloud-init_test.go | 181 +++++++++------------ pkg/document/document.go | 29 ---- pkg/document/errors.go | 21 --- pkg/document/selectors.go | 34 ---- pkg/document/selectors_test.go | 10 -- 7 files changed, 121 insertions(+), 235 deletions(-) diff --git a/pkg/api/v1alpha1/isoconfiguration.go b/pkg/api/v1alpha1/isoconfiguration.go index 8b9551a71..06280232a 100644 --- a/pkg/api/v1alpha1/isoconfiguration.go +++ b/pkg/api/v1alpha1/isoconfiguration.go @@ -16,7 +16,8 @@ package v1alpha1 import ( metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" - "sigs.k8s.io/kustomize/api/types" + + "opendev.org/airship/airshipctl/pkg/document" ) // IsoContainer structure contains parameters related to Docker runtime, used for building image @@ -32,11 +33,11 @@ type IsoContainer struct { // Isogen structure defines document selection criteria for cloud-init metadata type Isogen struct { // Cloud Init user data will be retrieved from the doc matching this object - UserDataSelector types.Selector `json:"userDataSelector,omitempty"` + UserDataSelector document.Selector `json:"userDataSelector,omitempty"` // Cloud init user data will be retrieved from this document key UserDataKey string `json:"userDataKey,omitempty"` // Cloud Init network config will be retrieved from the doc matching this object - NetworkConfigSelector types.Selector `json:"networkConfigSelector,omitempty"` + NetworkConfigSelector document.Selector `json:"networkConfigSelector,omitempty"` // Cloud init network config will be retrieved from this document key NetworkConfigKey string `json:"networkConfigKey,omitempty"` // File name to use for the output image that will be written to the container volume root diff --git a/pkg/bootstrap/cloudinit/cloud-init.go b/pkg/bootstrap/cloudinit/cloud-init.go index fb8914ab8..2253128a1 100644 --- a/pkg/bootstrap/cloudinit/cloud-init.go +++ b/pkg/bootstrap/cloudinit/cloud-init.go @@ -16,51 +16,40 @@ package cloudinit import ( "opendev.org/airship/airshipctl/pkg/document" - - "sigs.k8s.io/kustomize/api/resid" - "sigs.k8s.io/kustomize/api/types" ) -var ( - // Initialize defaults where we expect to find user-data and - // network config data in manifests - userDataSelectorDefaults = types.Selector{ - Gvk: resid.Gvk{Kind: document.SecretKind}, - LabelSelector: document.EphemeralUserDataSelector, - } - userDataKeyDefault = "userData" - networkConfigSelectorDefaults = types.Selector{ - Gvk: resid.Gvk{Kind: document.BareMetalHostKind}, - LabelSelector: document.EphemeralHostSelector, - } - networkConfigKeyDefault = "networkData" +const ( + defaultUserDataKey = "userData" + defaultNetworkConfigKey = "networkData" ) // GetCloudData reads YAML document input and generates cloud-init data for // ephemeral node. func GetCloudData( docBundle document.Bundle, - userDataSelector types.Selector, + userDataSelector document.Selector, userDataKey string, - networkConfigSelector types.Selector, + networkConfigSelector document.Selector, networkConfigKey string, ) (userData []byte, netConf []byte, err error) { userDataSelectorFinal, userDataKeyFinal := applyDefaultsAndGetData( userDataSelector, - userDataSelectorDefaults, + document.SecretKind, + document.EphemeralUserDataSelector, userDataKey, - userDataKeyDefault, + defaultUserDataKey, ) - userData, err = document.GetSecretData(docBundle, userDataSelectorFinal, userDataKeyFinal) + userData, err = getUserData(docBundle, userDataSelectorFinal, userDataKeyFinal) if err != nil { return nil, nil, err } netConfSelectorFinal, netConfKeyFinal := applyDefaultsAndGetData( networkConfigSelector, - networkConfigSelectorDefaults, + document.BareMetalHostKind, + document.EphemeralHostSelector, networkConfigKey, - networkConfigKeyDefault, + defaultNetworkConfigKey, ) netConf, err = getNetworkData(docBundle, netConfSelectorFinal, netConfKeyFinal) if err != nil { @@ -71,18 +60,18 @@ func GetCloudData( } func applyDefaultsAndGetData( - docSelector types.Selector, - docSelectorDefaults types.Selector, + docSelector document.Selector, + defaultKind string, + defaultLabel string, key string, keyDefault string, -) (types.Selector, string) { +) (document.Selector, string) { // Assign defaults if there are no user supplied overrides if docSelector.Kind == "" && docSelector.Name == "" && docSelector.AnnotationSelector == "" && docSelector.LabelSelector == "" { - docSelector.Kind = docSelectorDefaults.Kind - docSelector.LabelSelector = docSelectorDefaults.LabelSelector + docSelector = docSelector.ByKind(defaultKind).ByLabel(defaultLabel) } keyFinal := key @@ -93,20 +82,37 @@ func applyDefaultsAndGetData( return docSelector, keyFinal } +func getUserData( + docBundle document.Bundle, + userDataSelector document.Selector, + userDataKey string, +) ([]byte, error) { + doc, err := docBundle.SelectOne(userDataSelector) + if err != nil { + return nil, err + } + + data, err := document.GetSecretDataKey(doc, userDataKey) + if err != nil { + return nil, err + } + + return []byte(data), nil +} + func getNetworkData( docBundle document.Bundle, - netCfgSelector types.Selector, + netCfgSelector document.Selector, netCfgKey string, ) ([]byte, error) { // find the baremetal host indicated as the ephemeral node - selector := document.NewSelector().ByKind(netCfgSelector.Kind).ByLabel(netCfgSelector.LabelSelector) - d, err := docBundle.SelectOne(selector) + d, err := docBundle.SelectOne(netCfgSelector) if err != nil { return nil, err } // try and find these documents in our bundle - selector, err = document.NewNetworkDataSelector(d) + selector, err := document.NewNetworkDataSelector(d) if err != nil { return nil, err } @@ -117,10 +123,10 @@ func getNetworkData( } // finally, try and retrieve the data we want from the document - netData, err := document.DecodeSecretData(d, netCfgKey) + netData, err := document.GetSecretDataKey(d, netCfgKey) if err != nil { return nil, err } - return netData, nil + return []byte(netData), nil } diff --git a/pkg/bootstrap/cloudinit/cloud-init_test.go b/pkg/bootstrap/cloudinit/cloud-init_test.go index 142af100f..e7fd061a1 100644 --- a/pkg/bootstrap/cloudinit/cloud-init_test.go +++ b/pkg/bootstrap/cloudinit/cloud-init_test.go @@ -19,9 +19,30 @@ import ( "github.com/stretchr/testify/require" "opendev.org/airship/airshipctl/pkg/document" +) - "sigs.k8s.io/kustomize/api/resid" - "sigs.k8s.io/kustomize/api/types" +type selectors struct { + userDataSelector document.Selector + userDataKey string + networkConfigSelector document.Selector + networkConfigKey string +} + +var ( + emptySelectors = selectors{ + userDataSelector: document.NewSelector(), + networkConfigSelector: document.NewSelector(), + } + validSelectors = selectors{ + userDataSelector: document.NewSelector(). + ByKind("Secret"). + ByLabel("airshipit.org/ephemeral-user-data in (True, true)"), + userDataKey: defaultUserDataKey, + networkConfigSelector: document.NewSelector(). + ByKind("BareMetalHost"). + ByLabel("airshipit.org/ephemeral-node in (True, true)"), + networkConfigKey: defaultNetworkConfigKey, + } ) func TestGetCloudData(t *testing.T) { @@ -29,35 +50,25 @@ func TestGetCloudData(t *testing.T) { require.NoError(t, err, "Building Bundle Failed") tests := []struct { - labelFilter string - userDataSelector types.Selector - userDataKey string - networkConfigSelector types.Selector - networkConfigKey string - expectedUserData []byte - expectedNetData []byte - expectedErr error + name string + selectors + labelFilter string + expectedUserData []byte + expectedNetData []byte + expectedErr error }{ { - labelFilter: "test=validdocset", - userDataSelector: types.Selector{}, - networkConfigSelector: types.Selector{}, - expectedUserData: []byte("cloud-init"), - expectedNetData: []byte("net-config"), - expectedErr: nil, + name: "Default selectors", + labelFilter: "test=validdocset", + selectors: emptySelectors, + expectedUserData: []byte("cloud-init"), + expectedNetData: []byte("net-config"), + expectedErr: nil, }, { - labelFilter: "test=ephemeralmissing", - userDataSelector: types.Selector{ - Gvk: resid.Gvk{Kind: "Secret"}, - LabelSelector: "airshipit.org/ephemeral-user-data in (True, true)", - }, - userDataKey: "userData", - networkConfigSelector: types.Selector{ - Gvk: resid.Gvk{Kind: "BareMetalHost"}, - LabelSelector: "airshipit.org/ephemeral-node in (True, true)", - }, - networkConfigKey: "networkData", + name: "BareMetalHost document not found", + labelFilter: "test=ephemeralmissing", + selectors: validSelectors, expectedUserData: nil, expectedNetData: nil, expectedErr: document.ErrDocNotFound{ @@ -67,17 +78,9 @@ func TestGetCloudData(t *testing.T) { }, }, { - labelFilter: "test=ephemeralduplicate", - userDataSelector: types.Selector{ - Gvk: resid.Gvk{Kind: "Secret"}, - LabelSelector: "airshipit.org/ephemeral-user-data in (True, true)", - }, - userDataKey: "userData", - networkConfigSelector: types.Selector{ - Gvk: resid.Gvk{Kind: "BareMetalHost"}, - LabelSelector: "airshipit.org/ephemeral-node in (True, true)", - }, - networkConfigKey: "networkData", + name: "BareMetalHost document duplication", + labelFilter: "test=ephemeralduplicate", + selectors: validSelectors, expectedUserData: nil, expectedNetData: nil, expectedErr: document.ErrMultiDocsFound{ @@ -87,17 +90,9 @@ func TestGetCloudData(t *testing.T) { }, }, { - labelFilter: "test=networkdatabadpointer", - userDataSelector: types.Selector{ - Gvk: resid.Gvk{Kind: "Secret"}, - LabelSelector: "airshipit.org/ephemeral-user-data in (True, true)", - }, - userDataKey: "userData", - networkConfigSelector: types.Selector{ - Gvk: resid.Gvk{Kind: "BareMetalHost"}, - LabelSelector: "airshipit.org/ephemeral-node in (True, true)", - }, - networkConfigKey: "networkData", + name: "Bad network data document reference", + labelFilter: "test=networkdatabadpointer", + selectors: validSelectors, expectedUserData: nil, expectedNetData: nil, expectedErr: document.ErrDocNotFound{ @@ -108,55 +103,31 @@ func TestGetCloudData(t *testing.T) { }, }, { - labelFilter: "test=networkdatamalformed", - userDataSelector: types.Selector{ - Gvk: resid.Gvk{Kind: "Secret"}, - LabelSelector: "airshipit.org/ephemeral-user-data in (True, true)", - }, - userDataKey: "userData", - networkConfigSelector: types.Selector{ - Gvk: resid.Gvk{Kind: "BareMetalHost"}, - LabelSelector: "airshipit.org/ephemeral-node in (True, true)", - }, - networkConfigKey: "networkData", + name: "Bad network data document structure", + labelFilter: "test=networkdatamalformed", + selectors: validSelectors, expectedUserData: nil, expectedNetData: nil, - expectedErr: document.ErrDataNotSupplied{ + expectedErr: document.ErrDocumentDataKeyNotFound{ DocName: "networkdatamalformed-malformed", - Key: networkConfigKeyDefault, + Key: defaultNetworkConfigKey, }, }, { - labelFilter: "test=userdatamalformed", - userDataSelector: types.Selector{ - Gvk: resid.Gvk{Kind: "Secret"}, - LabelSelector: "airshipit.org/ephemeral-user-data in (True, true)", - }, - userDataKey: "userData", - networkConfigSelector: types.Selector{ - Gvk: resid.Gvk{Kind: "BareMetalHost"}, - LabelSelector: "airshipit.org/ephemeral-node in (True, true)", - }, - networkConfigKey: "networkData", + name: "Bad user data document structure", + labelFilter: "test=userdatamalformed", + selectors: validSelectors, expectedUserData: nil, expectedNetData: nil, - expectedErr: document.ErrDataNotSupplied{ + expectedErr: document.ErrDocumentDataKeyNotFound{ DocName: "userdatamalformed-somesecret", - Key: userDataKeyDefault, + Key: defaultUserDataKey, }, }, { - labelFilter: "test=userdatamissing", - userDataSelector: types.Selector{ - Gvk: resid.Gvk{Kind: "Secret"}, - LabelSelector: "airshipit.org/ephemeral-user-data in (True, true)", - }, - userDataKey: "userData", - networkConfigSelector: types.Selector{ - Gvk: resid.Gvk{Kind: "BareMetalHost"}, - LabelSelector: "airshipit.org/ephemeral-node in (True, true)", - }, - networkConfigKey: "networkData", + name: "User data document not found", + labelFilter: "test=userdatamissing", + selectors: validSelectors, expectedUserData: nil, expectedNetData: nil, expectedErr: document.ErrDocNotFound{ @@ -168,26 +139,28 @@ func TestGetCloudData(t *testing.T) { } for _, tt := range tests { - // prune the bundle down using the label filter for the specific test - selector := document.NewSelector().ByLabel(tt.labelFilter) - filteredBundle, err := bundle.SelectBundle(selector) - require.NoError(t, err, "Building filtered bundle for %s failed", tt.labelFilter) + t.Run(tt.name, func(t *testing.T) { + // prune the bundle down using the label filter for the specific test + selector := document.NewSelector().ByLabel(tt.labelFilter) + filteredBundle, err := bundle.SelectBundle(selector) + require.NoError(t, err, "Building filtered bundle for %s failed", tt.labelFilter) - // ensure each test case filter has at least one document - docs, err := filteredBundle.GetAllDocuments() - require.NoError(t, err, "GetAllDocuments failed") - require.NotZero(t, docs) + // ensure each test case filter has at least one document + docs, err := filteredBundle.GetAllDocuments() + require.NoError(t, err, "GetAllDocuments failed") + require.NotZero(t, docs) - actualUserData, actualNetData, actualErr := GetCloudData( - filteredBundle, - tt.userDataSelector, - tt.userDataKey, - tt.networkConfigSelector, - tt.networkConfigKey, - ) + actualUserData, actualNetData, actualErr := GetCloudData( + filteredBundle, + tt.userDataSelector, + tt.userDataKey, + tt.networkConfigSelector, + tt.networkConfigKey, + ) - assert.Equal(t, tt.expectedUserData, actualUserData) - assert.Equal(t, tt.expectedNetData, actualNetData) - assert.Equal(t, tt.expectedErr, actualErr) + assert.Equal(t, tt.expectedUserData, actualUserData) + assert.Equal(t, tt.expectedNetData, actualNetData) + assert.Equal(t, tt.expectedErr, actualErr) + }) } } diff --git a/pkg/document/document.go b/pkg/document/document.go index c27e104d7..78027d1e9 100644 --- a/pkg/document/document.go +++ b/pkg/document/document.go @@ -15,7 +15,6 @@ package document import ( - b64 "encoding/base64" "fmt" "k8s.io/apimachinery/pkg/runtime" @@ -298,31 +297,3 @@ func NewDocumentFromBytes(b []byte) (Document, error) { err = doc.SetKustomizeResource(res) return doc, err } - -// DecodeSecretData returns base64-decoded secret data -func DecodeSecretData(cfg Document, key string) ([]byte, error) { - var needsBase64Decode = false - - // TODO(alanmeadows): distinguish between missing key - // and missing data/stringData keys in the Secret - data, err := cfg.GetStringMap("data") - if err == nil { - needsBase64Decode = true - } else { - // we'll catch any error below - data, err = cfg.GetStringMap("stringData") - if err != nil { - return nil, ErrDataNotSupplied{DocName: cfg.GetName(), Key: "data or stringData"} - } - } - - res, ok := data[key] - if !ok { - return nil, ErrDataNotSupplied{DocName: cfg.GetName(), Key: key} - } - - if needsBase64Decode { - return b64.StdEncoding.DecodeString(res) - } - return []byte(res), nil -} diff --git a/pkg/document/errors.go b/pkg/document/errors.go index c0dc36720..f66a07a41 100644 --- a/pkg/document/errors.go +++ b/pkg/document/errors.go @@ -49,19 +49,6 @@ type ErrRuntimeObjectKind struct { Obj runtime.Object } -// ErrDataNotSupplied error returned of no data in the Secret -type ErrDataNotSupplied struct { - DocName string - Key string -} - -// ErrDuplicateNetworkDataDocuments error returned if multiple matching documents -// were found with the same name in the same namespace -type ErrDuplicateNetworkDataDocuments struct { - DocName string - Namespace string -} - // ErrBadValueFormat returned if wrong field type requested type ErrBadValueFormat struct { Value string @@ -89,14 +76,6 @@ func (e ErrRuntimeObjectKind) Error() string { return fmt.Sprintf("object %#v has either none or multiple kinds in scheme (expected one)", e.Obj) } -func (e ErrDataNotSupplied) Error() string { - return fmt.Sprintf("Document %s has no key %s", e.DocName, e.Key) -} - -func (e ErrDuplicateNetworkDataDocuments) Error() string { - return fmt.Sprintf("Found more than one document with the name %s in namespace %s", e.DocName, e.Namespace) -} - func (e ErrBadValueFormat) Error() string { return fmt.Sprintf("value of %s expected to have %s type, got %s", e.Value, e.Expected, e.Actual) } diff --git a/pkg/document/selectors.go b/pkg/document/selectors.go index 4f42c645d..b6ce34a0d 100644 --- a/pkg/document/selectors.go +++ b/pkg/document/selectors.go @@ -197,37 +197,3 @@ func NewValidatorExecutorSelector() Selector { DocumentValidationKind). ByName(DocumentValidationName) } - -//GetSecretData returns data located with a given key of a given document -func GetSecretData(docBundle Bundle, apiSelector types.Selector, key string) ([]byte, error) { - s := NewSelector() - if apiSelector.Group != "" && apiSelector.Version != "" && apiSelector.Kind != "" { - s = s.ByGvk(apiSelector.Group, apiSelector.Version, apiSelector.Kind) - } else if apiSelector.Kind != "" { - s = s.ByKind(apiSelector.Kind) - } - if apiSelector.Namespace != "" { - s = s.ByNamespace(apiSelector.Namespace) - } - if apiSelector.Name != "" { - s = s.ByName(apiSelector.Name) - } - if apiSelector.AnnotationSelector != "" { - s = s.ByAnnotation(apiSelector.AnnotationSelector) - } - if apiSelector.LabelSelector != "" { - s = s.ByLabel(apiSelector.LabelSelector) - } - - doc, docErr := docBundle.SelectOne(s) - if docErr != nil { - return nil, docErr - } - - // finally, try and retrieve the data we want from the document - data, keyErr := DecodeSecretData(doc, key) - if keyErr != nil { - return nil, keyErr - } - return data, nil -} diff --git a/pkg/document/selectors_test.go b/pkg/document/selectors_test.go index 0ddd97f38..f87a91ed1 100644 --- a/pkg/document/selectors_test.go +++ b/pkg/document/selectors_test.go @@ -60,16 +60,6 @@ func TestSelectorsPositive(t *testing.T) { require.NoError(t, err) assert.Len(t, doc, 1) }) - - t.Run("TestGetSecretData", func(t *testing.T) { - data, err := document.GetSecretData(bundle, types.Selector{ - Gvk: resid.Gvk{Kind: "Secret"}, - LabelSelector: "airshipit.org/ephemeral-user-data", - }, - "userData") - require.NoError(t, err) - assert.Equal(t, []byte("cloud-init"), data) - }) } func TestSelectorsNegative(t *testing.T) {