From 7c8c9b9a231a6dc32bf8832307fac19234813965 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Charles-Edouard=20Br=C3=A9t=C3=A9ch=C3=A9?= Date: Tue, 28 Sep 2021 22:39:56 +0200 Subject: [PATCH 1/2] feat: add support for custom audience in aws oidc provider fix: missing json tags fix: code gen fix: switch to additional audiences fix: oidc task fix: add integration test --- k8s/crds/kops.k8s.io_clusters.yaml | 6 ++++ pkg/apis/kops/cluster.go | 2 ++ pkg/apis/kops/v1alpha2/cluster.go | 2 ++ .../kops/v1alpha2/zz_generated.conversion.go | 2 ++ .../kops/v1alpha2/zz_generated.deepcopy.go | 7 +++- pkg/apis/kops/zz_generated.deepcopy.go | 7 +++- pkg/model/awsmodel/oidc_provider.go | 7 +++- ...cket_object_cluster-completed.spec_content | 2 ++ .../update_cluster/irsa/in-v1alpha2.yaml | 2 ++ .../update_cluster/irsa/kubernetes.tf | 2 +- upup/pkg/fi/cloudup/awstasks/BUILD.bazel | 1 + .../fi/cloudup/awstasks/iamoidcprovider.go | 36 +++++++++++++++++-- 12 files changed, 69 insertions(+), 7 deletions(-) diff --git a/k8s/crds/kops.k8s.io_clusters.yaml b/k8s/crds/kops.k8s.io_clusters.yaml index fc56504ba7..2e5a97f797 100644 --- a/k8s/crds/kops.k8s.io_clusters.yaml +++ b/k8s/crds/kops.k8s.io_clusters.yaml @@ -4332,6 +4332,12 @@ spec: description: ServiceAccountIssuerDiscovery configures the OIDC Issuer for ServiceAccounts. properties: + additionalAudiences: + description: AdditionalAudiences adds user defined audiences to + the provisionned AWS OIDC provider + items: + type: string + type: array discoveryStore: description: DiscoveryStore is the VFS path to where OIDC Issuer Discovery metadata is stored. diff --git a/pkg/apis/kops/cluster.go b/pkg/apis/kops/cluster.go index 88af1320c1..2b3ca154cf 100644 --- a/pkg/apis/kops/cluster.go +++ b/pkg/apis/kops/cluster.go @@ -223,6 +223,8 @@ type ServiceAccountIssuerDiscoveryConfig struct { DiscoveryStore string `json:"discoveryStore,omitempty"` // EnableAWSOIDCProvider will provision an AWS OIDC provider that trusts the ServiceAccount Issuer EnableAWSOIDCProvider bool `json:"enableAWSOIDCProvider,omitempty"` + // AdditionalAudiences adds user defined audiences to the provisionned AWS OIDC provider + AdditionalAudiences []string `json:"additionalAudiences,omitempty"` } // ServiceAccountExternalPermissions grants a ServiceAccount permissions to external resources. diff --git a/pkg/apis/kops/v1alpha2/cluster.go b/pkg/apis/kops/v1alpha2/cluster.go index ba6b5c9446..cee4dbbd90 100644 --- a/pkg/apis/kops/v1alpha2/cluster.go +++ b/pkg/apis/kops/v1alpha2/cluster.go @@ -221,6 +221,8 @@ type ServiceAccountIssuerDiscoveryConfig struct { DiscoveryStore string `json:"discoveryStore,omitempty"` // EnableAWSOIDCProvider will provision an AWS OIDC provider that trusts the ServiceAccount Issuer EnableAWSOIDCProvider bool `json:"enableAWSOIDCProvider,omitempty"` + // AdditionalAudiences adds user defined audiences to the provisionned AWS OIDC provider + AdditionalAudiences []string `json:"additionalAudiences,omitempty"` } // ServiceAccountExternalPermissions grants a ServiceAccount permissions to external resources. diff --git a/pkg/apis/kops/v1alpha2/zz_generated.conversion.go b/pkg/apis/kops/v1alpha2/zz_generated.conversion.go index d4bf0b661d..65c4d6bc21 100644 --- a/pkg/apis/kops/v1alpha2/zz_generated.conversion.go +++ b/pkg/apis/kops/v1alpha2/zz_generated.conversion.go @@ -6686,6 +6686,7 @@ func Convert_kops_ServiceAccountExternalPermission_To_v1alpha2_ServiceAccountExt func autoConvert_v1alpha2_ServiceAccountIssuerDiscoveryConfig_To_kops_ServiceAccountIssuerDiscoveryConfig(in *ServiceAccountIssuerDiscoveryConfig, out *kops.ServiceAccountIssuerDiscoveryConfig, s conversion.Scope) error { out.DiscoveryStore = in.DiscoveryStore out.EnableAWSOIDCProvider = in.EnableAWSOIDCProvider + out.AdditionalAudiences = in.AdditionalAudiences return nil } @@ -6697,6 +6698,7 @@ func Convert_v1alpha2_ServiceAccountIssuerDiscoveryConfig_To_kops_ServiceAccount func autoConvert_kops_ServiceAccountIssuerDiscoveryConfig_To_v1alpha2_ServiceAccountIssuerDiscoveryConfig(in *kops.ServiceAccountIssuerDiscoveryConfig, out *ServiceAccountIssuerDiscoveryConfig, s conversion.Scope) error { out.DiscoveryStore = in.DiscoveryStore out.EnableAWSOIDCProvider = in.EnableAWSOIDCProvider + out.AdditionalAudiences = in.AdditionalAudiences return nil } diff --git a/pkg/apis/kops/v1alpha2/zz_generated.deepcopy.go b/pkg/apis/kops/v1alpha2/zz_generated.deepcopy.go index 8f8aff2786..f45e6bae30 100644 --- a/pkg/apis/kops/v1alpha2/zz_generated.deepcopy.go +++ b/pkg/apis/kops/v1alpha2/zz_generated.deepcopy.go @@ -1184,7 +1184,7 @@ func (in *ClusterSpec) DeepCopyInto(out *ClusterSpec) { if in.ServiceAccountIssuerDiscovery != nil { in, out := &in.ServiceAccountIssuerDiscovery, &out.ServiceAccountIssuerDiscovery *out = new(ServiceAccountIssuerDiscoveryConfig) - **out = **in + (*in).DeepCopyInto(*out) } if in.SnapshotController != nil { in, out := &in.SnapshotController, &out.SnapshotController @@ -4641,6 +4641,11 @@ func (in *ServiceAccountExternalPermission) DeepCopy() *ServiceAccountExternalPe // DeepCopyInto is an autogenerated deepcopy function, copying the receiver, writing into out. in must be non-nil. func (in *ServiceAccountIssuerDiscoveryConfig) DeepCopyInto(out *ServiceAccountIssuerDiscoveryConfig) { *out = *in + if in.AdditionalAudiences != nil { + in, out := &in.AdditionalAudiences, &out.AdditionalAudiences + *out = make([]string, len(*in)) + copy(*out, *in) + } return } diff --git a/pkg/apis/kops/zz_generated.deepcopy.go b/pkg/apis/kops/zz_generated.deepcopy.go index 8c5435a792..2e2183b08c 100644 --- a/pkg/apis/kops/zz_generated.deepcopy.go +++ b/pkg/apis/kops/zz_generated.deepcopy.go @@ -1268,7 +1268,7 @@ func (in *ClusterSpec) DeepCopyInto(out *ClusterSpec) { if in.ServiceAccountIssuerDiscovery != nil { in, out := &in.ServiceAccountIssuerDiscovery, &out.ServiceAccountIssuerDiscovery *out = new(ServiceAccountIssuerDiscoveryConfig) - **out = **in + (*in).DeepCopyInto(*out) } if in.SnapshotController != nil { in, out := &in.SnapshotController, &out.SnapshotController @@ -4823,6 +4823,11 @@ func (in *ServiceAccountExternalPermission) DeepCopy() *ServiceAccountExternalPe // DeepCopyInto is an autogenerated deepcopy function, copying the receiver, writing into out. in must be non-nil. func (in *ServiceAccountIssuerDiscoveryConfig) DeepCopyInto(out *ServiceAccountIssuerDiscoveryConfig) { *out = *in + if in.AdditionalAudiences != nil { + in, out := &in.AdditionalAudiences, &out.AdditionalAudiences + *out = make([]string, len(*in)) + copy(*out, *in) + } return } diff --git a/pkg/model/awsmodel/oidc_provider.go b/pkg/model/awsmodel/oidc_provider.go index bdc2833e11..2f772dbe6f 100644 --- a/pkg/model/awsmodel/oidc_provider.go +++ b/pkg/model/awsmodel/oidc_provider.go @@ -49,11 +49,16 @@ func (b *OIDCProviderBuilder) Build(c *fi.ModelBuilderContext) error { thumbprints = append(thumbprints, fi.String(fingerprint)) } + audiences := []string{defaultAudience} + if b.Cluster.Spec.ServiceAccountIssuerDiscovery.AdditionalAudiences != nil { + audiences = append(audiences, b.Cluster.Spec.ServiceAccountIssuerDiscovery.AdditionalAudiences...) + } + c.AddTask(&awstasks.IAMOIDCProvider{ Name: fi.String(b.ClusterName()), Lifecycle: b.Lifecycle, URL: b.Cluster.Spec.KubeAPIServer.ServiceAccountIssuer, - ClientIDs: []*string{fi.String(defaultAudience)}, + ClientIDs: fi.StringSlice(audiences), Tags: b.CloudTags(b.ClusterName(), false), Thumbprints: thumbprints, }) diff --git a/tests/integration/update_cluster/irsa/data/aws_s3_bucket_object_cluster-completed.spec_content b/tests/integration/update_cluster/irsa/data/aws_s3_bucket_object_cluster-completed.spec_content index 8b29b2b58f..7c2c9ab30a 100644 --- a/tests/integration/update_cluster/irsa/data/aws_s3_bucket_object_cluster-completed.spec_content +++ b/tests/integration/update_cluster/irsa/data/aws_s3_bucket_object_cluster-completed.spec_content @@ -201,6 +201,8 @@ spec: podCIDR: 100.96.0.0/11 secretStore: memfs://clusters.example.com/minimal.example.com/secrets serviceAccountIssuerDiscovery: + additionalAudiences: + - sts.amazonaws.com discoveryStore: memfs://discovery.example.com/minimal.example.com enableAWSOIDCProvider: true serviceClusterIPRange: 100.64.0.0/13 diff --git a/tests/integration/update_cluster/irsa/in-v1alpha2.yaml b/tests/integration/update_cluster/irsa/in-v1alpha2.yaml index 1d83f52349..b425af2cd6 100644 --- a/tests/integration/update_cluster/irsa/in-v1alpha2.yaml +++ b/tests/integration/update_cluster/irsa/in-v1alpha2.yaml @@ -58,6 +58,8 @@ spec: serviceAccountIssuerDiscovery: enableAWSOIDCProvider: true discoveryStore: memfs://discovery.example.com/minimal.example.com + additionalAudiences: + - sts.amazonaws.com sshAccess: - 0.0.0.0/0 topology: diff --git a/tests/integration/update_cluster/irsa/kubernetes.tf b/tests/integration/update_cluster/irsa/kubernetes.tf index a59790bd36..08456edb86 100644 --- a/tests/integration/update_cluster/irsa/kubernetes.tf +++ b/tests/integration/update_cluster/irsa/kubernetes.tf @@ -291,7 +291,7 @@ resource "aws_iam_instance_profile" "nodes-minimal-example-com" { } resource "aws_iam_openid_connect_provider" "minimal-example-com" { - client_id_list = ["amazonaws.com"] + client_id_list = ["amazonaws.com", "sts.amazonaws.com"] tags = { "KubernetesCluster" = "minimal.example.com" "Name" = "minimal.example.com" diff --git a/upup/pkg/fi/cloudup/awstasks/BUILD.bazel b/upup/pkg/fi/cloudup/awstasks/BUILD.bazel index 098379992d..01f1da69e1 100644 --- a/upup/pkg/fi/cloudup/awstasks/BUILD.bazel +++ b/upup/pkg/fi/cloudup/awstasks/BUILD.bazel @@ -109,6 +109,7 @@ go_library( "//vendor/github.com/aws/aws-sdk-go/service/iam:go_default_library", "//vendor/github.com/aws/aws-sdk-go/service/route53:go_default_library", "//vendor/github.com/aws/aws-sdk-go/service/sqs:go_default_library", + "//vendor/k8s.io/apimachinery/pkg/util/sets:go_default_library", "//vendor/k8s.io/apimachinery/pkg/util/validation/field:go_default_library", "//vendor/k8s.io/klog/v2:go_default_library", ], diff --git a/upup/pkg/fi/cloudup/awstasks/iamoidcprovider.go b/upup/pkg/fi/cloudup/awstasks/iamoidcprovider.go index 395d590773..e573ce6a1f 100644 --- a/upup/pkg/fi/cloudup/awstasks/iamoidcprovider.go +++ b/upup/pkg/fi/cloudup/awstasks/iamoidcprovider.go @@ -23,6 +23,7 @@ import ( "github.com/aws/aws-sdk-go/aws" "github.com/aws/aws-sdk-go/service/iam" + "k8s.io/apimachinery/pkg/util/sets" "k8s.io/klog/v2" "k8s.io/kops/upup/pkg/fi" "k8s.io/kops/upup/pkg/fi/cloudup/awsup" @@ -110,9 +111,6 @@ func (s *IAMOIDCProvider) CheckChanges(a, e, changes *IAMOIDCProvider) error { } if a != nil { - if changes.ClientIDs != nil { - return fi.CannotChangeField("ClientIDs") - } if changes.URL != nil { return fi.CannotChangeField("URL") } @@ -178,6 +176,38 @@ func (p *IAMOIDCProvider) RenderAWS(t *awsup.AWSAPITarget, a, e, changes *IAMOID } } } + if changes.ClientIDs != nil { + actual := sets.NewString() + for _, aud := range a.ClientIDs { + actual.Insert(*aud) + } + expected := sets.NewString() + for _, aud := range e.ClientIDs { + expected.Insert(*aud) + } + toRemove := actual.Difference(expected) + for _, elem := range toRemove.List() { + request := &iam.RemoveClientIDFromOpenIDConnectProviderInput{ + OpenIDConnectProviderArn: a.arn, + ClientID: &elem, + } + _, err := t.Cloud.IAM().RemoveClientIDFromOpenIDConnectProvider(request) + if err != nil { + return fmt.Errorf("error removing audience %s to IAMOIDCProvider: %v", elem, err) + } + } + toAdd := expected.Difference(actual) + for _, elem := range toAdd.List() { + request := &iam.AddClientIDToOpenIDConnectProviderInput{ + OpenIDConnectProviderArn: a.arn, + ClientID: &elem, + } + _, err := t.Cloud.IAM().AddClientIDToOpenIDConnectProvider(request) + if err != nil { + return fmt.Errorf("error adding audience %s to IAMOIDCProvider: %v", elem, err) + } + } + } } return nil } From e438897665837038c0494e5b80acd162221c41d0 Mon Sep 17 00:00:00 2001 From: eddycharly Date: Wed, 29 Sep 2021 13:38:29 +0200 Subject: [PATCH 2/2] Apply suggestions from code review Co-authored-by: Peter Rifel --- k8s/crds/kops.k8s.io_clusters.yaml | 2 +- pkg/apis/kops/cluster.go | 2 +- pkg/apis/kops/v1alpha2/cluster.go | 2 +- 3 files changed, 3 insertions(+), 3 deletions(-) diff --git a/k8s/crds/kops.k8s.io_clusters.yaml b/k8s/crds/kops.k8s.io_clusters.yaml index 2e5a97f797..8693a11c3c 100644 --- a/k8s/crds/kops.k8s.io_clusters.yaml +++ b/k8s/crds/kops.k8s.io_clusters.yaml @@ -4334,7 +4334,7 @@ spec: properties: additionalAudiences: description: AdditionalAudiences adds user defined audiences to - the provisionned AWS OIDC provider + the provisioned AWS OIDC provider items: type: string type: array diff --git a/pkg/apis/kops/cluster.go b/pkg/apis/kops/cluster.go index 2b3ca154cf..69fe109fc0 100644 --- a/pkg/apis/kops/cluster.go +++ b/pkg/apis/kops/cluster.go @@ -223,7 +223,7 @@ type ServiceAccountIssuerDiscoveryConfig struct { DiscoveryStore string `json:"discoveryStore,omitempty"` // EnableAWSOIDCProvider will provision an AWS OIDC provider that trusts the ServiceAccount Issuer EnableAWSOIDCProvider bool `json:"enableAWSOIDCProvider,omitempty"` - // AdditionalAudiences adds user defined audiences to the provisionned AWS OIDC provider + // AdditionalAudiences adds user defined audiences to the provisioned AWS OIDC provider AdditionalAudiences []string `json:"additionalAudiences,omitempty"` } diff --git a/pkg/apis/kops/v1alpha2/cluster.go b/pkg/apis/kops/v1alpha2/cluster.go index cee4dbbd90..8f5fc2da5a 100644 --- a/pkg/apis/kops/v1alpha2/cluster.go +++ b/pkg/apis/kops/v1alpha2/cluster.go @@ -221,7 +221,7 @@ type ServiceAccountIssuerDiscoveryConfig struct { DiscoveryStore string `json:"discoveryStore,omitempty"` // EnableAWSOIDCProvider will provision an AWS OIDC provider that trusts the ServiceAccount Issuer EnableAWSOIDCProvider bool `json:"enableAWSOIDCProvider,omitempty"` - // AdditionalAudiences adds user defined audiences to the provisionned AWS OIDC provider + // AdditionalAudiences adds user defined audiences to the provisioned AWS OIDC provider AdditionalAudiences []string `json:"additionalAudiences,omitempty"` }