Skip to content

Conversation

padlar
Copy link
Contributor

@padlar padlar commented Apr 20, 2023

fixes #5256

@padlar padlar requested a review from a team as a code owner April 20, 2023 13:25
@padlar padlar requested review from tsaarni and sunjayBhatia and removed request for a team April 20, 2023 13:25
@pnbrown
Copy link

pnbrown commented Apr 20, 2023

As discussed in person, this will need to wait for #5214, but can be lined up now by basing the branch off the other PR.

@codecov
Copy link

codecov bot commented Apr 20, 2023

Codecov Report

Merging #5302 (36fbbb4) into main (bdb8b92) will decrease coverage by 0.46%.
The diff coverage is 40.00%.

❗ Current head 36fbbb4 differs from pull request most recent head 60f9e35. Consider uploading reports for the commit 60f9e35 to get more accurate results

Additional details and impacted files

Impacted file tree graph

@@            Coverage Diff             @@
##             main    #5302      +/-   ##
==========================================
- Coverage   78.18%   77.73%   -0.46%     
==========================================
  Files         138      139       +1     
  Lines       18480    18078     -402     
==========================================
- Hits        14449    14053     -396     
+ Misses       3755     3752       -3     
+ Partials      276      273       -3     
Impacted Files Coverage Δ
internal/provisioner/model/model.go 92.30% <ø> (-0.10%) ⬇️
...ernal/provisioner/objects/deployment/deployment.go 88.28% <0.00%> (-1.21%) ⬇️
internal/provisioner/controller/gateway.go 61.93% <100.00%> (-0.12%) ⬇️

... and 26 files with indirect coverage changes

@padlar padlar force-pushed the gw-provisioner-optout-crds branch from 36fbbb4 to dbc9b4f Compare April 20, 2023 13:54
@skriss skriss added the release-note/small A small change that needs one line of explanation in the release notes. label Apr 20, 2023
@skriss
Copy link
Member

skriss commented Apr 20, 2023

Thanks @padlar!

Once #5214 merges, we'll need to rebase this on upstream/main.

This will also need:

@padlar
Copy link
Contributor Author

padlar commented Apr 20, 2023

  • make generate to be run and the results committed

I have run make generate bunch of times with no changes. Not sure why the CI thinks otherwise. Go version v1.20.3 is the same.

@github-actions
Copy link

github-actions bot commented May 5, 2023

The Contour project currently lacks enough contributors to adequately respond to all PRs.

This bot triages PRs according to the following rules:

  • After 14d of inactivity, lifecycle/stale is applied
  • After 30d of inactivity since lifecycle/stale was applied, the PR is closed

You can:

  • Mark this PR as fresh by commenting or pushing a commit
  • Close this PR
  • Offer to help out with triage

Please send feedback to the #contour channel in the Kubernetes Slack

@github-actions github-actions bot added the lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. label May 5, 2023
@skriss skriss removed the lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. label May 16, 2023
@github-actions
Copy link

The Contour project currently lacks enough contributors to adequately respond to all PRs.

This bot triages PRs according to the following rules:

  • After 14d of inactivity, lifecycle/stale is applied
  • After 30d of inactivity since lifecycle/stale was applied, the PR is closed

You can:

  • Mark this PR as fresh by commenting or pushing a commit
  • Close this PR
  • Offer to help out with triage

Please send feedback to the #contour channel in the Kubernetes Slack

@github-actions github-actions bot added the lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. label May 31, 2023
@skriss
Copy link
Member

skriss commented Jun 1, 2023

@padlar we've just merged #5214, so this should be ready for a rebase onto main. I'd suggest something like:

git fetch upstream main
git rebase -i upstream/main
<in the interactive rebase prompt, delete the lines for any commits that aren't yours>
<resolve any conflicts>
git push -f

@skriss skriss removed the lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. label Jun 1, 2023
@skriss skriss self-requested a review June 1, 2023 14:43
@padlar padlar force-pushed the gw-provisioner-optout-crds branch from f3a4a7a to 2e7a9c9 Compare June 1, 2023 18:25
@padlar padlar force-pushed the gw-provisioner-optout-crds branch from 2e7a9c9 to 60f9e35 Compare June 1, 2023 18:26
@@ -114,6 +114,10 @@ type ContourSettings struct {
// Deployment describes the settings for running contour as a `Deployment`.
// +optional
Deployment *DeploymentSettings `json:"deployment,omitempty"`

// WatchNamespaces restricts watching contour instances to a subset of namespaces.
Copy link
Member

Choose a reason for hiding this comment

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

I think we want the documentation here and the release note to state that this field is for limiting the namespaces the Contour instance will watch for resource updates (and not what the Gateway Provisioner will watch)

@skriss
Copy link
Member

skriss commented Jun 21, 2023

One thing I'm realizing we haven't been great about as we add more options to ContourDeployment, is keeping in mind this section from the Gateway API spec:

It is recommended that [GatewayClass] be used as a template for Gateways. This means that a Gateway is based on the state of the GatewayClass at the time it was created and changes to the GatewayClass or associated parameters are not propagated down to existing Gateways. This recommendation is intended to limit the blast radius of changes to GatewayClass or associated parameters. If implementations choose to propagate GatewayClass changes to existing Gateways, that MUST be clearly documented by the implementation.

(ref. https://gateway-api.sigs.k8s.io/references/spec/#gateway.networking.k8s.io/v1beta1.GatewayClass)

So basically, any settings from the GatewayClass/ContourDeployment should be applied at the time of creating a Gateway's Kubernetes resources, but not subsequently updated if the GatewayClass/ContourDeployment change.

There is also kubernetes-sigs/gateway-api#1868 (GEP) to consider, which has recently been merged upstream but has not yet been added to the API.

@github-actions
Copy link

github-actions bot commented Jul 6, 2023

The Contour project currently lacks enough contributors to adequately respond to all PRs.

This bot triages PRs according to the following rules:

  • After 14d of inactivity, lifecycle/stale is applied
  • After 30d of inactivity since lifecycle/stale was applied, the PR is closed

You can:

  • Mark this PR as fresh by commenting or pushing a commit
  • Close this PR
  • Offer to help out with triage

Please send feedback to the #contour channel in the Kubernetes Slack

@github-actions github-actions bot added the lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. label Jul 6, 2023
@github-actions
Copy link

github-actions bot commented Aug 6, 2023

The Contour project currently lacks enough contributors to adequately respond to all PRs.

This bot triages PRs according to the following rules:

  • After 14d of inactivity, lifecycle/stale is applied
  • After 30d of inactivity since lifecycle/stale was applied, the PR is closed

You can:

  • Mark this PR as fresh by commenting or pushing a commit
  • Close this PR
  • Offer to help out with triage

Please send feedback to the #contour channel in the Kubernetes Slack

@github-actions github-actions bot closed this Aug 6, 2023
@sunjayBhatia
Copy link
Member

Hey @padlar looks like we'll have to move this to the next release milestone since 1.26.0 is coming up

We'll have to discuss how to handle @skriss' comment #5302 (comment) and how we want to move this one forward

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. release-note/small A small change that needs one line of explanation in the release notes.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Support namespace-scoped Contour instances via Gateway provisioner
4 participants