From fc572453a58f30ac09deb2a56427f4fd65be54f4 Mon Sep 17 00:00:00 2001
From: Vladimir Kozhukalov <kozhukalov@gmail.com>
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) {