Skip to content
This repository has been archived by the owner on Jan 11, 2023. It is now read-only.

Use enable-admission-plugins key for v1.10 and above #3090

Merged
merged 3 commits into from
May 30, 2018
Merged

Use enable-admission-plugins key for v1.10 and above #3090

merged 3 commits into from
May 30, 2018

Conversation

billpratt
Copy link
Contributor

@billpratt billpratt commented May 25, 2018

What this PR does / why we need it:

In k8s v1.10, the --admission-control flag was deprecated and replaced with --enable-admission-plugins. This PR uses the new flag for v1.10.

v1.10 release notes:

Add --enable-admission-plugins --disable-admission-plugins flags and deprecate --admission-control. When using the separate flag, the order in which they're specified doesn't matter.

More details:

Which issue this PR fixes (optional, in fixes #<issue number>(, fixes #<issue_number>, ...) format, will close that issue when PR gets merged): fixes #

Special notes for your reviewer:

If applicable:

  • documentation
  • unit tests
  • tested backward compatibility (ie. deploy with previous version, upgrade with this branch)

Release note:

@acs-bot acs-bot added the size/M label May 25, 2018
@billpratt
Copy link
Contributor Author

/assign @CecileRobertMichon

@billpratt
Copy link
Contributor Author

@CecileRobertMichon you and I spoke about this briefly during sync week.

admissionControlKey := "--enable-admission-plugins"
var admissionControlValues string

// --admission-control was used in v1.9 and earlier and was deprecated in 1.10
Copy link
Contributor

Choose a reason for hiding this comment

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

this looks to me like it's doing the opposite of what the comment says

Copy link
Contributor

Choose a reason for hiding this comment

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

sorry missed the ! please disregard my comment

@codecov
Copy link

codecov bot commented May 29, 2018

Codecov Report

Merging #3090 into master will increase coverage by 0.04%.
The diff coverage is 87.5%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #3090      +/-   ##
==========================================
+ Coverage    51.5%   51.54%   +0.04%     
==========================================
  Files          99       99              
  Lines       15036    15049      +13     
==========================================
+ Hits         7744     7757      +13     
  Misses       6582     6582              
  Partials      710      710
Impacted Files Coverage Δ
pkg/acsengine/defaults-apiserver.go 97.89% <87.5%> (+0.33%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update da8646d...3efba48. Read the comment docs.

@billpratt
Copy link
Contributor Author

@CecileRobertMichon This PR also sets MutatingAdmissionWebhook and ValidatingAdmissionWebhook for k8s version 1.9 and above. acs-engine currently sets AlwaysPullImages as a default admission control. In v1.10, this causes issues. Please see here: kubernetes/kubernetes#64333

Does AlwaysPullImages really need to be a default admission control?

cc @brendanburns

@jackfrancis
Copy link
Member

Also note the addition of DefaultTolerationSeconds

@ghost ghost assigned jackfrancis May 30, 2018
@ghost ghost added the in progress label May 30, 2018
@jackfrancis
Copy link
Member

/approve
/lgtm

Added docs

@acs-bot
Copy link

acs-bot commented May 30, 2018

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: jackfrancis

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

@jackfrancis jackfrancis merged commit f8753ad into Azure:master May 30, 2018
@ghost ghost removed the in progress label May 30, 2018
@jackfrancis
Copy link
Member

Thanks @billpratt!

@billpratt
Copy link
Contributor Author

@jackfrancis Does AlwaysPullImages need to be enabled? Please see above

@jackfrancis
Copy link
Member

@billpratt Can you kindly track the progress of the pathological behaviors of AlwaysPullImages w/ MutatingAdmissionWebhook? I'm happy to have this as-is at the moment, will be curious if folks complain about the security ramifications of removing AlwaysPullImages outlined here:

https://kubernetes.io/docs/reference/access-authn-authz/admission-controllers/#alwayspullimages

(Of course this is all user-configurable but our defaults add value to users)

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants