You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@flink.apache.org by "ryanvanhuuksloot (via GitHub)" <gi...@apache.org> on 2023/12/08 16:24:00 UTC
[PR] [FLINK-33454] [Operator] Add IngressTlsSpec to support TLS within the managed Ingress, also add Label Passthrough [flink-kubernetes-operator]
ryanvanhuuksloot opened a new pull request, #727:
URL: https://github.com/apache/flink-kubernetes-operator/pull/727
## What is the purpose of the change
The managed ingress `IngressSpec` doesn't support TLS. It means that the ingress can't be used with secrets to secure the hosts. We can extend the `IngressSpec` to have `tls: List<IngressTlsSpec>`. In theory we shouldn't need a List spec here since the template only supports one host but I thought it was better to follow the Kubernetes spec since the change is pretty simple.
The managed ingress also doesn't have the ability to pass labels through to the Ingress. It would be nice to support this. It could be in a separate PR if we'd like but is very trivial.
Open to `IngressTLSSpec` if folks think that is better. I didn't like the double capital S beside each other.
## Brief change log
Added an `IngressTlsSpec` to support tls entries in the `IngressSpec`
Added a labels entry to the `IngressSpec` to support label passthrough to the Ingress similar to annotations.
## Verifying this change
This change is a trivial rework / code cleanup without any test coverage.
- I did not add tests for the labels as we don't have explicit tests for the annotations. I wasn't sure if this was intentional. I'd be happy to add tests for the labels.
This change added tests and can be verified as follows:
- Added tests for IngressTls from the IngressUtils including
1. Empty tls
2. empty hosts with single secretName, single tls entry
3. empty secretName with single hosts, single tls entry
4. single hosts and single secretName, single tls entry
5. multiple hosts and single secretName, single tls entry
6. multiple tls entries
I still need to manually validate the ingress spec with a job.
## 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`: yes
- Core observer or reconciler logic that is regularly executed: no
## Documentation
- Does this pull request introduce a new feature? yes
- If yes, how is the feature documented? docs
--
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-33454] [Operator] Add IngressTlsSpec to support TLS within the managed Ingress, also add Label Passthrough [flink-kubernetes-operator]
Posted by "ryanvanhuuksloot (via GitHub)" <gi...@apache.org>.
ryanvanhuuksloot commented on code in PR #727:
URL: https://github.com/apache/flink-kubernetes-operator/pull/727#discussion_r1423102109
##########
docs/content/docs/custom-resource/reference.md:
##########
@@ -96,6 +96,8 @@ This page serves as a full reference for FlinkDeployment custom resource definit
| template | java.lang.String | Ingress template for the JobManager service. |
| className | java.lang.String | Ingress className for the Flink deployment. |
| annotations | java.util.Map<java.lang.String,java.lang.String> | Ingress annotations. |
+| labels | java.util.Map<java.lang.String,java.lang.String> | Ingress labels. |
+| tls | java.util.List<io.fabric8.kubernetes.api.model.networking.v1.IngressTLS> | Ingress tls. |
Review Comment:
Like you said in your review - I think it is fine to only show v1 here.
--
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-33454] [Operator] Add IngressTlsSpec to support TLS within the managed Ingress, also add Label Passthrough [flink-kubernetes-operator]
Posted by "gyfora (via GitHub)" <gi...@apache.org>.
gyfora commented on code in PR #727:
URL: https://github.com/apache/flink-kubernetes-operator/pull/727#discussion_r1422763184
##########
flink-kubernetes-operator-api/src/main/java/org/apache/flink/kubernetes/operator/api/spec/IngressTlsSpec.java:
##########
@@ -0,0 +1,44 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package org.apache.flink.kubernetes.operator.api.spec;
+
+import org.apache.flink.annotation.Experimental;
+
+import com.fasterxml.jackson.annotation.JsonIgnoreProperties;
+import lombok.AllArgsConstructor;
+import lombok.Builder;
+import lombok.Data;
+import lombok.NoArgsConstructor;
+
+import java.util.List;
+
+/** Ingress spec. */
+@Experimental
+@Data
+@NoArgsConstructor
+@AllArgsConstructor
+@Builder
+@JsonIgnoreProperties(ignoreUnknown = true)
+public class IngressTlsSpec {
Review Comment:
Why don't we use ` io.fabric8.kubernetes.api.model.networking.v1.IngressTLS` here directly? We can still convert back to the beta
--
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-33454] [Operator] Add IngressTlsSpec to support TLS within the managed Ingress, also add Label Passthrough [flink-kubernetes-operator]
Posted by "ryanvanhuuksloot (via GitHub)" <gi...@apache.org>.
ryanvanhuuksloot commented on code in PR #727:
URL: https://github.com/apache/flink-kubernetes-operator/pull/727#discussion_r1423103105
##########
flink-kubernetes-operator-api/src/main/java/org/apache/flink/kubernetes/operator/api/spec/IngressTlsSpec.java:
##########
@@ -0,0 +1,44 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package org.apache.flink.kubernetes.operator.api.spec;
+
+import org.apache.flink.annotation.Experimental;
+
+import com.fasterxml.jackson.annotation.JsonIgnoreProperties;
+import lombok.AllArgsConstructor;
+import lombok.Builder;
+import lombok.Data;
+import lombok.NoArgsConstructor;
+
+import java.util.List;
+
+/** Ingress spec. */
+@Experimental
+@Data
+@NoArgsConstructor
+@AllArgsConstructor
+@Builder
+@JsonIgnoreProperties(ignoreUnknown = true)
+public class IngressTlsSpec {
Review Comment:
No particular reason - didn't think about it. Swapped to the kubernetes spec.
--
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-33454] [Operator] Add IngressTlsSpec to support TLS within the managed Ingress, also add Label Passthrough [flink-kubernetes-operator]
Posted by "ryanvanhuuksloot (via GitHub)" <gi...@apache.org>.
ryanvanhuuksloot commented on code in PR #727:
URL: https://github.com/apache/flink-kubernetes-operator/pull/727#discussion_r1423102408
##########
flink-kubernetes-operator/src/main/java/org/apache/flink/kubernetes/operator/utils/IngressUtils.java:
##########
@@ -93,24 +96,45 @@ private static HasMetadata getIngress(
if (ingressInNetworkingV1(client)) {
return new IngressBuilder()
.withNewMetadata()
+ .withLabels(spec.getIngress().getLabels())
.withAnnotations(spec.getIngress().getAnnotations())
.withName(objectMeta.getName())
.withNamespace(objectMeta.getNamespace())
.endMetadata()
.withNewSpec()
.withIngressClassName(spec.getIngress().getClassName())
+ .withTls(spec.getIngress().getTls())
.withRules(getIngressRule(objectMeta, spec, effectiveConfig))
.endSpec()
.build();
} else {
+ List<IngressTLS> ingressTLS =
Review Comment:
Pulled it outside of the return because otherwise it is even more impossible to read.
--
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-33454] [Operator] Add IngressTlsSpec to support TLS within the managed Ingress, also add Label Passthrough [flink-kubernetes-operator]
Posted by "ryanvanhuuksloot (via GitHub)" <gi...@apache.org>.
ryanvanhuuksloot commented on code in PR #727:
URL: https://github.com/apache/flink-kubernetes-operator/pull/727#discussion_r1423102408
##########
flink-kubernetes-operator/src/main/java/org/apache/flink/kubernetes/operator/utils/IngressUtils.java:
##########
@@ -93,24 +96,45 @@ private static HasMetadata getIngress(
if (ingressInNetworkingV1(client)) {
return new IngressBuilder()
.withNewMetadata()
+ .withLabels(spec.getIngress().getLabels())
.withAnnotations(spec.getIngress().getAnnotations())
.withName(objectMeta.getName())
.withNamespace(objectMeta.getNamespace())
.endMetadata()
.withNewSpec()
.withIngressClassName(spec.getIngress().getClassName())
+ .withTls(spec.getIngress().getTls())
.withRules(getIngressRule(objectMeta, spec, effectiveConfig))
.endSpec()
.build();
} else {
+ List<IngressTLS> ingressTLS =
Review Comment:
Pulled it outside of the return because otherwise it was even more impossible to read
--
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-33454] [Operator] Add IngressTls to support TLS within the managed Ingress, also add Label Passthrough [flink-kubernetes-operator]
Posted by "gyfora (via GitHub)" <gi...@apache.org>.
gyfora merged PR #727:
URL: https://github.com/apache/flink-kubernetes-operator/pull/727
--
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