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

Workload identity support namespaced service account #39

Merged
merged 3 commits into from
Jun 4, 2024
Merged

Workload identity support namespaced service account #39

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
108c727
Workload identity support namespaced service account
RichardChen820 Apr 30, 2024
029de80
feedback
RichardChen820 May 16, 2024
29a03e9
Feedback
RichardChen820 May 21, 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
2 changes: 1 addition & 1 deletion deploy/parameter/helm-values.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,7 +17,7 @@ fullnameOverride: "az-appconfig-k8s-provider"

workloadIdentity:
enabled: true
customServiceAccountOnly: false
disableGlobalServiceAccount: false

serviceAccount:
# Specifies whether a service account should be created
Expand Down
14 changes: 14 additions & 0 deletions deploy/templates/clusterrole.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -53,6 +53,20 @@ rules:
- patch
- update
- watch
{{- if eq .Values.workloadIdentity.enabled true }}
- apiGroups:
- ""
resources:
- serviceaccounts
verbs:
- get
- apiGroups:
- ""
resources:
- serviceaccounts/token
juniwang marked this conversation as resolved.
Show resolved Hide resolved
verbs:
- create
{{- end }}
---
apiVersion: rbac.authorization.k8s.io/v1
kind: ClusterRoleBinding
Expand Down
6 changes: 3 additions & 3 deletions deploy/templates/deployment.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -52,9 +52,9 @@ spec:
value: {{ .Values.env.azureTenantId }}
{{- end }}
- name: WORKLOAD_IDENTITY_ENABLED
value: {{ .Values.workloadIdentity.enabled }}
- name: WORKLOAD_IDENTITY_CUSTOM_SERVICE_ACCOUNT_ONLY
value: {{ .Values.workloadIdentity.allowCustomServiceAccount }}
value: "{{ .Values.workloadIdentity.enabled }}"
- name: WORKLOAD_IDENTITY_DISABLE_GLOBAL_SERVICE_ACCOUNT
value: "{{ .Values.workloadIdentity.disableGlobalServiceAccount }}"
livenessProbe:
httpGet:
path: /healthz
Expand Down
14 changes: 7 additions & 7 deletions internal/controller/utils.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,11 +17,11 @@ import (
)

const (
MinimalSentinelBasedRefreshInterval time.Duration = time.Second
MinimalSecretRefreshInterval time.Duration = time.Minute
MinimalFeatureFlagRefreshInterval time.Duration = time.Second
WorkloadIdentityEnabled string = "WORKLOAD_IDENTITY_ENABLED"
WorkloadIdentityCustomServiceAccountOnly string = "WORKLOAD_IDENTITY_CUSTOM_SERVICE_ACCOUNT_ONLY"
MinimalSentinelBasedRefreshInterval time.Duration = time.Second
MinimalSecretRefreshInterval time.Duration = time.Minute
MinimalFeatureFlagRefreshInterval time.Duration = time.Second
WorkloadIdentityEnabled string = "WORKLOAD_IDENTITY_ENABLED"
WorkloadIdentityDisableGlobalServiceAccount string = "WORKLOAD_IDENTITY_DISABLE_GLOBAL_SERVICE_ACCOUNT"
)

func verifyObject(spec acpv1.AzureAppConfigurationProviderSpec) error {
Expand Down Expand Up @@ -247,14 +247,14 @@ func verifyWorkloadIdentityParameters(workloadIdentity *acpv1.WorkloadIdentityPa
var authCount int = 0

if workloadIdentity.ManagedIdentityClientId != nil {
if strings.EqualFold(os.Getenv(WorkloadIdentityCustomServiceAccountOnly), "true") {
if strings.EqualFold(os.Getenv(WorkloadIdentityDisableGlobalServiceAccount), "true") {
juniwang marked this conversation as resolved.
Show resolved Hide resolved
return loader.NewArgumentError("auth.workloadIdentity.managedIdentityClientId", fmt.Errorf("managedIdentityClientId is not allowed since only custom service account is allowed"))
Copy link

Choose a reason for hiding this comment

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

should we avoid "custom service account"? some like "since global service account is disabled" to keep consistent with the installation flag.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Updated

}
authCount++
}

if workloadIdentity.ManagedIdentityClientIdReference != nil {
if strings.EqualFold(os.Getenv(WorkloadIdentityCustomServiceAccountOnly), "true") {
if strings.EqualFold(os.Getenv(WorkloadIdentityDisableGlobalServiceAccount), "true") {
return loader.NewArgumentError("auth.workloadIdentity.managedIdentityClientIdReference", fmt.Errorf("managedIdentityClientIdReference is not allowed since only custom service account is allowed"))
}
authCount++
Expand Down
22 changes: 17 additions & 5 deletions internal/loader/configuration_client_manager.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,7 @@ import (
"math/rand"
"net"
"net/url"
"os"
"strconv"
"strings"
"time"
Expand Down Expand Up @@ -198,10 +199,10 @@ func (manager *ConfigurationClientManager) DiscoverFallbackClients(ctx context.C

select {
case <-newCtx.Done():
klog.Warningf("fail to build fall back clients, SRV DNS lookup is timeout")
klog.Warningf("fail to build fallback clients, SRV DNS lookup is timeout")
break
case err := <-errChan:
klog.Warningf("fail to build fall back clients %s", err.Error())
klog.Warningf("fail to build fallback clients %s", err.Error())
break
case srvTargetHosts := <-resultChan:
// Shuffle the list of SRV target hosts
Expand Down Expand Up @@ -492,11 +493,22 @@ func newClientAssertionCredential(ctx context.Context, serviceAccountName string
return nil, err
}

clientId := serviceAccountObj.ObjectMeta.Annotations[AnnotationClientID]
tenantId := serviceAccountObj.Annotations[AnnotationTenantID]
tenantId, ok := os.LookupEnv(strings.ToUpper(AzureTenantId))
Copy link
Contributor

Choose a reason for hiding this comment

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

Why the tenantId is required?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good question, the tenantId is there if enable the cluster workload identity, but need confirm it.

Copy link
Contributor Author

@RichardChen820 RichardChen820 May 17, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

Confirmed, once the workload identity is enabled, the tenantId should be there, and the existing way we support for workload identity also relies on this tenantId

Copy link
Contributor

Choose a reason for hiding this comment

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

If namespaced service account is used, is the value of tenantId same as AnnotationTenantID from service account? If customers need to explicitly set this value, will users from other namespace can get the tenantId through kubectl describe po k8sproviderpodxxxx？

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I don't quite get your question. tenantId if for global, each service account could overwrite is by setting AnnotationTenantID

if !ok {
return nil, fmt.Errorf("no tenant ID specified. Check pod configuration or set TenantID in the options")
}

if _, ok := serviceAccountObj.Annotations[AnnotationClientID]; !ok {
return nil, fmt.Errorf("service account %s/%s does not have annotation %s", serviceAccountNamespace, serviceAccountName, AnnotationClientID)
}

if _, ok := serviceAccountObj.Annotations[AnnotationTenantID]; ok {
tenantId = serviceAccountObj.Annotations[AnnotationTenantID]
Copy link
Contributor

Choose a reason for hiding this comment

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

What's the difference bewteen the AzureTenantId from pod config and AnnotationTenantID from service account?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

You can imagine that the AzureTenantId from pod is global, the AnnotationTenantID is for each specific service account. If users uses the same tenantID for all service account, it just need to use the global one, no need to specify in every service account

Copy link
Contributor

Choose a reason for hiding this comment

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

I see, thanks

Copy link

Choose a reason for hiding this comment

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

to be honest, I cannot think of a case when a different tenant id is used. Technically it has to be the same tenantId.

Copy link

Choose a reason for hiding this comment

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

I do a bit change to this, hope this make sense to you

}

getAssertionFunc := newGetAssertionFunc(serviceAccountNamespace, serviceAccountName)

clientAssertionCredential, err := azidentity.NewClientAssertionCredential(tenantId, clientId, getAssertionFunc, nil)
clientAssertionCredential, err := azidentity.NewClientAssertionCredential(tenantId, serviceAccountObj.Annotations[AnnotationClientID], getAssertionFunc, nil)
if err != nil {
return nil, err
}
Expand Down