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.

Sign up for GitHub

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

Jump to bottom

Remove ro service #8155

Merged
merged 1 commit into from
Jun 4, 2015
Merged

Remove ro service #8155

merged 1 commit into from
Jun 4, 2015
+32 −155

Conversation

lavalamp
Copy link
Member

Fixes #4567
Fixes #7963

@lavalamp lavalamp force-pushed the no-ro branch 2 times, most recently from 0fffc10 to 9182b08 Compare May 14, 2015 21:13
erictune
erictune reviewed May 19, 2015
View reviewed changes
docs/accessing_the_api.md
@@ -59,8 +54,7 @@ variety of uses cases:

## Expected changes
- Policy will limit the actions kubelets can do via the authed port.
- Kube-proxy currently uses the readonly port to read services and endpoints,
but will eventually use the auth port.
- Kube-proxy uses the authenticated port.
Copy link
Member

Choose a reason for hiding this comment

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

this change is complete.

Copy link
Member Author

Choose a reason for hiding this comment

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

Cool, fixed.

@erictune
Copy link
Member

LGTM, but title says WIP

@lavalamp lavalamp force-pushed the no-ro branch 2 times, most recently from f79002b to 31b2152 Compare May 29, 2015 00:54
@lavalamp
Copy link
Member Author

I think I have e2e passing now.

Work items I still have to do before we merge this:

  1. revisit my kube2sky change in this PR-- may not be necssary
  2. Add a ClientFromKubernetesEnv function or something to the client package that'll automatically get you a client from the default service token.
  3. Make kubectl do the same.

@erictune I'm thinking about punting on 3, there's no good way to test it yet. I want to run a registry on the cluster and push stuff to it and use pull secrets, but it'll require a lot of work in our build scripts to get it going. I don't know if we want to block on it. Unfortunately, this means there'd be no solution for things like the prometheus example, for which there's no obvious way to plug authentication in.

I suppose I could make it work and worry about testing later when we get registries in...

@lavalamp
Copy link
Member Author

I just removed the kube2sky change; I don't think it's necessary anyway.

@roberthbailey
Copy link
Contributor

Add a ClientFromKubernetesEnv function ...

Just to be clear, this isn't going to suck a token in from the KubeEnv in the metadata server, but will grab it from a service account, right?

@lavalamp
Copy link
Member Author

Just to be clear, this isn't going to suck a token in from the KubeEnv in the metadata server, but will grab it from a service account, right?

Yes, it'll use the token from the service accounts, and later we'll make it also check the master's certs.

@lavalamp
Copy link
Member Author

So the only thing left here is making kubectl work from a container. Still figuring out that long list of config merging thingies.

@erictune erictune added this to the v1.0 milestone Jun 1, 2015
@lavalamp lavalamp mentioned this pull request Jun 1, 2015
Closed
@erictune erictune mentioned this pull request Jun 1, 2015
Closed
@lavalamp lavalamp changed the title WIP: Remove ro service Remove ro service Jun 2, 2015
@lavalamp
Copy link
Member Author

lavalamp commented Jun 2, 2015

OK, I have kubectl running in a container. I'm going to run e2e again to make sure it (still) works.

@lavalamp
Copy link
Member Author

lavalamp commented Jun 2, 2015

Looks like heapster is broken with this, so I should fix that before we merge.

This was referenced Jun 2, 2015
Merged
Merged
@lavalamp
Copy link
Member Author

lavalamp commented Jun 2, 2015

Heapster was not broken-- I got bit by the DNS problem. I've now got every e2e test working with this.

@lavalamp lavalamp force-pushed the no-ro branch 2 times, most recently from c14ab32 to dd3caa9 Compare June 3, 2015 01:42
@lavalamp
Copy link
Member Author

lavalamp commented Jun 3, 2015

rebased

@goltermann goltermann removed the feature label Jun 3, 2015
@lavalamp
Copy link
Member Author

lavalamp commented Jun 3, 2015

This should be ready to go now.

erictune
erictune reviewed Jun 3, 2015
View reviewed changes
cluster/addons/fluentd-elasticsearch/es-image/elasticsearch_logging_discovery.go
}

c, err := client.New(config)
c, err := client.NewInCluster()
Copy link
Member

Choose a reason for hiding this comment

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

Fine for this PR, but I don't think we want to encourage a pattern where a program has to be in a cluster to run. Should be some way to fall back to kubeconfig from a file, such as when doing ad-hoc testing.

Copy link
Member Author

Choose a reason for hiding this comment

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

Yeah-- in another commit I have changes to kubectl code; people that use the same code path should be covered, but that is a very confusing bit of excessive overriding.

But this bit of code has essentially no usefulness outside of a cluster, so I think it's OK.

@satnam6502 you may want to glance at this.

Copy link
Contributor

Choose a reason for hiding this comment

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

Right, this code is designed to only run in a cluster. What's wrong with that? I thought this would be a not totally uncommon pattern?

@erictune
Copy link
Member

erictune commented Jun 3, 2015

LGTM

@erictune erictune added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Jun 3, 2015
@lavalamp
Copy link
Member Author

lavalamp commented Jun 3, 2015

All commits except first moved to #9211.

@bgrant0607 bgrant0607 mentioned this pull request Jun 3, 2015
Merged
@erictune
Copy link
Member

erictune commented Jun 3, 2015

still lgtm

bgrant0607 added a commit that referenced this pull request Jun 3, 2015
@bgrant0607
Merge pull request #9211 from lavalamp/no-ro-nonbreaking
f8bf996 
Nonbreaking pieces of #8155
@lavalamp
Copy link
Member Author

lavalamp commented Jun 3, 2015

Rebased & ready to go

@bgrant0607
Copy link
Member

Waiting on Shippable.

@bgrant0607
Copy link
Member

And e2e, if you'd like #8155 to not be rolled back in the case that this breaks e2e

@lavalamp
Copy link
Member Author

lavalamp commented Jun 3, 2015

Yes absolutely wait for e2e :)

@bgrant0607
Copy link
Member

#8155 passed e2e, but I need to merge another rollback PR first.

@bgrant0607
Copy link
Member

Sorry, I meant #9211 passed.

Tracking down another e2e failure at the moment - #9208.

bgrant0607 added a commit that referenced this pull request Jun 4, 2015
@bgrant0607
Merge pull request #8155 from lavalamp/no-ro
a5959d7 
Remove ro service
@bgrant0607 bgrant0607 merged commit a5959d7 into kubernetes:master Jun 4, 2015
@erictune erictune mentioned this pull request Jun 5, 2015
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees

@erictune erictune

Labels
lgtm "Looks good to me", indicates that a PR is ready to be merged.
Projects
None yet
Milestone
v1.0
Development

Successfully merging this pull request may close these issues.

The apiserver read-only API leaks bearer tokens Deprecate kubernetes-ro service
7 participants
@lavalamp @erictune @roberthbailey @bgrant0607 @satnam6502 @googlebot @goltermann