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

DNS Controller Limitation #3330

Merged
merged 1 commit into from
Sep 22, 2017

Conversation

gambol99
Copy link
Contributor

@gambol99 gambol99 commented Sep 2, 2017

The current implementation does not place any limitation on the dns annontation which the dns-controller can consume. In a multi-tenented environment we have to ensure certain safe guards are met, so users can't by accident or intentionally alter our internal dns. Note; the current behaviour has not been changed;

  • added the --watch-namespace option to the dns controller and WatchNamespace to the spec
  • cleaned up area of the code where possible or related
  • fixed an vetting issues that i came across on the journey
  • renamed the dns-controller watcher files

@k8s-ci-robot k8s-ci-robot added cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Sep 2, 2017
@gambol99
Copy link
Contributor Author

gambol99 commented Sep 2, 2017

@KashifSaadat .. can i have a review please :-)

@gambol99 gambol99 force-pushed the limit_dns_controller branch from 589e6e0 to c52456b Compare September 2, 2017 16:54
Copy link
Contributor

@KashifSaadat KashifSaadat left a comment

Choose a reason for hiding this comment

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

LGTM

@KashifSaadat
Copy link
Contributor

/test pull-kops-e2e-kubernetes-aws

@chrislovecnm
Copy link
Contributor

/retest

1 similar comment
@chrislovecnm
Copy link
Contributor

/retest

@chrislovecnm
Copy link
Contributor

grumble grumble testing ...

/retest

failed downloading https://storage.googleapis.com/kubernetes-release-dev/ci R=v1.9.0-alpha.0.391+440884be9ae54a

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

@gambol99 PR needs rebase

@justinsb
Copy link
Member

/lgtm

Nice change, but needs a rebase it looks like.

I believe our tests are (finally) happy again :-)

@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, 2017
@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 14, 2017
@chrislovecnm
Copy link
Contributor

/retest

@chrislovecnm
Copy link
Contributor

@gambol99 probably need a rebase on this. E2E seems to be running tonight

@chrislovecnm
Copy link
Contributor

@gambol99 can we get a rebase?

The current implementation does not place any limitation on the dns annontation which the dns-controller can consume. In a multi-tenented environment was have to ensure certain safe guards are met, so users can't byt accident or intentionally alter our internal dns. Note; the current behaviour has not been changed;

- added the --watch-namespace option to the dns controller and WatchNamespace to the spec
- cleaned up area of the code where possible or related
- fixed an vetting issues that i came across on the journey
- renamed the dns-controller watcher files
@gambol99 gambol99 force-pushed the limit_dns_controller branch from c52456b to b647956 Compare September 22, 2017 10:59
@k8s-github-robot
Copy link

/lgtm cancel //PR changed after LGTM, removing LGTM. @gambol99 @justinsb

@k8s-github-robot k8s-github-robot removed lgtm "Looks good to me", indicates that a PR is ready to be merged. needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. labels Sep 22, 2017
@chrislovecnm
Copy link
Contributor

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

[APPROVALNOTIFIER] This PR is APPROVED

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

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

Needs approval from an approver in each of these OWNERS Files:
  • OWNERS [chrislovecnm,justinsb]

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

@k8s-github-robot
Copy link

/test all [submit-queue is verifying that this PR is safe to merge]

@k8s-github-robot
Copy link

Automatic merge from submit-queue. .

@k8s-github-robot k8s-github-robot merged commit e96c13b into kubernetes:master Sep 22, 2017
@gambol99 gambol99 deleted the limit_dns_controller branch February 22, 2019 09:58
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.

6 participants