Change resource key logic for k8s #4916

ffjlabo · 2024-05-15T13:31:00Z

What this PR does / why we need it:

This fix will solve the problem that the cluster-scoped resource can't be deleted when we set the namespace in app.pipecd.yaml.
context: #4269 (comment)

expected behavior

We can delete the cluster-scoped resource.

livestate side

use "" for the cluster-scoped resource
use the namespace for the namespace scoped resource

When reading the manifests from git

use "" for the cluster-scoped resource
for the namespace scoped resource,
- use spec.input.namespace in app.pipecd.yaml if it is set.
- use the namespace on the manifest from git if it is set.
- use default if both of spec.input.namespace and the namespace are not set

Currently, we use the resource key to identify each k8s resource.
It is created by MakeResourceKey, and the rule is below.

livestate side

use default when the resource obj doesn't have the namespace.
use the namespace when the resource obj has the namespace.

When reading the manifests from git

use spec.input.namespace in app.pipecd.yaml if it is set.
use the namespace on the manifest from git if it is set.
use default when the namespace on the manifest is "".

This rule doesn't consider the cluster-scoped resource.
So for example, cluster-scoped resource don't have any namespace, but if we set the spec.input.namespace in the app.pipecd.yaml, it sets the value as the namespace to the resource key.

This causes the problem that can't prune the resource because the resource key can't identify the resource correctly.
So I fixed the the logic like below.

livestate side

use "" for the cluster-scoped resource
use the namespace for the namespace scoped resource

When reading the manifests from git

use "" for the cluster-scoped resource
for the namespace scoped resource,
- use spec.input.namespace in app.pipecd.yaml if it is set.
- use the namespace on the manifest from git if it is set.
- use default if both of spec.input.namespace and the namespace are not set

Which issue(s) this PR fixes:

Part of #4269

Does this PR introduce a user-facing change?:
How are users affected by this change:
Is this breaking change:
How to migrate (if breaking change):

ffjlabo · 2024-05-16T09:59:55Z

The Overview of the fix

refine the namespace by loader.refineNamespace when reading the manifests from git repo

fixed to obtain information to determine whether each k8s resource is cluster scoped when executor, planner, plan preview, and detector start. (provider.GetIsNamespacedResources)

remove the logic to determine the namespace from MakeResourceKey

I think MakeResourceKey shouldn't determine the namespace. This is only loader's responsibility.

ffjlabo · 2024-05-16T10:02:38Z

@khanhtc1202 @t-kikuc
This issue is complex and my explanation may be insufficient. If you have any questions, please feel free to ask. :) 🙏

khanhtc1202 · 2024-05-16T13:50:58Z

@ffjlabo some testcases are failed 👀

ffjlabo · 2024-05-17T04:44:01Z

@khanhtc1202 Sorry, I fixed some of them. 🙏
There are some failings, but these are under the /pipecdv1. I want to fix it after the current implementation looks good.

t-kikuc · 2024-05-27T02:16:28Z

/review

github-actions · 2024-05-27T02:17:05Z

PR Analysis

Main theme

Enhancement and Refactoring

PR summary

This PR introduces enhancements and refactoring to various components that interact with Kubernetes resources, such as planners, appliers, and detection mechanisms. It involves propagating the knowledge of namespaced versus non-namespaced resources through the system.

Type of PR

Enhancement, Refactoring

PR Feedback:

General suggestions

This PR contains quite a large set of changes across multiple components, focusing on ensuring that the system properly respects namespaced and cluster-wide resources in Kubernetes. While the PR introduces a central mechanism to define whether a GroupVersionKind is namespaced, it's vital to ensure that this information remains consistent and the system's behavior follows the cluster's actual state. Testing these changes in a staging environment before release would be highly recommended to catch any edge cases or regressions.

Code feedback

relevant file: pkg/app/piped/platformprovider/kubernetes/loader.go
suggestion: |-
It is essential to guarantee that the information regarding namespaced resources is always fresh and accurate. As this is probably cached information and the cluster's state may change (e.g., CRDs being added/removed), consider adding a refresh mechanism to update the isNamespacedResources map periodically or when a certain type of error suggests it might be out of date. (important)
relevant line: + isNamespacedResources: make(map[schema.GroupVersionKind]bool),
relevant file: pkg/app/piped/platformprovider/kubernetes/loader.go
suggestion: |-
There appears to be some unused or deleted code in the LoadManifests method, specifically regarding the initiation of Nashorn VM that was previously removed. Ensure that any references or initialization logic related to removed components are cleaned up to avoid confusion. (medium)
relevant line: l.initOnce.Do(func() {
relevant key: pkg/app/piped/platformprovider/kubernetes/loader.go
suggestion: |-
Ensure that error messages are informative and actionable. For example, when handling an 'unknown resource kind', the error message should suggest what actions a developer might take or indicate if this is an expected case during normal operations or an exceptional scenario. (medium)
relevant line: + return fmt.Errorf("unknown resource 登录 [PII] %s", m.u.GroupVersionKind().String())

Security concerns:

no

The code changes introduced in this PR do not seem to introduce any obvious security concerns such as SQL injection, XSS, or CSRF vulnerabilities. The updates mostly deal with internal configuration and state management concerning Kubernetes resources. However, the security of the mechanism used to determine the namespaced status of Kubernetes resources should be considered to ensure that it cannot be misused or lead to incorrect assumptions about resource accessibility and isolation.

khanhtc1202 · 2024-05-29T04:49:37Z

@ffjlabo Please rebase this PR due to pipedv1 package change 👀

Signed-off-by: Yoshiki Fujikane <ffjlabo@gmail.com>

Details: - [feat] get the resource info from the actual cluster in the deployExecutor.Execute - [faet] implement the loader.refineNamespace to infer the namespace from the manifests and app.pipecd.yaml stored in the git repo - [refactor] fix loader.NewLoader to pass isNamespacedResource - deployExecutor - rollbackExecutor - [refactor] fix to pass isNamespacedResource on detector - [memo] detector checks the diff by 1 minute. It might think about the amount of the traffic to the k8s cluster. - [refactor] fix to pass isNamespacedResource on planner - [refactor] fix to pass isNamespacedResource on planpreview - [refactor] remove the logic to fix the namespace when craeting the resource key on MakeResourceKey - maybe this is the refactoring for livestatestore - [refactor] use the actual resource key, not the annotation's one. Signed-off-by: Yoshiki Fujikane <ffjlabo@gmail.com>

Signed-off-by: Yoshiki Fujikane <ffjlabo@gmail.com>

codecov · 2024-05-29T04:56:24Z

Codecov Report

Attention: Patch coverage is 10.05291% with 170 lines in your changes are missing coverage. Please review.

Project coverage is 29.31%. Comparing base (6a3a5d0) to head (8adaa0b).
Report is 3 commits behind head on master.

Files	Patch %	Lines
pkg/app/piped/planpreview/kubernetesdiff.go	0.00%	33 Missing ⚠️
pkg/app/piped/executor/kubernetes/rollback.go	0.00%	32 Missing ⚠️
...pp/piped/platformprovider/kubernetes/kubernetes.go	0.00%	20 Missing ⚠️
pkg/app/piped/executor/kubernetes/primary.go	0.00%	19 Missing ⚠️
pkg/app/piped/executor/kubernetes/baseline.go	0.00%	17 Missing ⚠️
pkg/app/piped/planner/kubernetes/kubernetes.go	0.00%	17 Missing ⚠️
...kg/app/piped/platformprovider/kubernetes/loader.go	57.57%	14 Missing ⚠️
pkg/app/piped/executor/kubernetes/kubernetes.go	0.00%	13 Missing ⚠️
pkg/app/piped/planpreview/builder.go	0.00%	4 Missing ⚠️
...g/app/piped/platformprovider/kubernetes/applier.go	0.00%	1 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##           master    #4916      +/-   ##
==========================================
- Coverage   29.33%   29.31%   -0.03%     
==========================================
  Files         322      323       +1     
  Lines       40868    40984     +116     
==========================================
+ Hits        11989    12014      +25     
- Misses      27919    28008      +89     
- Partials      960      962       +2

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

ffjlabo · 2024-05-29T06:13:37Z

The failing test is solved by this PR 🙏 #4926

khanhtc1202 · 2024-05-29T07:40:51Z

pkg/app/piped/executor/kubernetes/rollback.go

+		e.LogPersister.Errorf("failed to fetch preferred resources: %v", zap.Error(err))
+		return model.StageStatus_STAGE_FAILURE
+	}
+	e.LogPersister.Info(fmt.Sprintf("successfully preferred resources that contains for %d groups", len(groupResources)))


Suggested change

e.LogPersister.Info(fmt.Sprintf("successfully preferred resources that contains for %d groups", len(groupResources)))

e.LogPersister.Infof("successfully preferred resources that contains for %d groups", len(groupResources))

And remove "fmt" package

Thanks! fixed ff15986

khanhtc1202 · 2024-05-29T09:19:37Z

pkg/app/piped/platformprovider/kubernetes/loader.go

+// 1. The namespace set in the application configuration.
+// 2. The namespace set in the manifest.
+// 3. The default namespace.
+func (l *loader) refineNamespace(m *Manifest) error {


Should it be named determineResourceNamespace or determineNamespace? 🤔

Thanks! fixed as determineNamespace 5d22c75

Signed-off-by: Yoshiki Fujikane <ffjlabo@gmail.com>

ffjlabo · 2024-05-31T06:51:49Z

pkg/app/piped/platformprovider/kubernetes/loader.go

+// determineNamespace fix the namespace of the given manifest.
+// The priority is as follows:
+// If the resource is cluster-scoped, it returns an empty string.
+// Otherwise, it is the namespace-scoped resource and the namespace is determined by the following order:
+// 1. The namespace set in the application configuration.
+// 2. The namespace set in the manifest.
+// 3. The default namespace.
+func (l *loader) determineNamespace(m *Manifest) error {
+	namespaced, ok := l.isNamespacedResources[m.u.GroupVersionKind()]
+	if !ok {
+		return fmt.Errorf("unknown resource kind %s", m.u.GroupVersionKind().String())
 	}
-	for i := range manifests {
-		manifests[i].Key.Namespace = namespace
+
+	namespace := "" // empty if cluster-scoped resource
+
+	if namespaced {
+		namespace = "default"
+
+		if ns := m.u.GetNamespace(); ns != "" {
+			namespace = ns
+		}
+
+		if l.input.Namespace != "" {
+			namespace = l.input.Namespace
+		}
 	}
+
+	m.Key.Namespace = namespace
+	m.u.SetNamespace(namespace)
+
+	return nil
 }


📝 Check the current logic

ffjlabo added the do-not-merge/wip label May 15, 2024

github-actions bot added the area/go label May 15, 2024

ffjlabo force-pushed the change-resource-key-logic-for-k8s branch from 1b0b3f7 to d617a06 Compare May 16, 2024 08:34

ffjlabo added do-not-merge/hold and removed do-not-merge/wip labels May 16, 2024

ffjlabo marked this pull request as ready for review May 16, 2024 10:01

ffjlabo requested review from nghialv, khanhtc1202, kentakozuka and t-kikuc as code owners May 16, 2024 10:01

ffjlabo mentioned this pull request May 22, 2024

Remove checking the resource key with the annotation because their generation logic are different #4891

Closed

ffjlabo removed the do-not-merge/hold label May 28, 2024

ffjlabo added 5 commits May 29, 2024 13:51

Refactor not to use loadRunningManifests

1116001

Signed-off-by: Yoshiki Fujikane <ffjlabo@gmail.com>

Define func to get resource info from cluster

e4fe9c4

Signed-off-by: Yoshiki Fujikane <ffjlabo@gmail.com>

Refactor refineManifests

6c2c4eb

Signed-off-by: Yoshiki Fujikane <ffjlabo@gmail.com>

Fix for failing test and warning

9ac0a78

Signed-off-by: Yoshiki Fujikane <ffjlabo@gmail.com>

ffjlabo force-pushed the change-resource-key-logic-for-k8s branch from f07abcb to 9ac0a78 Compare May 29, 2024 04:52

khanhtc1202 reviewed May 29, 2024

View reviewed changes

khanhtc1202 added the area/piped label May 30, 2024

ffjlabo added 3 commits May 31, 2024 00:45

Change method name to determineNamespace

5d22c75

Signed-off-by: Yoshiki Fujikane <ffjlabo@gmail.com>

Refactor to use Infof

ff15986

Signed-off-by: Yoshiki Fujikane <ffjlabo@gmail.com>

Refactor to change the order of the import path

8adaa0b

Signed-off-by: Yoshiki Fujikane <ffjlabo@gmail.com>

ffjlabo requested a review from khanhtc1202 May 31, 2024 06:20

ffjlabo commented May 31, 2024

View reviewed changes

ffjlabo added the do-not-merge/hold label May 31, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Change resource key logic for k8s #4916

Change resource key logic for k8s #4916

ffjlabo commented May 15, 2024 •

edited

ffjlabo commented May 16, 2024

ffjlabo commented May 16, 2024

khanhtc1202 commented May 16, 2024

ffjlabo commented May 17, 2024

t-kikuc commented May 27, 2024

github-actions bot commented May 27, 2024

khanhtc1202 commented May 29, 2024

codecov bot commented May 29, 2024 •

edited

ffjlabo commented May 29, 2024

khanhtc1202 May 29, 2024

ffjlabo May 30, 2024

khanhtc1202 May 29, 2024

ffjlabo May 30, 2024 •

edited

ffjlabo May 31, 2024

	e.LogPersister.Info(fmt.Sprintf("successfully preferred resources that contains for %d groups", len(groupResources)))
	e.LogPersister.Infof("successfully preferred resources that contains for %d groups", len(groupResources))

Change resource key logic for k8s #4916

Are you sure you want to change the base?

Change resource key logic for k8s #4916

Conversation

ffjlabo commented May 15, 2024 • edited

ffjlabo commented May 16, 2024

The Overview of the fix

ffjlabo commented May 16, 2024

khanhtc1202 commented May 16, 2024

ffjlabo commented May 17, 2024

t-kikuc commented May 27, 2024

github-actions bot commented May 27, 2024

PR Analysis

Main theme

PR summary

Type of PR

PR Feedback:

General suggestions

Code feedback

Security concerns:

khanhtc1202 commented May 29, 2024

codecov bot commented May 29, 2024 • edited

Codecov Report

ffjlabo commented May 29, 2024

khanhtc1202 May 29, 2024

Choose a reason for hiding this comment

ffjlabo May 30, 2024

Choose a reason for hiding this comment

khanhtc1202 May 29, 2024

Choose a reason for hiding this comment

ffjlabo May 30, 2024 • edited

Choose a reason for hiding this comment

ffjlabo May 31, 2024

Choose a reason for hiding this comment

ffjlabo commented May 15, 2024 •

edited

codecov bot commented May 29, 2024 •

edited

ffjlabo May 30, 2024 •

edited