You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@pekko.apache.org by "Iilun (via GitHub)" <gi...@apache.org> on 2024/05/03 15:48:05 UTC
[PR] #217 Support native kubernetes leases [pekko-management]
Iilun opened a new pull request, #218:
URL: https://github.com/apache/pekko-management/pull/218
Following #217, this is a first try at implementing the native Kubernetes lease as lease backend for Pekko coordination.
We tested internally against Kubernetes v1.27, however the `coordination.k8s.io/v1` is served since v1.14 so it seems like a very stable API that should not introduce unexpected behaviors.
Waiting for any feedback on this, thanks !
--
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: notifications-unsubscribe@pekko.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@pekko.apache.org
For additional commands, e-mail: notifications-help@pekko.apache.org
Re: [PR] #217 Support native kubernetes leases [pekko-management]
Posted by "pjfanning (via GitHub)" <gi...@apache.org>.
pjfanning commented on PR #218:
URL: https://github.com/apache/pekko-management/pull/218#issuecomment-2109645059
> Should I also work on modifying/ adding a sh file to test with this lease implementation ?
Thanks @Iilun - I would like to merge this PR as is to avoid letting it get too big. It would be great to have an integration test but could you keep your changes in a new branch for the moment?
--
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: notifications-unsubscribe@pekko.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@pekko.apache.org
For additional commands, e-mail: notifications-help@pekko.apache.org
Re: [PR] #217 Support native kubernetes leases [pekko-management]
Posted by "Iilun (via GitHub)" <gi...@apache.org>.
Iilun commented on code in PR #218:
URL: https://github.com/apache/pekko-management/pull/218#discussion_r1589776177
##########
lease-kubernetes-int-test/src/main/scala/org/apache/pekko/coordination/lease/kubernetes/LeaseSpec.scala:
##########
@@ -16,7 +16,7 @@ package org.apache.pekko.coordination.lease.kubernetes
import org.apache.pekko
import pekko.actor.ActorSystem
import pekko.coordination.lease.TimeoutSettings
-import pekko.coordination.lease.kubernetes.internal.KubernetesApiImpl
+import pekko.coordination.lease.kubernetes.internal.CRDKubernetesApiImpl
Review Comment:
Is it ok for the other renaming or should I also change it (companion object KubernetesLease -> AbstractKubernetesLease)
--
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: notifications-unsubscribe@pekko.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@pekko.apache.org
For additional commands, e-mail: notifications-help@pekko.apache.org
Re: [PR] #217 Support native kubernetes leases [pekko-management]
Posted by "pjfanning (via GitHub)" <gi...@apache.org>.
pjfanning commented on PR #218:
URL: https://github.com/apache/pekko-management/pull/218#issuecomment-2103233979
there are still too many binary compatibility issues
```
[info] done compiling
[error] pekko-lease-kubernetes: Failed binary compatibility check against org.apache.pekko:pekko-lease-kubernetes_2.12:1.0.0! Found 8 potential problems
[error] * abstract method checkLease()Boolean in class org.apache.pekko.coordination.lease.scaladsl.Lease does not have a correspondent in current version
[error] filter with: ProblemFilters.exclude[DirectAbstractMethodProblem]("org.apache.pekko.coordination.lease.scaladsl.Lease.checkLease")
[error] * abstract method release()scala.concurrent.Future in class org.apache.pekko.coordination.lease.scaladsl.Lease does not have a correspondent in current version
[error] filter with: ProblemFilters.exclude[DirectAbstractMethodProblem]("org.apache.pekko.coordination.lease.scaladsl.Lease.release")
[error] * abstract method acquire()scala.concurrent.Future in class org.apache.pekko.coordination.lease.scaladsl.Lease does not have a correspondent in current version
[error] filter with: ProblemFilters.exclude[DirectAbstractMethodProblem]("org.apache.pekko.coordination.lease.scaladsl.Lease.acquire")
[error] * abstract method acquire(scala.Function1)scala.concurrent.Future in class org.apache.pekko.coordination.lease.scaladsl.Lease does not have a correspondent in current version
[error] filter with: ProblemFilters.exclude[DirectAbstractMethodProblem]("org.apache.pekko.coordination.lease.scaladsl.Lease.acquire")
[error] * abstract synthetic method org$apache$pekko$coordination$lease$kubernetes$internal$KubernetesJsonSupport$_setter_$nativeSpecFormat_=(spray.json.JsonFormat)Unit in interface org.apache.pekko.coordination.lease.kubernetes.internal.KubernetesJsonSupport is present only in current version
[error] filter with: ProblemFilters.exclude[ReversedMissingMethodProblem]("org.apache.pekko.coordination.lease.kubernetes.internal.KubernetesJsonSupport.org$apache$pekko$coordination$lease$kubernetes$internal$KubernetesJsonSupport$_setter_$nativeSpecFormat_=")
[error] * abstract synthetic method org$apache$pekko$coordination$lease$kubernetes$internal$KubernetesJsonSupport$_setter_$leaseNativeResourceFormat_=(spray.json.RootJsonFormat)Unit in interface org.apache.pekko.coordination.lease.kubernetes.internal.KubernetesJsonSupport is present only in current version
[error] filter with: ProblemFilters.exclude[ReversedMissingMethodProblem]("org.apache.pekko.coordination.lease.kubernetes.internal.KubernetesJsonSupport.org$apache$pekko$coordination$lease$kubernetes$internal$KubernetesJsonSupport$_setter_$leaseNativeResourceFormat_=")
[error] * abstract method nativeSpecFormat()spray.json.JsonFormat in interface org.apache.pekko.coordination.lease.kubernetes.internal.KubernetesJsonSupport is present only in current version
[error] filter with: ProblemFilters.exclude[ReversedMissingMethodProblem]("org.apache.pekko.coordination.lease.kubernetes.internal.KubernetesJsonSupport.nativeSpecFormat")
[error] * abstract method leaseNativeResourceFormat()spray.json.RootJsonFormat in interface org.apache.pekko.coordination.lease.kubernetes.internal.KubernetesJsonSupport is present only in current version
[error] filter with: ProblemFilters.exclude[ReversedMissingMethodProblem]("org.apache.pekko.coordination.lease.kubernetes.internal.KubernetesJsonSupport.leaseNativeResourceFormat")
[error] java.lang.RuntimeException: Failed binary compatibility check against org.apache.pekko:pekko-lease-kubernetes_2.12:1.0.0! Found 8 potential problems
```
--
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: notifications-unsubscribe@pekko.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@pekko.apache.org
For additional commands, e-mail: notifications-help@pekko.apache.org
Re: [PR] #217 Support native kubernetes leases [pekko-management]
Posted by "laglangyue (via GitHub)" <gi...@apache.org>.
laglangyue commented on code in PR #218:
URL: https://github.com/apache/pekko-management/pull/218#discussion_r1598103627
##########
lease-kubernetes/src/main/scala/org/apache/pekko/coordination/lease/kubernetes/internal/NativeKubernetesApiImpl.scala:
##########
@@ -0,0 +1,196 @@
+/*
+ * 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.pekko.coordination.lease.kubernetes.internal
+
+import org.apache.pekko
+import pekko.actor.ActorSystem
+import pekko.annotation.InternalApi
+import pekko.coordination.lease.kubernetes.internal.NativeKubernetesApiImpl.RFC3339MICRO_FORMATTER
+import pekko.coordination.lease.kubernetes.{ KubernetesSettings, LeaseResource }
+import pekko.coordination.lease.LeaseException
+import pekko.http.scaladsl.marshalling.Marshal
+import pekko.http.scaladsl.model._
+import pekko.http.scaladsl.unmarshalling.Unmarshal
+import java.time.{ Instant, LocalDateTime, ZoneId }
+import java.time.format.{ DateTimeFormatter, DateTimeFormatterBuilder }
+import java.time.temporal.ChronoField
+import scala.concurrent.Future
+
+object NativeKubernetesApiImpl {
+ // From https://github.com/kubernetes-client/java/blob/e50fb2a6f30d4f07e3922430307e5e09058aaea1/kubernetes/src/main/java/io/kubernetes/client/openapi/JSON.java#L57
+ val RFC3339MICRO_FORMATTER: DateTimeFormatter =
+ new DateTimeFormatterBuilder().parseDefaulting(ChronoField.OFFSET_SECONDS,
+ 0).append(DateTimeFormatter.ofPattern("yyyy-MM-dd'T'HH:mm:ss")).optionalStart.appendFraction(
+ ChronoField.NANO_OF_SECOND, 6, 6, true).optionalEnd.appendLiteral("Z").toFormatter
+}
+
+/**
+ * Could be shared between leases: https://github.com/akka/akka-management/issues/680
+ * INTERNAL API
+ */
+@InternalApi private[pekko] class NativeKubernetesApiImpl(system: ActorSystem, settings: KubernetesSettings)
+ extends AbstractKubernetesApiImpl(system, settings) {
+
+ import system.dispatcher
+
+ /**
+ * Update the named resource.
+ *
+ * Must [[readOrCreateLeaseResource]] to first to get a resource version.
+ *
+ * Can return one of three things:
+ * - Future.Failure, e.g. timed out waiting for k8s api server to respond
+ * - Future.sucess[Left(resource)]: the update failed due to version not matching current in the k8s api server.
+ * In this case the current resource is returned so the version can be used for subsequent calls
+ * - Future.sucess[Right(resource)]: Returns the LeaseResource that contains the clientName and new version.
+ * The new version should be used for any subsequent calls
+ */
+ override def updateLeaseResource(
+ leaseName: String,
+ ownerName: String,
+ version: String,
+ time: Long = System.currentTimeMillis()): Future[Either[LeaseResource, LeaseResource]] = {
+ val lcr = NativeLeaseResource(Metadata(leaseName, Some(version)), NativeSpec(ownerName, currentTimeRFC3339))
+ for {
+ entity <- Marshal(lcr).to[RequestEntity]
+ response <- {
+ log.debug("updating {} to {}", leaseName, lcr)
+ makeRequest(
+ requestForPath(pathForLease(leaseName), method = HttpMethods.PUT, entity),
+ s"Timed out updating lease [$leaseName] to owner [$ownerName]. It is not known if the update happened")
+ }
+ result <- response.status match {
+ case StatusCodes.OK =>
+ Unmarshal(response.entity)
+ .to[NativeLeaseResource]
+ .map(updatedLcr => {
+ log.debug("LCR after update: {}", updatedLcr)
+ Right(toLeaseResource(updatedLcr))
+ })
+ case StatusCodes.Conflict =>
+ getLeaseResource(leaseName).flatMap {
+ case None =>
+ Future.failed(
+ new LeaseException(s"GET after PUT conflict did not return a lease. Lease[$leaseName-$ownerName]"))
+ case Some(lr) =>
+ log.debug("LeaseResource read after conflict: {}", lr)
+ Future.successful(Left(lr))
+ }
+ case StatusCodes.Unauthorized =>
+ handleUnauthorized(response)
+ case unexpected =>
+ Unmarshal(response.entity)
+ .to[String]
+ .flatMap(body => {
+ Future.failed(
+ new LeaseException(
+ s"PUT for lease $leaseName returned unexpected status code $unexpected. Body: $body"))
+ })
+ }
+ } yield result
+ }
+
+ override def getLeaseResource(name: String): Future[Option[LeaseResource]] = {
+ val fResponse = makeRequest(requestForPath(pathForLease(name)), s"Timed out reading lease $name")
+ for {
+ response <- fResponse
+ entity <- response.entity.toStrict(settings.bodyReadTimeout)
+ lr <- response.status match {
+ case StatusCodes.OK =>
+ // it exists, parse it
+ log.debug("Resource {} exists: {}", name, entity)
+ Unmarshal(entity)
+ .to[NativeLeaseResource]
+ .map(lcr => {
+ Some(toLeaseResource(lcr))
+ })
+ case StatusCodes.NotFound =>
+ response.discardEntityBytes()
+ log.debug("Resource does not exist: {}", name)
+ Future.successful(None)
+ case StatusCodes.Unauthorized =>
+ handleUnauthorized(response)
+ case unexpected =>
+ Unmarshal(response.entity)
+ .to[String]
+ .flatMap(body => {
+ Future.failed(new LeaseException(
+ s"Unexpected response from API server when retrieving lease StatusCode: $unexpected. Body: $body"))
+ })
+ }
+ } yield lr
+ }
+
+ override def pathForLease(name: String): Uri.Path =
+ Uri.Path.Empty / "apis" / "coordination.k8s.io" / "v1" / "namespaces" / namespace / "leases" / name
+ .replaceAll("[^\\d\\w\\-\\.]", "")
+ .toLowerCase
+
+ override def createLeaseResource(name: String): Future[Option[LeaseResource]] = {
+ val lcr = NativeLeaseResource(Metadata(name, None), NativeSpec("", currentTimeRFC3339))
+ for {
+ entity <- Marshal(lcr).to[RequestEntity]
+ response <- makeRequest(
+ requestForPath(pathForLease(""), HttpMethods.POST, entity = entity),
Review Comment:
Should pathForLease("") be pathForLease(name) ?
--
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: notifications-unsubscribe@pekko.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@pekko.apache.org
For additional commands, e-mail: notifications-help@pekko.apache.org
Re: [PR] #217 Support native kubernetes leases [pekko-management]
Posted by "Iilun (via GitHub)" <gi...@apache.org>.
Iilun commented on code in PR #218:
URL: https://github.com/apache/pekko-management/pull/218#discussion_r1589938447
##########
lease-kubernetes-int-test/src/main/scala/org/apache/pekko/coordination/lease/kubernetes/LeaseSpec.scala:
##########
@@ -16,7 +16,7 @@ package org.apache.pekko.coordination.lease.kubernetes
import org.apache.pekko
import pekko.actor.ActorSystem
import pekko.coordination.lease.TimeoutSettings
-import pekko.coordination.lease.kubernetes.internal.KubernetesApiImpl
+import pekko.coordination.lease.kubernetes.internal.CRDKubernetesApiImpl
Review Comment:
I fixed the issue of the renaming of KubernetesLease -> AbstractKubernetesLease by keeping a child object which references the abstract one, let me know if you like this solution
--
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: notifications-unsubscribe@pekko.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@pekko.apache.org
For additional commands, e-mail: notifications-help@pekko.apache.org
Re: [PR] #217 Support native kubernetes leases [pekko-management]
Posted by "pjfanning (via GitHub)" <gi...@apache.org>.
pjfanning commented on code in PR #218:
URL: https://github.com/apache/pekko-management/pull/218#discussion_r1589618890
##########
docs/src/main/paradox/kubernetes-lease.md:
##########
@@ -30,9 +34,7 @@ different `ActorSystem` names because they all need a separate lease.
- For @extref:[Cluster Singleton](pekko:typed/cluster-singleton.html#lease) there will be one per singleton.
- For @extref:[Cluster Sharding](pekko:typed/cluster-sharding.html#lease), there will be one per shard per type.
-### Configuring
Review Comment:
Can you leave the headings as is? No changes to the depth etc. Exactly as is.
New docs - fine. Messing with existing docs - please, no.
There might be pre-existing links to these 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: notifications-unsubscribe@pekko.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@pekko.apache.org
For additional commands, e-mail: notifications-help@pekko.apache.org
Re: [PR] #217 Support native kubernetes leases [pekko-management]
Posted by "pjfanning (via GitHub)" <gi...@apache.org>.
pjfanning merged PR #218:
URL: https://github.com/apache/pekko-management/pull/218
--
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: notifications-unsubscribe@pekko.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@pekko.apache.org
For additional commands, e-mail: notifications-help@pekko.apache.org
Re: [PR] #217 Support native kubernetes leases [pekko-management]
Posted by "pjfanning (via GitHub)" <gi...@apache.org>.
pjfanning commented on code in PR #218:
URL: https://github.com/apache/pekko-management/pull/218#discussion_r1598329574
##########
lease-kubernetes/src/main/scala/org/apache/pekko/coordination/lease/kubernetes/internal/AbstractKubernetesApiImpl.scala:
##########
@@ -0,0 +1,191 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * license agreements; and to You under the Apache License, version 2.0:
+ *
+ * https://www.apache.org/licenses/LICENSE-2.0
+ *
+ * This file is part of the Apache Pekko project, which was derived from Akka.
+ */
+
+/*
+ * Copyright (C) 2017-2021 Lightbend Inc. <https://www.lightbend.com>
+ */
+
+package org.apache.pekko.coordination.lease.kubernetes.internal
+
+import org.apache.pekko
+import pekko.Done
+import pekko.actor.ActorSystem
+import pekko.annotation.InternalApi
+import pekko.coordination.lease.kubernetes.{ KubernetesApi, KubernetesSettings, LeaseResource }
+import pekko.coordination.lease.{ LeaseException, LeaseTimeoutException }
+import pekko.event.{ LogSource, Logging, LoggingAdapter }
+import pekko.http.scaladsl.model._
+import pekko.http.scaladsl.model.headers.{ Authorization, OAuth2BearerToken }
+import pekko.http.scaladsl.unmarshalling.Unmarshal
+import pekko.http.scaladsl.{ ConnectionContext, Http, HttpExt, HttpsConnectionContext }
+import pekko.pattern.after
+import pekko.pki.kubernetes.PemManagersProvider
+
+import java.nio.charset.StandardCharsets
+import java.nio.file.{ Files, Paths }
+import java.security.{ KeyStore, SecureRandom }
+import javax.net.ssl.{ KeyManager, KeyManagerFactory, SSLContext, TrustManager }
+import scala.collection.immutable
+import scala.concurrent.Future
+import scala.util.control.NonFatal
+
+/**
+ * Could be shared between leases: https://github.com/akka/akka-management/issues/680
+ * INTERNAL API
+ */
+@InternalApi private[pekko] abstract class AbstractKubernetesApiImpl(system: ActorSystem, settings: KubernetesSettings)
+ extends KubernetesApi
+ with KubernetesJsonSupport {
+
+ import system.dispatcher
+
+ protected implicit val sys: ActorSystem = system
+ protected val log: LoggingAdapter = Logging(system, getClass)(LogSource.fromClass)
+ private val http: HttpExt = Http()(system)
+
+ private lazy val sslContext: SSLContext = {
+ val certificates = PemManagersProvider.loadCertificates(settings.apiCaPath)
+ val factory = KeyManagerFactory.getInstance(KeyManagerFactory.getDefaultAlgorithm)
+ val keyStore = KeyStore.getInstance("PKCS12")
+ keyStore.load(null)
+ factory.init(keyStore, Array.empty)
+ val km: Array[KeyManager] = factory.getKeyManagers
+ val tm: Array[TrustManager] =
+ PemManagersProvider.buildTrustManagers(certificates)
+ val random: SecureRandom = new SecureRandom
+ val sslContext = SSLContext.getInstance("TLSv1.2")
+ sslContext.init(km, tm, random)
+ sslContext
+ }
+
+ private lazy val clientSslContext: HttpsConnectionContext = ConnectionContext.httpsClient(sslContext)
+
+ protected val namespace: String =
+ settings.namespace.orElse(readConfigVarFromFilesystem(settings.namespacePath, "namespace")).getOrElse("default")
+
+ protected val scheme: String = if (settings.secure) "https" else "http"
+ private lazy val apiToken = readConfigVarFromFilesystem(settings.apiTokenPath, "api-token").getOrElse("")
+ private lazy val headers = if (settings.secure) immutable.Seq(Authorization(OAuth2BearerToken(apiToken))) else Nil
+
+ log.debug("kubernetes access namespace: {}. Secure: {}", namespace, settings.secure)
+
+ protected def createLeaseResource(name: String): Future[Option[LeaseResource]]
+
+ protected def getLeaseResource(name: String): Future[Option[LeaseResource]]
+
+ protected def pathForLease(name: String): Uri.Path
+
+ override def readOrCreateLeaseResource(name: String): Future[LeaseResource] = {
+ // TODO backoff retry
+ val maxTries = 5
+
+ def loop(tries: Int = 0): Future[LeaseResource] = {
+ log.debug("Trying to create lease {}", tries)
+ for {
+ olr <- getLeaseResource(name)
+ lr <- olr match {
+ case Some(found) =>
+ log.debug("{} already exists. Returning {}", name, found)
+ Future.successful(found)
+ case None =>
+ log.info("lease {} does not exist, creating", name)
+ createLeaseResource(name).flatMap {
+ case Some(created) => Future.successful(created)
+ case None =>
+ if (tries < maxTries) loop(tries + 1)
+ else Future.failed(new LeaseException(s"Unable to create or read lease after $maxTries tries"))
+ }
+ }
+ } yield lr
Review Comment:
This is the correct thing to do.
Code moved from around should not be modified unless absolutely necessary. Non urgent changes to the moved code could be submitted as separate PRs later.
--
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: notifications-unsubscribe@pekko.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@pekko.apache.org
For additional commands, e-mail: notifications-help@pekko.apache.org
Re: [PR] #217 Support native kubernetes leases [pekko-management]
Posted by "Iilun (via GitHub)" <gi...@apache.org>.
Iilun commented on code in PR #218:
URL: https://github.com/apache/pekko-management/pull/218#discussion_r1598322750
##########
lease-kubernetes/src/main/scala/org/apache/pekko/coordination/lease/kubernetes/internal/NativeKubernetesApiImpl.scala:
##########
@@ -0,0 +1,196 @@
+/*
+ * 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.pekko.coordination.lease.kubernetes.internal
+
+import org.apache.pekko
+import pekko.actor.ActorSystem
+import pekko.annotation.InternalApi
+import pekko.coordination.lease.kubernetes.internal.NativeKubernetesApiImpl.RFC3339MICRO_FORMATTER
+import pekko.coordination.lease.kubernetes.{ KubernetesSettings, LeaseResource }
+import pekko.coordination.lease.LeaseException
+import pekko.http.scaladsl.marshalling.Marshal
+import pekko.http.scaladsl.model._
+import pekko.http.scaladsl.unmarshalling.Unmarshal
+import java.time.{ Instant, LocalDateTime, ZoneId }
+import java.time.format.{ DateTimeFormatter, DateTimeFormatterBuilder }
+import java.time.temporal.ChronoField
+import scala.concurrent.Future
+
+object NativeKubernetesApiImpl {
+ // From https://github.com/kubernetes-client/java/blob/e50fb2a6f30d4f07e3922430307e5e09058aaea1/kubernetes/src/main/java/io/kubernetes/client/openapi/JSON.java#L57
+ val RFC3339MICRO_FORMATTER: DateTimeFormatter =
+ new DateTimeFormatterBuilder().parseDefaulting(ChronoField.OFFSET_SECONDS,
+ 0).append(DateTimeFormatter.ofPattern("yyyy-MM-dd'T'HH:mm:ss")).optionalStart.appendFraction(
+ ChronoField.NANO_OF_SECOND, 6, 6, true).optionalEnd.appendLiteral("Z").toFormatter
+}
+
+/**
+ * Could be shared between leases: https://github.com/akka/akka-management/issues/680
+ * INTERNAL API
+ */
+@InternalApi private[pekko] class NativeKubernetesApiImpl(system: ActorSystem, settings: KubernetesSettings)
+ extends AbstractKubernetesApiImpl(system, settings) {
+
+ import system.dispatcher
+
+ /**
+ * Update the named resource.
+ *
+ * Must [[readOrCreateLeaseResource]] to first to get a resource version.
+ *
+ * Can return one of three things:
+ * - Future.Failure, e.g. timed out waiting for k8s api server to respond
+ * - Future.sucess[Left(resource)]: the update failed due to version not matching current in the k8s api server.
+ * In this case the current resource is returned so the version can be used for subsequent calls
+ * - Future.sucess[Right(resource)]: Returns the LeaseResource that contains the clientName and new version.
+ * The new version should be used for any subsequent calls
+ */
+ override def updateLeaseResource(
+ leaseName: String,
+ ownerName: String,
+ version: String,
+ time: Long = System.currentTimeMillis()): Future[Either[LeaseResource, LeaseResource]] = {
+ val lcr = NativeLeaseResource(Metadata(leaseName, Some(version)), NativeSpec(ownerName, currentTimeRFC3339))
+ for {
+ entity <- Marshal(lcr).to[RequestEntity]
+ response <- {
+ log.debug("updating {} to {}", leaseName, lcr)
+ makeRequest(
+ requestForPath(pathForLease(leaseName), method = HttpMethods.PUT, entity),
+ s"Timed out updating lease [$leaseName] to owner [$ownerName]. It is not known if the update happened")
+ }
+ result <- response.status match {
+ case StatusCodes.OK =>
+ Unmarshal(response.entity)
+ .to[NativeLeaseResource]
+ .map(updatedLcr => {
+ log.debug("LCR after update: {}", updatedLcr)
+ Right(toLeaseResource(updatedLcr))
+ })
+ case StatusCodes.Conflict =>
+ getLeaseResource(leaseName).flatMap {
+ case None =>
+ Future.failed(
+ new LeaseException(s"GET after PUT conflict did not return a lease. Lease[$leaseName-$ownerName]"))
+ case Some(lr) =>
+ log.debug("LeaseResource read after conflict: {}", lr)
+ Future.successful(Left(lr))
+ }
+ case StatusCodes.Unauthorized =>
+ handleUnauthorized(response)
+ case unexpected =>
+ Unmarshal(response.entity)
+ .to[String]
+ .flatMap(body => {
+ Future.failed(
+ new LeaseException(
+ s"PUT for lease $leaseName returned unexpected status code $unexpected. Body: $body"))
+ })
+ }
+ } yield result
+ }
+
+ override def getLeaseResource(name: String): Future[Option[LeaseResource]] = {
+ val fResponse = makeRequest(requestForPath(pathForLease(name)), s"Timed out reading lease $name")
+ for {
+ response <- fResponse
+ entity <- response.entity.toStrict(settings.bodyReadTimeout)
+ lr <- response.status match {
+ case StatusCodes.OK =>
+ // it exists, parse it
+ log.debug("Resource {} exists: {}", name, entity)
+ Unmarshal(entity)
+ .to[NativeLeaseResource]
+ .map(lcr => {
+ Some(toLeaseResource(lcr))
+ })
+ case StatusCodes.NotFound =>
+ response.discardEntityBytes()
+ log.debug("Resource does not exist: {}", name)
+ Future.successful(None)
+ case StatusCodes.Unauthorized =>
+ handleUnauthorized(response)
+ case unexpected =>
+ Unmarshal(response.entity)
+ .to[String]
+ .flatMap(body => {
+ Future.failed(new LeaseException(
+ s"Unexpected response from API server when retrieving lease StatusCode: $unexpected. Body: $body"))
+ })
+ }
+ } yield lr
+ }
+
+ override def pathForLease(name: String): Uri.Path =
+ Uri.Path.Empty / "apis" / "coordination.k8s.io" / "v1" / "namespaces" / namespace / "leases" / name
+ .replaceAll("[^\\d\\w\\-\\.]", "")
+ .toLowerCase
+
+ override def createLeaseResource(name: String): Future[Option[LeaseResource]] = {
+ val lcr = NativeLeaseResource(Metadata(name, None), NativeSpec("", currentTimeRFC3339))
+ for {
+ entity <- Marshal(lcr).to[RequestEntity]
+ response <- makeRequest(
+ requestForPath(pathForLease(""), HttpMethods.POST, entity = entity),
Review Comment:
This is intended as the endpoint to create a lease in the [coordination API](https://kubernetes.io/docs/reference/kubernetes-api/cluster-resources/lease-v1/#create-create-a-lease) is `/leases` and not `/leases/name`
--
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: notifications-unsubscribe@pekko.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@pekko.apache.org
For additional commands, e-mail: notifications-help@pekko.apache.org
Re: [PR] #217 Support native kubernetes leases [pekko-management]
Posted by "pjfanning (via GitHub)" <gi...@apache.org>.
pjfanning commented on code in PR #218:
URL: https://github.com/apache/pekko-management/pull/218#discussion_r1589622007
##########
lease-kubernetes/src/test/scala/org/apache/pekko/coordination/lease/kubernetes/NativeKubernetesApiSpec.scala:
##########
@@ -0,0 +1,278 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * license agreements; and to You under the Apache License, version 2.0:
+ *
Review Comment:
new classes should not have Lightbend licenses or the header saying the class is derived from Akka. They should have the standard Apache license.
https://www.apache.org/legal/src-headers.html
--
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: notifications-unsubscribe@pekko.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@pekko.apache.org
For additional commands, e-mail: notifications-help@pekko.apache.org
Re: [PR] #217 Support native kubernetes leases [pekko-management]
Posted by "pjfanning (via GitHub)" <gi...@apache.org>.
pjfanning commented on code in PR #218:
URL: https://github.com/apache/pekko-management/pull/218#discussion_r1589799959
##########
lease-kubernetes/src/main/scala/org/apache/pekko/coordination/lease/kubernetes/internal/KubernetesJsonSupport.scala:
##########
@@ -38,14 +38,32 @@ case class Metadata(name: String, resourceVersion: Option[String])
* INTERNAL API
*/
@InternalApi
-case class Spec(owner: String, time: Long)
+case class CustomSpec(owner: String, time: Long)
+
+/**
+ * INTERNAL API
+ */
+@InternalApi
+case class NativeLeaseResource(
+ metadata: Metadata,
+ spec: NativeSpec,
+ kind: String = "Lease",
+ apiVersion: String = "coordination.k8s.io/v1")
+
+/**
+ * INTERNAL API
+ */
+@InternalApi
+case class NativeSpec(holderIdentity: String, acquireTime: String)
/**
* INTERNAL API
*/
@InternalApi
trait KubernetesJsonSupport extends SprayJsonSupport with DefaultJsonProtocol {
implicit val metadataFormat: JsonFormat[Metadata] = jsonFormat2(Metadata.apply)
- implicit val specFormat: JsonFormat[Spec] = jsonFormat2(Spec.apply)
+ implicit val customSpecFormat: JsonFormat[CustomSpec] = jsonFormat2(CustomSpec.apply)
Review Comment:
again - don't rename existing classes or fields
--
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: notifications-unsubscribe@pekko.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@pekko.apache.org
For additional commands, e-mail: notifications-help@pekko.apache.org
Re: [PR] #217 Support native kubernetes leases [pekko-management]
Posted by "Iilun (via GitHub)" <gi...@apache.org>.
Iilun commented on code in PR #218:
URL: https://github.com/apache/pekko-management/pull/218#discussion_r1589783678
##########
lease-kubernetes-int-test/src/main/scala/org/apache/pekko/coordination/lease/kubernetes/LeaseSpec.scala:
##########
@@ -16,7 +16,7 @@ package org.apache.pekko.coordination.lease.kubernetes
import org.apache.pekko
import pekko.actor.ActorSystem
import pekko.coordination.lease.TimeoutSettings
-import pekko.coordination.lease.kubernetes.internal.KubernetesApiImpl
+import pekko.coordination.lease.kubernetes.internal.CRDKubernetesApiImpl
Review Comment:
Just ran it, I only have the two MissingClassProblem for my added classes KubernetesNativeLease and KubernetesApiNativeImpl
--
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: notifications-unsubscribe@pekko.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@pekko.apache.org
For additional commands, e-mail: notifications-help@pekko.apache.org
Re: [PR] #217 Support native kubernetes leases [pekko-management]
Posted by "Iilun (via GitHub)" <gi...@apache.org>.
Iilun commented on code in PR #218:
URL: https://github.com/apache/pekko-management/pull/218#discussion_r1589783678
##########
lease-kubernetes-int-test/src/main/scala/org/apache/pekko/coordination/lease/kubernetes/LeaseSpec.scala:
##########
@@ -16,7 +16,7 @@ package org.apache.pekko.coordination.lease.kubernetes
import org.apache.pekko
import pekko.actor.ActorSystem
import pekko.coordination.lease.TimeoutSettings
-import pekko.coordination.lease.kubernetes.internal.KubernetesApiImpl
+import pekko.coordination.lease.kubernetes.internal.CRDKubernetesApiImpl
Review Comment:
Just ran it, I only have the two MissingClassProblem for my added classes KubernetesNativeLease and KubernetesApiNativeImpl
--
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: notifications-unsubscribe@pekko.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@pekko.apache.org
For additional commands, e-mail: notifications-help@pekko.apache.org
Re: [PR] #217 Support native kubernetes leases [pekko-management]
Posted by "laglangyue (via GitHub)" <gi...@apache.org>.
laglangyue commented on code in PR #218:
URL: https://github.com/apache/pekko-management/pull/218#discussion_r1598099762
##########
lease-kubernetes/src/main/scala/org/apache/pekko/coordination/lease/kubernetes/internal/NativeKubernetesApiImpl.scala:
##########
@@ -0,0 +1,196 @@
+/*
+ * 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.pekko.coordination.lease.kubernetes.internal
+
+import org.apache.pekko
+import pekko.actor.ActorSystem
+import pekko.annotation.InternalApi
+import pekko.coordination.lease.kubernetes.internal.NativeKubernetesApiImpl.RFC3339MICRO_FORMATTER
+import pekko.coordination.lease.kubernetes.{ KubernetesSettings, LeaseResource }
+import pekko.coordination.lease.LeaseException
+import pekko.http.scaladsl.marshalling.Marshal
+import pekko.http.scaladsl.model._
+import pekko.http.scaladsl.unmarshalling.Unmarshal
+import java.time.{ Instant, LocalDateTime, ZoneId }
+import java.time.format.{ DateTimeFormatter, DateTimeFormatterBuilder }
+import java.time.temporal.ChronoField
+import scala.concurrent.Future
+
+object NativeKubernetesApiImpl {
+ // From https://github.com/kubernetes-client/java/blob/e50fb2a6f30d4f07e3922430307e5e09058aaea1/kubernetes/src/main/java/io/kubernetes/client/openapi/JSON.java#L57
+ val RFC3339MICRO_FORMATTER: DateTimeFormatter =
+ new DateTimeFormatterBuilder().parseDefaulting(ChronoField.OFFSET_SECONDS,
+ 0).append(DateTimeFormatter.ofPattern("yyyy-MM-dd'T'HH:mm:ss")).optionalStart.appendFraction(
+ ChronoField.NANO_OF_SECOND, 6, 6, true).optionalEnd.appendLiteral("Z").toFormatter
+}
+
+/**
+ * Could be shared between leases: https://github.com/akka/akka-management/issues/680
+ * INTERNAL API
+ */
+@InternalApi private[pekko] class NativeKubernetesApiImpl(system: ActorSystem, settings: KubernetesSettings)
+ extends AbstractKubernetesApiImpl(system, settings) {
+
+ import system.dispatcher
+
+ /**
+ * Update the named resource.
+ *
+ * Must [[readOrCreateLeaseResource]] to first to get a resource version.
+ *
+ * Can return one of three things:
+ * - Future.Failure, e.g. timed out waiting for k8s api server to respond
+ * - Future.sucess[Left(resource)]: the update failed due to version not matching current in the k8s api server.
+ * In this case the current resource is returned so the version can be used for subsequent calls
+ * - Future.sucess[Right(resource)]: Returns the LeaseResource that contains the clientName and new version.
+ * The new version should be used for any subsequent calls
+ */
+ override def updateLeaseResource(
+ leaseName: String,
+ ownerName: String,
+ version: String,
+ time: Long = System.currentTimeMillis()): Future[Either[LeaseResource, LeaseResource]] = {
+ val lcr = NativeLeaseResource(Metadata(leaseName, Some(version)), NativeSpec(ownerName, currentTimeRFC3339))
+ for {
+ entity <- Marshal(lcr).to[RequestEntity]
+ response <- {
+ log.debug("updating {} to {}", leaseName, lcr)
+ makeRequest(
+ requestForPath(pathForLease(leaseName), method = HttpMethods.PUT, entity),
+ s"Timed out updating lease [$leaseName] to owner [$ownerName]. It is not known if the update happened")
+ }
+ result <- response.status match {
+ case StatusCodes.OK =>
+ Unmarshal(response.entity)
+ .to[NativeLeaseResource]
+ .map(updatedLcr => {
+ log.debug("LCR after update: {}", updatedLcr)
+ Right(toLeaseResource(updatedLcr))
+ })
+ case StatusCodes.Conflict =>
+ getLeaseResource(leaseName).flatMap {
+ case None =>
+ Future.failed(
+ new LeaseException(s"GET after PUT conflict did not return a lease. Lease[$leaseName-$ownerName]"))
+ case Some(lr) =>
+ log.debug("LeaseResource read after conflict: {}", lr)
+ Future.successful(Left(lr))
+ }
+ case StatusCodes.Unauthorized =>
+ handleUnauthorized(response)
+ case unexpected =>
+ Unmarshal(response.entity)
+ .to[String]
+ .flatMap(body => {
+ Future.failed(
+ new LeaseException(
+ s"PUT for lease $leaseName returned unexpected status code $unexpected. Body: $body"))
+ })
+ }
+ } yield result
+ }
+
+ override def getLeaseResource(name: String): Future[Option[LeaseResource]] = {
+ val fResponse = makeRequest(requestForPath(pathForLease(name)), s"Timed out reading lease $name")
+ for {
+ response <- fResponse
Review Comment:
- what is meaning of ``response <- fResponse`?
- It seems to be directly return in case, and remove for-yield
--
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: notifications-unsubscribe@pekko.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@pekko.apache.org
For additional commands, e-mail: notifications-help@pekko.apache.org
Re: [PR] #217 Support native kubernetes leases [pekko-management]
Posted by "pjfanning (via GitHub)" <gi...@apache.org>.
pjfanning commented on PR #218:
URL: https://github.com/apache/pekko-management/pull/218#issuecomment-2109844830
@lilun I merged this - thanks. If you could look at doing a new PR with an integration test that would be much appreciated.
--
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: notifications-unsubscribe@pekko.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@pekko.apache.org
For additional commands, e-mail: notifications-help@pekko.apache.org
Re: [PR] #217 Support native kubernetes leases [pekko-management]
Posted by "Iilun (via GitHub)" <gi...@apache.org>.
Iilun commented on code in PR #218:
URL: https://github.com/apache/pekko-management/pull/218#discussion_r1599011869
##########
lease-kubernetes/src/main/mima-filters/1.1.x.backwards.excludes/kubernetes-lease.backwards.excludes:
##########
@@ -0,0 +1,22 @@
+# 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.
+
+# The new Kubernetes Native Lease adds new classes and fields
+ProblemFilters.exclude[ReversedMissingMethodProblem]("org.apache.pekko.http.scaladsl.settings.Http2ServerSettings.frameTypeThrottleFrameTypes")
Review Comment:
thanks for the commit cleaning I don't know what I did but it wasn't good
--
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: notifications-unsubscribe@pekko.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@pekko.apache.org
For additional commands, e-mail: notifications-help@pekko.apache.org
Re: [PR] #217 Support native kubernetes leases [pekko-management]
Posted by "Iilun (via GitHub)" <gi...@apache.org>.
Iilun commented on code in PR #218:
URL: https://github.com/apache/pekko-management/pull/218#discussion_r1599011869
##########
lease-kubernetes/src/main/mima-filters/1.1.x.backwards.excludes/kubernetes-lease.backwards.excludes:
##########
@@ -0,0 +1,22 @@
+# 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.
+
+# The new Kubernetes Native Lease adds new classes and fields
+ProblemFilters.exclude[ReversedMissingMethodProblem]("org.apache.pekko.http.scaladsl.settings.Http2ServerSettings.frameTypeThrottleFrameTypes")
Review Comment:
thanks for the commit cleaning I don't know what I did but it wasn't good
--
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: notifications-unsubscribe@pekko.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@pekko.apache.org
For additional commands, e-mail: notifications-help@pekko.apache.org
Re: [PR] #217 Support native kubernetes leases [pekko-management]
Posted by "Iilun (via GitHub)" <gi...@apache.org>.
Iilun commented on code in PR #218:
URL: https://github.com/apache/pekko-management/pull/218#discussion_r1599008110
##########
lease-kubernetes/src/main/mima-filters/1.1.x.backwards.excludes/kubernetes-lease.backwards.excludes:
##########
@@ -0,0 +1,22 @@
+# 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.
+
+# The new Kubernetes Native Lease adds new classes and fields
+ProblemFilters.exclude[ReversedMissingMethodProblem]("org.apache.pekko.http.scaladsl.settings.Http2ServerSettings.frameTypeThrottleFrameTypes")
Review Comment:
I filled it with the problems mentioned on the mima check. Thanks for the commit
--
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: notifications-unsubscribe@pekko.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@pekko.apache.org
For additional commands, e-mail: notifications-help@pekko.apache.org
Re: [PR] #217 Support native kubernetes leases [pekko-management]
Posted by "Iilun (via GitHub)" <gi...@apache.org>.
Iilun commented on PR #218:
URL: https://github.com/apache/pekko-management/pull/218#issuecomment-2109925217
I'll work on it in the coming days
--
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: notifications-unsubscribe@pekko.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@pekko.apache.org
For additional commands, e-mail: notifications-help@pekko.apache.org
Re: [PR] #217 Support native kubernetes leases [pekko-management]
Posted by "pjfanning (via GitHub)" <gi...@apache.org>.
pjfanning commented on code in PR #218:
URL: https://github.com/apache/pekko-management/pull/218#discussion_r1598995634
##########
lease-kubernetes/src/main/mima-filters/1.1.x.backwards.excludes/kubernetes-lease.backwards.excludes:
##########
@@ -0,0 +1,22 @@
+# 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.
+
+# The new Kubernetes Native Lease adds new classes and fields
+ProblemFilters.exclude[ReversedMissingMethodProblem]("org.apache.pekko.http.scaladsl.settings.Http2ServerSettings.frameTypeThrottleFrameTypes")
Review Comment:
@Iilun I pushed this file to your PR to try to get the mima check to pass
--
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: notifications-unsubscribe@pekko.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@pekko.apache.org
For additional commands, e-mail: notifications-help@pekko.apache.org
Re: [PR] #217 Support native kubernetes leases [pekko-management]
Posted by "Iilun (via GitHub)" <gi...@apache.org>.
Iilun commented on code in PR #218:
URL: https://github.com/apache/pekko-management/pull/218#discussion_r1589936546
##########
lease-kubernetes/src/test/scala/org/apache/pekko/coordination/lease/kubernetes/NativeKubernetesApiSpec.scala:
##########
@@ -0,0 +1,282 @@
+/*
+ * 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.pekko.coordination.lease.kubernetes
+
+import com.github.tomakehurst.wiremock.WireMockServer
+import com.github.tomakehurst.wiremock.client.WireMock
+import com.github.tomakehurst.wiremock.client.WireMock._
+import com.github.tomakehurst.wiremock.core.WireMockConfiguration.wireMockConfig
+import com.typesafe.config.ConfigFactory
+import org.apache.pekko.Done
Review Comment:
I changed it, for the overall formatting of imports however I have a hard time finding a common formatting in the tests (differs between all 3 previous tests for this project), so I have just fixed the pekko style issue
##########
lease-kubernetes/src/main/scala/org/apache/pekko/coordination/lease/kubernetes/internal/KubernetesJsonSupport.scala:
##########
@@ -38,14 +38,32 @@ case class Metadata(name: String, resourceVersion: Option[String])
* INTERNAL API
*/
@InternalApi
-case class Spec(owner: String, time: Long)
+case class CustomSpec(owner: String, time: Long)
+
+/**
+ * INTERNAL API
+ */
+@InternalApi
+case class NativeLeaseResource(
+ metadata: Metadata,
+ spec: NativeSpec,
+ kind: String = "Lease",
+ apiVersion: String = "coordination.k8s.io/v1")
+
+/**
+ * INTERNAL API
+ */
+@InternalApi
+case class NativeSpec(holderIdentity: String, acquireTime: String)
/**
* INTERNAL API
*/
@InternalApi
trait KubernetesJsonSupport extends SprayJsonSupport with DefaultJsonProtocol {
implicit val metadataFormat: JsonFormat[Metadata] = jsonFormat2(Metadata.apply)
- implicit val specFormat: JsonFormat[Spec] = jsonFormat2(Spec.apply)
+ implicit val customSpecFormat: JsonFormat[CustomSpec] = jsonFormat2(CustomSpec.apply)
Review Comment:
Reverted
--
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: notifications-unsubscribe@pekko.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@pekko.apache.org
For additional commands, e-mail: notifications-help@pekko.apache.org
Re: [PR] #217 Support native kubernetes leases [pekko-management]
Posted by "laglangyue (via GitHub)" <gi...@apache.org>.
laglangyue commented on code in PR #218:
URL: https://github.com/apache/pekko-management/pull/218#discussion_r1598052777
##########
lease-kubernetes/src/main/scala/org/apache/pekko/coordination/lease/kubernetes/internal/AbstractKubernetesApiImpl.scala:
##########
@@ -0,0 +1,191 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * license agreements; and to You under the Apache License, version 2.0:
+ *
+ * https://www.apache.org/licenses/LICENSE-2.0
+ *
+ * This file is part of the Apache Pekko project, which was derived from Akka.
+ */
+
+/*
+ * Copyright (C) 2017-2021 Lightbend Inc. <https://www.lightbend.com>
+ */
+
+package org.apache.pekko.coordination.lease.kubernetes.internal
+
+import org.apache.pekko
+import pekko.Done
+import pekko.actor.ActorSystem
+import pekko.annotation.InternalApi
+import pekko.coordination.lease.kubernetes.{ KubernetesApi, KubernetesSettings, LeaseResource }
+import pekko.coordination.lease.{ LeaseException, LeaseTimeoutException }
+import pekko.event.{ LogSource, Logging, LoggingAdapter }
+import pekko.http.scaladsl.model._
+import pekko.http.scaladsl.model.headers.{ Authorization, OAuth2BearerToken }
+import pekko.http.scaladsl.unmarshalling.Unmarshal
+import pekko.http.scaladsl.{ ConnectionContext, Http, HttpExt, HttpsConnectionContext }
+import pekko.pattern.after
+import pekko.pki.kubernetes.PemManagersProvider
+
+import java.nio.charset.StandardCharsets
+import java.nio.file.{ Files, Paths }
+import java.security.{ KeyStore, SecureRandom }
+import javax.net.ssl.{ KeyManager, KeyManagerFactory, SSLContext, TrustManager }
+import scala.collection.immutable
+import scala.concurrent.Future
+import scala.util.control.NonFatal
+
+/**
+ * Could be shared between leases: https://github.com/akka/akka-management/issues/680
+ * INTERNAL API
+ */
+@InternalApi private[pekko] abstract class AbstractKubernetesApiImpl(system: ActorSystem, settings: KubernetesSettings)
+ extends KubernetesApi
+ with KubernetesJsonSupport {
+
+ import system.dispatcher
+
+ protected implicit val sys: ActorSystem = system
+ protected val log: LoggingAdapter = Logging(system, getClass)(LogSource.fromClass)
+ private val http: HttpExt = Http()(system)
+
+ private lazy val sslContext: SSLContext = {
+ val certificates = PemManagersProvider.loadCertificates(settings.apiCaPath)
+ val factory = KeyManagerFactory.getInstance(KeyManagerFactory.getDefaultAlgorithm)
+ val keyStore = KeyStore.getInstance("PKCS12")
+ keyStore.load(null)
+ factory.init(keyStore, Array.empty)
+ val km: Array[KeyManager] = factory.getKeyManagers
+ val tm: Array[TrustManager] =
+ PemManagersProvider.buildTrustManagers(certificates)
+ val random: SecureRandom = new SecureRandom
+ val sslContext = SSLContext.getInstance("TLSv1.2")
+ sslContext.init(km, tm, random)
+ sslContext
+ }
+
+ private lazy val clientSslContext: HttpsConnectionContext = ConnectionContext.httpsClient(sslContext)
+
+ protected val namespace: String =
+ settings.namespace.orElse(readConfigVarFromFilesystem(settings.namespacePath, "namespace")).getOrElse("default")
+
+ protected val scheme: String = if (settings.secure) "https" else "http"
+ private lazy val apiToken = readConfigVarFromFilesystem(settings.apiTokenPath, "api-token").getOrElse("")
+ private lazy val headers = if (settings.secure) immutable.Seq(Authorization(OAuth2BearerToken(apiToken))) else Nil
+
+ log.debug("kubernetes access namespace: {}. Secure: {}", namespace, settings.secure)
+
+ protected def createLeaseResource(name: String): Future[Option[LeaseResource]]
+
+ protected def getLeaseResource(name: String): Future[Option[LeaseResource]]
+
+ protected def pathForLease(name: String): Uri.Path
+
+ override def readOrCreateLeaseResource(name: String): Future[LeaseResource] = {
+ // TODO backoff retry
+ val maxTries = 5
Review Comment:
could it be a configraturable value ?
--
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: notifications-unsubscribe@pekko.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@pekko.apache.org
For additional commands, e-mail: notifications-help@pekko.apache.org
Re: [PR] #217 Support native kubernetes leases [pekko-management]
Posted by "pjfanning (via GitHub)" <gi...@apache.org>.
pjfanning commented on code in PR #218:
URL: https://github.com/apache/pekko-management/pull/218#discussion_r1589800238
##########
lease-kubernetes/src/main/scala/org/apache/pekko/coordination/lease/kubernetes/internal/NativeKubernetesApiImpl.scala:
##########
@@ -0,0 +1,195 @@
+/*
+ * 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.pekko.coordination.lease.kubernetes.internal
+
+import org.apache.pekko.actor.ActorSystem
Review Comment:
the style is import org.apache.pekko on 1 line
and then import pekko..... in the following lines - see all other source files
--
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: notifications-unsubscribe@pekko.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@pekko.apache.org
For additional commands, e-mail: notifications-help@pekko.apache.org
Re: [PR] #217 Support native kubernetes leases [pekko-management]
Posted by "pjfanning (via GitHub)" <gi...@apache.org>.
pjfanning commented on code in PR #218:
URL: https://github.com/apache/pekko-management/pull/218#discussion_r1589799607
##########
lease-kubernetes/src/main/scala/org/apache/pekko/coordination/lease/kubernetes/internal/KubernetesJsonSupport.scala:
##########
@@ -38,14 +38,32 @@ case class Metadata(name: String, resourceVersion: Option[String])
* INTERNAL API
*/
@InternalApi
-case class Spec(owner: String, time: Long)
+case class CustomSpec(owner: String, time: Long)
Review Comment:
please do not rename this class
--
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: notifications-unsubscribe@pekko.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@pekko.apache.org
For additional commands, e-mail: notifications-help@pekko.apache.org
Re: [PR] #217 Support native kubernetes leases [pekko-management]
Posted by "pjfanning (via GitHub)" <gi...@apache.org>.
pjfanning commented on PR #218:
URL: https://github.com/apache/pekko-management/pull/218#issuecomment-2108684840
@Iilun I tidied up the git history again - your merge to the branch duplicated a number of commits
--
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: notifications-unsubscribe@pekko.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@pekko.apache.org
For additional commands, e-mail: notifications-help@pekko.apache.org
Re: [PR] #217 Support native kubernetes leases [pekko-management]
Posted by "pjfanning (via GitHub)" <gi...@apache.org>.
pjfanning commented on code in PR #218:
URL: https://github.com/apache/pekko-management/pull/218#discussion_r1598086741
##########
lease-kubernetes/src/main/scala/org/apache/pekko/coordination/lease/kubernetes/AbstractKubernetesLease.scala:
##########
@@ -0,0 +1,123 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * license agreements; and to You under the Apache License, version 2.0:
+ *
+ * https://www.apache.org/licenses/LICENSE-2.0
+ *
+ * This file is part of the Apache Pekko project, which was derived from Akka.
+ */
+
+/*
+ * Copyright (C) 2017-2021 Lightbend Inc. <https://www.lightbend.com>
Review Comment:
New source files that contain code copied from other source files need to retain the existing header. So the PR has the right header.
Only source files that have brand new code should have the standard Apache header.
--
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: notifications-unsubscribe@pekko.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@pekko.apache.org
For additional commands, e-mail: notifications-help@pekko.apache.org
Re: [PR] #217 Support native kubernetes leases [pekko-management]
Posted by "Iilun (via GitHub)" <gi...@apache.org>.
Iilun commented on PR #218:
URL: https://github.com/apache/pekko-management/pull/218#issuecomment-2109511214
Should I also work on modifying/ adding a sh file to test with this lease implementation ?
--
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: notifications-unsubscribe@pekko.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@pekko.apache.org
For additional commands, e-mail: notifications-help@pekko.apache.org
Re: [PR] #217 Support native kubernetes leases [pekko-management]
Posted by "pjfanning (via GitHub)" <gi...@apache.org>.
pjfanning commented on code in PR #218:
URL: https://github.com/apache/pekko-management/pull/218#discussion_r1589620423
##########
lease-kubernetes-int-test/src/main/scala/org/apache/pekko/coordination/lease/kubernetes/LeaseSpec.scala:
##########
@@ -16,7 +16,7 @@ package org.apache.pekko.coordination.lease.kubernetes
import org.apache.pekko
import pekko.actor.ActorSystem
import pekko.coordination.lease.TimeoutSettings
-import pekko.coordination.lease.kubernetes.internal.KubernetesApiImpl
+import pekko.coordination.lease.kubernetes.internal.CRDKubernetesApiImpl
Review Comment:
-1 to the class rename
Like it or not, we cannot just rename classes or repurpose existing classes to do something else.
--
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: notifications-unsubscribe@pekko.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@pekko.apache.org
For additional commands, e-mail: notifications-help@pekko.apache.org
Re: [PR] #217 Support native kubernetes leases [pekko-management]
Posted by "pjfanning (via GitHub)" <gi...@apache.org>.
pjfanning commented on code in PR #218:
URL: https://github.com/apache/pekko-management/pull/218#discussion_r1589784522
##########
lease-kubernetes/src/test/scala/org/apache/pekko/coordination/lease/kubernetes/NativeKubernetesApiSpec.scala:
##########
@@ -0,0 +1,278 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * license agreements; and to You under the Apache License, version 2.0:
+ *
Review Comment:
any 'new' source files that contain code that was moved from existing source files should retain the headers from the original source files - even if the old code that was moved into them is only a small part of the new file
--
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: notifications-unsubscribe@pekko.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@pekko.apache.org
For additional commands, e-mail: notifications-help@pekko.apache.org
Re: [PR] #217 Support native kubernetes leases [pekko-management]
Posted by "Iilun (via GitHub)" <gi...@apache.org>.
Iilun commented on code in PR #218:
URL: https://github.com/apache/pekko-management/pull/218#discussion_r1589778326
##########
lease-kubernetes/src/test/scala/org/apache/pekko/coordination/lease/kubernetes/NativeKubernetesApiSpec.scala:
##########
@@ -0,0 +1,278 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * license agreements; and to You under the Apache License, version 2.0:
+ *
Review Comment:
Done for the strictly new classes, should it also be done for the classes that are essentially moved code ? I could also move no code and just copy it but i did want to put common code in common.
--
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: notifications-unsubscribe@pekko.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@pekko.apache.org
For additional commands, e-mail: notifications-help@pekko.apache.org
Re: [PR] #217 Support native kubernetes leases [pekko-management]
Posted by "Iilun (via GitHub)" <gi...@apache.org>.
Iilun commented on code in PR #218:
URL: https://github.com/apache/pekko-management/pull/218#discussion_r1598325966
##########
lease-kubernetes/src/main/scala/org/apache/pekko/coordination/lease/kubernetes/internal/NativeKubernetesApiImpl.scala:
##########
@@ -0,0 +1,196 @@
+/*
+ * 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.pekko.coordination.lease.kubernetes.internal
+
+import org.apache.pekko
+import pekko.actor.ActorSystem
+import pekko.annotation.InternalApi
+import pekko.coordination.lease.kubernetes.internal.NativeKubernetesApiImpl.RFC3339MICRO_FORMATTER
+import pekko.coordination.lease.kubernetes.{ KubernetesSettings, LeaseResource }
+import pekko.coordination.lease.LeaseException
+import pekko.http.scaladsl.marshalling.Marshal
+import pekko.http.scaladsl.model._
+import pekko.http.scaladsl.unmarshalling.Unmarshal
+import java.time.{ Instant, LocalDateTime, ZoneId }
+import java.time.format.{ DateTimeFormatter, DateTimeFormatterBuilder }
+import java.time.temporal.ChronoField
+import scala.concurrent.Future
+
+object NativeKubernetesApiImpl {
+ // From https://github.com/kubernetes-client/java/blob/e50fb2a6f30d4f07e3922430307e5e09058aaea1/kubernetes/src/main/java/io/kubernetes/client/openapi/JSON.java#L57
+ val RFC3339MICRO_FORMATTER: DateTimeFormatter =
+ new DateTimeFormatterBuilder().parseDefaulting(ChronoField.OFFSET_SECONDS,
+ 0).append(DateTimeFormatter.ofPattern("yyyy-MM-dd'T'HH:mm:ss")).optionalStart.appendFraction(
+ ChronoField.NANO_OF_SECOND, 6, 6, true).optionalEnd.appendLiteral("Z").toFormatter
+}
+
+/**
+ * Could be shared between leases: https://github.com/akka/akka-management/issues/680
+ * INTERNAL API
+ */
+@InternalApi private[pekko] class NativeKubernetesApiImpl(system: ActorSystem, settings: KubernetesSettings)
+ extends AbstractKubernetesApiImpl(system, settings) {
+
+ import system.dispatcher
+
+ /**
+ * Update the named resource.
+ *
+ * Must [[readOrCreateLeaseResource]] to first to get a resource version.
+ *
+ * Can return one of three things:
+ * - Future.Failure, e.g. timed out waiting for k8s api server to respond
+ * - Future.sucess[Left(resource)]: the update failed due to version not matching current in the k8s api server.
+ * In this case the current resource is returned so the version can be used for subsequent calls
+ * - Future.sucess[Right(resource)]: Returns the LeaseResource that contains the clientName and new version.
+ * The new version should be used for any subsequent calls
+ */
+ override def updateLeaseResource(
+ leaseName: String,
+ ownerName: String,
+ version: String,
+ time: Long = System.currentTimeMillis()): Future[Either[LeaseResource, LeaseResource]] = {
+ val lcr = NativeLeaseResource(Metadata(leaseName, Some(version)), NativeSpec(ownerName, currentTimeRFC3339))
+ for {
+ entity <- Marshal(lcr).to[RequestEntity]
+ response <- {
+ log.debug("updating {} to {}", leaseName, lcr)
+ makeRequest(
+ requestForPath(pathForLease(leaseName), method = HttpMethods.PUT, entity),
+ s"Timed out updating lease [$leaseName] to owner [$ownerName]. It is not known if the update happened")
+ }
+ result <- response.status match {
+ case StatusCodes.OK =>
+ Unmarshal(response.entity)
+ .to[NativeLeaseResource]
+ .map(updatedLcr => {
+ log.debug("LCR after update: {}", updatedLcr)
+ Right(toLeaseResource(updatedLcr))
+ })
+ case StatusCodes.Conflict =>
+ getLeaseResource(leaseName).flatMap {
+ case None =>
+ Future.failed(
+ new LeaseException(s"GET after PUT conflict did not return a lease. Lease[$leaseName-$ownerName]"))
+ case Some(lr) =>
+ log.debug("LeaseResource read after conflict: {}", lr)
+ Future.successful(Left(lr))
+ }
+ case StatusCodes.Unauthorized =>
+ handleUnauthorized(response)
+ case unexpected =>
+ Unmarshal(response.entity)
+ .to[String]
+ .flatMap(body => {
+ Future.failed(
+ new LeaseException(
+ s"PUT for lease $leaseName returned unexpected status code $unexpected. Body: $body"))
+ })
+ }
+ } yield result
+ }
+
+ override def getLeaseResource(name: String): Future[Option[LeaseResource]] = {
+ val fResponse = makeRequest(requestForPath(pathForLease(name)), s"Timed out reading lease $name")
+ for {
+ response <- fResponse
Review Comment:
This is the syntax used by the existing class (`KubernetesApiImpl`) to handle the futures
##########
lease-kubernetes/src/main/scala/org/apache/pekko/coordination/lease/kubernetes/internal/AbstractKubernetesApiImpl.scala:
##########
@@ -0,0 +1,191 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * license agreements; and to You under the Apache License, version 2.0:
+ *
+ * https://www.apache.org/licenses/LICENSE-2.0
+ *
+ * This file is part of the Apache Pekko project, which was derived from Akka.
+ */
+
+/*
+ * Copyright (C) 2017-2021 Lightbend Inc. <https://www.lightbend.com>
+ */
+
+package org.apache.pekko.coordination.lease.kubernetes.internal
+
+import org.apache.pekko
+import pekko.Done
+import pekko.actor.ActorSystem
+import pekko.annotation.InternalApi
+import pekko.coordination.lease.kubernetes.{ KubernetesApi, KubernetesSettings, LeaseResource }
+import pekko.coordination.lease.{ LeaseException, LeaseTimeoutException }
+import pekko.event.{ LogSource, Logging, LoggingAdapter }
+import pekko.http.scaladsl.model._
+import pekko.http.scaladsl.model.headers.{ Authorization, OAuth2BearerToken }
+import pekko.http.scaladsl.unmarshalling.Unmarshal
+import pekko.http.scaladsl.{ ConnectionContext, Http, HttpExt, HttpsConnectionContext }
+import pekko.pattern.after
+import pekko.pki.kubernetes.PemManagersProvider
+
+import java.nio.charset.StandardCharsets
+import java.nio.file.{ Files, Paths }
+import java.security.{ KeyStore, SecureRandom }
+import javax.net.ssl.{ KeyManager, KeyManagerFactory, SSLContext, TrustManager }
+import scala.collection.immutable
+import scala.concurrent.Future
+import scala.util.control.NonFatal
+
+/**
+ * Could be shared between leases: https://github.com/akka/akka-management/issues/680
+ * INTERNAL API
+ */
+@InternalApi private[pekko] abstract class AbstractKubernetesApiImpl(system: ActorSystem, settings: KubernetesSettings)
+ extends KubernetesApi
+ with KubernetesJsonSupport {
+
+ import system.dispatcher
+
+ protected implicit val sys: ActorSystem = system
+ protected val log: LoggingAdapter = Logging(system, getClass)(LogSource.fromClass)
+ private val http: HttpExt = Http()(system)
+
+ private lazy val sslContext: SSLContext = {
+ val certificates = PemManagersProvider.loadCertificates(settings.apiCaPath)
+ val factory = KeyManagerFactory.getInstance(KeyManagerFactory.getDefaultAlgorithm)
+ val keyStore = KeyStore.getInstance("PKCS12")
+ keyStore.load(null)
+ factory.init(keyStore, Array.empty)
+ val km: Array[KeyManager] = factory.getKeyManagers
+ val tm: Array[TrustManager] =
+ PemManagersProvider.buildTrustManagers(certificates)
+ val random: SecureRandom = new SecureRandom
+ val sslContext = SSLContext.getInstance("TLSv1.2")
+ sslContext.init(km, tm, random)
+ sslContext
+ }
+
+ private lazy val clientSslContext: HttpsConnectionContext = ConnectionContext.httpsClient(sslContext)
+
+ protected val namespace: String =
+ settings.namespace.orElse(readConfigVarFromFilesystem(settings.namespacePath, "namespace")).getOrElse("default")
+
+ protected val scheme: String = if (settings.secure) "https" else "http"
+ private lazy val apiToken = readConfigVarFromFilesystem(settings.apiTokenPath, "api-token").getOrElse("")
+ private lazy val headers = if (settings.secure) immutable.Seq(Authorization(OAuth2BearerToken(apiToken))) else Nil
+
+ log.debug("kubernetes access namespace: {}. Secure: {}", namespace, settings.secure)
+
+ protected def createLeaseResource(name: String): Future[Option[LeaseResource]]
+
+ protected def getLeaseResource(name: String): Future[Option[LeaseResource]]
+
+ protected def pathForLease(name: String): Uri.Path
+
+ override def readOrCreateLeaseResource(name: String): Future[LeaseResource] = {
+ // TODO backoff retry
+ val maxTries = 5
+
+ def loop(tries: Int = 0): Future[LeaseResource] = {
+ log.debug("Trying to create lease {}", tries)
+ for {
+ olr <- getLeaseResource(name)
+ lr <- olr match {
+ case Some(found) =>
+ log.debug("{} already exists. Returning {}", name, found)
+ Future.successful(found)
+ case None =>
+ log.info("lease {} does not exist, creating", name)
+ createLeaseResource(name).flatMap {
+ case Some(created) => Future.successful(created)
+ case None =>
+ if (tries < maxTries) loop(tries + 1)
+ else Future.failed(new LeaseException(s"Unable to create or read lease after $maxTries tries"))
+ }
+ }
+ } yield lr
Review Comment:
This could be changed, but every line in AbstractKubernetesApiImpl is simply moved from the previous classes so I did no modification to existing code
--
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: notifications-unsubscribe@pekko.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@pekko.apache.org
For additional commands, e-mail: notifications-help@pekko.apache.org
Re: [PR] #217 Support native kubernetes leases [pekko-management]
Posted by "pjfanning (via GitHub)" <gi...@apache.org>.
pjfanning commented on code in PR #218:
URL: https://github.com/apache/pekko-management/pull/218#discussion_r1599008415
##########
lease-kubernetes/src/main/mima-filters/1.1.x.backwards.excludes/kubernetes-lease.backwards.excludes:
##########
@@ -0,0 +1,22 @@
+# 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.
+
+# The new Kubernetes Native Lease adds new classes and fields
+ProblemFilters.exclude[ReversedMissingMethodProblem]("org.apache.pekko.http.scaladsl.settings.Http2ServerSettings.frameTypeThrottleFrameTypes")
Review Comment:
good spot with the fix - I hadn't saved my edits in Visual Studio so my excludes came from another file that I was using as a template
--
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: notifications-unsubscribe@pekko.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@pekko.apache.org
For additional commands, e-mail: notifications-help@pekko.apache.org
Re: [PR] #217 Support native kubernetes leases [pekko-management]
Posted by "laglangyue (via GitHub)" <gi...@apache.org>.
laglangyue commented on code in PR #218:
URL: https://github.com/apache/pekko-management/pull/218#discussion_r1598035362
##########
lease-kubernetes/src/main/scala/org/apache/pekko/coordination/lease/kubernetes/AbstractKubernetesLease.scala:
##########
@@ -0,0 +1,123 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * license agreements; and to You under the Apache License, version 2.0:
+ *
+ * https://www.apache.org/licenses/LICENSE-2.0
+ *
+ * This file is part of the Apache Pekko project, which was derived from Akka.
+ */
+
+/*
+ * Copyright (C) 2017-2021 Lightbend Inc. <https://www.lightbend.com>
Review Comment:
If this is a new file, you need to use the Apache header instead of lightbend
--
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: notifications-unsubscribe@pekko.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@pekko.apache.org
For additional commands, e-mail: notifications-help@pekko.apache.org
Re: [PR] #217 Support native kubernetes leases [pekko-management]
Posted by "laglangyue (via GitHub)" <gi...@apache.org>.
laglangyue commented on code in PR #218:
URL: https://github.com/apache/pekko-management/pull/218#discussion_r1598051782
##########
lease-kubernetes/src/main/scala/org/apache/pekko/coordination/lease/kubernetes/internal/AbstractKubernetesApiImpl.scala:
##########
@@ -0,0 +1,191 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * license agreements; and to You under the Apache License, version 2.0:
+ *
+ * https://www.apache.org/licenses/LICENSE-2.0
+ *
+ * This file is part of the Apache Pekko project, which was derived from Akka.
+ */
+
+/*
+ * Copyright (C) 2017-2021 Lightbend Inc. <https://www.lightbend.com>
+ */
+
+package org.apache.pekko.coordination.lease.kubernetes.internal
+
+import org.apache.pekko
+import pekko.Done
+import pekko.actor.ActorSystem
+import pekko.annotation.InternalApi
+import pekko.coordination.lease.kubernetes.{ KubernetesApi, KubernetesSettings, LeaseResource }
+import pekko.coordination.lease.{ LeaseException, LeaseTimeoutException }
+import pekko.event.{ LogSource, Logging, LoggingAdapter }
+import pekko.http.scaladsl.model._
+import pekko.http.scaladsl.model.headers.{ Authorization, OAuth2BearerToken }
+import pekko.http.scaladsl.unmarshalling.Unmarshal
+import pekko.http.scaladsl.{ ConnectionContext, Http, HttpExt, HttpsConnectionContext }
+import pekko.pattern.after
+import pekko.pki.kubernetes.PemManagersProvider
+
+import java.nio.charset.StandardCharsets
+import java.nio.file.{ Files, Paths }
+import java.security.{ KeyStore, SecureRandom }
+import javax.net.ssl.{ KeyManager, KeyManagerFactory, SSLContext, TrustManager }
+import scala.collection.immutable
+import scala.concurrent.Future
+import scala.util.control.NonFatal
+
+/**
+ * Could be shared between leases: https://github.com/akka/akka-management/issues/680
+ * INTERNAL API
+ */
+@InternalApi private[pekko] abstract class AbstractKubernetesApiImpl(system: ActorSystem, settings: KubernetesSettings)
+ extends KubernetesApi
+ with KubernetesJsonSupport {
+
+ import system.dispatcher
+
+ protected implicit val sys: ActorSystem = system
+ protected val log: LoggingAdapter = Logging(system, getClass)(LogSource.fromClass)
+ private val http: HttpExt = Http()(system)
+
+ private lazy val sslContext: SSLContext = {
+ val certificates = PemManagersProvider.loadCertificates(settings.apiCaPath)
+ val factory = KeyManagerFactory.getInstance(KeyManagerFactory.getDefaultAlgorithm)
+ val keyStore = KeyStore.getInstance("PKCS12")
+ keyStore.load(null)
+ factory.init(keyStore, Array.empty)
+ val km: Array[KeyManager] = factory.getKeyManagers
+ val tm: Array[TrustManager] =
+ PemManagersProvider.buildTrustManagers(certificates)
+ val random: SecureRandom = new SecureRandom
+ val sslContext = SSLContext.getInstance("TLSv1.2")
+ sslContext.init(km, tm, random)
+ sslContext
+ }
+
+ private lazy val clientSslContext: HttpsConnectionContext = ConnectionContext.httpsClient(sslContext)
+
+ protected val namespace: String =
+ settings.namespace.orElse(readConfigVarFromFilesystem(settings.namespacePath, "namespace")).getOrElse("default")
+
+ protected val scheme: String = if (settings.secure) "https" else "http"
+ private lazy val apiToken = readConfigVarFromFilesystem(settings.apiTokenPath, "api-token").getOrElse("")
+ private lazy val headers = if (settings.secure) immutable.Seq(Authorization(OAuth2BearerToken(apiToken))) else Nil
+
+ log.debug("kubernetes access namespace: {}. Secure: {}", namespace, settings.secure)
+
+ protected def createLeaseResource(name: String): Future[Option[LeaseResource]]
+
+ protected def getLeaseResource(name: String): Future[Option[LeaseResource]]
+
+ protected def pathForLease(name: String): Uri.Path
+
+ override def readOrCreateLeaseResource(name: String): Future[LeaseResource] = {
+ // TODO backoff retry
+ val maxTries = 5
+
+ def loop(tries: Int = 0): Future[LeaseResource] = {
+ log.debug("Trying to create lease {}", tries)
+ for {
+ olr <- getLeaseResource(name)
+ lr <- olr match {
+ case Some(found) =>
+ log.debug("{} already exists. Returning {}", name, found)
+ Future.successful(found)
+ case None =>
+ log.info("lease {} does not exist, creating", name)
+ createLeaseResource(name).flatMap {
+ case Some(created) => Future.successful(created)
+ case None =>
+ if (tries < maxTries) loop(tries + 1)
+ else Future.failed(new LeaseException(s"Unable to create or read lease after $maxTries tries"))
+ }
+ }
+ } yield lr
Review Comment:
I don't really like the style here, is it necessary to use for yield 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: notifications-unsubscribe@pekko.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@pekko.apache.org
For additional commands, e-mail: notifications-help@pekko.apache.org
Re: [PR] #217 Support native kubernetes leases [pekko-management]
Posted by "pjfanning (via GitHub)" <gi...@apache.org>.
pjfanning commented on code in PR #218:
URL: https://github.com/apache/pekko-management/pull/218#discussion_r1589800542
##########
lease-kubernetes/src/test/scala/org/apache/pekko/coordination/lease/kubernetes/NativeKubernetesApiSpec.scala:
##########
@@ -0,0 +1,282 @@
+/*
+ * 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.pekko.coordination.lease.kubernetes
+
+import com.github.tomakehurst.wiremock.WireMockServer
+import com.github.tomakehurst.wiremock.client.WireMock
+import com.github.tomakehurst.wiremock.client.WireMock._
+import com.github.tomakehurst.wiremock.core.WireMockConfiguration.wireMockConfig
+import com.typesafe.config.ConfigFactory
+import org.apache.pekko.Done
Review Comment:
could you check existing Scala source files to see how Pekko classes are imported
##########
lease-kubernetes/src/test/scala/org/apache/pekko/coordination/lease/kubernetes/NativeKubernetesApiSpec.scala:
##########
@@ -0,0 +1,282 @@
+/*
+ * 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.pekko.coordination.lease.kubernetes
+
+import com.github.tomakehurst.wiremock.WireMockServer
+import com.github.tomakehurst.wiremock.client.WireMock
+import com.github.tomakehurst.wiremock.client.WireMock._
+import com.github.tomakehurst.wiremock.core.WireMockConfiguration.wireMockConfig
+import com.typesafe.config.ConfigFactory
+import org.apache.pekko.Done
Review Comment:
could you check existing Scala source files to see how Pekko classes are imported?
--
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: notifications-unsubscribe@pekko.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@pekko.apache.org
For additional commands, e-mail: notifications-help@pekko.apache.org
Re: [PR] #217 Support native kubernetes leases [pekko-management]
Posted by "Iilun (via GitHub)" <gi...@apache.org>.
Iilun commented on code in PR #218:
URL: https://github.com/apache/pekko-management/pull/218#discussion_r1589936128
##########
lease-kubernetes/src/main/scala/org/apache/pekko/coordination/lease/kubernetes/internal/NativeKubernetesApiImpl.scala:
##########
@@ -0,0 +1,195 @@
+/*
+ * 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.pekko.coordination.lease.kubernetes.internal
+
+import org.apache.pekko.actor.ActorSystem
Review Comment:
Sorry I hadn't realized that, now fixed.
--
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: notifications-unsubscribe@pekko.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@pekko.apache.org
For additional commands, e-mail: notifications-help@pekko.apache.org
Re: [PR] #217 Support native kubernetes leases [pekko-management]
Posted by "pjfanning (via GitHub)" <gi...@apache.org>.
pjfanning commented on code in PR #218:
URL: https://github.com/apache/pekko-management/pull/218#discussion_r1589780917
##########
lease-kubernetes-int-test/src/main/scala/org/apache/pekko/coordination/lease/kubernetes/LeaseSpec.scala:
##########
@@ -16,7 +16,7 @@ package org.apache.pekko.coordination.lease.kubernetes
import org.apache.pekko
import pekko.actor.ActorSystem
import pekko.coordination.lease.TimeoutSettings
-import pekko.coordination.lease.kubernetes.internal.KubernetesApiImpl
+import pekko.coordination.lease.kubernetes.internal.CRDKubernetesApiImpl
Review Comment:
it's ok to introduce new shared classes to inherit from but the existing classes should still be binary compatible with the pekko-management 1.0.x code
Have you tried running the mima check to see what failures you get? `sbt mimaReportBinaryIssues`
We would ideally have minimal issues with those checks. We can exclude some failures - eg additions but removed classes, functions or accessible fields - that would only be acceptable in rare cases.
--
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: notifications-unsubscribe@pekko.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@pekko.apache.org
For additional commands, e-mail: notifications-help@pekko.apache.org
Re: [PR] #217 Support native kubernetes leases [pekko-management]
Posted by "Iilun (via GitHub)" <gi...@apache.org>.
Iilun commented on code in PR #218:
URL: https://github.com/apache/pekko-management/pull/218#discussion_r1589774579
##########
docs/src/main/paradox/kubernetes-lease.md:
##########
@@ -30,9 +34,7 @@ different `ActorSystem` names because they all need a separate lease.
- For @extref:[Cluster Singleton](pekko:typed/cluster-singleton.html#lease) there will be one per singleton.
- For @extref:[Cluster Sharding](pekko:typed/cluster-sharding.html#lease), there will be one per shard per type.
-### Configuring
Review Comment:
I reverted all header changes
--
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: notifications-unsubscribe@pekko.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@pekko.apache.org
For additional commands, e-mail: notifications-help@pekko.apache.org
Re: [PR] #217 Support native kubernetes leases [pekko-management]
Posted by "Iilun (via GitHub)" <gi...@apache.org>.
Iilun commented on code in PR #218:
URL: https://github.com/apache/pekko-management/pull/218#discussion_r1598350597
##########
lease-kubernetes/src/main/scala/org/apache/pekko/coordination/lease/kubernetes/internal/AbstractKubernetesApiImpl.scala:
##########
@@ -0,0 +1,191 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * license agreements; and to You under the Apache License, version 2.0:
+ *
+ * https://www.apache.org/licenses/LICENSE-2.0
+ *
+ * This file is part of the Apache Pekko project, which was derived from Akka.
+ */
+
+/*
+ * Copyright (C) 2017-2021 Lightbend Inc. <https://www.lightbend.com>
+ */
+
+package org.apache.pekko.coordination.lease.kubernetes.internal
+
+import org.apache.pekko
+import pekko.Done
+import pekko.actor.ActorSystem
+import pekko.annotation.InternalApi
+import pekko.coordination.lease.kubernetes.{ KubernetesApi, KubernetesSettings, LeaseResource }
+import pekko.coordination.lease.{ LeaseException, LeaseTimeoutException }
+import pekko.event.{ LogSource, Logging, LoggingAdapter }
+import pekko.http.scaladsl.model._
+import pekko.http.scaladsl.model.headers.{ Authorization, OAuth2BearerToken }
+import pekko.http.scaladsl.unmarshalling.Unmarshal
+import pekko.http.scaladsl.{ ConnectionContext, Http, HttpExt, HttpsConnectionContext }
+import pekko.pattern.after
+import pekko.pki.kubernetes.PemManagersProvider
+
+import java.nio.charset.StandardCharsets
+import java.nio.file.{ Files, Paths }
+import java.security.{ KeyStore, SecureRandom }
+import javax.net.ssl.{ KeyManager, KeyManagerFactory, SSLContext, TrustManager }
+import scala.collection.immutable
+import scala.concurrent.Future
+import scala.util.control.NonFatal
+
+/**
+ * Could be shared between leases: https://github.com/akka/akka-management/issues/680
+ * INTERNAL API
+ */
+@InternalApi private[pekko] abstract class AbstractKubernetesApiImpl(system: ActorSystem, settings: KubernetesSettings)
+ extends KubernetesApi
+ with KubernetesJsonSupport {
+
+ import system.dispatcher
+
+ protected implicit val sys: ActorSystem = system
+ protected val log: LoggingAdapter = Logging(system, getClass)(LogSource.fromClass)
+ private val http: HttpExt = Http()(system)
+
+ private lazy val sslContext: SSLContext = {
+ val certificates = PemManagersProvider.loadCertificates(settings.apiCaPath)
+ val factory = KeyManagerFactory.getInstance(KeyManagerFactory.getDefaultAlgorithm)
+ val keyStore = KeyStore.getInstance("PKCS12")
+ keyStore.load(null)
+ factory.init(keyStore, Array.empty)
+ val km: Array[KeyManager] = factory.getKeyManagers
+ val tm: Array[TrustManager] =
+ PemManagersProvider.buildTrustManagers(certificates)
+ val random: SecureRandom = new SecureRandom
+ val sslContext = SSLContext.getInstance("TLSv1.2")
+ sslContext.init(km, tm, random)
+ sslContext
+ }
+
+ private lazy val clientSslContext: HttpsConnectionContext = ConnectionContext.httpsClient(sslContext)
+
+ protected val namespace: String =
+ settings.namespace.orElse(readConfigVarFromFilesystem(settings.namespacePath, "namespace")).getOrElse("default")
+
+ protected val scheme: String = if (settings.secure) "https" else "http"
+ private lazy val apiToken = readConfigVarFromFilesystem(settings.apiTokenPath, "api-token").getOrElse("")
+ private lazy val headers = if (settings.secure) immutable.Seq(Authorization(OAuth2BearerToken(apiToken))) else Nil
+
+ log.debug("kubernetes access namespace: {}. Secure: {}", namespace, settings.secure)
+
+ protected def createLeaseResource(name: String): Future[Option[LeaseResource]]
+
+ protected def getLeaseResource(name: String): Future[Option[LeaseResource]]
+
+ protected def pathForLease(name: String): Uri.Path
+
+ override def readOrCreateLeaseResource(name: String): Future[LeaseResource] = {
+ // TODO backoff retry
+ val maxTries = 5
Review Comment:
This is simply moved code, this could be interesting but I don't think it is related to this PR
--
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: notifications-unsubscribe@pekko.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@pekko.apache.org
For additional commands, e-mail: notifications-help@pekko.apache.org
Re: [PR] #217 Support native kubernetes leases [pekko-management]
Posted by "Iilun (via GitHub)" <gi...@apache.org>.
Iilun commented on PR #218:
URL: https://github.com/apache/pekko-management/pull/218#issuecomment-2108687153
Yes thank you !
--
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: notifications-unsubscribe@pekko.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@pekko.apache.org
For additional commands, e-mail: notifications-help@pekko.apache.org
Re: [PR] #217 Support native kubernetes leases [pekko-management]
Posted by "pjfanning (via GitHub)" <gi...@apache.org>.
pjfanning commented on code in PR #218:
URL: https://github.com/apache/pekko-management/pull/218#discussion_r1598330917
##########
lease-kubernetes/src/main/scala/org/apache/pekko/coordination/lease/kubernetes/internal/AbstractKubernetesApiImpl.scala:
##########
@@ -0,0 +1,191 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * license agreements; and to You under the Apache License, version 2.0:
+ *
+ * https://www.apache.org/licenses/LICENSE-2.0
+ *
+ * This file is part of the Apache Pekko project, which was derived from Akka.
+ */
+
+/*
+ * Copyright (C) 2017-2021 Lightbend Inc. <https://www.lightbend.com>
+ */
+
+package org.apache.pekko.coordination.lease.kubernetes.internal
+
+import org.apache.pekko
+import pekko.Done
+import pekko.actor.ActorSystem
+import pekko.annotation.InternalApi
+import pekko.coordination.lease.kubernetes.{ KubernetesApi, KubernetesSettings, LeaseResource }
+import pekko.coordination.lease.{ LeaseException, LeaseTimeoutException }
+import pekko.event.{ LogSource, Logging, LoggingAdapter }
+import pekko.http.scaladsl.model._
+import pekko.http.scaladsl.model.headers.{ Authorization, OAuth2BearerToken }
+import pekko.http.scaladsl.unmarshalling.Unmarshal
+import pekko.http.scaladsl.{ ConnectionContext, Http, HttpExt, HttpsConnectionContext }
+import pekko.pattern.after
+import pekko.pki.kubernetes.PemManagersProvider
+
+import java.nio.charset.StandardCharsets
+import java.nio.file.{ Files, Paths }
+import java.security.{ KeyStore, SecureRandom }
+import javax.net.ssl.{ KeyManager, KeyManagerFactory, SSLContext, TrustManager }
+import scala.collection.immutable
+import scala.concurrent.Future
+import scala.util.control.NonFatal
+
+/**
+ * Could be shared between leases: https://github.com/akka/akka-management/issues/680
+ * INTERNAL API
+ */
+@InternalApi private[pekko] abstract class AbstractKubernetesApiImpl(system: ActorSystem, settings: KubernetesSettings)
+ extends KubernetesApi
+ with KubernetesJsonSupport {
+
+ import system.dispatcher
+
+ protected implicit val sys: ActorSystem = system
+ protected val log: LoggingAdapter = Logging(system, getClass)(LogSource.fromClass)
+ private val http: HttpExt = Http()(system)
+
+ private lazy val sslContext: SSLContext = {
+ val certificates = PemManagersProvider.loadCertificates(settings.apiCaPath)
+ val factory = KeyManagerFactory.getInstance(KeyManagerFactory.getDefaultAlgorithm)
+ val keyStore = KeyStore.getInstance("PKCS12")
+ keyStore.load(null)
+ factory.init(keyStore, Array.empty)
+ val km: Array[KeyManager] = factory.getKeyManagers
+ val tm: Array[TrustManager] =
+ PemManagersProvider.buildTrustManagers(certificates)
+ val random: SecureRandom = new SecureRandom
+ val sslContext = SSLContext.getInstance("TLSv1.2")
+ sslContext.init(km, tm, random)
+ sslContext
+ }
+
+ private lazy val clientSslContext: HttpsConnectionContext = ConnectionContext.httpsClient(sslContext)
+
+ protected val namespace: String =
+ settings.namespace.orElse(readConfigVarFromFilesystem(settings.namespacePath, "namespace")).getOrElse("default")
+
+ protected val scheme: String = if (settings.secure) "https" else "http"
+ private lazy val apiToken = readConfigVarFromFilesystem(settings.apiTokenPath, "api-token").getOrElse("")
+ private lazy val headers = if (settings.secure) immutable.Seq(Authorization(OAuth2BearerToken(apiToken))) else Nil
+
+ log.debug("kubernetes access namespace: {}. Secure: {}", namespace, settings.secure)
+
+ protected def createLeaseResource(name: String): Future[Option[LeaseResource]]
+
+ protected def getLeaseResource(name: String): Future[Option[LeaseResource]]
+
+ protected def pathForLease(name: String): Uri.Path
+
+ override def readOrCreateLeaseResource(name: String): Future[LeaseResource] = {
+ // TODO backoff retry
+ val maxTries = 5
+
+ def loop(tries: Int = 0): Future[LeaseResource] = {
+ log.debug("Trying to create lease {}", tries)
+ for {
+ olr <- getLeaseResource(name)
+ lr <- olr match {
+ case Some(found) =>
+ log.debug("{} already exists. Returning {}", name, found)
+ Future.successful(found)
+ case None =>
+ log.info("lease {} does not exist, creating", name)
+ createLeaseResource(name).flatMap {
+ case Some(created) => Future.successful(created)
+ case None =>
+ if (tries < maxTries) loop(tries + 1)
+ else Future.failed(new LeaseException(s"Unable to create or read lease after $maxTries tries"))
+ }
+ }
+ } yield lr
Review Comment:
In general, it is not a great idea to modify existing code just to change its style. There are risks with breaking the code and unless the style is really bad - the risks are not worth 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: notifications-unsubscribe@pekko.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@pekko.apache.org
For additional commands, e-mail: notifications-help@pekko.apache.org