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.

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

Lakom uses warning instead of failure policy Ignore #74

Merged
merged 8 commits into from
Mar 14, 2024

Conversation

vpnachev
Copy link
Member

@vpnachev vpnachev commented Feb 21, 2024

What this PR does / why we need it:
Lakom uses warning instead of failure policy Ignore

Which issue(s) this PR fixes:
Fixes #

Special notes for your reviewer:

Release note:

Lakom admission controller is extended with a new flag `--insecure-allow-untrusted-images`. When it is set, the admission webhook returns just warning but still allows the images that are not signed or are not signed with trusted keys. 
Lakom gardener extension controller configuration has new field `allowUntrustedImages`, it is used to control the lakom admission controller flag `--insecure-allow-untrusted-images`.
:warning: Lakom admission webhooks now always use failure policy `Fail` and it is no longer possible to change it to `Ignore`. If you want to allow untrusted images
- for the extension controller you can set the field `allowUntrustedImages` to true
- for the lakom application you can set the flag  `--insecure-allow-untrusted-images` to true
Both configs are also exposed via the helm charts values and ControllerDeployment config.

@gardener-robot gardener-robot added kind/api-change API change with impact on API users needs/second-opinion Needs second review by someone else labels Feb 21, 2024
@vpnachev vpnachev requested a review from a team as a code owner February 21, 2024 20:52
@gardener-robot gardener-robot added needs/review Needs review size/xl Size of pull request is huge (see gardener-robot robot/bots/size.py) labels Feb 21, 2024
@gardener-robot-ci-3 gardener-robot-ci-3 added reviewed/ok-to-test Has approval for testing (check PR in detail before setting this label because PR is run on CI/CD) needs/ok-to-test Needs approval for testing (check PR in detail before setting this label because PR is run on CI/CD) and removed reviewed/ok-to-test Has approval for testing (check PR in detail before setting this label because PR is run on CI/CD) labels Feb 21, 2024
Copy link
Member

@dimityrmirchev dimityrmirchev left a comment

Choose a reason for hiding this comment

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

Not a strong opinion, so I am fine both ways.

As a general comment I think that "skip" is not the proper term to use here since we still do the validation, but allow the image even if validation fails. So IMO "allow" should be the better term.

A second note. What is the use case for this change and why is it better than the ability to ignore denied requests as it is now?

cmd/lakom/app/app.go Outdated Show resolved Hide resolved
@vpnachev
Copy link
Member Author

vpnachev commented Mar 5, 2024

A second note. What is the use case for this change and why is it better than the ability to ignore denied requests as it is now?

The failure policy in kubernetes is scoped only to completing the admission review process, not the result of the admission review, i.e. whether the change is allowed or not. Failure policy Ignore is useful when the admission webhook is not highly available or the network is not reliable and the admission result is optional, i.e. the admission result of the webhook is not mandatory.

Lakom is deployed HA and by default runs locally to the cluster (in-cluster for the seeds and in the control plane for the shoots), so we should not have any expectation that the admission reviewes are subject to failure because of unreliable network, unavailable lakom instances, etc. Failure policy Ignore was proposed as a way to disable the image verification checks when it is expected unsigned or signed by untrusted signing keys images to be used, for this case an admission review response allowing the request but with warning is more appropriate.

There are several benefits from this change:

  1. Remove the copied and modified version of sigs.k8s.io/controller-runtime/pkg/webhook/admission, so no need to maintain the copy of 3rd party lib.
  2. From monitoring point of view, unsigned images will not be counted as API server failed to complete the admission review process, but as denied requests.
  3. The error message send to clients will correctly reflect why the request is denied, instead of representing it as failure to complete the admission review. Currently, when the webhook is running in Ignore policy, the clients do not receive any information that the image has issues.

@vpnachev vpnachev force-pushed the enh/use-admission-warnings branch from e3740d9 to 15eb9f6 Compare March 5, 2024 13:20
@gardener-robot-ci-1 gardener-robot-ci-1 added the reviewed/ok-to-test Has approval for testing (check PR in detail before setting this label because PR is run on CI/CD) label Mar 5, 2024
@vpnachev vpnachev requested a review from dimityrmirchev March 5, 2024 13:21
@gardener-robot-ci-1 gardener-robot-ci-1 removed the reviewed/ok-to-test Has approval for testing (check PR in detail before setting this label because PR is run on CI/CD) label Mar 5, 2024
New flag `--insecure-allow-untrusted-images` allows lakom to run in
detective mode, it will not disallow unsigned images, it will just
return warnings.
Extension controller config is extended with `allowUntrustedImages`
field which is set as value of the flag `insecure-allow-untrusted-images`.
Now when failure policy is always Fail, there is no need of custom
admission handler to set the HTTP status code to be the same as the
admission review status code.
It is no longer possible to control the failure policy of the
admission webhook configurations. If unsigned images must be allowed,
`allowUntrustedImages=true` can be used.
@vpnachev vpnachev force-pushed the enh/use-admission-warnings branch from 15eb9f6 to c009d67 Compare March 14, 2024 12:13
@gardener-robot-ci-2 gardener-robot-ci-2 added reviewed/ok-to-test Has approval for testing (check PR in detail before setting this label because PR is run on CI/CD) and removed reviewed/ok-to-test Has approval for testing (check PR in detail before setting this label because PR is run on CI/CD) labels Mar 14, 2024
@gardener-robot-ci-2 gardener-robot-ci-2 added reviewed/ok-to-test Has approval for testing (check PR in detail before setting this label because PR is run on CI/CD) and removed reviewed/ok-to-test Has approval for testing (check PR in detail before setting this label because PR is run on CI/CD) labels Mar 14, 2024
dimityrmirchev
dimityrmirchev previously approved these changes Mar 14, 2024
Copy link
Member

@dimityrmirchev dimityrmirchev left a comment

Choose a reason for hiding this comment

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

Two small suggestions that are completely optional

/lgtm

pkg/controller/lifecycle/actuator_test.go Outdated Show resolved Hide resolved
pkg/controller/lifecycle/actuator_test.go Outdated Show resolved Hide resolved
@gardener-robot gardener-robot added reviewed/lgtm Has approval for merging and removed needs/review Needs review needs/second-opinion Needs second review by someone else labels Mar 14, 2024
@gardener-robot-ci-3 gardener-robot-ci-3 added the reviewed/ok-to-test Has approval for testing (check PR in detail before setting this label because PR is run on CI/CD) label Mar 14, 2024
Co-authored-by: Dimitar Mirchev <[email protected]>
@gardener-robot gardener-robot added needs/review Needs review needs/second-opinion Needs second review by someone else and removed needs/review Needs review reviewed/lgtm Has approval for merging labels Mar 14, 2024
@gardener-robot-ci-1 gardener-robot-ci-1 removed the reviewed/ok-to-test Has approval for testing (check PR in detail before setting this label because PR is run on CI/CD) label Mar 14, 2024
@vpnachev vpnachev requested a review from dimityrmirchev March 14, 2024 13:56
Copy link
Member

@dimityrmirchev dimityrmirchev left a comment

Choose a reason for hiding this comment

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

/lgtm

@gardener-robot gardener-robot added reviewed/lgtm Has approval for merging and removed needs/second-opinion Needs second review by someone else labels Mar 14, 2024
@gardener-robot-ci-1 gardener-robot-ci-1 added the reviewed/ok-to-test Has approval for testing (check PR in detail before setting this label because PR is run on CI/CD) label Mar 14, 2024
@vpnachev vpnachev merged commit 59d9866 into gardener:main Mar 14, 2024
8 checks passed
@vpnachev vpnachev deleted the enh/use-admission-warnings branch March 14, 2024 14:27
@gardener-robot gardener-robot added the status/closed Issue is closed (either delivered or triaged) label Mar 14, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
kind/api-change API change with impact on API users needs/ok-to-test Needs approval for testing (check PR in detail before setting this label because PR is run on CI/CD) reviewed/lgtm Has approval for merging reviewed/ok-to-test Has approval for testing (check PR in detail before setting this label because PR is run on CI/CD) size/xl Size of pull request is huge (see gardener-robot robot/bots/size.py) status/closed Issue is closed (either delivered or triaged)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants