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

Jump to bottom

Refactor IAM Policy Builder #16351

Merged
merged 4 commits into from Feb 13, 2024
Merged

Refactor IAM Policy Builder #16351

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
21804bf
Migrate to non-deprecated Sets implementation
rifelpet Feb 13, 2024
f098401
Rename StringOrSlice to StringOrSet, sort lists
rifelpet Feb 13, 2024
b526448
Rename stringorslice package to stringorset
rifelpet Feb 13, 2024
4643c66
./hack/update-expected.sh
rifelpet Feb 13, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
4 changes: 2 additions & 2 deletions pkg/model/awsmodel/iam.go
View file
Open in desktop
Expand Up @@ -28,7 +28,7 @@ import (
"k8s.io/kops/pkg/apis/kops"
"k8s.io/kops/pkg/model"
"k8s.io/kops/pkg/model/iam"
"k8s.io/kops/pkg/util/stringorslice"
"k8s.io/kops/pkg/util/stringorset"
"k8s.io/kops/upup/pkg/fi"
"k8s.io/kops/upup/pkg/fi/cloudup/awstasks"
"k8s.io/kops/upup/pkg/fi/cloudup/awsup"
Expand Down Expand Up @@ -432,7 +432,7 @@ func formatAWSIAMStatement(accountId, partition, oidcProvider, namespace, name s
Principal: iam.Principal{
Federated: "arn:" + partition + ":iam::" + accountId + ":oidc-provider/" + oidcProvider,
},
Action: stringorslice.String("sts:AssumeRoleWithWebIdentity"),
Action: stringorset.String("sts:AssumeRoleWithWebIdentity"),
Condition: map[string]interface{}{
condition: map[string]interface{}{
oidcProvider + ":sub": "system:serviceaccount:" + namespace + ":" + name,
Expand Down
6 changes: 3 additions & 3 deletions pkg/model/awsmodel/iam_test.go
View file
Open in desktop
Expand Up @@ -21,7 +21,7 @@ import (
"testing"

"k8s.io/kops/pkg/model/iam"
"k8s.io/kops/pkg/util/stringorslice"
"k8s.io/kops/pkg/util/stringorset"
)

func TestIAMServiceEC2(t *testing.T) {
Expand Down Expand Up @@ -70,7 +70,7 @@ func Test_formatAWSIAMStatement(t *testing.T) {
Principal: iam.Principal{
Federated: "arn:aws-test:iam::0123456789:oidc-provider/oidc-test",
},
Action: stringorslice.String("sts:AssumeRoleWithWebIdentity"),
Action: stringorset.String("sts:AssumeRoleWithWebIdentity"),
Condition: map[string]interface{}{
"StringEquals": map[string]interface{}{
"oidc-test:sub": "system:serviceaccount:test:test",
Expand Down Expand Up @@ -104,7 +104,7 @@ func Test_formatAWSIAMStatement(t *testing.T) {
Principal: iam.Principal{
Federated: "arn:aws-test:iam::0123456789:oidc-provider/oidc-test",
},
Action: stringorslice.String("sts:AssumeRoleWithWebIdentity"),
Action: stringorset.String("sts:AssumeRoleWithWebIdentity"),
Condition: map[string]interface{}{
"StringLike": map[string]interface{}{
"oidc-test:sub": "system:serviceaccount:test-*:test",
Expand Down
14 changes: 7 additions & 7 deletions pkg/model/components/addonmanifests/certmanager/iam.go
View file
Open in desktop
Expand Up @@ -22,7 +22,7 @@ import (
"k8s.io/apimachinery/pkg/types"

"k8s.io/kops/pkg/model/iam"
"k8s.io/kops/pkg/util/stringorslice"
"k8s.io/kops/pkg/util/stringorset"
)

// ServiceAccount represents the service-account used by cert-manager.
Expand Down Expand Up @@ -57,22 +57,22 @@ func addCertManagerPermissions(b *iam.PolicyBuilder, p *iam.Policy) {

p.Statement = append(p.Statement, &iam.Statement{
Effect: iam.StatementEffectAllow,
Action: stringorslice.Of("route53:ChangeResourceRecordSets",
Action: stringorset.Of("route53:ChangeResourceRecordSets",
"route53:ListResourceRecordSets",
),
Resource: stringorslice.Slice(zoneResources),
Resource: stringorset.Set(zoneResources),
})

p.Statement = append(p.Statement, &iam.Statement{
Effect: iam.StatementEffectAllow,
Action: stringorslice.Slice([]string{"route53:GetChange"}),
Resource: stringorslice.Slice([]string{fmt.Sprintf("arn:%v:route53:::change/*", b.Partition)}),
Action: stringorset.Set([]string{"route53:GetChange"}),
Resource: stringorset.Set([]string{fmt.Sprintf("arn:%v:route53:::change/*", b.Partition)}),
})

wildcard := stringorslice.Slice([]string{"*"})
wildcard := stringorset.Set([]string{"*"})
p.Statement = append(p.Statement, &iam.Statement{
Effect: iam.StatementEffectAllow,
Action: stringorslice.Slice([]string{"route53:ListHostedZonesByName"}),
Action: stringorset.Set([]string{"route53:ListHostedZonesByName"}),
Resource: wildcard,
})
}
74 changes: 37 additions & 37 deletions pkg/model/iam/iam_builder.go
View file
Open in desktop
Expand Up @@ -38,7 +38,7 @@ import (

"k8s.io/kops/pkg/apis/kops"
"k8s.io/kops/pkg/apis/kops/model"
"k8s.io/kops/pkg/util/stringorslice"
"k8s.io/kops/pkg/util/stringorset"
"k8s.io/kops/upup/pkg/fi"
"k8s.io/kops/upup/pkg/fi/cloudup/awstasks"
"k8s.io/kops/util/pkg/vfs"
Expand All @@ -50,9 +50,9 @@ const PolicyDefaultVersion = "2012-10-17"
// Policy Struct is a collection of fields that form a valid AWS policy document
type Policy struct {
clusterName string
unconditionalAction sets.String
clusterTaggedAction sets.String
clusterTaggedCreateAction sets.String
unconditionalAction sets.Set[string]
clusterTaggedAction sets.Set[string]
clusterTaggedCreateAction sets.Set[string]
Statement []*Statement
partition string
Version string
Expand All @@ -77,8 +77,8 @@ func (p *Policy) AddEC2CreateAction(actions, resources []string) {
p.Statement = append(p.Statement,
&Statement{
Effect: StatementEffectAllow,
Action: stringorslice.String("ec2:CreateTags"),
Resource: stringorslice.Slice(actualResources),
Action: stringorset.String("ec2:CreateTags"),
Resource: stringorset.Set(actualResources),
Condition: Condition{
"StringEquals": map[string]interface{}{
"aws:RequestTag/KubernetesCluster": p.clusterName,
Expand All @@ -89,11 +89,11 @@ func (p *Policy) AddEC2CreateAction(actions, resources []string) {

&Statement{
Effect: StatementEffectAllow,
Action: stringorslice.Slice([]string{
Action: stringorset.Set([]string{
"ec2:CreateTags",
"ec2:DeleteTags", // aws.go, tag.go
}),
Resource: stringorslice.Slice(actualResources),
Resource: stringorset.Set(actualResources),
Condition: Condition{
"Null": map[string]string{
"aws:RequestTag/KubernetesCluster": "true",
Expand All @@ -111,15 +111,15 @@ func (p *Policy) AsJSON() (string, error) {
if len(p.unconditionalAction) > 0 {
p.Statement = append(p.Statement, &Statement{
Effect: StatementEffectAllow,
Action: stringorslice.Of(p.unconditionalAction.List()...),
Resource: stringorslice.String("*"),
Action: stringorset.Of(sets.List(p.unconditionalAction)...),
Resource: stringorset.String("*"),
})
}
if len(p.clusterTaggedAction) > 0 {
p.Statement = append(p.Statement, &Statement{
Effect: StatementEffectAllow,
Action: stringorslice.Of(p.clusterTaggedAction.List()...),
Resource: stringorslice.String("*"),
Action: stringorset.Of(sets.List(p.clusterTaggedAction)...),
Resource: stringorset.String("*"),
Condition: Condition{
"StringEquals": map[string]string{
"aws:ResourceTag/KubernetesCluster": p.clusterName,
Expand All @@ -130,8 +130,8 @@ func (p *Policy) AsJSON() (string, error) {
if len(p.clusterTaggedCreateAction) > 0 {
p.Statement = append(p.Statement, &Statement{
Effect: StatementEffectAllow,
Action: stringorslice.Of(p.clusterTaggedCreateAction.List()...),
Resource: stringorslice.String("*"),
Action: stringorset.Of(sets.List(p.clusterTaggedCreateAction)...),
Resource: stringorset.String("*"),
Condition: Condition{
"StringEquals": map[string]string{
"aws:RequestTag/KubernetesCluster": p.clusterName,
Expand All @@ -143,8 +143,8 @@ func (p *Policy) AsJSON() (string, error) {
if p.clusterTaggedCreateAction.Has("ec2:CreateSecurityGroup") {
p.Statement = append(p.Statement, &Statement{
Effect: StatementEffectAllow,
Action: stringorslice.Of("ec2:CreateSecurityGroup"),
Resource: stringorslice.String(fmt.Sprintf("arn:%s:ec2:*:*:vpc/*", p.partition)),
Action: stringorset.Of("ec2:CreateSecurityGroup"),
Resource: stringorset.String(fmt.Sprintf("arn:%s:ec2:*:*:vpc/*", p.partition)),
})
}
}
Expand Down Expand Up @@ -176,8 +176,8 @@ type Condition map[string]interface{}
type Statement struct {
Effect StatementEffect
Principal Principal
Action stringorslice.StringOrSlice
Resource stringorslice.StringOrSlice
Action stringorset.StringOrSet
Resource stringorset.StringOrSet
Condition Condition
}

Expand Down Expand Up @@ -338,9 +338,9 @@ func NewPolicy(clusterName, partition string) *Policy {
p := &Policy{
Version: PolicyDefaultVersion,
clusterName: clusterName,
unconditionalAction: sets.NewString(),
clusterTaggedAction: sets.NewString(),
clusterTaggedCreateAction: sets.NewString(),
unconditionalAction: sets.New[string](),
clusterTaggedAction: sets.New[string](),
clusterTaggedCreateAction: sets.New[string](),
partition: partition,
}
return p
Expand Down Expand Up @@ -594,13 +594,13 @@ func (b *PolicyBuilder) AddS3Permissions(p *Policy) (*Policy, error) {
for _, s3Bucket := range s3Buckets.List() {
p.Statement = append(p.Statement, &Statement{
Effect: StatementEffectAllow,
Action: stringorslice.Of(
Action: stringorset.Of(
"s3:GetBucketLocation",
"s3:GetEncryptionConfiguration",
"s3:ListBucket",
"s3:ListBucketVersions",
),
Resource: stringorslice.Slice([]string{
Resource: stringorset.Set([]string{
fmt.Sprintf("arn:%v:s3:::%v", p.partition, s3Bucket),
}),
})
Expand All @@ -612,13 +612,13 @@ func (b *PolicyBuilder) AddS3Permissions(p *Policy) (*Policy, error) {
func (b *PolicyBuilder) buildS3WriteStatements(p *Policy, iamS3Path string) {
p.Statement = append(p.Statement, &Statement{
Effect: StatementEffectAllow,
Action: stringorslice.Slice([]string{
Action: stringorset.Set([]string{
"s3:GetObject",
"s3:DeleteObject",
"s3:DeleteObjectVersion",
"s3:PutObject",
}),
Resource: stringorslice.Of(
Resource: stringorset.Of(
fmt.Sprintf("arn:%v:s3:::%v/*", p.partition, iamS3Path),
),
})
Expand All @@ -640,8 +640,8 @@ func (b *PolicyBuilder) buildS3GetStatements(p *Policy, iamS3Path string) error

p.Statement = append(p.Statement, &Statement{
Effect: StatementEffectAllow,
Action: stringorslice.Slice([]string{"s3:Get*"}),
Resource: stringorslice.Of(resources...),
Action: stringorset.Set([]string{"s3:Get*"}),
Resource: stringorset.Of(resources...),
})
}
return nil
Expand Down Expand Up @@ -800,10 +800,10 @@ func addEtcdManagerPermissions(p *Policy) {
p.Statement = append(p.Statement,
&Statement{
Effect: StatementEffectAllow,
Action: stringorslice.Of(
Action: stringorset.Of(
"ec2:AttachVolume",
),
Resource: stringorslice.Slice([]string{"*"}),
Resource: stringorset.Set([]string{"*"}),
Condition: Condition{
"StringEquals": map[string]string{
"aws:ResourceTag/k8s.io/role/master": "1",
Expand Down Expand Up @@ -1061,22 +1061,22 @@ func AddDNSControllerPermissions(b *PolicyBuilder, p *Policy) {

p.Statement = append(p.Statement, &Statement{
Effect: StatementEffectAllow,
Action: stringorslice.Of("route53:ChangeResourceRecordSets",
Action: stringorset.Of("route53:ChangeResourceRecordSets",
"route53:ListResourceRecordSets",
"route53:GetHostedZone"),
Resource: stringorslice.Slice([]string{fmt.Sprintf("arn:%v:route53:::hostedzone/%v", b.Partition, hostedZoneID)}),
Resource: stringorset.Set([]string{fmt.Sprintf("arn:%v:route53:::hostedzone/%v", b.Partition, hostedZoneID)}),
})

p.Statement = append(p.Statement, &Statement{
Effect: StatementEffectAllow,
Action: stringorslice.Slice([]string{"route53:GetChange"}),
Resource: stringorslice.Slice([]string{fmt.Sprintf("arn:%v:route53:::change/*", b.Partition)}),
Action: stringorset.Set([]string{"route53:GetChange"}),
Resource: stringorset.Set([]string{fmt.Sprintf("arn:%v:route53:::change/*", b.Partition)}),
})

wildcard := stringorslice.Slice([]string{"*"})
wildcard := stringorset.Set([]string{"*"})
p.Statement = append(p.Statement, &Statement{
Effect: StatementEffectAllow,
Action: stringorslice.Slice([]string{"route53:ListHostedZones", "route53:ListTagsForResource"}),
Action: stringorset.Set([]string{"route53:ListHostedZones", "route53:ListTagsForResource"}),
Resource: wildcard,
})
}
Expand Down Expand Up @@ -1169,10 +1169,10 @@ func addAmazonVPCCNIPermissions(p *Policy) {
p.Statement = append(p.Statement,
&Statement{
Effect: StatementEffectAllow,
Action: stringorslice.Slice([]string{
Action: stringorset.Set([]string{
"ec2:CreateTags",
}),
Resource: stringorslice.Slice([]string{
Resource: stringorset.Set([]string{
strings.Join([]string{"arn:", p.partition, ":ec2:*:*:network-interface/*"}, ""),
}),
},
Expand Down
12 changes: 6 additions & 6 deletions pkg/model/iam/iam_builder_test.go
View file
Open in desktop
Expand Up @@ -26,7 +26,7 @@ import (
"k8s.io/kops/pkg/apis/kops"
"k8s.io/kops/pkg/testutils"
"k8s.io/kops/pkg/testutils/golden"
"k8s.io/kops/pkg/util/stringorslice"
"k8s.io/kops/pkg/util/stringorset"
"k8s.io/kops/upup/pkg/fi"
)

Expand All @@ -38,18 +38,18 @@ func TestRoundTrip(t *testing.T) {
{
IAM: &Statement{
Effect: StatementEffectAllow,
Action: stringorslice.Of("ec2:DescribeRegions"),
Resource: stringorslice.Of("*"),
Action: stringorset.Of("ec2:DescribeRegions"),
Resource: stringorset.Of("*"),
},
JSON: "{\"Action\":\"ec2:DescribeRegions\",\"Effect\":\"Allow\",\"Resource\":\"*\"}",
},
{
IAM: &Statement{
Effect: StatementEffectDeny,
Action: stringorslice.Of("ec2:DescribeRegions", "ec2:DescribeInstances"),
Resource: stringorslice.Of("a", "b"),
Action: stringorset.Of("ec2:DescribeRegions", "ec2:DescribeInstances"),
Resource: stringorset.Of("a", "b"),
},
JSON: "{\"Action\":[\"ec2:DescribeRegions\",\"ec2:DescribeInstances\"],\"Effect\":\"Deny\",\"Resource\":[\"a\",\"b\"]}",
JSON: "{\"Action\":[\"ec2:DescribeInstances\",\"ec2:DescribeRegions\"],\"Effect\":\"Deny\",\"Resource\":[\"a\",\"b\"]}",
},
{
IAM: &Statement{
Expand Down
8 changes: 4 additions & 4 deletions pkg/model/iam/tests/iam_builder_master_gossip.json
View file
Open in desktop
Expand Up @@ -45,8 +45,8 @@
},
"Effect": "Allow",
"Resource": [
"arn:aws-test:ec2:*:*:volume/*",
"arn:aws-test:ec2:*:*:snapshot/*"
"arn:aws-test:ec2:*:*:snapshot/*",
"arn:aws-test:ec2:*:*:volume/*"
]
},
{
Expand All @@ -64,8 +64,8 @@
},
"Effect": "Allow",
"Resource": [
"arn:aws-test:ec2:*:*:volume/*",
"arn:aws-test:ec2:*:*:snapshot/*"
"arn:aws-test:ec2:*:*:snapshot/*",
"arn:aws-test:ec2:*:*:volume/*"
]
},
{
Expand Down
8 changes: 4 additions & 4 deletions pkg/model/iam/tests/iam_builder_master_gossip_ecr.json
View file
Open in desktop
Expand Up @@ -45,8 +45,8 @@
},
"Effect": "Allow",
"Resource": [
"arn:aws-test:ec2:*:*:volume/*",
"arn:aws-test:ec2:*:*:snapshot/*"
"arn:aws-test:ec2:*:*:snapshot/*",
"arn:aws-test:ec2:*:*:volume/*"
]
},
{
Expand All @@ -64,8 +64,8 @@
},
"Effect": "Allow",
"Resource": [
"arn:aws-test:ec2:*:*:volume/*",
"arn:aws-test:ec2:*:*:snapshot/*"
"arn:aws-test:ec2:*:*:snapshot/*",
"arn:aws-test:ec2:*:*:volume/*"
]
},
{
Expand Down
8 changes: 4 additions & 4 deletions pkg/model/iam/tests/iam_builder_master_strict.json
View file
Open in desktop
Expand Up @@ -45,8 +45,8 @@
},
"Effect": "Allow",
"Resource": [
"arn:aws-test:ec2:*:*:volume/*",
"arn:aws-test:ec2:*:*:snapshot/*"
"arn:aws-test:ec2:*:*:snapshot/*",
"arn:aws-test:ec2:*:*:volume/*"
]
},
{
Expand All @@ -64,8 +64,8 @@
},
"Effect": "Allow",
"Resource": [
"arn:aws-test:ec2:*:*:volume/*",
"arn:aws-test:ec2:*:*:snapshot/*"
"arn:aws-test:ec2:*:*:snapshot/*",
"arn:aws-test:ec2:*:*:volume/*"
]
},
{
Expand Down