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

provider: EG should not provision if parametersRef not found #5081

Open
wants to merge 8 commits into
base: main
Choose a base branch
from

Conversation

zirain
Copy link
Member

@zirain zirain commented Jan 16, 2025

fixes: #5080

@zirain zirain requested a review from a team as a code owner January 16, 2025 10:10
Copy link

codecov bot commented Jan 16, 2025

Codecov Report

Attention: Patch coverage is 80.00000% with 3 lines in your changes missing coverage. Please review.

Project coverage is 66.97%. Comparing base (c499b41) to head (ffa227f).

Files with missing lines Patch % Lines
internal/provider/kubernetes/controller.go 40.00% 3 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #5081      +/-   ##
==========================================
+ Coverage   66.84%   66.97%   +0.12%     
==========================================
  Files         211      211              
  Lines       32920    32906      -14     
==========================================
+ Hits        22007    22039      +32     
+ Misses       9586     9545      -41     
+ Partials     1327     1322       -5     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@zirain zirain force-pushed the eg-daemonset branch 3 times, most recently from ec1bcd6 to 99ed84d Compare January 16, 2025 12:50
@zirain zirain changed the title provider: EG should not provision if not found provider: EG should not provision if parametersRef not found Jan 16, 2025
@@ -406,11 +406,6 @@ func (i *Infra) deleteServiceAccount(ctx context.Context, r ResourceRender) (err

// deleteDeployment deletes the Envoy Deployment in the kube api server, if it exists.
func (i *Infra) deleteDeployment(ctx context.Context, r ResourceRender) (err error) {
// If deployment config is nil,ignore Deployment.
if deploymentConfig, er := r.DeploymentSpec(); deploymentConfig == nil {
Copy link
Member Author

Choose a reason for hiding this comment

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

You should not care whether the resource exists when deleting.

Copy link
Contributor

Choose a reason for hiding this comment

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

was this a bug, uncovered while testing this change ?

Copy link
Member Author

Choose a reason for hiding this comment

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

apiVersion: gateway.networking.k8s.io/v1
kind: GatewayClass
metadata:
  name: eg
spec:
  controllerName: gateway.envoyproxy.io/gatewayclass-controller
  parametersRef:
    group: gateway.envoyproxy.io
    kind: EnvoyProxy
    name: eg-daemonset
    namespace: default
---
apiVersion: gateway.networking.k8s.io/v1
kind: Gateway
metadata:
  name: eg-daemonset
  namespace: default
spec:
  gatewayClassName: eg
  listeners:
    - name: http
      port: 80
      protocol: HTTP
      allowedRoutes:
        namespaces:
          from: All
---
apiVersion: gateway.envoyproxy.io/v1alpha1
kind: EnvoyProxy
metadata:
  name: eg-daemonset
  namespace: default
spec:
  ipFamily: IPv4
  telemetry:
    accessLog:
      settings:
        - format:
            type: Text
            text: |
              [%START_TIME%] %METADATA(ROUTE:envoy-gateway:resources)% "%REQ(:METHOD)% %REQ(X-ENVOY-ORIGINAL-PATH?:PATH)% %PROTOCOL%" %RESPONSE_CODE% %RESPONSE_FLAGS% %BYTES_RECEIVED% %BYTES_SENT% %DURATION% "%REQ(X-FORWARDED-FOR)%" "%REQ(USER-AGENT)%" "%REQ(X-REQUEST-ID)%" "%REQ(:AUTHORITY)%" "%UPSTREAM_HOST%"
          matches:
            - "'x-envoy-logged' in request.headers"
          sinks:
            - type: OpenTelemetry
              openTelemetry:
                backendRefs:
                  - name: otel-collector
                    namespace: monitoring
                    port: 4317
                resources:
                  k8s.cluster.name: "envoy-gateway"
  provider:
    type: Kubernetes
    kubernetes:
      envoyDaemonSet:
        patch:
          type: StrategicMerge
          value:
            spec:
              template:
                spec:
                  containers:
                    - name: envoy
                      readinessProbe:
                        initialDelaySeconds: 5
---
apiVersion: gateway.networking.k8s.io/v1
kind: HTTPRoute
metadata:
  name: foo-route
  namespace: default
spec:
  parentRefs:
    - name: eg-daemonset
  rules:
    - backendRefs:
        - name: infra-backend-v1
          port: 8080
      matches:
        - path:
            type: PathPrefix
            value: /foo

with this demo, ds won't be removed.

@@ -138,10 +138,10 @@ kube-demo-undeploy: ## Uninstall the Kubernetes resources installed from the `ma
# tools/hack/run-kube-local.sh

.PHONY: conformance
conformance: create-cluster kube-install-image kube-deploy run-conformance delete-cluster ## Create a kind cluster, deploy EG into it, run Gateway API conformance, and clean up.
conformance: create-cluster kube-install-image kube-deploy install-eg-addons run-conformance delete-cluster ## Create a kind cluster, deploy EG into it, run Gateway API conformance, and clean up.
Copy link
Member Author

Choose a reason for hiding this comment

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

GC(envoy-gateway) refernece EnvoyProxy(proxy-config), that make service monitoring/otel-collector is required.

Copy link
Contributor

Choose a reason for hiding this comment

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

is this because we are using the same GC for conformance and e2e ? should we maintain 2 different GatewayClasses to keep each test suite more lightweight ?

Copy link
Member Author

Choose a reason for hiding this comment

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

yes, we can do that later.

@@ -568,8 +568,7 @@ func (r *gatewayAPIReconciler) updateStatusForGateway(ctx context.Context, gtw *
r.log.Info("failed to get Service for gateway",
"namespace", gtw.Namespace, "name", gtw.Name)
}
// update accepted condition
status.UpdateGatewayStatusAcceptedCondition(gtw, true)
Copy link
Contributor

Choose a reason for hiding this comment

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

curious why this was removed

Copy link
Member Author

Choose a reason for hiding this comment

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

I think we should respect the logic in controller.go.

Copy link
Member Author

Choose a reason for hiding this comment

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

this made gateway always accepted, even the ref EnvoyProxy is not found.

@@ -61,14 +61,6 @@ spec:
port: 4317
resources:
k8s.cluster.name: "envoy-gateway"
- sinks:
Copy link
Member Author

Choose a reason for hiding this comment

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

this's not working for a long time and should blocked gateway provision.

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.

provider: EG should not provision if parametersRef not found
2 participants