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

Amazon VPC CNI: Kubernetes 1.8+ Manifests #5290

Merged
merged 2 commits into from
Sep 21, 2018

Conversation

ripta
Copy link
Contributor

@ripta ripta commented Jun 8, 2018

This is a follow-up of PR #5119, which adds a separate addon manifest for Kubernetes 1.8+, as suggested by @mikesplain in this code review.

Namely, the only difference is that the new manifest for Kubernetes 1.8+ has the apiVersion rbac.authorization.k8s.io/v1 instead of rbac.authorization.k8s.io/v1beta1 for Kubernetes 1.7.

@k8s-ci-robot k8s-ci-robot added cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Jun 8, 2018
@ripta
Copy link
Contributor Author

ripta commented Jun 8, 2018

Placing this on hold until #5119 is merged.

/hold

@k8s-ci-robot k8s-ci-robot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Jun 8, 2018
@mikesplain
Copy link
Contributor

/ok-to-test

@k8s-ci-robot k8s-ci-robot removed the needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. label Jun 8, 2018
@mikesplain
Copy link
Contributor

@ripta Great thanks for the PR!

/assign

@mikesplain
Copy link
Contributor

Here's an idea, what if you also add a 1.10 manifest to this PR (with the 1.8 change as well), removing the k8s-ec2-srcdst like we talked about (it shouldn't be neccessary and isn't in AWS' official docs). Could be worth getting the discussion going on that now, but not injecting change for those running 1.8/1.9? That said, if users are currently running k8s-ec2-srcdst, removing it here shouldn't stop it in the cluster... so it may not be a big deal.

Thoughts?

Anyone else have thoughts, @chrislovecnm @justinsb @rdrgmnzs @chrisz100

@justinsb
Copy link
Member

justinsb commented Jun 8, 2018

I think that's a good suggestion @mikesplain. There's a number of changes around the VPC CNI plugin that we need to coordinate it looks like!

I believe RBAC v1beta1 is supported in 1.8 & 1.9 - I think we have a moratorium on any version removal at the moment. So what do you think @ripta?

@ripta
Copy link
Contributor Author

ripta commented Jun 13, 2018

@justinsb - Sounds fine. I had planned on submitting the removal of k8s-ec2-srcdst as a separate PR, mainly because I probably won't have time to validate that setup this week.

@justinsb
Copy link
Member

I propose we do this in kops 1.11 - it seems to be a cleanup for 1.10, and then we have plenty of time to look at the srcdst work etc

@justinsb justinsb added this to the 1.11 milestone Jun 19, 2018
@ripta
Copy link
Contributor Author

ripta commented Jul 3, 2018

@mikesplain - I finally had the chance to add a k8s 1.10 version of the manifest that removes srcdst. Can you take a look?

Also, I wasn't entirely clear re: v1beta1. Were you saying that we should keep the API version at v1beta1 for 1.8/1.9?

@justinsb
Copy link
Member

Thanks @ripta - this LGTM and the time is right!

For the API versions, we're not removing API versions, so v1beta1 should continue to work until we change the removal policy. We have to be careful that v1 is actually supported in the versions we're targeting - in this case in 1.8, and it was indeed introduced in 1.8 so this should work.

Technically we need to bump the version in bootstrapchannelbuilder.go, but I'll take a look at that as part of figuring out the other PRs!

/approve
/lgtm

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Sep 14, 2018
@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: justinsb, ripta

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 Sep 14, 2018
@ripta
Copy link
Contributor Author

ripta commented Sep 14, 2018

Thanks for taking a look and for the explanation, @justinsb.

Let me know if you want me to do that version bump (I can open a separate PR) and double-check to make sure the manifests are up-to-date.

@justinsb justinsb removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Sep 21, 2018
@k8s-ci-robot k8s-ci-robot merged commit 403a22a into kubernetes:master Sep 21, 2018
@ripta ripta deleted the avpc-k8s-1.8 branch September 22, 2018 01:15
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
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/L Denotes a PR that changes 100-499 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants