Skip to content

Commit fdc490c

Browse files
WarashiWarashi
authored
Make k8s plugin fields private (#5441)
* Move findConfigsAndSecrets to provider package Signed-off-by: Shinnosuke Sawada-Dazai <[email protected]> * Move FindUpdatedWorkloads to provider package Signed-off-by: Shinnosuke Sawada-Dazai <[email protected]> * Rename FindUpdatedWorkloads to FindSameWorkloads because it does not treat either the workloads are changed or not Signed-off-by: Shinnosuke Sawada-Dazai <[email protected]> * Make ResoruceKey's fields private Signed-off-by: Shinnosuke Sawada-Dazai <[email protected]> * Add godoc comments Signed-off-by: Shinnosuke Sawada-Dazai <[email protected]> * Make Manifest's fields private Signed-off-by: Shinnosuke Sawada-Dazai <[email protected]> * Fix the failed tests Signed-off-by: Shinnosuke Sawada-Dazai <[email protected]> --------- Signed-off-by: Shinnosuke Sawada-Dazai <[email protected]>
1 parent e00b429 commit fdc490c

18 files changed

+847
-740
lines changed

‎pkg/app/pipedv1/plugin/kubernetes/deployment/annotate.go

+7-7
Original file line numberDiff line numberDiff line change
@@ -31,12 +31,12 @@ func annotateConfigHash(manifests []provider.Manifest) error {
3131
configMaps := make(map[string]provider.Manifest)
3232
secrets := make(map[string]provider.Manifest)
3333
for _, m := range manifests {
34-
if m.Key.IsConfigMap() {
35-
configMaps[m.Key.Name] = m
34+
if m.Key().IsConfigMap() {
35+
configMaps[m.Key().Name()] = m
3636
continue
3737
}
38-
if m.Key.IsSecret() {
39-
secrets[m.Key.Name] = m
38+
if m.Key().IsSecret() {
39+
secrets[m.Key().Name()] = m
4040
}
4141
}
4242

@@ -47,7 +47,7 @@ func annotateConfigHash(manifests []provider.Manifest) error {
4747
}
4848

4949
for _, m := range manifests {
50-
if m.Key.IsDeployment() {
50+
if m.Key().IsDeployment() {
5151
if err := annotateConfigHashToWorkload(m, configMaps, secrets); err != nil {
5252
return err
5353
}
@@ -60,8 +60,8 @@ func annotateConfigHash(manifests []provider.Manifest) error {
6060
}
6161

6262
func annotateConfigHashToWorkload(m provider.Manifest, managedConfigMaps, managedSecrets map[string]provider.Manifest) error {
63-
configMaps := provider.FindReferencingConfigMaps(m.Body)
64-
secrets := provider.FindReferencingSecrets(m.Body)
63+
configMaps := provider.FindReferencingConfigMaps(m)
64+
secrets := provider.FindReferencingSecrets(m)
6565

6666
// The deployment is not referencing any config resources.
6767
if len(configMaps)+len(secrets) == 0 {

‎pkg/app/pipedv1/plugin/kubernetes/deployment/apply.go

+10-10
Original file line numberDiff line numberDiff line change
@@ -34,43 +34,43 @@ func applyManifests(ctx context.Context, applier applier, manifests []provider.M
3434
// 1. force-sync-by-replace
3535
// 2. sync-by-replace
3636
// 3. others
37-
if annotation := m.Body.GetAnnotations()[provider.LabelForceSyncReplace]; annotation == provider.UseReplaceEnabled {
37+
if annotation := m.GetAnnotations()[provider.LabelForceSyncReplace]; annotation == provider.UseReplaceEnabled {
3838
// Always try to replace first and create if it fails due to resource not found error.
3939
// This is because we cannot know whether resource already exists before executing command.
4040
err := applier.ForceReplaceManifest(ctx, m)
4141
if errors.Is(err, provider.ErrNotFound) {
42-
lp.Infof("Specified resource does not exist, so create the resource: %s (%w)", m.Key.ReadableString(), err)
42+
lp.Infof("Specified resource does not exist, so create the resource: %s (%w)", m.Key().ReadableString(), err)
4343
err = applier.CreateManifest(ctx, m)
4444
}
4545
if err != nil {
46-
lp.Errorf("Failed to forcefully replace or create manifest: %s (%w)", m.Key.ReadableString(), err)
46+
lp.Errorf("Failed to forcefully replace or create manifest: %s (%w)", m.Key().ReadableString(), err)
4747
return err
4848
}
49-
lp.Successf("- forcefully replaced or created manifest: %s", m.Key.ReadableString())
49+
lp.Successf("- forcefully replaced or created manifest: %s", m.Key().ReadableString())
5050
continue
5151
}
5252

53-
if annotation := m.Body.GetAnnotations()[provider.LabelSyncReplace]; annotation == provider.UseReplaceEnabled {
53+
if annotation := m.GetAnnotations()[provider.LabelSyncReplace]; annotation == provider.UseReplaceEnabled {
5454
// Always try to replace first and create if it fails due to resource not found error.
5555
// This is because we cannot know whether resource already exists before executing command.
5656
err := applier.ReplaceManifest(ctx, m)
5757
if errors.Is(err, provider.ErrNotFound) {
58-
lp.Infof("Specified resource does not exist, so create the resource: %s (%w)", m.Key.ReadableString(), err)
58+
lp.Infof("Specified resource does not exist, so create the resource: %s (%w)", m.Key().ReadableString(), err)
5959
err = applier.CreateManifest(ctx, m)
6060
}
6161
if err != nil {
62-
lp.Errorf("Failed to replace or create manifest: %s (%w)", m.Key.ReadableString(), err)
62+
lp.Errorf("Failed to replace or create manifest: %s (%w)", m.Key().ReadableString(), err)
6363
return err
6464
}
65-
lp.Successf("- replaced or created manifest: %s", m.Key.ReadableString())
65+
lp.Successf("- replaced or created manifest: %s", m.Key().ReadableString())
6666
continue
6767
}
6868

6969
if err := applier.ApplyManifest(ctx, m); err != nil {
70-
lp.Errorf("Failed to apply manifest: %s (%w)", m.Key.ReadableString(), err)
70+
lp.Errorf("Failed to apply manifest: %s (%w)", m.Key().ReadableString(), err)
7171
return err
7272
}
73-
lp.Successf("- applied manifest: %s", m.Key.ReadableString())
73+
lp.Successf("- applied manifest: %s", m.Key().ReadableString())
7474
continue
7575

7676
}

‎pkg/app/pipedv1/plugin/kubernetes/deployment/determine.go

+13-79
Original file line numberDiff line numberDiff line change
@@ -20,7 +20,6 @@ import (
2020
"strings"
2121

2222
"go.uber.org/zap"
23-
"k8s.io/apimachinery/pkg/apis/meta/v1/unstructured"
2423

2524
"github.com/pipe-cd/pipecd/pkg/app/pipedv1/plugin/kubernetes/config"
2625
"github.com/pipe-cd/pipecd/pkg/app/pipedv1/plugin/kubernetes/provider"
@@ -51,32 +50,8 @@ func parseContainerImage(image string) (img containerImage) {
5150
func determineVersions(manifests []provider.Manifest) ([]*model.ArtifactVersion, error) {
5251
imageMap := map[string]struct{}{}
5352
for _, m := range manifests {
54-
// TODO: we should consider other fields like spec.jobTempate.spec.template.spec.containers because CronJob uses this format.
55-
containers, ok, err := unstructured.NestedSlice(m.Body.Object, "spec", "template", "spec", "containers")
56-
if err != nil {
57-
// if the containers field is not an array, it will return an error.
58-
// we define this as error because the 'containers' is plural form, so it should be an array.
59-
return nil, err
60-
}
61-
if !ok {
62-
continue
63-
}
64-
// Remove duplicate images on multiple manifests.
65-
for _, c := range containers {
66-
m, ok := c.(map[string]interface{})
67-
if !ok {
68-
// TODO: Add logging.
69-
continue
70-
}
71-
img, ok := m["image"]
72-
if !ok {
73-
continue
74-
}
75-
imgStr, ok := img.(string)
76-
if !ok {
77-
return nil, fmt.Errorf("invalid image format: %T(%v)", img, img)
78-
}
79-
imageMap[imgStr] = struct{}{}
53+
for _, c := range provider.FindContainerImages(m) {
54+
imageMap[c] = struct{}{}
8055
}
8156
}
8257

@@ -98,10 +73,10 @@ func determineVersions(manifests []provider.Manifest) ([]*model.ArtifactVersion,
9873
func findManifests(kind, name string, manifests []provider.Manifest) []provider.Manifest {
9974
out := make([]provider.Manifest, 0, len(manifests))
10075
for _, m := range manifests {
101-
if m.Body.GetKind() != kind {
76+
if m.Key().Kind() != kind {
10277
continue
10378
}
104-
if name != "" && m.Body.GetName() != name {
79+
if name != "" && m.Key().Name() != name {
10580
continue
10681
}
10782
out = append(out, m)
@@ -133,47 +108,6 @@ type workloadPair struct {
133108
new provider.Manifest
134109
}
135110

136-
func findUpdatedWorkloads(olds, news []provider.Manifest) []workloadPair {
137-
pairs := make([]workloadPair, 0)
138-
oldMap := make(map[provider.ResourceKey]provider.Manifest, len(olds))
139-
nomalizeKey := func(k provider.ResourceKey) provider.ResourceKey {
140-
// Ignoring APIVersion because user can upgrade to the new APIVersion for the same workload.
141-
k.APIVersion = ""
142-
if k.Namespace == provider.DefaultNamespace {
143-
k.Namespace = ""
144-
}
145-
return k
146-
}
147-
for _, m := range olds {
148-
key := nomalizeKey(m.Key)
149-
oldMap[key] = m
150-
}
151-
for _, n := range news {
152-
key := nomalizeKey(n.Key)
153-
if o, ok := oldMap[key]; ok {
154-
pairs = append(pairs, workloadPair{
155-
old: o,
156-
new: n,
157-
})
158-
}
159-
}
160-
return pairs
161-
}
162-
163-
// findConfigsAndSecrets returns the manifests that are ConfigMap or Secret.
164-
func findConfigsAndSecrets(manifests []provider.Manifest) map[provider.ResourceKey]provider.Manifest {
165-
configs := make(map[provider.ResourceKey]provider.Manifest)
166-
for _, m := range manifests {
167-
if m.Key.IsConfigMap() {
168-
configs[m.Key] = m
169-
}
170-
if m.Key.IsSecret() {
171-
configs[m.Key] = m
172-
}
173-
}
174-
return configs
175-
}
176-
177111
func checkImageChange(ns diff.Nodes) (string, bool) {
178112
const containerImageQuery = `^spec\.template\.spec\.containers\.\d+.image$`
179113
nodes, _ := ns.Find(containerImageQuery)
@@ -234,32 +168,32 @@ func determineStrategy(olds, news []provider.Manifest, workloadRefs []config.K8s
234168
return model.SyncStrategy_QUICK_SYNC, "Quick sync by applying all manifests because it was unable to find workloads in the new manifests"
235169
}
236170

237-
workloads := findUpdatedWorkloads(oldWorkloads, newWorkloads)
171+
workloads := provider.FindSameManifests(oldWorkloads, newWorkloads)
238172
diffs := make(map[provider.ResourceKey]diff.Nodes, len(workloads))
239173

240174
for _, w := range workloads {
241175
// If the workload's pod template was touched
242176
// do progressive deployment with the specified pipeline.
243-
diffResult, err := provider.Diff(w.old, w.new, logger)
177+
diffResult, err := provider.Diff(w.Old, w.New, logger)
244178
if err != nil {
245179
return model.SyncStrategy_PIPELINE, fmt.Sprintf("Sync progressively due to an error while calculating the diff (%v)", err)
246180
}
247181
diffNodes := diffResult.Nodes()
248-
diffs[w.new.Key] = diffNodes
182+
diffs[w.New.Key()] = diffNodes
249183

250184
templateDiffs := diffNodes.FindByPrefix("spec.template")
251185
if len(templateDiffs) > 0 {
252186
if msg, changed := checkImageChange(templateDiffs); changed {
253187
return model.SyncStrategy_PIPELINE, msg
254188
}
255-
return model.SyncStrategy_PIPELINE, fmt.Sprintf("Sync progressively because pod template of workload %s was changed", w.new.Key.Name)
189+
return model.SyncStrategy_PIPELINE, fmt.Sprintf("Sync progressively because pod template of workload %s was changed", w.New.Key().Name())
256190
}
257191
}
258192

259193
// If the config/secret was touched, we also need to do progressive
260194
// deployment to check run with the new config/secret content.
261-
oldConfigs := findConfigsAndSecrets(olds)
262-
newConfigs := findConfigsAndSecrets(news)
195+
oldConfigs := provider.FindConfigsAndSecrets(olds)
196+
newConfigs := provider.FindConfigsAndSecrets(news)
263197
if len(oldConfigs) > len(newConfigs) {
264198
return model.SyncStrategy_PIPELINE, fmt.Sprintf("Sync progressively because %d configmap/secret deleted", len(oldConfigs)-len(newConfigs))
265199
}
@@ -269,22 +203,22 @@ func determineStrategy(olds, news []provider.Manifest, workloadRefs []config.K8s
269203
for k, oc := range oldConfigs {
270204
nc, ok := newConfigs[k]
271205
if !ok {
272-
return model.SyncStrategy_PIPELINE, fmt.Sprintf("Sync progressively because %s %s was deleted", oc.Key.Kind, oc.Key.Name)
206+
return model.SyncStrategy_PIPELINE, fmt.Sprintf("Sync progressively because %s %s was deleted", oc.Key().Kind(), oc.Key().Name())
273207
}
274208
result, err := provider.Diff(oc, nc, logger)
275209
if err != nil {
276210
return model.SyncStrategy_PIPELINE, fmt.Sprintf("Sync progressively due to an error while calculating the diff (%v)", err)
277211
}
278212
if result.HasDiff() {
279-
return model.SyncStrategy_PIPELINE, fmt.Sprintf("Sync progressively because %s %s was updated", oc.Key.Kind, oc.Key.Name)
213+
return model.SyncStrategy_PIPELINE, fmt.Sprintf("Sync progressively because %s %s was updated", oc.Key().Kind(), oc.Key().Name())
280214
}
281215
}
282216

283217
// Check if this is a scaling commit.
284218
scales := make([]string, 0, len(diffs))
285219
for k, d := range diffs {
286220
if before, after, changed := checkReplicasChange(d); changed {
287-
scales = append(scales, fmt.Sprintf("%s/%s from %s to %s", k.Kind, k.Name, before, after))
221+
scales = append(scales, fmt.Sprintf("%s/%s from %s to %s", k.Kind(), k.Name(), before, after))
288222
}
289223

290224
}

0 commit comments

Comments
 (0)