Skip to content

Conversation

@akafazov
Copy link
Contributor

@akafazov akafazov commented Nov 4, 2025

A version of local-setup with uses all local-charts and no public OCM components to run without Istio.

This version of the local-setup introduces a new task local-setup-prerelease which only uses a locally built OCM components from the local-charts. It also uses traefik as ingress controller.

Warning

Merging this could brake existing deployments using Istio, unless the necessary configuration is done in PlatformMesh resource.

refers to #183

…out istio

On-behalf-of: @SAP angel.kafazov@sap.com
Signed-off-by: Angel Kafazov <akafazov@cst-bg.net>
On-behalf-of: @SAP angel.kafazov@sap.com
Signed-off-by: Angel Kafazov <akafazov@cst-bg.net>
On-behalf-of: @SAP angel.kafazov@sap.com
Signed-off-by: Angel Kafazov <akafazov@cst-bg.net>
On-behalf-of: @SAP angel.kafazov@sap.com
Signed-off-by: Angel Kafazov <akafazov@cst-bg.net>
On-behalf-of: @SAP angel.kafazov@sap.com
Signed-off-by: Angel Kafazov <akafazov@cst-bg.net>
On-behalf-of: @SAP angel.kafazov@sap.com
Signed-off-by: Angel Kafazov <akafazov@cst-bg.net>
@akafazov akafazov marked this pull request as draft November 4, 2025 07:49
On-behalf-of: @SAP angel.kafazov@sap.com
Signed-off-by: Angel Kafazov <akafazov@cst-bg.net>
On-behalf-of: @SAP angel.kafazov@sap.com
Signed-off-by: Angel Kafazov <akafazov@cst-bg.net>
On-behalf-of: @SAP angel.kafazov@sap.com
Signed-off-by: Angel Kafazov <akafazov@cst-bg.net>
On-behalf-of: @SAP angel.kafazov@sap.com
Signed-off-by: Angel Kafazov <akafazov@cst-bg.net>
On-behalf-of: @SAP angel.kafazov@sap.com
Signed-off-by: Angel Kafazov <akafazov@cst-bg.net>
On-behalf-of: @SAP angel.kafazov@sap.com
Signed-off-by: Angel Kafazov <akafazov@cst-bg.net>
@akafazov akafazov changed the title Feat/gatewayApi feat(gatewayApi): update local-setup to use K8S Gateway API Nov 4, 2025
On-behalf-of: @SAP angel.kafazov@sap.com
Signed-off-by: Angel Kafazov <akafazov@cst-bg.net>
On-behalf-of: @SAP angel.kafazov@sap.com
Signed-off-by: Angel Kafazov <akafazov@cst-bg.net>
On-behalf-of: @SAP angel.kafazov@sap.com
Signed-off-by: Angel Kafazov <akafazov@cst-bg.net>
On-behalf-of: @SAP angel.kafazov@sap.com
Signed-off-by: Angel Kafazov <akafazov@cst-bg.net>
@akafazov akafazov added the ai-code-review CodeRabbitAI label Nov 11, 2025
@akafazov
Copy link
Contributor Author

@coderabbitai review

@coderabbitai
Copy link

coderabbitai bot commented Nov 11, 2025

✅ Actions performed

Review triggered.

Note: CodeRabbit is an incremental review system and does not re-review already reviewed commits. This command is applicable only when automatic reviews are paused.

@coderabbitai
Copy link

coderabbitai bot commented Nov 11, 2025

Walkthrough

This pull request introduces a comprehensive refactoring of the local development environment setup, shifting from Istio to Gateway API infrastructure while adding prerelease component workflows. Changes include workflow modifications to add Traefik repository initialization, updates to task orchestration with host alias patching hooks, new Gateway API CRD management via Flux, and reorganization of OCM deployment logic. Kubernetes manifests are updated to disable Istio across multiple components, enable Gateway API routing, and remove Istio-system namespace references. New prerelease-specific overlays, components, and startup scripts are introduced alongside improved e2e test resilience through extended timeouts. The .gitignore configuration is broadened to exclude archive files and PNG images from the repository root and test directories.


Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

Copy link

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

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

Actionable comments posted: 5

Caution

Some comments are outside the diff and can’t be posted inline due to platform limitations.

⚠️ Outside diff range comments (1)
local-setup/scripts/start.sh (1)

102-112: Certificate files referenced in script are missing and will cause deployment failure.

The script at lines 102-112 references certificate files that do not exist in the repository:

  • $SCRIPT_DIR/certs/cert.crt — missing
  • $SCRIPT_DIR/certs/cert.key — missing
  • $SCRIPT_DIR/certs/ca.crt — missing

The kubectl create secret commands using --from-file will fail at runtime. The certificate files must be present in local-setup/certs/ for these secrets to be created successfully.

Additionally, the secret placement in default and platform-mesh-system namespaces is correct and consumed by chart deployments via the domain-certificate credential name in the Gateway API configuration.

🧹 Nitpick comments (1)
local-setup/scripts/start.sh (1)

51-61: Update Traefik configuration to use stable Gateway API flags or pin version explicitly.

Kubernetes Gateway API support is stable in Traefik v3.1 and later (latest stable is v3.5.3). The code uses experimental flag naming (e.g., experimental.kubernetesGateway.enabled), which is outdated for current stable releases.

Recommend one of:

  1. Add explicit version pinning to the Helm command (e.g., --version 3.1.0 or higher) to ensure stable Gateway API features.
  2. Update flag names to non-experimental variants if available in the target version.

Without version pinning, the chart may install older versions where experimental flags were necessary.

📜 Review details

Configuration used: Repository: platform-mesh/coderabbit/.coderabbit.yaml

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 222413f and 7b10c86.

📒 Files selected for processing (25)
  • .github/workflows/kind-localsetup.yaml (1 hunks)
  • .gitignore (1 hunks)
  • Taskfile.yaml (10 hunks)
  • local-setup/README-developers.md (1 hunks)
  • local-setup/e2e/test-register-and-navigate.test.ts (5 hunks)
  • local-setup/kustomize/base/crds-extra/gateway-api-flux-extras.yaml (1 hunks)
  • local-setup/kustomize/base/crds-extra/kustomization.yaml (1 hunks)
  • local-setup/kustomize/base/crds/gateway-api-flux.yaml (1 hunks)
  • local-setup/kustomize/base/crds/kustomization.yaml (1 hunks)
  • local-setup/kustomize/base/namespaces/namespace.yaml (0 hunks)
  • local-setup/kustomize/components/ocm-prerelease-component/component.yaml (1 hunks)
  • local-setup/kustomize/components/ocm-prerelease-component/kustomization.yaml (1 hunks)
  • local-setup/kustomize/components/ocm-prerelease-component/repository.yaml (1 hunks)
  • local-setup/kustomize/components/platform-mesh-operator-resource/istio-gateway.yaml (0 hunks)
  • local-setup/kustomize/components/platform-mesh-operator-resource/kustomization.yaml (0 hunks)
  • local-setup/kustomize/components/platform-mesh-operator-resource/platform-mesh.yaml (8 hunks)
  • local-setup/kustomize/components/platform-mesh-operator/release.yaml (1 hunks)
  • local-setup/kustomize/overlays/k8s-ocm-toolkit-patch/kustomization.yaml (1 hunks)
  • local-setup/kustomize/overlays/ocm-prerelease/kustomization.yaml (0 hunks)
  • local-setup/kustomize/overlays/ocm-prerelease/vs-registry.yaml (0 hunks)
  • local-setup/kustomize/overlays/platform-mesh-operator-prerelease/kustomization.yaml (1 hunks)
  • local-setup/kustomize/overlays/platform-mesh-resource/kustomization.yaml (1 hunks)
  • local-setup/kustomize/overlays/prerelease/kustomization.yaml (1 hunks)
  • local-setup/scripts/start-prerelease.sh (1 hunks)
  • local-setup/scripts/start.sh (3 hunks)
💤 Files with no reviewable changes (5)
  • local-setup/kustomize/overlays/ocm-prerelease/kustomization.yaml
  • local-setup/kustomize/base/namespaces/namespace.yaml
  • local-setup/kustomize/components/platform-mesh-operator-resource/kustomization.yaml
  • local-setup/kustomize/components/platform-mesh-operator-resource/istio-gateway.yaml
  • local-setup/kustomize/overlays/ocm-prerelease/vs-registry.yaml
🔇 Additional comments (31)
.gitignore (2)

19-19: Verify the glob pattern scope change.

The change from ./*.tgz to *.tgz broadens the ignore scope to match .tgz files anywhere in the repository, rather than only in the root directory. Ensure this is intentional and aligns with where prerelease and build artifacts are generated.


24-24: PNG exclusion for e2e test artifacts looks appropriate.

Excluding PNG files from the e2e test directory aligns with the PR objectives around e2e test improvements and prevents test artifacts from being tracked.

local-setup/kustomize/components/platform-mesh-operator-resource/platform-mesh.yaml (4)

18-23: Istio migration to Gateway API verified and complete.

All Istio components (istio-base, istio-istiod, istio-gateway) are confirmed disabled. Verification across the kustomize directory confirms three services migrated to Gateway API with no remaining Istio dependencies. No Istio-specific networking resources (VirtualService, DestinationRule, PeerAuthentication) or sidecar injection annotations found in the codebase.


130-134: Gateway configuration verified—no issues found.

The k8sapi-gateway resource is properly defined in charts/infra/templates/gatewayapi/gateway.yaml and configured in charts/infra/values.yaml. The gateway includes a listener named websecure (port 8443, protocol HTTPS) with TLS termination enabled using the domain-certificate credential. The HttpRoute reference in the platform-mesh.yaml file correctly targets this gateway with the matching section name. The configuration is consistent and properly supports HTTPS traffic routing.


24-27: Multiple components have implicit service dependencies despite declaring empty dependsOn arrays; confirm platform-mesh framework handles these correctly.

The analysis reveals a structural mismatch: 11 components declare dependsOn: [] but contain functional dependencies on other services:

  • portal requires kubernetes-graphql-gateway (line 137: OPENMFP_PORTAL_CONTEXT_CRD_GATEWAY_API_URL references kubernetes-graphql-gateway endpoint)
  • security-operator requires keycloak (line 111: idp-from-address=keycloak@portal.dev.local)
  • openfga requires observability (line 162: observability-opentelemetry-collector endpoint)
  • virtual-workspaces and kubernetes-graphql-gateway require frontproxy/infra (deployment serverUrl references frontproxy-front-proxy)

The README.md deployment documentation does not explain the framework's dependency resolution strategy or whether empty arrays indicate auto-detected dependencies, retry logic, or other handling mechanisms. No health checks or readiness probes are configured in this file.

Verify whether the platform-mesh framework automatically resolves these implicit service dependencies or whether explicit dependsOn entries should be added to ensure correct initialization ordering and prevent race conditions.


62-65: Verify host alias IP address allocation and network accessibility.

The host alias IP has been changed from 10.96.188.4 to 10.96.73.54 across the configuration (lines 62, 118, and 151). Code analysis confirms the migration is complete and consistent. However, verify that:

  • The IP 10.96.73.54 is correctly allocated in your cluster
  • DNS hostnames kcp.api.portal.dev.local and portal.dev.local resolve to this address
  • The IP is reachable from within the cluster

Additionally, confirm that the k8sapi-gateway resource (referenced at line 132) is properly deployed via the infra chart component before the Portal and kubernetes-graphql-gateway components start.

local-setup/kustomize/components/platform-mesh-operator/release.yaml (1)

16-21: LGTM: Istio disabled in platform-mesh-operator.

The values configuration correctly disables Istio and sets the appropriate feature flags, aligning with the PR's goal of migrating to Gateway API infrastructure.

local-setup/kustomize/base/crds/gateway-api-flux.yaml (2)

1-10: LGTM: Gateway API source configured correctly.

The GitRepository configuration properly references the official Kubernetes Gateway API repository at version v1.2.1.


24-33: Remove the misleading TODO comment; the CRD name is correct.

BackendTLSPolicy is included in the Kubernetes Gateway API v1.2.1 experimental channel with version v1alpha3, and the CRD name backendtlspolicies.policy.networking.k8s.io is correct. The inline comment suggesting the name should be replaced with an "exact standard CRD name" is inaccurate since BackendTLSPolicy is an experimental (alpha) resource, not a standard one. Remove the comment on line 29 to reflect the completed implementation.

local-setup/e2e/test-register-and-navigate.test.ts (2)

16-16: LGTM: Reasonable timeout values for e2e test operations.

The timeout values (10-30 seconds) are appropriate for the respective operations and improve test resilience.

Also applies to: 22-22, 50-50, 59-59, 93-93


34-34: Verify the exceptionally long timeout (200 seconds).

The timeout of 200000ms (3.33 minutes) for clicking the "Update Your Account" link is significantly longer than other timeouts in this test. Please confirm this is intentional and not a typo.

If this operation legitimately requires such a long timeout, consider adding a comment explaining why the email invitation processing takes so long, as this could indicate a performance issue worth investigating.

local-setup/kustomize/base/crds/kustomization.yaml (1)

7-7: LGTM: Gateway API Flux resource added.

The addition of gateway-api-flux.yaml to the CRD resources is appropriate for managing Gateway API CRDs via Flux.

local-setup/kustomize/components/ocm-prerelease-component/repository.yaml (1)

1-10: LGTM: Prerelease repository configuration is correct.

The OCIRegistry configuration properly references the local OCI registry service for prerelease components.

.github/workflows/kind-localsetup.yaml (1)

66-66: LGTM: Traefik repository addition is necessary.

Adding the Traefik Helm repository before running local-setup is appropriate, as the PR introduces Traefik as the ingress controller.

local-setup/kustomize/overlays/k8s-ocm-toolkit-patch/kustomization.yaml (1)

1-4: File verification passed.

The referenced patch file ocm-k8s-toolkit-patch.yaml exists in the specified directory. The kustomization configuration is correct.

local-setup/kustomize/components/ocm-prerelease-component/kustomization.yaml (1)

1-5: No issues found.

Both repository.yaml and component.yaml files are present in the local-setup/kustomize/components/ocm-prerelease-component/ directory as expected. The kustomization.yaml file correctly references existing resources.

local-setup/kustomize/base/crds-extra/kustomization.yaml (1)

1-5: Kustomization structure looks good.

The base kustomization correctly references the gateway-api-flux-extras resource. Ensure that the referenced file exists and is properly formatted.

local-setup/README-developers.md (1)

15-18: Verify referenced Taskfile parameters and tasks exist.

The documentation references Taskfile.yaml parameters (COMPONENT_PRERELEASE_VERSION, CUSTOM_LOCAL_COMPONENTS_CHART_PATHS, COMPONENT_VERSION_FIX_DEPEDENCY_VERSIONS) and tasks (ocm:build, ocm:apply, ocm:cleanup). Ensure these are defined in Taskfile.yaml and that the documentation aligns with actual configuration.

local-setup/kustomize/overlays/platform-mesh-operator-prerelease/kustomization.yaml (1)

1-34: Verify prerelease image tag and component configuration.

The overlay patches the HelmRelease to use image tag kcp-gates (line 23) and switches component/OCM references to prerelease. Confirm:

  1. The image tag kcp-gates exists in the container registry and is suitable for prerelease testing.
  2. The prerelease component and repository are properly defined in other manifests.

Structure and syntax are correct.

local-setup/kustomize/overlays/prerelease/kustomization.yaml (1)

1-32: IP address inconsistency between prerelease and non-prerelease overlays.

This prerelease overlay patches hostAliases to 10.96.165.217 (lines 14-21), whereas local-setup/kustomize/overlays/platform-mesh-resource/kustomization.yaml uses 10.96.57.97 for the same paths. Clarify whether:

  1. Both IP addresses are intentional for their deployment contexts, or
  2. One is incorrect and should be synchronized.

Additionally, verify that ocm-prerelease-component and platform-mesh-operator-resource (lines 4-5) exist and are properly structured.

local-setup/kustomize/components/ocm-prerelease-component/component.yaml (1)

1-15: Verify prerelease repository and semver configuration.

The component configuration introduces repositoryRef (line 6-7), semver: 1.0.0 (line 9), and references the component github.com/platform-mesh/prerelease (line 8). Ensure:

  1. The prerelease repository is properly defined in related manifests.
  2. Semver 1.0.0 is correct for this prerelease component.
  3. The interval 1m (line 10) is appropriate for prerelease polling.

Structure is valid.

local-setup/scripts/start.sh (2)

121-122: Messaging update correctly reflects content.

Line 121 changes the echo message from "OCM Controller and Platform Mesh" to "Kyverno policies", which accurately reflects that line 122 applies Kyverno policies. This change is appropriate for the Istio-to-Gateway API migration.


99-100: Add explanatory comment and error handling for CRD deletion.

The verification found no components in the codebase depend on backendtlspolicies.gateway.networking.k8s.io, contrary to the original concern. However, the deletion lacks explanation and error handling remains valid issues:

  1. Add an inline comment explaining why this CRD must be deleted before applying crds-extra
  2. Consider adding explicit error handling (e.g., || true suffix) if the CRD may not exist in all scenarios, or document why the script should fail if deletion fails

The relationship between this deletion and the subsequent Flux-managed CRDs in line 100 should be clarified in a code comment.

local-setup/kustomize/overlays/platform-mesh-resource/kustomization.yaml (1)

5-19: Document the purpose of hostAlias IP addresses across deployment overlays.

The IP addresses are correctly configured as environment-specific values: 10.96.57.97 for the platform-mesh-resource overlay (standard deployment) and 10.96.165.217 for the prerelease overlay (prerelease environment). This differentiation is intentional and appropriate.

However, document the purpose of these hostAlias IP addresses. Add a comment or documentation explaining which service or endpoint each IP refers to, enabling maintainers to understand the configuration intent and validate correctness for future deployments.

local-setup/kustomize/base/crds-extra/gateway-api-flux-extras.yaml (1)

1-14: Address experimental CRD stability concern in production-intended configuration.

Verification confirms the gateway-api GitRepository exists and is correctly referenced; namespace placement is appropriate and consistent. However, the configuration uses config/crd/experimental (line 14) rather than the stable ./config/crd path used by the related gateway-api-crds Kustomization.

Experimental CRDs are not recommended for production use and are explicitly subject to change. Determine whether experimental CRD usage is intentional:

  • If intentional: document the risk acceptance and rationale.
  • If not intentional: switch to the stable path (./config/crd) to align with the parallel gateway-api-crds resource.
local-setup/scripts/start-prerelease.sh (2)

37-37: Verify certificate generation script path.

Line 37 references $SCRIPT_DIR/../scripts/gen-certs.sh. Since the current script is already in scripts/, this path traverses back then re-enters scripts/. Verify the intended path is correct or simplify to $SCRIPT_DIR/gen-certs.sh.


219-222: Verify git diff reliability in non-git environments.

Line 219 uses git diff to check for changes in a kustomize file. This command may fail if the repository is not initialized or if the script runs in an environment without git context. Consider adding error handling or documenting this dependency.

Taskfile.yaml (4)

169-169: Clarify intent of changing internal visibility flags.

Lines 169, 293, and 421 change tasks from internal: true to internal: false. This makes these tasks visible and directly callable. Verify this is intentional and document why external visibility is needed for ocm:deploy:ociregistry, ocm:configure:tls, and ocm:deploy:transfer-pod, as they appear to be implementation details.

Also applies to: 293-293, 421-421


464-467: Verify yq patch syntax and file path.

Lines 464–467 use yq to update the semver field in local-setup/kustomize/components/ocm-prerelease-component/component.yaml. Ensure this file path exists and that the yq in-place edit syntax (using -i with single quotes) is compatible with all supported versions of yq.


542-550: Verify kustomization.yaml structure for host alias patching.

Lines 542–550 use yq to patch kustomization.yaml files, assuming a specific structure with .patches[0] containing a patch field. Verify that both local-setup/kustomize/overlays/platform-mesh-resource/kustomization.yaml and local-setup/kustomize/overlays/prerelease/kustomization.yaml have this structure, and that the yq selector and in-place edit work as intended.


114-120: Approve environment variable propagation for mid-hook orchestration.

The addition of START_MID_HOOK and START_MID_HOOK2 environment variables across local-setup tasks provides a clear mechanism for dynamic hook injection during prerelease workflows. The orchestration aligns with the prerelease script's mid-hook expectations and supports proper sequencing of patches before and after PlatformMesh resource creation.

Also applies to: 130-146, 156-160, 517-537

…single file

On-behalf-of: @SAP angel.kafazov@sap.com
Signed-off-by: Angel Kafazov <akafazov@cst-bg.net>
…script

- Fixed typo in the message for clearing existing certs directory.
- Updated Traefik installation command to specify version 37.3.0.
- Added --ignore-not-found flag to the kubectl delete command for CRDs.
- Minor formatting adjustments for consistency in echo statements.

On-behalf-of: @SAP angel.kafazov@sap.com
Signed-off-by: Angel Kafazov <akafazov@cst-bg.net>
On-behalf-of: @SAP angel.kafazov@sap.com
Signed-off-by: Angel Kafazov <akafazov@cst-bg.net>
On-behalf-of: @SAP angel.kafazov@sap.com
Signed-off-by: Angel Kafazov <akafazov@cst-bg.net>
Copy link
Contributor

@aaronschweig aaronschweig left a comment

Choose a reason for hiding this comment

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

I think I really need a lot of explanations of why things got introduced the way they are in this PR.

The main goal should only be one:

Instead of using Istio for ingress, we want a local setup that uses the Gateway API, with an example (you chose traefik).

In my head the changeset should be really minimal:

  1. Do not install istio in the local setup.
  2. Set the values to disable istio resources and enable gateway resources

In this PR I feel there is so much other stuff that is happening, I can't really digest and process what is really necessary and what not.

Also the whole topic that got mixed in with the prereleases makes it even harder.

My suggestion would be the following:

We leave this PR open for now, but I'd like to see a quick timeboxed separate PR that ONLY contains the necessary changes to get the local setup running with the gateway API resources, not all the other stuff.

This way it is easier to see if and how the gateway API got implemented.

Would that work for you?

Comment on lines +25 to +33
- patch: |
apiVersion: apiextensions.k8s.io/v1
kind: CustomResourceDefinition
metadata:
name: backendtlspolicies.policy.networking.k8s.io # replace with the exact standard CRD name
$patch: delete
target:
kind: CustomResourceDefinition
name: backendtlspolicies.policy.networking.k8s.io No newline at end of file
Copy link
Contributor

Choose a reason for hiding this comment

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

Can you explain me what this does?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That was an attempt to install the experimental Gateway API CRDs next to the standard ones, but is actually handled in the start script, so could be removed from here.

type: None
enabled: false
account-operator:
dependsOn: []
Copy link
Contributor

Choose a reason for hiding this comment

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

Not sure if we should not rather change the operator-components chart to not depend on istio by default and add the addtional dependency in the showrooms, as we normally say we want to overwrite as less as possible in the pm resource for the local setup

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This could be done either in the chart or via configuration in PlatformMesh. The latter which is this approach is simpler because doesn't rely on the prerelease changes, which are needed if we want to deploy the changed charts. Also, this will impact current users.

Comment on lines +11 to +19
- op: replace
path: /spec/values/infra/values/hostAliases/entries/0/ip
value: "10.96.182.144"
- op: replace
path: /spec/values/security-operator/values/hostAliases/items/0/ip
value: "10.96.182.144"
- op: replace
path: /spec/values/portal/values/deployment/hostAliases/0/ip
value: "10.96.182.144"
Copy link
Contributor

Choose a reason for hiding this comment

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

this looks like some very dangerous replacements. Why are they needed now and were not needed before?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is actually replaced by the correct IP using a script. I can put placeholders here, but it is not dangerous. For the ingress controller, we need the IP of the NodePort service to communicate with the gateway somehow, so this hostAliases was the selected way to do it for now. If you have better suggestions, they are welcome.

Comment on lines +13 to +21
- op: replace
path: /spec/values/infra/values/hostAliases/entries/0/ip
value: "10.96.165.217"
- op: replace
path: /spec/values/security-operator/values/hostAliases/items/0/ip
value: "10.96.165.217"
- op: replace
path: /spec/values/portal/values/deployment/hostAliases/0/ip
value: "10.96.165.217"
Copy link
Contributor

Choose a reason for hiding this comment

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

Same here

Copy link
Contributor Author

Choose a reason for hiding this comment

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

same comment as above

Comment on lines +121 to +122
kubectl delete crds backendtlspolicies.gateway.networking.k8s.io --ignore-not-found=true
kubectl apply -k $SCRIPT_DIR/../kustomize/base/crds-extra
Copy link
Contributor

Choose a reason for hiding this comment

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

I'd really like to know why we delete it just to recreate it?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is needed because backendtlspolicies is part of both standard and experimental CRDs and migration requires to delete the standard before installing the experimental CRD.

Comment on lines +146 to +150
# Hook: run optional mid-setup command(s) before applying Platform Mesh base
if [ -n "${START_MID_HOOK:-}" ]; then
echo -e "${COL}[$(date '+%H:%M:%S')] Running mid hook: ${START_MID_HOOK}${COL_RES}"
eval "${START_MID_HOOK}"
fi
Copy link
Contributor

Choose a reason for hiding this comment

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

what are the mid setup commands? why did they need to be introduced and what do they contain?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Those are needed to handle additional complexity of prerelease changes in the same start.sh script. Could simplify it, I guess.

Comment on lines +182 to +185
if [ -n "${START_MID_HOOK2:-}" ]; then
echo -e "${COL}[$(date '+%H:%M:%S')] Running mid hook: ${START_MID_HOOK2}${COL_RES}"
eval "${START_MID_HOOK2}"
fi
Copy link
Contributor

Choose a reason for hiding this comment

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

what is MID_HOOK_2?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

same as above

Comment on lines +117 to +118
env:
START_MID_HOOK2: "task patch-platformmesh-hostaliases"
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't really understand it and I am not sure I like it.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This task is needed to configure the correct service IP in the hostAliases.

Copy link
Contributor

Choose a reason for hiding this comment

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

so we can remove it now, that we have a static IP again?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yes, those hooks will be removed completely and will pass cmd params to the start.sh script instead.

@akafazov
Copy link
Contributor Author

I think I really need a lot of explanations of why things got introduced the way they are in this PR.

The main goal should only be one:

Instead of using Istio for ingress, we want a local setup that uses the Gateway API, with an example (you chose traefik).

In my head the changeset should be really minimal:

  1. Do not install istio in the local setup.
  2. Set the values to disable istio resources and enable gateway resources

In this PR I feel there is so much other stuff that is happening, I can't really digest and process what is really necessary and what not.

Also the whole topic that got mixed in with the prereleases makes it even harder.

My suggestion would be the following:

We leave this PR open for now, but I'd like to see a quick timeboxed separate PR that ONLY contains the necessary changes to get the local setup running with the gateway API resources, not all the other stuff.

This way it is easier to see if and how the gateway API got implemented.

Would that work for you?

I agree with the excessive scope scope of the PR, it does more than Gateway API changes and yes this could be easier to review in a separate PR. This refers to the "prerelease" changes too.

The Gateway API needs an ingress provider of some sort, could be Istio but in this case I replaced it with Traefik. NGINX is also possibility, but something needs to be installed in the local-setup in addition to only the API.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

ai-code-review CodeRabbitAI

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants