From 336d96d501311103dd9921bcea55722e2c8e774a Mon Sep 17 00:00:00 2001 From: Shinnosuke Sawada-Dazai Date: Wed, 25 Dec 2024 11:12:34 +0900 Subject: [PATCH] Remove `key` field from `provider.Manifest` in the k8s plugin (#5446) * Dynamically calculate resource keys Signed-off-by: Shinnosuke Sawada-Dazai * Add test for executing K8S_SYNC stage with input namespace in deployment service Signed-off-by: Shinnosuke Sawada-Dazai * Refactor deployment service tests to improve setup and reduce duplication Signed-off-by: Shinnosuke Sawada-Dazai * Remove empty line in imports Signed-off-by: Shinnosuke Sawada-Dazai * Rename Revision field to CommitHash in tests Signed-off-by: Shinnosuke Sawada-Dazai --------- Signed-off-by: Shinnosuke Sawada-Dazai --- .../kubernetes/deployment/server_test.go | 111 ++++++++++++++++-- .../kubernetes/provider/applier_test.go | 35 ++++-- .../plugin/kubernetes/provider/loader.go | 15 --- .../plugin/kubernetes/provider/loader_test.go | 35 ------ .../plugin/kubernetes/provider/manifest.go | 3 +- .../plugin/kubernetes/provider/resource.go | 2 +- 6 files changed, 126 insertions(+), 75 deletions(-) diff --git a/pkg/app/pipedv1/plugin/kubernetes/deployment/server_test.go b/pkg/app/pipedv1/plugin/kubernetes/deployment/server_test.go index cdf23fb7bc..4eb49d17f7 100644 --- a/pkg/app/pipedv1/plugin/kubernetes/deployment/server_test.go +++ b/pkg/app/pipedv1/plugin/kubernetes/deployment/server_test.go @@ -32,6 +32,7 @@ import ( "k8s.io/client-go/tools/clientcmd" clientcmdapi "k8s.io/client-go/tools/clientcmd/api" "sigs.k8s.io/controller-runtime/pkg/envtest" + "sigs.k8s.io/yaml" kubeConfigPkg "github.com/pipe-cd/pipecd/pkg/app/pipedv1/plugin/kubernetes/config" config "github.com/pipe-cd/pipecd/pkg/configv1" @@ -84,19 +85,20 @@ func kubeconfigFromRestConfig(restConfig *rest.Config) (string, error) { return string(b), nil } -func TestDeploymentService_executeK8sSyncStage(t *testing.T) { - ctx := context.Background() - - // initialize tool registry - testRegistry, err := toolregistrytest.NewToolRegistry(t) - require.NoError(t, err) +func setupEnvTest(t *testing.T) *rest.Config { + t.Helper() - // initialize envtest tEnv := new(envtest.Environment) kubeCfg, err := tEnv.Start() require.NoError(t, err) t.Cleanup(func() { tEnv.Stop() }) + return kubeCfg +} + +func setupTestPluginConfig(t *testing.T, kubeCfg *rest.Config) *config.PipedPlugin { + t.Helper() + kubeconfig, err := kubeconfigFromRestConfig(kubeCfg) require.NoError(t, err) @@ -109,7 +111,7 @@ func TestDeploymentService_executeK8sSyncStage(t *testing.T) { require.NoError(t, err) // prepare the piped plugin config - pluginCfg := &config.PipedPlugin{ + return &config.PipedPlugin{ Name: "kubernetes", URL: "file:///path/to/kubernetes/plugin", // dummy for testing Port: 0, // dummy for testing @@ -119,10 +121,28 @@ func TestDeploymentService_executeK8sSyncStage(t *testing.T) { Config: json.RawMessage(deployTarget), }}, } +} + +func setupTestPluginConfigAndDynamicClient(t *testing.T) (*config.PipedPlugin, dynamic.Interface) { + t.Helper() + kubeCfg := setupEnvTest(t) + pluginCfg := setupTestPluginConfig(t, kubeCfg) + + dynamicClient, err := dynamic.NewForConfig(kubeCfg) + require.NoError(t, err) + + return pluginCfg, dynamicClient +} + +func TestDeploymentService_executeK8sSyncStage(t *testing.T) { + ctx := context.Background() + + // read the application config from the example file cfg, err := os.ReadFile(filepath.Join(examplesDir(), "kubernetes", "simple", "app.pipecd.yaml")) require.NoError(t, err) + // prepare the request req := &deployment.ExecuteStageRequest{ Input: &deployment.ExecutePluginInput{ Deployment: &model.Deployment{ @@ -145,16 +165,19 @@ func TestDeploymentService_executeK8sSyncStage(t *testing.T) { }, } + // initialize tool registry + testRegistry, err := toolregistrytest.NewToolRegistry(t) + require.NoError(t, err) + + // initialize plugin config and dynamic client for assertions with envtest + pluginCfg, dynamicClient := setupTestPluginConfigAndDynamicClient(t) + svc := NewDeploymentService(pluginCfg, zaptest.NewLogger(t), testRegistry, logpersistertest.NewTestLogPersister(t)) resp, err := svc.ExecuteStage(ctx, req) require.NoError(t, err) assert.Equal(t, model.StageStatus_STAGE_SUCCESS.String(), resp.GetStatus().String()) - // check the deployment is created with client-go - dynamicClient, err := dynamic.NewForConfig(kubeCfg) - require.NoError(t, err) - deployment, err := dynamicClient.Resource(schema.GroupVersionResource{Group: "apps", Version: "v1", Resource: "deployments"}).Namespace("default").Get(context.Background(), "simple", metav1.GetOptions{}) require.NoError(t, err) @@ -168,3 +191,67 @@ func TestDeploymentService_executeK8sSyncStage(t *testing.T) { assert.Equal(t, "0123456789", deployment.GetAnnotations()["pipecd.dev/commit-hash"]) } + +func TestDeploymentService_executeK8sSyncStage_withInputNamespace(t *testing.T) { + ctx := context.Background() + + // read the application config from the example file + cfg, err := os.ReadFile(filepath.Join(examplesDir(), "kubernetes", "simple", "app.pipecd.yaml")) + require.NoError(t, err) + + // decode and override the autoCreateNamespace and namespace + spec, err := config.DecodeYAML[*kubeConfigPkg.KubernetesApplicationSpec](cfg) + require.NoError(t, err) + spec.Spec.Input.AutoCreateNamespace = true + spec.Spec.Input.Namespace = "test-namespace" + cfg, err = yaml.Marshal(spec) + require.NoError(t, err) + + // prepare the request + req := &deployment.ExecuteStageRequest{ + Input: &deployment.ExecutePluginInput{ + Deployment: &model.Deployment{ + PipedId: "piped-id", + ApplicationId: "app-id", + DeployTargets: []string{"default"}, + }, + Stage: &model.PipelineStage{ + Id: "stage-id", + Name: "K8S_SYNC", + }, + StageConfig: []byte(``), + RunningDeploymentSource: nil, + TargetDeploymentSource: &deployment.DeploymentSource{ + ApplicationDirectory: filepath.Join(examplesDir(), "kubernetes", "simple"), + CommitHash: "0123456789", + ApplicationConfig: cfg, + ApplicationConfigFilename: "app.pipecd.yaml", + }, + }, + } + + // initialize tool registry + testRegistry, err := toolregistrytest.NewToolRegistry(t) + require.NoError(t, err) + + // initialize plugin config and dynamic client for assertions with envtest + pluginCfg, dynamicClient := setupTestPluginConfigAndDynamicClient(t) + + svc := NewDeploymentService(pluginCfg, zaptest.NewLogger(t), testRegistry, logpersistertest.NewTestLogPersister(t)) + resp, err := svc.ExecuteStage(ctx, req) + + require.NoError(t, err) + assert.Equal(t, model.StageStatus_STAGE_SUCCESS.String(), resp.GetStatus().String()) + + deployment, err := dynamicClient.Resource(schema.GroupVersionResource{Group: "apps", Version: "v1", Resource: "deployments"}).Namespace("test-namespace").Get(context.Background(), "simple", metav1.GetOptions{}) + require.NoError(t, err) + + assert.Equal(t, "simple", deployment.GetName()) + assert.Equal(t, "simple", deployment.GetLabels()["app"]) + assert.Equal(t, "piped", deployment.GetAnnotations()["pipecd.dev/managed-by"]) + assert.Equal(t, "piped-id", deployment.GetAnnotations()["pipecd.dev/piped"]) + assert.Equal(t, "app-id", deployment.GetAnnotations()["pipecd.dev/application"]) + assert.Equal(t, "apps/v1", deployment.GetAnnotations()["pipecd.dev/original-api-version"]) + assert.Equal(t, "apps/v1:Deployment::simple", deployment.GetAnnotations()["pipecd.dev/resource-key"]) // This assertion differs from the non-plugin-arched piped's Kubernetes platform provider, but we decided to change this behavior. + assert.Equal(t, "0123456789", deployment.GetAnnotations()["pipecd.dev/commit-hash"]) +} diff --git a/pkg/app/pipedv1/plugin/kubernetes/provider/applier_test.go b/pkg/app/pipedv1/plugin/kubernetes/provider/applier_test.go index 3501246b8e..d095b15e9c 100644 --- a/pkg/app/pipedv1/plugin/kubernetes/provider/applier_test.go +++ b/pkg/app/pipedv1/plugin/kubernetes/provider/applier_test.go @@ -20,6 +20,7 @@ import ( "testing" "go.uber.org/zap" + "k8s.io/apimachinery/pkg/apis/meta/v1/unstructured" "github.com/pipe-cd/pipecd/pkg/app/pipedv1/plugin/kubernetes/config" ) @@ -164,8 +165,12 @@ func TestApplier_ApplyManifest(t *testing.T) { ) manifest := Manifest{ - key: ResourceKey{ - namespace: "test-namespace", + body: &unstructured.Unstructured{ + Object: map[string]interface{}{ + "metadata": map[string]interface{}{ + "namespace": "test-namespace", + }, + }, }, } @@ -254,8 +259,12 @@ func TestApplier_CreateManifest(t *testing.T) { ) manifest := Manifest{ - key: ResourceKey{ - namespace: "test-namespace", + body: &unstructured.Unstructured{ + Object: map[string]interface{}{ + "metadata": map[string]interface{}{ + "namespace": "test-namespace", + }, + }, }, } @@ -316,11 +325,14 @@ func TestApplier_ReplaceManifest(t *testing.T) { ) manifest := Manifest{ - key: ResourceKey{ - namespace: "test-namespace", + body: &unstructured.Unstructured{ + Object: map[string]interface{}{ + "metadata": map[string]interface{}{ + "namespace": "test-namespace", + }, + }, }, } - err := applier.ReplaceManifest(context.Background(), manifest) if !errors.Is(err, tc.expectedErr) { t.Errorf("expected error %v, got %v", tc.expectedErr, err) @@ -378,11 +390,14 @@ func TestApplier_ForceReplaceManifest(t *testing.T) { ) manifest := Manifest{ - key: ResourceKey{ - namespace: "test-namespace", + body: &unstructured.Unstructured{ + Object: map[string]interface{}{ + "metadata": map[string]interface{}{ + "namespace": "test-namespace", + }, + }, }, } - err := applier.ForceReplaceManifest(context.Background(), manifest) if !errors.Is(err, tc.expectedErr) { t.Errorf("expected error %v, got %v", tc.expectedErr, err) diff --git a/pkg/app/pipedv1/plugin/kubernetes/provider/loader.go b/pkg/app/pipedv1/plugin/kubernetes/provider/loader.go index 4a54e0b921..8cac2b9965 100644 --- a/pkg/app/pipedv1/plugin/kubernetes/provider/loader.go +++ b/pkg/app/pipedv1/plugin/kubernetes/provider/loader.go @@ -85,10 +85,6 @@ func NewLoader(registry ToolRegistry) *Loader { func (l *Loader) LoadManifests(ctx context.Context, input LoaderInput) (manifests []Manifest, err error) { defer func() { - // Override namespace if set because ParseManifests does not parse it - // if namespace is not explicitly specified in the manifests. - setNamespace(manifests, input.Namespace) - // Add builtin annotations for tracking application live state. for i := range manifests { manifests[i].AddAnnotations(map[string]string{ @@ -132,16 +128,6 @@ func (l *Loader) LoadManifests(ctx context.Context, input LoaderInput) (manifest } } -func setNamespace(manifests []Manifest, namespace string) { - if namespace == "" { - return - } - for i := range manifests { - // TODO: consider way to not modify the original object. - manifests[i].key.namespace = namespace - } -} - func sortManifests(manifests []Manifest) { if len(manifests) < 2 { return @@ -258,7 +244,6 @@ func ParseManifests(data string) ([]Manifest, error) { continue } manifests = append(manifests, Manifest{ - key: MakeResourceKey(&obj), body: &obj, }) } diff --git a/pkg/app/pipedv1/plugin/kubernetes/provider/loader_test.go b/pkg/app/pipedv1/plugin/kubernetes/provider/loader_test.go index 4b2db703b3..79223d081d 100644 --- a/pkg/app/pipedv1/plugin/kubernetes/provider/loader_test.go +++ b/pkg/app/pipedv1/plugin/kubernetes/provider/loader_test.go @@ -57,11 +57,6 @@ metadata: `, want: []Manifest{ { - key: ResourceKey{ - apiVersion: "v1", - kind: "ConfigMap", - name: "test-config", - }, body: &unstructured.Unstructured{ Object: map[string]interface{}{ "apiVersion": "v1", @@ -90,11 +85,6 @@ metadata: `, want: []Manifest{ { - key: ResourceKey{ - apiVersion: "v1", - kind: "ConfigMap", - name: "test-config", - }, body: &unstructured.Unstructured{ Object: map[string]interface{}{ "apiVersion": "v1", @@ -106,11 +96,6 @@ metadata: }, }, { - key: ResourceKey{ - apiVersion: "v1", - kind: "Service", - name: "test-service", - }, body: &unstructured.Unstructured{ Object: map[string]interface{}{ "apiVersion": "v1", @@ -188,11 +173,6 @@ metadata: }, want: []Manifest{ { - key: ResourceKey{ - apiVersion: "v1", - kind: "ConfigMap", - name: "test-config", - }, body: &unstructured.Unstructured{ Object: map[string]interface{}{ "apiVersion": "v1", @@ -230,11 +210,6 @@ metadata: }, want: []Manifest{ { - key: ResourceKey{ - apiVersion: "v1", - kind: "Service", - name: "test-service", - }, body: &unstructured.Unstructured{ Object: map[string]interface{}{ "apiVersion": "v1", @@ -271,11 +246,6 @@ metadata: }, want: []Manifest{ { - key: ResourceKey{ - apiVersion: "v1", - kind: "ConfigMap", - name: "test-config", - }, body: &unstructured.Unstructured{ Object: map[string]interface{}{ "apiVersion": "v1", @@ -287,11 +257,6 @@ metadata: }, }, { - key: ResourceKey{ - apiVersion: "v1", - kind: "Service", - name: "test-service", - }, body: &unstructured.Unstructured{ Object: map[string]interface{}{ "apiVersion": "v1", diff --git a/pkg/app/pipedv1/plugin/kubernetes/provider/manifest.go b/pkg/app/pipedv1/plugin/kubernetes/provider/manifest.go index a788309341..2b913a8cc2 100644 --- a/pkg/app/pipedv1/plugin/kubernetes/provider/manifest.go +++ b/pkg/app/pipedv1/plugin/kubernetes/provider/manifest.go @@ -24,12 +24,11 @@ import ( // Manifest represents a Kubernetes resource manifest. type Manifest struct { - key ResourceKey body *unstructured.Unstructured } func (m *Manifest) Key() ResourceKey { - return m.key + return makeResourceKey(m.body) } // UnmarshalJSON implements the json.Unmarshaler interface. diff --git a/pkg/app/pipedv1/plugin/kubernetes/provider/resource.go b/pkg/app/pipedv1/plugin/kubernetes/provider/resource.go index 313b1b4049..260a64ef47 100644 --- a/pkg/app/pipedv1/plugin/kubernetes/provider/resource.go +++ b/pkg/app/pipedv1/plugin/kubernetes/provider/resource.go @@ -96,7 +96,7 @@ func (k ResourceKey) ReadableString() string { return fmt.Sprintf("name=%q, kind=%q, namespace=%q, apiVersion=%q", k.name, k.kind, k.namespace, k.apiVersion) } -func MakeResourceKey(obj *unstructured.Unstructured) ResourceKey { +func makeResourceKey(obj *unstructured.Unstructured) ResourceKey { k := ResourceKey{ apiVersion: obj.GetAPIVersion(), kind: obj.GetKind(),