-
Notifications
You must be signed in to change notification settings - Fork 359
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.
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
bug: fixing running antrea-controller --version "outside of K8s" print warnings
#5993
Conversation
Signed-off-by: Griffin <prakritimandal611@gmail.com>
running antrea-controller --version "outside of K8s" print warnings
@@ -54,6 +49,22 @@ var ipsecTunnelUsages = sets.New[string]( | |||
|
|||
var _ approver = (*ipsecCSRApprover)(nil) | |||
|
|||
func getAntreaAgentServiceAccount() string { |
There was a problem hiding this comment.
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",
}, ":")
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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
@@ -123,7 +134,7 @@ func (ic *ipsecCSRApprover) verifyCertificateRequest(req *x509.CertificateReques | |||
} | |||
|
|||
func (ic *ipsecCSRApprover) verifyIdentity(nodeName string, csr *certificatesv1.CertificateSigningRequest) error { | |||
if csr.Spec.Username != antreaAgentServiceAccountName { | |||
if csr.Spec.Username != getAntreaAgentServiceAccount() { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Even though I don't think that verifyIdentity
is called very often, we may want to just add antreaAgentServiceAccountName
as a field to the ipsecCSRApprover
struct, and initialize the field by calling getAntreaAgentServiceAccount()
when we instantiate ipsecCSRApprover
objects. It may be a good idea to have a newIPsecCSRApprover
helper function to instantiate.
Signed-off-by: Griffin <prakritimandal611@gmail.com>
@@ -123,7 +126,7 @@ func (ic *ipsecCSRApprover) verifyCertificateRequest(req *x509.CertificateReques | |||
} | |||
|
|||
func (ic *ipsecCSRApprover) verifyIdentity(nodeName string, csr *certificatesv1.CertificateSigningRequest) error { | |||
if csr.Spec.Username != antreaAgentServiceAccountName { | |||
if csr.Spec.Username != ic.getAntreaAgentServiceAccount() { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this should be csr.Spec.Username != ic. antreaAgentServiceAccountName
@@ -54,6 +49,14 @@ var ipsecTunnelUsages = sets.New[string]( | |||
|
|||
var _ approver = (*ipsecCSRApprover)(nil) | |||
|
|||
func (ic *ipsecCSRApprover) getAntreaAgentServiceAccount() string { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this should not be a member function
and ic.antreaAgentServiceAccountName
should be populated (by calling getAntreaAgentServiceAccount()
) as part of a new "constructor", newIPsecCSRApprover
, as I pointed out in my earlier review
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done
Signed-off-by: Griffin <prakritimandal611@gmail.com>
func newIPsecCSRApprover() *ipsecCSRApprover { | ||
return &ipsecCSRApprover{ | ||
antreaAgentServiceAccountName: getAntreaAgentServiceAccount(), | ||
} | ||
} |
There was a problem hiding this comment.
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(),
}
}
@@ -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() |
There was a problem hiding this comment.
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{
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
thank you
@@ -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() |
There was a problem hiding this comment.
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
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
will revert and fix : )
Signed-off-by: Griffin <prakritimandal611@gmail.com>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
one small nit, otherwise LGTM
agentServiceAccountName := strings.Join([]string{ | ||
"system", "serviceaccount", env.GetAntreaNamespace(), "antrea-agent", | ||
}, ":") | ||
|
||
return agentServiceAccountName |
There was a problem hiding this comment.
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(...)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
oh I get it
/test-all |
fix: #5990
getAntreaAgentServiceAccount()
which fixes the warnings outside k8skubectl exec -it antrea-controller-6b554d9c74-l9ms2 -n kube-system -- /bin/bash root@kind-worker:/# antrea-controller --version
antrea-controller version v2.0.0-dev-43ee1f47 linux/amd64 go1.21.7
docker run antrea/antrea-controller-ubuntu:v2.0.0-dev-43ee1f47 antrea-controll er --version antrea-controller version v2.0.0-dev-43ee1f47 linux/amd64 go1.21.7