Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat: added construct for application ingress #79

Merged
merged 1 commit into from
May 30, 2024

Conversation

nemanja-kovacevic-thinkit
Copy link
Contributor

@nemanja-kovacevic-thinkit nemanja-kovacevic-thinkit commented May 24, 2024

ticket: https://techfromsage.atlassian.net/browse/PLT-651

This PR creates a new cdk8s construct for alb-application-ingress which contains all the logic and annotations required to allow sharing an ingress for a service and migrating traffic using weighting between namespaces.

@nemanja-kovacevic-thinkit nemanja-kovacevic-thinkit added the 30% 30% Code Review label May 24, 2024
@nemanja-kovacevic-thinkit nemanja-kovacevic-thinkit requested a review from a team May 24, 2024 12:03
@nemanja-kovacevic-thinkit nemanja-kovacevic-thinkit force-pushed the PLT-651-alb-application-ingress branch from 500c5d5 to e5e60cb Compare May 24, 2024 12:05
@nemanja-kovacevic-thinkit nemanja-kovacevic-thinkit changed the title PLT-651: added ingress construct feat: added construct for application ingress May 24, 2024
Comment on lines 15 to 20
/**
* Custom selector labels, they will be merged with the default app, role, and instance.
* They will be applied to the workload, the pod and the service.
* @default { app: "<app label from chart>", role: "server", instance: "<construct id>" }
*/
readonly selectorLabels?: { [key: string]: string };
Copy link
Member

Choose a reason for hiding this comment

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

In other constructs, we call this selectorLabels because we use these labels for spec.selector.matchLabels. In case of Ingress, there is no such concept. But as we may still want to be able to add/override labels, I'd just call this - labels

Suggested change
/**
* Custom selector labels, they will be merged with the default app, role, and instance.
* They will be applied to the workload, the pod and the service.
* @default { app: "<app label from chart>", role: "server", instance: "<construct id>" }
*/
readonly selectorLabels?: { [key: string]: string };
/**
* Custom labels, they will be merged with the default app, role, and instance.
* @default { app: "<app label from chart>", role: "server", instance: "<construct id>" }
*/
readonly labels?: { [key: string]: string };

Also, I think we'll need a separate props for app, and instance labels. Currently, the Ingress objects are created as part of the app's chart, but this construct will live in a separate chart that will be deployed into another namespace.

What we want to do here, is mirror the labels of the application's Pods, and the Service that exposes those Pods, labels.

For example, we have the following Service deployed to Persona's namespace:

apiVersion: v1
kind: Service
metadata:
  name: api-service
  namespace: persona-220728
  labels:
    app: persona
    canary: "false"
    environment: production
    instance: api
    managed-by: cdk8s
    region: eu
    role: server
    service: persona-eu

So in our Ingress chart, we want to be able to define an object that clearly points to the same thing:

new Ingress(this, `persona-api`, {
  app: "persona",     // <- we could create Ingresses for multiple apps in the same chart
  instance: "api",    // <- different from construct id above
  labels: {
    canary: "false",  // <- additional label for this case
  },
  hostnames: ["https://users.talis.com"],
  serviceRouting: [
    {
      namespace: "persona-220728",
      name: "api-service",
      port: 80,
      weight: 100,
    }
  ],
});

The service label can be created from app, environment, and region (we can skip watermark here):
https://github.com/talis/talis-cdk8s-constructs/blob/v4.8.0/lib/talis-chart/talis-chart.ts#L60

The end result should be an Ingress + external Service, created in the ingress namespace, with all the labels matching the Service shown above.

Comment on lines 22 to 34
/**
* Domain name for TLS certificate discovery.
* @see https://kubernetes-sigs.github.io/aws-load-balancer-controller/v2.3/guide/ingress/cert_discovery/
*/
readonly tlsDomain?: string | string[];
Copy link
Member

Choose a reason for hiding this comment

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

I wonder if migration to this Ingress is also an opportunity to move away from certificate discovery, and just use cert ARNs. So maybe we should instead have a property for certificateArn

Suggested change
/**
* Domain name for TLS certificate discovery.
* @see https://kubernetes-sigs.github.io/aws-load-balancer-controller/v2.3/guide/ingress/cert_discovery/
*/
readonly tlsDomain?: string | string[];
/**
* ARN of one or more certificate managed by AWS Certificate Manager
*/
readonly certificateArn?: string | string[];

Copy link
Contributor

Choose a reason for hiding this comment

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

Agree with this - makes moving certificates around a lot easier

Copy link
Member

Choose a reason for hiding this comment

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

Ditto, makes sense!

Comment on lines 141 to 170
function convertToJsonContent(obj: unknown): string {
return JSON.stringify(obj);
}
Copy link
Member

Choose a reason for hiding this comment

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

You can import the helpers already present in the library:
https://github.com/talis/talis-cdk8s-constructs/blob/526214c97af149ef673a3e957da4220d59f0fb76/lib/web-service/annotation-util.ts#L85-L92

You may move annotation-util.ts into lib/common/ directory.

@nemanja-kovacevic-thinkit nemanja-kovacevic-thinkit force-pushed the PLT-651-alb-application-ingress branch 2 times, most recently from d455c8d to a7606be Compare May 28, 2024 08:20
/**
* Ingress class name.
*/
readonly ingressClassName: string;
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
readonly ingressClassName: string;
readonly ingressClassName: string;
/**
* Ingress class priority, between -1000 and 1000
*/
readonly ingressClassPriority: number;

This determines the relative order in the ingressClass of each ingress rules - should default to 0

@nemanja-kovacevic-thinkit nemanja-kovacevic-thinkit force-pushed the PLT-651-alb-application-ingress branch 3 times, most recently from ecf9345 to bf32e0d Compare May 29, 2024 14:15
@nemanja-kovacevic-thinkit nemanja-kovacevic-thinkit added 90% 90% Code Review and removed 30% 30% Code Review labels May 29, 2024
@nemanja-kovacevic-thinkit nemanja-kovacevic-thinkit force-pushed the PLT-651-alb-application-ingress branch from bf32e0d to 78e4011 Compare May 29, 2024 14:36
Comment on lines 143 to 167
if (props.ingressClassPriority)
if (
props.ingressClassPriority < -1000 ||
props.ingressClassPriority > 1000
)
throw new Error(
"Ingress class priority has to be between -1000 and 1000",
);

if (props.hostnames.length < 1)
throw new Error("At least one hostname has to be defined.");

if (props.serviceRouting.length < 1)
throw new Error("At least one service route has to be defined.");

const totalWeight = props.serviceRouting.reduce(
(sum, serviceRoute) => sum + serviceRoute.weight,
0,
);
if (totalWeight != 100)
throw new Error("Total service routing weigth must be 100.");
Copy link
Contributor

Choose a reason for hiding this comment

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

For consistency these if blocks should use { ... } syntax for the blocks

@nemanja-kovacevic-thinkit nemanja-kovacevic-thinkit force-pushed the PLT-651-alb-application-ingress branch from 78e4011 to 4d2cecd Compare May 29, 2024 15:12
@nemanja-kovacevic-thinkit nemanja-kovacevic-thinkit merged commit 094bb66 into main May 30, 2024
8 checks passed
@nemanja-kovacevic-thinkit nemanja-kovacevic-thinkit deleted the PLT-651-alb-application-ingress branch May 30, 2024 07:57
@talisaspire
Copy link
Collaborator

🎉 This PR is included in version 4.9.0 🎉

The release is available on GitHub release

Your semantic-release bot 📦🚀

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
90% 90% Code Review released
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants