Whitelisting #310
Whitelisting #310
Conversation
aramase
force-pushed
the
whitelisting
branch
2 times, most recently
from
c975b9f
to
af98dba
Compare
Ran 15 of 15 Specs in 2374.242 seconds
SUCCESS! -- 15 Passed | 0 Failed | 0 Pending | 0 Skipped
--- PASS: TestAADPodIdentity (2374.24s)
PASS
ok github.com/Azure/aad-pod-identity/test/e2e 2374.271s |
| @@ -372,7 +372,9 @@ func (c *TestCrdClient) CreateAssignedIdentity(assignedIdentity *aadpodid.AzureA | |||
| func (c *TestCrdClient) UpdateAzureAssignedIdentityStatus(assignedIdentity *aadpodid.AzureAssignedIdentity, status string) error { | |||
| assignedIdentity.Status.Status = status | |||
| assignedIdentityToStore := *assignedIdentity //Make a copy to store in the map. | |||
| c.mu.Lock() | |||
| c.assignedIDMap[assignedIdentity.Name] = &assignedIdentityToStore | |||
There was a problem hiding this comment.
We should also wrap the Stats update (if they are happening in parallel) with its own lock to prevent any parallel operation causing map issues.
|
|
||
| enableUserAssignedIdentityOnCluster(nodeList, keyvaultIdentity) | ||
|
|
||
| err = azurepodidentityexception.Create(identityValidator, templateOutputPath, map[string]string{"app": "identity-validator"}) |
There was a problem hiding this comment.
Would it be possible to do two of these and two deployments, to ensure that it can handle more than one ? And also one more which is really not bound by exception to see that its working even when exception is available ?
| @@ -67,6 +67,8 @@ func main() { | |||
|
|
|||
| // testClusterWideUserAssignedIdentity will verify whether cluster-wide user assigned identity is working properly | |||
| func testClusterWideUserAssignedIdentity(logger *log.Entry, msiEndpoint, subscriptionID, resourceGroup, identityClientID string) error { | |||
| os.Setenv("AZURE_CLIENT_ID", identityClientID) | |||
There was a problem hiding this comment.
Can you please add the comments on what we found today on how its is getting used internally in the adal code.
| //AzurePodIdentityException contains the pod selectors for all pods that don't require | ||
| // NMI to process and request token on their behalf. | ||
|
|
||
| //+k8s:deepcopy-gen:interfaces=k8s.io/apimachinery/pkg/runtime.Object |
There was a problem hiding this comment.
Can you please add a README under the readme directory on how ExceptionList can be used.
pkg/nmi/server/server.go
Outdated
| @@ -266,15 +267,15 @@ func (s *Server) isMIC(podNS, rsName string) bool { | |||
| return false | |||
| } | |||
|
|
|||
| func (s *Server) getMICToken(logger *log.Entry, rqClientID, rqResource string) ([]byte, int, error) { | |||
| func (s *Server) gotTokenForExceptedPod(logger *log.Entry, rqClientID, rqResource string) ([]byte, int, error) { | |||
pkg/nmi/server/server.go
Outdated
| @@ -266,15 +267,15 @@ func (s *Server) isMIC(podNS, rsName string) bool { | |||
| return false | |||
| } | |||
|
|
|||
| func (s *Server) getMICToken(logger *log.Entry, rqClientID, rqResource string) ([]byte, int, error) { | |||
| func (s *Server) gotTokenForExceptedPod(logger *log.Entry, rqClientID, rqResource string) ([]byte, int, error) { | |||
There was a problem hiding this comment.
I think we can just say getTokenForPod without really stating excepted since it covers both cases.
There was a problem hiding this comment.
It's actually only excepted pods. Which other case does it cover?
|
|
||
| // labelInExceptionList checks if the labels defined in azurepodidentityexception match label defined in pods | ||
| func labelInExceptionList(podLabels map[string]string, exceptionList []aadpodid.AzurePodIdentityException) bool { | ||
| for _, podIdentityException := range exceptionList { |
There was a problem hiding this comment.
podIdentityException ==> exception ?
| if err != nil { | ||
| return nil, err | ||
| } | ||
| stats.Put(stats.PodList, time.Since(begin)) | ||
| return listPods, nil | ||
| } | ||
|
|
||
| // IsPodExcepted returns true if pod label is part of exception crd | ||
| func IsPodExcepted(podLabels map[string]string, exceptionList *[]aadpodid.AzurePodIdentityException) bool { |
There was a problem hiding this comment.
We have to make a full clean up to avoid passing slice pointers, instead use slices directly. Meanwhile for the new code we are adding can we avoid it ? Also if we pass the slice, we don't need to check for nil case and do direct len simplifying the code below.
| if err != nil { | ||
| return nil, err | ||
| } | ||
| stats.Put(stats.PodList, time.Since(begin)) | ||
| return listPods, nil | ||
| } | ||
|
|
||
| // IsPodExcepted returns true if pod label is part of exception crd | ||
| func IsPodExcepted(podLabels map[string]string, exceptionList *[]aadpodid.AzurePodIdentityException) bool { |
There was a problem hiding this comment.
can you please add a UT for this function.
| apiVersion: apiextensions.k8s.io/v1beta1 | ||
| kind: CustomResourceDefinition | ||
| metadata: | ||
| name: azurepodidentityexceptions.aadpodidentity.k8s.io |
There was a problem hiding this comment.
We should add it in the deploy file under master directory as welll. And then move that to the deployment file in the top level folder.
Reason for Change:
There are instances where pods deployed in a cluster need to request token directly instead of using aad-pod-identity. The following changes are done to support that use case -
AzurePodIdentityException.spec.PodLabelsfield accepts a list of labels. If a pod needs to bypass aad-pod-identity while requesting a token, the label defined in the exception CRD should also be added to the pod. If a match is found, thenNMIwill proxy the request for token without and send the response back without any validation.AzurePodIdentityExceptionCRD and label matching is done per namespace, which means if the same labels need to be used in multiple namespaces for bypassing pod-identity, a CRD needs to be defined in each namespace required.Issue Fixed:
Feature #284
Notes for Reviewers: