You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@flink.apache.org by "csviri (via GitHub)" <gi...@apache.org> on 2025/08/22 11:07:42 UTC
[PR] [FLINK-37193] Feature flag if operator should manage ingress [flink-kubernetes-operator]
csviri opened a new pull request, #1018:
URL: https://github.com/apache/flink-kubernetes-operator/pull/1018
## What is the purpose of the change
This change adds a feature flag to set if the controller should manage the ingress. If set to false controller won't register the related InformerEventSource, and won't create ingress even if set in the spec.
## Brief change log
- adds feature flag as described above and related tests
## Verifying this change
This change added tests and can be verified as follows:
- Added unit tests for the non-trivial logic
## Does this pull request potentially affect one of the following parts:
- Dependencies (does it add or upgrade a dependency): no
- The public API, i.e., is any changes to the `CustomResourceDescriptors`: no
- Core observer or reconciler logic that is regularly executed: yes
## Documentation
- Does this pull request introduce a new feature? yes
- If yes, how is the feature documented? JavaDocs
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@flink.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [FLINK-37193] Feature flag if operator should manage ingress [flink-kubernetes-operator]
Posted by "gyfora (via GitHub)" <gi...@apache.org>.
gyfora commented on code in PR #1018:
URL: https://github.com/apache/flink-kubernetes-operator/pull/1018#discussion_r2296676874
##########
flink-kubernetes-operator/src/main/java/org/apache/flink/kubernetes/operator/utils/IngressUtils.java:
##########
@@ -68,13 +70,30 @@ public class IngressUtils {
private static final String REST_SVC_NAME_SUFFIX = "-rest";
private static final Logger LOG = LoggerFactory.getLogger(IngressUtils.class);
+ public static final String INGRESS_MANAGEMENT_OFF_BUT_SPEC_SET =
+ "Ingress management is turned off but ingress set in spec";
+ public static final String INGRESS_MANAGEMENT = "IngressManagement";
public static void reconcileIngress(
FlinkResourceContext<?> ctx,
FlinkDeploymentSpec spec,
Configuration effectiveConfig,
- KubernetesClient client) {
+ KubernetesClient client,
+ EventRecorder eventRecorder) {
+ if (!ctx.getOperatorConfig().isManageIngress()) {
+ if (spec.getIngress() != null) {
+ log.warn(INGRESS_MANAGEMENT_OFF_BUT_SPEC_SET);
Review Comment:
No need to both log and trigger the event as events are already logged. Logging can be removed
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@flink.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [FLINK-37193] Feature flag if operator should manage ingress [flink-kubernetes-operator]
Posted by "csviri (via GitHub)" <gi...@apache.org>.
csviri commented on code in PR #1018:
URL: https://github.com/apache/flink-kubernetes-operator/pull/1018#discussion_r2293593738
##########
flink-kubernetes-operator/src/main/java/org/apache/flink/kubernetes/operator/utils/IngressUtils.java:
##########
@@ -74,7 +74,9 @@ public static void reconcileIngress(
FlinkDeploymentSpec spec,
Configuration effectiveConfig,
KubernetesClient client) {
-
+ if (!ctx.getOperatorConfig().isManageIngress()) {
+ return;
Review Comment:
makes sense, changed it, PTAL, thx
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@flink.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [FLINK-37193] Feature flag if operator should manage ingress [flink-kubernetes-operator]
Posted by "gyfora (via GitHub)" <gi...@apache.org>.
gyfora merged PR #1018:
URL: https://github.com/apache/flink-kubernetes-operator/pull/1018
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@flink.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [FLINK-37193] Feature flag if operator should manage ingress [flink-kubernetes-operator]
Posted by "csviri (via GitHub)" <gi...@apache.org>.
csviri commented on code in PR #1018:
URL: https://github.com/apache/flink-kubernetes-operator/pull/1018#discussion_r2293741401
##########
flink-kubernetes-operator/src/main/java/org/apache/flink/kubernetes/operator/utils/IngressUtils.java:
##########
@@ -68,13 +70,30 @@ public class IngressUtils {
private static final String REST_SVC_NAME_SUFFIX = "-rest";
private static final Logger LOG = LoggerFactory.getLogger(IngressUtils.class);
+ public static final String INGRESS_MANAGEMENT_OFF_BUT_SPEC_SET =
+ "Ingress management is turned off but ingress set in spec";
+ public static final String INGRESS_MANAGEMENT = "IngressManagement";
public static void reconcileIngress(
FlinkResourceContext<?> ctx,
FlinkDeploymentSpec spec,
Configuration effectiveConfig,
- KubernetesClient client) {
+ KubernetesClient client,
+ EventRecorder eventRecorder) {
+ if (!ctx.getOperatorConfig().isManageIngress()) {
+ if (spec.getIngress() != null) {
Review Comment:
TBH I like such checks separated before the actual logic, but if you insist I can change it.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@flink.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [FLINK-37193] Feature flag if operator should manage ingress [flink-kubernetes-operator]
Posted by "csviri (via GitHub)" <gi...@apache.org>.
csviri commented on code in PR #1018:
URL: https://github.com/apache/flink-kubernetes-operator/pull/1018#discussion_r2297428697
##########
flink-kubernetes-operator/src/main/java/org/apache/flink/kubernetes/operator/utils/IngressUtils.java:
##########
@@ -68,13 +70,30 @@ public class IngressUtils {
private static final String REST_SVC_NAME_SUFFIX = "-rest";
private static final Logger LOG = LoggerFactory.getLogger(IngressUtils.class);
+ public static final String INGRESS_MANAGEMENT_OFF_BUT_SPEC_SET =
+ "Ingress management is turned off but ingress set in spec";
+ public static final String INGRESS_MANAGEMENT = "IngressManagement";
public static void reconcileIngress(
FlinkResourceContext<?> ctx,
FlinkDeploymentSpec spec,
Configuration effectiveConfig,
- KubernetesClient client) {
+ KubernetesClient client,
+ EventRecorder eventRecorder) {
+ if (!ctx.getOperatorConfig().isManageIngress()) {
+ if (spec.getIngress() != null) {
+ log.warn(INGRESS_MANAGEMENT_OFF_BUT_SPEC_SET);
Review Comment:
removed log, thx!
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@flink.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [FLINK-37193] Feature flag if operator should manage ingress [flink-kubernetes-operator]
Posted by "gyfora (via GitHub)" <gi...@apache.org>.
gyfora commented on code in PR #1018:
URL: https://github.com/apache/flink-kubernetes-operator/pull/1018#discussion_r2293953303
##########
flink-kubernetes-operator/src/main/java/org/apache/flink/kubernetes/operator/utils/IngressUtils.java:
##########
@@ -68,13 +70,30 @@ public class IngressUtils {
private static final String REST_SVC_NAME_SUFFIX = "-rest";
private static final Logger LOG = LoggerFactory.getLogger(IngressUtils.class);
+ public static final String INGRESS_MANAGEMENT_OFF_BUT_SPEC_SET =
+ "Ingress management is turned off but ingress set in spec";
+ public static final String INGRESS_MANAGEMENT = "IngressManagement";
public static void reconcileIngress(
FlinkResourceContext<?> ctx,
FlinkDeploymentSpec spec,
Configuration effectiveConfig,
- KubernetesClient client) {
+ KubernetesClient client,
+ EventRecorder eventRecorder) {
+ if (!ctx.getOperatorConfig().isManageIngress()) {
+ if (spec.getIngress() != null) {
Review Comment:
ok, lets keep it
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@flink.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [FLINK-37193] Feature flag if operator should manage ingress [flink-kubernetes-operator]
Posted by "gyfora (via GitHub)" <gi...@apache.org>.
gyfora commented on code in PR #1018:
URL: https://github.com/apache/flink-kubernetes-operator/pull/1018#discussion_r2293494223
##########
flink-kubernetes-operator/src/main/java/org/apache/flink/kubernetes/operator/utils/IngressUtils.java:
##########
@@ -74,7 +74,9 @@ public static void reconcileIngress(
FlinkDeploymentSpec spec,
Configuration effectiveConfig,
KubernetesClient client) {
-
+ if (!ctx.getOperatorConfig().isManageIngress()) {
+ return;
Review Comment:
may be better to throw a reconciliation error if the user actually needs an ingress and it is turned off. Or at least log a warning and trigger an event. Maybe the exception is too drastic but the event is a minimum
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@flink.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [FLINK-37193] Feature flag if operator should manage ingress [flink-kubernetes-operator]
Posted by "gyfora (via GitHub)" <gi...@apache.org>.
gyfora commented on code in PR #1018:
URL: https://github.com/apache/flink-kubernetes-operator/pull/1018#discussion_r2293728907
##########
flink-kubernetes-operator/src/main/java/org/apache/flink/kubernetes/operator/utils/IngressUtils.java:
##########
@@ -68,13 +70,30 @@ public class IngressUtils {
private static final String REST_SVC_NAME_SUFFIX = "-rest";
private static final Logger LOG = LoggerFactory.getLogger(IngressUtils.class);
+ public static final String INGRESS_MANAGEMENT_OFF_BUT_SPEC_SET =
+ "Ingress management is turned off but ingress set in spec";
+ public static final String INGRESS_MANAGEMENT = "IngressManagement";
public static void reconcileIngress(
FlinkResourceContext<?> ctx,
FlinkDeploymentSpec spec,
Configuration effectiveConfig,
- KubernetesClient client) {
+ KubernetesClient client,
+ EventRecorder eventRecorder) {
+ if (!ctx.getOperatorConfig().isManageIngress()) {
+ if (spec.getIngress() != null) {
Review Comment:
would be cleaner to move this logic in the already existing `if (spec.getIngress() != null) {` branch, and return from there on error
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@flink.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org