Skip to content

kmesh security: pod manage#489

Merged
kmesh-bot merged 3 commits intokmesh-net:mainfrom
lec-bit:kmesh-security
Jul 17, 2024
Merged

kmesh security: pod manage#489
kmesh-bot merged 3 commits intokmesh-net:mainfrom
lec-bit:kmesh-security

Conversation

@lec-bit
Copy link
Copy Markdown
Contributor

@lec-bit lec-bit commented Jul 5, 2024

What type of PR is this?
/kind enhancement

What this PR does / why we need it:
kmesh secutity only manage pod managed by kmesh
Which issue(s) this PR fixes:
Fixes #

Special notes for your reviewer:

Does this PR introduce a user-facing change?:


@kmesh-bot kmesh-bot added the kind/enhancement New feature or request label Jul 5, 2024
return false
}

pod, err := client.CoreV1().Pods(podNamespace).Get(context.TODO(), podName, metav1.GetOptions{})
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

donot call apiserver each time, we could get from the informer cache

return false
}

if pod.Annotations["kmesh.net/redirection"] == "enabled" && pod.Annotations["kmesh.net/bypass"] == "" {
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

use predefined constants


pod, err := client.CoreV1().Pods(podNamespace).Get(context.TODO(), podName, metav1.GetOptions{})
if err != nil {
log.Errorf("failed to get pod: %v", err)
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
log.Errorf("failed to get pod: %v", err)
log.Errorf("get pod failed: %v", err)

hzxuzhonghu
hzxuzhonghu previously approved these changes Jul 15, 2024
var secertManager *security.SecretManager
var err error
if c.enableSecretManager {
secertManager, err = security.NewSecretManager()
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Does ad mode need to start secret manager too?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

no, ads mode is not support now

func sendCertRequest(security *kmeshsecurity.SecretManager, pod *corev1.Pod, op int) {
if security != nil {
Identity := spiffe.Identity{
TrustDomain: "cluster.local",
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

add a const

@codecov
Copy link
Copy Markdown

codecov bot commented Jul 15, 2024

Codecov Report

Attention: Patch coverage is 50.00000% with 17 lines in your changes missing coverage. Please review.

Project coverage is 49.64%. Comparing base (18b35d4) to head (eb1c54e).
Report is 38 commits behind head on main.

Files Coverage Δ
pkg/controller/security/manager.go 92.37% <ø> (ø)
pkg/controller/workload/workload_processor.go 44.08% <ø> (+1.40%) ⬆️
pkg/controller/controller.go 0.00% <0.00%> (ø)
pkg/controller/manage/kmesh_manage.go 60.66% <65.38%> (+7.63%) ⬆️

... and 2 files with indirect coverage changes


Continue to review full report in Codecov by Sentry.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 8dce347...eb1c54e. Read the comment docs.

@lec-bit lec-bit force-pushed the kmesh-security branch 2 times, most recently from 316d1eb to 0fbdb4c Compare July 16, 2024 08:54
@kmesh-bot kmesh-bot added size/L and removed size/M labels Jul 16, 2024
@lec-bit lec-bit force-pushed the kmesh-security branch 2 times, most recently from 6845ed2 to 803f7f4 Compare July 16, 2024 14:21
Signed-off-by: let-bit <glfhmzmy@126.com>
Signed-off-by: let-bit <glfhmzmy@126.com>
Copy link
Copy Markdown
Member

@hzxuzhonghu hzxuzhonghu left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

"time"

"istio.io/istio/pkg/spiffe"

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

rm blank line

},
DeleteFunc: func(obj interface{}) {
pod, ok := obj.(*corev1.Pod)
if !ok {
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

To be more accurate add the below cast

	// When a delete is dropped, the relist will notice a job in the store not
	// in the list, leading to the insertion of a tombstone object which contains
	// the deleted key/value. Note that this value might be stale.
	if !ok {
		tombstone, ok := obj.(cache.DeletedFinalStateUnknown)
		if !ok {
			utilruntime.HandleError(fmt.Errorf("couldn't get object from tombstone %#v", obj))
			return
		}
		job, ok = tombstone.Obj.(*corev1.Pod)
		if !ok {
			utilruntime.HandleError(fmt.Errorf("tombstone contained object that is not a Job %#v", obj))
			return
		}
	}

Signed-off-by: let-bit <glfhmzmy@126.com>
@kmesh-bot
Copy link
Copy Markdown
Collaborator

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: hzxuzhonghu

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Details Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@hzxuzhonghu
Copy link
Copy Markdown
Member

/lgtm

@kmesh-bot kmesh-bot added the lgtm label Jul 17, 2024
@kmesh-bot kmesh-bot merged commit ab8778e into kmesh-net:main Jul 17, 2024
@hzxuzhonghu hzxuzhonghu deleted the kmesh-security branch July 17, 2024 08:07
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants