Skip to content

Conversation

jhosteny
Copy link
Contributor

@jhosteny jhosteny commented Mar 16, 2020

This change reworks the prior fix in #5252, by providing a flag for ATC
to set the default badge visibility. The default setting for
default-badge-visibility is for it to maintain backwards compatible
behavior (pipeline-visibility), while the public setting makes all
pipeline and job badge routes public by default.

Fixes #996.

Signed-off-by Joe Hosteny jhosteny@carnegierobotics.com

Existing Issue

Fixes #996.

Why do we need this PR?

PR #5252 contained an initial implementation for this functionality, but it did so in a backwards incompatible manner. This fix keeps the default behavior, but allows it to be overridden with a new ATC flag.

Changes proposed in this pull request

  • Add the new flag default-badge-visibility with a default setting of pipeline-visibility

Contributor Checklist

Reviewer Checklist

This section is intended for the core maintainers only, to track review progress. Please do not
fill out this section.

  • Code reviewed
  • Tests reviewed
  • Documentation reviewed
  • Release notes reviewed
  • PR acceptance performed
  • New config flags added? Ensure that they are added to the BOSH
    and Helm packaging; otherwise, ignored for the integration tests (for example, if they are Garden configs that are not displayed in the --help text).

@jhosteny jhosteny requested a review from a team March 16, 2020 14:53
@jhosteny
Copy link
Contributor Author

jhosteny commented Mar 16, 2020

@pivotal-jamie-klassen here is the initial cut at the ATC flag implementation for the badges. The DefaultBadgeVisibilityFactory code is perhaps a bit overly pedantic, but it seemed to make testing easier. I also have a change to the bosh / helm repos if you would like. Release notes to follow shortly.

This change reworks the prior fix in concourse#5252, by providing a flag for ATC
to set the default badge visibility. The default setting for
`default-badge-visibility` is for it to maintain backwards compatible
behavior (`pipeline-visibility`), while the `public` setting makes all
pipeline and job badge routes public by default.

Fixes concourse#996.

Signed-off-by: Joseph Hosteny <jhosteny@carnegierobotics.com>
@jhosteny jhosteny force-pushed the feat/badge-visibility branch from 66fc392 to c7eb3b7 Compare March 16, 2020 14:56
Signed-off-by: Joseph Hosteny <jhosteny@carnegierobotics.com>
@vito vito added this to the v6.0.0 milestone Mar 16, 2020
@YoussB YoussB assigned YoussB and jamieklassen and unassigned YoussB and jamieklassen Mar 17, 2020
@vito
Copy link
Member

vito commented Mar 17, 2020

@jhosteny Sorry, but I bring bad news: in order to get 6.0 out the door I'm going to revert your original PR (#5252) for now. In talking with the team we've started thinking a bit more about this problem space and we'd like to continue developing the idea, possibly in a different direction from this PR, so that it will make more people in the community happy.

Since that'll take a bit more time and since 6.0 has already been delayed for a while I'd like to just revert the other change for now to de-risk the release a bit. Sorry to have strung you along. 🙁

So, here's what we're thinking: if we make these status endpoints (both badge and cc.xml) opt-in through a command like fly publish-status we can then have specific API endpoints which are always publicly readable without having to reason about team roles and permissions and preferences at all, because by their very nature they are meant to be able to be fetched without auth, and users opt-in by creating them.

Something like this:

$ fly publish-status --team some-team
badge: https://ci.example.com/api/v1/status/<guid>/badge
cc.xml: https://ci.example.com/api/v1/status/<guid>/cc.xml

$ fly publish-status --pipeline my-pipeline
badge: https://ci.example.com/api/v1/status/<guid>/badge
cc.xml: https://ci.example.com/api/v1/status/<guid>/cc.xml

$ fly publish-status --job my-pipeline/my-job
badge: https://ci.example.com/api/v1/status/<guid>/badge
cc.xml: https://ci.example.com/api/v1/status/<guid>/cc.xml

...and I haven't really thought further than that. 😅 There may be some way to delete and list them.

This would accomplish the following goals:

  • supporting multiple levels of status summarization: per-job, per-pipeline, per-team
  • making cc.xml and badges always publicly viewable, as they've been opted-in to and now use anonymized URLs

I'm leaning towards this approach because it will allow for greater flexibility while not having to make the decision cluster-wide. We really want to limit the amount of globally-configured flags because many instances of Concourse are multi-tenant, and while one team may want their badges publicly exposed, other teams may have security concerns that prohibit it.

I don't meant to expand the scope on you, but since this area has been a recurring pain point for our users I'd like to take the opportunity to discuss it more in-depth. We'd be happy to help guide along these changes or if you feel that the scope has increased beyond what you have time for, we can try to put together an RFC so that someone in the community may volunteer. Let me know what you think! 🙏

@jhosteny
Copy link
Contributor Author

@vito no worries, I understand.

I think this probably needs some community feedback. My first reaction is that the GUID is slightly inconvenient. The badge URL is not self-describing, which means that I need to publish the status before I can update a README, for example, and I would invalidate a consumer of the endpoint if I unpublished / republished. Perhaps you could have a deactivate instead of delete to handle this latter case, and re-use the GUID.

One other thing to consider is that you'd have to still maintain the old badge routes. Would the existing pipeline / job badge route functionality get extracted to new status servers, that the old routes would redirect to if public or authenticated / authorized, and the GUID based routes would redirect to dynamically?

My personal preference would be setting an option on the pipeline or job configuration that made it explicit that the badge is public, but I understand this is at odds with the CLI for control of pipeline visibility. Or, alternatively, the badge authentication route handler could check the DB to see if it had been exposed, so the route is still the same and there is no global flag (a DB migration would set the default scope for backwards compatibility).

Regardless, I'd be willing to help out on this, but I expect that it would take a fair bit longer to implement given that I'm pretty new to the codebase.

@vito
Copy link
Member

vito commented Mar 17, 2020

Thanks for understanding. 🙂

I think this probably needs some community feedback. My first reaction is that the GUID is slightly inconvenient. The badge URL is not self-describing, which means that I need to publish the status before I can update a README, for example, and I would invalidate a consumer of the endpoint if I unpublished / republished. Perhaps you could have a deactivate instead of delete to handle this latter case, and re-use the GUID.

Hm yeah true. Either way though there's some sort of operation you have to do prior to updating the README; if it's not creating a badge, it'd be enabling the badge endpoint. But you're right that a GUID in the URL would require you to do so before committing the change.

One advantage of having an absolute, anonymous URL is that the URL could survive pipeline/job renaming and pipeline ownership-changing (assuming we implement fly chown-pipeline at some point) - so you wouldn't have to update your README in that case.

I guess it ultimately comes down to which event happens more often: renaming/moving pipelines/jobs or deleting badges. My gut tells me the former, since badges are relatively set-and-forget, but pipelines and jobs tend to evolve and sometimes need to be reorganized or renamed.

But it is true that we would need to be conscious of operations which would invalidate these URLs. So we would at least need a way to update a status's configuration in-place instead of deleting and re-creating.

One other thing to consider is that you'd have to still maintain the old badge routes. Would the existing pipeline / job badge route functionality get extracted to new status servers, that the old routes would redirect to if public or authenticated / authorized, and the GUID based routes would redirect to dynamically?

True. Yeah, that sounds like it'd be a reasonable approach. I think they could both ultimately end up going down the same code path. The GUID based routes could load the pipeline/job status based on the badge's config in the database, and the legacy routes would just get them from the URL. And then they both could call the same bit of code for actually returning the response.

My personal preference would be setting an option on the pipeline or job configuration that made it explicit that the badge is public, but I understand this is at odds with the CLI for control of pipeline visibility. Or, alternatively, the badge authentication route handler could check the DB to see if it had been exposed, so the route is still the same and there is no global flag (a DB migration would set the default scope for backwards compatibility).

This actually wouldn't be a bad idea either. We already have a notion of whether a job is public, so it's not just the CLI controlling the visibility of things. Now I'm curious: for jobs which you would want a badge for, would you also want that badge to be able to link to a build, and have the build output be publicly visible too? If so, then maybe we just need the badge endpoint to respect public: true?

To view the build you would need to run expose-pipeline anyway, which defeats the entire point of this conversation, but I'm still curious about the use case. 🙂

@jhosteny
Copy link
Contributor Author

This actually wouldn't be a bad idea either. We already have a notion of whether a job is public, so it's not just the CLI controlling the visibility of things. Now I'm curious: for jobs which you would want a badge for, would you also want that badge to be able to link to a build, and have the build output be publicly visible too? If so, then maybe we just need the badge endpoint to respect public: true?

To view the build you would need to run expose-pipeline anyway, which defeats the entire point of this conversation, but I'm still curious about the use case. 🙂

I think I intended that just the badge itself would be public, which means it would be a separate setting.

@clarafu
Copy link
Contributor

clarafu commented Apr 24, 2020

Hi @jhosteny! I like your idea of having this be a setting on the job/pipeline rather than as a global flag setting. It would be pretty similar to how you can make a job public through the public setting. https://concourse-ci.org/jobs.html#schema.job.public

One question I would have about this approach would be about making a private pipeline have public badges. If we make it so that you can make a private job have public badges through a setting in it's config, would we need a similar approach for making a private pipeline have public badges? Not sure how that would look, would it need a fly CLI for it similar to expose-pipeline?

I would be willing to help you through implementing this new approach with adding a new job configuration for public badges if you would be interested in it! If you are unsure about where to start within the codebase I can definitely help with that too. 😄

@jhosteny
Copy link
Contributor Author

Hi @clarafu, I'm sorry for the long delay in responding. I am definitely interested in helping on this, but I am a bit behind at work right now and haven't had the time to start. I also need some time to review the existing codebase to think about this added functionality will work.

Now that I am actually starting to move pipelines between teams, I better understand @vito's comments regarding fly chown-pipeline, and agree on the GUID based routes. If there has to be a fly command to create a pipeline badge namespace, there would have to be one to delete it, so I suppose you can add an update command as well? The pipeline-badge-create could take a setting for the visibility of default (whatever the pipeline visibility is), public or private, the latter two of which disregard the pipeline setting. The pipeline-badge-update would allow you to change this badge visibility setting.

The specific use case for us is that all pipelines / jobs are private by default. I want only the badge (no logs, or any other info) to be public, just so we can have a badge in the README on GH, for example. I think a perfectly reasonable first cut at this doesn't allow the jobs to override the pipeline setting - they get the same badge visibility setting as the pipeline badge.

@clarafu
Copy link
Contributor

clarafu commented May 19, 2020

@jhosteny Don't worry take all the time you need :) If you ever have any questions or get stuck on any part of the codebase, please don't hesitate to ask us!

I'm wondering if we should have the pipeline badge commands be similar to setting a pipeline config or our wall api endpoints? We typically have one set endpoint where it can both create and update, then one get where it returns what has already been configured and then one delete. Here is an example of how the wall endpoints look https://github.com/concourse/concourse/blob/master/atc/api/wallserver/wall.go.

This also seems like it can be a bigger discussion which can be in the format of an RFC. There are quite a few pieces here and it might be better to be able to clearly discuss how we are going to approach this. If you haven't taken a look at our existing RFCs before, they live here!

@clarafu
Copy link
Contributor

clarafu commented Jan 4, 2021

Hi @jhosteny! I'd like to close this PR for now because it has been almost one year of inactivity but feel free to reopen this PR or open a new one once you find time to tackle it! Thanks :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Feature request: public badges for private jobs
5 participants