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

Serve robots.txt for Prow #4365

Merged
merged 11 commits into from
Nov 15, 2021
Merged

Conversation

ammarlakis
Copy link
Contributor

@ammarlakis ammarlakis commented Oct 26, 2021

Description

Changes proposed in this pull request:

  • New deployment of an Nginx reverse proxy that can be configured through a ConfigMap. One caveat is that we need a deployment reloader to restart nginx when the ConfigMap changes.

Other approaches considered:

Related issue(s)

@ammarlakis ammarlakis requested a review from a team as a code owner October 26, 2021 10:02
@kyma-bot kyma-bot added the size/M Denotes a PR that changes 30-99 lines, ignoring generated files. label Oct 26, 2021
@ammarlakis
Copy link
Contributor Author

/hold

@kyma-bot kyma-bot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Oct 26, 2021
Copy link
Contributor

@dekiel dekiel left a comment

Choose a reason for hiding this comment

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

Do we need to host robts.txt on a gcp bucket and proxy request there? We can simply mount this file in to nginx container and treat nginx as ordinary http server not revers proxy? Without proxying we can store robots.txt in our test-infra repo and have it versioned and deployed along with other prow components and configuration. robots.txt content can be provided as config map.

@Ressetkk
Copy link
Contributor

Ressetkk commented Oct 26, 2021

Do we need to host robts.txt on a gcp bucket and proxy request there? We can simply mount this file in to nginx container and treat nginx as ordinary http server not revers proxy? Without proxying we can store robots.txt in our test-infra repo and have it versioned and deployed along with other prow components and configuration. robots.txt content can be provided as config map.

It'd also need to populate some kind of ConfigMap reloader which would restart the instance once the configmap changes.

Maybe instead of using nginx the better approach would be writing simple agent based on net/http which hosts the robots.txt file and watches the changes on the ConfigMap. If it updates then the agent will update and host new files. This way the robots.txt file could reside on the repo and config-updated would update the ConfigMap.

@dekiel
Copy link
Contributor

dekiel commented Oct 26, 2021

Do we need to host robts.txt on a gcp bucket and proxy request there? We can simply mount this file in to nginx container and treat nginx as ordinary http server not revers proxy? Without proxying we can store robots.txt in our test-infra repo and have it versioned and deployed along with other prow components and configuration. robots.txt content can be provided as config map.

It'd also need to populate some kind of ConfigMap reloader which would restart the instance once the configmap changes

I don't think this file will change often. But simple k8s command implemented to run as postsubmit when file change will do the trick.
restart the containers in the deployment
kubectl rollout restart deployment wiki-deployment

This could be added eventualy as extension configreloader.

@Ressetkk
Copy link
Contributor

Ressetkk commented Oct 26, 2021

If creating simple ConfigMap watcher will be enough then I see no sense in introducing another ProwJob.

@dekiel
Copy link
Contributor

dekiel commented Oct 26, 2021

If creating simple ConfigMap watcher will be enough then I see no sense in introducing another ProwJob.

Right, @Ressetkk could you please share why introducing simple ConfigMap watcher has more sense than another ProwJob?

@Ressetkk
Copy link
Contributor

If creating simple ConfigMap watcher will be enough then I see no sense in introducing another ProwJob.

Right, @Ressetkk could you please share why introducing simple ConfigMap watcher has more sense than another ProwJob?

Of course:

  1. Local observability - config changes are traced in cluster
  2. No requests are sent to GitHub when the file is changed - no statuses are reported to GitHub so it does not affect rate-limits.
  3. Prow-independent - the approach will be universal and can be used with any workload on any k8s cluster

@ammarlakis
Copy link
Contributor Author

I agree that using ConfigMap is a simpler approach but I avoided it so we won't need a ConfigMap reloader.
I'll update the PR with the robots.txt as a ConfigMap and deploy Reloader.

@Ressetkk
Copy link
Contributor

I would go with writing simple go server with the ConfigMap watch capability instead of external reloader

@Ressetkk
Copy link
Contributor

One last remark - k8s automatically updates the mounted configmaps as volumes, so if robots.txt is updated in cluster then the Mounted ConfigMap files will also update. With that you can go with simple server that hosts only robots.txt file and the ConfigMap mounted as a volume without thinking about reloading the ConfigMap.

@dekiel
Copy link
Contributor

dekiel commented Oct 26, 2021

If creating simple ConfigMap watcher will be enough then I see no sense in introducing another ProwJob.

Right, @Ressetkk could you please share why introducing simple ConfigMap watcher has more sense than another ProwJob?

Of course:

  1. Local observability - config changes are traced in cluster
  2. No requests are sent to GitHub when the file is changed - no statuses are reported to GitHub so it does not affect rate-limits.
  3. Prow-independent - the approach will be universal and can be used with any workload on any k8s cluster
  1. How we will use this local observability? Will me monitor this somehow? Trace this requests? Measure it in any way?
  2. Reporting to github is not mandatory, it can be disabled. However file will be updated very, very rarely and will not impact our rate-limits.
  3. Do we have need for such feature in other workloads? I think there is no such task in our backlog.

We don't need to write anything new for this task. All pieces we need are in place.

@ammarlakis please check:

config_updater:

https://github.com/kubernetes/test-infra/tree/master/prow/plugins/updateconfig

This plugin should do the job for updating config-map in a cluster. With last comment from Kamil your done. You don't need anything else.

@ammarlakis
Copy link
Contributor Author

@Ressetkk I see the minimalistic approach you propose, but it does not bring much benefit considering the cost of maintaining code (even a simple one), and the cost of losing the features nginx provides and we may use in the future, like reverse proxy or hosting other static files with complicated path rules or response manipulation.
@dekiel I don't understand how config updater would do the job here. Correct me if I'm wrong, it doesn't restart the pod that uses the ConfigMap, right? it only applies the ConfigMap to the cluster (and we already do that with applying files in the components directory).

@ammarlakis
Copy link
Contributor Author

I missed the point that we were talking about updating the robots.txt ConfigMap and was thinking about updating the nginx configuration ConfigMap. I'll update the PR according to your comments.

@ammarlakis
Copy link
Contributor Author

/unhold

@kyma-bot kyma-bot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Oct 27, 2021
@ammarlakis ammarlakis force-pushed the prow-robotstxt branch 2 times, most recently from b795a8e to b1478b3 Compare October 27, 2021 11:42
Copy link
Contributor

@dekiel dekiel left a comment

Choose a reason for hiding this comment

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

Can we change directory name from static_files to static-files as this is rather convention used in prow directory?
Static-files should be placed in a directory like web-server-content to indicate purpose/destination of this static files.
Please add some short README.md with explanation why we have this robots.txt, how and where it's used. How to update it.
Why we need empty index.html? Nginx requires it to start? If yes, describe it in README.md

prow/cluster/components/tls-ing_ingress.yaml Outdated Show resolved Hide resolved
@ammarlakis
Copy link
Contributor Author

/test pre-main-test-infra-validate-image-url-helper

prow/README.md Outdated Show resolved Hide resolved
Copy link
Contributor

@dekiel dekiel left a comment

Choose a reason for hiding this comment

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

Please extend operational documentation how to work with this component.

@ammarlakis ammarlakis requested a review from dekiel November 9, 2021 13:57
Copy link
Contributor

@dekiel dekiel left a comment

Choose a reason for hiding this comment

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

Please check my suggestion.

prow/cluster/README.md Outdated Show resolved Hide resolved
Co-authored-by: Przemek Pokrywka <[email protected]>
@dekiel
Copy link
Contributor

dekiel commented Nov 9, 2021

/test pre-test-infra-validate-scripts

dekiel
dekiel previously approved these changes Nov 9, 2021
@kyma-bot kyma-bot added the lgtm Looks good to me! label Nov 9, 2021
Co-authored-by: Aleksandra Simeonova <[email protected]>
@kyma-bot kyma-bot removed the lgtm Looks good to me! label Nov 15, 2021
@kyma-bot kyma-bot added the lgtm Looks good to me! label Nov 15, 2021
@kyma-bot kyma-bot merged commit 94837ee into kyma-project:main Nov 15, 2021
@kyma-bot
Copy link
Contributor

@ammarlakis: Updated the plugins configmap in namespace default at cluster default using the following files:

  • key plugins.yaml using file prow/plugins.yaml

In response to this:

Description

Changes proposed in this pull request:

  • New deployment of an Nginx reverse proxy that can be configured through a ConfigMap. One caveat is that we need a deployment reloader to restart nginx when the ConfigMap changes.

Other approaches considered:

Related issue(s)

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
lgtm Looks good to me! size/M Denotes a PR that changes 30-99 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Some logs are being crawled by google
5 participants