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

Reduce policy size further #11843

Merged
merged 9 commits into from
Jul 4, 2021
Merged

Reduce policy size further #11843

merged 9 commits into from
Jul 4, 2021

Conversation

olemarkus
Copy link
Member

@olemarkus olemarkus commented Jun 22, 2021 •
edited
Loading

This PR reduces policies by using a set for the actions that have well-known conditions (any or tagged by the cluster tag).

@k8s-ci-robot k8s-ci-robot added cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. labels Jun 22, 2021
@k8s-ci-robot k8s-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jun 25, 2021
@k8s-ci-robot k8s-ci-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jun 27, 2021
@olemarkus olemarkus force-pushed the reduce-policy-size-further branch 3 times, most recently from f9e48a6 to ec5a2b5 Compare June 27, 2021 09:07
@olemarkus
Copy link
Member Author

/test pull-e2e-kops-aws-load-balancer-controller

1 similar comment
@olemarkus
Copy link
Member Author

/test pull-e2e-kops-aws-load-balancer-controller

@olemarkus
Copy link
Member Author

/test pull-e2e-kops-aws-load-balancer-controller

@olemarkus
Copy link
Member Author

/cc @rifelpet

johngmyers
johngmyers reviewed Jun 27, 2021
View reviewed changes
Copy link
Member

@johngmyers johngmyers left a comment

Choose a reason for hiding this comment

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

The first commit is extremely big to review. It would be much easier if it were broken up into smaller commits.

I like to put nontrivial refactors in separate commits than semantic changes. When golden files change significantly I like to put them in an immediately following commit.

pkg/model/iam/iam_builder.go Show resolved Hide resolved
pkg/model/iam/iam_builder.go Show resolved Hide resolved
pkg/model/iam/iam_builder.go Show resolved Hide resolved
pkg/model/iam/iam_builder.go Show resolved Hide resolved
@olemarkus
Copy link
Member Author

I think that was a lot of valuable input and suggestions, but perhaps out of the scope of this PR. Would be nice to get this in as the policy limit is blocking/breaking stuff right now.

@johngmyers
Copy link
Member

The only thing blocking is the difficulty of reviewing such a large commit.

@olemarkus
Copy link
Member Author

The code changes aren't that big, are they? For the generated output files, the changes would be fairly big even if this was done addon-by-addon. https://github.com/kubernetes/kops/pull/11843/files#diff-dfcb900da852396ff87561d730a05b2c55fb43e394e4cbe7586a913fc84c9400 gives perhaps the best idea of how this change.

The new function in one go both deduplicates and sorts as a consequence of using sets.

@k8s-ci-robot k8s-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jun 30, 2021
@k8s-ci-robot k8s-ci-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jul 1, 2021
@olemarkus olemarkus requested a review from johngmyers July 2, 2021 07:44
@olemarkus olemarkus mentioned this pull request Jul 3, 2021
Merged
@hakman
Copy link
Member

hakman commented Jul 3, 2021

I managed to follow the commits and seems fine. Now that they are split, it is a little easier.
/lgtm

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Jul 3, 2021
@rifelpet
Copy link
Member

rifelpet commented Jul 3, 2021

Some of the condition keys are changing for certain actions:

  • autoscaling:SetDesiredCapacity is changing from autoscaling:ResourceTag/KubernetesCluster to aws:ResourceTag/KubernetesCluster
  • ec2:DeleteTags and ec2:CreateRoute are changing from ec2:ResourceTag/KubernetesCluster to aws:ResourceTag/KubernetesCluster

Given that some of these actions are not exercised during the e2e tests, have we confirmed these are compatible changes?

The aws:ResourceTag docs mention:

This key is included in the request context when the requested resource already has attached tags.

which I would guess would not apply to a Create* request and instead should use the aws:RequestTag condition instead. I also don't know if there is a functional difference between the aws, ec2, and autoscaling variants of these conditions. Does anyone know if they are interchangeable?

@olemarkus
Copy link
Member Author

aws:ResourceTag is interchangeable with the more specific ones. They were added to make it easier to build policies.

Create* should indeed use aws:RequestTag in most cases, but CreateRoute doesn't actually create an AWS resource. It adds a route to a RouteTable, and the Conditions are against the RouteTable.

You can verify both claims on this page: https://docs.aws.amazon.com/service-authorization/latest/reference/list_amazonec2.html

johngmyers
johngmyers approved these changes Jul 4, 2021
View reviewed changes
pkg/model/iam/iam_builder.go
"elasticloadbalancing:DescribeRules",
"elasticloadbalancing:DescribeTargetHealth",
"elasticloadbalancing:DescribeListenerCertificates",
"elasticloadbalancing:CreateRule",
Copy link
Member

Choose a reason for hiding this comment

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

Should this CreateRule permission be cluster-scoped?

pkg/model/iam/iam_builder.go
),
Resource: resource,
})
p.unconditionalAction.Insert(
Copy link
Member

Choose a reason for hiding this comment

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

Again, should some of these be cluster-scoped?

I see from the tests output that it wasn't cluster scoped before, so this is at best a follow-up item.

pkg/model/iam/iam_builder.go

func createResource(b *PolicyBuilder) stringorslice.StringOrSlice {
var resource stringorslice.StringOrSlice
if b.ResourceARN != nil {
Copy link
Member

Choose a reason for hiding this comment

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

Should we get rid of the rest of ResourceARN?

@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: johngmyers

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@k8s-ci-robot k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Jul 4, 2021
@k8s-ci-robot k8s-ci-robot merged commit cf834ce into kubernetes:master Jul 4, 2021
@k8s-ci-robot k8s-ci-robot added this to the v1.22 milestone Jul 4, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@johngmyers johngmyers johngmyers approved these changes

@zetaab zetaab Awaiting requested review from zetaab

@rifelpet rifelpet Awaiting requested review from rifelpet

Assignees

@johngmyers johngmyers

@hakman hakman

Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. lgtm "Looks good to me", indicates that a PR is ready to be merged. size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files.
Projects
None yet
Milestone
v1.22
Development

Successfully merging this pull request may close these issues.
5 participants
@olemarkus @johngmyers @hakman @rifelpet @k8s-ci-robot