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

Adding a module to allow organizations to track usage of Jenkins within their domain #1301

Merged
merged 1 commit into from Feb 24, 2016

Conversation

@kohsuke
Copy link
Member

kohsuke commented Jun 27, 2014

There is a privacy concern in having this information reported, but OTOH in a corporate network this seems like a reasonable compromise, and people really often do not have any clues where they are running.

…in their domain.

There is a privacy concern in having this information reported, but OTOH
in a corporate network this seems like a reasonable compromise, and
people really often do not have any clues where they are running.
@cloudbees-pull-request-builder
Copy link

cloudbees-pull-request-builder commented Jun 27, 2014

core » jenkins-core #950 FAILURE
Looks like there's a problem with this pull request

@daniel-beck
Copy link
Member

daniel-beck commented Jul 11, 2014

Maybe add a System property option to disable it to be safe? Or would that defeat the purpose? I mean, I could just block this in the firewall if I wanted…

kohsuke added a commit that referenced this pull request Feb 24, 2016
Adding a module to allow organizations to track usage of Jenkins within their domain
@kohsuke kohsuke merged commit f6a8a6c into master Feb 24, 2016
@KostyaSha

This comment has been minimized.

Copy link
Member

KostyaSha commented on 358b9ce Feb 24, 2016

What jira issues explains this request?

@KostyaSha
Copy link
Member

KostyaSha commented Feb 24, 2016

Why PR that has no approved signs and has needs-review was merged?

@KostyaSha
Copy link
Member

KostyaSha commented Feb 24, 2016

Where is @reviewbybees ?

@orrc
Copy link
Member

orrc commented Feb 24, 2016

What jira issues explains this request?

The PR description explains it.

Why PR that has no approved signs and has needs-review was merged?

Presumably because it's small, has tests, and sat here for 18 months without objections?

Where is @reviewbybees ?

It didn't exist when this PR was created. But it wouldn't have hurt to use it anyway, I guess.

The feature has been documented (and I added a few more details to the wiki page), but I do agree with Daniel that — like the other Jenkins discovery mechanisms — it should also have a system property feature flag.

A changelog entry would also be helpful.

@rbywater
Copy link
Member

rbywater commented Feb 24, 2016

👍 to having a feature flag and personally (not knowing what the other mechanisms do) have it turned off by default.

@KostyaSha
Copy link
Member

KostyaSha commented Feb 24, 2016

@orrc @kohsuke is cloudbees employee (right?) and should use @reviewbybees like all other employees because it was already discussed that CB has rule for doing reviews for all their changes?

Presumably because it's small, has tests, and sat here for 18 months without objections?

@orrc by this logic many PRs should be merged even if they are bad.
I can also mention that people usually replying to their PRs to get attention.

it should also have a system property feature flag.

cc @stephenc ;)

@stephenc
Copy link
Member

stephenc commented Feb 25, 2016

@KostyaSha for the Nth time. At least all the employees that joined CloudBees up to and including my joining date have a very clear delineation between work done for the company and work done for your own enjoyment. (You'd need to ask newer employees what the current wording is though... Mine is very clear and specific)

As such, when "off the clock" we are not required to use the reviewed by bees handle...

In any case, the whole use of reviewed by bees is an internal policy we have chosen for ourselves. We do not ask anyone to police our internal policies.

We could keep the internal reviews private, but we believe that there is a reasonable likelihood of at least a third of our internal reviews either addressing comments that others would ask, or otherwise explain our thinking...

Additionally, if a proposed change is completely unwanted, we want maintainers to be able to reject early, rather than having to reject a fully polished unwanted change that has undergone extensive review already.

Now whether the Jenkins community's code review team should have been given an chance to review before merging us a completely different - and in my view - much more legitimate question.

@KostyaSha
Copy link
Member

KostyaSha commented Feb 25, 2016

@stephenc no idea to what else i can appeal to. Could you provide any technical comments about this change?

and sat here for 18 months without objections?

review was at least started by @daniel-beck with his question and it wasn't answered .

IMHO such PR decreases jenkins QA.
👎

@stephenc
Copy link
Member

stephenc commented Feb 25, 2016

@KostyaSha I will repeat what I said earlier

Now whether the Jenkins community's code review team should have been given an chance to review before merging us a completely different - and in my view - much more legitimate question.

@stephenc
Copy link
Member

stephenc commented Feb 25, 2016

I see no technical reason not to merge this functionality.

I have technical concerns about the initial implementation of the functionality

I would prefer to see this discovery mediated by DNS SVR records rather than a "magic" hostname of discover-jenkins.... ok if there is no DNS SVR records then fall back to such a "magic" hostname mechanism... but privacy concerns would be mitigated if this used DNS SVR records and any organization that has this need should have enough control of their DNS infrastructure that they should be able to define some DNS SVR record e.g. _jenkins-discovery._tcp.example.com. 86400 IN SRV 20 0 8080 jenkins-inventory.example.com.

But that is more an issue with the existing module implementation and not an issue with enabling the feature per-se

@KostyaSha
Copy link
Member

KostyaSha commented Feb 25, 2016

@stephenc Thanks for answer. Why people can't install this plugin separately and it should be in default war?

@jtnord
Copy link
Member

jtnord commented Feb 25, 2016

@KostyaSha

why people can't install this plugin separately and it should be in default war?

a plugin is opt-in rather than opt-out. For the use case where @kohsuke has targeted this opt-in is not suitable; think a large big faceless corp, where random devs are spinning up new instances in random labs (in different network segments) - and these random devs wouldn't install this plugin - they probably don't even know it exists - so the reporting fails to fulfill the needs).
Having said that big faceless corp should already be able to do this as they would be able to control the routing by force and use multicast discovery. If they can't control the routing into labs - then the method that this uses is also likely to fail.

That said in 2.0 we are secure out of the box (JENKINS-30749) so I think @kzantow needs to disable this feature by default :)

@kohsuke
Copy link
Member Author

kohsuke commented Feb 25, 2016

OK, so the follow-up changes are to switch to SRV record and then add a system property to disable this.

@jtnord explains well the targeted situation this feature is for, and I've never managed to sell DNS multicast as a solution; people who deal with Jenkins in those big companies aren't network experts, and they are unlikely to have a leverage to influence routing of the entire corporate internal network. Adding DNS entry is something much more routine and easier.

jglick added a commit that referenced this pull request Feb 25, 2016
The root problem was missing <scope>test</scope> in domain-discovery.
@jglick
Copy link
Member

jglick commented Feb 25, 2016

Reverted this PR since it was uncompilable. Could be redone more carefully and pushed to the same branch to reopen.

@rtyler
Copy link
Member

rtyler commented Feb 25, 2016

FWIW, I really like @stephenc's suggestion:

I would prefer to see this discovery mediated by DNS SVR records rather than a "magic" hostname of discover-jenkins.... ok if there is no DNS SVR records then fall back to such a "magic" hostname mechanism... but privacy concerns would be mitigated if this used DNS SVR records and any organization that has this need should have enough control of their DNS infrastructure that they should be able to define some DNS SVR record e.g. _jenkins-discovery._tcp.example.com. 86400 IN SRV 20 0 8080 jenkins-inventory.example.com.

This would give me "as a sysadmin" a lot more control over this and hell if I can get a a full URL in that SRV record I could just have this ping an existing inventory service without much trouble.

@daniel-beck daniel-beck deleted the domain-discovery branch Sep 21, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

You can’t perform that action at this time.