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

GCE: Gracefully handle permission errors when attempting to create firewall rules #51562

Merged
merged 1 commit into from Sep 5, 2017

Conversation

nicksardo
Copy link
Contributor

@nicksardo nicksardo commented Aug 29, 2017

Purpose of this PR is to raise events from the GCE cloud provider if the GCE service account does not have the permissions necessary to create/update/delete firewall rules.

Fixes #51812

Release note:

NONE

Example Events:

Events:
  FirstSeen     LastSeen        Count   From                    SubObjectPath   Type            Reason                          Message
  ---------     --------        -----   ----                    -------------   --------        ------                          -------
  2m            2m              1       service-controller                      Normal          EnsuringLoadBalancer            Ensuring load balancer
  2m            2m              1       gce-cloudprovider                       Normal          LoadBalancerManualChange        Firewall change required by network admin: `gcloud compute firewall-rules create aa8a1dd628ddb11e78ce042010a80000 --network https://www.googleapis.com/compute/v1/projects/playground/global/networks/e2e-test-nicksardo --description "{\"kubernetes.io/service-name\":\"default/myechosvc1\", \"kubernetes.io/service-ip\":\"\"}" --allow tcp:9000 --source-ranges 0.0.0.0/0 --target-tags e2e-test-nicksardo-minion --project playground`
  2m            2m              1       gce-cloudprovider                       Normal          LoadBalancerManualChange        Firewall change required by network admin: `gcloud compute firewall-rules create k8s-1aee5045e658d174-node-hc --network https://www.googleapis.com/compute/v1/projects/playground/global/networks/e2e-test-nicksardo --description "" --allow tcp:10256 --source-ranges 130.211.0.0/22,35.191.0.0/16,209.85.152.0/22,209.85.204.0/22 --target-tags e2e-test-nicksardo-minion --project playground`
  1m            1m              1       service-controller                      Normal          EnsuredLoadBalancer             Ensured load balancer

@k8s-ci-robot k8s-ci-robot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. labels Aug 29, 2017
@freehan freehan added this to the v1.8 milestone Aug 29, 2017
@nicksardo
Copy link
Contributor Author

/assign @nicksardo

@k8s-github-robot k8s-github-robot added needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. release-note-none Denotes a PR that doesn't merit a release note. labels Aug 29, 2017
@nicksardo nicksardo force-pushed the gce-attempt-firewall branch 2 times, most recently from 3df39ae to f9eed82 Compare August 29, 2017 23:03
@k8s-github-robot k8s-github-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Aug 29, 2017
@nicksardo nicksardo force-pushed the gce-attempt-firewall branch 2 times, most recently from 0cb7627 to c330c05 Compare August 31, 2017 18:39
@k8s-github-robot k8s-github-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Sep 1, 2017
@k8s-github-robot k8s-github-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Sep 1, 2017
@bowei
Copy link
Member

bowei commented Sep 1, 2017

Testing? also does this qualify as a bugfix?

@k8s-github-robot k8s-github-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Sep 1, 2017
@nicksardo
Copy link
Contributor Author

nicksardo commented Sep 1, 2017

Existing testing will ensure that this PR doesn't break service creation and deletion. However, we cannot E2E test the desired behavior until we have a test suite using xpn - I've only done manual testing. That's a larger effort which won't make 1.8.0 and also depends on #51739.

@nicksardo nicksardo force-pushed the gce-attempt-firewall branch 4 times, most recently from f94b9fa to 020b844 Compare September 2, 2017 00:16
@bowei
Copy link
Member

bowei commented Sep 2, 2017

/assign @bowei

@bowei
Copy link
Member

bowei commented Sep 2, 2017

/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 2, 2017
@fejta-bot
Copy link

/retest
This bot automatically retries jobs that failed/flaked on approved PRs (send feedback to @fejta).

Review the full test history for this PR.

3 similar comments
@fejta-bot
Copy link

/retest
This bot automatically retries jobs that failed/flaked on approved PRs (send feedback to @fejta).

Review the full test history for this PR.

@fejta-bot
Copy link

/retest
This bot automatically retries jobs that failed/flaked on approved PRs (send feedback to @fejta).

Review the full test history for this PR.

@fejta-bot
Copy link

/retest
This bot automatically retries jobs that failed/flaked on approved PRs (send feedback to @fejta).

Review the full test history for this PR.

@fejta-bot
Copy link

/retest
This bot automatically retries jobs that failed/flaked on approved PRs (send feedback to @fejta).

Review the full test history for this PR.

2 similar comments
@fejta-bot
Copy link

/retest
This bot automatically retries jobs that failed/flaked on approved PRs (send feedback to @fejta).

Review the full test history for this PR.

@fejta-bot
Copy link

/retest
This bot automatically retries jobs that failed/flaked on approved PRs (send feedback to @fejta).

Review the full test history for this PR.

@cblecker
Copy link
Member

cblecker commented Sep 4, 2017

/lgtm cancel
Removing LGTM to halt re-testing loop. This doesn't build:

W0904 03:05:41.405] # k8s.io/kubernetes/pkg/cloudprovider/providers/gce
W0904 03:05:41.406] pkg/cloudprovider/providers/gce/gce_util.go:203: isForbidden redeclared in this block
W0904 03:05:41.407] 	previous declaration at pkg/cloudprovider/providers/gce/gce_util.go:191

@k8s-ci-robot k8s-ci-robot removed the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Sep 4, 2017
@nicksardo
Copy link
Contributor Author

Thanks @cblecker

76945ad#diff-513dec35d48c4eca1f8e84c165571af7 merged with the same function after code freeze. I've rebased and removed the duplicate.
/cc @bowei

@dims
Copy link
Member

dims commented Sep 5, 2017

re-applying lgtm after rebase, original lgtm was from @bowei and the problem pointed to by @cblecker was fixed as well

/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 5, 2017
@k8s-github-robot
Copy link

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: bowei, dims, nicksardo

Associated issue: 51812

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

Needs approval from an approver in each of these OWNERS Files:

You can indicate your approval by writing /approve in a comment
You can cancel your approval by writing /approve cancel in a comment

@fejta-bot
Copy link

/retest
This bot automatically retries jobs that failed/flaked on approved PRs (send feedback to @fejta).

Review the full test history for this PR.

@k8s-github-robot
Copy link

Automatic merge from submit-queue (batch tested with PRs 51915, 51294, 51562, 51911)

@k8s-github-robot k8s-github-robot merged commit 1732a8b into kubernetes:master Sep 5, 2017
@k8s-ci-robot
Copy link
Contributor

@nicksardo: The following test failed, say /retest to rerun them all:

Test name Commit Details Rerun command
pull-kubernetes-e2e-kops-aws 676b95e link /test pull-kubernetes-e2e-kops-aws

Full PR test history. Your PR dashboard. Please help us cut down on flakes by linking to an open issue when you hit one in your PR.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. I understand the commands that are listed here.

@nicksardo nicksardo deleted the gce-attempt-firewall branch January 12, 2018 07:00
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. release-note-none Denotes a PR that doesn't merit a release note. 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

8 participants