Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

bug: fixing running antrea-controller --version "outside of K8s" print warnings #5993

Merged
merged 5 commits into from
Feb 22, 2024
+21 −23
Merged

bug: fixing running antrea-controller --version "outside of K8s" print warnings #5993

Changes from 3 commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
db9e800
bug: fixing unwanted warnings outside k8s environment
prakrit55 Feb 15, 2024
e4f48e7
configured the function
prakrit55 Feb 20, 2024
6f76989
fix: added constructor newIPsecCSRApprover constructor
prakrit55 Feb 21, 2024
1bcd7f8
fix: instantiated constructor
prakrit55 Feb 22, 2024
ef7d6bf
clean up
prakrit55 Feb 22, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
26 changes: 18 additions & 8 deletions pkg/controller/certificatesigningrequest/ipsec_csr_approver.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -38,14 +38,9 @@ const (
ipsecCSRApproverName = "AntreaIPsecCSRApprover"
)

var (
antreaAgentServiceAccountName = strings.Join([]string{
"system", "serviceaccount", env.GetAntreaNamespace(), "antrea-agent",
}, ":")
)

type ipsecCSRApprover struct {
client clientset.Interface
client clientset.Interface
antreaAgentServiceAccountName string
}

var ipsecTunnelUsages = sets.New[string](
Expand All @@ -54,6 +49,20 @@ var ipsecTunnelUsages = sets.New[string](

var _ approver = (*ipsecCSRApprover)(nil)

func getAntreaAgentServiceAccount() string {
Copy link
Contributor

Choose a reason for hiding this comment

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

the function implementation is not correct. If the absence of a POD_NAMESPACE env variable, using kube-system as the default is what we want

the function should just preserve the existing behavior return:

	strings.Join([]string{
		"system", "serviceaccount", env.GetAntreaNamespace(), "antrea-agent",
	}, ":")

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hey @antoninbas, need a reconfirmation, here the logic was to see if the namespace is present or not, obviously its absence as a container but as in a pod it would definitely have one. So, in pod it will peacefully execute the env.GetAntreaNamespace() but in container it wont have it and give the warnings. WDYS

Copy link
Contributor

Choose a reason for hiding this comment

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

the main case scenario in which the namespace is not defined is for unit tests, in this case we prefer to default to kube-system
it may also apply when running the antrea agent in a "simulator" context, outside of a K8s Node. In this case, we also prefer to default to kube-system IMO

agentServiceAccountName := strings.Join([]string{
"system", "serviceaccount", env.GetAntreaNamespace(), "antrea-agent",
}, ":")

return agentServiceAccountName
Copy link
Contributor

@antoninbas antoninbas Feb 22, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

nit: it would be more concise not to define an extra variable. You can just return strings.Join(...)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

oh I get it

}

func newIPsecCSRApprover() *ipsecCSRApprover {
return &ipsecCSRApprover{
antreaAgentServiceAccountName: getAntreaAgentServiceAccount(),
}
}
Copy link
Contributor

Choose a reason for hiding this comment

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

this should be:

func newIPsecCSRApprover(client clientset.Interface) *ipsecCSRApprover {
	return &ipsecCSRApprover{
		client: client,
		antreaAgentServiceAccountName: getAntreaAgentServiceAccount(),
	}
}


func (ic *ipsecCSRApprover) recognize(csr *certificatesv1.CertificateSigningRequest) bool {
return csr.Spec.SignerName == antreaapis.AntreaIPsecCSRSignerName
}
Expand Down Expand Up @@ -123,7 +132,8 @@ func (ic *ipsecCSRApprover) verifyCertificateRequest(req *x509.CertificateReques
}

func (ic *ipsecCSRApprover) verifyIdentity(nodeName string, csr *certificatesv1.CertificateSigningRequest) error {
if csr.Spec.Username != antreaAgentServiceAccountName {
c := newIPsecCSRApprover()
Copy link
Contributor

Choose a reason for hiding this comment

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

the constructor should not be called here
you need to find all places where ipsecCSRApprover is instantiated directly using &ipsecCSRApprover{...}, and replace them with calls to newIPsecCSRApprover(...).

abasSMD6R:antrea abas$ git grep "&ipsecCSRApprover"
pkg/controller/certificatesigningrequest/approving_controller.go:                       &ipsecCSRApprover{
pkg/controller/certificatesigningrequest/ipsec_csr_approver_test.go:                    ic := &ipsecCSRApprover{
pkg/controller/certificatesigningrequest/ipsec_csr_approver_test.go:                    ic := &ipsecCSRApprover{
pkg/controller/certificatesigningrequest/ipsec_csr_approver_test.go:                    ic := &ipsecCSRApprover{
pkg/controller/certificatesigningrequest/ipsec_csr_approver_test.go:                    ic := &ipsecCSRApprover{

Copy link
Contributor Author

Choose a reason for hiding this comment

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

thank you

Copy link
Contributor

Choose a reason for hiding this comment

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

you are creating a new instance of `ipsecCSRApprover, this is not right

Copy link
Contributor Author

Choose a reason for hiding this comment

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

will revert and fix : )

if csr.Spec.Username != c.antreaAgentServiceAccountName {
return errUserUnauthorized
}
podNameValues, podUIDValues := csr.Spec.Extra[sautil.PodNameKey], csr.Spec.Extra[sautil.PodUIDKey]
Expand Down